Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9f6ed500-fa60-c898-69fd-41ae4a401eb4@intel.com>
Date:   Thu, 11 May 2023 10:59:08 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32_XFD
 MSR
Content-Language: en-US
To:     Adamos Ttofari <attofari@amazon.de>
Cc:     abusse@amazon.de, dwmw@amazon.co.uk, hborghor@amazon.de,
        sironi@amazon.de, Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, Kyle Huey <me@kylehuey.com>,
        "Chang S. Bae" <chang.seok.bae@intel.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        linux-kernel@vger.kernel.org
References: <20230511152818.13839-1-attofari@amazon.de>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20230511152818.13839-1-attofari@amazon.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/11/23 08:28, Adamos Ttofari wrote:
> @@ -179,8 +179,14 @@ void fpu__init_cpu_xstate(void)
>  	 * key as that does not work on the boot CPU. This also ensures
>  	 * that any stale state is wiped out from XFD.
>  	 */
> -	if (cpu_feature_enabled(X86_FEATURE_XFD))
> -		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
> +	if (cpu_feature_enabled(X86_FEATURE_XFD)) {
> +		u64 xfd;
> +
> +		rdmsrl(MSR_IA32_XFD, xfd);
> +		__this_cpu_write(xfd_state, xfd);
> +
> +		xfd_update_state(&init_fpstate);
> +	}

The above comment didn't _quite_ make it into the context, so I'll paste
it here for your convenience:

>         /*
>          * Must happen after CR4 setup and before xsetbv() to allow KVM
>          * lazy passthrough.  Write independent of the dynamic state static
>          * key as that does not work on the boot CPU. This also ensures
>          * that any stale state is wiped out from XFD.
>          */

Translating there, "the dynamic state static key" means
'__fpu_state_size_dynamic' which is used here:

> static __always_inline __pure bool fpu_state_size_dynamic(void)
> {
>         return static_branch_unlikely(&__fpu_state_size_dynamic);
> }

You might recognize fpu_state_size_dynamic() from the first line of
xfd_update_state(), the function that you added to the above hunk.

Which brings me to ask what the point of calling xfd_update_state() is
in the first place if you're getting away with it not working on the
boot CPU.

Why not just short-circuit the (non-working) xfd_update_state() and do
this directly:

	wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
        __this_cpu_write(xfd_state, init_fpstate.xfd);

I don't think you even need to *READ* the MSR.  You're going to blow it
away anyway.

>  	/*
>  	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
> @@ -915,7 +921,7 @@ void fpu__resume_cpu(void)
>  	}
>  
>  	if (fpu_state_size_dynamic())
> -		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
> +		xfd_update_state(&init_fpstate);
>  }
>  
>  /*