linux-kernel - Re: [PATCH 04/11] KVM: selftests: Rework OSXSAVE CR4=>CPUID test to play nice with AVX insns

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87r08wi6l1.fsf@redhat.com>
Date: Fri, 04 Oct 2024 11:02:34 +0200
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Sean Christopherson
 <seanjc@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/11] KVM: selftests: Rework OSXSAVE CR4=>CPUID test to
 play nice with AVX insns

Sean Christopherson <seanjc@...gle.com> writes:

> Rework the CR4/CPUID sync test to clear CR4.OSXSAVE, do CPUID, and restore
> CR4.OSXSAVE in assembly, so that there is zero chance of AVX instructions
> being executed while CR4.OSXSAVE is disabled.  This will allow enabling
> CR4.OSXSAVE by default for selftests vCPUs as a general means of playing
> nice with AVX instructions.
>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  .../kvm/x86_64/cr4_cpuid_sync_test.c          | 46 +++++++++++++------
>  1 file changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> index 624dc725e14d..da818afb7031 100644
> --- a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> @@ -19,15 +19,14 @@
>  #include "kvm_util.h"
>  #include "processor.h"
>  
> -static inline bool cr4_cpuid_is_sync(void)
> -{
> -	uint64_t cr4 = get_cr4();
> -
> -	return (this_cpu_has(X86_FEATURE_OSXSAVE) == !!(cr4 & X86_CR4_OSXSAVE));
> -}
> +#define MAGIC_HYPERCALL_PORT	0x80
>  
>  static void guest_code(void)
>  {
> +	u32 regs[4] = {
> +		[KVM_CPUID_EAX] = X86_FEATURE_OSXSAVE.function,
> +		[KVM_CPUID_ECX] = X86_FEATURE_OSXSAVE.index,
> +	};
>  	uint64_t cr4;
>  
>  	/* turn on CR4.OSXSAVE */
> @@ -36,13 +35,29 @@ static void guest_code(void)
>  	set_cr4(cr4);
>  
>  	/* verify CR4.OSXSAVE == CPUID.OSXSAVE */
> -	GUEST_ASSERT(cr4_cpuid_is_sync());
> +	GUEST_ASSERT(this_cpu_has(X86_FEATURE_OSXSAVE));
>  
> -	/* notify hypervisor to change CR4 */
> -	GUEST_SYNC(0);
> +	/*
> +	 * Notify hypervisor to clear CR4.0SXSAVE, do CPUID and save output,
> +	 * and then restore CR4.  Do this all in  assembly to ensure no AVX
> +	 * instructions are executed while OSXSAVE=0.
> +	 */
> +	asm volatile (
> +		"out %%al, $" __stringify(MAGIC_HYPERCALL_PORT) "\n\t"
> +		"cpuid\n\t"
> +		"mov %%rdi, %%cr4\n\t"
> +		: "+a" (regs[KVM_CPUID_EAX]),
> +		  "=b" (regs[KVM_CPUID_EBX]),
> +		  "+c" (regs[KVM_CPUID_ECX]),
> +		  "=d" (regs[KVM_CPUID_EDX])
> +		: "D" (get_cr4())
> +	);
>  
> -	/* check again */
> -	GUEST_ASSERT(cr4_cpuid_is_sync());
> +	/* Verify KVM cleared OSXSAVE in CPUID when it was cleared in CR4. */
> +	GUEST_ASSERT(!(regs[X86_FEATURE_OSXSAVE.reg] & BIT(X86_FEATURE_OSXSAVE.bit)));
> +
> +	/* Verify restoring CR4 also restored OSXSAVE in CPUID. */
> +	GUEST_ASSERT(this_cpu_has(X86_FEATURE_OSXSAVE));
>  
>  	GUEST_DONE();
>  }
> @@ -62,13 +77,16 @@ int main(int argc, char *argv[])
>  		vcpu_run(vcpu);
>  		TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  
> -		switch (get_ucall(vcpu, &uc)) {
> -		case UCALL_SYNC:
> +		if (vcpu->run->io.port == MAGIC_HYPERCALL_PORT &&
> +		    vcpu->run->io.direction == KVM_EXIT_IO_OUT) {
>  			/* emulate hypervisor clearing CR4.OSXSAVE */
>  			vcpu_sregs_get(vcpu, &sregs);
>  			sregs.cr4 &= ~X86_CR4_OSXSAVE;
>  			vcpu_sregs_set(vcpu, &sregs);
> -			break;
> +			continue;
> +		}
> +
> +		switch (get_ucall(vcpu, &uc)) {
>  		case UCALL_ABORT:
>  			REPORT_GUEST_ASSERT(uc);
>  			break;

Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>

-- 
Vitaly