From mboxrd@z Thu Jan 1 00:00:00 1970 From: Junaid Shahid Subject: Re: [PATCH v3 7/8] kvm: x86: mmu: Lockless access tracking for Intel CPUs without EPT A bits. Date: Fri, 16 Dec 2016 18:04:22 -0800 Message-ID: <4157789.R9cn7kUSZu@js-desktop.mtv.corp.google.com> References: <1481071577-40250-8-git-send-email-junaids@google.com> <93b5692a-0f76-a31d-46f3-b85d19298d92@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Cc: kvm@vger.kernel.org, andreslc@google.com, pfeiner@google.com, pbonzini@redhat.com To: Xiao Guangrong Return-path: Received: from mail-pg0-f51.google.com ([74.125.83.51]:34123 "EHLO mail-pg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756125AbcLQCEY (ORCPT ); Fri, 16 Dec 2016 21:04:24 -0500 Received: by mail-pg0-f51.google.com with SMTP id a1so15199432pgf.1 for ; Fri, 16 Dec 2016 18:04:24 -0800 (PST) In-Reply-To: <93b5692a-0f76-a31d-46f3-b85d19298d92@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On Friday, December 16, 2016 09:04:56 PM Xiao Guangrong wrote: > > void kvm_mmu_set_mask_ptes(u64 user_mask, u64 accessed_mask, > > -=09=09u64 dirty_mask, u64 nx_mask, u64 x_mask, u64 p_mask); > > +=09=09u64 dirty_mask, u64 nx_mask, u64 x_mask, u64 p_mask, > > +=09=09u64 acc_track_mask); >=20 > Actually, this is the mask cleared by acc-track rather that _set_ by > acc-track, maybe suppress_by_acc_track_mask is a better name. Well, the original reason behind it was that a PTE is an access-track P= TE if when masked by acc_track_mask, it yields acc_track_value. But we = can change the name if it is confusing. Though suppress_by_acc_track_ma= sk isn=E2=80=99t quite right since only the RWX bits are cleared, but t= he Special bit is set and the mask includes both of these. > > +#define VMX_EPT_MT_MASK=09=09=09=09(7ull << VMX_EPT_MT_EPTE_SHIFT)= >=20 > I saw no space using this mask, can be dropped. Ok. I=E2=80=99ll drop it. > > +/* The mask for the R/X bits in EPT PTEs */ > > +#define PT64_EPT_READABLE_MASK=09=09=090x1ull > > +#define PT64_EPT_EXECUTABLE_MASK=09=090x4ull > > + >=20 > Can we move this EPT specific stuff out of mmu.c? We need these in order to define the shadow_acc_track_saved_bits_mask a= nd since we don=E2=80=99t have vmx.h included in mmu.c so I had to defi= ne these here. Is adding an #include for vmx.h better? Alternatively, w= e can have the shadow_acc_track_saved_bits_mask passed by kvm_intel whe= n it loads, which was the case in the original version but I had change= d it to a constant based on previous feedback. > > +static inline bool is_access_track_spte(u64 spte) > > +{ > > +=09return shadow_acc_track_mask !=3D 0 && > > +=09 (spte & shadow_acc_track_mask) =3D=3D shadow_acc_track_v= alue; > > +} >=20 > spte & SPECIAL_MASK && !is_mmio(spte) is more clearer. We can change to that. But it seems less flexible as it assumes that th= ere is never going to be a 3rd type of Special PTE. > > +=09/* > > +=09 * Verify that the write-protection that we do below will be fi= xable > > +=09 * via the fast page fault path. Currently, that is always the = case, at > > +=09 * least when using EPT (which is when access tracking would be= used). > > +=09 */ > > +=09WARN_ONCE((spte & PT_WRITABLE_MASK) && > > +=09=09 !spte_can_locklessly_be_made_writable(spte), > > +=09=09 "kvm: Writable SPTE is not locklessly dirty-trackable\n");= >=20 > This code is right but i can not understand the comment here... :( Basically, I was just trying to say that since making the PTE an acc-tr= ack PTE will remove the write access as well, so we better have the abi= lity to restore the write access later in fast_page_fault. I=E2=80=99ll= try to make the comment more clear. > > > > -=09=09/* > > -=09=09 * Currently, to simplify the code, only the spte > > -=09=09 * write-protected by dirty-log can be fast fixed. > > -=09=09 */ > > -=09=09if (!spte_can_locklessly_be_made_writable(spte)) > > +=09=09remove_acc_track =3D is_access_track_spte(spte); > > + >=20 > Why not check cached R/X permission can satisfy R/X access before got= o atomic path? Yes, I guess we can do that since if the restored PTE doesn=E2=80=99t s= atisfy the access we are just going to get another fault anyway. > > +void vmx_enable_tdp(void) > > +{ > > +=09kvm_mmu_set_mask_ptes(VMX_EPT_READABLE_MASK, > > +=09=09enable_ept_ad_bits ? VMX_EPT_ACCESS_BIT : 0ull, > > +=09=09enable_ept_ad_bits ? VMX_EPT_DIRTY_BIT : 0ull, > > +=09=090ull, VMX_EPT_EXECUTABLE_MASK, > > +=09=09cpu_has_vmx_ept_execute_only() ? 0ull : VMX_EPT_READABLE_MAS= K, > > +=09=09enable_ept_ad_bits ? 0ull : SPTE_SPECIAL_MASK | VMX_EPT_RWX_= MASK); >=20 > I think commonly set SPTE_SPECIAL_MASK (move set SPTE_SPECIAL_MASK to= mmu.c) for > mmio-mask and acc-track-mask can make the code more clearer... Ok. So you mean that vmx.c should just pass VMX_EPT_RWX_MASK here and V= MX_EPT_MISCONFIG_WX_VALUE for the mmio mask and then mmu.c should add i= n SPTE_SPECIAL_MASK before storing these values in shadow_acc_track_mas= k and shadow_mmio_mask? Thanks, Junaid