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

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

[Vijaya Kumar K]

scan_its_table() return 1 on success. 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;
 
 	return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

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

[Auger Eric]

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)


 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.

Thanks

Eric
>  
>  	return ret;
> 

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

[wanghaibin]

On 2017/9/6 15:22, Auger Eric wrote:

> 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)
> 

I think there maybe something wrong.
A weeks ago, I met the migrate failed both the save interface and restore interface.
The restore failed reason is that vm is in booting, and haven't to probe pci devs at that moment.
and all dtes is invalid at that moment.

In this case, the interface will return -EINVAL. I think we should take this as success.

Not only for dtes, but the ites.

Thanks.


> 
>  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.
> 
> Thanks
> 
> Eric
>>  
>>  	return ret;
>>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 
> 

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

[vkilari]

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.

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;
           else
                          ret = 0;

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

[Auger Eric]

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
>