Hi Vijaya,
On 17/09/2017 10:10, [email protected] wrote:
> Hi Eric,
>
> Sorry for delayed reply.
>
>> -----Original Message-----
>> From: Auger Eric [mailto:[email protected]]
>> Sent: Wednesday, September 6, 2017 12:53 PM
>> To: Vijaya Kumar K <[email protected]>;
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]
>> Subject: Re: [PATCH] KVM: arm64: vgic-its: Fix wrong return value check in
>> vgic_its_restore_device_tables
>>
>> Hi Vijaya,
>>
>> On 06/09/2017 07:26, Vijaya Kumar K wrote:
>>> scan_its_table() return 1 on success.
>>
>> As mentioned in the kernel-doc comment of scan_its_table, this latter
>> returns 1 if the last element is not found. Than can happen while scanning
> an
>> L2 table but shouldn't happen if we scan an L1 table.
>>
>> * Return: < 0 on error, 0 if last element was identified, 1 otherwise
>> * (the last element may not be found on second level tables)
>
> OK. I will fix this comment
>
>>
>>
>> In the function vgic_its_restore_device_tables()
>>> the return value of scan_its_table() is checked against success value
>>> and returns -EINVAL. Hence migration fails for VM with ITS.
>>>
>>> With this patch the failure return value is checked while returning
>>> -EINVAL.
>>>
>>> Signed-off-by: Vijaya Kumar K <[email protected]>
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c
>>> b/virt/kvm/arm/vgic/vgic-its.c index aa6b68d..63f8ac3 100644
>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>> @@ -2142,7 +2142,7 @@ static int vgic_its_restore_device_tables(struct
>> vgic_its *its)
>>> vgic_its_restore_dte, NULL);
>>> }
>>>
>>> - if (ret > 0)
>>> + if (ret <= 0)
>>> ret = -EINVAL;
>> your modification would return -EINVAL for whatever error encountered
>> during the scan table or if last element is found. I don't think this is
> what we
>> want.
>
> IIUC, ret 0 indicates last entry of the table. So in this case return value
> 0 is also success.
> with the assumption that table might be smaller than size.
0 indicates you successfully found all the valid data laid out in the
table and you are done.
>
> So only check for < 0 and return -EINVAL. For all other return values 0 and
>> 0 return 0.
> as below. Please correct me if I wrong.
>
> If (ret < 0)
> ret = -EINVAL;
why overriding ret value by -EINVAL?
> else
> ret = 0;
I would rather do:
if (ret > 0)
ret = 0;
return ret;
I think Wanghaibin intends to respin + his fix of same issue on
vgic_its_restore_itt returned value.
see https://www.spinics.net/lists/kvm-arm/msg27248.html
Thanks
Eric
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
