Re: [PATCH V4 3/7] KVM, pkeys: update memeory permission bitmask for pkeys

[Paolo Bonzini <pbonzini@redhat.com>]

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