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@xxxxxxxxx>
>> ---
>> 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!
