| Message ID | 20190408213516.17966-7-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6,v5,nVMX] : Check "load IA32_PAT" VM-exit control on vmentry | expand |
On 08/04/19 23:35, Krish Sadhukhan wrote: > .to verify KVM performs the appropriate consistency checks for loading > IA32_PAT as part of running a nested guest. > > According to section "Checks on Host Control Registers and MSRs" in Intel > SDM vol 3C, the following check is performed on vmentry: > > If the “load IA32_PAT” VM-exit control is 1, the value of the field > for the IA32_PAT MSR must be one that could be written by WRMSR > without fault at CPL 0. Specifically, each of the 8 bytes in the > field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), > 6 (WB), or 7 (UC-). > > Since a PAT value higher than 8 will yield the same test result as that > of 8, we want to confine our tests only up to 8 in order to reduce > redundancy of tests and to avoid too many vmentries. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > x86/vmx_tests.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 72 insertions(+) > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index 66a87f6..380fd85 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -4995,6 +4995,76 @@ static void test_sysenter_field(u32 field, const char *name) > vmcs_write(field, addr_saved); > } > > +/* > + * PAT values higher than 8 are uninteresting since they're likely lumped > + * in with "8". We cap the tests at PAT value of 8 in order to reduce the > + * number of VM-Entries and keep the runtime reasonable. > + */ > +#define PAT_VAL_LIMIT 8 > + > +static void test_pat(u32 field, const char * field_name, u32 ctrl_field, > + u64 ctrl_bit) > +{ > + u32 ctrl_saved = vmcs_read(ctrl_field); > + u64 pat_saved = vmcs_read(field); > + u64 i, val; > + u32 j; > + int error; > + > + vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit); > + for (i = 0; i <= PAT_VAL_LIMIT; i++) { > + /* Test PAT0..PAT7 fields */ > + for (j = 0; j < 8; j++) { > + val = i << j * 8; > + vmcs_write(field, val); > + report_prefix_pushf("%s %lx", field_name, val); > + test_vmx_vmlaunch(0, false); > + report_prefix_pop(); > + } > + } > + > + vmcs_write(ctrl_field, ctrl_saved | ctrl_bit); > + for (i = 0; i <= PAT_VAL_LIMIT; i++) { > + /* Test PAT0..PAT7 fields */ > + for (j = 0; j < 8; j++) { > + val = i << j * 8; > + vmcs_write(field, val); > + report_prefix_pushf("%s %lx", field_name, val); > + if (i == 0x2 || i == 0x3 || i >= 0x8) > + error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD; > + else > + error = 0; > + test_vmx_vmlaunch(error, false); > + report_prefix_pop(); > + } Here are some small changes to remove redundant tests and also improve coverage of values > 8: diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index fd1f483..7adc76a 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -6633,8 +6633,8 @@ static void test_host_ctl_regs(void) /* * PAT values higher than 8 are uninteresting since they're likely lumped - * in with "8". We cap the tests at PAT value of 8 in order to reduce the - * number of VM-Entries and keep the runtime reasonable. + * in with "8". We only test values above 8 one bit at a time, + * in order to reduce the number of VM-Entries and keep the runtime reasonable. */ #define PAT_VAL_LIMIT 8 @@ -6648,9 +6648,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field, int error; vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit); - for (i = 0; i <= PAT_VAL_LIMIT; i++) { + for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2) { /* Test PAT0..PAT7 fields */ - for (j = 0; j < 8; j++) { + for (j = 0; j < (i ? 8 : 1); j++) { val = i << j * 8; vmcs_write(field, val); report_prefix_pushf("%s %lx", field_name, val); @@ -6660,9 +6660,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field, } vmcs_write(ctrl_field, ctrl_saved | ctrl_bit); - for (i = 0; i <= PAT_VAL_LIMIT; i++) { + for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2) { /* Test PAT0..PAT7 fields */ - for (j = 0; j < 8; j++) { + for (j = 0; j < (i ? 8 : 1); j++) { val = i << j * 8; vmcs_write(field, val); report_prefix_pushf("%s %lx", field_name, val); For now I queued the patch with thse changes, holler if you disagree! Thanks, Paolo
On Wed, Apr 10, 2019 at 02:03:54PM +0200, Paolo Bonzini wrote: > Here are some small changes to remove redundant tests and also > improve coverage of values > 8: > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index fd1f483..7adc76a 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -6633,8 +6633,8 @@ static void test_host_ctl_regs(void) > > /* > * PAT values higher than 8 are uninteresting since they're likely lumped > - * in with "8". We cap the tests at PAT value of 8 in order to reduce the > - * number of VM-Entries and keep the runtime reasonable. > + * in with "8". We only test values above 8 one bit at a time, > + * in order to reduce the number of VM-Entries and keep the runtime reasonable. > */ > #define PAT_VAL_LIMIT 8 > > @@ -6648,9 +6648,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field, > int error; > > vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit); > - for (i = 0; i <= PAT_VAL_LIMIT; i++) { > + for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2) { > /* Test PAT0..PAT7 fields */ > - for (j = 0; j < 8; j++) { > + for (j = 0; j < (i ? 8 : 1); j++) { I don't think "j < (i ? 8 : 1)" is what you intended. As-is only i==0, i.e. UC memtype, gets shortcircuited to test PAT0 only. Did you perhaps intend to test only PAT0 for i>8? E.g.: for (j = 0; j < (i <= PAT_VAL_LIMIT : 8 ? 1); j++) > val = i << j * 8; As an alternative to iterating over PAT0..PAT7, which is the real source of pain, what about randomizing the start index and shifting values through that? E.g.: j = rand(); for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2, j++) { val = i << ((j % 8) * 8); vmcs_write(field, val); report_prefix_pushf("%s %lx", field_name, val); } And at that point I'd be ok hitting all values [0..255]. Which indirectly broaches another topic: how do people feel about introducing randomness into kvm-unit-tests? Or perhaps selftests would be a better landing spot since randomness would take us even further away from true "unit tests". > vmcs_write(field, val); > report_prefix_pushf("%s %lx", field_name, val); > @@ -6660,9 +6660,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field, > } > > vmcs_write(ctrl_field, ctrl_saved | ctrl_bit); > - for (i = 0; i <= PAT_VAL_LIMIT; i++) { > + for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2) { > /* Test PAT0..PAT7 fields */ > - for (j = 0; j < 8; j++) { > + for (j = 0; j < (i ? 8 : 1); j++) { > val = i << j * 8; > vmcs_write(field, val); > report_prefix_pushf("%s %lx", field_name, val); > > For now I queued the patch with thse changes, holler if you disagree! > > Thanks, > > Paolo
On 10/04/19 19:17, Sean Christopherson wrote: >> vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit); >> - for (i = 0; i <= PAT_VAL_LIMIT; i++) { >> + for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2) { >> /* Test PAT0..PAT7 fields */ >> - for (j = 0; j < 8; j++) { >> + for (j = 0; j < (i ? 8 : 1); j++) { > I don't think "j < (i ? 8 : 1)" is what you intended. As-is only i==0, > i.e. UC memtype, gets shortcircuited to test PAT0 only. Did you perhaps > intend to test only PAT0 for i>8? E.g.: No, it is what I intended. The reason is that i == 0 gives the same "i << (j * 8)" for all values of j. Otherwise, the logs show 8 entries for GUEST_IA32_PAT = 0. :) Paolo
On Wed, Apr 10, 2019 at 07:22:51PM +0200, Paolo Bonzini wrote: > On 10/04/19 19:17, Sean Christopherson wrote: > >> vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit); > >> - for (i = 0; i <= PAT_VAL_LIMIT; i++) { > >> + for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2) { > >> /* Test PAT0..PAT7 fields */ > >> - for (j = 0; j < 8; j++) { > >> + for (j = 0; j < (i ? 8 : 1); j++) { > > I don't think "j < (i ? 8 : 1)" is what you intended. As-is only i==0, > > i.e. UC memtype, gets shortcircuited to test PAT0 only. Did you perhaps > > intend to test only PAT0 for i>8? E.g.: > > No, it is what I intended. The reason is that i == 0 gives the same "i > << (j * 8)" for all values of j. Otherwise, the logs show 8 entries for > GUEST_IA32_PAT = 0. :) *sigh* Math is hard. Thanks!
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index 66a87f6..380fd85 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -4995,6 +4995,76 @@ static void test_sysenter_field(u32 field, const char *name) vmcs_write(field, addr_saved); } +/* + * PAT values higher than 8 are uninteresting since they're likely lumped + * in with "8". We cap the tests at PAT value of 8 in order to reduce the + * number of VM-Entries and keep the runtime reasonable. + */ +#define PAT_VAL_LIMIT 8 + +static void test_pat(u32 field, const char * field_name, u32 ctrl_field, + u64 ctrl_bit) +{ + u32 ctrl_saved = vmcs_read(ctrl_field); + u64 pat_saved = vmcs_read(field); + u64 i, val; + u32 j; + int error; + + vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit); + for (i = 0; i <= PAT_VAL_LIMIT; i++) { + /* Test PAT0..PAT7 fields */ + for (j = 0; j < 8; j++) { + val = i << j * 8; + vmcs_write(field, val); + report_prefix_pushf("%s %lx", field_name, val); + test_vmx_vmlaunch(0, false); + report_prefix_pop(); + } + } + + vmcs_write(ctrl_field, ctrl_saved | ctrl_bit); + for (i = 0; i <= PAT_VAL_LIMIT; i++) { + /* Test PAT0..PAT7 fields */ + for (j = 0; j < 8; j++) { + val = i << j * 8; + vmcs_write(field, val); + report_prefix_pushf("%s %lx", field_name, val); + if (i == 0x2 || i == 0x3 || i >= 0x8) + error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD; + else + error = 0; + test_vmx_vmlaunch(error, false); + report_prefix_pop(); + } + } + + vmcs_write(ctrl_field, ctrl_saved); + vmcs_write(field, pat_saved); +} + +/* + * If the "load IA32_PAT" VM-exit control is 1, the value of the field + * for the IA32_PAT MSR must be one that could be written by WRMSR + * without fault at CPL 0. Specifically, each of the 8 bytes in the + * field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), + * 6 (WB), or 7 (UC-). + * + * [Intel SDM] + */ +static void test_load_host_pat(void) +{ + /* + * "load IA32_PAT" VM-exit control + */ + if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT)) { + printf("\"Load-IA32-PAT\" exit control not supported\n"); + return; + } + + test_pat(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT); +} + /* * Check that the virtual CPU checks the VMX Host State Area as * documented in the Intel SDM. @@ -5010,6 +5080,8 @@ static void vmx_host_state_area_test(void) test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP"); test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP"); + + test_load_host_pat(); } static bool valid_vmcs_for_vmentry(void)
.to verify KVM performs the appropriate consistency checks for loading IA32_PAT as part of running a nested guest. According to section "Checks on Host Control Registers and MSRs" in Intel SDM vol 3C, the following check is performed on vmentry: If the “load IA32_PAT” VM-exit control is 1, the value of the field for the IA32_PAT MSR must be one that could be written by WRMSR without fault at CPL 0. Specifically, each of the 8 bytes in the field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), 6 (WB), or 7 (UC-). Since a PAT value higher than 8 will yield the same test result as that of 8, we want to confine our tests only up to 8 in order to reduce redundancy of tests and to avoid too many vmentries. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> --- x86/vmx_tests.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+)