From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [PATCH 3/6] KVM: MMU: Add EPT support Date: Fri, 18 Apr 2008 10:54:04 -0500 Message-ID: <4808C41C.3020405@codemonkey.ws> References: <200804181729.21164.sheng.yang@intel.com> <4808A266.2000701@codemonkey.ws> <200804182316.37306.sheng.yang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel@lists.sourceforge.net To: "Yang, Sheng" Return-path: In-Reply-To: <200804182316.37306.sheng.yang@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces@lists.sourceforge.net Errors-To: kvm-devel-bounces@lists.sourceforge.net List-Id: kvm.vger.kernel.org Yang, Sheng wrote: > On Friday 18 April 2008 21:30:14 Anthony Liguori wrote: > >> Yang, Sheng wrote: >> >>> @@ -1048,17 +1071,18 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, >>> u64 *shadow_pte, >>> * whether the guest actually used the pte (in order to detect >>> * demand paging). >>> */ >>> - spte = PT_PRESENT_MASK | PT_DIRTY_MASK; >>> + spte = shadow_base_present_pte | shadow_dirty_mask; >>> if (!speculative) >>> pte_access |= PT_ACCESSED_MASK; >>> if (!dirty) >>> pte_access &= ~ACC_WRITE_MASK; >>> - if (!(pte_access & ACC_EXEC_MASK)) >>> - spte |= PT64_NX_MASK; >>> - >>> - spte |= PT_PRESENT_MASK; >>> + if (pte_access & ACC_EXEC_MASK) { >>> + if (shadow_x_mask) >>> + spte |= shadow_x_mask; >>> + } else if (shadow_nx_mask) >>> + spte |= shadow_nx_mask; >>> >> This looks like it may be a bug. The old behavior sets NX if >> (pte_access & ACC_EXEC_MASK). The new behavior unconditionally sets NX >> and never sets PRESENT. Also, the if (shadow_x_mas k) checks are >> unnecessary. spte |= 0 is a nop. >> > > Thanks for the comment! I realized two judgments of shadow_nx/x_mask is > unnecessary... In fact, the correct behavior is either set shadow_x_mask or > shadow_nx_mask, may be there is a better approach for this. The logic assured > by program itself is always safer. But I will remove the redundant code at > first. > > But I don't think it's a bug. The old behavior set NX if (!(pte_access & > ACC_EXEC_MASK)), the same as the new one. The new behavior sets NX regardless of whether (pte_access & ACC_EXEC_MASK). Is the desired change to unconditionally set NX? > And I also curious about the > PRESENT bit. You see, the PRESENT bit was set at the beginning of the code, > and I really don't know why the duplicate one exists there... > Looking at the code, you appear to be right. In the future, I think you should separate any cleanups (like removing the redundant setting of PRESENT) into a separate patch and stick to just programmatic changes of PT_USER_MASK => shadow_user_mask, etc. in this patch. That makes it a lot easier to review correctness. Regards, Anthony Liguori >>> if (pte_access & ACC_USER_MASK) >>> - spte |= PT_USER_MASK; >>> + spte |= shadow_user_mask; >>> if (largepage) >>> spte |= PT_PAGE_SIZE_MASK; >>> > > ------------------------------------------------------------------------- This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2. http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone