[+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 <[email protected]>
> ---
> 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 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 <[email protected]>
> > ---
> > 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
> >
Some TI keystone C667X devices do no support bus/hot reset. Its PCIESS
automatically disables LTSSM when secondary bus reset is received and
device stops working. Prevent bus reset by adding quirk_no_bus_reset to
the device. With this change device can be assigned to VMs with VFIO,
but it will leak state between VMs.
Reference https://e2e.ti.com/support/processors/f/791/t/954382
Signed-off-by: Antti Järvinen <[email protected]>
---
drivers/pci/quirks.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..d9201ad1ca39 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3578,6 +3578,16 @@ 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.
+ * Its PCIESS automatically disables LTSSM when secondary bus reset is
+ * received and device stops working. Prevent bus reset by adding
+ * quirk_no_bus_reset to the device. With this change device can be
+ * assigned to VMs with VFIO, but it will leak state between VMs.
+ * Reference 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
Hi Antti,
A few nitpicks, so feel free to ignore these, of course.
If possible, capitalise the subject line. Also, perhaps "Add quirk to
prevent bus (...)" might read better.
> Some TI keystone C667X devices do no support bus/hot reset. Its PCIESS
[...]
It would be KeyStone in the above sentence.
[...]
> With this change device can be assigned to VMs with VFIO, but it will
> leak state between VMs.
Following-up on Bojrn's question about the state leak, see:
https://lore.kernel.org/linux-pci/20210129234946.GA124037@bjorn-Precision-5520/
Would there be anything else that has to be done?
Krzysztof
Some TI KeyStone C667X devices do no support bus/hot reset. Its PCIESS
automatically disables LTSSM when secondary bus reset is received and
device stops working. Prevent bus reset by adding quirk_no_bus_reset to
the device. With this change device can be assigned to VMs with VFIO,
but it will leak state between VMs.
Reference: https://e2e.ti.com/support/processors/f/791/t/954382
Signed-off-by: Antti Järvinen <[email protected]>
---
drivers/pci/quirks.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..d9201ad1ca39 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3578,6 +3578,16 @@ 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.
+ * Its PCIESS automatically disables LTSSM when secondary bus reset is
+ * received and device stops working. Prevent bus reset by adding
+ * quirk_no_bus_reset to the device. With this change device can be
+ * assigned to VMs with VFIO, but it will leak state between VMs.
+ * Reference 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 7.3.2021 2.22, Krzysztof Wilczyński wrote:
> Hi Antti,
>
> A few nitpicks, so feel free to ignore these, of course.
>
> If possible, capitalise the subject line. Also, perhaps "Add quirk to
> prevent bus (...)" might read better.
>
>> Some TI keystone C667X devices do no support bus/hot reset. Its PCIESS
> [...]
>
> It would be KeyStone in the above sentence.
>
> [...]
>> With this change device can be assigned to VMs with VFIO, but it will
>> leak state between VMs.
>
> Following-up on Bojrn's question about the state leak, see:
> https://lore.kernel.org/linux-pci/20210129234946.GA124037@bjorn-Precision-5520/
>
> Would there be anything else that has to be done?
>
To my understanding this is all that needs to be done. At least on other similar
case, adding quirk was the only change https://lore.kernel.org/patchwork/patch/1086083/
On Mon, Mar 08, 2021 at 02:21:30PM +0000, Antti J?rvinen wrote:
> Some TI KeyStone C667X devices do no support bus/hot reset. Its PCIESS
> automatically disables LTSSM when secondary bus reset is received and
> device stops working. Prevent bus reset by adding quirk_no_bus_reset to
> the device. With this change device can be assigned to VMs with VFIO,
> but it will leak state between VMs.
s/do no/do/not/ (also in the comment below)
Does the user get any indication of this leaking state? I looked
through drivers/vfio and drivers/pci, but I haven't found anything
yet.
We *could* log something in quirk_no_bus_reset(), but that would just
be noise for people who don't pass the device through to a VM. So
maybe it would be nicer if we logged something when we actually *do*
pass it through to a VM.
> Reference: https://e2e.ti.com/support/processors/f/791/t/954382
> Signed-off-by: Antti J?rvinen <[email protected]>
> ---
> drivers/pci/quirks.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..d9201ad1ca39 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3578,6 +3578,16 @@ 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.
> + * Its PCIESS automatically disables LTSSM when secondary bus reset is
> + * received and device stops working. Prevent bus reset by adding
> + * quirk_no_bus_reset to the device. With this change device can be
> + * assigned to VMs with VFIO, but it will leak state between VMs.
> + * Reference 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
>
Some TI KeyStone C667X devices do not support bus/hot reset. Its PCIESS
automatically disables LTSSM when secondary bus reset is received and
device stops working. Prevent bus reset by adding quirk_no_bus_reset to
the device. With this change device can be assigned to VMs with VFIO,
but it will leak state between VMs.
Reference: https://e2e.ti.com/support/processors/f/791/t/954382
Signed-off-by: Antti Järvinen <[email protected]>
---
drivers/pci/quirks.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..d9201ad1ca39 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3578,6 +3578,16 @@ 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.
+ * Its PCIESS automatically disables LTSSM when secondary bus reset is
+ * received and device stops working. Prevent bus reset by adding
+ * quirk_no_bus_reset to the device. With this change device can be
+ * assigned to VMs with VFIO, but it will leak state between VMs.
+ * Reference 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 12.3.2021 23.09, Bjorn Helgaas wrote:
> On Mon, Mar 08, 2021 at 02:21:30PM +0000, Antti Järvinen wrote:
>> Some TI KeyStone C667X devices do no support bus/hot reset. Its PCIESS
>> automatically disables LTSSM when secondary bus reset is received and
>> device stops working. Prevent bus reset by adding quirk_no_bus_reset to
>> the device. With this change device can be assigned to VMs with VFIO,
>> but it will leak state between VMs.
>
> s/do no/do/not/ (also in the comment below)
>
Should be fixed in v4 patch.
> Does the user get any indication of this leaking state? I looked
> through drivers/vfio and drivers/pci, but I haven't found anything
> yet.
>
I haven't seen any indication too.
Overall I think all devices having this quirk will leak state, as they
don't get resetted.
> We *could* log something in quirk_no_bus_reset(), but that would just
> be noise for people who don't pass the device through to a VM. So
> maybe it would be nicer if we logged something when we actually *do*
> pass it through to a VM.
>
>> Reference: https://e2e.ti.com/support/processors/f/791/t/954382
>> Signed-off-by: Antti Järvinen <[email protected]>
>> ---
>> drivers/pci/quirks.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 653660e3ba9e..d9201ad1ca39 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3578,6 +3578,16 @@ 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.
>> + * Its PCIESS automatically disables LTSSM when secondary bus reset is
>> + * received and device stops working. Prevent bus reset by adding
>> + * quirk_no_bus_reset to the device. With this change device can be
>> + * assigned to VMs with VFIO, but it will leak state between VMs.
>> + * Reference 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 15/03/21 3:56 pm, Antti Järvinen wrote:
> Some TI KeyStone C667X devices do not support bus/hot reset. Its PCIESS
> automatically disables LTSSM when secondary bus reset is received and
> device stops working. Prevent bus reset by adding quirk_no_bus_reset to
> the device. With this change device can be assigned to VMs with VFIO,
> but it will leak state between VMs.
>
> Reference: https://e2e.ti.com/support/processors/f/791/t/954382
> Signed-off-by: Antti Järvinen <[email protected]>
Reviewed-by: Kishon Vijay Abraham I <[email protected]>
> ---
> drivers/pci/quirks.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..d9201ad1ca39 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3578,6 +3578,16 @@ 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.
> + * Its PCIESS automatically disables LTSSM when secondary bus reset is
> + * received and device stops working. Prevent bus reset by adding
> + * quirk_no_bus_reset to the device. With this change device can be
> + * assigned to VMs with VFIO, but it will leak state between VMs.
> + * Reference 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)
> {
> /*
>
On Mon, Mar 15, 2021 at 10:26:06AM +0000, Antti J?rvinen wrote:
> Some TI KeyStone C667X devices do not support bus/hot reset. Its PCIESS
> automatically disables LTSSM when secondary bus reset is received and
> device stops working. Prevent bus reset by adding quirk_no_bus_reset to
> the device. With this change device can be assigned to VMs with VFIO,
> but it will leak state between VMs.
>
> Reference: https://e2e.ti.com/support/processors/f/791/t/954382
> Signed-off-by: Antti J?rvinen <[email protected]>
Applied to pci/virtualization for v5.14 with subject
PCI: Mark TI C667X to avoid bus reset
Thanks!
> ---
> drivers/pci/quirks.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..d9201ad1ca39 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3578,6 +3578,16 @@ 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.
> + * Its PCIESS automatically disables LTSSM when secondary bus reset is
> + * received and device stops working. Prevent bus reset by adding
> + * quirk_no_bus_reset to the device. With this change device can be
> + * assigned to VMs with VFIO, but it will leak state between VMs.
> + * Reference 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
>