virpci:fix Secondary Bus Reset bug

[libvirt] [PATCH] virpci:fix Secondary Bus Reset bug

Posted by hexin900110@163.com 4 years, 8 months ago

From: hexin <hexin15@baidu.com>

The parent bridge configuration of the current device
should be read and reset, instead of reading the current
device configuration.

Signed-off-by: He Xin <hexin15@baidu.com>
Signed-off-by: Liu Qi <liuqi16@baidu.com>
Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
---
 src/util/virpci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 61a6b359e5..483de2cb16 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -821,7 +821,7 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev,
     /* Read the control register, set the reset flag, wait 200ms,
      * unset the reset flag and wait 200ms.
      */
-    ctl = virPCIDeviceRead16(dev, cfgfd, PCI_BRIDGE_CONTROL);
+    ctl = virPCIDeviceRead16(dev, parentfd, PCI_BRIDGE_CONTROL);
 
     virPCIDeviceWrite16(parent, parentfd, PCI_BRIDGE_CONTROL,
                         ctl | PCI_BRIDGE_CTL_RESET);
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virpci:fix Secondary Bus Reset bug

Posted by Michal Privoznik 4 years, 8 months ago

On 8/15/19 11:44 AM, hexin900110@163.com wrote:
> From: hexin <hexin15@baidu.com>
> 
> The parent bridge configuration of the current device
> should be read and reset, instead of reading the current
> device configuration.
> 
> Signed-off-by: He Xin <hexin15@baidu.com>
> Signed-off-by: Liu Qi <liuqi16@baidu.com>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> ---
>   src/util/virpci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

And pushed. Nice catch.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virpci:fix Secondary Bus Reset bug

Posted by Laine Stump 4 years, 8 months ago

The fact that you are modifying this code implies that you are using it, 
and that implies that you are still using legacy KVM device assignment 
(i.e. the pcistub driver) instead of VFIO device assignment. (I say this 
because the function that calls this function you've patched, 
virPCIDeviceReset(), has a check very early on that short-circuits the 
entire function if the device is bound to vfio-pci (and if the device is 
bound to a host driver, then you shouldn't be resetting it anyway, so 
the only reason it would be executed is if you're using legacy KVM 
device assignment or are calling virsh nodedev-reset when you shouldn't).

Legacy PCI device assignment was deprecated over 5 years ago, and was 
removed from the kernel sometime after that. We are seriously 
considering removing all vestigal support for legacy KVM device 
assignment from libvirt, since any kernel that is less than 5 years old 
have VFIO, and most don't even support legacy KVM device assignment at all.

So are you actually using legacy KVM device assignment, or did you just 
find this bug by manual code inspection? If the latter, then you need to 
seriously look into using VFIO instead. If the latter, then this patch 
can safely be pushed, but it's not going to actually do anything (and 
the code it's in will likely no longer exist within the next 6 months).

On 8/15/19 5:44 AM, hexin900110@163.com wrote:
> From: hexin <hexin15@baidu.com>
>
> The parent bridge configuration of the current device
> should be read and reset, instead of reading the current
> device configuration.
>
> Signed-off-by: He Xin <hexin15@baidu.com>
> Signed-off-by: Liu Qi <liuqi16@baidu.com>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> ---
>   src/util/virpci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 61a6b359e5..483de2cb16 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -821,7 +821,7 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev,
>       /* Read the control register, set the reset flag, wait 200ms,
>        * unset the reset flag and wait 200ms.
>        */
> -    ctl = virPCIDeviceRead16(dev, cfgfd, PCI_BRIDGE_CONTROL);
> +    ctl = virPCIDeviceRead16(dev, parentfd, PCI_BRIDGE_CONTROL);
>   
>       virPCIDeviceWrite16(parent, parentfd, PCI_BRIDGE_CONTROL,
>                           ctl | PCI_BRIDGE_CTL_RESET);

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virpci:fix Secondary Bus Reset bug

Posted by Michal Privoznik 4 years, 8 months ago

On 8/16/19 5:10 PM, Laine Stump wrote:
 >

To extend this question - there's also xen's pciback, but I don't know 
enough about Xen to tell if it's still in use. Might be worth dropping 
that too, becasue it looks a lot like pci-stub (used for KVM 
assignment). And because of that I can't really drop all the functions, 
only very few. Jim?

BTW: does Xen's kernel have 'driver_override' file for PCI devices? See 
virPCIDeviceBindWithDriverOverride() for more info. But if it does, I'd 
like to also drop the old 'newid' style of overriding kernel driver and 
leave us with solely with 'driver_override' (introduced in kernel-3.16.0 
so I guess everybody has it).

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virpci:fix Secondary Bus Reset bug

Posted by Jim Fehlig 4 years, 8 months ago

On 8/19/19 7:22 AM, Michal Privoznik wrote:
> On 8/16/19 5:10 PM, Laine Stump wrote:
>  >
> 
> To extend this question - there's also xen's pciback, but I don't know enough 
> about Xen to tell if it's still in use.

Yes, it is still in use.

> Might be worth dropping that too, 
> becasue it looks a lot like pci-stub (used for KVM assignment). And because of 
> that I can't really drop all the functions, only very few. Jim?

Nooooooo! Please don't drop (or break) functionality related to xen-pciback :-).

> BTW: does Xen's kernel have 'driver_override' file for PCI devices? See 
> virPCIDeviceBindWithDriverOverride() for more info.

Yes, xen supports driver_override.

> But if it does, I'd like to 
> also drop the old 'newid' style of overriding kernel driver and leave us with 
> solely with 'driver_override' (introduced in kernel-3.16.0 so I guess everybody 
> has it).

Would be nice to kill the 'newid' handling. I thought it sounded familiar, and 
not in a good way:

70f83f9d526 (Jim Fehlig 2016-08-01 21:36:45 -0600 1265)     * to the unpleasant 
new_id interface.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virpci:fix Secondary Bus Reset bug

Posted by 何鑫 4 years, 8 months ago

At 2019-08-16 23:10:31, "Laine Stump" <laine@laine.org> wrote:
>The fact that you are modifying this code implies that you are using it, 
>and that implies that you are still using legacy KVM device assignment 
>(i.e. the pcistub driver) instead of VFIO device assignment. (I say this 
>because the function that calls this function you've patched, 
>virPCIDeviceReset(), has a check very early on that short-circuits the 
>entire function if the device is bound to vfio-pci (and if the device is 
>bound to a host driver, then you shouldn't be resetting it anyway, so 
>the only reason it would be executed is if you're using legacy KVM 
>device assignment or are calling virsh nodedev-reset when you shouldn't).
>
>Legacy PCI device assignment was deprecated over 5 years ago, and was 
>removed from the kernel sometime after that. We are seriously 
>considering removing all vestigal support for legacy KVM device 
>assignment from libvirt, since any kernel that is less than 5 years old 
>have VFIO, and most don't even support legacy KVM device assignment at all.
>
>So are you actually using legacy KVM device assignment, or did you just 
>find this bug by manual code inspection? If the latter, then you need to 
>seriously look into using VFIO instead. If the latter, then this patch 
>can safely be pushed, but it's not going to actually do anything (and 
>the code it's in will likely no longer exist within the next 6 months).
>
>On 8/15/19 5:44 AM, hexin900110@163.com wrote:
>> From: hexin <hexin15@baidu.com>
>>
>> The parent bridge configuration of the current device
>> should be read and reset, instead of reading the current
>> device configuration.
>>
>> Signed-off-by: He Xin <hexin15@baidu.com>
>> Signed-off-by: Liu Qi <liuqi16@baidu.com>
>> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
>> ---
>>   src/util/virpci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index 61a6b359e5..483de2cb16 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -821,7 +821,7 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev,
>>       /* Read the control register, set the reset flag, wait 200ms,
>>        * unset the reset flag and wait 200ms.
>>        */
>> -    ctl = virPCIDeviceRead16(dev, cfgfd, PCI_BRIDGE_CONTROL);
>> +    ctl = virPCIDeviceRead16(dev, parentfd, PCI_BRIDGE_CONTROL);
>>   
>>       virPCIDeviceWrite16(parent, parentfd, PCI_BRIDGE_CONTROL,
>>                           ctl | PCI_BRIDGE_CTL_RESET);

>


Thanks for attention. I finded this bug by manual code inspection.


Thanks，
HeXin--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list