Message ID | 20200925212302.3979661-2-bgardon@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Introduce the TDP MMU | expand |
On Fri, Sep 25, 2020 at 02:22:41PM -0700, Ben Gardon wrote: > +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 = sptep_to_sp(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; This is an impossible condition. Well, maybe it's theoretically possible if page track is active, with EPT exec-only support (shadow_present_mask is zero), and pfn==0. But in that case, returning early is wrong. Rather than return the spte, what about returning 'ret', passing 'new_spte' as a u64 *, and dropping the bail early path? That would also eliminate the minor wart of make_spte() relying on the caller to initialize 'ret'. > + > + 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; > -- > 2.28.0.709.gb0816b6eb0-goog >
On Tue, Sep 29, 2020 at 9:55 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Fri, Sep 25, 2020 at 02:22:41PM -0700, Ben Gardon wrote: > > +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 = sptep_to_sp(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; > > This is an impossible condition. Well, maybe it's theoretically possible > if page track is active, with EPT exec-only support (shadow_present_mask is > zero), and pfn==0. But in that case, returning early is wrong. > > Rather than return the spte, what about returning 'ret', passing 'new_spte' > as a u64 *, and dropping the bail early path? That would also eliminate > the minor wart of make_spte() relying on the caller to initialize 'ret'. I agree that would make this much cleaner. > > > + > > + 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; > > -- > > 2.28.0.709.gb0816b6eb0-goog > >
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 71aa3da2a0b7b..81240b558d67f 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2971,20 +2971,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 = sptep_to_sp(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; @@ -3037,27 +3031,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 = sptep_to_sp(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;