* [PATCH kvm-unit-tests 0/2] remove tss_descr, replace with a function @ 2021-10-20 16:53 Paolo Bonzini 2021-10-20 16:53 ` [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors Paolo Bonzini 2021-10-20 16:53 ` [PATCH kvm-unit-tests 2/2] replace tss_descr global with a function Paolo Bonzini 0 siblings, 2 replies; 7+ messages in thread From: Paolo Bonzini @ 2021-10-20 16:53 UTC (permalink / raw) To: kvm; +Cc: zxwang42, marcorr, seanjc, jroedel, varad.gautam 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors 2021-10-20 16:53 [PATCH kvm-unit-tests 0/2] remove tss_descr, replace with a function Paolo Bonzini @ 2021-10-20 16:53 ` Paolo Bonzini 2021-10-20 17:58 ` Sean Christopherson 2021-10-20 20:05 ` Jim Mattson 2021-10-20 16:53 ` [PATCH kvm-unit-tests 2/2] replace tss_descr global with a function Paolo Bonzini 1 sibling, 2 replies; 7+ messages in thread From: Paolo Bonzini @ 2021-10-20 16:53 UTC (permalink / raw) To: kvm; +Cc: zxwang42, marcorr, seanjc, jroedel, varad.gautam 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors 2021-10-20 16:53 ` [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors Paolo Bonzini @ 2021-10-20 17:58 ` Sean Christopherson 2021-10-20 19:10 ` Paolo Bonzini 2021-10-20 20:05 ` Jim Mattson 1 sibling, 1 reply; 7+ messages in thread From: Sean Christopherson @ 2021-10-20 17:58 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, zxwang42, marcorr, jroedel, varad.gautam, Jim Mattson +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); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors 2021-10-20 17:58 ` Sean Christopherson @ 2021-10-20 19:10 ` Paolo Bonzini 0 siblings, 0 replies; 7+ messages in thread From: Paolo Bonzini @ 2021-10-20 19:10 UTC (permalink / raw) To: Sean Christopherson Cc: kvm, zxwang42, marcorr, jroedel, varad.gautam, Jim Mattson 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors 2021-10-20 16:53 ` [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors Paolo Bonzini 2021-10-20 17:58 ` Sean Christopherson @ 2021-10-20 20:05 ` Jim Mattson 1 sibling, 0 replies; 7+ messages in thread From: Jim Mattson @ 2021-10-20 20:05 UTC (permalink / raw) To: Paolo Bonzini, Aaron Lewis Cc: kvm, zxwang42, marcorr, seanjc, jroedel, varad.gautam +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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH kvm-unit-tests 2/2] replace tss_descr global with a function 2021-10-20 16:53 [PATCH kvm-unit-tests 0/2] remove tss_descr, replace with a function Paolo Bonzini 2021-10-20 16:53 ` [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors Paolo Bonzini @ 2021-10-20 16:53 ` Paolo Bonzini 2021-10-20 18:01 ` Sean Christopherson 1 sibling, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2021-10-20 16:53 UTC (permalink / raw) To: kvm; +Cc: zxwang42, marcorr, seanjc, jroedel, varad.gautam 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH kvm-unit-tests 2/2] replace tss_descr global with a function 2021-10-20 16:53 ` [PATCH kvm-unit-tests 2/2] replace tss_descr global with a function Paolo Bonzini @ 2021-10-20 18:01 ` Sean Christopherson 0 siblings, 0 replies; 7+ messages in thread From: Sean Christopherson @ 2021-10-20 18:01 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, zxwang42, marcorr, jroedel, varad.gautam, Jim Mattson +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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-20 20:06 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-20 16:53 [PATCH kvm-unit-tests 0/2] remove tss_descr, replace with a function Paolo Bonzini 2021-10-20 16:53 ` [PATCH kvm-unit-tests 1/2] unify structs for GDT descriptors Paolo Bonzini 2021-10-20 17:58 ` Sean Christopherson 2021-10-20 19:10 ` Paolo Bonzini 2021-10-20 20:05 ` Jim Mattson 2021-10-20 16:53 ` [PATCH kvm-unit-tests 2/2] replace tss_descr global with a function Paolo Bonzini 2021-10-20 18:01 ` Sean Christopherson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.