[PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[David Daney]

With the recent improvements in arm64 and vfio-pci, we are seeing
failures like this (on cn8890 based systems):

[  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
[  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
.
.
.
[  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
[  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
[  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
[  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
[  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
[  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
[  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
[  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
[  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
[  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
[  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
[  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
[  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
[  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
[  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4

These are caused by the inability of the PCIe root port and Intel
e1000e to sucessfully do a bus reset.

The proposed fix is to not do a bus reset on these systems.

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.

 drivers/pci/pci.c    | 4 ++++
 drivers/pci/quirks.c | 8 ++++++++
 2 files changed, 12 insertions(+)

-- 
2.9.4

[PATCH 1/2] PCI: Allow PCI_DEV_FLAGS_NO_BUS_RESET to be used on bus device.

[David Daney]

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 know 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>
---
 drivers/pci/pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5b..b64e4d1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4318,6 +4318,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.4

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

[David Daney]

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>
---
 drivers/pci/quirks.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 085fb78..02cd847 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3347,6 +3347,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 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.4

Re: [PATCH 1/2] PCI: Allow PCI_DEV_FLAGS_NO_BUS_RESET to be used on bus device.

[Auger Eric]

Hi,

On 16/05/2017 02:17, David Daney wrote:
> 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 know not to behave well after a bus reset.
nit: s/know/known
> 
> 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>

Thanks

Eric
> ---
>  drivers/pci/pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5b..b64e4d1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4318,6 +4318,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)))
> 

Re: [PATCH 2/2] PCI: Avoid bus reset for Cavium cn8xxx root ports.

[Auger Eric]

Hi,

On 16/05/2017 02:17, David Daney wrote:
> 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.
I understand the bus reset would work along with a variety of other
child devices. I guess there is no way to be more accurate and forbid
the bus reset only when incompatible child devices are found?
> 
> Signed-off-by: David Daney <david.daney@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 085fb78..02cd847 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3347,6 +3347,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 child devices.  Config
s/types /types of
Thanks

Eric
> + * 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)
>  {
>  	/*
> 

Re: [PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[Auger Eric]

Hi David,

On 16/05/2017 02:17, David Daney wrote:
> With the recent improvements in arm64 and vfio-pci, we are seeing
> failures like this (on cn8890 based systems):
> 
> [  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
> [  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
> .
> .
> .
> [  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
> [  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
> [  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
> [  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
> [  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
> [  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
> [  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
> [  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
> [  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
> [  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
> [  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
> [  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
> [  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
> [  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
> [  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4
> 
> These are caused by the inability of the PCIe root port and Intel
> e1000e to sucessfully do a bus reset.

I tested the series on Cavium ThunderX with an e1000e device and it
fixes the above issue.

Feel free to add
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


> 
> The proposed fix is to not do a bus reset on these systems.
> 
> 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.
> 
>  drivers/pci/pci.c    | 4 ++++
>  drivers/pci/quirks.c | 8 ++++++++
>  2 files changed, 12 insertions(+)
> 

Re: [PATCH 2/2] PCI: Avoid bus reset for Cavium cn8xxx root ports.

[David Daney]

On 05/16/2017 01:14 PM, Auger Eric wrote:
> Hi,
> 
> On 16/05/2017 02:17, David Daney wrote:
>> 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.
> I understand the bus reset would work along with a variety of other
> child devices. I guess there is no way to be more accurate and forbid
> the bus reset only when incompatible child devices are found?

That's right.  Conceptually we have a 2 dimensional array of all 
possible upstream devices vs. all possible downstream devices.  The 
maintenance burden of correctly populating this and keeping it up to 
date would be impossible.

The e1000e works well, I am sure, with most Intel root ports.  We 
haven't investigated exactly which end of the link is responsible for 
the failures against the cn8890 root port, but the simplest way forward 
is to just say it cannot support bus reset.

>>
>> Signed-off-by: David Daney <david.daney@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 085fb78..02cd847 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3347,6 +3347,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 child devices.  Config
> s/types /types of

Yeah, I need to be more careful.  We really need to improve the 
grammatical analysis capabilities of checkpatch.pl, I think I will lay 
the blame there.

I can resubmit or Bjorn can fix it before committing, I will let him decide.

David.


> Thanks
> 
> Eric
>> + * 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)
>>   {
>>   	/*
>>

Re: [PATCH 2/2] PCI: Avoid bus reset for Cavium cn8xxx root ports.

[Alex Williamson]

On Tue, 16 May 2017 13:29:58 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 05/16/2017 01:14 PM, Auger Eric wrote:
> > Hi,
> > 
> > On 16/05/2017 02:17, David Daney wrote:  
> >> 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.  
> > I understand the bus reset would work along with a variety of other
> > child devices. I guess there is no way to be more accurate and forbid
> > the bus reset only when incompatible child devices are found?  
> 
> That's right.  Conceptually we have a 2 dimensional array of all 
> possible upstream devices vs. all possible downstream devices.  The 
> maintenance burden of correctly populating this and keeping it up to 
> date would be impossible.
> 
> The e1000e works well, I am sure, with most Intel root ports.  We 
> haven't investigated exactly which end of the link is responsible for 
> the failures against the cn8890 root port, but the simplest way forward 
> is to just say it cannot support bus reset.

That's a pretty serious limitation though, you only need to look at the
world of graphics cards where the device offers no capability for
function level reset and PM reset does nothing useful.  The only way we
can get the device to a fairly reproducible state is via bus reset.  So
let's just throw out GPU assignment on this platform as unsupportable.
What other sorts of devices are we going to find have inconsistent
behavior compared to platforms where bus reset is possible?  What's
going to be the user experience?  Tracking which devices have trouble
with bus reset is a problem, but so is limiting access to bus reset
where it really is useful.  Thanks,

Alex

> >>
> >> Signed-off-by: David Daney <david.daney@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 085fb78..02cd847 100644
> >> --- a/drivers/pci/quirks.c
> >> +++ b/drivers/pci/quirks.c
> >> @@ -3347,6 +3347,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 child devices.  Config  
> > s/types /types of  
> 
> Yeah, I need to be more careful.  We really need to improve the 
> grammatical analysis capabilities of checkpatch.pl, I think I will lay 
> the blame there.
> 
> I can resubmit or Bjorn can fix it before committing, I will let him decide.
> 
> David.
> 
> 
> > Thanks
> > 
> > Eric  
> >> + * 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)
> >>   {
> >>   	/*
> >>  
> 

Re: [PATCH 2/2] PCI: Avoid bus reset for Cavium cn8xxx root ports.

[Joe Perches]

On Tue, 2017-05-16 at 13:29 -0700, David Daney wrote:
> We really need to improve the 
> grammatical analysis capabilities of checkpatch.pl, I think I will lay 
> the blame there.

Uh huh.  Best of luck.  Submit a patch...

;)

Re: [PATCH 2/2] PCI: Avoid bus reset for Cavium cn8xxx root ports.

[Jon Masters]

Hahahahahaha :)

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

> On May 17, 2017, at 03:08, Joe Perches <joe@perches.com> wrote:
> 
>> On Tue, 2017-05-16 at 13:29 -0700, David Daney wrote:
>> We really need to improve the 
>> grammatical analysis capabilities of checkpatch.pl, I think I will lay 
>> the blame there.
> 
> Uh huh.  Best of luck.  Submit a patch...
> 
> ;)
> 

Re: [PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[Bjorn Helgaas]

On Mon, May 15, 2017 at 05:17:34PM -0700, David Daney wrote:
> With the recent improvements in arm64 and vfio-pci, we are seeing
> failures like this (on cn8890 based systems):
> 
> [  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
> [  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
> .
> .
> .
> [  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
> [  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
> [  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
> [  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
> [  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
> [  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
> [  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
> [  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
> [  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
> [  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
> [  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
> [  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
> [  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
> [  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
> [  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4
> 
> These are caused by the inability of the PCIe root port and Intel
> e1000e to sucessfully do a bus reset.
> 
> The proposed fix is to not do a bus reset on these systems.
> 
> 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.
> 
>  drivers/pci/pci.c    | 4 ++++
>  drivers/pci/quirks.c | 8 ++++++++
>  2 files changed, 12 insertions(+)

Applied with Eric's reviewed-by and typo fixes to pci/virtualization for
v4.13, thanks!

Re: [PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[Alex Williamson]

On Tue, 23 May 2017 15:47:50 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Mon, May 15, 2017 at 05:17:34PM -0700, David Daney wrote:
> > With the recent improvements in arm64 and vfio-pci, we are seeing
> > failures like this (on cn8890 based systems):
> > 
> > [  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
> > [  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
> > .
> > .
> > .
> > [  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
> > [  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
> > [  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
> > [  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
> > [  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
> > [  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
> > [  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
> > [  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
> > [  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
> > [  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
> > [  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
> > [  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
> > [  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
> > [  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
> > [  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4
> > 
> > These are caused by the inability of the PCIe root port and Intel
> > e1000e to sucessfully do a bus reset.
> > 
> > The proposed fix is to not do a bus reset on these systems.
> > 
> > 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.
> > 
> >  drivers/pci/pci.c    | 4 ++++
> >  drivers/pci/quirks.c | 8 ++++++++
> >  2 files changed, 12 insertions(+)  
> 
> Applied with Eric's reviewed-by and typo fixes to pci/virtualization for
> v4.13, thanks!

Hmm, well let me again express my concerns that I'm really not sure how
to support this since it removes our last opportunity to reset devices
that may otherwise have no reset mechanism.  Certain classes of devices
are entirely unsupportable for the code path indicated above without a
bus reset.  If we have an endpoint device that goes bonkers at a bus
reset, at least we know it's going to behave just as poorly no matter
what the host platform.  This series allows endpoints that work
perfectly well on one host to be handled differently on another.  It
certainly suggests something non-spec compliant about the root port
implementation and I wish there was more analysis about exactly what
that problem is since this is coming from the hardware vendor.

https://lkml.org/lkml/2017/5/16/662

Thanks,
Alex

Re: [PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[Bjorn Helgaas]

On Tue, May 23, 2017 at 03:04:04PM -0600, Alex Williamson wrote:
> On Tue, 23 May 2017 15:47:50 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > On Mon, May 15, 2017 at 05:17:34PM -0700, David Daney wrote:
> > > With the recent improvements in arm64 and vfio-pci, we are seeing
> > > failures like this (on cn8890 based systems):
> > > 
> > > [  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
> > > [  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
> > > .
> > > .
> > > .
> > > [  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
> > > [  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
> > > [  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
> > > [  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
> > > [  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
> > > [  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
> > > [  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
> > > [  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
> > > [  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
> > > [  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
> > > [  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
> > > [  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
> > > [  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
> > > [  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
> > > [  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4
> > > 
> > > These are caused by the inability of the PCIe root port and Intel
> > > e1000e to sucessfully do a bus reset.
> > > 
> > > The proposed fix is to not do a bus reset on these systems.
> > > 
> > > 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.
> > > 
> > >  drivers/pci/pci.c    | 4 ++++
> > >  drivers/pci/quirks.c | 8 ++++++++
> > >  2 files changed, 12 insertions(+)  
> > 
> > Applied with Eric's reviewed-by and typo fixes to pci/virtualization for
> > v4.13, thanks!
> 
> Hmm, well let me again express my concerns that I'm really not sure how
> to support this since it removes our last opportunity to reset devices
> that may otherwise have no reset mechanism.  Certain classes of devices
> are entirely unsupportable for the code path indicated above without a
> bus reset.  If we have an endpoint device that goes bonkers at a bus
> reset, at least we know it's going to behave just as poorly no matter
> what the host platform.  This series allows endpoints that work
> perfectly well on one host to be handled differently on another.  It
> certainly suggests something non-spec compliant about the root port
> implementation and I wish there was more analysis about exactly what
> that problem is since this is coming from the hardware vendor.
> 
> https://lkml.org/lkml/2017/5/16/662

I almost poked you about this on IRC; guess I should have :)

Is it better to leave it as-is, and just take the aborts David
reported?

I agree, it would be nice to know what's really going on.  I assume
Cavium is interested in that as well to make sure future parts don't
have the issue.

Bjorn

Re: [PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[David Daney]

On 05/23/2017 02:04 PM, Alex Williamson wrote:
> On Tue, 23 May 2017 15:47:50 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
>> On Mon, May 15, 2017 at 05:17:34PM -0700, David Daney wrote:
>>> With the recent improvements in arm64 and vfio-pci, we are seeing
>>> failures like this (on cn8890 based systems):
>>>
>>> [  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
>>> [  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
>>> .
>>> .
>>> .
>>> [  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
>>> [  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
>>> [  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
>>> [  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
>>> [  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
>>> [  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
>>> [  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
>>> [  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
>>> [  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
>>> [  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
>>> [  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
>>> [  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
>>> [  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
>>> [  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
>>> [  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4
>>>
>>> These are caused by the inability of the PCIe root port and Intel
>>> e1000e to sucessfully do a bus reset.
>>>
>>> The proposed fix is to not do a bus reset on these systems.
>>>
>>> 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.
>>>
>>>   drivers/pci/pci.c    | 4 ++++
>>>   drivers/pci/quirks.c | 8 ++++++++
>>>   2 files changed, 12 insertions(+)
>>
>> Applied with Eric's reviewed-by and typo fixes to pci/virtualization for
>> v4.13, thanks!
> 
> Hmm, well let me again express my concerns that I'm really not sure how
> to support this since it removes our last opportunity to reset devices
> that may otherwise have no reset mechanism.  Certain classes of devices
> are entirely unsupportable for the code path indicated above without a
> bus reset.  If we have an endpoint device that goes bonkers at a bus
> reset, at least we know it's going to behave just as poorly no matter
> what the host platform.  This series allows endpoints that work
> perfectly well on one host to be handled differently on another.

Yes that is correct.  We choose not to crash the system.  I'm not sure 
what you are suggesting as an alternative.

If a PCI device doesn't work with vfio-pci in such a system, my 
suggestion would be not to use vfio-pci with the device in that system.

>  It
> certainly suggests something non-spec compliant about the root port
> implementation and I wish there was more analysis about exactly what
> that problem is since this is coming from the hardware vendor.

There are two main possibilities here:

1) Some (but not all) Intel e1000e and LSI HBA devices are non-spec 
compliant.

2) Cavium root port is non-spec compliant.

If #1 turns out to be true, would you suggest blacklisting e1000e on all 
systems, including Intel based servers?

If #2 turns out to be true would you still object to the patch?



> 
> https://lkml.org/lkml/2017/5/16/662
> 
> Thanks,
> Alex
> 

Re: [PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[Alex Williamson]

On Tue, 23 May 2017 14:22:01 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 05/23/2017 02:04 PM, Alex Williamson wrote:
> > On Tue, 23 May 2017 15:47:50 -0500
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >   
> >> On Mon, May 15, 2017 at 05:17:34PM -0700, David Daney wrote:  
> >>> With the recent improvements in arm64 and vfio-pci, we are seeing
> >>> failures like this (on cn8890 based systems):
> >>>
> >>> [  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
> >>> [  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
> >>> .
> >>> .
> >>> .
> >>> [  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
> >>> [  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
> >>> [  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
> >>> [  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
> >>> [  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
> >>> [  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
> >>> [  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
> >>> [  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
> >>> [  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
> >>> [  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
> >>> [  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
> >>> [  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
> >>> [  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
> >>> [  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
> >>> [  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4
> >>>
> >>> These are caused by the inability of the PCIe root port and Intel
> >>> e1000e to sucessfully do a bus reset.
> >>>
> >>> The proposed fix is to not do a bus reset on these systems.
> >>>
> >>> 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.
> >>>
> >>>   drivers/pci/pci.c    | 4 ++++
> >>>   drivers/pci/quirks.c | 8 ++++++++
> >>>   2 files changed, 12 insertions(+)  
> >>
> >> Applied with Eric's reviewed-by and typo fixes to pci/virtualization for
> >> v4.13, thanks!  
> > 
> > Hmm, well let me again express my concerns that I'm really not sure how
> > to support this since it removes our last opportunity to reset devices
> > that may otherwise have no reset mechanism.  Certain classes of devices
> > are entirely unsupportable for the code path indicated above without a
> > bus reset.  If we have an endpoint device that goes bonkers at a bus
> > reset, at least we know it's going to behave just as poorly no matter
> > what the host platform.  This series allows endpoints that work
> > perfectly well on one host to be handled differently on another.  
> 
> Yes that is correct.  We choose not to crash the system.  I'm not sure 
> what you are suggesting as an alternative.

It's a valid solution, but my concern is that it's not without
consequences and I didn't get the impression that was fully recognized,
so I wonder if there are better options.

> If a PCI device doesn't work with vfio-pci in such a system, my 
> suggestion would be not to use vfio-pci with the device in that system.

And I can use this as the official support statement from Cavium when
that occurs?
 
> >  It
> > certainly suggests something non-spec compliant about the root port
> > implementation and I wish there was more analysis about exactly what
> > that problem is since this is coming from the hardware vendor.  
> 
> There are two main possibilities here:
> 
> 1) Some (but not all) Intel e1000e and LSI HBA devices are non-spec 
> compliant.
> 
> 2) Cavium root port is non-spec compliant.
> 
> If #1 turns out to be true, would you suggest blacklisting e1000e on all 
> systems, including Intel based servers?

Since we haven't heard anything to the contrary, I think we can also
assume that AMD root ports are compatible.
 
> If #2 turns out to be true would you still object to the patch?

My hope would be that such analysis would help us understand what's
really happening and perhaps present a less drastic workaround.  Is the
point at which we crash simply the first read from the device?  Is the
link state shown as stable at this point?  Is there any chance that we
have a timing issue and an additional delay could resolve it?  Is there
some manipulation of the link state before or after the bus reset that
produces different results?  If such options have been exhausted, then
I guess I have no objection, or at least no better solution, and I'll
keep an eye out for any fallout.  Thanks,

Alex

Re: [PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[Jon Masters]

Following up on this thread...

On 05/23/2017 06:15 PM, Alex Williamson wrote:
> On Tue, 23 May 2017 14:22:01 -0700
> David Daney <ddaney@caviumnetworks.com> wrote:
> 
>> On 05/23/2017 02:04 PM, Alex Williamson wrote:
>>> On Tue, 23 May 2017 15:47:50 -0500
>>> Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>   
>>>> On Mon, May 15, 2017 at 05:17:34PM -0700, David Daney wrote:  
>>>>> With the recent improvements in arm64 and vfio-pci, we are seeing
>>>>> failures like this (on cn8890 based systems):
>>>>>
>>>>> [  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
>>>>> [  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
>>>>> .
>>>>> .
>>>>> .
>>>>> [  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
>>>>> [  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
>>>>> [  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
>>>>> [  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
>>>>> [  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
>>>>> [  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
>>>>> [  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
>>>>> [  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
>>>>> [  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
>>>>> [  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
>>>>> [  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
>>>>> [  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
>>>>> [  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
>>>>> [  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
>>>>> [  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4
>>>>>
>>>>> These are caused by the inability of the PCIe root port and Intel
>>>>> e1000e to sucessfully do a bus reset.

Right now, we have a whole bunch of systems in our labs and across the
industry where people are testing VFIO on Cavium ThunderX platforms. It
is amazing how many people have e1000 cards lying around (this is even
more popular on ARM servers because things like Tianocore usually "just
work" with an e1000 card installed...). I know I have many more e1000s
than I do x86 systems. So we need a solution to not crash on use.

>>>>> The proposed fix is to not do a bus reset on these systems.

It's not the best solution, but it's a solution, and there are plenty of
other quirks. I would like to see us figure out a sensible path to
perhaps take this patch now and continue researching to determine
whether there's an even better option later.

The reasons I say this are:

1). It will take some time for various teams with protocol analyzers to
determine which end is not fully spec compliant, while in the interim
everyone has to carry this patch or some other nasty hack anyway.

2). I have an even worse patch in my test kernels (just disable reset
for every device, which is nasty) and I'm sure I'm not alone. It would
be better (IMO) to confine the lack of reset to a subset of devices.

<snip>

> My hope would be that such analysis would help us understand what's
> really happening and perhaps present a less drastic workaround.  Is the
> point at which we crash simply the first read from the device?  Is the
> link state shown as stable at this point?  Is there any chance that we
> have a timing issue and an additional delay could resolve it?  Is there
> some manipulation of the link state before or after the bus reset that
> produces different results?  If such options have been exhausted, then
> I guess I have no objection, or at least no better solution, and I'll
> keep an eye out for any fallout.  Thanks,

My understanding is that various teams have already spent man months of
time with protocol analyzer equipment trying to figure out who is at
"fault". I have no reason not to believe that they don't intend to
continue that work, since they certainly want to ensure future root port
implementations are event more robust in the presence of both fully
compliant, and also somewhat compliant endpoints. If David affirms that
the work will continue, can we take this for now?

Jon.

Re: [PATCH 0/2] PCI: Workaround for bus reset on Cavium cn8xxx root ports

[Alex Williamson]

On Mon, 29 May 2017 23:30:52 -0400
Jon Masters <jcm@jonmasters.org> wrote:

> Following up on this thread...
> 
> On 05/23/2017 06:15 PM, Alex Williamson wrote:
> > On Tue, 23 May 2017 14:22:01 -0700
> > David Daney <ddaney@caviumnetworks.com> wrote:
> >   
> >> On 05/23/2017 02:04 PM, Alex Williamson wrote:  
> >>> On Tue, 23 May 2017 15:47:50 -0500
> >>> Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>>     
> >>>> On Mon, May 15, 2017 at 05:17:34PM -0700, David Daney wrote:    
> >>>>> With the recent improvements in arm64 and vfio-pci, we are seeing
> >>>>> failures like this (on cn8890 based systems):
> >>>>>
> >>>>> [  235.622361] Unhandled fault: synchronous external abort (0x96000210) at 0xfffffc00c1000100
> >>>>> [  235.630625] Internal error: : 96000210 [#1] PREEMPT SMP
> >>>>> .
> >>>>> .
> >>>>> .
> >>>>> [  236.208820] [<fffffc0008411250>] pci_generic_config_read+0x38/0x9c
> >>>>> [  236.214992] [<fffffc0008435ed4>] thunder_pem_config_read+0x54/0x1e8
> >>>>> [  236.221250] [<fffffc0008411620>] pci_bus_read_config_dword+0x74/0xa0
> >>>>> [  236.227596] [<fffffc000841853c>] pci_find_next_ext_capability.part.15+0x40/0xb8
> >>>>> [  236.234896] [<fffffc0008419428>] pci_find_ext_capability+0x20/0x30
> >>>>> [  236.241068] [<fffffc0008423e2c>] pci_restore_vc_state+0x34/0x88
> >>>>> [  236.246979] [<fffffc000841af3c>] pci_restore_state.part.37+0x2c/0x1fc
> >>>>> [  236.253410] [<fffffc000841b174>] pci_dev_restore+0x4c/0x50
> >>>>> [  236.258887] [<fffffc000841b19c>] pci_bus_restore+0x24/0x4c
> >>>>> [  236.264362] [<fffffc000841c2dc>] pci_try_reset_bus+0x7c/0xa0
> >>>>> [  236.270021] [<fffffc00060a1ab0>] vfio_pci_ioctl+0xc34/0xc3c [vfio_pci]
> >>>>> [  236.276547] [<fffffc0005eb0410>] vfio_device_fops_unl_ioctl+0x20/0x30 [vfio]
> >>>>> [  236.283587] [<fffffc000824b314>] do_vfs_ioctl+0xac/0x744
> >>>>> [  236.288890] [<fffffc000824ba30>] SyS_ioctl+0x84/0x98
> >>>>> [  236.293846] [<fffffc0008082ca0>] __sys_trace_return+0x0/0x4
> >>>>>
> >>>>> These are caused by the inability of the PCIe root port and Intel
> >>>>> e1000e to sucessfully do a bus reset.  
> 
> Right now, we have a whole bunch of systems in our labs and across the
> industry where people are testing VFIO on Cavium ThunderX platforms. It
> is amazing how many people have e1000 cards lying around (this is even
> more popular on ARM servers because things like Tianocore usually "just
> work" with an e1000 card installed...). I know I have many more e1000s
> than I do x86 systems. So we need a solution to not crash on use.
> 
> >>>>> The proposed fix is to not do a bus reset on these systems.  
> 
> It's not the best solution, but it's a solution, and there are plenty of
> other quirks. I would like to see us figure out a sensible path to
> perhaps take this patch now and continue researching to determine
> whether there's an even better option later.
> 
> The reasons I say this are:
> 
> 1). It will take some time for various teams with protocol analyzers to
> determine which end is not fully spec compliant, while in the interim
> everyone has to carry this patch or some other nasty hack anyway.
> 
> 2). I have an even worse patch in my test kernels (just disable reset
> for every device, which is nasty) and I'm sure I'm not alone. It would
> be better (IMO) to confine the lack of reset to a subset of devices.
> 
> <snip>
> 
> > My hope would be that such analysis would help us understand what's
> > really happening and perhaps present a less drastic workaround.  Is the
> > point at which we crash simply the first read from the device?  Is the
> > link state shown as stable at this point?  Is there any chance that we
> > have a timing issue and an additional delay could resolve it?  Is there
> > some manipulation of the link state before or after the bus reset that
> > produces different results?  If such options have been exhausted, then
> > I guess I have no objection, or at least no better solution, and I'll
> > keep an eye out for any fallout.  Thanks,  
> 
> My understanding is that various teams have already spent man months of
> time with protocol analyzer equipment trying to figure out who is at
> "fault". I have no reason not to believe that they don't intend to
> continue that work, since they certainly want to ensure future root port
> implementations are event more robust in the presence of both fully
> compliant, and also somewhat compliant endpoints. If David affirms that
> the work will continue, can we take this for now?

I'd feel slightly more inclined to agree on that course if we gave
hardware vendors some incentive to resolve the problem and users some
indication that a problem exists, by adding a dev_warn when we apply
PCI_DEV_FLAGS_NO_BUS_RESET to the device.  I'd also entertain the idea
of only printing a warning when an affected devices is opened by the
user through vfio.  Thanks,

Alex