Hi Philippe,
On 2021/2/1 23:14, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 12/17/20 2:49 AM, 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 <zhukeqi...@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));
>
> Why not return an error instead of aborting the VM?
Yep, I return an error in v1. As suggested by Peter Xu: "Returning -EINVAL is
the same as abort() currently - it'll just abort() at
kvm_log_clear() instead."
>
>> /*
>> * We need to extend either the start or the size or both to
>> * satisfy the KVM interface requirement. Firstly, do the start
>>
>
> .
>
Thanks for review.
Keqian.
