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