From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yang, Sheng" Subject: Re: [PATCH 3/6] KVM: MMU: Add EPT support Date: Sun, 20 Apr 2008 21:51:07 +0800 Message-ID: <200804202151.07501.sheng.yang@intel.com> References: <200804181729.21164.sheng.yang@intel.com> <200804182316.37306.sheng.yang@intel.com> <4808C41C.3020405@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: kvm-devel@lists.sourceforge.net To: Anthony Liguori Return-path: In-Reply-To: <4808C41C.3020405@codemonkey.ws> Content-Disposition: inline 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 On Friday 18 April 2008 23:54:04 Anthony Liguori wrote: > 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 *vcp= u, > >>> u64 *shadow_pte, > >>> * whether the guest actually used the pte (in order to detect > >>> * demand paging). > >>> */ > >>> - spte =3D PT_PRESENT_MASK | PT_DIRTY_MASK; > >>> + spte =3D shadow_base_present_pte | shadow_dirty_mask; > >>> if (!speculative) > >>> pte_access |=3D PT_ACCESSED_MASK; > >>> if (!dirty) > >>> pte_access &=3D ~ACC_WRITE_MASK; > >>> - if (!(pte_access & ACC_EXEC_MASK)) > >>> - spte |=3D PT64_NX_MASK; > >>> - > >>> - spte |=3D PT_PRESENT_MASK; > >>> + if (pte_access & ACC_EXEC_MASK) { > >>> + if (shadow_x_mask) > >>> + spte |=3D shadow_x_mask; > >>> + } else if (shadow_nx_mask) > >>> + spte |=3D 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 |=3D 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? Oh, I may see the point... shadow_x_mask !=3D shadow_nx_mask. the old behavior was: if (!(pte_access & ACC_EXEC_MASK)) spte |=3D PT64_NX_MASK; the new behavior is: if (pte_access & ACC_EXEC_MASK) { spte |=3D shadow_x_mask; } else spte |=3D shadow_nx_mask; For current behavior, kvm_arch_init() got: =A0=A0=A0=A0=A0=A0=A0kvm_mmu_set_mask_ptes(PT_USER_MASK, PT_ACCESSED_MASK, =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0PT_DIR= TY_MASK, PT64_NX_MASK, 0); which means shadow_nx_mask =3D PT64_NX_MASK, and shadow_x_mask =3D 0 (NX me= ans not = executable, and X means executable). = In patch 5/6, EPT got: =A0=A0=A0=A0=A0=A0=A0kvm_mmu_set_mask_ptes(0ull, VMX_EPT_FAKE_ACCESSED_MASK, =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0VMX_EP= T_FAKE_DIRTY_MASK, 0ull, =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0VMX_EP= T_EXECUTABLE_MASK); which means, shadow_nx_mask =3D 0, and shadow_x_mask =3D VMX_EPT_EXECUTABLE= _MASK So, when shadow enabled, and (!(pte_access & ACC_EXEC_MASK)), then spte |= =3D = shadow_nx_mask =3D PT64_NX_MASK (no change would happen when the condition = is = not satisfied). = When EPT enabled, and (pte_access & ACC_EXEC_MASK), then spte |=3D shadow_x= _mask = =3D VMX_EPT_EXECUTABLE_MASK (no change would happen when condition is not = satisfied). They are two different bit and mutual exclusive ones. Maybe there are some = better way to get their meaning more clearly... > > > 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 =3D> shadow_user_mask, etc. in this patch. That makes it a > lot easier to review correctness. Thanks for the advice, it's important to separate the cleanups. I will get = it = done more properly next time. = -- = Thanks Yang, Sheng > > Regards, > > Anthony Liguori > > >>> if (pte_access & ACC_USER_MASK) > >>> - spte |=3D PT_USER_MASK; > >>> + spte |=3D shadow_user_mask; > >>> if (largepage) > >>> spte |=3D 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/java= one