Re: [PATCH v7 05/12] KVM: arm64: Refactor kvm_arch_commit_memory_region()

[Ricardo Koller <ricarkol@google.com>]

On Mon, Apr 17, 2023 at 02:41:39PM +0800, Gavin Shan wrote:
> On 4/9/23 2:29 PM, Ricardo Koller wrote:
> > Refactor kvm_arch_commit_memory_region() as a preparation for a future
> > commit to look cleaner and more understandable. Also, it looks more
> > like its x86 counterpart (in kvm_mmu_slot_apply_flags()).
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> > ---
> >   arch/arm64/kvm/mmu.c | 15 +++++++++++----
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> 
> With the following nits addressed:
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> 
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index efdaab3f154de..37d7d2aa472ab 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1761,20 +1761,27 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> >   				   const struct kvm_memory_slot *new,
> >   				   enum kvm_mr_change change)
> >   {
> > +	bool log_dirty_pages = new && new->flags & KVM_MEM_LOG_DIRTY_PAGES;
> > +
> >   	/*
> >   	 * At this point memslot has been committed and there is an
> >   	 * allocated dirty_bitmap[], dirty pages will be tracked while the
> >   	 * memory slot is write protected.
> >   	 */
> > -	if (change != KVM_MR_DELETE && new->flags & KVM_MEM_LOG_DIRTY_PAGES) {
> > +	if (log_dirty_pages) {
> > +
> > +		if (change == KVM_MR_DELETE)
> > +			return;
> > +
> >   		/*
> >   		 * If we're with initial-all-set, we don't need to write
> >   		 * protect any pages because they're all reported as dirty.
> >   		 * Huge pages and normal pages will be write protect gradually.
> >   		 */
> 
> The comments need to be adjusted after this series is applied. The huge pages
> won't be write protected gradually. Instead, the huge pages will be split and
> write protected in one shoot.
>

I see, this comment is a bit confusing. Will update it to this:

                /*
                 * Pages are write-protected on either of these two
                 * cases:
                 *
                 * 1. with initial-all-set: gradually with CLEAR ioctls,
                 */
                if (kvm_dirty_log_manual_protect_and_init_set(kvm))
                        return;
                /*
                 * or
                 * 2. without initial-all-set: all in one shot when
                 *    enabling dirty logging.
                 */
                kvm_mmu_wp_memory_region(kvm, new->id);

Will update the comment to include splitting when introducing eager-splitting
on the CLEAR ioctl (case 1.): "KVM: arm64: Split huge pages during
KVM_CLEAR_DIRTY_LOG".

> > -		if (!kvm_dirty_log_manual_protect_and_init_set(kvm)) {
> > -			kvm_mmu_wp_memory_region(kvm, new->id);
> > -		}
> > +		if (kvm_dirty_log_manual_protect_and_init_set(kvm))
> > +			return;
> > +
> > +		kvm_mmu_wp_memory_region(kvm, new->id);
> >   	}
> >   }
> > 
> 
> Thanks,
> Gavin
>