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

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

On Wed, 2015-12-16 at 02:12 -0700, Jan Beulich wrote:
> > > > On 16.12.15 at 10:03, <huaitong.han@intel.com> wrote:
> > On Wed, 2015-12-16 at 01:32 -0700, Jan Beulich wrote:
> > > > > > On 16.12.15 at 09:16, <huaitong.han@intel.com> wrote:
> > > > On Tue, 2015-12-15 at 02:02 -0700, Jan Beulich wrote:
> > > > > 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;
> > > > }
> > > 
> > > Depending on how frequently this might get called, the allocation
> > > overhead may not be tolerable. I.e. you may want to set up e.g.
> > > a per-CPU buffer up front. Or you check whether using RDPKRU
> > > (with temporarily setting CR4.PKE) is cheaper than what you
> > > do right now.
> > RDPKRU does cost less than the function, and if temporarily setting
> > CR4.PKE is accepted, I will use RDPKRU instead of the function.
> 
> The question isn't just the RDPKRU cost, but that of the two CR4
> writes.
Testing result with NOW() function:
Time of the function 10 times execution
(XEN)xsave time is 1376 ns
(XEN)read_pkru time is 28 ns

So, read_pkru function does cost less than get_xsave_pkru, and I will
use read_pkru.


Testing codes:
 static void reboot_machine(unsigned char key, struct cpu_user_regs
*regs)
 {
-    printk("'%c' pressed -> rebooting machine\n", key);
-    machine_restart(0);
+  //  printk("read pkru test\n", key);
+       s_time_t pre, last;
+       unsigned int pkru = 0;
+       unsigned int i = 0;
+       void * offset;
+       struct xsave_struct *save_area = _xzalloc(1024, 64);
+
+       pre = NOW();
+       for (;i< 10;i++){
+       xsave(current, XSTATE_PKRU, save_area);
+       offset = (void *)save_area + XSTATE_AREA_MIN_SIZE;
+       memcpy(&pkru, offset, sizeof(pkru));}
+       last = NOW();
+       printk("xsave time is %lu\n", (last - pre));
+
+       pre = NOW();
+       for (;i< 10;i++){
+       pkru  = read_pkru();
+       }
+       last = NOW();
+       printk("read_pkru time is %lu\n", (last - pre));
+  //  machine_restart(0);
 }

 static inline unsigned int read_pkru(void)
{
    unsigned int pkru;

    set_in_cr4(X86_CR4_PKE);
    asm volatile (".byte 0x0f,0x01,0xee"
        : "=a" (pkru) : "c" (0) : "dx");
    clear_in_cr4(X86_CR4_PKE);

    return pkru;
}

> 
> Jan
>