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

[zhukeqian]

On 2020/12/11 23:25, Peter Xu wrote:
> On Fri, Dec 11, 2020 at 09:13:10AM +0800, zhukeqian wrote:
>>
>> On 2020/12/10 22:50, Peter Xu wrote:
>>> On Thu, Dec 10, 2020 at 10:53:23AM +0800, zhukeqian wrote:
>>>>
>>>>
>>>> On 2020/12/10 10:08, Peter Xu wrote:
>>>>> Keqian,
>>>>>
>>>>> On Thu, Dec 10, 2020 at 09:46:06AM +0800, zhukeqian wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I see that if start or size is not PAGE aligned, it also clears areas
>>>>>> which beyond caller's expectation, so do we also need to consider this?
>>>>>
>>>>> Could you elaborate?
>>>>>
>>>>> If start_delta != 0, kvm_log_clear_one_slot() should already go the slow 
>>>>> path.
>>>>>
>>>>> Thanks,
>>>>>
>>>>
>>>> Hi Peter,
>>>>
>>>> start_delta /= psize;
>>>>
>>>> If start is not PAGE aligned, then start_delta is not PAGE aligned.
>>>> so I think the above code will implicitly extend our start to be PAGE 
>>>> aligned.
>>>>
>>>> I suggest that we should shrink the start and (start + size) to be PAGE 
>>>> aligned
>>>> at beginning of this function.
>>>
>>> Callers should be with TARGET_PAGE_SIZE aligned on the size, so at least 
>>> x86_64
>>> should be pretty safe since host/guest page sizes match.
>>>
>>> Though indeed I must confess I don't know how it worked in general when host
>>> page size != target page size, at least for migration.  For example, I 
>>> believe
>>> kvm dirty logging is host page size based, though migration should be 
>>> migrating
>>> pages in guest page size granule when it spots a dirty bit set.
>>>
>> Hi,
>>
>> Indeed, we handle target_page_size aligned @start and @size in general. 
>> Maybe we'd better
>> add explicit function comments about alignment requirement, and explicit 
>> alignment assert
>> on @start and @size?
> 

Hi Peter,

> Yes we can, but I think it's not strongly necessary.  As Zenghui pointed out,
> the callers of memory_region_clear_dirty_bitmap() should always be aware of 
> the
> fact that dirty bitmap is always page size based.
Agree.

> 
> OTOH I'm more worried on the other question on how we handle guest psize !=
> host psize case for migration now...
I think it does not matter when guest_psize != host_psize, as we only need to 
interact with
stage2 page tables during migration. Stage2 is enough to tracking guest dirty 
memory, and even
if guest close stage1, we also can do a successful migration.

Please point out if I misunderstood what you meant.

Thanks,
Keqian

> 
> Thanks,
>