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

[Zenghui Yu <yuzenghui@huawei.com>]

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