On 03/08/2016 06:01 PM, Paolo Bonzini wrote:
On 08/03/2016 10:19, Xiao Guangrong wrote:
As PKEY is not enabled on softmmu, the gva_to_gpa mostly comes from
internal
KVM, that means we should always set PFEC.PKEY for all the gva_to_gpa
request.
Wasting a bit is really unnecessary.
And it is always better to move more workload from permission_fault() to
update_permission_bitmask() as the former is much hotter than the
latter.
I agree, but I'm not sure why you say that adding a bits adds more work
to permission_fault().
A branch to check PFEC.PKEY, which is not well predictable on soft mmu. (It
should always be set in EPT as the page table walking is done by software,
however, if we only consider EPT we can assume it is always true).
Ok, I agree we shouldn't add such a branch.
It's really messy that PKERR_PK_MASK is computed at runtime. It means
we need to check U=1 in permission_fault, and we need to rethink all of
the handling of PKERR_PK_MASK in fact. I think we should extend the
algorithm from earlier in the thread to handle the AD bit too:
/* Bit 0: should PKRU.AD be considered if PFEC=0 or 1?
* Bit 1: should PKRU.WD be considered if PFEC=0 or 1?
* Bit 2: should PKRU.AD be considered if PFEC=2 or 3?
* Bit 3: should PKRU.WD be considered if PFEC=2 or 3?
* Ignore PKRU if U=0.
* etc.
*/
u32 pkru_mask;
In update_permission_bitmask, the bits of mmu->pkru_mask only depend on
the byte, not on the bit.
Bits 0, 2, ... are computed from pku, i.e. cr4_pku && long_mode && !ff.
Bits 1, 3, ... are computed from pku && (wf &&
(is_write_protection(vcpu) || uf)).
The permissions[] array can keep 16 entries, because PFERR_PK_MASK is
never passed to permission_fault (like PFERR_RSVD_MASK). The
permission_fault code is something like:
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?
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.
pfec |= -pkru_bits & PFERR_PK_MASK;
What do you think?
Very good. :)
I still believe that the above checks should be wrapped in a
if (unlikely(mmu->pkru_mask) {
...
}
It should be predicted well and removes the potential performance cost
of branchless code on !PKE OSes.
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);
offset |= !!(pte_access & ACC_USER_MASK) << PFERR_RSVD_BIT;
offset >>= 1;
pkru_bits &= pkru_mask >> offset;
pfec |= -pkru_bits & PFERR_PK_MASK;
Now, this is no branch and no much overload introduced in update_permission_bitmask().
--
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
