All of lore.kernel.org
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH 0/2] Couple of SVM fixes
@ 2021-08-06 16:08 Babu Moger
  2021-08-06 16:08 ` [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations Babu Moger
  2021-08-06 16:08 ` [kvm-unit-tests PATCH 2/2] nSVM: Fix NPT reserved bits test hang Babu Moger
  0 siblings, 2 replies; 10+ messages in thread
From: Babu Moger @ 2021-08-06 16:08 UTC (permalink / raw)
  Cc: pbonzini, thuth, drjones, kvm, babu.moger

This series fixes couple of unittest failures for SVM.
1. The test ./x86/access is failing with timeout.
2. The test ./x86/svm failure with infinite loop.
More details in each patch.

---

Babu Moger (2):
      x86: access: Fix timeout failure by limiting number of flag combinations
      nSVM: Fix NPT reserved bits test hang


 lib/x86/processor.h |   10 ++++++++++
 x86/access.c        |    4 ++--
 x86/svm_tests.c     |   25 +++++++++++++++++++------
 3 files changed, 31 insertions(+), 8 deletions(-)

--

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations
  2021-08-06 16:08 [kvm-unit-tests PATCH 0/2] Couple of SVM fixes Babu Moger
@ 2021-08-06 16:08 ` Babu Moger
       [not found]   ` <YQ1pA9nN6DP0veQ1@google.com>
  2021-08-06 16:08 ` [kvm-unit-tests PATCH 2/2] nSVM: Fix NPT reserved bits test hang Babu Moger
  1 sibling, 1 reply; 10+ messages in thread
From: Babu Moger @ 2021-08-06 16:08 UTC (permalink / raw)
  Cc: pbonzini, thuth, drjones, kvm, babu.moger

From: Babu Moger <Babu.Moger@amd.com>

The test ./x86/access fails with a timeout. This is due to the number test
combination. The test cases increase exponentially as the features get
enabled. The new machine adds the feature AC_CPU_CR4_PKE. The default
timeout is 180 seconds. Seen this problem both on AMD and Intel machines.

#./tests/access
qemu-system-x86_64: terminating on signal 15 from pid 20050 (timeout)
FAIL access (timeout; duration=180)

This test can take about 7 minutes without timeout.
time ./tests/access
58982405 tests, 0 failures
PASS access

real	7m10.063s
user	7m9.063s
sys	0m0.309s

Fix the problem by adding few more limit checks.

Signed-off-by: Babu Moger <Babu.Moger@amd.com>
---
 x86/access.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index 47807cc..e371dd5 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -317,9 +317,9 @@ static _Bool ac_test_legal(ac_test_t *at)
     /*
      * Shorten the test by avoiding testing too many reserved bit combinations
      */
-    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13)) > 1)
+    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13) + F(AC_CPU_CR4_PKE)) > 1)
         return false;
-    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
+    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36) + F(AC_CPU_CR4_PKE)) > 1)
         return false;
 
     return true;


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 2/2] nSVM: Fix NPT reserved bits test hang
  2021-08-06 16:08 [kvm-unit-tests PATCH 0/2] Couple of SVM fixes Babu Moger
  2021-08-06 16:08 ` [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations Babu Moger
@ 2021-08-06 16:08 ` Babu Moger
  1 sibling, 0 replies; 10+ messages in thread
From: Babu Moger @ 2021-08-06 16:08 UTC (permalink / raw)
  Cc: pbonzini, thuth, drjones, kvm, babu.moger

From: Babu Moger <Babu.Moger@amd.com>

SVM reserved bits tests hangs in a infinite loop. The test uses the
instruction 'rdtsc' to generate the random reserved bits. It hangs
while generating the valid reserved bits.

The AMD64 Architecture Programmers Manual Volume 2: System
Programming manual says, When using the TSC to measure elapsed time,
programmers must be aware that for some implementations, the rate at
which the TSC is incremented varies based on the processor power
management state (Pstate). For other implementations, the TSC
increment rate is fixed and is not subject to power-management
related changes in processor frequency.

In AMD gen3 machine, the rdtsc value is a P state multiplier.
Here are the rdtsc value in 10 sucessive reads.
0 rdtsc = 0x1ec92919b9710
1 rdtsc = 0x1ec92919c01f0
2 rdtsc = 0x1ec92919c0f70
3 rdtsc = 0x1ec92919c18d0
4 rdtsc = 0x1ec92919c2060
5 rdtsc = 0x1ec92919c28d0
6 rdtsc = 0x1ec92919c30b0
7 rdtsc = 0x1ec92919c5660
8 rdtsc = 0x1ec92919c6150
9 rdtsc = 0x1ec92919c7c80

This test uses the lower nibble and right shifts to generate the
valid reserved bit. It loops forever because the lower nibble is always
zero.

Fixing the issue with replacing rdrand instruction if available or
skipping the test if we cannot generate the valid reserved bits.

Signed-off-by: Babu Moger <Babu.Moger@amd.com>
---
 lib/x86/processor.h |   10 ++++++++++
 x86/svm_tests.c     |   25 +++++++++++++++++++------
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index a08ea1f..974077a 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -531,6 +531,16 @@ static inline void sti(void)
     asm volatile ("sti");
 }
 
+static inline unsigned long long rdrand(void)
+{
+	long long r;
+
+	asm volatile("1:;\n\
+	              rdrand %0;\n\
+		      jnc 1b;\n":"=r"(r));
+	return r;
+}
+
 static inline unsigned long long rdtsc(void)
 {
 	long long r;
diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 79ed48e..a2963c0 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -2704,11 +2704,23 @@ static void _svm_npt_rsvd_bits_test(u64 *pxe, u64 pxe_rsvd_bits,  u64 efer,
 
 static u64 get_random_bits(u64 hi, u64 low)
 {
-	u64 rsvd_bits;
+	unsigned retry = 5;
+	u64 rsvd_bits = 0;
+
+	if (this_cpu_has(X86_FEATURE_RDRAND)) {
+		do {
+			rsvd_bits = (rdrand() << low) & GENMASK_ULL(hi, low);
+			retry--;
+		} while (!rsvd_bits && retry);
+	}
 
-	do {
-		rsvd_bits = (rdtsc() << low) & GENMASK_ULL(hi, low);
-	} while (!rsvd_bits);
+	if (!rsvd_bits) {
+		retry = 5;
+		do {
+			rsvd_bits = (rdtsc() << low) & GENMASK_ULL(hi, low);
+			retry--;
+		} while (!rsvd_bits && retry);
+	}
 
 	return rsvd_bits;
 }
@@ -2733,10 +2745,11 @@ static void svm_npt_rsvd_bits_test(void)
 
 	/*
 	 * 4k PTEs don't have reserved bits if MAXPHYADDR >= 52, just skip the
-	 * sub-test.  The NX test is still valid, but the extra bit of coverage
+	 * sub-test. Also skip if cannot generate the valid random reserved bits.
+	 * The NX test is still valid, but the extra bit of coverage
 	 * isn't worth the extra complexity.
 	 */
-	if (cpuid_maxphyaddr() >= 52)
+	if ((cpuid_maxphyaddr() >= 52) || !get_random_bits(51, cpuid_maxphyaddr()))
 		goto skip_pte_test;
 
 	_svm_npt_rsvd_bits_test(npt_get_pte((u64)basic_guest_main),


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations
       [not found]   ` <YQ1pA9nN6DP0veQ1@google.com>
@ 2021-08-09 19:43     ` Babu Moger
  2021-08-10 16:59       ` Babu Moger
  0 siblings, 1 reply; 10+ messages in thread
From: Babu Moger @ 2021-08-09 19:43 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: pbonzini, thuth, drjones, kvm



On 8/6/21 11:53 AM, Sean Christopherson wrote:
> On Fri, Aug 06, 2021, Babu Moger wrote:
>> From: Babu Moger <Babu.Moger@amd.com>
>>
>> The test ./x86/access fails with a timeout. This is due to the number test
>> combination. The test cases increase exponentially as the features get
>> enabled. The new machine adds the feature AC_CPU_CR4_PKE. The default
>> timeout is 180 seconds. Seen this problem both on AMD and Intel machines.
>>
>> #./tests/access
>> qemu-system-x86_64: terminating on signal 15 from pid 20050 (timeout)
>> FAIL access (timeout; duration=180)
>>
>> This test can take about 7 minutes without timeout.
>> time ./tests/access
>> 58982405 tests, 0 failures
>> PASS access
>>
>> real	7m10.063s
>> user	7m9.063s
>> sys	0m0.309s
>>
>> Fix the problem by adding few more limit checks.
> 
> Please state somewhere in the changelog what is actually being changed, and the
> actual effect of the change.  E.g.
> 
>   Disallow protection keys testcase in combination with reserved bit
>   testcasess to further limit the number of tests run to avoid timeouts on
>   systems with support for protection keys.
> 
>   Disallowing this combination reduces the total number of tests from
>   58982405 to <???>, and the runtime from ~7 minutes to <???>

Sure. Will do.
> 
>> Signed-off-by: Babu Moger <Babu.Moger@amd.com>
>> ---
>>  x86/access.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/x86/access.c b/x86/access.c
>> index 47807cc..e371dd5 100644
>> --- a/x86/access.c
>> +++ b/x86/access.c
>> @@ -317,9 +317,9 @@ static _Bool ac_test_legal(ac_test_t *at)
>>      /*
>>       * Shorten the test by avoiding testing too many reserved bit combinations
>>       */
>> -    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13)) > 1)
>> +    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13) + F(AC_CPU_CR4_PKE)) > 1)
>>          return false;
>> -    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
>> +    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36) + F(AC_CPU_CR4_PKE)) > 1)
> 
> Why are protection keys the sacrifical lamb?  Simply because they're the newest?

Yes. I added it because it was new ):.
> 
> And before we start killing off arbitrary combinations, what about sharding the
> test so that testcases that depend on a specific CR0/CR4/EFER bit, i.e. trigger
> a VM-Exit when the configuration changes, are separate runs?  Being able to run
> a specific combination would also hopefully make it easier to debug issues as
> the user could specify which combo to run without having to modify the code and
> recompile.
> 
> That probably won't actually reduce the total run time, but it would make each
> run a separate test and give developers a warm fuzzy feeling that they're indeed
> making progress :-)
> 
> Not sure how this could be automagically expressed this in unittest.cfg though...

Let me investigate if we can do that fairly easy. Will let you know.
Thanks
Babu
> 
>>          return false;
>>  
>>      return true;
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations
  2021-08-09 19:43     ` Babu Moger
@ 2021-08-10 16:59       ` Babu Moger
  2021-08-10 23:38         ` Babu Moger
  0 siblings, 1 reply; 10+ messages in thread
From: Babu Moger @ 2021-08-10 16:59 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: pbonzini, thuth, drjones, kvm



On 8/9/21 2:43 PM, Babu Moger wrote:
> 
> 
> On 8/6/21 11:53 AM, Sean Christopherson wrote:
>> On Fri, Aug 06, 2021, Babu Moger wrote:
>>> From: Babu Moger <Babu.Moger@amd.com>
>>>
>>> The test ./x86/access fails with a timeout. This is due to the number test
>>> combination. The test cases increase exponentially as the features get
>>> enabled. The new machine adds the feature AC_CPU_CR4_PKE. The default
>>> timeout is 180 seconds. Seen this problem both on AMD and Intel machines.
>>>
>>> #./tests/access
>>> qemu-system-x86_64: terminating on signal 15 from pid 20050 (timeout)
>>> FAIL access (timeout; duration=180)
>>>
>>> This test can take about 7 minutes without timeout.
>>> time ./tests/access
>>> 58982405 tests, 0 failures
>>> PASS access
>>>
>>> real	7m10.063s
>>> user	7m9.063s
>>> sys	0m0.309s
>>>
>>> Fix the problem by adding few more limit checks.
>>
>> Please state somewhere in the changelog what is actually being changed, and the
>> actual effect of the change.  E.g.
>>
>>   Disallow protection keys testcase in combination with reserved bit
>>   testcasess to further limit the number of tests run to avoid timeouts on
>>   systems with support for protection keys.
>>
>>   Disallowing this combination reduces the total number of tests from
>>   58982405 to <???>, and the runtime from ~7 minutes to <???>
> 
> Sure. Will do.
>>
>>> Signed-off-by: Babu Moger <Babu.Moger@amd.com>
>>> ---
>>>  x86/access.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/x86/access.c b/x86/access.c
>>> index 47807cc..e371dd5 100644
>>> --- a/x86/access.c
>>> +++ b/x86/access.c
>>> @@ -317,9 +317,9 @@ static _Bool ac_test_legal(ac_test_t *at)
>>>      /*
>>>       * Shorten the test by avoiding testing too many reserved bit combinations
>>>       */
>>> -    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13)) > 1)
>>> +    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13) + F(AC_CPU_CR4_PKE)) > 1)
>>>          return false;
>>> -    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
>>> +    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36) + F(AC_CPU_CR4_PKE)) > 1)
>>
>> Why are protection keys the sacrifical lamb?  Simply because they're the newest?
> 
> Yes. I added it because it was new ):.
>>
>> And before we start killing off arbitrary combinations, what about sharding the
>> test so that testcases that depend on a specific CR0/CR4/EFER bit, i.e. trigger
>> a VM-Exit when the configuration changes, are separate runs?  Being able to run
>> a specific combination would also hopefully make it easier to debug issues as
>> the user could specify which combo to run without having to modify the code and
>> recompile.
>>
>> That probably won't actually reduce the total run time, but it would make each
>> run a separate test and give developers a warm fuzzy feeling that they're indeed
>> making progress :-)
>>
>> Not sure how this could be automagically expressed this in unittest.cfg though...

As we know now that we cannot run a huge number of tests without running
into timeout, I was thinking of adding a extra parameter "max_runs" for
these tests and add a check in ac_test_run to limit the number of runs.
The max_runs will be set to default 10000000. But it can be changed in
unittests.cfg. Something like this.

[access]
 file = access.flat
 arch = x86_64
-extra_params = -cpu max
+extra_params = -cpu max -append 10000000
 timeout = 180

Thoughts?

> 
> Let me investigate if we can do that fairly easy. Will let you know.
> Thanks
> Babu
>>
>>>          return false;
>>>  
>>>      return true;
>>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations
  2021-08-10 16:59       ` Babu Moger
@ 2021-08-10 23:38         ` Babu Moger
  2021-08-11  7:09           ` Paolo Bonzini
  0 siblings, 1 reply; 10+ messages in thread
From: Babu Moger @ 2021-08-10 23:38 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: pbonzini, thuth, drjones, kvm



On 8/10/21 11:59 AM, Babu Moger wrote:
> 
> 
> On 8/9/21 2:43 PM, Babu Moger wrote:
>>
>>
>> On 8/6/21 11:53 AM, Sean Christopherson wrote:
>>> On Fri, Aug 06, 2021, Babu Moger wrote:
>>>> From: Babu Moger <Babu.Moger@amd.com>
>>>>
>>>> The test ./x86/access fails with a timeout. This is due to the number test
>>>> combination. The test cases increase exponentially as the features get
>>>> enabled. The new machine adds the feature AC_CPU_CR4_PKE. The default
>>>> timeout is 180 seconds. Seen this problem both on AMD and Intel machines.
>>>>
>>>> #./tests/access
>>>> qemu-system-x86_64: terminating on signal 15 from pid 20050 (timeout)
>>>> FAIL access (timeout; duration=180)
>>>>
>>>> This test can take about 7 minutes without timeout.
>>>> time ./tests/access
>>>> 58982405 tests, 0 failures
>>>> PASS access
>>>>
>>>> real	7m10.063s
>>>> user	7m9.063s
>>>> sys	0m0.309s
>>>>
>>>> Fix the problem by adding few more limit checks.
>>>
>>> Please state somewhere in the changelog what is actually being changed, and the
>>> actual effect of the change.  E.g.
>>>
>>>   Disallow protection keys testcase in combination with reserved bit
>>>   testcasess to further limit the number of tests run to avoid timeouts on
>>>   systems with support for protection keys.
>>>
>>>   Disallowing this combination reduces the total number of tests from
>>>   58982405 to <???>, and the runtime from ~7 minutes to <???>
>>
>> Sure. Will do.
>>>
>>>> Signed-off-by: Babu Moger <Babu.Moger@amd.com>
>>>> ---
>>>>  x86/access.c |    4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/x86/access.c b/x86/access.c
>>>> index 47807cc..e371dd5 100644
>>>> --- a/x86/access.c
>>>> +++ b/x86/access.c
>>>> @@ -317,9 +317,9 @@ static _Bool ac_test_legal(ac_test_t *at)
>>>>      /*
>>>>       * Shorten the test by avoiding testing too many reserved bit combinations
>>>>       */
>>>> -    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13)) > 1)
>>>> +    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13) + F(AC_CPU_CR4_PKE)) > 1)
>>>>          return false;
>>>> -    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
>>>> +    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36) + F(AC_CPU_CR4_PKE)) > 1)
>>>
>>> Why are protection keys the sacrifical lamb?  Simply because they're the newest?
>>
>> Yes. I added it because it was new ):.
>>>
>>> And before we start killing off arbitrary combinations, what about sharding the
>>> test so that testcases that depend on a specific CR0/CR4/EFER bit, i.e. trigger
>>> a VM-Exit when the configuration changes, are separate runs?  Being able to run
>>> a specific combination would also hopefully make it easier to debug issues as
>>> the user could specify which combo to run without having to modify the code and
>>> recompile.
>>>
>>> That probably won't actually reduce the total run time, but it would make each
>>> run a separate test and give developers a warm fuzzy feeling that they're indeed
>>> making progress :-)
>>>
>>> Not sure how this could be automagically expressed this in unittest.cfg though...
> 
> As we know now that we cannot run a huge number of tests without running
> into timeout, I was thinking of adding a extra parameter "max_runs" for
> these tests and add a check in ac_test_run to limit the number of runs.
> The max_runs will be set to default 10000000. But it can be changed in
> unittests.cfg. Something like this.
> 
> [access]
>  file = access.flat
>  arch = x86_64
> -extra_params = -cpu max
> +extra_params = -cpu max -append 10000000

No. This will not work. The PKU feature flag is bit 30. That is 2^30
iterations to cover the tests for this feature. Looks like I need to split
the tests into PKU and non PKU tests. For PKU tests I may need to change
the bump frequency (in ac_test_bump_one) to much higher value. Right now,
it is 1. Let me try that,

>  timeout = 180
> 
> Thoughts?
> 
>>
>> Let me investigate if we can do that fairly easy. Will let you know.
>> Thanks
>> Babu
>>>
>>>>          return false;
>>>>  
>>>>      return true;
>>>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations
  2021-08-10 23:38         ` Babu Moger
@ 2021-08-11  7:09           ` Paolo Bonzini
  2021-08-11 16:03             ` Babu Moger
  0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2021-08-11  7:09 UTC (permalink / raw)
  To: Babu Moger, Sean Christopherson; +Cc: thuth, drjones, kvm

On 11/08/21 01:38, Babu Moger wrote:
> No. This will not work. The PKU feature flag is bit 30. That is 2^30
> iterations to cover the tests for this feature. Looks like I need to split
> the tests into PKU and non PKU tests. For PKU tests I may need to change
> the bump frequency (in ac_test_bump_one) to much higher value. Right now,
> it is 1. Let me try that,

The simplest way to cut on tests, which is actually similar to this 
patch, would be:

- do not try all combinations of PTE access bits when reserved bits are set

- do not try combinations with more than one reserved bit set

Paolo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations
  2021-08-11  7:09           ` Paolo Bonzini
@ 2021-08-11 16:03             ` Babu Moger
  2021-08-11 16:13               ` Sean Christopherson
  0 siblings, 1 reply; 10+ messages in thread
From: Babu Moger @ 2021-08-11 16:03 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson; +Cc: thuth, drjones, kvm



On 8/11/21 2:09 AM, Paolo Bonzini wrote:
> On 11/08/21 01:38, Babu Moger wrote:
>> No. This will not work. The PKU feature flag is bit 30. That is 2^30
>> iterations to cover the tests for this feature. Looks like I need to split
>> the tests into PKU and non PKU tests. For PKU tests I may need to change
>> the bump frequency (in ac_test_bump_one) to much higher value. Right now,
>> it is 1. Let me try that,
> 
> The simplest way to cut on tests, which is actually similar to this patch,
> would be:
> 
> - do not try all combinations of PTE access bits when reserved bits are set
> 
> - do not try combinations with more than one reserved bit set

Did you mean this? Just doing this reduces the combination by huge number.
I don't need to add your first PTE access combinations.

diff --git a/x86/access.c b/x86/access.c
index 47807cc..a730b6b 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -317,9 +317,7 @@ static _Bool ac_test_legal(ac_test_t *at)
     /*
      * Shorten the test by avoiding testing too many reserved bit
combinations
      */
-    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13)) > 1)
-        return false;
-    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
+    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13) +
F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
         return false;

     return true;


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations
  2021-08-11 16:03             ` Babu Moger
@ 2021-08-11 16:13               ` Sean Christopherson
  2021-08-11 16:43                 ` Babu Moger
  0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2021-08-11 16:13 UTC (permalink / raw)
  To: Babu Moger; +Cc: Paolo Bonzini, thuth, drjones, kvm

On Wed, Aug 11, 2021, Babu Moger wrote:
> 
> On 8/11/21 2:09 AM, Paolo Bonzini wrote:
> > On 11/08/21 01:38, Babu Moger wrote:
> >> No. This will not work. The PKU feature flag is bit 30. That is 2^30
> >> iterations to cover the tests for this feature. Looks like I need to split
> >> the tests into PKU and non PKU tests. For PKU tests I may need to change
> >> the bump frequency (in ac_test_bump_one) to much higher value. Right now,
> >> it is 1. Let me try that,
> > 
> > The simplest way to cut on tests, which is actually similar to this patch,
> > would be:
> > 
> > - do not try all combinations of PTE access bits when reserved bits are set
> > 
> > - do not try combinations with more than one reserved bit set
> 
> Did you mean this? Just doing this reduces the combination by huge number.
> I don't need to add your first PTE access combinations.
> 
> diff --git a/x86/access.c b/x86/access.c
> index 47807cc..a730b6b 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -317,9 +317,7 @@ static _Bool ac_test_legal(ac_test_t *at)
>      /*
>       * Shorten the test by avoiding testing too many reserved bit
> combinations
>       */
> -    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13)) > 1)
> -        return false;
> -    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
> +    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13) +
> F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
>          return false;
> 
>      return true;

Looks good to me, is it sufficient to keep the overall runtime sane?.  And maybe
update the comment too, e.g. something like

	/*
	 * Skip testing multiple reserved bits to shorten the test.  Reserved
	 * bit page faults are terminal and multiple reserved bits do not affect
	 * the error code; the odds of a KVM bug are super low, and the odds of
	 * actually being able to detect a bug are even lower.
	 */

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations
  2021-08-11 16:13               ` Sean Christopherson
@ 2021-08-11 16:43                 ` Babu Moger
  0 siblings, 0 replies; 10+ messages in thread
From: Babu Moger @ 2021-08-11 16:43 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: Paolo Bonzini, thuth, drjones, kvm



On 8/11/21 11:13 AM, Sean Christopherson wrote:
> On Wed, Aug 11, 2021, Babu Moger wrote:
>>
>> On 8/11/21 2:09 AM, Paolo Bonzini wrote:
>>> On 11/08/21 01:38, Babu Moger wrote:
>>>> No. This will not work. The PKU feature flag is bit 30. That is 2^30
>>>> iterations to cover the tests for this feature. Looks like I need to split
>>>> the tests into PKU and non PKU tests. For PKU tests I may need to change
>>>> the bump frequency (in ac_test_bump_one) to much higher value. Right now,
>>>> it is 1. Let me try that,
>>>
>>> The simplest way to cut on tests, which is actually similar to this patch,
>>> would be:
>>>
>>> - do not try all combinations of PTE access bits when reserved bits are set
>>>
>>> - do not try combinations with more than one reserved bit set
>>
>> Did you mean this? Just doing this reduces the combination by huge number.
>> I don't need to add your first PTE access combinations.
>>
>> diff --git a/x86/access.c b/x86/access.c
>> index 47807cc..a730b6b 100644
>> --- a/x86/access.c
>> +++ b/x86/access.c
>> @@ -317,9 +317,7 @@ static _Bool ac_test_legal(ac_test_t *at)
>>      /*
>>       * Shorten the test by avoiding testing too many reserved bit
>> combinations
>>       */
>> -    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13)) > 1)
>> -        return false;
>> -    if ((F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
>> +    if ((F(AC_PDE_BIT51) + F(AC_PDE_BIT36) + F(AC_PDE_BIT13) +
>> F(AC_PTE_BIT51) + F(AC_PTE_BIT36)) > 1)
>>          return false;
>>
>>      return true;
> 
> Looks good to me, is it sufficient to keep the overall runtime sane?.  And maybe

It keeps the running time about 2 minutes.

> update the comment too, e.g. something like
> 
> 	/*
> 	 * Skip testing multiple reserved bits to shorten the test.  Reserved
> 	 * bit page faults are terminal and multiple reserved bits do not affect
> 	 * the error code; the odds of a KVM bug are super low, and the odds of
> 	 * actually being able to detect a bug are even lower.
> 	 */
> 

Sure. Will update the commit log.
Thanks
Babu

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-08-11 16:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-06 16:08 [kvm-unit-tests PATCH 0/2] Couple of SVM fixes Babu Moger
2021-08-06 16:08 ` [kvm-unit-tests PATCH 1/2] x86: access: Fix timeout failure by limiting number of flag combinations Babu Moger
     [not found]   ` <YQ1pA9nN6DP0veQ1@google.com>
2021-08-09 19:43     ` Babu Moger
2021-08-10 16:59       ` Babu Moger
2021-08-10 23:38         ` Babu Moger
2021-08-11  7:09           ` Paolo Bonzini
2021-08-11 16:03             ` Babu Moger
2021-08-11 16:13               ` Sean Christopherson
2021-08-11 16:43                 ` Babu Moger
2021-08-06 16:08 ` [kvm-unit-tests PATCH 2/2] nSVM: Fix NPT reserved bits test hang Babu Moger

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.