Re: [V2 PATCH 9/9] x86/hvm: pkeys, add pkeys support for gva2gfn funcitons

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

On Wed, 2015-12-02 at 04:35 -0700, Jan Beulich wrote:
>  >>> On 27.11.15 at 10:52, <huaitong.han@intel.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4304,7 +4304,8 @@ static enum hvm_copy_result
> > __hvm_clear(paddr_t addr, int size)
> >      p2m_type_t p2mt;
> >      char *p;
> >      int count, todo = size;
> > -    uint32_t pfec = PFEC_page_present | PFEC_write_access;
> > +    uint32_t pfec = PFEC_page_present | PFEC_write_access |
> > +        hvm_pku_enabled(curr) ? PFEC_prot_key : 0;
> >  
> >      /*
> >       * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant
> > -table ops
> > @@ -4405,7 +4406,8 @@ enum hvm_copy_result hvm_copy_to_guest_virt(
> >  {
> >      return __hvm_copy(buf, vaddr, size,
> >                        HVMCOPY_to_guest | HVMCOPY_fault |
> > HVMCOPY_virt,
> > -                      PFEC_page_present | PFEC_write_access |
> > pfec);
> > +                      PFEC_page_present | PFEC_write_access | pfec
> > |
> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
> > 0);
> >  }
> >  
> >  enum hvm_copy_result hvm_copy_from_guest_virt(
> > @@ -4413,7 +4415,8 @@ enum hvm_copy_result
> > hvm_copy_from_guest_virt(
> >  {
> >      return __hvm_copy(buf, vaddr, size,
> >                        HVMCOPY_from_guest | HVMCOPY_fault |
> > HVMCOPY_virt,
> > -                      PFEC_page_present | pfec);
> > +                      PFEC_page_present | pfec |
> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
> > 0);
> >  }
> >  
> >  enum hvm_copy_result hvm_fetch_from_guest_virt(
> > @@ -4431,7 +4434,8 @@ enum hvm_copy_result
> > hvm_copy_to_guest_virt_nofault(
> >  {
> >      return __hvm_copy(buf, vaddr, size,
> >                        HVMCOPY_to_guest | HVMCOPY_no_fault |
> > HVMCOPY_virt,
> > -                      PFEC_page_present | PFEC_write_access |
> > pfec);
> > +                      PFEC_page_present | PFEC_write_access | pfec
> > |
> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
> > 0);
> >  }
> >  
> >  enum hvm_copy_result hvm_copy_from_guest_virt_nofault(
> > @@ -4439,7 +4443,8 @@ enum hvm_copy_result
> > hvm_copy_from_guest_virt_nofault(
> >  {
> >      return __hvm_copy(buf, vaddr, size,
> >                        HVMCOPY_from_guest | HVMCOPY_no_fault |
> > HVMCOPY_virt,
> > -                      PFEC_page_present | pfec);
> > +                      PFEC_page_present | pfec |
> > +                      hvm_pku_enabled(current) ? PFEC_prot_key :
> > 0);
> >  }
> >  
> >  enum hvm_copy_result hvm_fetch_from_guest_virt_nofault(
> 
> Was this patch tested at all? The lack of parentheses in all the
> changes you make result - afaict - in PFEC_prot_key to be
> unconditionally passed to __hvm_copy(), which can't be right.
Yes, the patch can work, I understand, if the pfec parameter of __hvm_c
opy is zero, it means that memory permission check is not to be
required, when pfec is not 0, PKRU has access disable and write
disable, so, PFEC_prot_key is unconditionally passed to the functions.

Thanks
Huaitong.