Re: [PATCH] KVM: Discard zero mask with function kvm_dirty_ring_reset

[maobibo]

On 2024/6/14 上午12:43, Sean Christopherson wrote:
> On Thu, Jun 13, 2024, Bibo Mao wrote:
> > Function kvm_reset_dirty_gfn may be called with parameters cur_slot /
> > cur_offset / mask are all zero, it does not represent real dirty page.
> > It is not necessary to clear dirty page in this condition. Also return
> > value of macro __fls() is undefined if mask is zero which is called in
> > funciton kvm_reset_dirty_gfn(). Here just discard it.
> >
> > Signed-off-by: Bibo Mao <maobibo@xxxxxxxxxxx>
> > ---
> >   virt/kvm/dirty_ring.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
> > index 86d267db87bb..05f4c1c40cc7 100644
> > --- a/virt/kvm/dirty_ring.c
> > +++ b/virt/kvm/dirty_ring.c
> > @@ -147,14 +147,16 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring)
> >   				continue;
> >   			}
> >   		}
> > -		kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
> > +		if (mask)
> > +			kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
> >   		cur_slot = next_slot;
> >   		cur_offset = next_offset;
> >   		mask = 1;
> >   		first_round = false;
> >   	}
> >   
> > -	kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
> > +	if (mask)
> > +		kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
>
> Given that mask must be checked before __fls(), just do:
That is ok for me. To be frankly I am not familiar with kvm common code,
I submit this patch just when I look through the migration source code.

Regards
Bibo Mao
> diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
> index 86d267db87bb..7bc74969a819 100644
> --- a/virt/kvm/dirty_ring.c
> +++ b/virt/kvm/dirty_ring.c
> @@ -55,6 +55,9 @@ static void kvm_reset_dirty_gfn(struct kvm *kvm, u32 slot, u64 offset, u64 mask)
>          struct kvm_memory_slot *memslot;
>          int as_id, id;
>   
> +       if (!mask)
> +               return;
> +
>          as_id = slot >> 16;
>          id = (u16)slot;