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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.12.15 at 09:14, <huaitong.han@xxxxxxxxx> wrote:
> 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.

"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()).

Jan


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