Re: [PATCH] kvm: Take into account the unaligned section size when preparing bitmap

[Zenghui Yu]

On 2020/12/10 5:09, Peter Xu wrote:
> On Wed, Dec 09, 2020 at 10:33:41AM +0800, Zenghui Yu wrote:
> > Hi Peter,
> >
> > Thanks for having a look at it.
> >
> > On 2020/12/8 23:16, Peter Xu wrote:
> > > Hi, Zenghui,
> > >
> > > On Tue, Dec 08, 2020 at 07:40:13PM +0800, Zenghui Yu wrote:
> > > > The kernel KVM_CLEAR_DIRTY_LOG interface has align requirement on both the
> > > > start and the size of the given range of pages. We have been careful to
> > > > handle the unaligned cases when performing CLEAR on one slot. But it seems
> > > > that we forget to take the unaligned *size* case into account when
> > > > preparing bitmap for the interface, and we may end up clearing dirty status
> > > > for pages outside of [start, start + size).
> > >
> > > Thanks for the patch, though my understanding is that this is not a bug.
> > >
> > > Please have a look at kvm_memslot_init_dirty_bitmap() where we'll allocate the
> > > dirty bitmap to be aligned to 8 bytes (assuming that's the possible max of the
> > > value sizeof(unsigned long)).  That exactly covers 64 pages.
> > >
> > > So here as long as start_delta==0 (so the value of "bmap_npages - size / psize"
> > > won't really matter a lot, imho), then we'll definitely have KVMSlot.dirty_bmap
> > > long enough to cover the range we'd like to clear.
> >
> > I agree.  But actually I'm not saying that KVMSlot.dirty_bmap is not
> > long enough.  What I was having in mind is something like:
> >
> >      // psize = qemu_real_host_page_size;
> >      // slot.start_addr = 0;
> >      // slot.memory_size = 64 * psize;
> >
> >      kvm_log_clear_one_slot(slot, as, 0 * psize, 32 * psize);   --> [1]
> >      kvm_log_clear_one_slot(slot, as, 32 * psize, 32 * psize);  --> [2]
> >
> > So the @size is not aligned with 64 pages.  Before this patch, we'll
> > clear dirty status for all pages(0-63) through [1].  It looks to me that
> > this violates the caller's expectation since we only want to clear
> > pages(0-31).
>
> Now I see; I think you're right. :)
>
> > As I said, I don't think this will happen in practice -- the migration
> > code should always provide us with a 64-page aligned section (right?).
>
> Yes, migration is the major consumer, and that should be guaranteed indeed, see
> CLEAR_BITMAP_SHIFT_MIN.
>
> Not sure about VGA - that should try to do log clear even without migration,
> but I guess that satisfies the 64-page alignment too, since it's not a huge
> number (256KB).  The VGA buffer size could be related to screen resolution,
> then N*1024*768 could still guarantee a safe use of the fast path.
>
> > I'm just thinking about the correctness of the specific algorithm used
> > by kvm_log_clear_one_slot().
>
> Yeah, then I think it's okay to have this, just in case someday we'll hit it.
>
> Acked-by: Peter Xu <peterx@redhat.com>

Thanks for that.

> (It would be nicer if above example could be squashed into commit message)

I'll squash it if v2 is needed.


Thanks,
Zenghui