Re: [RFC PATCH 3/3] kvm: arm/arm64: vgic-its: fix return value for restore

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

Hi,

On 13/09/2017 22:02, Christoffer Dall wrote:
> Hi Eric,
> 
> On Wed, Sep 06, 2017 at 05:18:35PM +0200, Auger Eric wrote:
>> Hi Wanghaibin,
>>
>> On 06/09/2017 15:05, wanghaibin wrote:
>>> This patch fix the migrate restore tables failure.
>>>
>>> The same scene, at the destination, the restore tables interface traversal guest
>>> memory, and check the dte/ite is valid or not.
>>> If all dtes/ites are invalid, we will do try next one, and the last it will take
>>> the 1 return value, but currently, it be treated as error. That's not correct.
>> There's indeed a bug here! In case all entries are invalid we shouldn't
>> return an error.
>>
>> One solution could be to relax the error checking in scan_its_table()
>> and do not return 1 when the whole length has been scanned. This would
>> fix your issue.
If you do not return 1 in scan_its_table if the whole size has been
scanned, you achieve the same thing as in this patch and you simplify
the error handling.
>>
>> drawback of that change:
>> at the moment we check the consistency of the entry data (next offset
>> field). At the moment if the next_offset points to an entry outside of
>> the table scope we are able to return an error (for top level tables).
>>
>> Otherwise, if we want to keep that check, I think we would need to add a
>> bool *valid parameter to entry_fn_t. in scan_its_table() we would return
>> 1 only if last fn() call returns valid and len <= 0.
>>
> 
> I don't really understand what you're proposing.
The above method or Wanghaibin's patch removes a consistency check on
the entry next offset field. But maybe this is better to drop it and
have a code that gains in readability.

Thanks

Eric
> 
> I think Wanghaibin's patch actually looks correct.  Is there any problem
> with it that you see?
> 
> Thanks,
> -Christoffer
> 
>>>
>>> This patch try to fix this problem.
>>>
>>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-its.c | 5 +----
>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>> index 5c20352..2c69aeb 100644
>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>> @@ -2025,7 +2025,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
>>>  		return PTR_ERR(dev);
>>>  
>>>  	ret = vgic_its_restore_itt(its, dev);
>>> -	if (ret) {
>>> +	if (ret < 0) {
>>>  		vgic_its_free_device(its->dev->kvm, dev);
>>>  		return ret;
>>>  	}
>>> @@ -2147,9 +2147,6 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>>>  				     vgic_its_restore_dte, NULL);
>>>  	}
>>>  
>>> -	if (ret > 0)
>>> -		ret = -EINVAL;
>>> -
>>>  	return ret;
>>>  }
>>>  
>>>