Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 6 Apr 2021 23:42:06 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Keqian Zhu <zhukeqian1@huawei.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        wanghaibin.wang@huawei.com, Ben Gardon <bgardon@google.com>
Subject: Re: [RFC PATCH] KVM: x86: Support write protect huge pages lazily
Message-ID: <YGzxzsRlqouaJv6a@google.com>
References: <20200828081157.15748-1-zhukeqian1@huawei.com>
 <107696eb-755f-7807-a484-da63aad01ce4@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <107696eb-755f-7807-a484-da63aad01ce4@huawei.com>
Precedence: bulk

+Ben

On Tue, Apr 06, 2021, Keqian Zhu wrote:
> Hi Paolo,
> 
> I plan to rework this patch and do full test. What do you think about this idea
> (enable dirty logging for huge pages lazily)?

Ben, don't you also have something similar (or maybe the exact opposite?) in the
hopper?  This sounds very familiar, but I can't quite connect the dots that are
floating around my head...
 
> PS: As dirty log of TDP MMU has been supported, I should add more code.
> 
> On 2020/8/28 16:11, Keqian Zhu wrote:
> > Currently during enable dirty logging, if we're with init-all-set,
> > we just write protect huge pages and leave normal pages untouched,
> > for that we can enable dirty logging for these pages lazily.
> > 
> > It seems that enable dirty logging lazily for huge pages is feasible
> > too, which not only reduces the time of start dirty logging, also
> > greatly reduces side-effect on guest when there is high dirty rate.
> > 
> > (These codes are not tested, for RFC purpose :-) ).
> > 
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  3 +-
> >  arch/x86/kvm/mmu/mmu.c          | 65 ++++++++++++++++++++++++++-------
> >  arch/x86/kvm/vmx/vmx.c          |  3 +-
> >  arch/x86/kvm/x86.c              | 22 +++++------
> >  4 files changed, 62 insertions(+), 31 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 5303dbc5c9bc..201a068cf43d 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1296,8 +1296,7 @@ void kvm_mmu_set_mask_ptes(u64 user_mask, u64 accessed_mask,
> >  
> >  void kvm_mmu_reset_context(struct kvm_vcpu *vcpu);
> >  void kvm_mmu_slot_remove_write_access(struct kvm *kvm,
> > -				      struct kvm_memory_slot *memslot,
> > -				      int start_level);
> > +				      struct kvm_memory_slot *memslot);
> >  void kvm_mmu_zap_collapsible_sptes(struct kvm *kvm,
> >  				   const struct kvm_memory_slot *memslot);
> >  void kvm_mmu_slot_leaf_clear_dirty(struct kvm *kvm,
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 43fdb0c12a5d..4b7d577de6cd 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -1625,14 +1625,45 @@ static bool __rmap_set_dirty(struct kvm *kvm, struct kvm_rmap_head *rmap_head)
> >  }
> >  
> >  /**
> > - * kvm_mmu_write_protect_pt_masked - write protect selected PT level pages
> > + * kvm_mmu_write_protect_largepage_masked - write protect selected largepages
> >   * @kvm: kvm instance
> >   * @slot: slot to protect
> >   * @gfn_offset: start of the BITS_PER_LONG pages we care about
> >   * @mask: indicates which pages we should protect
> >   *
> > - * Used when we do not need to care about huge page mappings: e.g. during dirty
> > - * logging we do not have any such mappings.
> > + * @ret: true if all pages are write protected
> > + */
> > +static bool kvm_mmu_write_protect_largepage_masked(struct kvm *kvm,
> > +				    struct kvm_memory_slot *slot,
> > +				    gfn_t gfn_offset, unsigned long mask)
> > +{
> > +	struct kvm_rmap_head *rmap_head;
> > +	bool protected, all_protected;
> > +	gfn_t start_gfn = slot->base_gfn + gfn_offset;
> > +	int i;
> > +
> > +	all_protected = true;
> > +	while (mask) {
> > +		protected = false;
> > +		for (i = PG_LEVEL_2M; i <= KVM_MAX_HUGEPAGE_LEVEL; ++i) {
> > +			rmap_head = __gfn_to_rmap(start_gfn + __ffs(mask), i, slot);
> > +			protectd |= __rmap_write_protect(kvm, rmap_head, false);
> > +		}
> > +
> > +		all_protected &= protectd;
> > +		/* clear the first set bit */
> > +		mask &= mask - 1;
> > +	}
> > +
> > +	return all_protected;
> > +}
> > +
> > +/**
> > + * kvm_mmu_write_protect_pt_masked - write protect selected PT level pages
> > + * @kvm: kvm instance
> > + * @slot: slot to protect
> > + * @gfn_offset: start of the BITS_PER_LONG pages we care about
> > + * @mask: indicates which pages we should protect
> >   */
> >  static void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
> >  				     struct kvm_memory_slot *slot,
> > @@ -1679,18 +1710,25 @@ EXPORT_SYMBOL_GPL(kvm_mmu_clear_dirty_pt_masked);
> >  
> >  /**
> >   * kvm_arch_mmu_enable_log_dirty_pt_masked - enable dirty logging for selected
> > - * PT level pages.
> > - *
> > - * It calls kvm_mmu_write_protect_pt_masked to write protect selected pages to
> > - * enable dirty logging for them.
> > - *
> > - * Used when we do not need to care about huge page mappings: e.g. during dirty
> > - * logging we do not have any such mappings.
> > + * dirty pages.
> >   */
> >  void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
> >  				struct kvm_memory_slot *slot,
> >  				gfn_t gfn_offset, unsigned long mask)
> >  {
> > +	/*
> > +	 * If we're with initial-all-set, huge pages are NOT
> > +	 * write protected when we start dirty log, so we must
> > +	 * write protect them here.
> > +	 */
> > +	if (kvm_dirty_log_manual_protect_and_init_set(kvm)) {
> > +		if (kvm_mmu_write_protect_largepage_masked(kvm, slot,
> > +					gfn_offset, mask))
> > +			return;
> > +	}
> > +
> > +	/* Then we can handle the 4K level pages */
> > +
> >  	if (kvm_x86_ops.enable_log_dirty_pt_masked)
> >  		kvm_x86_ops.enable_log_dirty_pt_masked(kvm, slot, gfn_offset,
> >  				mask);
> > @@ -5906,14 +5944,13 @@ static bool slot_rmap_write_protect(struct kvm *kvm,
> >  }
> >  
> >  void kvm_mmu_slot_remove_write_access(struct kvm *kvm,
> > -				      struct kvm_memory_slot *memslot,
> > -				      int start_level)
> > +				      struct kvm_memory_slot *memslot)
> >  {
> >  	bool flush;
> >  
> >  	spin_lock(&kvm->mmu_lock);
> > -	flush = slot_handle_level(kvm, memslot, slot_rmap_write_protect,
> > -				start_level, KVM_MAX_HUGEPAGE_LEVEL, false);
> > +	flush = slot_handle_all_level(kvm, memslot, slot_rmap_write_protect,
> > +				      false);
> >  	spin_unlock(&kvm->mmu_lock);
> >  
> >  	/*
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 819c185adf09..ba871c52ef8b 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -7538,8 +7538,7 @@ static void vmx_sched_in(struct kvm_vcpu *vcpu, int cpu)
> >  static void vmx_slot_enable_log_dirty(struct kvm *kvm,
> >  				     struct kvm_memory_slot *slot)
> >  {
> > -	if (!kvm_dirty_log_manual_protect_and_init_set(kvm))
> > -		kvm_mmu_slot_leaf_clear_dirty(kvm, slot);
> > +	kvm_mmu_slot_leaf_clear_dirty(kvm, slot);
> >  	kvm_mmu_slot_largepage_remove_write_access(kvm, slot);
> >  }
> >  
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index d39d6cf1d473..c31c32f1424b 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10225,22 +10225,18 @@ static void kvm_mmu_slot_apply_flags(struct kvm *kvm,
> >  	 * is enabled the D-bit or the W-bit will be cleared.
> >  	 */
> >  	if (new->flags & KVM_MEM_LOG_DIRTY_PAGES) {
> > +		/*
> > +		 * If we're with initial-all-set, we don't need
> > +		 * to write protect any page because they're
> > +		 * reported as dirty already.
> > +		 */
> > +		if (kvm_dirty_log_manual_protect_and_init_set(kvm))
> > +			return;
> > +
> >  		if (kvm_x86_ops.slot_enable_log_dirty) {
> >  			kvm_x86_ops.slot_enable_log_dirty(kvm, new);
> >  		} else {
> > -			int level =
> > -				kvm_dirty_log_manual_protect_and_init_set(kvm) ?
> > -				PG_LEVEL_2M : PG_LEVEL_4K;
> > -
> > -			/*
> > -			 * If we're with initial-all-set, we don't need
> > -			 * to write protect any small page because
> > -			 * they're reported as dirty already.  However
> > -			 * we still need to write-protect huge pages
> > -			 * so that the page split can happen lazily on
> > -			 * the first write to the huge page.
> > -			 */
> > -			kvm_mmu_slot_remove_write_access(kvm, new, level);
> > +			kvm_mmu_slot_remove_write_access(kvm, new);
> >  		}
> >  	} else {
> >  		if (kvm_x86_ops.slot_disable_log_dirty)
> >