From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4 Date: Mon, 31 Mar 2014 09:28:53 +0200 Message-ID: <53391935.3040908@redhat.com> References: <1396028184-24486-1-git-send-email-feng.wu@intel.com> <1396028184-24486-3-git-send-email-feng.wu@intel.com> <5335650E.8060800@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit To: "Wu, Feng" , "gleb@redhat.com" , "hpa@zytor.com" , "kvm@vger.kernel.org" Return-path: Received: from mail-wg0-f51.google.com ([74.125.82.51]:37668 "EHLO mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752572AbaCaH26 (ORCPT ); Mon, 31 Mar 2014 03:28:58 -0400 Received: by mail-wg0-f51.google.com with SMTP id k14so5375460wgh.34 for ; Mon, 31 Mar 2014 00:28:56 -0700 (PDT) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: Il 31/03/2014 08:16, Wu, Feng ha scritto: > -------------------------------------------------------------------------------------------------------------------- > /* > * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1. > * > * If CPL = 3, SMAP applies to all supervisor-mode data accesses > * (these are implicit supervisor accesses) regardless of the value > * of EFLAGS.AC. > * > * This computes (cpl < 3) && (rflags & X86_EFLAGS_AC), leaving > * the result in X86_EFLAGS_AC. We then insert it in place of > * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec, > * but it will be one in index if SMAP checks are being overridden. > * It is important to keep this branchless. > */ > smap = (cpl - 3) & (rflags & X86_EFLAGS_AC); > index = > (pfec >> 1) + > (smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1)); > > return (mmu->permissions[index] >> pte_access) & 1; > > The direction of PFERR_RSVD_MASK is the opposite compared to your code. > ------------------------------------------------------------------------------------------------------------------- Correct. > I am a little confused about some points of the above code: > 1. "smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);" > "smap" equals 1 when it is overridden and it is 0 when being enforced. Actually, smap equals X86_EFLAGS_AC when it is overridden. Perhaps this is the source of the confusion. Note that I'm using &, not &&. > So "index" > will be (pfec >> 1) when SMAP is enforced, but in my understanding of this case, we > should use the index with PFERR_RSVD_MASK bit being 1 in mmu-> permissions[] > to check the fault. > 2. " smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1)" > I am not quite understand this line. BTW, I cannot find the definition of "PFERR_RSVD_BIT", > Do you mean PFERR_RSVD_BIT equals 3? Yes. You can similarly add PFERR_PRESENT_BIT (equal to 0) etc. > I think the basic idea is using group 0 to check permission faults when !((cpl - 3) & (rflags & X86_EFLAGS_AC)), that is SMAP is overridden > while using group 1 to check faults when (cpl - 3) & (rflags & X86_EFLAGS_AC), that is SMAP is enforced. > > Here is the code base on your proposal in my understanding: > > ------------------------------------------------------------------------------------------------------------------- > smap = !((cpl - 3) & (rflags & X86_EFLAGS_AC)); > index = > (pfec >> 1) + (smap << (PFERR_RSVD_BIT - 1)); /*assuming PFERR_RSVD_BIT == 3*/ > > return (mmu->permissions[index] >> pte_access) & 1; > ------------------------------------------------------------------------------------------------------------------- > > Could you please have a look at it? Appreciate your help! :) It is faster if you avoid the "!" and shift right from the AC bit into position PFERR_RSVD_BIT - 1. In update_permission_bitmask you can invert the direction of the bit when you extract it from pfec. Paolo