linux-kernel - Re: [PATCH 08/19] x86/fpu: Move xfd_update

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <874k7gxepi.ffs@tglx>
Date:   Fri, 10 Dec 2021 23:44:25 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Yang Zhong <yang.zhong@...el.com>, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        pbonzini@...hat.com
Cc:     seanjc@...gle.com, jun.nakajima@...el.com, kevin.tian@...el.com,
        jing2.liu@...ux.intel.com, jing2.liu@...el.com,
        yang.zhong@...el.com
Subject: Re: [PATCH 08/19] x86/fpu: Move xfd_update_state() to xstate.c and
 export symbol

On Tue, Dec 07 2021 at 19:03, Yang Zhong wrote:
> From: Jing Liu <jing2.liu@...el.com>
>
> xfd_update_state() is the interface to update IA32_XFD and its per-cpu
> cache. All callers of this interface are currently in fpu core. KVM only
> indirectly triggers IA32_XFD update via a helper function
> (fpu_swap_kvm_fpstate()) when switching between user fpu and guest fpu.
>
> Supporting AMX in guest now requires KVM to directly update IA32_XFD
> with the guest value (when emulating WRMSR) so XSAVE/XRSTOR can manage
> XSTATE components correctly inside guest.
>
> This patch moves xfd_update_state() from fpu/xstate.h to fpu/xstate.c

s/This patch moves/Move/

please. See Documentation/process/submitting-patches.rst and search for
'This patch'

> and export it for reference outside of fpu core.
>
> Signed-off-by: Jing Liu <jing2.liu@...el.com>
> Signed-off-by: Yang Zhong <yang.zhong@...el.com>
> ---
>  arch/x86/include/asm/fpu/api.h |  2 ++
>  arch/x86/kernel/fpu/xstate.c   | 12 ++++++++++++
>  arch/x86/kernel/fpu/xstate.h   | 14 +-------------
>  3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
> index 7532f73c82a6..999d89026be9 100644
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -131,8 +131,10 @@ DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
>  /* Process cleanup */
>  #ifdef CONFIG_X86_64
>  extern void fpstate_free(struct fpu *fpu);
> +extern void xfd_update_state(struct fpstate *fpstate);
>  #else
>  static inline void fpstate_free(struct fpu *fpu) { }
> +static void xfd_update_state(struct fpstate *fpstate) { }

Try a 32bit build to see the warnings this causes. That wants to be
'static inline void' obviously.

>  #ifdef CONFIG_X86_64
> -static inline void xfd_update_state(struct fpstate *fpstate)
> -{
> -	if (fpu_state_size_dynamic()) {
> -		u64 xfd = fpstate->xfd;
> -
> -		if (__this_cpu_read(xfd_state) != xfd) {
> -			wrmsrl(MSR_IA32_XFD, xfd);
> -			__this_cpu_write(xfd_state, xfd);
> -		}
> -	}
> -}
> -#else
> -static inline void xfd_update_state(struct fpstate *fpstate) { }
> +extern void xfd_update_state(struct fpstate *fpstate);

Why? It's already declared in the global header. So all of this has to
be simply removed, no?

Thanks,

        tglx