Skip to contentSkip to conversation

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

PATCH In review Vijaya Kumar K 3 participants 5 messages Sep 6, 2017 – Sep 19, 2017

Subsystems (2)
KVM KVM arm64
lore
Share & tools
lore

Vijaya Kumar K

author
#0 lore raw

PATCH 1/1 KVM: arm64: vgic-its: Fix wrong return value check in vgic_its_restore_device_tables

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 <[email protected]>
virt/kvm/arm/vgic/vgic-its.c +1 -1
index aa6b68d..63f8ac3 100644
@@ -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;
Signature · 1 line
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
#1 lore raw

Hi Vijaya,

Quoted 1 line · Vijaya Kumar K

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

Quoted 13 lines

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]>
virt/kvm/arm/vgic/vgic-its.c +0 -0
index aa6b68d..63f8ac3 100644

virt/kvm/arm/vgic/vgic-its.c static int vgic_its_restore_device_tables(struct vgic_its *its) · line 2146

Quoted 6 lines
virt/kvm/arm/vgic/vgic-its.c +1 -1
@@ -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

Quoted 3 lines
virt/kvm/arm/vgic/vgic-its.c +0 -0
 	return ret;
#2 lore raw
Quoted 12 lines · Auger Eric

Hi Vijaya,

Quoted 1 line · Vijaya Kumar K

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.

Quoted 37 lines

In the function vgic_its_restore_device_tables()

Quoted 13 lines

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]>
virt/kvm/arm/vgic/vgic-its.c +0 -0
index aa6b68d..63f8ac3 100644

virt/kvm/arm/vgic/vgic-its.c static int vgic_its_restore_device_tables(struct vgic_its *its) · line 2146

Quoted 6 lines
virt/kvm/arm/vgic/vgic-its.c +1 -1
@@ -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

virt/kvm/arm/vgic/vgic-its.c static int vgic_its_restore_device_tables(struct vgic_its *its) · line 2148

Quoted 3 lines
virt/kvm/arm/vgic/vgic-its.c +0 -0
 	return ret;

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

author
#3 lore raw
In reply to Auger Eric

Hi Eric,

Sorry for delayed reply.

Quoted 11 lines

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

Quoted 7 lines

Hi Vijaya,

Quoted 1 line · Vijaya Kumar K

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

Quoted 4 lines

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

Quoted 17 lines

In the function vgic_its_restore_device_tables()

virt/kvm/arm/vgic/vgic-its.c static int vgic_its_restore_device_tables(struct

Quoted 13 lines

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]>
(quoted diff) +0 -0

b/virt/kvm/arm/vgic/vgic-its.c index aa6b68d..63f8ac3 100644

virt/kvm/arm/vgic/vgic-its.c +0 -0
@@ -2142,7 +2142,7 @@ static int vgic_its_restore_device_tables(struct

vgic_its *its)

Quoted 8 lines

virt/kvm/arm/vgic/vgic-its.c static int vgic_its_restore_device_tables(struct vgic_its *its) · line 2146

Quoted 6 lines
virt/kvm/arm/vgic/vgic-its.c +1 -1
 				     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

Quoted 1 line

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

Quoted 1 line

0 return 0.

as below. Please correct me if I wrong.

           If (ret < 0)
                          ret = -EINVAL;
           else
                          ret = 0;
#4 lore raw

Hi Vijaya,

Quoted 55 lines · On 17/09/2017 10:10, [email protected] wrote:

Hi Eric,

Sorry for delayed reply.

Quoted 11 lines

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

Quoted 7 lines

Hi Vijaya,

Quoted 1 line · Vijaya Kumar K

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

Quoted 4 lines

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

Quoted 17 lines

In the function vgic_its_restore_device_tables()

virt/kvm/arm/vgic/vgic-its.c static int vgic_its_restore_device_tables(struct

Quoted 13 lines

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]>
(quoted diff) +0 -0

b/virt/kvm/arm/vgic/vgic-its.c index aa6b68d..63f8ac3 100644

virt/kvm/arm/vgic/vgic-its.c +0 -0
@@ -2142,7 +2142,7 @@ static int vgic_its_restore_device_tables(struct

vgic_its *its)

Quoted 8 lines

virt/kvm/arm/vgic/vgic-its.c static int vgic_its_restore_device_tables(struct vgic_its *its) · line 2146

Quoted 6 lines
virt/kvm/arm/vgic/vgic-its.c +1 -1
 				     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

Quoted 6 lines

what we

Quoted 1 line

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.

Quoted 7 lines

So only check for < 0 and return -EINVAL. For all other return values 0 and

Quoted 1 line

0 return 0.

as below. Please correct me if I wrong.

           If (ret < 0)
                          ret = -EINVAL;

why overriding ret value by -EINVAL?

Quoted 2 lines
           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

Quoted 7 lines

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel