diff mbox series

PCI: s390: Handle ARI on bus without associated struct pci_dev

Message ID 20240730-ari_no_bus_dev-v1-1-7de17676f9fe@linux.ibm.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI: s390: Handle ARI on bus without associated struct pci_dev | expand

Commit Message

Niklas Schnelle July 30, 2024, 7:36 p.m. UTC 
On s390 PCI busses are virtualized and the downstream ports are
invisible to the OS and struct pci_bus::self is NULL. This associated
struct pci_dev is however relied upon in pci_ari_enabled() to check
whether ARI is enabled for the bus. ARI is therefor always detected as
disabled.

At the same time firmware on s390 always enables and relies upon ARI
thus causing a mismatch. Moreover with per-PCI function pass-through
there may exist busses with no function with devfn 0. For example
a SR-IOV capable device with two PFs may have separate function
dependency link chains for each of the PFs and their child VFs. In this
case the OS may only see the second PF and its child VFs on a bus
without a devfn 0 function. A situation which is also not supported by
the common pci_configure_ari() code.

Dispite simply being a mismatch this causes problems as some PCI devices
present a different SR-IOV topology depending on PCI_SRIOV_CTRL_ARI.

A similar mismatch may occur with SR-IOV when virtfn_add_bus() creates new
busses with no associated struct pci_dev. Here too pci_ari_enabled()
on these busses would return false even if ARI is actually used.

Prevent both mismatches by moving the ari_enabled flag from struct
pci_dev to struct pci_bus making it independent from struct pci_bus::
self. Let the bus inherit the ari_enabled state from its parent bus when
there is no bridge device such that busses added by virtfn_add_bus()
match their parent. For s390 set ari_enabled when the device supports
ARI in the awareness that all PCIe ports on s390 systems are ARI
capable.

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/s390/pci/pci_bus.c | 12 ++++++++++++
 drivers/pci/pci.c       |  4 ++--
 drivers/pci/probe.c     |  1 +
 include/linux/pci.h     |  4 ++--
 4 files changed, 17 insertions(+), 4 deletions(-)


---
base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
change-id: 20240724-ari_no_bus_dev-52b2a27f3466

Best regards,

Comments

Niklas Schnelle July 30, 2024, 7:59 p.m. UTC | #1 
On Tue, 2024-07-30 at 21:36 +0200, Niklas Schnelle wrote:
> On s390 PCI busses are virtualized and the downstream ports are
> invisible to the OS and struct pci_bus::self is NULL. This associated
> struct pci_dev is however relied upon in pci_ari_enabled() to check
> whether ARI is enabled for the bus. ARI is therefor always detected as
> disabled.
> 
> At the same time firmware on s390 always enables and relies upon ARI
> thus causing a mismatch. Moreover with per-PCI function pass-through
> there may exist busses with no function with devfn 0. For example
> a SR-IOV capable device with two PFs may have separate function
> dependency link chains for each of the PFs and their child VFs. In this
> case the OS may only see the second PF and its child VFs on a bus
> without a devfn 0 function. A situation which is also not supported by
> the common pci_configure_ari() code.
> 
> Dispite simply being a mismatch this causes problems as some PCI devices
> present a different SR-IOV topology depending on PCI_SRIOV_CTRL_ARI.
> 
> A similar mismatch may occur with SR-IOV when virtfn_add_bus() creates new
> busses with no associated struct pci_dev. Here too pci_ari_enabled()
> on these busses would return false even if ARI is actually used.
> 
> Prevent both mismatches by moving the ari_enabled flag from struct
> pci_dev to struct pci_bus making it independent from struct pci_bus::
> self. Let the bus inherit the ari_enabled state from its parent bus when
> there is no bridge device such that busses added by virtfn_add_bus()
> match their parent. For s390 set ari_enabled when the device supports
> ARI in the awareness that all PCIe ports on s390 systems are ARI
> capable.
> 
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  arch/s390/pci/pci_bus.c | 12 ++++++++++++
>  drivers/pci/pci.c       |  4 ++--
>  drivers/pci/probe.c     |  1 +
>  include/linux/pci.h     |  4 ++--
>  4 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
> index daa5d7450c7d..021319438dad 100644
> --- a/arch/s390/pci/pci_bus.c
> +++ b/arch/s390/pci/pci_bus.c
> @@ -278,6 +278,18 @@ void pcibios_bus_add_device(struct pci_dev *pdev)
>  {
>  	struct zpci_dev *zdev = to_zpci(pdev);
>  
> +	/*
> +	 * On s390 PCI busses are virtualized and the bridge
> +	 * devices are invisible to the OS. Furthermore busses
> +	 * may exist without a devfn 0 function. Thus the normal
> +	 * ARI detection does not work. At the same time fw/hw
> +	 * has always enabled ARI when possible. Reflect the actual
> +	 * state by setting ari_enabled whenever a device on the bus
> +	 * supports it.
> +	 */
> +	if (pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ARI))
> +		zdev->zbus->bus->ari_enabled = 1;
> +

@Bjorn unstead of adding the above code to s390 specific code an
alternative I considered would be to modify pci_configure_ari() like
below. I tested this as well but wasn't sure if it is too much churn
especially the handling of the dev->devfn != 0 case. Then again it
might be nice to have this in common code.

@@ -3523,12 +3524,18 @@ void pci_configure_ari(struct pci_dev *dev)
        u32 cap;
        struct pci_dev *bridge;

-       if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
+       if (pcie_ari_disabled || !pci_is_pcie(dev))
+               return;
+
+       if (dev->devfn && !hypervisor_isolated_pci_functions())
                return;

        bridge = dev->bus->self;
-       if (!bridge)
+       if (!bridge) {
+               if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
+                       dev->bus->ari_enabled = 1;
                return;
+       }

        pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
        if (!(cap & PCI_EXP_DEVCAP2_ARI))
Bjorn Helgaas Aug. 1, 2024, 4:59 p.m. UTC | #2 
On Tue, Jul 30, 2024 at 09:59:13PM +0200, Niklas Schnelle wrote:
> On Tue, 2024-07-30 at 21:36 +0200, Niklas Schnelle wrote:
> > On s390 PCI busses are virtualized and the downstream ports are
> > invisible to the OS and struct pci_bus::self is NULL. This associated
> > struct pci_dev is however relied upon in pci_ari_enabled() to check
> > whether ARI is enabled for the bus. ARI is therefor always detected as
> > disabled.
> > 
> > At the same time firmware on s390 always enables and relies upon ARI
> > thus causing a mismatch. Moreover with per-PCI function pass-through
> > there may exist busses with no function with devfn 0. For example
> > a SR-IOV capable device with two PFs may have separate function
> > dependency link chains for each of the PFs and their child VFs. In this
> > case the OS may only see the second PF and its child VFs on a bus
> > without a devfn 0 function. A situation which is also not supported by
> > the common pci_configure_ari() code.
> > 
> > Dispite simply being a mismatch this causes problems as some PCI devices
> > present a different SR-IOV topology depending on PCI_SRIOV_CTRL_ARI.
> > 
> > A similar mismatch may occur with SR-IOV when virtfn_add_bus() creates new
> > busses with no associated struct pci_dev. Here too pci_ari_enabled()
> > on these busses would return false even if ARI is actually used.
> > 
> > Prevent both mismatches by moving the ari_enabled flag from struct
> > pci_dev to struct pci_bus making it independent from struct pci_bus::
> > self. Let the bus inherit the ari_enabled state from its parent bus when
> > there is no bridge device such that busses added by virtfn_add_bus()
> > match their parent. For s390 set ari_enabled when the device supports
> > ARI in the awareness that all PCIe ports on s390 systems are ARI
> > capable.
> > 
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >  arch/s390/pci/pci_bus.c | 12 ++++++++++++
> >  drivers/pci/pci.c       |  4 ++--
> >  drivers/pci/probe.c     |  1 +
> >  include/linux/pci.h     |  4 ++--
> >  4 files changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
> > index daa5d7450c7d..021319438dad 100644
> > --- a/arch/s390/pci/pci_bus.c
> > +++ b/arch/s390/pci/pci_bus.c
> > @@ -278,6 +278,18 @@ void pcibios_bus_add_device(struct pci_dev *pdev)
> >  {
> >  	struct zpci_dev *zdev = to_zpci(pdev);
> >  
> > +	/*
> > +	 * On s390 PCI busses are virtualized and the bridge
> > +	 * devices are invisible to the OS. Furthermore busses
> > +	 * may exist without a devfn 0 function. Thus the normal
> > +	 * ARI detection does not work. At the same time fw/hw
> > +	 * has always enabled ARI when possible. Reflect the actual
> > +	 * state by setting ari_enabled whenever a device on the bus
> > +	 * supports it.
> > +	 */
> > +	if (pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ARI))
> > +		zdev->zbus->bus->ari_enabled = 1;
> > +
> 
> @Bjorn unstead of adding the above code to s390 specific code an
> alternative I considered would be to modify pci_configure_ari() like
> below. I tested this as well but wasn't sure if it is too much churn
> especially the handling of the dev->devfn != 0 case. Then again it
> might be nice to have this in common code.
> 
> @@ -3523,12 +3524,18 @@ void pci_configure_ari(struct pci_dev *dev)
>         u32 cap;
>         struct pci_dev *bridge;
> 
> -       if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
> +       if (pcie_ari_disabled || !pci_is_pcie(dev))
> +               return;
> +
> +       if (dev->devfn && !hypervisor_isolated_pci_functions())
>                 return;
> 
>         bridge = dev->bus->self;
> -       if (!bridge)
> +       if (!bridge) {
> +               if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
> +                       dev->bus->ari_enabled = 1;

In the generic case here, how do we know whether the invisible bridge
leading here has ARI enabled?  If that's known to always be the case
for s390, I understand that, but I don't understand the other cases
(jailhouse, passthrough, etc).

>                 return;
> +       }
> 
>         pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
>         if (!(cap & PCI_EXP_DEVCAP2_ARI))
>
Niklas Schnelle Aug. 5, 2024, 7:14 p.m. UTC | #3 
On Thu, 2024-08-01 at 11:59 -0500, Bjorn Helgaas wrote:
> On Tue, Jul 30, 2024 at 09:59:13PM +0200, Niklas Schnelle wrote:
> > On Tue, 2024-07-30 at 21:36 +0200, Niklas Schnelle wrote:
> > > On s390 PCI busses are virtualized and the downstream ports are
> > > invisible to the OS and struct pci_bus::self is NULL. This associated
> > > struct pci_dev is however relied upon in pci_ari_enabled() to check
> > > whether ARI is enabled for the bus. ARI is therefor always detected as
> > > disabled.
> > > 
> > > At the same time firmware on s390 always enables and relies upon ARI
> > > thus causing a mismatch. Moreover with per-PCI function pass-through
> > > there may exist busses with no function with devfn 0. For example
> > > a SR-IOV capable device with two PFs may have separate function
> > > dependency link chains for each of the PFs and their child VFs. In this
> > > case the OS may only see the second PF and its child VFs on a bus
> > > without a devfn 0 function. A situation which is also not supported by
> > > the common pci_configure_ari() code.
> > > 
> > > Dispite simply being a mismatch this causes problems as some PCI devices
> > > present a different SR-IOV topology depending on PCI_SRIOV_CTRL_ARI.
> > > 
> > > A similar mismatch may occur with SR-IOV when virtfn_add_bus() creates new
> > > busses with no associated struct pci_dev. Here too pci_ari_enabled()
> > > on these busses would return false even if ARI is actually used.
> > > 
> > > Prevent both mismatches by moving the ari_enabled flag from struct
> > > pci_dev to struct pci_bus making it independent from struct pci_bus::
> > > self. Let the bus inherit the ari_enabled state from its parent bus when
> > > there is no bridge device such that busses added by virtfn_add_bus()
> > > match their parent. For s390 set ari_enabled when the device supports
> > > ARI in the awareness that all PCIe ports on s390 systems are ARI
> > > capable.
> > > 
> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > ---
---8<---
> > @@ -3523,12 +3524,18 @@ void pci_configure_ari(struct pci_dev *dev)
> >         u32 cap;
> >         struct pci_dev *bridge;
> > 
> > -       if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
> > +       if (pcie_ari_disabled || !pci_is_pcie(dev))
> > +               return;
> > +
> > +       if (dev->devfn && !hypervisor_isolated_pci_functions())
> >                 return;
> > 
> >         bridge = dev->bus->self;
> > -       if (!bridge)
> > +       if (!bridge) {
> > +               if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
> > +                       dev->bus->ari_enabled = 1;
> 
> In the generic case here, how do we know whether the invisible bridge
> leading here has ARI enabled?  If that's known to always be the case
> for s390, I understand that, but I don't understand the other cases
> (jailhouse, passthrough, etc).

Good point! Yes this is probably not correct if Jailhouse doesn't also
guarantee ARI. I guess if we really want the generic solution, and I'm
fine with an s390 specific one too, then we would need to add some
indication that the invisible bridges support ARI. Honestly I'm not
even entirely sure if the bridge is even NULL on jailhouse too. In
QEMU/KVM for example I think everyone besides s390 emulates bridges.

> 
> >                 return;
> > +       }
> > 
> >         pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
> >         if (!(cap & PCI_EXP_DEVCAP2_ARI))
> > 
>
diff mbox series

Patch

diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
index daa5d7450c7d..021319438dad 100644
--- a/arch/s390/pci/pci_bus.c
+++ b/arch/s390/pci/pci_bus.c
@@ -278,6 +278,18 @@  void pcibios_bus_add_device(struct pci_dev *pdev)
 {
 	struct zpci_dev *zdev = to_zpci(pdev);
 
+	/*
+	 * On s390 PCI busses are virtualized and the bridge
+	 * devices are invisible to the OS. Furthermore busses
+	 * may exist without a devfn 0 function. Thus the normal
+	 * ARI detection does not work. At the same time fw/hw
+	 * has always enabled ARI when possible. Reflect the actual
+	 * state by setting ari_enabled whenever a device on the bus
+	 * supports it.
+	 */
+	if (pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ARI))
+		zdev->zbus->bus->ari_enabled = 1;
+
 	/*
 	 * With pdev->no_vf_scan the common PCI probing code does not
 	 * perform PF/VF linking.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e3a49f66982d..2a362d26e3e5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3537,11 +3537,11 @@  void pci_configure_ari(struct pci_dev *dev)
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI)) {
 		pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
 					 PCI_EXP_DEVCTL2_ARI);
-		bridge->ari_enabled = 1;
+		dev->bus->ari_enabled = 1;
 	} else {
 		pcie_capability_clear_word(bridge, PCI_EXP_DEVCTL2,
 					   PCI_EXP_DEVCTL2_ARI);
-		bridge->ari_enabled = 0;
+		dev->bus->ari_enabled = 0;
 	}
 }
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b14b9876c030..c318929438c1 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1143,6 +1143,7 @@  static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 
 	if (!bridge) {
 		child->dev.parent = parent->bridge;
+		child->ari_enabled = parent->ari_enabled;
 		goto add_dev;
 	}
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4cf89a4b4cbc..3d1f4a392dd6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -429,7 +429,6 @@  struct pci_dev {
 	unsigned int	irq_reroute_variant:2;	/* Needs IRQ rerouting variant */
 	unsigned int	msi_enabled:1;
 	unsigned int	msix_enabled:1;
-	unsigned int	ari_enabled:1;		/* ARI forwarding */
 	unsigned int	ats_enabled:1;		/* Address Translation Svc */
 	unsigned int	pasid_enabled:1;	/* Process Address Space ID */
 	unsigned int	pri_enabled:1;		/* Page Request Interface */
@@ -679,6 +678,7 @@  struct pci_bus {
 	struct bin_attribute	*legacy_mem;	/* Legacy mem */
 	unsigned int		is_added:1;
 	unsigned int		unsafe_warn:1;	/* warned about RW1C config write */
+	unsigned int		ari_enabled:1;	/* ARI forwarding enabled */
 };
 
 #define to_pci_bus(n)	container_of(n, struct pci_bus, dev)
@@ -2637,7 +2637,7 @@  static inline bool pci_is_dev_assigned(struct pci_dev *pdev)
  */
 static inline bool pci_ari_enabled(struct pci_bus *bus)
 {
-	return bus->self && bus->self->ari_enabled;
+	return bus->ari_enabled;
 }
 
 /**