* [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers @ 2020-03-10 22:51 Krish Sadhukhan 2020-03-10 22:51 ` [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests Krish Sadhukhan 2020-03-10 23:51 ` [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Jim Mattson 0 siblings, 2 replies; 16+ messages in thread From: Krish Sadhukhan @ 2020-03-10 22:51 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson Even thought today's x86 hardware uses paging and not segmentation for memory management, it is still good to have some tests that can verify the sanity of the segment register fields on vmentry of nested guests. The test on SS Selector field is failing because the hardware (I am using Intel Xeon Platinum 8167M 2.00GHz) doesn't raise any error even if the prescribed bit pattern is not set and as a result vmentry succeeds. [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment lib/x86/processor.h | 1 + x86/vmx_tests.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) Krish Sadhukhan (1): nVMX: Test Selector and Base Address fields of Guest Segment Registers on ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-10 22:51 [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Krish Sadhukhan @ 2020-03-10 22:51 ` Krish Sadhukhan 2020-03-11 15:05 ` Sean Christopherson 2020-03-10 23:51 ` [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Jim Mattson 1 sibling, 1 reply; 16+ messages in thread From: Krish Sadhukhan @ 2020-03-10 22:51 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson According to section "Checks on Guest Segment Registers" in Intel SDM vol 3C, the following checks are performed on the Guest Segment Registers on vmentry of nested guests: Selector fields: — TR. The TI flag (bit 2) must be 0. — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0. — SS. If the guest will not be virtual-8086 and the "unrestricted guest" VM-execution control is 0, the RPL (bits 1:0) must equal the RPL of the selector field for CS.1 Base-address fields: — CS, SS, DS, ES, FS, GS. If the guest will be virtual-8086, the address must be the selector field shifted left 4 bits (multiplied by 16). — The following checks are performed on processors that support Intel 64 architecture: TR, FS, GS. The address must be canonical. LDTR. If LDTR is usable, the address must be canonical. CS. Bits 63:32 of the address must be zero. SS, DS, ES. If the register is usable, bits 63:32 of the address must be zero. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> --- lib/x86/processor.h | 1 + x86/vmx_tests.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/lib/x86/processor.h b/lib/x86/processor.h index 03fdf64..3642212 100644 --- a/lib/x86/processor.h +++ b/lib/x86/processor.h @@ -57,6 +57,7 @@ #define X86_EFLAGS_OF 0x00000800 #define X86_EFLAGS_IOPL 0x00003000 #define X86_EFLAGS_NT 0x00004000 +#define X86_EFLAGS_VM 0x00020000 #define X86_EFLAGS_AC 0x00040000 #define X86_EFLAGS_ALU (X86_EFLAGS_CF | X86_EFLAGS_PF | X86_EFLAGS_AF | \ diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index a7abd63..5e96dfa 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -7681,6 +7681,113 @@ static void test_load_guest_pat(void) test_pat(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT); } +#define GUEST_SEG_USABLE_MASK 1u << 16 +#define TEST_SEGMENT_SEL(seg_sel, seg_sel_name, val, val_saved) \ + vmcs_write(seg_sel, val); \ + enter_guest_with_invalid_guest_state(); \ + report_guest_state_test(seg_sel_name, \ + VMX_ENTRY_FAILURE | \ + VMX_FAIL_STATE, \ + val, seg_sel_name); \ + vmcs_write(seg_sel, val_saved); + +/* + * The following checks are done on the Selector field of the Guest Segment + * Registers: + * — TR. The TI flag (bit 2) must be 0. + * — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0. + * — SS. If the guest will not be virtual-8086 and the "unrestricted + * guest" VM-execution control is 0, the RPL (bits 1:0) must equal + * the RPL of the selector field for CS. + * + * [Intel SDM] + */ +static void test_guest_segment_sel_fields(void) +{ + u16 sel_saved; + u16 sel; + + sel_saved = vmcs_read(GUEST_SEL_TR); + sel = sel_saved | 0x4; + TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved); + + sel_saved = vmcs_read(GUEST_SEL_LDTR); + sel = sel_saved | 0x4; + TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved); + + if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) && + !(vmcs_read(CPU_SECONDARY) & CPU_URG)) { + u16 cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3; + sel_saved = vmcs_read(GUEST_SEL_SS); + sel = sel_saved | (~cs_rpl_bits & 0x3); + TEST_SEGMENT_SEL(GUEST_SEL_SS, "GUEST_SEL_SS", sel, sel_saved); + } +} + +#define TEST_SEGMENT_BASE_ADDR_UPPER_BITS(seg_base, seg_base_name) \ + addr_saved = vmcs_read(seg_base); \ + for (i = 32; i < 63; i = i + 4) { \ + addr = addr_saved | 1ull << i; \ + vmcs_write(seg_base, addr); \ + enter_guest_with_invalid_guest_state(); \ + report_guest_state_test(seg_base_name, \ + VMX_ENTRY_FAILURE | \ + VMX_FAIL_STATE, \ + addr, seg_base_name); \ + } \ + \ + vmcs_write(seg_base, addr_saved); + +#define TEST_SEGMENT_BASE_ADDR_CANONICAL(seg_base, seg_base_name) \ + addr_saved = vmcs_read(seg_base); \ + vmcs_write(seg_base, NONCANONICAL); \ + enter_guest_with_invalid_guest_state(); \ + report_guest_state_test(seg_base_name, \ + VMX_ENTRY_FAILURE | VMX_FAIL_STATE, \ + NONCANONICAL, seg_base_name); \ + vmcs_write(seg_base, addr_saved); + +/* + * The following checks are done on the Base Address field of the Guest + * Segment Registers on processors that support Intel 64 architecture: + * - TR, FS, GS : The address must be canonical. + * - LDTR : If LDTR is usable, the address must be canonical. + * - CS : Bits 63:32 of the address must be zero. + * - SS, DS, ES : If the register is usable, bits 63:32 of the address + * must be zero. + * + * [Intel SDM] + */ +static void test_guest_segment_base_addr_fields(void) +{ + u64 addr_saved, addr; + int i; + + /* + * The address of TR, FS, GS and LDTR must be canonical. + */ + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_TR, "GUEST_BASE_TR"); + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_FS, "GUEST_BASE_FS"); + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_GS, "GUEST_BASE_GS"); + if (!(vmcs_read(GUEST_AR_LDTR) & GUEST_SEG_USABLE_MASK)) + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_LDTR, + "GUEST_BASE_LDTR"); + + /* + * Bits 63:32 in CS, SS, DS and ES base address must be zero + */ + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_CS, "GUEST_BASE_CS"); + if (!(vmcs_read(GUEST_AR_SS) & GUEST_SEG_USABLE_MASK)) + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_SS, + "GUEST_BASE_SS"); + if (!(vmcs_read(GUEST_AR_DS) & GUEST_SEG_USABLE_MASK)) + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_DS, + "GUEST_BASE_DS"); + if (!(vmcs_read(GUEST_AR_ES) & GUEST_SEG_USABLE_MASK)) + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_ES, + "GUEST_BASE_ES"); +} + /* * Check that the virtual CPU checks the VMX Guest State Area as * documented in the Intel SDM. @@ -7701,6 +7808,8 @@ static void vmx_guest_state_area_test(void) test_load_guest_pat(); test_guest_efer(); test_load_guest_perf_global_ctrl(); + test_guest_segment_sel_fields(); + test_guest_segment_base_addr_fields(); /* * Let the guest finish execution -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-10 22:51 ` [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests Krish Sadhukhan @ 2020-03-11 15:05 ` Sean Christopherson 2020-03-11 15:19 ` Sean Christopherson 2020-03-11 20:38 ` Krish Sadhukhan 0 siblings, 2 replies; 16+ messages in thread From: Sean Christopherson @ 2020-03-11 15:05 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: kvm, pbonzini, jmattson On Tue, Mar 10, 2020 at 06:51:49PM -0400, Krish Sadhukhan wrote: > According to section "Checks on Guest Segment Registers" in Intel SDM vol 3C, > the following checks are performed on the Guest Segment Registers on vmentry > of nested guests: > > Selector fields: > — TR. The TI flag (bit 2) must be 0. > — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0. > — SS. If the guest will not be virtual-8086 and the "unrestricted > guest" VM-execution control is 0, the RPL (bits 1:0) must equal > the RPL of the selector field for CS.1 > > Base-address fields: > — CS, SS, DS, ES, FS, GS. If the guest will be virtual-8086, the > address must be the selector field shifted left 4 bits (multiplied > by 16). > — The following checks are performed on processors that support Intel > 64 architecture: > TR, FS, GS. The address must be canonical. > LDTR. If LDTR is usable, the address must be canonical. > CS. Bits 63:32 of the address must be zero. > SS, DS, ES. If the register is usable, bits 63:32 of the > address must be zero. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > --- > lib/x86/processor.h | 1 + > x86/vmx_tests.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 110 insertions(+) > > diff --git a/lib/x86/processor.h b/lib/x86/processor.h > index 03fdf64..3642212 100644 > --- a/lib/x86/processor.h > +++ b/lib/x86/processor.h > @@ -57,6 +57,7 @@ > #define X86_EFLAGS_OF 0x00000800 > #define X86_EFLAGS_IOPL 0x00003000 > #define X86_EFLAGS_NT 0x00004000 > +#define X86_EFLAGS_VM 0x00020000 > #define X86_EFLAGS_AC 0x00040000 > > #define X86_EFLAGS_ALU (X86_EFLAGS_CF | X86_EFLAGS_PF | X86_EFLAGS_AF | \ > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index a7abd63..5e96dfa 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -7681,6 +7681,113 @@ static void test_load_guest_pat(void) > test_pat(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT); > } > > +#define GUEST_SEG_USABLE_MASK 1u << 16 s/USABLE/UNUSABLE The usage below looks correct, just the name is inverted. > +#define TEST_SEGMENT_SEL(seg_sel, seg_sel_name, val, val_saved) \ > + vmcs_write(seg_sel, val); \ > + enter_guest_with_invalid_guest_state(); \ > + report_guest_state_test(seg_sel_name, \ > + VMX_ENTRY_FAILURE | \ > + VMX_FAIL_STATE, \ > + val, seg_sel_name); \ > + vmcs_write(seg_sel, val_saved); > + > +/* > + * The following checks are done on the Selector field of the Guest Segment > + * Registers: > + * — TR. The TI flag (bit 2) must be 0. > + * — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0. > + * — SS. If the guest will not be virtual-8086 and the "unrestricted > + * guest" VM-execution control is 0, the RPL (bits 1:0) must equal > + * the RPL of the selector field for CS. > + * > + * [Intel SDM] > + */ > +static void test_guest_segment_sel_fields(void) > +{ > + u16 sel_saved; > + u16 sel; > + > + sel_saved = vmcs_read(GUEST_SEL_TR); > + sel = sel_saved | 0x4; > + TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved); > + > + sel_saved = vmcs_read(GUEST_SEL_LDTR); > + sel = sel_saved | 0x4; > + TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved); > + > + if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) && > + !(vmcs_read(CPU_SECONDARY) & CPU_URG)) { Rather than react to the environment, these tests should configure every relevant aspect and ignore the ones it can't change. E.g. the unit tests aren't going to randomly launch a vm86 guest. Ditto for the unusuable bit, it's unlikely to be set for most segments and would be something to test explicitly. > + u16 cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3; > + sel_saved = vmcs_read(GUEST_SEL_SS); > + sel = sel_saved | (~cs_rpl_bits & 0x3); > + TEST_SEGMENT_SEL(GUEST_SEL_SS, "GUEST_SEL_SS", sel, sel_saved); > + } > +} > + > +#define TEST_SEGMENT_BASE_ADDR_UPPER_BITS(seg_base, seg_base_name) \ > + addr_saved = vmcs_read(seg_base); \ > + for (i = 32; i < 63; i = i + 4) { \ > + addr = addr_saved | 1ull << i; \ > + vmcs_write(seg_base, addr); \ > + enter_guest_with_invalid_guest_state(); \ > + report_guest_state_test(seg_base_name, \ > + VMX_ENTRY_FAILURE | \ > + VMX_FAIL_STATE, \ > + addr, seg_base_name); \ > + } \ > + \ > + vmcs_write(seg_base, addr_saved); > + > +#define TEST_SEGMENT_BASE_ADDR_CANONICAL(seg_base, seg_base_name) \ > + addr_saved = vmcs_read(seg_base); \ > + vmcs_write(seg_base, NONCANONICAL); \ > + enter_guest_with_invalid_guest_state(); \ > + report_guest_state_test(seg_base_name, \ > + VMX_ENTRY_FAILURE | VMX_FAIL_STATE, \ > + NONCANONICAL, seg_base_name); \ > + vmcs_write(seg_base, addr_saved); > + > +/* > + * The following checks are done on the Base Address field of the Guest > + * Segment Registers on processors that support Intel 64 architecture: > + * - TR, FS, GS : The address must be canonical. > + * - LDTR : If LDTR is usable, the address must be canonical. > + * - CS : Bits 63:32 of the address must be zero. > + * - SS, DS, ES : If the register is usable, bits 63:32 of the address > + * must be zero. > + * > + * [Intel SDM] > + */ > +static void test_guest_segment_base_addr_fields(void) > +{ > + u64 addr_saved, addr; > + int i; > + > + /* > + * The address of TR, FS, GS and LDTR must be canonical. > + */ > + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_TR, "GUEST_BASE_TR"); > + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_FS, "GUEST_BASE_FS"); > + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_GS, "GUEST_BASE_GS"); FS/GS bases aren't checked if the segment is unusable. > + if (!(vmcs_read(GUEST_AR_LDTR) & GUEST_SEG_USABLE_MASK)) > + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_LDTR, > + "GUEST_BASE_LDTR"); > + > + /* > + * Bits 63:32 in CS, SS, DS and ES base address must be zero > + */ > + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_CS, "GUEST_BASE_CS"); > + if (!(vmcs_read(GUEST_AR_SS) & GUEST_SEG_USABLE_MASK)) > + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_SS, > + "GUEST_BASE_SS"); > + if (!(vmcs_read(GUEST_AR_DS) & GUEST_SEG_USABLE_MASK)) > + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_DS, > + "GUEST_BASE_DS"); > + if (!(vmcs_read(GUEST_AR_ES) & GUEST_SEG_USABLE_MASK)) > + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_ES, > + "GUEST_BASE_ES"); > +} > + > /* > * Check that the virtual CPU checks the VMX Guest State Area as > * documented in the Intel SDM. > @@ -7701,6 +7808,8 @@ static void vmx_guest_state_area_test(void) > test_load_guest_pat(); > test_guest_efer(); > test_load_guest_perf_global_ctrl(); > + test_guest_segment_sel_fields(); > + test_guest_segment_base_addr_fields(); > > /* > * Let the guest finish execution > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 15:05 ` Sean Christopherson @ 2020-03-11 15:19 ` Sean Christopherson 2020-03-11 20:38 ` Krish Sadhukhan 1 sibling, 0 replies; 16+ messages in thread From: Sean Christopherson @ 2020-03-11 15:19 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: kvm, pbonzini, jmattson On Wed, Mar 11, 2020 at 08:05:16AM -0700, Sean Christopherson wrote: > On Tue, Mar 10, 2020 at 06:51:49PM -0400, Krish Sadhukhan wrote: > > + /* > > + * The address of TR, FS, GS and LDTR must be canonical. > > + */ > > + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_TR, "GUEST_BASE_TR"); > > + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_FS, "GUEST_BASE_FS"); > > + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_GS, "GUEST_BASE_GS"); > > FS/GS bases aren't checked if the segment is unusable. Ah, I stand corrected, I misread the section on loading guest segs. There is an exception clause for FS/GS base inside the unusuable path. And the "checks on guest segments" clearly states FS/GS base are unconditionally checked. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 15:05 ` Sean Christopherson 2020-03-11 15:19 ` Sean Christopherson @ 2020-03-11 20:38 ` Krish Sadhukhan 2020-03-11 21:46 ` Sean Christopherson 1 sibling, 1 reply; 16+ messages in thread From: Krish Sadhukhan @ 2020-03-11 20:38 UTC (permalink / raw) To: Sean Christopherson; +Cc: kvm, pbonzini, jmattson On 3/11/20 8:05 AM, Sean Christopherson wrote: > On Tue, Mar 10, 2020 at 06:51:49PM -0400, Krish Sadhukhan wrote: >> According to section "Checks on Guest Segment Registers" in Intel SDM vol 3C, >> the following checks are performed on the Guest Segment Registers on vmentry >> of nested guests: >> >> Selector fields: >> — TR. The TI flag (bit 2) must be 0. >> — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0. >> — SS. If the guest will not be virtual-8086 and the "unrestricted >> guest" VM-execution control is 0, the RPL (bits 1:0) must equal >> the RPL of the selector field for CS.1 >> >> Base-address fields: >> — CS, SS, DS, ES, FS, GS. If the guest will be virtual-8086, the >> address must be the selector field shifted left 4 bits (multiplied >> by 16). >> — The following checks are performed on processors that support Intel >> 64 architecture: >> TR, FS, GS. The address must be canonical. >> LDTR. If LDTR is usable, the address must be canonical. >> CS. Bits 63:32 of the address must be zero. >> SS, DS, ES. If the register is usable, bits 63:32 of the >> address must be zero. >> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >> --- >> lib/x86/processor.h | 1 + >> x86/vmx_tests.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 110 insertions(+) >> >> diff --git a/lib/x86/processor.h b/lib/x86/processor.h >> index 03fdf64..3642212 100644 >> --- a/lib/x86/processor.h >> +++ b/lib/x86/processor.h >> @@ -57,6 +57,7 @@ >> #define X86_EFLAGS_OF 0x00000800 >> #define X86_EFLAGS_IOPL 0x00003000 >> #define X86_EFLAGS_NT 0x00004000 >> +#define X86_EFLAGS_VM 0x00020000 >> #define X86_EFLAGS_AC 0x00040000 >> >> #define X86_EFLAGS_ALU (X86_EFLAGS_CF | X86_EFLAGS_PF | X86_EFLAGS_AF | \ >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >> index a7abd63..5e96dfa 100644 >> --- a/x86/vmx_tests.c >> +++ b/x86/vmx_tests.c >> @@ -7681,6 +7681,113 @@ static void test_load_guest_pat(void) >> test_pat(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT); >> } >> >> +#define GUEST_SEG_USABLE_MASK 1u << 16 > s/USABLE/UNUSABLE > > The usage below looks correct, just the name is inverted. > >> +#define TEST_SEGMENT_SEL(seg_sel, seg_sel_name, val, val_saved) \ >> + vmcs_write(seg_sel, val); \ >> + enter_guest_with_invalid_guest_state(); \ >> + report_guest_state_test(seg_sel_name, \ >> + VMX_ENTRY_FAILURE | \ >> + VMX_FAIL_STATE, \ >> + val, seg_sel_name); \ >> + vmcs_write(seg_sel, val_saved); >> + >> +/* >> + * The following checks are done on the Selector field of the Guest Segment >> + * Registers: >> + * — TR. The TI flag (bit 2) must be 0. >> + * — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0. >> + * — SS. If the guest will not be virtual-8086 and the "unrestricted >> + * guest" VM-execution control is 0, the RPL (bits 1:0) must equal >> + * the RPL of the selector field for CS. >> + * >> + * [Intel SDM] >> + */ >> +static void test_guest_segment_sel_fields(void) >> +{ >> + u16 sel_saved; >> + u16 sel; >> + >> + sel_saved = vmcs_read(GUEST_SEL_TR); >> + sel = sel_saved | 0x4; >> + TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved); >> + >> + sel_saved = vmcs_read(GUEST_SEL_LDTR); >> + sel = sel_saved | 0x4; >> + TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved); >> + >> + if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) && >> + !(vmcs_read(CPU_SECONDARY) & CPU_URG)) { > Rather than react to the environment, these tests should configure every > relevant aspect and ignore the ones it can't change. E.g. the unit tests > aren't going to randomly launch a vm86 guest. Ditto for the unusuable bit, > it's unlikely to be set for most segments and would be something to test > explicitly. Just wanted to clarify on the "unusable bit" part of your comment. Do you mean each of the segment register checks from the SDM should have two tests, one with the "unusable bit" set and the other with that bit not set, irrespective of the checks being conditional on the setting of that bit ? > >> + u16 cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3; >> + sel_saved = vmcs_read(GUEST_SEL_SS); >> + sel = sel_saved | (~cs_rpl_bits & 0x3); >> + TEST_SEGMENT_SEL(GUEST_SEL_SS, "GUEST_SEL_SS", sel, sel_saved); >> + } >> +} >> + >> +#define TEST_SEGMENT_BASE_ADDR_UPPER_BITS(seg_base, seg_base_name) \ >> + addr_saved = vmcs_read(seg_base); \ >> + for (i = 32; i < 63; i = i + 4) { \ >> + addr = addr_saved | 1ull << i; \ >> + vmcs_write(seg_base, addr); \ >> + enter_guest_with_invalid_guest_state(); \ >> + report_guest_state_test(seg_base_name, \ >> + VMX_ENTRY_FAILURE | \ >> + VMX_FAIL_STATE, \ >> + addr, seg_base_name); \ >> + } \ >> + \ >> + vmcs_write(seg_base, addr_saved); >> + >> +#define TEST_SEGMENT_BASE_ADDR_CANONICAL(seg_base, seg_base_name) \ >> + addr_saved = vmcs_read(seg_base); \ >> + vmcs_write(seg_base, NONCANONICAL); \ >> + enter_guest_with_invalid_guest_state(); \ >> + report_guest_state_test(seg_base_name, \ >> + VMX_ENTRY_FAILURE | VMX_FAIL_STATE, \ >> + NONCANONICAL, seg_base_name); \ >> + vmcs_write(seg_base, addr_saved); >> + >> +/* >> + * The following checks are done on the Base Address field of the Guest >> + * Segment Registers on processors that support Intel 64 architecture: >> + * - TR, FS, GS : The address must be canonical. >> + * - LDTR : If LDTR is usable, the address must be canonical. >> + * - CS : Bits 63:32 of the address must be zero. >> + * - SS, DS, ES : If the register is usable, bits 63:32 of the address >> + * must be zero. >> + * >> + * [Intel SDM] >> + */ >> +static void test_guest_segment_base_addr_fields(void) >> +{ >> + u64 addr_saved, addr; >> + int i; >> + >> + /* >> + * The address of TR, FS, GS and LDTR must be canonical. >> + */ >> + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_TR, "GUEST_BASE_TR"); >> + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_FS, "GUEST_BASE_FS"); >> + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_GS, "GUEST_BASE_GS"); > FS/GS bases aren't checked if the segment is unusable. > >> + if (!(vmcs_read(GUEST_AR_LDTR) & GUEST_SEG_USABLE_MASK)) >> + TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_LDTR, >> + "GUEST_BASE_LDTR"); >> + >> + /* >> + * Bits 63:32 in CS, SS, DS and ES base address must be zero >> + */ >> + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_CS, "GUEST_BASE_CS"); >> + if (!(vmcs_read(GUEST_AR_SS) & GUEST_SEG_USABLE_MASK)) >> + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_SS, >> + "GUEST_BASE_SS"); >> + if (!(vmcs_read(GUEST_AR_DS) & GUEST_SEG_USABLE_MASK)) >> + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_DS, >> + "GUEST_BASE_DS"); >> + if (!(vmcs_read(GUEST_AR_ES) & GUEST_SEG_USABLE_MASK)) >> + TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_ES, >> + "GUEST_BASE_ES"); >> +} >> + >> /* >> * Check that the virtual CPU checks the VMX Guest State Area as >> * documented in the Intel SDM. >> @@ -7701,6 +7808,8 @@ static void vmx_guest_state_area_test(void) >> test_load_guest_pat(); >> test_guest_efer(); >> test_load_guest_perf_global_ctrl(); >> + test_guest_segment_sel_fields(); >> + test_guest_segment_base_addr_fields(); >> >> /* >> * Let the guest finish execution >> -- >> 1.8.3.1 >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 20:38 ` Krish Sadhukhan @ 2020-03-11 21:46 ` Sean Christopherson 2020-03-11 21:53 ` Nadav Amit 0 siblings, 1 reply; 16+ messages in thread From: Sean Christopherson @ 2020-03-11 21:46 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: kvm, pbonzini, jmattson On Wed, Mar 11, 2020 at 01:38:24PM -0700, Krish Sadhukhan wrote: > > On 3/11/20 8:05 AM, Sean Christopherson wrote: > >>+static void test_guest_segment_sel_fields(void) > >>+{ > >>+ u16 sel_saved; > >>+ u16 sel; > >>+ > >>+ sel_saved = vmcs_read(GUEST_SEL_TR); > >>+ sel = sel_saved | 0x4; > >>+ TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved); > >>+ > >>+ sel_saved = vmcs_read(GUEST_SEL_LDTR); > >>+ sel = sel_saved | 0x4; > >>+ TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved); > >>+ > >>+ if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) && > >>+ !(vmcs_read(CPU_SECONDARY) & CPU_URG)) { > >Rather than react to the environment, these tests should configure every > >relevant aspect and ignore the ones it can't change. E.g. the unit tests > >aren't going to randomly launch a vm86 guest. Ditto for the unusuable bit, > >it's unlikely to be set for most segments and would be something to test > >explicitly. > > > Just wanted to clarify on the "unusable bit" part of your comment. Do you > mean each of the segment register checks from the SDM should have two tests, > one with the "unusable bit" set and the other with that bit not set, > irrespective of the checks being conditional on the setting of that bit ? Sort of. In an ideal world, kvm-unit-tests would verify correctness of KVM for both unusable=1 and unusable=0. But, the unusable=1 validation space is enormous, i.e. there are a bazillion combinations of random garbage that can be thrown into GUEST_*S_{SE,ARBYTE,BASE}. So yeah, it could be as simple as running the same test as unusable=0, but expecting VM-Entry to succeed. That being said, I don't understand the motivation for these tests. KVM doesn't have any dedicated logic for checking guest segments, i.e. these tests are validating hardware behavior, not KVM behavior. The validation resources thrown at hardware dwarf what kvm-unit-tests can do, i.e. the odds of finding a silicon bug are tiny, and the odds of such a bug being exploitable aginst L0 are downright miniscule. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 21:46 ` Sean Christopherson @ 2020-03-11 21:53 ` Nadav Amit 2020-03-11 22:54 ` Liran Alon 0 siblings, 1 reply; 16+ messages in thread From: Nadav Amit @ 2020-03-11 21:53 UTC (permalink / raw) To: Sean Christopherson; +Cc: Krish Sadhukhan, kvm list, Paolo Bonzini, jmattson > On Mar 11, 2020, at 2:46 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Wed, Mar 11, 2020 at 01:38:24PM -0700, Krish Sadhukhan wrote: >> On 3/11/20 8:05 AM, Sean Christopherson wrote: >>>> +static void test_guest_segment_sel_fields(void) >>>> +{ >>>> + u16 sel_saved; >>>> + u16 sel; >>>> + >>>> + sel_saved = vmcs_read(GUEST_SEL_TR); >>>> + sel = sel_saved | 0x4; >>>> + TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved); >>>> + >>>> + sel_saved = vmcs_read(GUEST_SEL_LDTR); >>>> + sel = sel_saved | 0x4; >>>> + TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved); >>>> + >>>> + if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) && >>>> + !(vmcs_read(CPU_SECONDARY) & CPU_URG)) { >>> Rather than react to the environment, these tests should configure every >>> relevant aspect and ignore the ones it can't change. E.g. the unit tests >>> aren't going to randomly launch a vm86 guest. Ditto for the unusuable bit, >>> it's unlikely to be set for most segments and would be something to test >>> explicitly. >> >> >> Just wanted to clarify on the "unusable bit" part of your comment. Do you >> mean each of the segment register checks from the SDM should have two tests, >> one with the "unusable bit" set and the other with that bit not set, >> irrespective of the checks being conditional on the setting of that bit ? > > Sort of. In an ideal world, kvm-unit-tests would verify correctness of KVM > for both unusable=1 and unusable=0. But, the unusable=1 validation space is > enormous, i.e. there are a bazillion combinations of random garbage that can > be thrown into GUEST_*S_{SE,ARBYTE,BASE}. So yeah, it could be as simple as > running the same test as unusable=0, but expecting VM-Entry to succeed. > > That being said, I don't understand the motivation for these tests. KVM > doesn't have any dedicated logic for checking guest segments, i.e. these > tests are validating hardware behavior, not KVM behavior. The validation > resources thrown at hardware dwarf what kvm-unit-tests can do, i.e. the > odds of finding a silicon bug are tiny, and the odds of such a bug being > exploitable aginst L0 are downright miniscule. I see no reason for not including such tests. Liran said he uses kvm-unit-test with WHPX, and I also use it in some non-KVM setups. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 21:53 ` Nadav Amit @ 2020-03-11 22:54 ` Liran Alon 2020-03-11 23:12 ` Sean Christopherson 0 siblings, 1 reply; 16+ messages in thread From: Liran Alon @ 2020-03-11 22:54 UTC (permalink / raw) To: Nadav Amit, Sean Christopherson Cc: Krish Sadhukhan, kvm list, Paolo Bonzini, jmattson On 11/03/2020 23:53, Nadav Amit wrote: >> On Mar 11, 2020, at 2:46 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: >> >> On Wed, Mar 11, 2020 at 01:38:24PM -0700, Krish Sadhukhan wrote: >> >> That being said, I don't understand the motivation for these tests. KVM >> doesn't have any dedicated logic for checking guest segments, i.e. these >> tests are validating hardware behavior, not KVM behavior. The validation >> resources thrown at hardware dwarf what kvm-unit-tests can do, i.e. the >> odds of finding a silicon bug are tiny, and the odds of such a bug being >> exploitable aginst L0 are downright miniscule. > I see no reason for not including such tests. Liran said he uses > kvm-unit-test with WHPX, and I also use it in some non-KVM setups. +1. I admit I haven't read this thread at all but I wanted to point out something I already told Paolo at KVM Forum: kvm-unit-tests should be renamed to cpu-unit-tests. i.e. It should be treated as a suite of unit-tests that verifies CPU behavior. It doesn't matter if the CPU it runs on top of is Bare-Metal (As Nadav runs it) or on top of a vCPU implementation (E.g. KVM, VirtualBox, VMware, Hyper-V, whatever). There are multiple reasons of why it should be treated like that: * It allows us to verify that the unit-test indeed pass on a real CPU and thus enforce on vCPU implementation a behavior that a real CPU performs. Instead of what the unit-test author thought the CPU should perform. I have personally already made a mistake on this area when I wrote the unit-test to verify KVM handling of INIT signal while vCPU is in VMX root-mode. Nadav run the test on top of a Bare-Metal CPU and showed that the unit-test fails and therefore should written otherwise. * It allows, hopefully, for multiple hypervisor vendors to collaborate on the same set of unit-tests. Sharing regression tests and helping each other enforce correct vCPU behavior. As Nadav have pointed out, I have used this technique already to run the unit-tests on top of Hyper-V using WHPX. Which indeed revealed bugs in Hyper-V. Of course it was best if Intel would have shared their unit-tests for CPU functionality (Sean? I'm looking at you :P), but I am not aware that they did. So cpu-unit-tests (Previously named kvm-unit-tests) should be our best replacement for now. This is also why I would wish that we will make sure that all unit-tests enforcing CPU functionality will be written in cpu-unit-tests and not on KVM selftests. Only tests involving using KVM userspace API should be written in KVM selftests suite. -Liran ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 22:54 ` Liran Alon @ 2020-03-11 23:12 ` Sean Christopherson 2020-03-11 23:21 ` Nadav Amit 2020-03-11 23:22 ` Liran Alon 0 siblings, 2 replies; 16+ messages in thread From: Sean Christopherson @ 2020-03-11 23:12 UTC (permalink / raw) To: Liran Alon; +Cc: Nadav Amit, Krish Sadhukhan, kvm list, Paolo Bonzini, jmattson On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote: > Of course it was best if Intel would have shared their unit-tests for CPU > functionality (Sean? I'm looking at you :P), but I am not aware that they > did. Only in my dreams :-) I would love to open source some of Intel's validation tools so that they could be adapted to hammer KVM, but it'll never happen for a variety of reasons. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 23:12 ` Sean Christopherson @ 2020-03-11 23:21 ` Nadav Amit 2020-03-11 23:25 ` Sean Christopherson 2020-03-11 23:22 ` Liran Alon 1 sibling, 1 reply; 16+ messages in thread From: Nadav Amit @ 2020-03-11 23:21 UTC (permalink / raw) To: Sean Christopherson, Liran Alon Cc: Krish Sadhukhan, kvm list, Paolo Bonzini, Jim Mattson > On Mar 11, 2020, at 4:12 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote: >> Of course it was best if Intel would have shared their unit-tests for CPU >> functionality (Sean? I'm looking at you :P), but I am not aware that they >> did. > > Only in my dreams :-) I would love to open source some of Intel's > validation tools so that they could be adapted to hammer KVM, but it'll > never happen for a variety of reasons. FYI: In 2014 I ran Intel’s fuzzing-tool (Cafe) to test KVM and found (and fixed) ~100 bugs. And I did not even test nested virtualization… http://www.cs.technion.ac.il/people/namit/online-publications/161-amit.pdf ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 23:21 ` Nadav Amit @ 2020-03-11 23:25 ` Sean Christopherson 2020-03-11 23:35 ` Nadav Amit 0 siblings, 1 reply; 16+ messages in thread From: Sean Christopherson @ 2020-03-11 23:25 UTC (permalink / raw) To: Nadav Amit Cc: Liran Alon, Krish Sadhukhan, kvm list, Paolo Bonzini, Jim Mattson On Wed, Mar 11, 2020 at 04:21:08PM -0700, Nadav Amit wrote: > > On Mar 11, 2020, at 4:12 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote: > >> Of course it was best if Intel would have shared their unit-tests for CPU > >> functionality (Sean? I'm looking at you :P), but I am not aware that they > >> did. > > > > Only in my dreams :-) I would love to open source some of Intel's > > validation tools so that they could be adapted to hammer KVM, but it'll > > never happen for a variety of reasons. > > FYI: In 2014 I ran Intel’s fuzzing-tool (Cafe) to test KVM and found (and > fixed) ~100 bugs. And I did not even test nested virtualization… Heh, I worked on Cafe for 6+ years :-) > http://www.cs.technion.ac.il/people/namit/online-publications/161-amit.pdf ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 23:25 ` Sean Christopherson @ 2020-03-11 23:35 ` Nadav Amit 0 siblings, 0 replies; 16+ messages in thread From: Nadav Amit @ 2020-03-11 23:35 UTC (permalink / raw) To: Sean Christopherson Cc: Liran Alon, Krish Sadhukhan, kvm list, Paolo Bonzini, Jim Mattson > On Mar 11, 2020, at 4:25 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Wed, Mar 11, 2020 at 04:21:08PM -0700, Nadav Amit wrote: >>> On Mar 11, 2020, at 4:12 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: >>> >>> On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote: >>>> Of course it was best if Intel would have shared their unit-tests for CPU >>>> functionality (Sean? I'm looking at you :P), but I am not aware that they >>>> did. >>> >>> Only in my dreams :-) I would love to open source some of Intel's >>> validation tools so that they could be adapted to hammer KVM, but it'll >>> never happen for a variety of reasons. >> >> FYI: In 2014 I ran Intel’s fuzzing-tool (Cafe) to test KVM and found (and >> fixed) ~100 bugs. And I did not even test nested virtualization… > > Heh, I worked on Cafe for 6+ years :-) Well, I was working on its predecessor, Janus, for 5 years… ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 23:12 ` Sean Christopherson 2020-03-11 23:21 ` Nadav Amit @ 2020-03-11 23:22 ` Liran Alon 2020-03-11 23:30 ` Sean Christopherson 1 sibling, 1 reply; 16+ messages in thread From: Liran Alon @ 2020-03-11 23:22 UTC (permalink / raw) To: Sean Christopherson Cc: Nadav Amit, Krish Sadhukhan, kvm list, Paolo Bonzini, jmattson On 12/03/2020 1:12, Sean Christopherson wrote: > On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote: >> Of course it was best if Intel would have shared their unit-tests for CPU >> functionality (Sean? I'm looking at you :P), but I am not aware that they >> did. > Only in my dreams :-) I would love to open source some of Intel's > validation tools so that they could be adapted to hammer KVM, but it'll > never happen for a variety of reasons. I hope then you at least built a setup internally at Intel that runs these test suites on top of KVM to find bugs. :) It would also be nice of Intel to even setup this internally on top of other common hypervisors (e.g. Hyper-V, VMware) and report bugs to vendors. -Liran ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-03-11 23:22 ` Liran Alon @ 2020-03-11 23:30 ` Sean Christopherson 0 siblings, 0 replies; 16+ messages in thread From: Sean Christopherson @ 2020-03-11 23:30 UTC (permalink / raw) To: Liran Alon; +Cc: Nadav Amit, Krish Sadhukhan, kvm list, Paolo Bonzini, jmattson On Wed, Mar 11, 2020 at 11:22:13PM +0000, Liran Alon wrote: > > On 12/03/2020 1:12, Sean Christopherson wrote: > >On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote: > >>Of course it was best if Intel would have shared their unit-tests for CPU > >>functionality (Sean? I'm looking at you :P), but I am not aware that they > >>did. > >Only in my dreams :-) I would love to open source some of Intel's > >validation tools so that they could be adapted to hammer KVM, but it'll > >never happen for a variety of reasons. > > I hope then you at least built a setup internally at Intel that runs these > test suites on top of KVM to find bugs. :) It's on my wish list. > It would also be nice of Intel to even setup this internally on top of other > common hypervisors (e.g. Hyper-V, VMware) and report bugs to vendors. > > -Liran > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers 2020-03-10 22:51 [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Krish Sadhukhan 2020-03-10 22:51 ` [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests Krish Sadhukhan @ 2020-03-10 23:51 ` Jim Mattson [not found] ` <20200311152459.GD21852@linux.intel.com> 1 sibling, 1 reply; 16+ messages in thread From: Jim Mattson @ 2020-03-10 23:51 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: kvm list, Paolo Bonzini, Sean Christopherson On Tue, Mar 10, 2020 at 4:29 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > Even thought today's x86 hardware uses paging and not segmentation for memory > management, it is still good to have some tests that can verify the sanity of > the segment register fields on vmentry of nested guests. > > The test on SS Selector field is failing because the hardware (I am using > Intel Xeon Platinum 8167M 2.00GHz) doesn't raise any error even if the > prescribed bit pattern is not set and as a result vmentry succeeds. Are you sure this isn't just an L0 bug? For instance, does your L0 set "unrestricted guest" in vmcs02, even when L1 doesn't set it in vmcs12? ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20200311152459.GD21852@linux.intel.com>]
* Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers [not found] ` <20200311152459.GD21852@linux.intel.com> @ 2020-03-12 23:37 ` Krish Sadhukhan 0 siblings, 0 replies; 16+ messages in thread From: Krish Sadhukhan @ 2020-03-12 23:37 UTC (permalink / raw) To: Sean Christopherson, Jim Mattson; +Cc: kvm list, Paolo Bonzini On 3/11/20 8:24 AM, Sean Christopherson wrote: > On Tue, Mar 10, 2020 at 04:51:40PM -0700, Jim Mattson wrote: >> On Tue, Mar 10, 2020 at 4:29 PM Krish Sadhukhan >> <krish.sadhukhan@oracle.com> wrote: >>> Even thought today's x86 hardware uses paging and not segmentation for memory >>> management, it is still good to have some tests that can verify the sanity of >>> the segment register fields on vmentry of nested guests. >>> >>> The test on SS Selector field is failing because the hardware (I am using >>> Intel Xeon Platinum 8167M 2.00GHz) doesn't raise any error even if the >>> prescribed bit pattern is not set and as a result vmentry succeeds. >> Are you sure this isn't just an L0 bug? For instance, does your L0 set >> "unrestricted guest" in vmcs02, even when L1 doesn't set it in vmcs12? > > I assume this is the check being discussed? The code is flawed, if CS=3 > and SS=3, "sel = sel_saved | (~cs_rpl_bits & 0x3)" will yield SS=3 and pass. > > I think you wanted something like: > > sel = (sel_saved & ~0x3) | (~cs_rpl_bits & 0x3); Yes, my bit-setting was wrong and I have fixed it. But that's not the cause of the failure. It appears that Jim's suspicion is correct. L0 is not checking in prepare_vmcs02_early(), whether vmcs12 has "unrestricted guest" turned off. After I put the relevant setting in that function, the test now passes (meaning vmentry fails). I will add this fix in v2. > > >> + if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) && >> + !(vmcs_read(CPU_SECONDARY) & CPU_URG)) { >> + u16 cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3; >> + sel_saved = vmcs_read(GUEST_SEL_SS); >> + sel = sel_saved | (~cs_rpl_bits & 0x3); >> + TEST_SEGMENT_SEL(GUEST_SEL_SS, "GUEST_SEL_SS", sel, sel_saved); >> + } >> +} ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-03-12 23:38 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 22:51 [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Krish Sadhukhan 2020-03-10 22:51 ` [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests Krish Sadhukhan 2020-03-11 15:05 ` Sean Christopherson 2020-03-11 15:19 ` Sean Christopherson 2020-03-11 20:38 ` Krish Sadhukhan 2020-03-11 21:46 ` Sean Christopherson 2020-03-11 21:53 ` Nadav Amit 2020-03-11 22:54 ` Liran Alon 2020-03-11 23:12 ` Sean Christopherson 2020-03-11 23:21 ` Nadav Amit 2020-03-11 23:25 ` Sean Christopherson 2020-03-11 23:35 ` Nadav Amit 2020-03-11 23:22 ` Liran Alon 2020-03-11 23:30 ` Sean Christopherson 2020-03-10 23:51 ` [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Jim Mattson [not found] ` <20200311152459.GD21852@linux.intel.com> 2020-03-12 23:37 ` Krish Sadhukhan
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.