| Message ID | 20200624165455.19266-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: move IDT away from address 0 | expand |
On 24/06/2020 18.54, Paolo Bonzini wrote: > Address 0 is also used for the SIPI vector (which is probably something worth > changing as well), and now that we call setup_idt very early the SIPI vector > overwrites the first few bytes of the IDT, and in particular the #DE handler. > > Fix this for both 32-bit and 64-bit, even though the different form of the > descriptors meant that only 32-bit showed a failure. > > Reported-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > x86/cstart.S | 10 +++++++--- > x86/cstart64.S | 11 ++++++++++- > 2 files changed, 17 insertions(+), 4 deletions(-) Thanks, this fixes the eventinj test for me! Tested-by: Thomas Huth <thuth@redhat.com>
> On Jun 24, 2020, at 9:54 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Address 0 is also used for the SIPI vector (which is probably something worth > changing as well), and now that we call setup_idt very early the SIPI vector > overwrites the first few bytes of the IDT, and in particular the #DE handler. > > Fix this for both 32-bit and 64-bit, even though the different form of the > descriptors meant that only 32-bit showed a failure. > > Reported-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > x86/cstart.S | 10 +++++++--- > x86/cstart64.S | 11 ++++++++++- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/x86/cstart.S b/x86/cstart.S > index 77dc34d..e93dbca 100644 > --- a/x86/cstart.S > +++ b/x86/cstart.S > @@ -4,8 +4,6 @@ > .globl boot_idt > .global online_cpus > > -boot_idt = 0 > - I think that there is a hidden assumption about the IDT location in realmode’s test_int(), which this would break: static void test_int(void) { init_inregs(NULL); boot_idt[11] = 0x1000; /* Store a pointer to address 0x1000 in IDT entry 0x11 */ *(u8 *)(0x1000) = 0xcf; /* 0x1000 contains an IRET instruction */ MK_INSN(int11, "int $0x11\n\t"); exec_in_big_real_mode(&insn_int11); report("int 1", 0, 1); }
> On Jun 25, 2020, at 11:59 AM, Nadav Amit <nadav.amit@gmail.com> wrote: > >> On Jun 24, 2020, at 9:54 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> Address 0 is also used for the SIPI vector (which is probably something worth >> changing as well), and now that we call setup_idt very early the SIPI vector >> overwrites the first few bytes of the IDT, and in particular the #DE handler. >> >> Fix this for both 32-bit and 64-bit, even though the different form of the >> descriptors meant that only 32-bit showed a failure. >> >> Reported-by: Thomas Huth <thuth@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> x86/cstart.S | 10 +++++++--- >> x86/cstart64.S | 11 ++++++++++- >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/x86/cstart.S b/x86/cstart.S >> index 77dc34d..e93dbca 100644 >> --- a/x86/cstart.S >> +++ b/x86/cstart.S >> @@ -4,8 +4,6 @@ >> .globl boot_idt >> .global online_cpus >> >> -boot_idt = 0 >> - > > I think that there is a hidden assumption about the IDT location in > realmode’s test_int(), which this would break. [ Sorry for the previous wrong quote of my attempt the fix ] The original offending code: static void test_int(void) { init_inregs(NULL); *(u32 *)(0x11 * 4) = 0x1000; /* Store a pointer to address 0x1000 in IDT entry 0x11 */ *(u8 *)(0x1000) = 0xcf; /* 0x1000 contains an IRET instruction */ MK_INSN(int11, "int $0x11\n\t"); exec_in_big_real_mode(&insn_int11); report("int 1", 0, 1); } static void test_sti_inhibit(void) { init_inregs(NULL); *(u32 *)(0x73 * 4) = 0x1000; /* Store IRQ 11 handler in the IDT */ *(u8 *)(0x1000) = 0xcf; /* 0x1000 contains an IRET instruction */ MK_INSN(sti_inhibit, "cli\n\t" "movw $0x200b, %dx\n\t" "movl $1, %eax\n\t" "outl %eax, %dx\n\t" /* Set IRQ11 */ "movl $0, %eax\n\t" "outl %eax, %dx\n\t" /* Clear IRQ11 */ "sti\n\t" "hlt\n\t"); exec_in_big_real_mode(&insn_sti_inhibit); report("sti inhibit", ~0, 1); }
On 25/06/20 20:59, Nadav Amit wrote: > I think that there is a hidden assumption about the IDT location in > realmode’s test_int(), which this would break: > > static void test_int(void) > { > init_inregs(NULL); > > boot_idt[11] = 0x1000; /* Store a pointer to address 0x1000 in IDT entry 0x11 */ > *(u8 *)(0x1000) = 0xcf; /* 0x1000 contains an IRET instruction */ > > MK_INSN(int11, "int $0x11\n\t"); > > exec_in_big_real_mode(&insn_int11); > report("int 1", 0, 1); > } Uuuuuuuuuuuuuuuumph... you're right. :( Will send a patch tomorrow. Paolo
On 25/06/20 21:18, Paolo Bonzini wrote: > On 25/06/20 20:59, Nadav Amit wrote: >> I think that there is a hidden assumption about the IDT location in >> realmode’s test_int(), which this would break: >> >> static void test_int(void) >> { >> init_inregs(NULL); >> >> boot_idt[11] = 0x1000; /* Store a pointer to address 0x1000 in IDT entry 0x11 */ >> *(u8 *)(0x1000) = 0xcf; /* 0x1000 contains an IRET instruction */ >> >> MK_INSN(int11, "int $0x11\n\t"); >> >> exec_in_big_real_mode(&insn_int11); >> report("int 1", 0, 1); >> } > > Uuuuuuuuuuuuuuuumph... you're right. :( Will send a patch tomorrow. Actually the IDTR is not reloaded by exec_in_big_real_mode, so this (while a bit weird) works fine. Paolo
> On Jun 26, 2020, at 12:05 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 25/06/20 21:18, Paolo Bonzini wrote: >> On 25/06/20 20:59, Nadav Amit wrote: >>> I think that there is a hidden assumption about the IDT location in >>> realmode’s test_int(), which this would break: >>> >>> static void test_int(void) >>> { >>> init_inregs(NULL); >>> >>> boot_idt[11] = 0x1000; /* Store a pointer to address 0x1000 in IDT entry 0x11 */ >>> *(u8 *)(0x1000) = 0xcf; /* 0x1000 contains an IRET instruction */ >>> >>> MK_INSN(int11, "int $0x11\n\t"); >>> >>> exec_in_big_real_mode(&insn_int11); >>> report("int 1", 0, 1); >>> } >> >> Uuuuuuuuuuuuuuuumph... you're right. :( Will send a patch tomorrow. > > Actually the IDTR is not reloaded by exec_in_big_real_mode, so this > (while a bit weird) works fine. Err… So it means I need to debug why it does not work for *me*…
On 26/06/20 09:06, Nadav Amit wrote: >> Actually the IDTR is not reloaded by exec_in_big_real_mode, so this >> (while a bit weird) works fine. > Err… So it means I need to debug why it does not work for *me*… Hmm, maybe a dislike for an IDT that is placed above the first MiB of memory? But I cannot read anything about it in the manuals. In any case I would accept a patch that switches to the "usual" address 0 IDT in exec_big_real_mode. Paolo
diff --git a/x86/cstart.S b/x86/cstart.S index 77dc34d..e93dbca 100644 --- a/x86/cstart.S +++ b/x86/cstart.S @@ -4,8 +4,6 @@ .globl boot_idt .global online_cpus -boot_idt = 0 - ipi_vector = 0x20 max_cpus = MAX_TEST_CPUS @@ -30,6 +28,12 @@ i = 0 i = i + 1 .endr +boot_idt: + .rept 256 + .quad 0 + .endr +end_boot_idt: + .globl gdt32 gdt32: .quad 0 @@ -71,7 +75,7 @@ tss: tss_end: idt_descr: - .word 16 * 256 - 1 + .word end_boot_idt - boot_idt - 1 .long boot_idt .section .init diff --git a/x86/cstart64.S b/x86/cstart64.S index 1ecfbdb..b44d0ae 100644 --- a/x86/cstart64.S +++ b/x86/cstart64.S @@ -9,6 +9,8 @@ boot_idt = 0 .globl gdt64_desc .globl online_cpus +boot_idt = 0 + ipi_vector = 0x20 max_cpus = MAX_TEST_CPUS @@ -51,6 +53,13 @@ ptl5: .align 4096 +boot_idt: + .rept 256 + .quad 0 + .quad 0 + .endr +end_boot_idt: + gdt64_desc: .word gdt64_end - gdt64 - 1 .quad gdt64 @@ -282,7 +291,7 @@ lvl5: retq idt_descr: - .word 16 * 256 - 1 + .word end_boot_idt - boot_idt - 1 .quad boot_idt online_cpus:
Address 0 is also used for the SIPI vector (which is probably something worth changing as well), and now that we call setup_idt very early the SIPI vector overwrites the first few bytes of the IDT, and in particular the #DE handler. Fix this for both 32-bit and 64-bit, even though the different form of the descriptors meant that only 32-bit showed a failure. Reported-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- x86/cstart.S | 10 +++++++--- x86/cstart64.S | 11 ++++++++++- 2 files changed, 17 insertions(+), 4 deletions(-)