| Message ID | 20200203230911.39755-3-bgardon@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] kvm: mmu: Replace unsigned with unsigned int for PTE access | expand |
Ben Gardon <bgardon@google.com> writes: > Separate the functions for generating leaf page table entries from the > function that inserts them into the paging structure. This refactoring > will facilitate changes to the MMU sychronization model to use atomic > compare / exchanges (which are not guaranteed to succeed) instead of a > monolithic MMU lock. > > No functional change expected. > > Tested by running kvm-unit-tests on an Intel Haswell machine. This > commit introduced no new failures. > > This commit can be viewed in Gerrit at: > https://linux-review.googlesource.com/c/virt/kvm/kvm/+/2360 > > Signed-off-by: Ben Gardon <bgardon@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > --- > arch/x86/kvm/mmu/mmu.c | 52 +++++++++++++++++++++++++++--------------- > 1 file changed, 34 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index b81010d0edae1..9239ad5265dc6 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -3000,20 +3000,14 @@ static bool kvm_is_mmio_pfn(kvm_pfn_t pfn) > #define SET_SPTE_WRITE_PROTECTED_PT BIT(0) > #define SET_SPTE_NEED_REMOTE_TLB_FLUSH BIT(1) > > -static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > - unsigned int pte_access, int level, > - gfn_t gfn, kvm_pfn_t pfn, bool speculative, > - bool can_unsync, bool host_writable) > +static u64 make_spte(struct kvm_vcpu *vcpu, unsigned int pte_access, int level, > + gfn_t gfn, kvm_pfn_t pfn, u64 old_spte, bool speculative, > + bool can_unsync, bool host_writable, bool ad_disabled, > + int *ret) With such a long parameter list we may think about passing a pointer to a structure instead (common for make_spte()/set_spte()) > { > u64 spte = 0; > - int ret = 0; > - struct kvm_mmu_page *sp; > - > - if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) > - return 0; > > - sp = page_header(__pa(sptep)); > - if (sp_ad_disabled(sp)) > + if (ad_disabled) > spte |= SPTE_AD_DISABLED_MASK; > else if (kvm_vcpu_ad_need_write_protect(vcpu)) > spte |= SPTE_AD_WRPROT_ONLY_MASK; > @@ -3066,27 +3060,49 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > * is responsibility of mmu_get_page / kvm_sync_page. > * Same reasoning can be applied to dirty page accounting. > */ > - if (!can_unsync && is_writable_pte(*sptep)) > - goto set_pte; > + if (!can_unsync && is_writable_pte(old_spte)) > + return spte; > > if (mmu_need_write_protect(vcpu, gfn, can_unsync)) { > pgprintk("%s: found shadow page for %llx, marking ro\n", > __func__, gfn); > - ret |= SET_SPTE_WRITE_PROTECTED_PT; > + *ret |= SET_SPTE_WRITE_PROTECTED_PT; > pte_access &= ~ACC_WRITE_MASK; > spte &= ~(PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE); > } > } > > - if (pte_access & ACC_WRITE_MASK) { > - kvm_vcpu_mark_page_dirty(vcpu, gfn); > + if (pte_access & ACC_WRITE_MASK) > spte |= spte_shadow_dirty_mask(spte); > - } > > if (speculative) > spte = mark_spte_for_access_track(spte); > > -set_pte: > + return spte; > +} > + > +static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > + unsigned int pte_access, int level, > + gfn_t gfn, kvm_pfn_t pfn, bool speculative, > + bool can_unsync, bool host_writable) > +{ > + u64 spte = 0; > + struct kvm_mmu_page *sp; > + int ret = 0; > + > + if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) > + return 0; > + > + sp = page_header(__pa(sptep)); > + > + spte = make_spte(vcpu, pte_access, level, gfn, pfn, *sptep, speculative, > + can_unsync, host_writable, sp_ad_disabled(sp), &ret); I'm probably missing something, but in make_spte() I see just one place which writes to '*ret' so at the end, this is either SET_SPTE_WRITE_PROTECTED_PT or 0 (which we got only because we initialize it to 0 in set_spte()). Unless this is preparation to some other change, I don't see much value in the complication. Can we actually reverse the logic, pass 'spte' by reference and return 'ret'? > + if (!spte) > + return 0; > + > + if (spte & PT_WRITABLE_MASK) > + kvm_vcpu_mark_page_dirty(vcpu, gfn); > + > if (mmu_spte_update(sptep, spte)) > ret |= SET_SPTE_NEED_REMOTE_TLB_FLUSH; > return ret;
On 05/02/20 14:52, Vitaly Kuznetsov wrote: >> + spte = make_spte(vcpu, pte_access, level, gfn, pfn, *sptep, speculative, >> + can_unsync, host_writable, sp_ad_disabled(sp), &ret); > I'm probably missing something, but in make_spte() I see just one place > which writes to '*ret' so at the end, this is either > SET_SPTE_WRITE_PROTECTED_PT or 0 (which we got only because we > initialize it to 0 in set_spte()). Unless this is preparation to some > other change, I don't see much value in the complication. > > Can we actually reverse the logic, pass 'spte' by reference and return > 'ret'? > It gives a similar calling convention between make_spte and make_mmio_spte. It's not the most beautiful thing but I think I prefer it. But the overwhelming function parameters are quite ugly, especially old_spte. I don't think it's an improvement, let's consider it together with the rest of your changes instead. Paolo
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index b81010d0edae1..9239ad5265dc6 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3000,20 +3000,14 @@ static bool kvm_is_mmio_pfn(kvm_pfn_t pfn) #define SET_SPTE_WRITE_PROTECTED_PT BIT(0) #define SET_SPTE_NEED_REMOTE_TLB_FLUSH BIT(1) -static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, - unsigned int pte_access, int level, - gfn_t gfn, kvm_pfn_t pfn, bool speculative, - bool can_unsync, bool host_writable) +static u64 make_spte(struct kvm_vcpu *vcpu, unsigned int pte_access, int level, + gfn_t gfn, kvm_pfn_t pfn, u64 old_spte, bool speculative, + bool can_unsync, bool host_writable, bool ad_disabled, + int *ret) { u64 spte = 0; - int ret = 0; - struct kvm_mmu_page *sp; - - if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) - return 0; - sp = page_header(__pa(sptep)); - if (sp_ad_disabled(sp)) + if (ad_disabled) spte |= SPTE_AD_DISABLED_MASK; else if (kvm_vcpu_ad_need_write_protect(vcpu)) spte |= SPTE_AD_WRPROT_ONLY_MASK; @@ -3066,27 +3060,49 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, * is responsibility of mmu_get_page / kvm_sync_page. * Same reasoning can be applied to dirty page accounting. */ - if (!can_unsync && is_writable_pte(*sptep)) - goto set_pte; + if (!can_unsync && is_writable_pte(old_spte)) + return spte; if (mmu_need_write_protect(vcpu, gfn, can_unsync)) { pgprintk("%s: found shadow page for %llx, marking ro\n", __func__, gfn); - ret |= SET_SPTE_WRITE_PROTECTED_PT; + *ret |= SET_SPTE_WRITE_PROTECTED_PT; pte_access &= ~ACC_WRITE_MASK; spte &= ~(PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE); } } - if (pte_access & ACC_WRITE_MASK) { - kvm_vcpu_mark_page_dirty(vcpu, gfn); + if (pte_access & ACC_WRITE_MASK) spte |= spte_shadow_dirty_mask(spte); - } if (speculative) spte = mark_spte_for_access_track(spte); -set_pte: + return spte; +} + +static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, + unsigned int pte_access, int level, + gfn_t gfn, kvm_pfn_t pfn, bool speculative, + bool can_unsync, bool host_writable) +{ + u64 spte = 0; + struct kvm_mmu_page *sp; + int ret = 0; + + if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) + return 0; + + sp = page_header(__pa(sptep)); + + spte = make_spte(vcpu, pte_access, level, gfn, pfn, *sptep, speculative, + can_unsync, host_writable, sp_ad_disabled(sp), &ret); + if (!spte) + return 0; + + if (spte & PT_WRITABLE_MASK) + kvm_vcpu_mark_page_dirty(vcpu, gfn); + if (mmu_spte_update(sptep, spte)) ret |= SET_SPTE_NEED_REMOTE_TLB_FLUSH; return ret;