[PATCH kvm-unit-tests 0/2] remove tss_descr, replace with a function

[PATCH kvm-unit-tests 0/2] remove tss_descr, replace with a function

[Paolo Bonzini]

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.

Patch 1 adjusts the structs for GDT descriptors, so that the same
code works for both 32-bit and 64-bit (apart from the high 32 bits
of the base field).

Paolo

Paolo Bonzini (2):
  unify structs for GDT descriptors
  replace tss_descr global with a function

 lib/x86/desc.c         | 26 +++++++++++--------
 lib/x86/desc.h         | 58 ++++++++++++++++++++++++++++++++++--------
 x86/cstart64.S         |  1 -
 x86/svm_tests.c        | 15 +++--------
 x86/taskswitch.c       |  2 +-
 x86/vmware_backdoors.c | 22 +++++-----------
 x86/vmx.c              |  9 ++++---
 7 files changed, 78 insertions(+), 55 deletions(-)

-- 
2.27.0

[PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors

[Paolo Bonzini]

Use the same names and definitions (apart from the high base field)
for GDT descriptors.  gdt_entry_t is for 8-byte entries and
gdt_desc_entry_t is for when 64-bit tests should use a 16-byte
entry.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 lib/x86/desc.c         | 16 ++++++----------
 lib/x86/desc.h         | 36 ++++++++++++++++++++++++++----------
 x86/svm_tests.c        | 12 ++++++------
 x86/taskswitch.c       |  2 +-
 x86/vmware_backdoors.c |  8 ++++----
 5 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index e7378c1..3d38565 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -285,17 +285,13 @@ void set_gdt_entry(int sel, u32 base,  u32 limit, u8 access, u8 gran)
 	int num = sel >> 3;
 
 	/* Setup the descriptor base address */
-	gdt32[num].base_low = (base & 0xFFFF);
-	gdt32[num].base_middle = (base >> 16) & 0xFF;
-	gdt32[num].base_high = (base >> 24) & 0xFF;
+	gdt32[num].base1 = (base & 0xFFFF);
+	gdt32[num].base2 = (base >> 16) & 0xFF;
+	gdt32[num].base3 = (base >> 24) & 0xFF;
 
-	/* Setup the descriptor limits */
-	gdt32[num].limit_low = (limit & 0xFFFF);
-	gdt32[num].granularity = ((limit >> 16) & 0x0F);
-
-	/* Finally, set up the granularity and access flags */
-	gdt32[num].granularity |= (gran & 0xF0);
-	gdt32[num].access = access;
+	/* Setup the descriptor limits, granularity and flags */
+	gdt32[num].limit1 = (limit & 0xFFFF);
+	gdt32[num].type_limit_flags = ((limit & 0xF0000) >> 8) | ((gran & 0xF0) << 8) | access;
 }
 
 void set_gdt_task_gate(u16 sel, u16 tss_sel)
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index a6ffb38..3aa1eca 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -164,15 +164,28 @@ typedef struct {
 } idt_entry_t;
 
 typedef struct {
-	u16 limit_low;
-	u16 base_low;
-	u8 base_middle;
-	u8 access;
-	u8 granularity;
-	u8 base_high;
-} gdt_entry_t;
-
-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;
+} __attribute__((__packed__)) gdt_entry_t;
+
+#ifdef __x86_64__
+typedef struct {
 	uint16_t limit1;
 	uint16_t base1;
 	uint8_t  base2;
@@ -193,7 +206,10 @@ struct segment_desc64 {
 	uint8_t  base3;
 	uint32_t base4;
 	uint32_t zero;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) gdt_desc_entry_t;
+#else
+typedef gdt_entry_t gdt_desc_entry_t;
+#endif
 
 #define DESC_BUSY ((uint64_t) 1 << 41)
 
diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index afdd359..014fbbf 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -1876,22 +1876,22 @@ 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;
+    gdt_entry_t *gdt_table;
+    gdt_entry_t *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_table = (gdt_entry_t *) gdt.base;
+    tss_entry = &gdt_table[tr / sizeof(gdt_entry_t)];
+    *((gdt_entry_t **)data) = tss_entry;
 }
 
 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);
diff --git a/x86/taskswitch.c b/x86/taskswitch.c
index 889831e..b6b3451 100644
--- a/x86/taskswitch.c
+++ b/x86/taskswitch.c
@@ -21,7 +21,7 @@ fault_handler(unsigned long error_code)
 
 	tss.eip += 2;
 
-	gdt32[TSS_MAIN / 8].access &= ~2;
+	gdt32[TSS_MAIN / 8].type &= ~2;
 
 	set_gdt_task_gate(TSS_RETURN, tss_intr.prev);
 }
diff --git a/x86/vmware_backdoors.c b/x86/vmware_backdoors.c
index b4902a9..24870d7 100644
--- a/x86/vmware_backdoors.c
+++ b/x86/vmware_backdoors.c
@@ -133,8 +133,8 @@ 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;
+	gdt_entry_t *gdt_table;
+	gdt_desc_entry_t *tss_entry;
 	u16 tr = 0;
 	tss64_t *tss;
 	unsigned char *ioperm_bitmap;
@@ -142,8 +142,8 @@ static void set_tss_ioperm(void)
 
 	sgdt(&gdt);
 	tr = str();
-	gdt_table = (struct segment_desc64 *) gdt.base;
-	tss_entry = &gdt_table[tr / sizeof(struct segment_desc64)];
+	gdt_table = (gdt_entry_t *) gdt.base;
+	tss_entry = (gdt_desc_entry_t *) &gdt_table[tr / sizeof(gdt_entry_t)];
 	tss_base = ((uint64_t) tss_entry->base1 |
 			((uint64_t) tss_entry->base2 << 16) |
 			((uint64_t) tss_entry->base3 << 24) |
-- 
2.27.0

[PATCH kvm-unit-tests 2/2] replace tss_descr global with a function

[Paolo Bonzini]

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@redhat.com>
---
 lib/x86/desc.c         | 10 ++++++++++
 lib/x86/desc.h         | 22 +++++++++++++++++++++-
 x86/cstart64.S         |  1 -
 x86/svm_tests.c        | 15 +++------------
 x86/vmware_backdoors.c | 20 +++++---------------
 x86/vmx.c              |  9 +++++----
 6 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 3d38565..ec06a53 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -409,3 +409,13 @@ void __set_exception_jmpbuf(jmp_buf *addr)
 {
 	exception_jmpbuf = addr;
 }
+
+gdt_desc_entry_t *get_tss_descr(void)
+{
+	struct descriptor_table_ptr gdt_ptr;
+	u8 *gdt;
+
+	sgdt(&gdt_ptr);
+	gdt = (u8 *)gdt_ptr.base;
+	return (gdt_desc_entry_t *) (gdt + (str() & ~7));
+}
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 3aa1eca..cd4d2e7 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -211,7 +211,7 @@ typedef struct {
 typedef gdt_entry_t gdt_desc_entry_t;
 #endif
 
-#define DESC_BUSY ((uint64_t) 1 << 41)
+#define DESC_BUSY 2
 
 extern idt_entry_t boot_idt[256];
 
@@ -255,4 +255,16 @@ static inline void *get_idt_addr(idt_entry_t *entry)
 	return (void *)addr;
 }
 
+extern gdt_desc_entry_t *get_tss_descr(void);
+
+static inline unsigned long get_gdt_entry_base(gdt_desc_entry_t *entry)
+{
+	unsigned long base;
+	base = entry->base1 | ((u32)entry->base2 << 16) | ((u32)entry->base3 << 24);
+#ifdef __x86_64__
+	base |= (u64)entry->base4 << 32;
+#endif
+	return base;
+}
+
 #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 014fbbf..b663c5e 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;
-    gdt_entry_t *gdt_table;
-    gdt_entry_t *tss_entry;
-    u16 tr = 0;
-
-    sgdt(&gdt);
-    tr = str();
-    gdt_table = (gdt_entry_t *) gdt.base;
-    tss_entry = &gdt_table[tr / sizeof(gdt_entry_t)];
-    *((gdt_entry_t **)data) = tss_entry;
+    *((gdt_desc_entry_t **)data) = get_tss_descr();
 }
 
 static int orig_cpu_count;
 
 static void init_startup_prepare(struct svm_test *test)
 {
-    gdt_entry_t *tss_entry;
+    gdt_desc_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 24870d7..85b7c09 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;
-	gdt_entry_t *gdt_table;
 	gdt_desc_entry_t *tss_entry;
-	u16 tr = 0;
 	tss64_t *tss;
 	unsigned char *ioperm_bitmap;
-	uint64_t tss_base;
-
-	sgdt(&gdt);
-	tr = str();
-	gdt_table = (gdt_entry_t *) gdt.base;
-	tss_entry = (gdt_desc_entry_t *) &gdt_table[tr / sizeof(gdt_entry_t)];
-	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..bfa05ce 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_desc_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);
 
 	vmcs_write(GUEST_AR_CS, 0xa09b);
 	vmcs_write(GUEST_AR_DS, 0xc093);
-- 
2.27.0

Re: [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors

[Sean Christopherson]

+Jim who is poking at this area too.

On Wed, Oct 20, 2021, Paolo Bonzini wrote:
> Use the same names and definitions (apart from the high base field)
> for GDT descriptors.  gdt_entry_t is for 8-byte entries and
> gdt_desc_entry_t is for when 64-bit tests should use a 16-byte
> entry.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index a6ffb38..3aa1eca 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -164,15 +164,28 @@ typedef struct {
>  } idt_entry_t;
>  
>  typedef struct {
> -	u16 limit_low;
> -	u16 base_low;
> -	u8 base_middle;
> -	u8 access;
> -	u8 granularity;
> -	u8 base_high;
> -} gdt_entry_t;
> -
> -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;
> +} __attribute__((__packed__)) gdt_entry_t;
> +
> +#ifdef __x86_64__
> +typedef struct {
>  	uint16_t limit1;

Changelog says "unify struct", yet gdt_entry_t and gdt_desc_entry_t have fully
redundant information.  Apparently anonymous structs aren't a thing (or I'm just
doing it wrong), but even so, fully unifying this is not hugely problematic for
the sole consumer.



>  	uint16_t base1;
>  	uint8_t  base2;
> @@ -193,7 +206,10 @@ struct segment_desc64 {
>  	uint8_t  base3;
>  	uint32_t base4;
>  	uint32_t zero;
> -} __attribute__((__packed__));
> +} __attribute__((__packed__)) gdt_desc_entry_t;

This is misleading, only system descriptors have the extra 8 bytes.

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 3aa1eca..3b213c2 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -186,29 +186,13 @@ typedef struct {

 #ifdef __x86_64__
 typedef struct {
-       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__));
+       gdt_entry_t common;
        uint8_t  base3;
        uint32_t base4;
        uint32_t zero;
-} __attribute__((__packed__)) gdt_desc_entry_t;
+} __attribute__((__packed__)) gdt_system_desc_entry_t;
 #else
-typedef gdt_entry_t gdt_desc_entry_t;
+typedef gdt_entry_t gdt_system_desc_entry_t;
 #endif

 #define DESC_BUSY ((uint64_t) 1 << 41)
diff --git a/x86/vmware_backdoors.c b/x86/vmware_backdoors.c
index 24870d7..4d82617 100644
--- a/x86/vmware_backdoors.c
+++ b/x86/vmware_backdoors.c
@@ -134,7 +134,7 @@ static void set_tss_ioperm(void)
 {
        struct descriptor_table_ptr gdt;
        gdt_entry_t *gdt_table;
-       gdt_desc_entry_t *tss_entry;
+       gdt_system_desc_entry_t *tss_entry;
        u16 tr = 0;
        tss64_t *tss;
        unsigned char *ioperm_bitmap;
@@ -143,11 +143,11 @@ static void set_tss_ioperm(void)
        sgdt(&gdt);
        tr = str();
        gdt_table = (gdt_entry_t *) gdt.base;
-       tss_entry = (gdt_desc_entry_t *) &gdt_table[tr / sizeof(gdt_entry_t)];
-       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_entry = (gdt_system_desc_entry_t *) &gdt_table[tr / sizeof(gdt_entry_t)];
+       tss_base = ((uint64_t) tss_entry->common.base1 |
+                  ((uint64_t) tss_entry->common.base2 << 16) |
+                  ((uint64_t) tss_entry->base3 << 24) |
+                  ((uint64_t) tss_entry->base4 << 32));
        tss = (tss64_t *)tss_base;
        tss->iomap_base = sizeof(*tss);
        ioperm_bitmap = ((unsigned char *)tss+tss->iomap_base);

Re: [PATCH kvm-unit-tests 2/2] replace tss_descr global with a function

[Sean Christopherson]

+Jim again

On Wed, Oct 20, 2021, Paolo Bonzini wrote:
> @@ -255,4 +255,16 @@ static inline void *get_idt_addr(idt_entry_t *entry)
>  	return (void *)addr;
>  }
>  
> +extern gdt_desc_entry_t *get_tss_descr(void);
> +
> +static inline unsigned long get_gdt_entry_base(gdt_desc_entry_t *entry)

Bad naming again, base4 doesn't exist for segment descriptors.  It's a bit
wordy, but maybe this?

static inline unsigned long get_system_desc_base(gdt_system_desc_entry_t *entry)

> +{
> +	unsigned long base;
> +	base = entry->base1 | ((u32)entry->base2 << 16) | ((u32)entry->base3 << 24);
> +#ifdef __x86_64__
> +	base |= (u64)entry->base4 << 32;
> +#endif
> +	return base;
> +}
> +
>  #endif

Re: [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors

[Paolo Bonzini]

On 20/10/21 19:58, Sean Christopherson wrote:
> Changelog says "unify struct", yet gdt_entry_t and gdt_desc_entry_t have fully
> redundant information.  Apparently anonymous structs aren't a thing (or I'm just
> doing it wrong), but even so, fully unifying this is not hugely problematic for
> the sole consumer.

Right, the unification is more between 32- and 64 bits.

"Unifying" the reply to both patches, the best thing to do is:

- always use gdt_entry_t as the argument

- use your definition of gdt_desc_entry_t and call it 
gdt_system_desc_entry_t.

- read the type and, if needed, cast to gdt_system_desc_entry_t to "OR" 
in the base4 field.

Paolo

Re: [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors

[Jim Mattson]

+Aaron Lewis

On Wed, Oct 20, 2021 at 9:53 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Use the same names and definitions (apart from the high base field)
> for GDT descriptors.  gdt_entry_t is for 8-byte entries and
> gdt_desc_entry_t is for when 64-bit tests should use a 16-byte
> entry.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  lib/x86/desc.c         | 16 ++++++----------
>  lib/x86/desc.h         | 36 ++++++++++++++++++++++++++----------
>  x86/svm_tests.c        | 12 ++++++------
>  x86/taskswitch.c       |  2 +-
>  x86/vmware_backdoors.c |  8 ++++----
>  5 files changed, 43 insertions(+), 31 deletions(-)
>
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index e7378c1..3d38565 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -285,17 +285,13 @@ void set_gdt_entry(int sel, u32 base,  u32 limit, u8 access, u8 gran)
>         int num = sel >> 3;
>
>         /* Setup the descriptor base address */
> -       gdt32[num].base_low = (base & 0xFFFF);
> -       gdt32[num].base_middle = (base >> 16) & 0xFF;
> -       gdt32[num].base_high = (base >> 24) & 0xFF;
> +       gdt32[num].base1 = (base & 0xFFFF);
> +       gdt32[num].base2 = (base >> 16) & 0xFF;
> +       gdt32[num].base3 = (base >> 24) & 0xFF;
>
> -       /* Setup the descriptor limits */
> -       gdt32[num].limit_low = (limit & 0xFFFF);
> -       gdt32[num].granularity = ((limit >> 16) & 0x0F);
> -
> -       /* Finally, set up the granularity and access flags */
> -       gdt32[num].granularity |= (gran & 0xF0);
> -       gdt32[num].access = access;
> +       /* Setup the descriptor limits, granularity and flags */
> +       gdt32[num].limit1 = (limit & 0xFFFF);
> +       gdt32[num].type_limit_flags = ((limit & 0xF0000) >> 8) | ((gran & 0xF0) << 8) | access;
>  }
>
>  void set_gdt_task_gate(u16 sel, u16 tss_sel)
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index a6ffb38..3aa1eca 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -164,15 +164,28 @@ typedef struct {
>  } idt_entry_t;
>
>  typedef struct {
> -       u16 limit_low;
> -       u16 base_low;
> -       u8 base_middle;
> -       u8 access;
> -       u8 granularity;
> -       u8 base_high;
> -} gdt_entry_t;
> -
> -struct segment_desc64 {
> +       uint16_t limit1;
> +       uint16_t base1;
> +       uint8_t  base2;

These names are even more horrible than their predecessors. How about
something like:

limit_15_0
base_15_0
base_23_16

> +       union {
> +               uint16_t  type_limit_flags;      /* Type and limit flags */

/* Type, limit[19:16], and flags */

> +               struct {
> +                       uint16_t type:4;
> +                       uint16_t s:1;
> +                       uint16_t dpl:2;
> +                       uint16_t p:1;
> +                       uint16_t limit:4;

Perhaps limit_19_16?

> +                       uint16_t avl:1;
> +                       uint16_t l:1;
> +                       uint16_t db:1;
> +                       uint16_t g:1;
> +               } __attribute__((__packed__));
> +       } __attribute__((__packed__));
> +       uint8_t  base3;

base_31_24?

> +} __attribute__((__packed__)) gdt_entry_t;
> +

Why specify 'gdt'? Aren't ldt descriptors the same format?

> +#ifdef __x86_64__
> +typedef struct {
>         uint16_t limit1;
>         uint16_t base1;
>         uint8_t  base2;
> @@ -193,7 +206,10 @@ struct segment_desc64 {
>         uint8_t  base3;
>         uint32_t base4;
>         uint32_t zero;
> -} __attribute__((__packed__));
> +} __attribute__((__packed__)) gdt_desc_entry_t;
> +#else
> +typedef gdt_entry_t gdt_desc_entry_t;
> +#endif
>
>  #define DESC_BUSY ((uint64_t) 1 << 41)
>
> diff --git a/x86/svm_tests.c b/x86/svm_tests.c
> index afdd359..014fbbf 100644
> --- a/x86/svm_tests.c
> +++ b/x86/svm_tests.c
> @@ -1876,22 +1876,22 @@ 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;
> +    gdt_entry_t *gdt_table;
> +    gdt_entry_t *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_table = (gdt_entry_t *) gdt.base;
> +    tss_entry = &gdt_table[tr / sizeof(gdt_entry_t)];
> +    *((gdt_entry_t **)data) = tss_entry;
>  }
>
>  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);
> diff --git a/x86/taskswitch.c b/x86/taskswitch.c
> index 889831e..b6b3451 100644
> --- a/x86/taskswitch.c
> +++ b/x86/taskswitch.c
> @@ -21,7 +21,7 @@ fault_handler(unsigned long error_code)
>
>         tss.eip += 2;
>
> -       gdt32[TSS_MAIN / 8].access &= ~2;
> +       gdt32[TSS_MAIN / 8].type &= ~2;

Is this right? I got the impression that there was one TSS descriptor per CPU.

>
>         set_gdt_task_gate(TSS_RETURN, tss_intr.prev);
>  }
> diff --git a/x86/vmware_backdoors.c b/x86/vmware_backdoors.c
> index b4902a9..24870d7 100644
> --- a/x86/vmware_backdoors.c
> +++ b/x86/vmware_backdoors.c
> @@ -133,8 +133,8 @@ 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;
> +       gdt_entry_t *gdt_table;
> +       gdt_desc_entry_t *tss_entry;
>         u16 tr = 0;
>         tss64_t *tss;
>         unsigned char *ioperm_bitmap;
> @@ -142,8 +142,8 @@ static void set_tss_ioperm(void)
>
>         sgdt(&gdt);
>         tr = str();
> -       gdt_table = (struct segment_desc64 *) gdt.base;
> -       tss_entry = &gdt_table[tr / sizeof(struct segment_desc64)];
> +       gdt_table = (gdt_entry_t *) gdt.base;
> +       tss_entry = (gdt_desc_entry_t *) &gdt_table[tr / sizeof(gdt_entry_t)];
>         tss_base = ((uint64_t) tss_entry->base1 |
>                         ((uint64_t) tss_entry->base2 << 16) |
>                         ((uint64_t) tss_entry->base3 << 24) |

It seems like there should be a library function for reconstructing a
base address from a segment descriptor.

> --
> 2.27.0
>
>