Re: [V3 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

["Han, Huaitong" <huaitong.han@intel.com>]

On Tue, 2015-12-15 at 02:02 -0700, Jan Beulich wrote:
> > > > On 15.12.15 at 09:14, <huaitong.han@intel.com> wrote:
> > On Fri, 2015-12-11 at 02:26 -0700, Jan Beulich wrote:
> > > > > > On 10.12.15 at 19:19, <george.dunlap@citrix.com> wrote:
> > > > On 07/12/15 09:16, Huaitong Han wrote:
> > > > > +    if ( likely(!pte_pkeys) )
> > > > > +        return 0;
> > > > > +
> > > > > +    /* Update vcpu xsave area */
> > > > > +    fpu_xsave(vcpu);
> > > > 
> > > > Is there a reason you're calling fpu_xsave() directly here,
> > > > rather
> > > > than
> > > > just calling vcpu_save_fpu()?  That saves you actually doing
> > > > the
> > > > xsave
> > > > if the fpu hasn't been modified since the last time you read
> > > > it.
> > > 
> > > I've already said on an earlier version that wholesale saving of
> > > the
> > > entire XSAVE state is wrong here. It should just be the single
> > > piece
> > > that we're actually interested in, and it quite likely shouldn't
> > > go
> > > into
> > > struct vcpu (but e.g. into a local buffer).
> > 
> > The comments on V2 version said using vcpu_save_fpu is wrong
> > because of
> >  v->fpu_dirtied, but why wholesale saving of the entire XSAVE state
> > is
> > wrong here? I understand xsave maybe cost a little more.
> 
> "A little" is quite a bit of an understatement.
> 
> > But if we just
> > save the single piece, many functions are not reused because
> > xstate_comp_offsets is pointless, and we need add a new function as
> > follow that looks not good:
> 
> Well, I wouldn't want you to introduce a brand new function, but
> instead just factor out the necessary piece from xsave() (making
> the new one take a struct xsave_struct * instead of a struct vcpu *,
> and calling it from what is now xsave()).
So the function looks like this:
unsigned int get_xsave_pkru(struct vcpu *v)
{
    void *offset;
    struct xsave_struct *xsave_area;
    uint64_t mask = XSTATE_PKRU;
    unsigned int index = fls64(mask) - 1;
    unsigned int pkru = 0;

    if ( !cpu_has_xsave )
        return 0;
    
    BUG_ON(xsave_cntxt_size < XSTATE_AREA_MIN_SIZE);
    xsave_area = _xzalloc(xsave_cntxt_size, 64);
    if ( xsave_area == NULL )
        return 0;

    xsave(xsave_area, mask);
    offset = (void *)xsave_area + (xsave_area_compressed(xsave) ? 
            XSTATE_AREA_MIN_SIZE : xstate_offsets[index] );
    memcpy(&pkru, offset, sizeof(pkru));

    xfree(xsave_area);

    return pkru;
}
> 
> Jan
>