All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: selftest: Move XFD CPUID checking out of __vm_xsave_require_permission()
@ 2022-11-25  2:38 Wang, Lei
  2022-11-28 17:03 ` Sean Christopherson
  0 siblings, 1 reply; 3+ messages in thread
From: Wang, Lei @ 2022-11-25  2:38 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, seanjc, yang.zhong

kvm_cpu_has(X86_FEATURE_XFD) will call kvm_get_supported_cpuid() which will
cache the cpuid information when it is firstly called. Move this line out
of __vm_xsave_require_permission() and check it afterwards so that the
CPUID change will not be veiled by the cached CPUID information.

Signed-off-by: Wang, Lei <lei4.wang@intel.com>
---
 tools/testing/selftests/kvm/lib/x86_64/processor.c | 2 --
 tools/testing/selftests/kvm/x86_64/amx_test.c      | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 39c4409ef56a..5686eacd4700 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -616,8 +616,6 @@ void __vm_xsave_require_permission(int bit, const char *name)
 		.addr = (unsigned long) &bitmask
 	};
 
-	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XFD));
-
 	kvm_fd = open_kvm_dev_path_or_exit();
 	rc = __kvm_ioctl(kvm_fd, KVM_GET_DEVICE_ATTR, &attr);
 	close(kvm_fd);
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index dadcbad10a1d..1e3457ff304b 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -312,6 +312,7 @@ int main(int argc, char *argv[])
 	/* Create VM */
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 
+	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XFD));
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE));
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_AMX_TILE));
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XTILECFG));
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: selftest: Move XFD CPUID checking out of __vm_xsave_require_permission()
  2022-11-25  2:38 [PATCH] KVM: selftest: Move XFD CPUID checking out of __vm_xsave_require_permission() Wang, Lei
@ 2022-11-28 17:03 ` Sean Christopherson
  2022-11-29  1:01   ` Wang, Lei
  0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2022-11-28 17:03 UTC (permalink / raw)
  To: Wang, Lei; +Cc: kvm, pbonzini, yang.zhong

On Thu, Nov 24, 2022, Wang, Lei wrote:
> kvm_cpu_has(X86_FEATURE_XFD) will call kvm_get_supported_cpuid() which will
> cache the cpuid information when it is firstly called. Move this line out
> of __vm_xsave_require_permission() and check it afterwards so that the
> CPUID change will not be veiled by the cached CPUID information.

Please call out exactly what CPUID change is being referred to.  Someone that
doesn't already know about ARCH_REQ_XCOMP_GUEST_PERM and it's interaction with
KVM_GET_SUPPORTED_CPUID will have zero clue what this fixes.

E.g.

Move the kvm_cpu_has() check on X86_FEATURE_XFD out of the helper to
enable off-by-default XSAVE-managed features and into the one test that
currenty requires XFD (XFeature Disable) support.   kvm_cpu_has() uses
kvm_get_supported_cpuid() and thus caches KVM_GET_SUPPORTED_CPUID, and so
using kvm_cpu_has() before ARCH_REQ_XCOMP_GUEST_PERM effectively results
in the test caching stale values, e.g. subsequent checks on AMX_TILE will
get false negatives.

Although off-by-default features are nonsensical without XFD, checking
for XFD virtualization prior to enabling such features isn't strictly
required.

Fixes: 7fbb653e01fd ("KVM: selftests: Check KVM's supported CPUID, not host CPUID, for XFD")

> Signed-off-by: Wang, Lei <lei4.wang@intel.com>
> ---
>  tools/testing/selftests/kvm/lib/x86_64/processor.c | 2 --
>  tools/testing/selftests/kvm/x86_64/amx_test.c      | 1 +
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 39c4409ef56a..5686eacd4700 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -616,8 +616,6 @@ void __vm_xsave_require_permission(int bit, const char *name)
>  		.addr = (unsigned long) &bitmask
>  	};
>  
> -	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XFD));
> -
>  	kvm_fd = open_kvm_dev_path_or_exit();
>  	rc = __kvm_ioctl(kvm_fd, KVM_GET_DEVICE_ATTR, &attr);
>  	close(kvm_fd);
> diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
> index dadcbad10a1d..1e3457ff304b 100644
> --- a/tools/testing/selftests/kvm/x86_64/amx_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
> @@ -312,6 +312,7 @@ int main(int argc, char *argv[])
>  	/* Create VM */
>  	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
>  
> +	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XFD));

I think we should disallow kvm_get_supported_cpuid() before
__vm_xsave_require_permission(), otherwise we'll reintroduce a similar bug in the
future.
 
>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE));
>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_AMX_TILE));
>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XTILECFG));

And then as a follow-up, we should move these above vm_create_with_one_vcpu(),
checking them after vm_create_with_one_vcpu() is odd.

I'll send a v2 with the reworded changelog and additional patches to assert that
__vm_xsave_require_permission() isn't used after kvm_get_supported_cpuid().

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: selftest: Move XFD CPUID checking out of __vm_xsave_require_permission()
  2022-11-28 17:03 ` Sean Christopherson
@ 2022-11-29  1:01   ` Wang, Lei
  0 siblings, 0 replies; 3+ messages in thread
From: Wang, Lei @ 2022-11-29  1:01 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, pbonzini, yang.zhong

On 11/29/2022 1:03 AM, Sean Christopherson wrote:
> On Thu, Nov 24, 2022, Wang, Lei wrote:
>> kvm_cpu_has(X86_FEATURE_XFD) will call kvm_get_supported_cpuid() which will
>> cache the cpuid information when it is firstly called. Move this line out
>> of __vm_xsave_require_permission() and check it afterwards so that the
>> CPUID change will not be veiled by the cached CPUID information.
> 
> Please call out exactly what CPUID change is being referred to.  Someone that
> doesn't already know about ARCH_REQ_XCOMP_GUEST_PERM and it's interaction with
> KVM_GET_SUPPORTED_CPUID will have zero clue what this fixes.
> 
> E.g.
> 
> Move the kvm_cpu_has() check on X86_FEATURE_XFD out of the helper to
> enable off-by-default XSAVE-managed features and into the one test that
> currenty requires XFD (XFeature Disable) support.   kvm_cpu_has() uses
> kvm_get_supported_cpuid() and thus caches KVM_GET_SUPPORTED_CPUID, and so
> using kvm_cpu_has() before ARCH_REQ_XCOMP_GUEST_PERM effectively results
> in the test caching stale values, e.g. subsequent checks on AMX_TILE will
> get false negatives.
> 
> Although off-by-default features are nonsensical without XFD, checking
> for XFD virtualization prior to enabling such features isn't strictly
> required.
> 
> Fixes: 7fbb653e01fd ("KVM: selftests: Check KVM's supported CPUID, not host CPUID, for XFD")

Makes sense.

>> Signed-off-by: Wang, Lei <lei4.wang@intel.com>
>> ---
>>  tools/testing/selftests/kvm/lib/x86_64/processor.c | 2 --
>>  tools/testing/selftests/kvm/x86_64/amx_test.c      | 1 +
>>  2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> index 39c4409ef56a..5686eacd4700 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> @@ -616,8 +616,6 @@ void __vm_xsave_require_permission(int bit, const char *name)
>>  		.addr = (unsigned long) &bitmask
>>  	};
>>  
>> -	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XFD));
>> -
>>  	kvm_fd = open_kvm_dev_path_or_exit();
>>  	rc = __kvm_ioctl(kvm_fd, KVM_GET_DEVICE_ATTR, &attr);
>>  	close(kvm_fd);
>> diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
>> index dadcbad10a1d..1e3457ff304b 100644
>> --- a/tools/testing/selftests/kvm/x86_64/amx_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
>> @@ -312,6 +312,7 @@ int main(int argc, char *argv[])
>>  	/* Create VM */
>>  	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
>>  
>> +	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XFD));
> 
> I think we should disallow kvm_get_supported_cpuid() before
> __vm_xsave_require_permission(), otherwise we'll reintroduce a similar bug in the
> future.

Agree.

>>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE));
>>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_AMX_TILE));
>>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XTILECFG));
> 
> And then as a follow-up, we should move these above vm_create_with_one_vcpu(),
> checking them after vm_create_with_one_vcpu() is odd.
> 
> I'll send a v2 with the reworded changelog and additional patches to assert that
> __vm_xsave_require_permission() isn't used after kvm_get_supported_cpuid().

Already seen your patches, thanks Sean!

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-29  1:01 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-25  2:38 [PATCH] KVM: selftest: Move XFD CPUID checking out of __vm_xsave_require_permission() Wang, Lei
2022-11-28 17:03 ` Sean Christopherson
2022-11-29  1:01   ` Wang, Lei

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.