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

[Peter Xu]

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>

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

Thanks,

-- 
Peter Xu