| Message ID | 20181206182055.16927-4-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-test,1/3] KVM nVMX: test_vmcs_page_* functions need to accept alignment size as a parameter | expand |
On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > According to section "Checks on VMX Controls" in Intel SDM vol 3C, the > following checks performed for the VM-exit MSR-store address if the > the VM-exit MSR-store count field is non-zero: > > - The lower 4 bits of the VM-exit MSR-store address must be 0. > The address should not set any bits beyond the processor’s > physical-address width. > > - The address of the last byte in the VM-exit MSR-store area > should not set any bits beyond the processor’s physical-address > width. The address of this last byte is VM-exit MSR-store address > + (MSR count * 16) - 1. (The arithmetic used for the computation > uses more bits than the processor’s physical-address width.) > > If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits > in the range 63:32. I think there are several existing tests that have ignored this footnote. Perhaps you could fix this omission in a general way in a separate patch? > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> > --- > x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 70 insertions(+) > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index 900dcf4..af856cf 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -4731,6 +4731,75 @@ static void test_pml(void) > } > > /* > + * The following checks are performed for the VM-exit MSR-store address if > + * the VM-exit MSR-store count field is non-zero: > + * > + * - The lower 4 bits of the VM-exit MSR-store address must be 0. > + * The address should not set any bits beyond the processor’s > + * physical-address width. > + * > + * - The address of the last byte in the VM-exit MSR-store area > + * should not set any bits beyond the processor’s physical-address > + * width. The address of this last byte is VM-exit MSR-store address > + * + (MSR count * 16) - 1. (The arithmetic used for the computation > + * uses more bits than the processor’s physical-address width.) > + * > + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits > + * in the range 63:32. > + * > + * [Intel SDM] > + */ > +static void test_exit_msr_store(void) > +{ > + exit_msr_store = alloc_page(); > + u64 tmp; > + u32 exit_msr_st_cnt = 1; > + int i; > + bool ctrl = true; > + u32 addr_len = 64; > + > + vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt); > + > + /* Check first 4 bits of VM-exit MSR-store address */ > + for (i = 0; i < 4; i++) { > + tmp = (u64)exit_msr_store | 1ull << i; > + vmcs_write(EXIT_MSR_ST_ADDR, tmp); > + report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx", > + tmp & 0xf); > + if (i && ctrl == true) { > + ctrl = false; > + } > + test_vmx_controls(false, false); > + report_prefix_pop(); > + } > + > + if (basic.val & (1ul << 48)) > + addr_len = 32; > + > + test_vmcs_page_values("VM-exit-MSR-store address", > + EXIT_MSR_ST_ADDR, false, false, 16, > + 4, addr_len - 1); > + > + /* Check last byte of VM-exit MSR-store address */ > + for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) { Why the different initializers? Even when IA32_VMX_BASIC[48] is set, the *end* of the MSR store buffer doesn't have to fit in 32 bits. > + exit_msr_store = (struct vmx_msr_entry *) > + (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) | > + 1ul << i); > + > + vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store); > + test_vmx_controls(false, false); > + } I could just be confused, but I don't think this is testing what you think it's testing. The constraints are that the entire msr_store buffer must reside within the bounds of physically addressable memory. Thus, with EXI_MSR_ST_CNT set to 1, it might be interesting to test EXIT_MSR_ST_ADDR values of, say: (1ULL << cpuid_maxphyaddr()) - 15 [FAIL] (1ULL << cpuid_maxphyaddr()) - 16 [PASS] (1ULL << cpuid_maxphyaddr()) - 17 [PASS] > +} > + > +/* > + * Tests for VMX exit controls > + */ > +static void test_exit_ctls(void) > +{ > + test_exit_msr_store(); > +} > + > +/* > * Check that the virtual CPU checks all of the VMX controls as > * documented in the Intel SDM. > */ > @@ -4756,6 +4825,7 @@ static void vmx_controls_test(void) > test_invalid_event_injection(); > test_vpid(); > test_eptp(); > + test_exit_ctls(); > } > > static bool valid_vmcs_for_vmentry(void) > -- > 2.9.5 >
On 12/06/2018 03:02 PM, Jim Mattson wrote: > On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan > <krish.sadhukhan@oracle.com> wrote: >> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the >> following checks performed for the VM-exit MSR-store address if the >> the VM-exit MSR-store count field is non-zero: >> >> - The lower 4 bits of the VM-exit MSR-store address must be 0. >> The address should not set any bits beyond the processor’s >> physical-address width. >> >> - The address of the last byte in the VM-exit MSR-store area >> should not set any bits beyond the processor’s physical-address >> width. The address of this last byte is VM-exit MSR-store address >> + (MSR count * 16) - 1. (The arithmetic used for the computation >> uses more bits than the processor’s physical-address width.) >> >> If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits >> in the range 63:32. > I think there are several existing tests that have ignored this > footnote. Perhaps you could fix this omission in a general way in a > separate patch? Will do. >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> >> --- >> x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 70 insertions(+) >> >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >> index 900dcf4..af856cf 100644 >> --- a/x86/vmx_tests.c >> +++ b/x86/vmx_tests.c >> @@ -4731,6 +4731,75 @@ static void test_pml(void) >> } >> >> /* >> + * The following checks are performed for the VM-exit MSR-store address if >> + * the VM-exit MSR-store count field is non-zero: >> + * >> + * - The lower 4 bits of the VM-exit MSR-store address must be 0. >> + * The address should not set any bits beyond the processor’s >> + * physical-address width. >> + * >> + * - The address of the last byte in the VM-exit MSR-store area >> + * should not set any bits beyond the processor’s physical-address >> + * width. The address of this last byte is VM-exit MSR-store address >> + * + (MSR count * 16) - 1. (The arithmetic used for the computation >> + * uses more bits than the processor’s physical-address width.) >> + * >> + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits >> + * in the range 63:32. >> + * >> + * [Intel SDM] >> + */ >> +static void test_exit_msr_store(void) >> +{ >> + exit_msr_store = alloc_page(); >> + u64 tmp; >> + u32 exit_msr_st_cnt = 1; >> + int i; >> + bool ctrl = true; >> + u32 addr_len = 64; >> + >> + vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt); >> + >> + /* Check first 4 bits of VM-exit MSR-store address */ >> + for (i = 0; i < 4; i++) { >> + tmp = (u64)exit_msr_store | 1ull << i; >> + vmcs_write(EXIT_MSR_ST_ADDR, tmp); >> + report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx", >> + tmp & 0xf); >> + if (i && ctrl == true) { >> + ctrl = false; >> + } >> + test_vmx_controls(false, false); >> + report_prefix_pop(); >> + } >> + >> + if (basic.val & (1ul << 48)) >> + addr_len = 32; >> + >> + test_vmcs_page_values("VM-exit-MSR-store address", >> + EXIT_MSR_ST_ADDR, false, false, 16, >> + 4, addr_len - 1); >> + >> + /* Check last byte of VM-exit MSR-store address */ >> + for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) { > Why the different initializers? Even when IA32_VMX_BASIC[48] is set, > the *end* of the MSR store buffer doesn't have to fit in 32 bits. I am not sure I follow it... According to the footnote, setting any bit in [63:32] in the address of the last byte is illegal if IA32_VMX_BASIC[48] is set and according to appendix A-1 in SDM vol 3D: "If the bit is 0, these addresses are limited to the processor’s physical-address width. If the bit is 1, these addresses are limited to 32 bits. This bit is always 0 for processors that support Intel 64 architecture." Therefore, on x86_64, the addresses can set any bits in [40:0] but on x86_32, they are limited to [31:0]. Am I missing something ? > >> + exit_msr_store = (struct vmx_msr_entry *) >> + (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) | >> + 1ul << i); >> + >> + vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store); >> + test_vmx_controls(false, false); >> + } > I could just be confused, but I don't think this is testing what you > think it's testing. I have verified that the following check in nested_vmx_check_msr_switch(), (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr) passes when a bit in [63:40] is set in the address of the last byte of the MSR-store address. Am I missing anything ? > > The constraints are that the entire msr_store buffer must reside > within the bounds of physically addressable memory. Thus, with > EXI_MSR_ST_CNT set to 1, it might be interesting to test > EXIT_MSR_ST_ADDR values of, say: > (1ULL << cpuid_maxphyaddr()) - 15 [FAIL] > (1ULL << cpuid_maxphyaddr()) - 16 [PASS] > (1ULL << cpuid_maxphyaddr()) - 17 [PASS] I can certainly add these cases. BTW, the last one (17) on your list is not a PASS case because it sets the low 4 bits in the MSR-store address itself (and not address of the last byte) and that's not legal. > >> +} >> + >> +/* >> + * Tests for VMX exit controls >> + */ >> +static void test_exit_ctls(void) >> +{ >> + test_exit_msr_store(); >> +} >> + >> +/* >> * Check that the virtual CPU checks all of the VMX controls as >> * documented in the Intel SDM. >> */ >> @@ -4756,6 +4825,7 @@ static void vmx_controls_test(void) >> test_invalid_event_injection(); >> test_vpid(); >> test_eptp(); >> + test_exit_ctls(); >> } >> >> static bool valid_vmcs_for_vmentry(void) >> -- >> 2.9.5 >>
On Thu, Dec 6, 2018 at 4:51 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > > > On 12/06/2018 03:02 PM, Jim Mattson wrote: > > On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan > > <krish.sadhukhan@oracle.com> wrote: > >> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the > >> following checks performed for the VM-exit MSR-store address if the > >> the VM-exit MSR-store count field is non-zero: > >> > >> - The lower 4 bits of the VM-exit MSR-store address must be 0. > >> The address should not set any bits beyond the processor’s > >> physical-address width. > >> > >> - The address of the last byte in the VM-exit MSR-store area > >> should not set any bits beyond the processor’s physical-address > >> width. The address of this last byte is VM-exit MSR-store address > >> + (MSR count * 16) - 1. (The arithmetic used for the computation > >> uses more bits than the processor’s physical-address width.) > >> > >> If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits > >> in the range 63:32. > > I think there are several existing tests that have ignored this > > footnote. Perhaps you could fix this omission in a general way in a > > separate patch? > > Will do. > > >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > >> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> > >> --- > >> x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 70 insertions(+) > >> > >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > >> index 900dcf4..af856cf 100644 > >> --- a/x86/vmx_tests.c > >> +++ b/x86/vmx_tests.c > >> @@ -4731,6 +4731,75 @@ static void test_pml(void) > >> } > >> > >> /* > >> + * The following checks are performed for the VM-exit MSR-store address if > >> + * the VM-exit MSR-store count field is non-zero: > >> + * > >> + * - The lower 4 bits of the VM-exit MSR-store address must be 0. > >> + * The address should not set any bits beyond the processor’s > >> + * physical-address width. > >> + * > >> + * - The address of the last byte in the VM-exit MSR-store area > >> + * should not set any bits beyond the processor’s physical-address > >> + * width. The address of this last byte is VM-exit MSR-store address > >> + * + (MSR count * 16) - 1. (The arithmetic used for the computation > >> + * uses more bits than the processor’s physical-address width.) > >> + * > >> + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits > >> + * in the range 63:32. > >> + * > >> + * [Intel SDM] > >> + */ > >> +static void test_exit_msr_store(void) > >> +{ > >> + exit_msr_store = alloc_page(); > >> + u64 tmp; > >> + u32 exit_msr_st_cnt = 1; > >> + int i; > >> + bool ctrl = true; > >> + u32 addr_len = 64; > >> + > >> + vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt); > >> + > >> + /* Check first 4 bits of VM-exit MSR-store address */ > >> + for (i = 0; i < 4; i++) { > >> + tmp = (u64)exit_msr_store | 1ull << i; > >> + vmcs_write(EXIT_MSR_ST_ADDR, tmp); > >> + report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx", > >> + tmp & 0xf); > >> + if (i && ctrl == true) { > >> + ctrl = false; > >> + } > >> + test_vmx_controls(false, false); > >> + report_prefix_pop(); > >> + } > >> + > >> + if (basic.val & (1ul << 48)) > >> + addr_len = 32; > >> + > >> + test_vmcs_page_values("VM-exit-MSR-store address", > >> + EXIT_MSR_ST_ADDR, false, false, 16, > >> + 4, addr_len - 1); > >> + > >> + /* Check last byte of VM-exit MSR-store address */ > >> + for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) { > > Why the different initializers? Even when IA32_VMX_BASIC[48] is set, > > the *end* of the MSR store buffer doesn't have to fit in 32 bits. > > I am not sure I follow it... > > According to the footnote, setting any bit in [63:32] in the address of > the last byte is illegal if IA32_VMX_BASIC[48] is set and according to > appendix A-1 in SDM vol 3D: > > "If the bit is 0, these addresses are limited to the processor’s > physical-address width. If the bit is 1, these addresses are limited to > 32 bits. This bit is always 0 for processors that support Intel 64 > architecture." > > Therefore, on x86_64, the addresses can set any bits in [40:0] but on > x86_32, they are limited to [31:0]. > > Am I missing something ? My bad. Re-reading the SDM, the 32-bit constraint clearly applies to both ends of the buffer. > > > >> + exit_msr_store = (struct vmx_msr_entry *) > >> + (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) | > >> + 1ul << i); > >> + > >> + vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store); > >> + test_vmx_controls(false, false); > >> + } > > I could just be confused, but I don't think this is testing what you > > think it's testing. > > I have verified that the following check in nested_vmx_check_msr_switch(), > > (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr) > > > passes when a bit in [63:40] is set in the address of the last byte of > the MSR-store address. > > Am I missing anything ? If the initial exit_msr_store address is 0, then you are testing the following addresses when addr_len ==32: 10000000f 30000001e 70000002d f0000003c 1f0000004b 3f0000005a 7f00000069 ff00000078 1ff00000087 3ff00000096 7ff000000a5 fff000000b4 1fff000000c3 3fff000000d2 7fff000000e1 ffff000000f0 1ffff000000ff 3ffff0000010e 7ffff0000011d fffff0000012c 1fffff0000013b 3fffff0000014a 7fffff00000159 ffffff00000168 1ffffff00000177 3ffffff00000186 7ffffff00000195 fffffff000001a4 1fffffff000001b3 3fffffff000001c2 7fffffff000001d1 ffffffff000001e0 These addresses seem strange to me. If addr_len == 64 and cpuid_maxphyaddr() is 46, you are testing: 40000000000f c0000000001e 1c0000000002d 3c0000000003c 7c0000000004b fc0000000005a 1fc00000000069 3fc00000000078 7fc00000000087 ffc00000000096 1ffc000000000a5 3ffc000000000b4 7ffc000000000c3 fffc000000000d2 1fffc000000000e1 3fffc000000000f0 7fffc000000000ff ffffc0000000010e These addresses also seem strange to me. > > > > The constraints are that the entire msr_store buffer must reside > > within the bounds of physically addressable memory. Thus, with > > EXI_MSR_ST_CNT set to 1, it might be interesting to test > > EXIT_MSR_ST_ADDR values of, say: > > (1ULL << cpuid_maxphyaddr()) - 15 [FAIL] > > (1ULL << cpuid_maxphyaddr()) - 16 [PASS] > > (1ULL << cpuid_maxphyaddr()) - 17 [PASS] > > I can certainly add these cases. BTW, the last one (17) on your list is > not a PASS case because it sets the low 4 bits in the MSR-store address > itself (and not address of the last byte) and that's not legal. Okay. Given the multiple constraints, I'd suggest setting the store count to 2 and testing: (1ULL << cpuid_maxphyaddr()) - 48 (1ULL << cpuid_maxphyaddr()) - 32 (1ULL << cpuid_maxphyaddr()) - 16
On 12/07/2018 09:57 AM, Jim Mattson wrote: > On Thu, Dec 6, 2018 at 4:51 PM Krish Sadhukhan > <krish.sadhukhan@oracle.com> wrote: >> >> >> On 12/06/2018 03:02 PM, Jim Mattson wrote: >>> On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan >>> <krish.sadhukhan@oracle.com> wrote: >>>> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the >>>> following checks performed for the VM-exit MSR-store address if the >>>> the VM-exit MSR-store count field is non-zero: >>>> >>>> - The lower 4 bits of the VM-exit MSR-store address must be 0. >>>> The address should not set any bits beyond the processor’s >>>> physical-address width. >>>> >>>> - The address of the last byte in the VM-exit MSR-store area >>>> should not set any bits beyond the processor’s physical-address >>>> width. The address of this last byte is VM-exit MSR-store address >>>> + (MSR count * 16) - 1. (The arithmetic used for the computation >>>> uses more bits than the processor’s physical-address width.) >>>> >>>> If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits >>>> in the range 63:32. >>> I think there are several existing tests that have ignored this >>> footnote. Perhaps you could fix this omission in a general way in a >>> separate patch? >> Will do. >> >>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >>>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> >>>> --- >>>> x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 70 insertions(+) >>>> >>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >>>> index 900dcf4..af856cf 100644 >>>> --- a/x86/vmx_tests.c >>>> +++ b/x86/vmx_tests.c >>>> @@ -4731,6 +4731,75 @@ static void test_pml(void) >>>> } >>>> >>>> /* >>>> + * The following checks are performed for the VM-exit MSR-store address if >>>> + * the VM-exit MSR-store count field is non-zero: >>>> + * >>>> + * - The lower 4 bits of the VM-exit MSR-store address must be 0. >>>> + * The address should not set any bits beyond the processor’s >>>> + * physical-address width. >>>> + * >>>> + * - The address of the last byte in the VM-exit MSR-store area >>>> + * should not set any bits beyond the processor’s physical-address >>>> + * width. The address of this last byte is VM-exit MSR-store address >>>> + * + (MSR count * 16) - 1. (The arithmetic used for the computation >>>> + * uses more bits than the processor’s physical-address width.) >>>> + * >>>> + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits >>>> + * in the range 63:32. >>>> + * >>>> + * [Intel SDM] >>>> + */ >>>> +static void test_exit_msr_store(void) >>>> +{ >>>> + exit_msr_store = alloc_page(); >>>> + u64 tmp; >>>> + u32 exit_msr_st_cnt = 1; >>>> + int i; >>>> + bool ctrl = true; >>>> + u32 addr_len = 64; >>>> + >>>> + vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt); >>>> + >>>> + /* Check first 4 bits of VM-exit MSR-store address */ >>>> + for (i = 0; i < 4; i++) { >>>> + tmp = (u64)exit_msr_store | 1ull << i; >>>> + vmcs_write(EXIT_MSR_ST_ADDR, tmp); >>>> + report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx", >>>> + tmp & 0xf); >>>> + if (i && ctrl == true) { >>>> + ctrl = false; >>>> + } >>>> + test_vmx_controls(false, false); >>>> + report_prefix_pop(); >>>> + } >>>> + >>>> + if (basic.val & (1ul << 48)) >>>> + addr_len = 32; >>>> + >>>> + test_vmcs_page_values("VM-exit-MSR-store address", >>>> + EXIT_MSR_ST_ADDR, false, false, 16, >>>> + 4, addr_len - 1); >>>> + >>>> + /* Check last byte of VM-exit MSR-store address */ >>>> + for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) { >>> Why the different initializers? Even when IA32_VMX_BASIC[48] is set, >>> the *end* of the MSR store buffer doesn't have to fit in 32 bits. >> I am not sure I follow it... >> >> According to the footnote, setting any bit in [63:32] in the address of >> the last byte is illegal if IA32_VMX_BASIC[48] is set and according to >> appendix A-1 in SDM vol 3D: >> >> "If the bit is 0, these addresses are limited to the processor’s >> physical-address width. If the bit is 1, these addresses are limited to >> 32 bits. This bit is always 0 for processors that support Intel 64 >> architecture." >> >> Therefore, on x86_64, the addresses can set any bits in [40:0] but on >> x86_32, they are limited to [31:0]. >> >> Am I missing something ? > My bad. Re-reading the SDM, the 32-bit constraint clearly applies to > both ends of the buffer. > >>>> + exit_msr_store = (struct vmx_msr_entry *) >>>> + (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) | >>>> + 1ul << i); >>>> + >>>> + vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store); >>>> + test_vmx_controls(false, false); >>>> + } >>> I could just be confused, but I don't think this is testing what you >>> think it's testing. >> I have verified that the following check in nested_vmx_check_msr_switch(), >> >> (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr) >> >> >> passes when a bit in [63:40] is set in the address of the last byte of >> the MSR-store address. >> >> Am I missing anything ? > If the initial exit_msr_store address is 0, then you are testing the > following addresses when addr_len ==32: exit_msr_store is allocated and is not 0 in this part of the test. For example, on my x86_64 system, I see the following addresses being tested: 0x1000047900f 0x3000047901e 0x7000047902d 0xf000047903c 0x1f000047904b 0x3f000047905a 0x7f0000479069 0xff0000479078 0x1ff0000479087 0x3ff0000479096 0x7ff00004790a5 0xfff00004790b4 0x1fff00004790c3 0x3fff00004790d2 0x7fff00004790e1 0xffff00004790f0 0x1ffff00004790ff 0x3ffff000047910e 0x7ffff000047911d 0xfffff000047912c 0x1fffff000047913b 0x3fffff000047914a 0x7fffff0000479159 0xffffff0000479168 Here, my goal is to use a real address (returned by alloc_page()) for the starting byte of exit_msr_store and then make the address of the last byte invalid by setting a bit in [63:40] in the latter. > > 10000000f > 30000001e > 70000002d > f0000003c > 1f0000004b > 3f0000005a > 7f00000069 > ff00000078 > 1ff00000087 > 3ff00000096 > 7ff000000a5 > fff000000b4 > 1fff000000c3 > 3fff000000d2 > 7fff000000e1 > ffff000000f0 > 1ffff000000ff > 3ffff0000010e > 7ffff0000011d > fffff0000012c > 1fffff0000013b > 3fffff0000014a > 7fffff00000159 > ffffff00000168 > 1ffffff00000177 > 3ffffff00000186 > 7ffffff00000195 > fffffff000001a4 > 1fffffff000001b3 > 3fffffff000001c2 > 7fffffff000001d1 > ffffffff000001e0 > > These addresses seem strange to me. > > If addr_len == 64 and cpuid_maxphyaddr() is 46, you are testing: > > 40000000000f > c0000000001e > 1c0000000002d > 3c0000000003c > 7c0000000004b > fc0000000005a > 1fc00000000069 > 3fc00000000078 > 7fc00000000087 > ffc00000000096 > 1ffc000000000a5 > 3ffc000000000b4 > 7ffc000000000c3 > fffc000000000d2 > 1fffc000000000e1 > 3fffc000000000f0 > 7fffc000000000ff > ffffc0000000010e > > These addresses also seem strange to me. > >>> The constraints are that the entire msr_store buffer must reside >>> within the bounds of physically addressable memory. Thus, with >>> EXI_MSR_ST_CNT set to 1, it might be interesting to test >>> EXIT_MSR_ST_ADDR values of, say: >>> (1ULL << cpuid_maxphyaddr()) - 15 [FAIL] >>> (1ULL << cpuid_maxphyaddr()) - 16 [PASS] >>> (1ULL << cpuid_maxphyaddr()) - 17 [PASS] >> I can certainly add these cases. BTW, the last one (17) on your list is >> not a PASS case because it sets the low 4 bits in the MSR-store address >> itself (and not address of the last byte) and that's not legal. > Okay. Given the multiple constraints, I'd suggest setting the store > count to 2 and testing: > > (1ULL << cpuid_maxphyaddr()) - 48 > (1ULL << cpuid_maxphyaddr()) - 32 > (1ULL << cpuid_maxphyaddr()) - 16 Will add these cases.
On Fri, Dec 7, 2018 at 5:52 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > exit_msr_store is allocated and is not 0 in this part of the test. For > example, on my x86_64 system, I see the following addresses being tested: > > 0x1000047900f > 0x3000047901e > 0x7000047902d > 0xf000047903c > 0x1f000047904b > 0x3f000047905a > 0x7f0000479069 > 0xff0000479078 > 0x1ff0000479087 > 0x3ff0000479096 > 0x7ff00004790a5 > 0xfff00004790b4 > 0x1fff00004790c3 > 0x3fff00004790d2 > 0x7fff00004790e1 > 0xffff00004790f0 > 0x1ffff00004790ff > 0x3ffff000047910e > 0x7ffff000047911d > 0xfffff000047912c > 0x1fffff000047913b > 0x3fffff000047914a > 0x7fffff0000479159 > 0xffffff0000479168 > > > Here, my goal is to use a real address (returned by alloc_page()) for > the starting byte of exit_msr_store and then make the address of the > last byte invalid by setting a bit in [63:40] in the latter. Okay. Given that goal: 1) Why do most of these addresses set more than one bit in [63:40]? 2) Why do most of these addresses set illegal bits in [3:0]?
On 12/10/2018 09:26 AM, Jim Mattson wrote: > On Fri, Dec 7, 2018 at 5:52 PM Krish Sadhukhan > <krish.sadhukhan@oracle.com> wrote: > >> exit_msr_store is allocated and is not 0 in this part of the test. For >> example, on my x86_64 system, I see the following addresses being tested: >> >> 0x1000047900f >> 0x3000047901e >> 0x7000047902d >> 0xf000047903c >> 0x1f000047904b >> 0x3f000047905a >> 0x7f0000479069 >> 0xff0000479078 >> 0x1ff0000479087 >> 0x3ff0000479096 >> 0x7ff00004790a5 >> 0xfff00004790b4 >> 0x1fff00004790c3 >> 0x3fff00004790d2 >> 0x7fff00004790e1 >> 0xffff00004790f0 >> 0x1ffff00004790ff >> 0x3ffff000047910e >> 0x7ffff000047911d >> 0xfffff000047912c >> 0x1fffff000047913b >> 0x3fffff000047914a >> 0x7fffff0000479159 >> 0xffffff0000479168 >> >> >> Here, my goal is to use a real address (returned by alloc_page()) for >> the starting byte of exit_msr_store and then make the address of the >> last byte invalid by setting a bit in [63:40] in the latter. > Okay. Given that goal: > 1) Why do most of these addresses set more than one bit in [63:40]? > 2) Why do most of these addresses set illegal bits in [3:0]? Sorry for the oversight ! Both of these need to be fixed. Thanks !
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index 900dcf4..af856cf 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -4731,6 +4731,75 @@ static void test_pml(void) } /* + * The following checks are performed for the VM-exit MSR-store address if + * the VM-exit MSR-store count field is non-zero: + * + * - The lower 4 bits of the VM-exit MSR-store address must be 0. + * The address should not set any bits beyond the processor’s + * physical-address width. + * + * - The address of the last byte in the VM-exit MSR-store area + * should not set any bits beyond the processor’s physical-address + * width. The address of this last byte is VM-exit MSR-store address + * + (MSR count * 16) - 1. (The arithmetic used for the computation + * uses more bits than the processor’s physical-address width.) + * + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits + * in the range 63:32. + * + * [Intel SDM] + */ +static void test_exit_msr_store(void) +{ + exit_msr_store = alloc_page(); + u64 tmp; + u32 exit_msr_st_cnt = 1; + int i; + bool ctrl = true; + u32 addr_len = 64; + + vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt); + + /* Check first 4 bits of VM-exit MSR-store address */ + for (i = 0; i < 4; i++) { + tmp = (u64)exit_msr_store | 1ull << i; + vmcs_write(EXIT_MSR_ST_ADDR, tmp); + report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx", + tmp & 0xf); + if (i && ctrl == true) { + ctrl = false; + } + test_vmx_controls(false, false); + report_prefix_pop(); + } + + if (basic.val & (1ul << 48)) + addr_len = 32; + + test_vmcs_page_values("VM-exit-MSR-store address", + EXIT_MSR_ST_ADDR, false, false, 16, + 4, addr_len - 1); + + /* Check last byte of VM-exit MSR-store address */ + for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) { + exit_msr_store = (struct vmx_msr_entry *) + (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) | + 1ul << i); + + vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store); + test_vmx_controls(false, false); + } +} + +/* + * Tests for VMX exit controls + */ +static void test_exit_ctls(void) +{ + test_exit_msr_store(); +} + +/* * Check that the virtual CPU checks all of the VMX controls as * documented in the Intel SDM. */ @@ -4756,6 +4825,7 @@ static void vmx_controls_test(void) test_invalid_event_injection(); test_vpid(); test_eptp(); + test_exit_ctls(); } static bool valid_vmcs_for_vmentry(void)
According to section "Checks on VMX Controls" in Intel SDM vol 3C, the following checks performed for the VM-exit MSR-store address if the the VM-exit MSR-store count field is non-zero: - The lower 4 bits of the VM-exit MSR-store address must be 0. The address should not set any bits beyond the processor’s physical-address width. - The address of the last byte in the VM-exit MSR-store area should not set any bits beyond the processor’s physical-address width. The address of this last byte is VM-exit MSR-store address + (MSR count * 16) - 1. (The arithmetic used for the computation uses more bits than the processor’s physical-address width.) If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits in the range 63:32. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> --- x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)