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

[Sean Christopherson <seanjc@google.com>]

+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);