Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot

[Keqian Zhu]

On 2021/3/9 21:48, Thomas Huth wrote:
> On 17/12/2020 02.49, Keqian Zhu wrote:
>> The parameters start and size are transfered from QEMU memory
>> emulation layer. It can promise that they are TARGET_PAGE_SIZE
>> aligned. However, KVM needs they are qemu_real_page_size aligned.
>>
>> Though no caller breaks this aligned requirement currently, we'd
>> better add an explicit assert to avoid future breaking.
>>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>>   accel/kvm/kvm-all.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> ---
>> v2
>>   - Address Andrew's commment (Use assert instead of return err).
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index f6b16a8df8..73b195cc41 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -692,6 +692,10 @@ out:
>>   #define KVM_CLEAR_LOG_ALIGN  (qemu_real_host_page_size << 
>> KVM_CLEAR_LOG_SHIFT)
>>   #define KVM_CLEAR_LOG_MASK   (-KVM_CLEAR_LOG_ALIGN)
>>   +/*
>> + * As the granule of kvm dirty log is qemu_real_host_page_size,
>> + * @start and @size are expected and restricted to align to it.
>> + */
>>   static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>>                                     uint64_t size)
>>   {
>> @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int 
>> as_id, uint64_t start,
>>       unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
>>       int ret;
>>   +    /* Make sure start and size are qemu_real_host_page_size aligned */
>> +    assert(QEMU_IS_ALIGNED(start | size, psize));
> 
> Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
> 
> $ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
> qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690: 
> kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' 
> failed.
> Aborted (core dumped)
Hi Thomas,

I think this patch is ok, maybe it trigger a potential bug?

Thanks,
Keqian

> 
> Can we please revert this patch?
> 
>  Thomas
> 
> .
>