On 22/12/15 13:02, Jan Beulich wrote:
>>>> On 22.12.15 at 11:30, <huaitong....@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
