From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Han, Huaitong" <huaitong.han@intel.com>
Subject: Re: [V3 PATCH 7/9] x86/hvm: pkeys,
 add pkeys support for guest_walk_tables
Date: Tue, 15 Dec 2015 08:14:13 +0000
Message-ID: <1450167259.10563.9.camel@intel.com>
References: <1449479780-19146-1-git-send-email-huaitong.han@intel.com>
	<1449479780-19146-8-git-send-email-huaitong.han@intel.com>
	<5669C23F.6080203@citrix.com>
	<566AA4D702000078000BE866@prv-mh.provo.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <566AA4D702000078000BE866@prv-mh.provo.novell.com>
Content-Language: en-US
Content-ID: <9DF73884CF487041982860F24D39E5E0@intel.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: "JBeulich@suse.com" <JBeulich@suse.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>, "wei.liu2@citrix.com" <wei.liu2@citrix.com>, "ian.campbell@citrix.com" <ian.campbell@citrix.com>, "stefano.stabellini@eu.citrix.com" <stefano.stabellini@eu.citrix.com>, "george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>, "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>, "ian.jackson@eu.citrix.com" <ian.jackson@eu.citrix.com>, "george.dunlap@citrix.com" <george.dunlap@citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, "Nakajima,
	Jun" <jun.nakajima@intel.com>, "keir@xen.org" <keir@xen.org>
List-Id: xen-devel@lists.xenproject.org

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. 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
>