From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 4/5] KVM: MMU: Optimize pte permission checks Date: Thu, 13 Sep 2012 15:15:31 +0300 Message-ID: <5051CE63.7040605@redhat.com> References: <1347460194-11807-1-git-send-email-avi@redhat.com> <1347460194-11807-5-git-send-email-avi@redhat.com> <5051CD03.2080206@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Xiao Guangrong Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39290 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757982Ab2IMMQC (ORCPT ); Thu, 13 Sep 2012 08:16:02 -0400 In-Reply-To: <5051CD03.2080206@linux.vnet.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 09/13/2012 03:09 PM, Xiao Guangrong wrote: >> >> The result is short, branch-free code. >> >> Signed-off-by: Avi Kivity > >> +static void update_permission_bitmask(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) >> +{ >> + unsigned bit, byte, pfec; >> + u8 map; >> + bool fault, x, w, u, wf, uf, ff, smep; >> + >> + smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP); >> + for (byte = 0; byte < ARRAY_SIZE(mmu->permissions); ++byte) { >> + pfec = byte << 1; >> + map = 0; >> + wf = pfec & PFERR_WRITE_MASK; >> + uf = pfec & PFERR_USER_MASK; >> + ff = pfec & PFERR_FETCH_MASK; >> + for (bit = 0; bit < 8; ++bit) { >> + x = bit & ACC_EXEC_MASK; >> + w = bit & ACC_WRITE_MASK; >> + u = bit & ACC_USER_MASK; >> + >> + /* Not really needed: !nx will cause pte.nx to fault */ >> + x |= !mmu->nx; >> + /* Allow supervisor writes if !cr0.wp */ >> + w |= !is_write_protection(vcpu) && !uf; >> + /* Disallow supervisor fetches if cr4.smep */ >> + x &= !(smep && !uf); > > In the case of smep, supervisor mode can fetch the memory if pte.u == 0, > so, it should be x &= !(smep && !uf && u)? Good catch, will fix. > >> @@ -3672,20 +3672,18 @@ static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva, >> gpa_t *gpa, struct x86_exception *exception, >> bool write) >> { >> - u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0; >> + u32 access = ((kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0) >> + | (write ? PFERR_WRITE_MASK : 0); >> + u8 bit = vcpu->arch.access; >> >> - if (vcpu_match_mmio_gva(vcpu, gva) && >> - check_write_user_access(vcpu, write, access, >> - vcpu->arch.access)) { >> + if (vcpu_match_mmio_gva(vcpu, gva) >> + && ((vcpu->arch.walk_mmu->permissions[access >> 1] >> bit) & 1)) { > > !((vcpu->arch.walk_mmu->permissions[access >> 1] >> bit) & 1) ? > > It is better introducing a function to do the permission check? > Probably, I'll rethink it. -- error compiling committee.c: too many arguments to function