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
