diff mbox series

[kvm-unit-tests,v2,10/10] travis.yml: Add x86 build with clang 10

Message ID 20200901085056.33391-11-r.bolshakov@yadro.com (mailing list archive)
State New, archived
Headers show
Series Add support for generic ELF cross-compiler | expand

Commit Message

Roman Bolshakov Sept. 1, 2020, 8:50 a.m. UTC
.gitlab-ci.yml already has a job to build the tests with clang but it's
not clear how to set it up on a personal github repo.

NB, realmode test is disabled because it fails immediately after start
if compiled with clang-10.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 .travis.yml | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Thomas Huth Sept. 4, 2020, 2:31 p.m. UTC | #1
On 01/09/2020 10.50, Roman Bolshakov wrote:
> .gitlab-ci.yml already has a job to build the tests with clang but it's
> not clear how to set it up on a personal github repo.

You can't use gitlab-ci from a github repo, it's a separate git forge
system.

> NB, realmode test is disabled because it fails immediately after start
> if compiled with clang-10.
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  .travis.yml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/.travis.yml b/.travis.yml
> index f3a8899..ae4ed08 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -17,6 +17,16 @@ jobs:
>                 kvmclock_test msr pcid rdpru realmode rmap_chain s3 setjmp umip"
>        - ACCEL="kvm"
>  
> +    - addons:
> +        apt_packages: clang-10 qemu-system-x86
> +      env:
> +      - CONFIG="--cc=clang-10"
> +      - BUILD_DIR="."
> +      - TESTS="access asyncpf debug emulator ept hypercall hyperv_stimer
> +               hyperv_synic idt_test intel_iommu ioapic ioapic-split
> +               kvmclock_test msr pcid rdpru rmap_chain s3 setjmp umip"
> +      - ACCEL="kvm"

We already have two jobs for compiling on x86, one for testing in-tree
builds and one for testing out-of-tree builds ... I wonder whether we
should simply switch one of those two jobs to use clang-10 instead of
gcc (since the in/out-of-tree stuff should be hopefully independent of
the compiler type)? Since Travis limits the amount of jobs that run at
the same time, that would not increase the total testing time, I think.

 Thomas


PS: Maybe we could update from bionic to focal now, too, and see whether
some more tests are working with the newer version of QEMU there...
Roman Bolshakov Sept. 14, 2020, 2:45 p.m. UTC | #2
On Fri, Sep 04, 2020 at 04:31:03PM +0200, Thomas Huth wrote:
> On 01/09/2020 10.50, Roman Bolshakov wrote:
> > .gitlab-ci.yml already has a job to build the tests with clang but it's
> > not clear how to set it up on a personal github repo.
> 
> You can't use gitlab-ci from a github repo, it's a separate git forge
> system.
> 
> > NB, realmode test is disabled because it fails immediately after start
> > if compiled with clang-10.
> > 
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> >  .travis.yml | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/.travis.yml b/.travis.yml
> > index f3a8899..ae4ed08 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -17,6 +17,16 @@ jobs:
> >                 kvmclock_test msr pcid rdpru realmode rmap_chain s3 setjmp umip"
> >        - ACCEL="kvm"
> >  
> > +    - addons:
> > +        apt_packages: clang-10 qemu-system-x86
> > +      env:
> > +      - CONFIG="--cc=clang-10"
> > +      - BUILD_DIR="."
> > +      - TESTS="access asyncpf debug emulator ept hypercall hyperv_stimer
> > +               hyperv_synic idt_test intel_iommu ioapic ioapic-split
> > +               kvmclock_test msr pcid rdpru rmap_chain s3 setjmp umip"
> > +      - ACCEL="kvm"
> 
> We already have two jobs for compiling on x86, one for testing in-tree
> builds and one for testing out-of-tree builds ... I wonder whether we
> should simply switch one of those two jobs to use clang-10 instead of
> gcc (since the in/out-of-tree stuff should be hopefully independent of
> the compiler type)? Since Travis limits the amount of jobs that run at
> the same time, that would not increase the total testing time, I think.
> 

Hi Thomas,

sure, that works for me.

>  Thomas
> 
> 
> PS: Maybe we could update from bionic to focal now, too, and see whether
> some more tests are working with the newer version of QEMU there...
> 

no problem, here're results for focal/kvm on IBM x3500 M3 (Nehalem) if
the tests are built with clang:

PASS apic-split (53 tests)
PASS ioapic-split (19 tests)
PASS apic (53 tests)
PASS ioapic (26 tests)
SKIP cmpxchg8b (i386 only)
PASS smptest (1 tests)
PASS smptest3 (1 tests)
PASS vmexit_cpuid
PASS vmexit_vmcall
PASS vmexit_mov_from_cr8
PASS vmexit_mov_to_cr8
PASS vmexit_inl_pmtimer
PASS vmexit_ipi
PASS vmexit_ipi_halt
PASS vmexit_ple_round_robin
PASS vmexit_tscdeadline
PASS vmexit_tscdeadline_immed
PASS access
SKIP smap (0 tests)
SKIP pku (0 tests)
PASS asyncpf (1 tests)
PASS emulator (125 tests, 2 skipped)
PASS eventinj (13 tests)
PASS hypercall (2 tests)
PASS idt_test (4 tests)
PASS memory (8 tests)
PASS msr (12 tests)
SKIP pmu (/proc/sys/kernel/nmi_watchdog not equal to 0)
SKIP vmware_backdoors (/sys/module/kvm/parameters/enable_vmware_backdoor
not equal to Y)
PASS port80
FAIL realmode
PASS s3
PASS setjmp (10 tests)
PASS sieve
PASS syscall (2 tests)
PASS tsc (3 tests)
PASS tsc_adjust (5 tests)
PASS xsave (4 tests)
PASS rmap_chain
SKIP svm (0 tests)
SKIP taskswitch (i386 only)
SKIP taskswitch2 (i386 only)
PASS kvmclock_test
PASS pcid (3 tests)
PASS pcid-disabled (3 tests)
PASS rdpru (1 tests)
PASS umip (21 tests)
FAIL vmx
PASS ept (8636 tests)
SKIP vmx_eoi_bitmap_ioapic_scan (1 tests, 1 skipped)
SKIP vmx_hlt_with_rvi_test (1 tests, 1 skipped)
SKIP vmx_apicv_test (2 tests, 2 skipped)
PASS vmx_apic_passthrough_thread (8 tests)
FAIL vmx_init_signal_test (8 tests, 1 unexpected failures)
FAIL vmx_apic_passthrough_tpr_threshold_test (timeout; duration=10)
PASS vmx_vmcs_shadow_test (142218 tests)
PASS debug (11 tests)
PASS hyperv_synic (1 tests)
PASS hyperv_connections (7 tests)
PASS hyperv_stimer (12 tests)
PASS hyperv_clock
PASS intel_iommu (11 tests)
PASS tsx-ctrl

Here are results for the same server if tests are built with gcc:
SureSS apic-split (53 tests)
PASS ioapic-split (19 tests)
PASS apic (53 tests)
PASS ioapic (26 tests)
SKIP cmpxchg8b (i386 only)
PASS smptest (1 tests)
PASS smptest3 (1 tests)
PASS vmexit_cpuid
PASS vmexit_vmcall
PASS vmexit_mov_from_cr8
PASS vmexit_mov_to_cr8
PASS vmexit_inl_pmtimer
PASS vmexit_ipi
PASS vmexit_ipi_halt
PASS vmexit_ple_round_robin
PASS vmexit_tscdeadline
PASS vmexit_tscdeadline_immed
PASS access
SKIP smap (0 tests)
SKIP pku (0 tests)
PASS asyncpf (1 tests)
PASS emulator (125 tests, 2 skipped)
PASS eventinj (13 tests)
PASS hypercall (2 tests)
PASS idt_test (4 tests)
PASS memory (8 tests)
PASS msr (12 tests)
SKIP pmu (/proc/sys/kernel/nmi_watchdog not equal to 0)
SKIP vmware_backdoors (/sys/module/kvm/parameters/enable_vmware_backdoor
not equal to Y)
PASS port80
PASS realmode
PASS s3
PASS setjmp (10 tests)
PASS sieve
PASS syscall (2 tests)
PASS tsc (3 tests)
PASS tsc_adjust (5 tests)
PASS xsave (4 tests)
PASS rmap_chain
SKIP svm (0 tests)
SKIP taskswitch (i386 only)
SKIP taskswitch2 (i386 only)
PASS kvmclock_test
PASS pcid (3 tests)
PASS pcid-disabled (3 tests)
PASS rdpru (1 tests)
PASS umip (21 tests)
FAIL vmx
PASS ept (8636 tests)
SKIP vmx_eoi_bitmap_ioapic_scan (1 tests, 1 skipped)
SKIP vmx_hlt_with_rvi_test (1 tests, 1 skipped)
SKIP vmx_apicv_test (2 tests, 2 skipped)
PASS vmx_apic_passthrough_thread (8 tests)
FAIL vmx_init_signal_test (8 tests, 1 unexpected failures)
FAIL vmx_apic_passthrough_tpr_threshold_test (timeout; duration=10)
PASS vmx_vmcs_shadow_test (142218 tests)
PASS debug (11 tests)
PASS hyperv_synic (1 tests)
PASS hyperv_connections (7 tests)
PASS hyperv_stimer (12 tests)
PASS hyperv_clock
PASS intel_iommu (11 tests)
PASS tsx-ctrl

The difference is only realmode test which doesn't work if built by
clang.

Thanks,
Roman
Thomas Huth Sept. 14, 2020, 4:37 p.m. UTC | #3
On 14/09/2020 16.45, Roman Bolshakov wrote:
> On Fri, Sep 04, 2020 at 04:31:03PM +0200, Thomas Huth wrote:
>> On 01/09/2020 10.50, Roman Bolshakov wrote:
>>> .gitlab-ci.yml already has a job to build the tests with clang but it's
>>> not clear how to set it up on a personal github repo.
>>
>> You can't use gitlab-ci from a github repo, it's a separate git forge
>> system.
>>
>>> NB, realmode test is disabled because it fails immediately after start
>>> if compiled with clang-10.
>>>
>>> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>>> ---
>>>  .travis.yml | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/.travis.yml b/.travis.yml
>>> index f3a8899..ae4ed08 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -17,6 +17,16 @@ jobs:
>>>                 kvmclock_test msr pcid rdpru realmode rmap_chain s3 setjmp umip"
>>>        - ACCEL="kvm"
>>>  
>>> +    - addons:
>>> +        apt_packages: clang-10 qemu-system-x86
>>> +      env:
>>> +      - CONFIG="--cc=clang-10"
>>> +      - BUILD_DIR="."
>>> +      - TESTS="access asyncpf debug emulator ept hypercall hyperv_stimer
>>> +               hyperv_synic idt_test intel_iommu ioapic ioapic-split
>>> +               kvmclock_test msr pcid rdpru rmap_chain s3 setjmp umip"
>>> +      - ACCEL="kvm"
>>
>> We already have two jobs for compiling on x86, one for testing in-tree
>> builds and one for testing out-of-tree builds ... I wonder whether we
>> should simply switch one of those two jobs to use clang-10 instead of
>> gcc (since the in/out-of-tree stuff should be hopefully independent of
>> the compiler type)? Since Travis limits the amount of jobs that run at
>> the same time, that would not increase the total testing time, I think.
>>
> 
> Hi Thomas,
> 
> sure, that works for me.
> 
>>  Thomas
>>
>>
>> PS: Maybe we could update from bionic to focal now, too, and see whether
>> some more tests are working with the newer version of QEMU there...
>>
> 
> no problem, here're results for focal/kvm on IBM x3500 M3 (Nehalem) if
> the tests are built with clang:
[...]

Thanks for checking, that looks promising!

> The difference is only realmode test which doesn't work if built by
> clang.

Hmm, if you got some spare minutes, could you check if it works when
replacing the asm() statements there with asm volatile() ?
(Otherwise I'll check it if I got some spare time again ... so likely
not this week ;-))

 Thomas
Roman Bolshakov Sept. 15, 2020, 3:59 p.m. UTC | #4
On Mon, Sep 14, 2020 at 06:37:33PM +0200, Thomas Huth wrote:
> On 14/09/2020 16.45, Roman Bolshakov wrote:
> > The difference is only realmode test which doesn't work if built by
> > clang.
> 
> Hmm, if you got some spare minutes, could you check if it works when
> replacing the asm() statements there with asm volatile() ?
> (Otherwise I'll check it if I got some spare time again ... so likely
> not this week ;-))
> 

Hi Thomas,

Sure. There are only two places where volatile is missed (inside functions) and
would make sense to add it. Unfortunately it doesn't help much:

diff --git a/x86/realmode.c b/x86/realmode.c
index 7c2d776..30691bc 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -271,7 +271,7 @@ static void report(const char *name, u16 regs_ignore, _Bool ok)
 }

 #define MK_INSN(name, str)                             \
-    asm (                                              \
+    asm volatile (                                             \
         ".pushsection .data.insn  \n\t"                \
         "insn_" #name ": \n\t"                         \
         ".word 1001f, 1002f - 1001f \n\t"              \
@@ -1448,7 +1448,7 @@ static void test_cpuid(void)

     eax = inregs.eax;
     ecx = inregs.ecx;
-    asm("cpuid" : "+a"(eax), "=b"(ebx), "+c"(ecx), "=d"(edx));
+    asm volatile("cpuid" : "+a"(eax), "=b"(ebx), "+c"(ecx), "=d"(edx));
     exec_in_big_real_mode(&insn_cpuid);
     report("cpuid", R_AX|R_BX|R_CX|R_DX,
           outregs.eax == eax && outregs.ebx == ebx




So, I looked further and noticed that multiboot header is missed in the flat
binary:
        ".section .init \n\t"

        ".code32 \n\t"

        "mb_magic = 0x1BADB002 \n\t"
        "mb_flags = 0x0 \n\t"

        "# multiboot header \n\t"
        ".long mb_magic, mb_flags, 0 - (mb_magic + mb_flags) \n\t"


But I can see it in the object file. So, I believe linker didn't place .init
section at 0x1000 despite realmode.lds instruction:

SECTIONS
{
    . = 16K;
    stext = .;
    .text : { *(.init) *(.text) }
    . = ALIGN(4K);
    .data : { *(.data) *(.rodata*) }
    . = ALIGN(16);
    .bss : { *(.bss) }
    edata = .;
}
ENTRY(start)


If I read it correctly, given the segment in realmode.elf:
Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x001000 0x00004000 0x00004000 0x03c90 0x03c90 R E 0x1000

Here's multiboot header in GCC-compiled binary:
00001000: 02b0ad1b 00000000 fe4f52e4 66b83412  .........OR.f.4.

Here's the same location in clang-compiled binary:
00001000: 04000000 14000000 03000000 474e5500  ............GNU.

Here's verbose invocation of linker by clang:

$ clang-10 -v -m32 -nostdlib -o x86/realmode.elf -Wl,-m,elf_i386 -Wl,-T,/home/roolebo
/dev/kvm-unit-tests/x86/realmode.lds x86/realmode.o
clang version 10.0.0-4ubuntu1
Target: i386-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/9
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/9
 "/usr/bin/ld" -z relro --hash-style=gnu --build-id --eh-frame-hdr -m elf_i386 -dynamic-linker /lib/ld-linux.so.2 -o x86/realmode.elf -L/lib/../lib32 -L/usr/lib/../lib32 -L/usr/lib/llvm-10/bin/../lib -L/lib -L/usr/lib -m elf_i386 -T /home/roolebo/dev/kvm-unit-tests/x86/realmode.lds x86/realmode.o

(BTW, I'm surprised to see -dynamic-linker being set, it has little sense for
the tests but that's likely because -static is not passed explicitly)

And if I pass --print-map option to the linker I can see that GNU build id is
placed at 0x4000:

$ clang-10 -m32 -nostdlib -o x86/realmode.elf -Wl,-M -Wl,-m,elf_i386 -Wl,-T,/home/roo
lebo/dev/kvm-unit-tests/x86/realmode.lds x86/realmode.o

Discarded input sections

 .llvm_addrsig  0x0000000000000000      0x122 x86/realmode.o

Memory Configuration

Name             Origin             Length             Attributes
*default*        0x0000000000000000 0xffffffffffffffff

Linker script and memory map

                0x0000000000004000                . = 0x4000
                0x0000000000004000                stext = .

.note.gnu.build-id
                0x0000000000004000       0x24
 .note.gnu.build-id
                0x0000000000004000       0x24 x86/realmode.o

.text           0x0000000000004030     0x290c
 *(.init)
 .init          0x0000000000004030        0xc x86/realmode.o
 *(.text)
 *fill*         0x000000000000403c        0x4
 .text          0x0000000000004040     0x28fc x86/realmode.o
                0x000000000000404c                start
                0x00000000000040a0                realmode_start

.text.insn      0x000000000000693c      0x40e
 .text.insn     0x000000000000693c      0x40e x86/realmode.o

.iplt           0x0000000000006d4a        0x0
 .iplt          0x0000000000006d4a        0x0 x86/realmode.o

.rel.dyn        0x0000000000006d4c        0x0
 .rel.got       0x0000000000006d4c        0x0 x86/realmode.o
 .rel.iplt      0x0000000000006d4c        0x0 x86/realmode.o
                0x0000000000007000                . = ALIGN (0x1000)

.data           0x0000000000007000     0x2538
 *(.data)
 .data          0x0000000000007000     0x1064 x86/realmode.o
                0x0000000000008000                r_gdt
                0x0000000000008018                r_gdt_descr
                0x000000000000801e                r_idt_descr
 *(.rodata*)
 .rodata.str1.1
                0x0000000000008064      0x4d3 x86/realmode.o
                                        0x4f9 (size before relaxing)
 *fill*         0x0000000000008537        0x1
 .rodata        0x0000000000008538     0x1000 x86/realmode.o

.data.insn      0x0000000000009538      0x21c
 .data.insn     0x0000000000009538      0x21c x86/realmode.o

.got            0x0000000000009754        0x0
 .got           0x0000000000009754        0x0 x86/realmode.o

.got.plt        0x0000000000009754        0x0
 .got.plt       0x0000000000009754        0x0 x86/realmode.o

.igot.plt       0x0000000000009754        0x0
 .igot.plt      0x0000000000009754        0x0 x86/realmode.o
                0x0000000000009760                . = ALIGN (0x10)

.bss            0x0000000000009760      0x284
 *(.bss)
 .bss           0x0000000000009760      0x284 x86/realmode.o
                0x0000000000009764                tmp_stack
                0x00000000000099e4                edata = .
LOAD x86/realmode.o
OUTPUT(x86/realmode.elf elf32-i386)

.debug_str      0x0000000000000000      0x509
 .debug_str     0x0000000000000000      0x509 x86/realmode.o
                                        0x59f (size before relaxing)

.debug_loc      0x0000000000000000      0x484
 .debug_loc     0x0000000000000000      0x484 x86/realmode.o

.debug_abbrev   0x0000000000000000      0x212
 .debug_abbrev  0x0000000000000000      0x212 x86/realmode.o

.debug_info     0x0000000000000000     0x1935
 .debug_info    0x0000000000000000     0x1935 x86/realmode.o

.comment        0x0000000000000000       0x1f
 .comment       0x0000000000000000       0x1f x86/realmode.o
                                         0x20 (size before relaxing)

.note.GNU-stack
                0x0000000000000000        0x0
 .note.GNU-stack
                0x0000000000000000        0x0 x86/realmode.o

.debug_frame    0x0000000000000000      0x824
 .debug_frame   0x0000000000000000      0x824 x86/realmode.o

.debug_line     0x0000000000000000      0xc06
 .debug_line    0x0000000000000000      0xc06 x86/realmode.o


So, a workaround for that could be adding '-Wl,--build-id=none' to the
makefile rule for realmode.elf. Then multiboot magic is placed properly
at 0x4000 instead of 0x4030. Unfortunately it doesn't help with the
test :-)

-Roman
Paolo Bonzini Sept. 22, 2020, 2:51 p.m. UTC | #5
On 15/09/20 17:59, Roman Bolshakov wrote:
> So, a workaround for that could be adding '-Wl,--build-id=none' to the
> makefile rule for realmode.elf. Then multiboot magic is placed properly
> at 0x4000 instead of 0x4030. Unfortunately it doesn't help with the
> test :-)

Heh, weird.  I also tried adding

    /DISCARD/ : { *(.note.gnu.build-id) }

to the linker script and I got a very helpful (not) linker warning:

/usr/bin/ld: warning: .note.gnu.build-id section discarded, --build-id ignored.

... except that the --build-id was placed not by me but rather by gcc.
So we should probably simplify things doing this:

diff --git a/x86/Makefile.common b/x86/Makefile.common
index 090ce22..10c8a42 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -69,8 +69,8 @@ test_cases: $(tests-common) $(tests)
 $(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/x86 -I lib
 
 $(TEST_DIR)/realmode.elf: $(TEST_DIR)/realmode.o
-	$(CC) -m32 -nostdlib -o $@ -Wl,-m,elf_i386 \
-	      -Wl,-T,$(SRCDIR)/$(TEST_DIR)/realmode.lds $^
+	$(LD) -o $@ -m elf_i386 \
+	      -T $(SRCDIR)/$(TEST_DIR)/realmode.lds $^
 
 $(TEST_DIR)/realmode.o: bits = 32
 
diff --git a/x86/realmode.lds b/x86/realmode.lds
index 0ed3063..3220c19 100644
--- a/x86/realmode.lds
+++ b/x86/realmode.lds
@@ -1,5 +1,6 @@
 SECTIONS
 {
+    /DISCARD/ : { *(.note.gnu.build-id) }
     . = 16K;
     stext = .;
     .text : { *(.init) *(.text) }

which I will squash in your patch 3.

But the main issue is that clang does not support .code16gcc so it
writes 32-bit code that is run in 16-bit mode.  It'd be a start to
use -m16 instead of -m32, but then I think it still miscompiles the
(32-bit) code between "start" and the .code16gcc label.

Paolo
Roman Bolshakov Sept. 22, 2020, 9:25 p.m. UTC | #6
On Tue, Sep 22, 2020 at 04:51:18PM +0200, Paolo Bonzini wrote:
> On 15/09/20 17:59, Roman Bolshakov wrote:
> > So, a workaround for that could be adding '-Wl,--build-id=none' to the
> > makefile rule for realmode.elf. Then multiboot magic is placed properly
> > at 0x4000 instead of 0x4030. Unfortunately it doesn't help with the
> > test :-)
> 
> Heh, weird.  I also tried adding
> 
>     /DISCARD/ : { *(.note.gnu.build-id) }
> 
> to the linker script and I got a very helpful (not) linker warning:
> 
> /usr/bin/ld: warning: .note.gnu.build-id section discarded, --build-id ignored.
> 
> ... except that the --build-id was placed not by me but rather by gcc.
> So we should probably simplify things doing this:
> 
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index 090ce22..10c8a42 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -69,8 +69,8 @@ test_cases: $(tests-common) $(tests)
>  $(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/x86 -I lib
>  
>  $(TEST_DIR)/realmode.elf: $(TEST_DIR)/realmode.o
> -	$(CC) -m32 -nostdlib -o $@ -Wl,-m,elf_i386 \
> -	      -Wl,-T,$(SRCDIR)/$(TEST_DIR)/realmode.lds $^
> +	$(LD) -o $@ -m elf_i386 \
> +	      -T $(SRCDIR)/$(TEST_DIR)/realmode.lds $^
>  

Agreed, in the case it's better to tell linker directly what is needed
rather than fighting with compiler's way of invoking the linker.

>  $(TEST_DIR)/realmode.o: bits = 32
>  
> diff --git a/x86/realmode.lds b/x86/realmode.lds
> index 0ed3063..3220c19 100644
> --- a/x86/realmode.lds
> +++ b/x86/realmode.lds
> @@ -1,5 +1,6 @@
>  SECTIONS
>  {
> +    /DISCARD/ : { *(.note.gnu.build-id) }
>      . = 16K;
>      stext = .;
>      .text : { *(.init) *(.text) }
> 
> which I will squash in your patch 3.
> 

Thanks!

There's another difference right after multiboot header.

Here's how GCC binary looks:

00004000 <stext>:
    4000:       02 b0 ad 1b 00 00       add    0x1bad(%eax),%dh
    4006:       00 00                   add    %al,(%eax)
    4008:       fe 4f 52                decb   0x52(%edi)
    400b:       e4                      .byte 0xe4

0000400c <test_function>:
    400c:       66 b8 34 12             mov    $0x1234,%ax
    4010:       00 00                   add    %al,(%eax)
    4012:       66 c3                   retw

Here's clang:

00004000 <stext>:
    4000:       02 b0 ad 1b 00 00       add    0x1bad(%eax),%dh
    4006:       00 00                   add    %al,(%eax)
    4008:       fe 4f 52                decb   0x52(%edi)
    400b:       e4 66                   in     $0x66,%al
    400d:       90                      nop
    400e:       66 90                   xchg   %ax,%ax

00004010 <test_function>:
    4010:       66 b8 34 12             mov    $0x1234,%ax
    4014:       00 00                   add    %al,(%eax)
    4016:       66 c3                   retw


So, clang pads stext with two NOPs after 400b until it's quad-aligned.
I'm not sure how we can ask it to stop doing that.

The assembly (clang-10 -S) doesn't show an alignment requirement:

.set mb_magic, 464367618
.set mb_flags, 0
        # multiboot header
        .long   464367618
        .long   0
        .long   -464367618
        .p2align        0, 0x90
        .globl  start

".p2align 0, 0x90" behaves like ".p2align 4, 0x90", sounds like a bug?

But it doesn't introduce an issue as it turned out later

> But the main issue is that clang does not support .code16gcc so it
> writes 32-bit code that is run in 16-bit mode.

I had impression that it does support .code16gcc from the PR (and
included since LLVM 4.0):

https://reviews.llvm.org/D20109
https://github.com/llvm/llvm-project/commit/6477ce2697bf1d9afd2bcc0cf0c16c7cf08713be

Then another changes register allocation since LLVM 5.0 but I don't
know if it breaks anything (I'm not familiar with LLVM TBH).

https://github.com/llvm/llvm-project/commit/f5f593b674ed031f3f5aa2c44ac705547532d5cb

> It'd be a start to use -m16 instead of -m32, but then I think it still
> miscompiles the (32-bit) code between "start" and the .code16gcc
> label.
> 

Bingo! Changing target variable "bits = 32" to "bits = 16" helps, it
proceeds properly until "iret 1" (insn_code_iret32) test and then it
hangs.

Inline assembly:

        MK_INSN(iret32, "pushf\n\t"
                        "pushl %cs\n\t"
                        "call 1f\n\t" /* a near call will push eip onto the stack */
                        "jmp 2f\n\t"
                        "1: iretl\n\t"
                        "2:\n\t"
                     );

GCC:

00006c25 <insn_code_iret32>:
    6c25:       66 9c                   pushfw
    6c27:       66 0e                   pushw  %cs
    6c29:       66 e8 02 00             callw  6c2f <insn_code_iret32+0xa>
    6c2d:       00 00                   add    %al,(%eax)
    6c2f:       eb 02                   jmp    6c33 <insn_code_iret16>
    6c31:       66 cf                   iretw

Clang saves 16-bit registers but restores 32-bit in `iret` and `call`
doesn't have an operand-size prefix:

00007547 <insn_code_iret32>:
    7547:       9c                      pushf
    7548:       66 0e                   pushw  %cs
    754a:       e8 02 00 eb 02          call   2eb7551 <edata+0x2eacb6d>
    754f:       66 cf                   iretw

So, this fixes the test and makes "iret 3" pass (otherwise it hangs):

diff --git a/x86/realmode.c b/x86/realmode.c
index 7c2d776..0ae5186 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -761,9 +761,9 @@ static void test_pusha_popa(void)

 static void test_iret(void)
 {
-       MK_INSN(iret32, "pushf\n\t"
+       MK_INSN(iret32, "pushfl\n\t"
                        "pushl %cs\n\t"
-                       "call 1f\n\t" /* a near call will push eip onto the stack */
+                       "calll 1f\n\t" /* a near call will push eip onto the stack */
                        "jmp 2f\n\t"
                        "1: iretl\n\t"
                        "2:\n\t"
@@ -782,7 +782,7 @@ static void test_iret(void)
                              "orl $0xffc18028, %eax\n\t"
                              "pushl %eax\n\t"
                              "pushl %cs\n\t"
-                             "call 1f\n\t"
+                             "calll 1f\n\t"
                              "jmp 2f\n\t"
                              "1: iretl\n\t"
                              "2:\n\t");

With the above change I get the following machine code for iret32 which
is equivalent to GCC:

00007547 <insn_code_iret32>:
    7547:       66 9c                   pushfw
    7549:       66 0e                   pushw  %cs
    754b:       66 e8 02 00             callw  7551 <insn_code_iret32+0xa>
    754f:       00 00                   add    %al,(%eax)
    7551:       eb 02                   jmp    7555 <insn_code_iret16>
    7553:       66 cf                   iretw

Still, there're a few more failed tests but realmode doesn't hang anymore:
FAIL: pusha/popa 1
FAIL: pusha/popa 1
FAIL: jmp far 1

And explicit instruction suffixes fix them too:

@@ -639,7 +639,7 @@ static void test_jcc_near(void)

 static void test_long_jmp(void)
 {
-       MK_INSN(long_jmp, "call 1f\n\t"
+       MK_INSN(long_jmp, "calll 1f\n\t"
                          "jmp 2f\n\t"
                          "1: jmp $0, $test_function\n\t"
                          "2:\n\t");
@@ -728,26 +728,26 @@ static void test_null(void)

 static void test_pusha_popa(void)
 {
-       MK_INSN(pusha, "pusha\n\t"
-                      "pop %edi\n\t"
-                      "pop %esi\n\t"
-                      "pop %ebp\n\t"
-                      "add $4, %esp\n\t"
-                      "pop %ebx\n\t"
-                      "pop %edx\n\t"
-                      "pop %ecx\n\t"
-                      "pop %eax\n\t"
+       MK_INSN(pusha, "pushal\n\t"
+                      "popl %edi\n\t"
+                      "popl %esi\n\t"
+                      "popl %ebp\n\t"
+                      "addl $4, %esp\n\t"
+                      "popl %ebx\n\t"
+                      "popl %edx\n\t"
+                      "popl %ecx\n\t"
+                      "popl %eax\n\t"
                       );

-       MK_INSN(popa, "push %eax\n\t"
-                     "push %ecx\n\t"
-                     "push %edx\n\t"
-                     "push %ebx\n\t"
-                     "push %esp\n\t"
-                     "push %ebp\n\t"
-                     "push %esi\n\t"
-                     "push %edi\n\t"
-                     "popa\n\t"
+       MK_INSN(popa, "pushl %eax\n\t"
+                     "pushl %ecx\n\t"
+                     "pushl %edx\n\t"
+                     "pushl %ebx\n\t"
+                     "pushl %esp\n\t"
+                     "pushl %ebp\n\t"
+                     "pushl %esi\n\t"
+                     "pushl %edi\n\t"
+                     "popal\n\t"
                      );

        init_inregs(&(struct regs){ .eax = 0, .ebx = 1, .ecx = 2, .edx = 3, .esi = 4, .edi = 5, .ebp = 6 });



Then everything passes in realmode test:
$ ./x86-run x86/realmode.flat | grep FAIL
qemu-system-x86_64: warning: host doesn't support requested feature:
CPUID.80000001H:ECX.svm [bit 2]
$


Perhaps it's worth to respin the series.

Thanks,
Roman
Paolo Bonzini Sept. 23, 2020, 2:37 a.m. UTC | #7
On 22/09/20 23:25, Roman Bolshakov wrote:
> 
> Then everything passes in realmode test:
> $ ./x86-run x86/realmode.flat | grep FAIL
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:ECX.svm [bit 2]
> $
> 
> 
> Perhaps it's worth to respin the series.

No, just send stuff on top.  I've now pushed everything to master.

Paolo
diff mbox series

Patch

diff --git a/.travis.yml b/.travis.yml
index f3a8899..ae4ed08 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -17,6 +17,16 @@  jobs:
                kvmclock_test msr pcid rdpru realmode rmap_chain s3 setjmp umip"
       - ACCEL="kvm"
 
+    - addons:
+        apt_packages: clang-10 qemu-system-x86
+      env:
+      - CONFIG="--cc=clang-10"
+      - BUILD_DIR="."
+      - TESTS="access asyncpf debug emulator ept hypercall hyperv_stimer
+               hyperv_synic idt_test intel_iommu ioapic ioapic-split
+               kvmclock_test msr pcid rdpru rmap_chain s3 setjmp umip"
+      - ACCEL="kvm"
+
     - addons:
         apt_packages: gcc qemu-system-x86
       env: