Re: [Xen-devel] [PATCH V4 4/6] x86/hvm: pkeys, add pkeys support for guest_walk_tables

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

On Mon, 2015-12-21 at 08:32 -0700, Jan Beulich wrote:
> > > > On 21.12.15 at 08:21, <huaitong.han@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/mm/guest_walk.c
> > +++ b/xen/arch/x86/mm/guest_walk.c
> > @@ -90,6 +90,55 @@ static uint32_t set_ad_bits(void *guest_p, void
> > *walk_p, int set_dirty)
> >      return 0;
> >  }
> >  
> > +#if GUEST_PAGING_LEVELS >= CONFIG_PAGING_LEVELS
> 
> GUEST_PAGING_LEVELS >= 4 (just like further down)
The code is modified according Andrew's comments:
"
  This is a latent linking bug for the future 
  when 5 levels comes along.

  It will probably be best to use the same trick
  as gw_page_flags to compile it once but use it
  multiple times.
"

> 
> > +bool_t leaf_pte_pkeys_check(struct vcpu *vcpu,
> > +        uint32_t pfec, uint32_t pte_pkey)
> > +{
> > +    unsigned int pkru = 0;
> > +    bool_t pkru_ad, pkru_wd;
> > +
> 
> Stray blank line.
> 
> > +    bool_t pf = !!(pfec & PFEC_page_present);
> > +    bool_t uf = !!(pfec & PFEC_user_mode);
> > +    bool_t wf = !!(pfec & PFEC_write_access);
> > +    bool_t ff = !!(pfec & PFEC_insn_fetch);
> > +    bool_t rsvdf = !!(pfec & PFEC_reserved_bit);
> > +
> > +    /* When page is present,  PFEC_prot_key is always checked */
> > +    if ( !pf || is_pv_vcpu(vcpu) )
> > +        return 0;
> 
> I think for a function called "check" together with how its callers
> use
> it the return value meaning is inverted here. 
I will change the function name to "pkey_page_fault".

> Also the comment seems
> inverted wrt the actual check (and is missing a full stop). And
> doesn't
> key 0 have static meaning, in which case you could bail early (and
> namely avoid the expensive RDPKRU further down)?
Key 0 have no static meaning, the default key maybe different in
different OS.
> 
> > +    /*
> > +     * PKU:  additional mechanism by which the paging controls
> > +     * access to user-mode addresses based on the value in the
> > +     * PKRU register. A fault is considered as a PKU violation if
> > all
> > +     * of the following conditions are ture:
> > +     * 1.CR4_PKE=1.
> > +     * 2.EFER_LMA=1.
> > +     * 3.page is present with no reserved bit violations.
> > +     * 4.the access is not an instruction fetch.
> > +     * 5.the access is to a user page.
> > +     * 6.PKRU.AD=1
> > +     *       or The access is a data write and PKRU.WD=1
> > +     *            and either CR0.WP=1 or it is a user access.
> > +     */
> > +    if ( !hvm_pku_enabled(vcpu) ||
> > +            !hvm_long_mode_enabled(vcpu) || rsvdf || ff )
> 
> Where's the "user page" check? Also - indentation.
it should be:
    if ( !hvm_pku_enabled(vcpu) || !hvm_long_mode_enabled(vcpu) ||
            rsvdf || ff || !(pte_flags & _PAGE_USER) )
> 
> > +        return 0;
> > +
> > +    pkru = read_pkru();
> > +    if ( unlikely(pkru) )
> > +    {
> > +        pkru_ad = read_pkru_ad(pkru, pte_pkey);
> > +        pkru_wd = read_pkru_wd(pkru, pte_pkey);
> > +        /* Condition 6 */
> > +        if ( pkru_ad || (pkru_wd && wf && (hvm_wp_enabled(vcpu) ||
> > uf)))
> 
> Ah, uf is being checked here. But according to the comment it could
> (and should, again to avoid the RDPKRU) move up.
> 
> > @@ -270,6 +324,12 @@ guest_walk_tables(struct vcpu *v, struct
> > p2m_domain *p2m,
> >  
> >      pse2M = (gflags & _PAGE_PSE) && guest_supports_superpages(v); 
> >  
> > +#if GUEST_PAGING_LEVELS >= 4
> > +    pkey = guest_l2e_get_pkey(gw->l2e);
> > +    if ( pse2M && leaf_pte_pkeys_check(v, pfec, pkey) )
> > +        rc |= _PAGE_PKEY_BITS;
> > +#endif
> 
> I think the #ifdef isn't really needed here, if you moved the one
> around leaf_pte_pkeys_check() into that function, and if you
> perhaps also dropped the "pkey" local variable.
guest_l2e_get_pkey has different macro depend on GUEST_PAGING_LEVELS
too, and I think it's better to keep it.
> 
> Jan
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel