| Message ID | 20210112153643.17930-1-antti.jarvinen@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: quirk for preventing bus reset on TI C667X | expand |
[+cc Alex, Murali, Kishon] On Tue, Jan 12, 2021 at 03:36:43PM +0000, Antti Järvinen wrote: > TI C667X does not support bus/hot reset. > See https://e2e.ti.com/support/processors/f/791/t/954382 You can cite the URL as the source, but the URL will eventually become stale, so let's include the relevant details here directly. From the forum, it looks like the device doesn't respond after a reset (config accesses return ~0). It seems somewhat surprising that something as basic as a reset would be completely broken. I wonder if we're not doing the reset correctly. It looks like we would probably be trying a Secondary Bus Reset using the bridge leading to the C667X. Can you confirm? Wonder if you could try doing what pci_reset_secondary_bus() does by hand: # BRIDGE=... # PCI address, e.g., 00:1c.0 # C667X=... # setpci -s$C667X VENDOR_ID.w # setpci -s$BRIDGE BRIDGE_CONTROL.w # prints "val" # setpci -s$BRIDGE BRIDGE_CONTROL.w= # val | 0x40 (set SBR) # sleep 1 # setpci -s$BRIDGE BRIDGE_CONTROL.w= # val (clear SBR) # sleep 1 # setpci -s$C667X VENDOR_ID.w=0 # setpci -s$C667X VENDOR_ID.w If we use this quirk and avoid the reset, I assume that means assigning the device to VMs with VFIO will leak state between VMs? > Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com> > --- > drivers/pci/quirks.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 653660e3ba9e..c8fcf24c5bd0 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -3578,6 +3578,12 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset); > */ > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset); > > +/* > + * Some TI keystone C667X devices do no support bus/hot reset. > + * https://e2e.ti.com/support/processors/f/791/t/954382 > + */ > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset); > + > static void quirk_no_pm_reset(struct pci_dev *dev) > { > /* > -- > 2.17.1 >
On 22.1.2021 1.55, Bjorn Helgaas wrote:> > It looks like we would probably be trying a Secondary Bus Reset using > the bridge leading to the C667X. Can you confirm? Yes, this is my understanding too. > Wonder if you > could try doing what pci_reset_secondary_bus() does by hand: > I tried this by hand. It looks that result is same as through VFIO. # cat sbr.sh BRIDGE=10:00.0 C667X=11:00.0 setpci -s$C667X VENDOR_ID.w VAL=$(setpci -s$BRIDGE BRIDGE_CONTROL.w) echo $VAL setpci -s$BRIDGE BRIDGE_CONTROL.w=$(($VAL | 0x40)) sleep 1 setpci -s$BRIDGE BRIDGE_CONTROL.w=$VAL sleep 1 setpci -s$C667X VENDOR_ID.w=0 setpci -s$C667X VENDOR_ID.w # ./sbr.sh 104c 0003 ffff > # BRIDGE=... # PCI address, e.g., 00:1c.0 > # C667X=... > # setpci -s$C667X VENDOR_ID.w > # setpci -s$BRIDGE BRIDGE_CONTROL.w # prints "val" > # setpci -s$BRIDGE BRIDGE_CONTROL.w= # val | 0x40 (set SBR) > # sleep 1 > # setpci -s$BRIDGE BRIDGE_CONTROL.w= # val (clear SBR) > # sleep 1 > # setpci -s$C667X VENDOR_ID.w=0 > # setpci -s$C667X VENDOR_ID.w > > If we use this quirk and avoid the reset, I assume that means > assigning the device to VMs with VFIO will leak state between VMs? I think this is true.
On Tue, Jan 26, 2021 at 01:22:18PM +0200, Antti Järvinen wrote: > On 22.1.2021 1.55, Bjorn Helgaas wrote:> > > > It looks like we would probably be trying a Secondary Bus Reset using > > the bridge leading to the C667X. Can you confirm? > > Yes, this is my understanding too. > > > Wonder if you > > could try doing what pci_reset_secondary_bus() does by hand: > > > > I tried this by hand. It looks that result is same as through VFIO. > > # cat sbr.sh > BRIDGE=10:00.0 > C667X=11:00.0 > > setpci -s$C667X VENDOR_ID.w > > VAL=$(setpci -s$BRIDGE BRIDGE_CONTROL.w) > echo $VAL > setpci -s$BRIDGE BRIDGE_CONTROL.w=$(($VAL | 0x40)) > sleep 1 > setpci -s$BRIDGE BRIDGE_CONTROL.w=$VAL > sleep 1 > setpci -s$C667X VENDOR_ID.w=0 > setpci -s$C667X VENDOR_ID.w > > > # ./sbr.sh > 104c > 0003 > ffff > > > > # BRIDGE=... # PCI address, e.g., 00:1c.0 > > # C667X=... > > # setpci -s$C667X VENDOR_ID.w > > # setpci -s$BRIDGE BRIDGE_CONTROL.w # prints "val" > > # setpci -s$BRIDGE BRIDGE_CONTROL.w= # val | 0x40 (set SBR) > > # sleep 1 > > # setpci -s$BRIDGE BRIDGE_CONTROL.w= # val (clear SBR) > > # sleep 1 > > # setpci -s$C667X VENDOR_ID.w=0 > > # setpci -s$C667X VENDOR_ID.w > > > > If we use this quirk and avoid the reset, I assume that means > > assigning the device to VMs with VFIO will leak state between VMs? > > I think this is true. Alex, is there some warning about situations like this where a device may leak state between VMs? There's nothing in quirk_no_bus_reset() itself where we set PCI_DEV_FLAGS_NO_BUS_RESET, and nothing in pci_parent_bus_reset() or pci_dev_reset_slot_function() where we test it, but I didn't chase into VFIO. Seems important enough that we might want to mention it at least once and maybe even every time we try to reset the device. I hope the leak isn't completely silent. Bjorn
On Thu, Jan 21, 2021 at 05:55:47PM -0600, Bjorn Helgaas wrote: > On Tue, Jan 12, 2021 at 03:36:43PM +0000, Antti Järvinen wrote: > > TI C667X does not support bus/hot reset. > > See https://e2e.ti.com/support/processors/f/791/t/954382 > > You can cite the URL as the source, but the URL will eventually become > stale, so let's include the relevant details here directly. Thanks for trying the experiment below. I'll look for a repost that includes details from the URL directly in the commit log. > From the forum, it looks like the device doesn't respond after a > reset (config accesses return ~0). It seems somewhat surprising that > something as basic as a reset would be completely broken. I wonder if > we're not doing the reset correctly. > > It looks like we would probably be trying a Secondary Bus Reset using > the bridge leading to the C667X. Can you confirm? Wonder if you > could try doing what pci_reset_secondary_bus() does by hand: > > # BRIDGE=... # PCI address, e.g., 00:1c.0 > # C667X=... > # setpci -s$C667X VENDOR_ID.w > # setpci -s$BRIDGE BRIDGE_CONTROL.w # prints "val" > # setpci -s$BRIDGE BRIDGE_CONTROL.w= # val | 0x40 (set SBR) > # sleep 1 > # setpci -s$BRIDGE BRIDGE_CONTROL.w= # val (clear SBR) > # sleep 1 > # setpci -s$C667X VENDOR_ID.w=0 > # setpci -s$C667X VENDOR_ID.w > > If we use this quirk and avoid the reset, I assume that means > assigning the device to VMs with VFIO will leak state between VMs? > > > Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com> > > --- > > drivers/pci/quirks.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index 653660e3ba9e..c8fcf24c5bd0 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -3578,6 +3578,12 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset); > > */ > > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset); > > > > +/* > > + * Some TI keystone C667X devices do no support bus/hot reset. > > + * https://e2e.ti.com/support/processors/f/791/t/954382 > > + */ > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset); > > + > > static void quirk_no_pm_reset(struct pci_dev *dev) > > { > > /* > > -- > > 2.17.1 > >
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 653660e3ba9e..c8fcf24c5bd0 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3578,6 +3578,12 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset); */ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset); +/* + * Some TI keystone C667X devices do no support bus/hot reset. + * https://e2e.ti.com/support/processors/f/791/t/954382 + */ +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset); + static void quirk_no_pm_reset(struct pci_dev *dev) { /*
TI C667X does not support bus/hot reset. See https://e2e.ti.com/support/processors/f/791/t/954382 Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com> --- drivers/pci/quirks.c | 6 ++++++ 1 file changed, 6 insertions(+)