lkml.org 
[lkml]   [2023]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v6 07/25] x86/fpu/xstate: Tweak guest fpstate to support kernel dynamic xfeatures
    From
    Date
    On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
    > The guest fpstate is sized with fpu_kernel_cfg.default_size (by preceding
    > fix) and the kernel dynamic xfeatures are not taken into account, so add
    > the support and tweak fpstate xfeatures and size accordingly.
    >
    > Below configuration steps are currently enforced to get guest fpstate:
    > 1) User space sets thread group xstate permits via arch_prctl().
    > 2) User space creates vcpu thread.
    > 3) User space enables guest dynamic xfeatures.
    >
    > In #1, guest fpstate size (i.e., __state_size [1]) is induced from
    > (fpu_kernel_cfg.default_features | user dynamic xfeatures) [2].
    > In #2, guest fpstate size is calculated with fpu_kernel_cfg.default_size
    > and fpstate->size is set to the same. fpstate->xfeatures is set to
    > fpu_kernel_cfg.default_features.
    > In #3, guest fpstate is re-allocated as [1] and fpstate->xfeatures is
    > set to [2].
    >
    > By adding kernel dynamic xfeatures in above #1 and #2, guest xstate area
    > size is expanded to hold (fpu_kernel_cfg.default_features | kernel dynamic
    > _xfeatures | user dynamic xfeatures)[3], and guest fpstate->xfeatures is
    > set to [3]. Then host xsaves/xrstors can act on all guest xfeatures.
    >
    > The user_* fields remain unchanged for compatibility of non-compacted KVM
    > uAPIs.
    >
    > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
    > ---
    > arch/x86/kernel/fpu/core.c | 56 +++++++++++++++++++++++++++++-------
    > arch/x86/kernel/fpu/xstate.c | 2 +-
    > arch/x86/kernel/fpu/xstate.h | 2 ++
    > 3 files changed, 49 insertions(+), 11 deletions(-)
    >
    > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
    > index a42d8ad26ce6..e5819b38545a 100644
    > --- a/arch/x86/kernel/fpu/core.c
    > +++ b/arch/x86/kernel/fpu/core.c
    > @@ -33,6 +33,8 @@ DEFINE_STATIC_KEY_FALSE(__fpu_state_size_dynamic);
    > DEFINE_PER_CPU(u64, xfd_state);
    > #endif
    >
    > +extern unsigned int xstate_calculate_size(u64 xfeatures, bool compacted);
    > +
    > /* The FPU state configuration data for kernel and user space */
    > struct fpu_state_config fpu_kernel_cfg __ro_after_init;
    > struct fpu_state_config fpu_user_cfg __ro_after_init;
    > @@ -193,8 +195,6 @@ void fpu_reset_from_exception_fixup(void)
    > }
    >
    > #if IS_ENABLED(CONFIG_KVM)
    > -static void __fpstate_reset(struct fpstate *fpstate, u64 xfd);
    > -
    > static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
    > {
    > struct fpu_state_perm *fpuperm;
    > @@ -215,28 +215,64 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
    > gfpu->perm = perm & ~FPU_GUEST_PERM_LOCKED;
    > }
    >
    > -bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
    > +static struct fpstate *__fpu_alloc_init_guest_fpstate(struct fpu_guest *gfpu)
    > {
    > + bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED);
    > + unsigned int gfpstate_size, size;
    > struct fpstate *fpstate;
    > - unsigned int size;
    > + u64 xfeatures;
    > +
    > + /*
    > + * fpu_kernel_cfg.default_features includes all enabled xfeatures
    > + * except those dynamic xfeatures. Compared with user dynamic
    > + * xfeatures, the kernel dynamic ones are enabled for guest by
    > + * default, so add the kernel dynamic xfeatures back when calculate
    > + * guest fpstate size.
    > + *
    > + * If the user dynamic xfeatures are enabled, the guest fpstate will
    > + * be re-allocated to hold all guest enabled xfeatures, so omit user
    > + * dynamic xfeatures here.
    > + */
    > + xfeatures = fpu_kernel_cfg.default_features |
    > + fpu_kernel_dynamic_xfeatures;


    This is roughly what I had in mind when I was reviewing the previous patch,
    however for the sake of not hard-coding even more of the KVM policy here,
    I would let the KVM tell which dynamic kernel features it wants to enable
    as a parameter of this function, or even better *which* features it wants
    to enable.


    > +
    > + gfpstate_size = xstate_calculate_size(xfeatures, compacted);
    >
    > - size = fpu_kernel_cfg.default_size +
    > - ALIGN(offsetof(struct fpstate, regs), 64);
    > + size = gfpstate_size + ALIGN(offsetof(struct fpstate, regs), 64);
    >
    > fpstate = vzalloc(size);
    > if (!fpstate)
    > - return false;
    > + return NULL;
    > + /*
    > + * Initialize sizes and feature masks, use fpu_user_cfg.*
    > + * for user_* settings for compatibility of exiting uAPIs.
    > + */
    > + fpstate->size = gfpstate_size;
    > + fpstate->xfeatures = xfeatures;
    > + fpstate->user_size = fpu_user_cfg.default_size;
    > + fpstate->user_xfeatures = fpu_user_cfg.default_features;
    > + fpstate->xfd = 0;
    >
    > - /* Leave xfd to 0 (the reset value defined by spec) */
    > - __fpstate_reset(fpstate, 0);
    > fpstate_init_user(fpstate);
    > fpstate->is_valloc = true;
    > fpstate->is_guest = true;
    >
    > gfpu->fpstate = fpstate;
    > - gfpu->xfeatures = fpu_user_cfg.default_features;
    > + gfpu->xfeatures = xfeatures;
    > gfpu->perm = fpu_user_cfg.default_features;
    >
    > + return fpstate;
    > +}


    I think that this code will break, later when permission api is called by the KVM,
    because it will overwrite the fpstate->user_size and with fpstate->size
    assuming that all kernel dynamic features are enabled/disabled (depending
    on Sean's patch).


    > +
    > +bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
    > +{
    > + struct fpstate *fpstate;
    > +
    > + fpstate = __fpu_alloc_init_guest_fpstate(gfpu);
    > +
    > + if (!fpstate)
    > + return false;
    > +

    What is the point of the __fpu_alloc_init_guest_fpstate / fpu_alloc_guest_fpstate split,
    since there is only one caller?


    Best regards,
    Maxim Levitsky

    > /*
    > * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
    > * to userspace, even when XSAVE is unsupported, so that restoring FPU
    > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
    > index c5d903b4df4d..87149aba6f11 100644
    > --- a/arch/x86/kernel/fpu/xstate.c
    > +++ b/arch/x86/kernel/fpu/xstate.c
    > @@ -561,7 +561,7 @@ static bool __init check_xstate_against_struct(int nr)
    > return true;
    > }
    >
    > -static unsigned int xstate_calculate_size(u64 xfeatures, bool compacted)
    > +unsigned int xstate_calculate_size(u64 xfeatures, bool compacted)
    > {
    > unsigned int topmost = fls64(xfeatures) - 1;
    > unsigned int offset = xstate_offsets[topmost];
    > diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
    > index a4ecb04d8d64..9c6e3ca05c5c 100644
    > --- a/arch/x86/kernel/fpu/xstate.h
    > +++ b/arch/x86/kernel/fpu/xstate.h
    > @@ -10,6 +10,8 @@
    > DECLARE_PER_CPU(u64, xfd_state);
    > #endif
    >
    > +extern u64 fpu_kernel_dynamic_xfeatures;
    > +
    > static inline void xstate_init_xcomp_bv(struct xregs_state *xsave, u64 mask)
    > {
    > /*







    \
     
     \ /
      Last update: 2023-10-31 18:47    [W:7.445 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site