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

[PATCH v2 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.

Paolo

v1->v2: correctly handle segment descriptors
	remove duplication between struct definitions

Paolo Bonzini (2):
  unify field names and definitions for GDT descriptors
  replace tss_descr global with a function

 lib/x86/desc.c         | 38 ++++++++++++++++++++++++++++----------
 lib/x86/desc.h         | 22 +++++++++++-----------
 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, 54 insertions(+), 55 deletions(-)

-- 
2.27.0

[PATCH v2 kvm-unit-tests 1/2] unify field names and definitions for GDT descriptors

[Paolo Bonzini]

Use the same names and definitions (apart from the high base field)
for GDT descriptors in both 32-bit and 64-bit code.  The next patch
will also reuse gdt_entry_t in the 16-byte struct definition, for now
some duplication remains.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 lib/x86/desc.c   | 16 ++++++----------
 lib/x86/desc.h   | 28 +++++++++++++++++++++-------
 x86/taskswitch.c |  2 +-
 3 files changed, 28 insertions(+), 18 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..0af37e3 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -164,14 +164,27 @@ 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;
+	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__
 struct segment_desc64 {
 	uint16_t limit1;
 	uint16_t base1;
@@ -194,6 +207,7 @@ struct segment_desc64 {
 	uint32_t base4;
 	uint32_t zero;
 } __attribute__((__packed__));
+#endif
 
 #define DESC_BUSY ((uint64_t) 1 << 41)
 
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);
 }
-- 
2.27.0

[PATCH v2 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         | 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);
 
 	vmcs_write(GUEST_AR_CS, 0xa09b);
 	vmcs_write(GUEST_AR_DS, 0xc093);
-- 
2.27.0

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

[Sean Christopherson]

On Wed, Oct 20, 2021, Paolo Bonzini wrote:
> +typedef struct {
> +	gdt_entry_t common;
>  	uint32_t base4;
>  	uint32_t zero;
> -} __attribute__((__packed__));
> +} __attribute__((__packed__)) gdt_entry16_t;

I doubt I am the only one that is going to misread this as "GDT entry in 16-bit
mode" without thinking critically about what "16" might mean.

Why not this, or some variant thereof?  The gdt64 part is a bit gross, but I
don't think it's likely to be misinterpreted.

	gdt64_system_entry_t;

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

[Jim Mattson]

+Aaron Lewis

On Wed, Oct 20, 2021 at 12:27 PM Paolo Bonzini <pbonzini@redhat.com> 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@redhat.com>
> ---
>  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
>

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

[Paolo Bonzini]

On 20/10/21 22:46, Jim Mattson wrote:
>> -       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?
> 

Yes, but this is just for L2.  The host TR limit is restored to 0x67 on 
every vmexit, and it seemed weird to run L1 and L2 with different limits.

Paolo

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

[Jim Mattson]

On Wed, Oct 20, 2021 at 2:12 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 20/10/21 22:46, Jim Mattson wrote:
> >> -       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?
> >
>
> Yes, but this is just for L2.  The host TR limit is restored to 0x67 on
> every vmexit, and it seemed weird to run L1 and L2 with different limits.

Perhaps you could change the limits in the GDT entries to match?

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

[Paolo Bonzini]

On 20/10/21 23:18, Jim Mattson wrote:
>>>> -       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?
>>>
>> Yes, but this is just for L2.  The host TR limit is restored to 0x67 on
>> every vmexit, and it seemed weird to run L1 and L2 with different limits.
> Perhaps you could change the limits in the GDT entries to match?

So keep it 0x67 and adjust it to the size of the IOPM in the VMware 
backdoor test?

Paolo

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

[Jim Mattson]

On Wed, Oct 20, 2021 at 2:31 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 20/10/21 23:18, Jim Mattson wrote:
> >>>> -       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?
> >>>
> >> Yes, but this is just for L2.  The host TR limit is restored to 0x67 on
> >> every vmexit, and it seemed weird to run L1 and L2 with different limits.
> > Perhaps you could change the limits in the GDT entries to match?
>
> So keep it 0x67 and adjust it to the size of the IOPM in the VMware
> backdoor test?

Right. That would seem to achieve the greatest consistency.

Re: [PATCH v2 kvm-unit-tests 1/2] unify field names and definitions for GDT descriptors

[Marc Orr]

On Wed, Oct 20, 2021 at 12:27 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Use the same names and definitions (apart from the high base field)
> for GDT descriptors in both 32-bit and 64-bit code.  The next patch
> will also reuse gdt_entry_t in the 16-byte struct definition, for now
> some duplication remains.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  lib/x86/desc.c   | 16 ++++++----------
>  lib/x86/desc.h   | 28 +++++++++++++++++++++-------
>  x86/taskswitch.c |  2 +-
>  3 files changed, 28 insertions(+), 18 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..0af37e3 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -164,14 +164,27 @@ 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;
> +       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__
>  struct segment_desc64 {
>         uint16_t limit1;
>         uint16_t base1;

I had been thinking of suggesting to use `gdt_entry_t` inline, within
the `segment_desc64` struct to avoid all of this redundancy. But I see
that's done in the next patch (which I haven't reviewed yet).

> @@ -194,6 +207,7 @@ struct segment_desc64 {
>         uint32_t base4;
>         uint32_t zero;
>  } __attribute__((__packed__));
> +#endif
>
>  #define DESC_BUSY ((uint64_t) 1 << 41)
>
> 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);
>  }
> --
> 2.27.0
>
>

Reviewed-by: Marc Orr <marcorr@google.com>

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

[Marc Orr]

On Wed, Oct 20, 2021 at 12:27 PM Paolo Bonzini <pbonzini@redhat.com> 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@redhat.com>
> ---
>  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@google.com>

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

[Paolo Bonzini]

On 21/10/21 00:01, Jim Mattson wrote:
> On Wed, Oct 20, 2021 at 2:31 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 20/10/21 23:18, Jim Mattson wrote:
>>>>>> -       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?
>>>>>
>>>> Yes, but this is just for L2.  The host TR limit is restored to 0x67 on
>>>> every vmexit, and it seemed weird to run L1 and L2 with different limits.
>>> Perhaps you could change the limits in the GDT entries to match?
>>
>> So keep it 0x67 and adjust it to the size of the IOPM in the VMware
>> backdoor test?
> 
> Right. That would seem to achieve the greatest consistency.
> 

Let's just add a get_gdt_entry_limit that takes into account the G bit too.

Paolo