On 09/03/2016 06:03, Xiao Guangrong wrote:
>> WARN_ON(pfec & PFERR_PK_MASK);
>> pkru_mask = pte_access & ACC_USER_MASK ? mmu->pkru_mask : 0;
>> pkru_bits &= pkru_mask >> (pfec & ~1);
>
> ^ it should be pfec >> 1? As we
> never take PFEC.P into account?
It should be "(pfec >> 1) * 2" (don't take PFEC.P into account, but
reserve two bits per entry), which is the same as pfec & ~1.
> Actually pkru_mask can be u16 since only three bits that are PFEC.W, PFEC.U
> and PFEC.F need to be taken into account and every offset occupies 2 bits.
Right, but PFEC.F is bit 4. There is RSVD in the middle. So it needs
to be u32.
> I am considering we can use 'pte_access & ACC_USER_MASK' replacing PFEC.RSVD
> in your algorithm (then pkru_mask is 32 bit), permission_fault() is like
> this:
>
> WARN_ON(pfec & PFERR_PK_MASK);
> offset = pfec & (PFERR_WRITE_MASK | PFERR_USER_MASK);
You're forgetting PFERR_FETCH_MASK.
> offset |= !!(pte_access & ACC_USER_MASK) << PFERR_RSVD_BIT;
Better:
offset |= (pte_access & ACC_USER_MASK) <<
(PFERR_RSVD_BIT - ACC_USER_BIT);
> offset >>= 1;
As discussed above, bit 0 is necessary.
> pkru_bits &= pkru_mask >> offset;
> pfec |= -pkru_bits & PFERR_PK_MASK;
So, putting it together:
if (mmu->pkru_mask) {
WARN_ON(pfec & PFERR_PK_MASK);
offset = pfec & ~1;
offset |= (pte_access & ACC_USER_MASK) <<
(PFERR_RSVD_BIT - ACC_USER_BIT);
pkru_bits &= mmu->pkru_mask >> offset;
pfec |= -pkru_bits & PFERR_PK_MASK;
}
With this change, I think it's better to make update_pkru_bitmask a
separate function, instead of merging it in update_permissions_bitmask.
There will be a little duplicated code, but not much.
Thanks!
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
