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

[Keqian Zhu]

On 2020/12/16 15:23, Andrew Jones wrote:
> On Tue, Dec 15, 2020 at 12:55:50PM +0100, Andrew Jones wrote:
>> On Tue, Dec 15, 2020 at 03:19:48PM +0800, 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 check to avoid future breaking.
>>>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> ---
>>>  accel/kvm/kvm-all.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index c5e06288eb..3d0e3aa872 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -701,6 +701,11 @@ 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 psize aligned */
>>> +    if (!QEMU_IS_ALIGNED(start, psize) || !QEMU_IS_ALIGNED(size, psize)) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>      /*
>>>       * We need to extend either the start or the size or both to
>>>       * satisfy the KVM interface requirement.  Firstly, do the start
>>> -- 
>>> 2.23.0
>>>
>>>
>>
>> It's not clear to me that this function has any restrictions on start
>> and size. If it does, then please document those restrictions in the
>> function's header and assert rather than return.
>>
> 
> Also, I see this patch is on its way in
> 
> https://patchwork.ozlabs.org/project/qemu-devel/patch/20201215175445.1272776-27-pbonzini@redhat.com/

Hi drew,

Thanks for informing me. I see that they are not the same patch.
The above patch fixes 64-pages-alignment problem,
but this patch handles page-alignment problem.

Thanks,
Keqian
> 
> Thanks,
> drew 
> 
> .
>