lkml.org 
[lkml]   [2017]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate()
    On Thu, Jan 26, 2017 at 11:22:56AM +0100, Ingo Molnar wrote:
    > Similar to:
    >
    > x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user()
    >
    > No change in functionality.

    ...

    > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
    > index 7d24fe305d4b..aed9b2cb4d3d 100644
    > --- a/arch/x86/kernel/fpu/xstate.c
    > +++ b/arch/x86/kernel/fpu/xstate.c
    > @@ -1083,7 +1083,71 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
    > }
    >
    > /*
    > - * Convert from a ptrace standard-format buffer to kernel XSAVES format
    > + * Convert from a ptrace standard-format kernel buffer to kernel XSAVES format
    > + * and copy to the target thread. This is called from xstateregs_set() and
    > + * there we check the CPU has XSAVES and a whole standard-sized buffer
    > + * exists.
    > + */
    > +int copy_kernel_to_xstate(const void *kbuf, const void __user *ubuf,
    > + struct xregs_state *xsave)

    I'm wondering if we could avoid that code duplication in a cleaner way.
    So that function is doing two things conceptually:

    * get xfeatures
    * iterate over them

    It is being called by xstateregs_set(), for example. So you could do:

    xstateregs_set:

    if (kbuf)
    copy_kernel_to_xstate()
    else
    copy_user_to_xstate()

    Now those two could assign the copying function pointers:

    copy_fn = memcpy;

    and
    copy_fn = __copy_from_user;

    or a wrapper or whatever.

    And then call a "workhorse" function:

    /* get_xfeatures */
    __copy_x_to_xstate(xstate, xfeatures, buf, copy_fn);

    The buf thing would have to be cast to (void *) to drop the __user
    annotation, though. Would that still be ok for sparse, I dunno?

    Hmmm?

    > +{
    > + unsigned int offset, size;
    > + int i;
    > + u64 xfeatures;
    > + u64 allowed_features;
    > +
    > + offset = offsetof(struct xregs_state, header);
    > + size = sizeof(xfeatures);
    > +
    > + if (kbuf) {
    > + memcpy(&xfeatures, kbuf + offset, size);
    > + } else {
    > + if (__copy_from_user(&xfeatures, ubuf + offset, size))
    > + return -EFAULT;
    > + }
    > +
    > + /*
    > + * Reject if the user sets any disabled or supervisor features:
    > + */
    > + allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
    > +
    > + if (xfeatures & ~allowed_features)
    > + return -EINVAL;
    > +
    > + for (i = 0; i < XFEATURE_MAX; i++) {
    > + u64 mask = ((u64)1 << i);
    > +
    > + if (xfeatures & mask) {

    So this mask is redundant - this could be done this way:

    if (xfeatures & BIT_ULL(i))

    > + void *dst = __raw_xsave_addr(xsave, 1 << i);

    This thing looks yucky too:

    we have a bit number, we convert it to a bit mask, in __raw_xsave_addr()
    we convert it *back* to a bit number... are we testing the bitops speed
    of the CPU or what's going on?

    AFAICT, this interface could simply be converted to accepting bit
    numbers and the callers of get_xsave_addr() could be simplified too.
    From looking at fill_xsave() and load_xsave() in kvm, they already
    generate the bit indices for CPUID - no need to generate the masks too.

    Btw, __raw_xsave_addr() can be static too.

    Btw 2,

    void *get_xsave_addr(struct xregs_state *xsave, int xstate_feature)

    ...

    WARN_ONCE(!(xfeatures_mask & xstate_feature),
    "get of unsupported state");
    ^^^^^^^^^^^^^^^^^^^^^^^^^

    that's funny english.

    --
    Regards/Gruss,
    Boris.

    Good mailing practices for 400: avoid top-posting and trim the reply.

    \
     
     \ /
      Last update: 2017-01-27 12:57    [W:3.074 / U:0.668 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site