| Message ID | 20221125023839.315207-1-lei4.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftest: Move XFD CPUID checking out of __vm_xsave_require_permission() | expand |
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().
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!
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));
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(-)