From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH V4 3/7] KVM, pkeys: update memeory permission bitmask for pkeys Date: Wed, 9 Mar 2016 09:10:23 +0100 Message-ID: <56DFDA6F.1000900@redhat.com> References: <1457177252-7577-1-git-send-email-huaitong.han@intel.com> <1457177252-7577-4-git-send-email-huaitong.han@intel.com> <56DBDF57.4030607@linux.intel.com> <56DCB9BF.4020904@redhat.com> <56DE80B8.40900@linux.intel.com> <56DE8D7D.5010302@redhat.com> <56DE991E.8070007@linux.intel.com> <56DEA2FA.70302@redhat.com> <56DFAEA5.9080604@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org To: Xiao Guangrong , Huaitong Han , gleb@kernel.org Return-path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:35208 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751891AbcCIIK2 (ORCPT ); Wed, 9 Mar 2016 03:10:28 -0500 Received: by mail-wm0-f66.google.com with SMTP id 1so8574377wmg.2 for ; Wed, 09 Mar 2016 00:10:27 -0800 (PST) In-Reply-To: <56DFAEA5.9080604@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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