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

[George Dunlap <george.dunlap@citrix.com>]

On 22/12/15 13:02, Jan Beulich wrote:
>>>> On 22.12.15 at 11:30, <huaitong.han@intel.com> wrote:
> 
> I dislike having to repeat this: Please trim your Cc lists.
> 
>> --- a/xen/arch/x86/mm/guest_walk.c
>> +++ b/xen/arch/x86/mm/guest_walk.c
>> @@ -90,6 +90,57 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p, int set_dirty)
>>      return 0;
>>  }
>>  
>> +extern bool_t pkey_fault(struct vcpu *vcpu, uint32_t pfec,
>> +        uint32_t pte_flags, uint32_t pte_pkey);
>> +#if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
>> +bool_t pkey_fault(struct vcpu *vcpu, uint32_t pfec,
>> +        uint32_t pte_flags, uint32_t pte_pkey)
>> +{
> 
> See my comments on the previous version. Please avoid sending new
> versions without having addressed all comments on the previous one
> (verbally or by code changes). Having done the suggested change
> just partially (by removing the #ifdef-s from the call sites) you now
> do the key check universally, and things remain correct just because
> of the long mode check in the middle of the function.
> 
>> +    unsigned int pkru = 0;
>> +    bool_t pkru_ad, pkru_wd;
>> +
>> +    bool_t pf = !!(pfec & PFEC_page_present);
> 
> There's still this stray blank line above (and I continue to wonder
> whether you really need all these boolean variables many of which
> get used just once).

I suspect the "stray blank line" was added for readability.

But I agree that I'd prefer not to use local boolean variables, and just
to put the flag checking inline.

> 
>> +    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 isn't present,  PKEY isn't checked. */
>> +    if ( !pf || is_pv_vcpu(vcpu) )
>> +        return 0;
>> +
>> +    /*
>> +     * 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:

*true

>> +     * 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 || !(pte_flags & _PAGE_USER) )

And I think you might as well make this one line per condition,
something like this:

    if ( is_pv_vcpu(vcpu) ||
         !hvm_pku_enabled(vcpu) ||
         !hvm_long_mode_enabled(vcpu)
         !(pfec & PFEC_page_present) ||
         (pfec & (PFEC_insn_fetch|PFEC_reserved_bit)) ||
         !(pte_flags & _PAGE_USER) )
        return 0;

 -George