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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.12.15 at 09:12, <huaitong.han@intel.com> wrote:
> On Mon, 2015-12-21 at 08:32 -0700, Jan Beulich wrote:
>> > > > On 21.12.15 at 08:21, <huaitong.han@intel.com> 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.
> "

Okay, understood. But then you should follow _that_ model (using
== instead of >=) instead of inventing a 3rd variant. Similar things
should be done in similar ways so that they can be easily recognized
being similar.

Otoh the function isn't being called from code other than
GUEST_PAGING_LEVELS == 4, so at present it could be static,
which would take care of the latent linking issue Andrew referred to.

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

Perhaps just pkey_fault() then, since it's clear there's no other fault
possible here besides a 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.

Okay - I thought I had seen something like this mentioned in
another sub-thread.

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

I didn't suggest to drop guest_l2e_get_pkey(). I'd like to see the
#ifdef-s go away if at all possible, to help code readability.

Jan