> -----Original Message----- > From: Paolo Bonzini [mailto:paolo.bonzini@xxxxxxxxx] On Behalf Of Paolo > Bonzini > Sent: Monday, March 31, 2014 3:29 PM > To: Wu, Feng; gleb@xxxxxxxxxx; hpa@xxxxxxxxx; kvm@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4 > > 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 &&. > Thanks for your explanation, you are right I didn't notice *&* just now, I was thinking it is a *&&*, sorry for making the confusion. > > 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. So in that case, we should set "smapf" in update_permission_bitmask() this way, right? smapf = !(pfec & PFERR_RSVD_MASK); > > Paolo -- 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
