diff mbox series

[v5,07/21] x86/fpu: Provide fpu_enable_guest_xfd_features() for KVM

Message ID 20220105123532.12586-8-yang.zhong@intel.com (mailing list archive)
State New
Headers show
Series AMX Support in KVM | expand

Commit Message

Yang Zhong Jan. 5, 2022, 12:35 p.m. UTC 
From: Sean Christopherson <seanjc@google.com>

Provide a wrapper for expanding the guest fpstate buffer according
to requested xfeatures. KVM wants to call this wrapper to manage
any dynamic xstate used by the guest.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 arch/x86/include/asm/fpu/api.h |  1 +
 arch/x86/kernel/fpu/core.c     | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

Comments

Paolo Bonzini Jan. 5, 2022, 1:06 p.m. UTC | #1 
On 1/5/22 13:35, Yang Zhong wrote:
> +int fpu_enable_guest_xfd_features(struct fpu_guest *guest_fpu, u64 xfeatures)
> +{
> +	lockdep_assert_preemption_enabled();
> +

The old fpu_update_guest_perm_features(guest_fpu) is equivalent to

	fpu_enable_guest_xfd_features(guest_fpu, guest_fpu->perm);

Missing doc comment:

/*
  * fpu_enable_guest_xfd_features - Enable xfeatures according to guest perm
  * @guest_fpu:         Pointer to the guest FPU container
  * @xfeatures:         Features requested by guest CPUID
  *
  * Enable all dynamic xfeatures according to guest perm and requested CPUID.
  * Invoked if the caller wants to conservatively expand fpstate buffer instead
  * of waiting until XCR0 or XFD MSR is written.
  *
  * Return: 0 on success, error code otherwise
  */

Also, the check for 32-bit is slightly imprecise:

	/* Dynamic xfeatures are not supported with 32-bit kernels. */
	if (!IS_ENABLED(CONFIG_X86_64))
-		return 0;
+		return -EINVAL;

since we only get here with xfeatures != 0 (if it compiles, just removing
the IS_ENABLED check altogether would be even better).  With these changes,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks,

Paolo
Tian, Kevin Jan. 6, 2022, 12:51 a.m. UTC | #2 
> From: Paolo Bonzini <pbonzini@redhat.com>
> Sent: Wednesday, January 5, 2022 9:07 PM
> 
> On 1/5/22 13:35, Yang Zhong wrote:
> > +int fpu_enable_guest_xfd_features(struct fpu_guest *guest_fpu, u64
> xfeatures)
> > +{
> > +	lockdep_assert_preemption_enabled();
> > +
> 
> The old fpu_update_guest_perm_features(guest_fpu) is equivalent to
> 
> 	fpu_enable_guest_xfd_features(guest_fpu, guest_fpu->perm);
> 
> Missing doc comment:
> 
> /*
>   * fpu_enable_guest_xfd_features - Enable xfeatures according to guest
> perm
>   * @guest_fpu:         Pointer to the guest FPU container
>   * @xfeatures:         Features requested by guest CPUID
>   *
>   * Enable all dynamic xfeatures according to guest perm and requested
> CPUID.
>   * Invoked if the caller wants to conservatively expand fpstate buffer instead
>   * of waiting until XCR0 or XFD MSR is written.
>   *
>   * Return: 0 on success, error code otherwise
>   */

It's not equivalent. The old interface enables all xfeatures allowed by
guest perm while the new one just enables feature bits according to
the caller request. It also becomes a more general interface instead of
being only for conservative expansion. Since both points in the old
comment don't hold now and this function is obvious, we didn't put
a comment here (on par with other KVM helpers in that block).

If still necessary we could add one like below:

/*
  * fpu_enable_guest_xfd_features - Enable xfeatures for guest fpu container
  * @guest_fpu:         Pointer to the guest FPU container
  * @xfeatures:         Features requested by the caller
  *
  * Enable dynamic xfeatures and expand guest fpstate buffer accordingly.
  * KVM should call this function before the requested xfeatures are used
  * by the guest.
  *
  * Return: 0 on success, error code otherwise
  */

> 
> Also, the check for 32-bit is slightly imprecise:
> 
> 	/* Dynamic xfeatures are not supported with 32-bit kernels. */
> 	if (!IS_ENABLED(CONFIG_X86_64))
> -		return 0;
> +		return -EINVAL;
> 
> since we only get here with xfeatures != 0 (if it compiles, just removing
> the IS_ENABLED check altogether would be even better).  With these changes,
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks,
> 
> Paolo
Yang Zhong Jan. 6, 2022, 6:02 a.m. UTC | #3 
On Wed, Jan 05, 2022 at 02:06:40PM +0100, Paolo Bonzini wrote:
> On 1/5/22 13:35, Yang Zhong wrote:
> >+int fpu_enable_guest_xfd_features(struct fpu_guest *guest_fpu, u64 xfeatures)
> >+{
> >+	lockdep_assert_preemption_enabled();
> >+
> 
> The old fpu_update_guest_perm_features(guest_fpu) is equivalent to
> 
> 	fpu_enable_guest_xfd_features(guest_fpu, guest_fpu->perm);
> 
> Missing doc comment:
> 
> /*
>  * fpu_enable_guest_xfd_features - Enable xfeatures according to guest perm
>  * @guest_fpu:         Pointer to the guest FPU container
>  * @xfeatures:         Features requested by guest CPUID
>  *
>  * Enable all dynamic xfeatures according to guest perm and requested CPUID.
>  * Invoked if the caller wants to conservatively expand fpstate buffer instead
>  * of waiting until XCR0 or XFD MSR is written.
>  *
>  * Return: 0 on success, error code otherwise
>  */
> 
> Also, the check for 32-bit is slightly imprecise:
> 
> 	/* Dynamic xfeatures are not supported with 32-bit kernels. */
> 	if (!IS_ENABLED(CONFIG_X86_64))
> -		return 0;
> +		return -EINVAL;
> 
> since we only get here with xfeatures != 0 (if it compiles, just removing
> the IS_ENABLED check altogether would be even better).  With these changes,
> 

  Paolo, I did 32 bit kernel build tests

  (1). w/ IS_ENABLED(CONFIG_X86_64)

      if (!IS_ENABLED(CONFIG_X86_64))
      	return -EINVAL; 
    
     This 32 bit kernel can successfully build.

  (2). remove IS_ENABLED(CONFIG_X86_64)

      The 32 bit kernel build is failed, and the error as:

      ld: arch/x86/kernel/fpu/core.o: in function `fpu_enable_guest_xfd_features':
/root/amx/v5/kvm/arch/x86/kernel/fpu/core.c:278: undefined reference to `__xfd_enable_feature'

      Seems we need define 32 bit API for __xfd_enable_feature().

   I also tried (1) in desktop, but I could not reboot the machine after installed 32 bit kernel.

   thanks a lot!

   Yang 





> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks,
> 
> Paolo
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index d8c222290e68..1ed2a247a84e 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -138,6 +138,7 @@  extern inline u64 xstate_get_guest_group_perm(void);
 extern bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu);
 extern void fpu_free_guest_fpstate(struct fpu_guest *gfpu);
 extern int fpu_swap_kvm_fpstate(struct fpu_guest *gfpu, bool enter_guest);
+extern int fpu_enable_guest_xfd_features(struct fpu_guest *guest_fpu, u64 xfeatures);
 
 extern void fpu_copy_guest_fpstate_to_uabi(struct fpu_guest *gfpu, void *buf, unsigned int size, u32 pkru);
 extern int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf, u64 xcr0, u32 *vpkru);
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index a78bc547fc03..5b08d20a4005 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -261,6 +261,23 @@  void fpu_free_guest_fpstate(struct fpu_guest *gfpu)
 }
 EXPORT_SYMBOL_GPL(fpu_free_guest_fpstate);
 
+int fpu_enable_guest_xfd_features(struct fpu_guest *guest_fpu, u64 xfeatures)
+{
+	lockdep_assert_preemption_enabled();
+
+	/* Nothing to do if all requested features are already enabled. */
+	xfeatures &= ~guest_fpu->xfeatures;
+	if (!xfeatures)
+		return 0;
+
+	/* Dynamic xfeatures are not supported with 32-bit kernels. */
+	if (!IS_ENABLED(CONFIG_X86_64))
+		return 0;
+
+	return __xfd_enable_feature(xfeatures, guest_fpu);
+}
+EXPORT_SYMBOL_GPL(fpu_enable_guest_xfd_features);
+
 int fpu_swap_kvm_fpstate(struct fpu_guest *guest_fpu, bool enter_guest)
 {
 	struct fpstate *guest_fps = guest_fpu->fpstate;