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

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

On Fri, 2015-12-11 at 02:26 -0700, Jan Beulich wrote:
> > > > On 10.12.15 at 19:19, <george.dunlap@xxxxxxxxxx> 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. 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:

(just a example):
unsigned int get_xsave_pkru(void)
{
    struct xsave_struct *xstate;
    void *xsave_addr;
    unsigned int pkru = 0;

    xstate = _xzalloc(xsave_cntxt_size, 64);
    if ( xstate == NULL )
        return -ENOMEM;

    asm volatile ( ".byte 0x0f,0xae,0x27" /* XSAVE */
                        : "=m" (*xstate)
                        : "a" (XSTATE_PKRU), "d" (0), "D" (xstate) );

    xsave_addr = get_xsave_addr(xstate, fls64(XSTATE_PKRU)-1);
    if ( xsave_addr )
        memcpy(&pkru, xsave_addr, sizeof(pkru));

    xfree(xstate);

    return pkru;
}

Thanks a lot.
Huaitong

> 
> Jan
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel