[PATCH v2 0/3] Workaround for bus reset on Cavium cn8xxx root ports

[PATCH v2 0/3] Workaround for bus reset on Cavium cn8xxx root ports

[Jan Glauber]

I've picked this up from David, keeping his patches but preventing to
probe vfio-pci devices that can't be reset. 

Without this series starting qemu with a vfio-pci enabled device
can lead to a kernel panic on Cavium systems, depending on the used
hardware.

Changes to v1 (https://lkml.org/lkml/2017/5/15/934):
- Prevent probing by vfio-pci

David Daney (2):
  PCI: Allow PCI_DEV_FLAGS_NO_BUS_RESET to be used on bus device
  PCI: Avoid bus reset for Cavium cn8xxx root ports

Jan Glauber (1):
  vfio/pci: Don't probe devices that can't be reset

 drivers/pci/pci.c           | 4 ++++
 drivers/pci/quirks.c        | 8 ++++++++
 drivers/vfio/pci/vfio_pci.c | 6 ++++++
 3 files changed, 18 insertions(+)

-- 
2.9.0.rc0.21.g7777322

[PATCH v2 1/3] PCI: Allow PCI_DEV_FLAGS_NO_BUS_RESET to be used on bus device

[Jan Glauber]

From: David Daney <david.daney@cavium.com>

When checking to see if a PCI bus can safely be reset, we check to see
if any of the children have their PCI_DEV_FLAGS_NO_BUS_RESET flag set.
As these devices are known not to behave well after a bus reset.

Some PCIe root port bridges also do not behave well after a bus reset,
sometimes causing the devices behind the bridge to become unusable.

Add a check for the PCI_DEV_FLAGS_NO_BUS_RESET flag being set in the
bridge device to allow these bridges to be flagged, and prevent their
buses from being reset.

A follow on patch will add a quirk for this type of bridge.

Signed-off-by: David Daney <david.daney@cavium.com>
[jglauber at cavium.com: fixed typo]
Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/pci/pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index af0cc34..d9abbc9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4290,6 +4290,10 @@ static bool pci_bus_resetable(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
 
+
+	if (bus->self && (bus->self->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET))
+		return false;
+
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		if (dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
 		    (dev->subordinate && !pci_bus_resetable(dev->subordinate)))
-- 
2.9.0.rc0.21.g7777322

[PATCH v2 2/3] PCI: Avoid bus reset for Cavium cn8xxx root ports

[Jan Glauber]

From: David Daney <david.daney@cavium.com>

Root ports of cn8xxx do not function after bus reset when used with
some e1000e and LSI HBA devices. Add a quirk to prevent bus reset on
these root ports.

Signed-off-by: David Daney <david.daney@cavium.com>
[jglauber at cavium.com: fixed typo and whitespaces]
Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/pci/quirks.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 6967c6b..85191b8 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3364,6 +3364,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0032, quirk_no_bus_reset);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x003c, quirk_no_bus_reset);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0033, quirk_no_bus_reset);
 
+/*
+ * Root port on some Cavium CN8xxx chips do not successfully complete
+ * a bus reset when used with certain types of child devices. Config
+ * space access to the child may quit responding. Flag the root port
+ * as not supporting bus reset.
+ */
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
+
 static void quirk_no_pm_reset(struct pci_dev *dev)
 {
 	/*
-- 
2.9.0.rc0.21.g7777322

[PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset

[Jan Glauber]

If a PCI device supports neither function-level reset, nor slot
or bus reset then refuse to probe it. A line is printed to inform
the user.

Without this change starting qemu with a vfio-pci device can lead to
a kernel panic on some Cavium cn8xxx systems, depending on the used
device.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/vfio/pci/vfio_pci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 063c1ce..029ba13 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1196,6 +1196,12 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
 		return -EINVAL;
 
+	ret = pci_probe_reset_bus(pdev->bus);
+	if (ret) {
+		dev_warn(&pdev->dev, "Refusing to probe because reset is not possible.\n");
+		return ret;
+	}
+
 	group = vfio_iommu_group_get(&pdev->dev);
 	if (!group)
 		return -EINVAL;
-- 
2.9.0.rc0.21.g7777322

[PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset

[Alex Williamson]

On Thu, 17 Aug 2017 10:14:23 +0200
Jan Glauber <jglauber@cavium.com> wrote:

> If a PCI device supports neither function-level reset, nor slot
> or bus reset then refuse to probe it. A line is printed to inform
> the user.

But that's not what this does, this requires that the device is on a
reset-able bus.  This is a massive regression.  With this we could no
longer assign devices on the root complex or any device which doesn't
return from bus reset and currently makes use of the NO_BUS_RESET flag
and works happily otherwise.  Full NAK.  Thanks,

Alex
 
> Without this change starting qemu with a vfio-pci device can lead to
> a kernel panic on some Cavium cn8xxx systems, depending on the used
> device.
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> ---
>  drivers/vfio/pci/vfio_pci.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 063c1ce..029ba13 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1196,6 +1196,12 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
>  		return -EINVAL;
>  
> +	ret = pci_probe_reset_bus(pdev->bus);
> +	if (ret) {
> +		dev_warn(&pdev->dev, "Refusing to probe because reset is not possible.\n");
> +		return ret;
> +	}
> +
>  	group = vfio_iommu_group_get(&pdev->dev);
>  	if (!group)
>  		return -EINVAL;

[PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset

[Jan Glauber]

On Thu, Aug 17, 2017 at 07:00:17AM -0600, Alex Williamson wrote:
> On Thu, 17 Aug 2017 10:14:23 +0200
> Jan Glauber <jglauber@cavium.com> wrote:
> 
> > If a PCI device supports neither function-level reset, nor slot
> > or bus reset then refuse to probe it. A line is printed to inform
> > the user.
> 
> But that's not what this does, this requires that the device is on a
> reset-able bus.  This is a massive regression.  With this we could no
> longer assign devices on the root complex or any device which doesn't
> return from bus reset and currently makes use of the NO_BUS_RESET flag
> and works happily otherwise.  Full NAK.  Thanks,

Looks like I missed the slot reset check. So how about this:

if (pci_probe_reset_slot(pdev->slot) && pci_probe_reset_bus(pdev->bus)) {
	dev_warn(...);
	return -ENODEV;
}

Or am I still missing something here?

thanks,
Jan

> Alex
>  
> > Without this change starting qemu with a vfio-pci device can lead to
> > a kernel panic on some Cavium cn8xxx systems, depending on the used
> > device.
> > 
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > ---
> >  drivers/vfio/pci/vfio_pci.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index 063c1ce..029ba13 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1196,6 +1196,12 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
> >  		return -EINVAL;
> >  
> > +	ret = pci_probe_reset_bus(pdev->bus);
> > +	if (ret) {
> > +		dev_warn(&pdev->dev, "Refusing to probe because reset is not possible.\n");
> > +		return ret;
> > +	}
> > +
> >  	group = vfio_iommu_group_get(&pdev->dev);
> >  	if (!group)
> >  		return -EINVAL;

[PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset

[Alex Williamson]

On Fri, 18 Aug 2017 15:42:31 +0200
Jan Glauber <jan.glauber@caviumnetworks.com> wrote:

> On Thu, Aug 17, 2017 at 07:00:17AM -0600, Alex Williamson wrote:
> > On Thu, 17 Aug 2017 10:14:23 +0200
> > Jan Glauber <jglauber@cavium.com> wrote:
> >   
> > > If a PCI device supports neither function-level reset, nor slot
> > > or bus reset then refuse to probe it. A line is printed to inform
> > > the user.  
> > 
> > But that's not what this does, this requires that the device is on a
> > reset-able bus.  This is a massive regression.  With this we could no
> > longer assign devices on the root complex or any device which doesn't
> > return from bus reset and currently makes use of the NO_BUS_RESET flag
> > and works happily otherwise.  Full NAK.  Thanks,  
> 
> Looks like I missed the slot reset check. So how about this:
> 
> if (pci_probe_reset_slot(pdev->slot) && pci_probe_reset_bus(pdev->bus)) {
> 	dev_warn(...);
> 	return -ENODEV;
> }
> 
> Or am I still missing something here?

We don't require that a device is on a reset-able bus/slot, so any
attempt to impose that requirement means that there are devices that
might work perfectly fine that are now excluded from assignment.  The
entire premise is unacceptable.  Thanks,

Alex

[PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset

[David Daney]

On 08/18/2017 07:12 AM, Alex Williamson wrote:
> On Fri, 18 Aug 2017 15:42:31 +0200
> Jan Glauber <jan.glauber@caviumnetworks.com> wrote:
> 
>> On Thu, Aug 17, 2017 at 07:00:17AM -0600, Alex Williamson wrote:
>>> On Thu, 17 Aug 2017 10:14:23 +0200
>>> Jan Glauber <jglauber@cavium.com> wrote:
>>>    
>>>> If a PCI device supports neither function-level reset, nor slot
>>>> or bus reset then refuse to probe it. A line is printed to inform
>>>> the user.
>>>
>>> But that's not what this does, this requires that the device is on a
>>> reset-able bus.  This is a massive regression.  With this we could no
>>> longer assign devices on the root complex or any device which doesn't
>>> return from bus reset and currently makes use of the NO_BUS_RESET flag
>>> and works happily otherwise.  Full NAK.  Thanks,
>>
>> Looks like I missed the slot reset check. So how about this:
>>
>> if (pci_probe_reset_slot(pdev->slot) && pci_probe_reset_bus(pdev->bus)) {
>> 	dev_warn(...);
>> 	return -ENODEV;
>> }
>>
>> Or am I still missing something here?
> 
> We don't require that a device is on a reset-able bus/slot, so any
> attempt to impose that requirement means that there are devices that
> might work perfectly fine that are now excluded from assignment.  The
> entire premise is unacceptable.  Thanks,


You previously rejected the idea to silently ignore bus reset requests 
on buses that do not support it.

So this leaves us with two options:

1) Do nothing, and crash the kernel on systems with bad combinations of 
PCIe target devices and cn88xx when vfio_pci is used.

2) Do something else.

We are trying to figure out what that something else should be.  The 
general concept we are working on is that if vfio_pci wants to reset a 
device, *and* bus reset is the only option available, *and* cn88xx, then 
make vfio_pci fail.

What is your opinion of doing that (assuming it is properly implemented)?

Thanks,
David Daney

[PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset

[Alex Williamson]

On Fri, 18 Aug 2017 08:57:09 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 08/18/2017 07:12 AM, Alex Williamson wrote:
> > On Fri, 18 Aug 2017 15:42:31 +0200
> > Jan Glauber <jan.glauber@caviumnetworks.com> wrote:
> >   
> >> On Thu, Aug 17, 2017 at 07:00:17AM -0600, Alex Williamson wrote:  
> >>> On Thu, 17 Aug 2017 10:14:23 +0200
> >>> Jan Glauber <jglauber@cavium.com> wrote:
> >>>      
> >>>> If a PCI device supports neither function-level reset, nor slot
> >>>> or bus reset then refuse to probe it. A line is printed to inform
> >>>> the user.  
> >>>
> >>> But that's not what this does, this requires that the device is on a
> >>> reset-able bus.  This is a massive regression.  With this we could no
> >>> longer assign devices on the root complex or any device which doesn't
> >>> return from bus reset and currently makes use of the NO_BUS_RESET flag
> >>> and works happily otherwise.  Full NAK.  Thanks,  
> >>
> >> Looks like I missed the slot reset check. So how about this:
> >>
> >> if (pci_probe_reset_slot(pdev->slot) && pci_probe_reset_bus(pdev->bus)) {
> >> 	dev_warn(...);
> >> 	return -ENODEV;
> >> }
> >>
> >> Or am I still missing something here?  
> > 
> > We don't require that a device is on a reset-able bus/slot, so any
> > attempt to impose that requirement means that there are devices that
> > might work perfectly fine that are now excluded from assignment.  The
> > entire premise is unacceptable.  Thanks,  
> 
> 
> You previously rejected the idea to silently ignore bus reset requests 
> on buses that do not support it.
> 
> So this leaves us with two options:
> 
> 1) Do nothing, and crash the kernel on systems with bad combinations of 
> PCIe target devices and cn88xx when vfio_pci is used.
> 
> 2) Do something else.
> 
> We are trying to figure out what that something else should be.  The 
> general concept we are working on is that if vfio_pci wants to reset a 
> device, *and* bus reset is the only option available, *and* cn88xx, then 
> make vfio_pci fail.

But that's not what these attempts do, they say if we can't do a bus or
slot reset, fail the device probe.  The comment is trying to suggest
they do something else, am I misinterpreting the actual code change?
There are plenty of devices out there that don't care if bus reset
doesn't work, they support FLR or PM reset or device specific reset or
just deal without a reset.  We can't suddenly say this new thing is a
requirement and sorry if you were happily using device assignment
before, but there's a slim chance you're on this platform that falls
over if we attempt to do a secondary bus reset.

> What is your opinion of doing that (assuming it is properly implemented)?

It seems like these attempts are trying to completely turn off vfio-pci
on cn88xx, do you just want it unsupported on these platforms?  Should
we blacklist anything where dev->bus->self is this root port?
Otherwise, what's wrong with returning an error if a bus reset fails,
because we should *never* silently ignore the request and pretend that
it worked, perhaps even dev_warn()'ing that the platform doesn't
support bus resets?  Thanks,

Alex

[PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset

[Jan Glauber]

On Fri, Aug 18, 2017 at 09:55:53PM -0600, Alex Williamson wrote:
> On Fri, 18 Aug 2017 08:57:09 -0700
> David Daney <ddaney@caviumnetworks.com> wrote:
> 
> > On 08/18/2017 07:12 AM, Alex Williamson wrote:

[...]

> > You previously rejected the idea to silently ignore bus reset requests 
> > on buses that do not support it.
> > 
> > So this leaves us with two options:
> > 
> > 1) Do nothing, and crash the kernel on systems with bad combinations of 
> > PCIe target devices and cn88xx when vfio_pci is used.
> > 
> > 2) Do something else.
> > 
> > We are trying to figure out what that something else should be.  The 
> > general concept we are working on is that if vfio_pci wants to reset a 
> > device, *and* bus reset is the only option available, *and* cn88xx, then 
> > make vfio_pci fail.
> 
> But that's not what these attempts do, they say if we can't do a bus or
> slot reset, fail the device probe.  The comment is trying to suggest
> they do something else, am I misinterpreting the actual code change?
> There are plenty of devices out there that don't care if bus reset
> doesn't work, they support FLR or PM reset or device specific reset or
> just deal without a reset.  We can't suddenly say this new thing is a
> requirement and sorry if you were happily using device assignment
> before, but there's a slim chance you're on this platform that falls
> over if we attempt to do a secondary bus reset.

Thanks for explaining this, I agree that we should not fail the device
probe as we only need to prevent the reset from happening.
So let's just drop this patch.


> > What is your opinion of doing that (assuming it is properly implemented)?
> 
> It seems like these attempts are trying to completely turn off vfio-pci
> on cn88xx, do you just want it unsupported on these platforms?  Should
> we blacklist anything where dev->bus->self is this root port?
> Otherwise, what's wrong with returning an error if a bus reset fails,
> because we should *never* silently ignore the request and pretend that
> it worked, perhaps even dev_warn()'ing that the platform doesn't
> support bus resets?  Thanks,

The ioctl's that trigger the slot/bus reset are already checking
if reset is possible. With David's patches pci_probe_reset_bus()
already fails.

But we also need to make pci_probe_reset_slot() fail on cn88xx to avoid
the same issue for the slot reset:

[  178.815041] [<fffffc000850b67c>] pci_generic_config_read+0x5c/0xf0
[  178.821221] [<fffffc0008534f60>] thunder_pem_config_read+0x90/0x228
[  178.827487] [<fffffc000850b564>] pci_bus_read_config_dword+0x84/0xb8
[  178.833841] [<fffffc000850d374>] pci_read_config_dword+0x5c/0x70
[  178.839848] [<fffffc0008513e54>] pci_find_next_ext_capability.part.7+0x44/0xc8
[  178.847075] [<fffffc0008514b00>] pci_find_ext_capability+0x48/0x58
[  178.853256] [<fffffc0008520e6c>] pci_restore_vc_state+0x44/0xa0
[  178.859175] [<fffffc0008514d4c>] pci_restore_state.part.26+0x3c/0x240
[  178.865614] [<fffffc0008514fe0>] pci_dev_restore+0x58/0x60
[  178.871098] [<fffffc00085150a0>] pci_slot_restore+0x60/0x78
[  178.876669] [<fffffc000851599c>] pci_try_reset_slot+0xcc/0x140
[  178.882512] [<fffffc0000d91b78>] vfio_pci_ioctl+0xb30/0xb88 [vfio_pci]
[  178.889050] [<fffffc0000ba02b4>] vfio_device_fops_unl_ioctl+0x44/0x70 [vfio]
[  178.896100] [<fffffc0008267e00>] do_vfs_ioctl+0xb0/0x748
[  178.901411] [<fffffc000826852c>] SyS_ioctl+0x94/0xa8
[  178.906375] [<fffffc00080834a0>] __sys_trace_return+0x0/0x4
[  178.911947] Code: 7100069f 540003c0 71000a9f 54000240 (b9400001) 
[  178.918108] ---[ end trace 07143dcba854194e ]---
[  178.922784] Kernel panic - not syncing: Fatal exception

So far I don't see how this can be done in a clean way, there is no quirk
available for the slot.

--Jan