+Aaron Lewis
On Wed, Oct 20, 2021 at 12:27 PM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
> tss_descr is declared as a struct descriptor_table_ptr but it is actualy
> pointing to an _entry_ in the GDT. Also it is different per CPU, but
> tss_descr does not recognize that. Fix both by reusing the code
> (already present e.g. in the vmware_backdoors test) that extracts
> the base from the GDT entry; and also provide a helper to retrieve
> the limit, which is needed in vmx.c.
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> lib/x86/desc.c | 22 ++++++++++++++++++++++
> lib/x86/desc.h | 28 +++++++---------------------
> x86/cstart64.S | 1 -
> x86/svm_tests.c | 15 +++------------
> x86/vmware_backdoors.c | 22 ++++++----------------
> x86/vmx.c | 9 +++++----
> 6 files changed, 43 insertions(+), 54 deletions(-)
>
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 3d38565..cfda2b2 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -409,3 +409,25 @@ void __set_exception_jmpbuf(jmp_buf *addr)
> {
> exception_jmpbuf = addr;
> }
> +
> +gdt_entry_t *get_tss_descr(void)
> +{
> + struct descriptor_table_ptr gdt_ptr;
> + gdt_entry_t *gdt;
> +
> + sgdt(&gdt_ptr);
> + gdt = (gdt_entry_t *)gdt_ptr.base;
> + return &gdt[str() / 8];
> +}
> +
> +unsigned long get_gdt_entry_base(gdt_entry_t *entry)
> +{
> + unsigned long base;
> + base = entry->base1 | ((u32)entry->base2 << 16) | ((u32)entry->base3 << 24);
> +#ifdef __x86_64__
> + if (!entry->s) {
> + base |= (u64)((gdt_entry16_t *)entry)->base4 << 32;
> + }
> +#endif
> + return base;
> +}
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index 0af37e3..e8a6c21 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -185,31 +185,14 @@ typedef struct {
> } __attribute__((__packed__)) gdt_entry_t;
>
> #ifdef __x86_64__
> -struct segment_desc64 {
> - uint16_t limit1;
> - uint16_t base1;
> - uint8_t base2;
> - union {
> - uint16_t type_limit_flags; /* Type and limit flags */
> - struct {
> - uint16_t type:4;
> - uint16_t s:1;
> - uint16_t dpl:2;
> - uint16_t p:1;
> - uint16_t limit:4;
> - uint16_t avl:1;
> - uint16_t l:1;
> - uint16_t db:1;
> - uint16_t g:1;
> - } __attribute__((__packed__));
> - } __attribute__((__packed__));
> - uint8_t base3;
> +typedef struct {
> + gdt_entry_t common;
> uint32_t base4;
> uint32_t zero;
Technically, only bits 12:8 must be zero. The rest are ignored. See
the APM, volume 2, figure 4-22. But, perhaps that's a bit too
pedantic.
> -} __attribute__((__packed__));
> +} __attribute__((__packed__)) gdt_entry16_t;
> #endif
>
> -#define DESC_BUSY ((uint64_t) 1 << 41)
> +#define DESC_BUSY 2
>
> extern idt_entry_t boot_idt[256];
>
...
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 20dc677..063f96a 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -75,7 +75,6 @@ union vmx_ept_vpid ept_vpid;
>
> extern struct descriptor_table_ptr gdt64_desc;
> extern struct descriptor_table_ptr idt_descr;
> -extern struct descriptor_table_ptr tss_descr;
> extern void *vmx_return;
> extern void *entry_sysenter;
> extern void *guest_entry;
> @@ -1275,7 +1274,7 @@ static void init_vmcs_host(void)
> vmcs_write(HOST_SEL_FS, KERNEL_DS);
> vmcs_write(HOST_SEL_GS, KERNEL_DS);
> vmcs_write(HOST_SEL_TR, TSS_MAIN);
> - vmcs_write(HOST_BASE_TR, tss_descr.base);
> + vmcs_write(HOST_BASE_TR, get_gdt_entry_base(get_tss_descr()));
> vmcs_write(HOST_BASE_GDTR, gdt64_desc.base);
> vmcs_write(HOST_BASE_IDTR, idt_descr.base);
> vmcs_write(HOST_BASE_FS, 0);
> @@ -1291,6 +1290,8 @@ static void init_vmcs_host(void)
>
> static void init_vmcs_guest(void)
> {
> + gdt_entry_t *tss_descr = get_tss_descr();
> +
> /* 26.3 CHECKING AND LOADING GUEST STATE */
> ulong guest_cr0, guest_cr4, guest_cr3;
> /* 26.3.1.1 */
> @@ -1331,7 +1332,7 @@ static void init_vmcs_guest(void)
> vmcs_write(GUEST_BASE_DS, 0);
> vmcs_write(GUEST_BASE_FS, 0);
> vmcs_write(GUEST_BASE_GS, 0);
> - vmcs_write(GUEST_BASE_TR, tss_descr.base);
> + vmcs_write(GUEST_BASE_TR, get_gdt_entry_base(tss_descr));
> vmcs_write(GUEST_BASE_LDTR, 0);
>
> vmcs_write(GUEST_LIMIT_CS, 0xFFFFFFFF);
> @@ -1341,7 +1342,7 @@ static void init_vmcs_guest(void)
> vmcs_write(GUEST_LIMIT_FS, 0xFFFFFFFF);
> vmcs_write(GUEST_LIMIT_GS, 0xFFFFFFFF);
> vmcs_write(GUEST_LIMIT_LDTR, 0xffff);
> - vmcs_write(GUEST_LIMIT_TR, tss_descr.limit);
> + vmcs_write(GUEST_LIMIT_TR, 0x67);
Isn't the limit still set to 0xFFFF in {cstart,cstart64}.S? And
doesn't the VMware backdoor test assume there's room for an I/O
permission bitmap?
> vmcs_write(GUEST_AR_CS, 0xa09b);
> vmcs_write(GUEST_AR_DS, 0xc093);
> --
> 2.27.0
>
