Re: [PATCH] KVM: arm64: vgic-its: Fix wrong return value check in vgic_its_restore_device_tables

[Auger Eric <eric.auger@redhat.com>]

Hi Vijaya,
On 17/09/2017 10:10, vkilari@codeaurora.org wrote:
> Hi Eric,
> 
>  Sorry for delayed reply.
> 
>> -----Original Message-----
>> From: Auger Eric [mailto:eric.auger@redhat.com]
>> Sent: Wednesday, September 6, 2017 12:53 PM
>> To: Vijaya Kumar K <vkilari@codeaurora.org>;
>> kvmarm@lists.cs.columbia.edu; marc.zyngier@arm.com; cdall@linaro.org;
>> andre.przywara@arm.com
>> Cc: vvenkat@codeaurora.org; shankerd@codeaurora.org;
>> kvm@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> 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 <vkilari@codeaurora.org>
>>>
>>> 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
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>