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;
> -} __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];
>
> @@ -253,4 +236,7 @@ static inline void *get_idt_addr(idt_entry_t *entry)
> return (void *)addr;
> }
>
> +extern gdt_entry_t *get_tss_descr(void);
> +extern unsigned long get_gdt_entry_base(gdt_entry_t *entry);
> +
> #endif
> diff --git a/x86/cstart64.S b/x86/cstart64.S
> index 5c6ad38..cf38bae 100644
> --- a/x86/cstart64.S
> +++ b/x86/cstart64.S
> @@ -4,7 +4,6 @@
> .globl boot_idt
>
> .globl idt_descr
> -.globl tss_descr
> .globl gdt64_desc
> .globl online_cpus
> .globl cpu_online_count
> diff --git a/x86/svm_tests.c b/x86/svm_tests.c
> index afdd359..8ad6122 100644
> --- a/x86/svm_tests.c
> +++ b/x86/svm_tests.c
> @@ -1875,23 +1875,14 @@ static bool reg_corruption_check(struct svm_test *test)
>
> static void get_tss_entry(void *data)
> {
> - struct descriptor_table_ptr gdt;
> - struct segment_desc64 *gdt_table;
> - struct segment_desc64 *tss_entry;
> - u16 tr = 0;
> -
> - sgdt(&gdt);
> - tr = str();
> - gdt_table = (struct segment_desc64 *) gdt.base;
> - tss_entry = &gdt_table[tr / sizeof(struct segment_desc64)];
> - *((struct segment_desc64 **)data) = tss_entry;
> + *((gdt_entry_t **)data) = get_tss_descr();
> }
>
> static int orig_cpu_count;
>
> static void init_startup_prepare(struct svm_test *test)
> {
> - struct segment_desc64 *tss_entry;
> + gdt_entry_t *tss_entry;
> int i;
>
> on_cpu(1, get_tss_entry, &tss_entry);
> @@ -1905,7 +1896,7 @@ static void init_startup_prepare(struct svm_test *test)
>
> --cpu_online_count;
>
> - *(uint64_t *)tss_entry &= ~DESC_BUSY;
> + tss_entry->type &= ~DESC_BUSY;
>
> apic_icr_write(APIC_DEST_PHYSICAL | APIC_DM_STARTUP, id_map[1]);
>
> diff --git a/x86/vmware_backdoors.c b/x86/vmware_backdoors.c
> index b4902a9..bc10020 100644
> --- a/x86/vmware_backdoors.c
> +++ b/x86/vmware_backdoors.c
> @@ -132,23 +132,13 @@ struct fault_test vmware_backdoor_tests[] = {
> */
> static void set_tss_ioperm(void)
> {
> - struct descriptor_table_ptr gdt;
> - struct segment_desc64 *gdt_table;
> - struct segment_desc64 *tss_entry;
> - u16 tr = 0;
> + gdt_entry_t *tss_entry;
> tss64_t *tss;
> unsigned char *ioperm_bitmap;
> - uint64_t tss_base;
> -
> - sgdt(&gdt);
> - tr = str();
> - gdt_table = (struct segment_desc64 *) gdt.base;
> - tss_entry = &gdt_table[tr / sizeof(struct segment_desc64)];
> - tss_base = ((uint64_t) tss_entry->base1 |
> - ((uint64_t) tss_entry->base2 << 16) |
> - ((uint64_t) tss_entry->base3 << 24) |
> - ((uint64_t) tss_entry->base4 << 32));
> - tss = (tss64_t *)tss_base;
> + u16 tr = str();
> +
> + tss_entry = get_tss_descr();
> + tss = (tss64_t *)get_gdt_entry_base(tss_entry);
> tss->iomap_base = sizeof(*tss);
> ioperm_bitmap = ((unsigned char *)tss+tss->iomap_base);
>
> @@ -157,7 +147,7 @@ static void set_tss_ioperm(void)
> 1 << (RANDOM_IO_PORT % 8);
> ioperm_bitmap[VMWARE_BACKDOOR_PORT / 8] |=
> 1 << (VMWARE_BACKDOOR_PORT % 8);
> - *(uint64_t *)tss_entry &= ~DESC_BUSY;
> + tss_entry->type &= ~DESC_BUSY;
>
> /* Update TSS */
> ltr(tr);
> 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);
optional: It would be good to add a comment explaining the `0x67` value here.
>
> vmcs_write(GUEST_AR_CS, 0xa09b);
> vmcs_write(GUEST_AR_DS, 0xc093);
> --
> 2.27.0
>
Reviewed-by: Marc Orr <marcorr@xxxxxxxxxx>
