[PATCH v2 0/2] Remove PCIE root bridge LSI on powernv

[PATCH v2 0/2] Remove PCIE root bridge LSI on powernv

[Frederic Barrat]

The powernv8/powernv9/powernv10 machines allocate a LSI for their root
port bridge, which is not the case on real hardware. The default root
port implementation in qemu requests a LSI. Since the powernv
implementation derives from it, that's where the LSI is coming
from. This series fixes it, so that the model matches the hardware.

However, the code in hw/pci to handle AER and hotplug events assume a
LSI is defined. It tends to assert/deassert a LSI if MSI or MSIX is
not enabled. Since we have hardware where that is not true, this patch
also fixes a few code paths to check if a LSI is configured before
trying to trigger it.


Changes from v1:
 - addressed comments from Daniel


Frederic Barrat (2):
  pcie: Don't try triggering a LSI when not defined
  ppc/pnv: Remove LSI on the PCIE host bridge

 hw/pci-host/pnv_phb3.c | 1 +
 hw/pci-host/pnv_phb4.c | 1 +
 hw/pci/pcie.c          | 5 +++--
 hw/pci/pcie_aer.c      | 2 +-
 4 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.35.1

[PATCH v2 1/2] pcie: Don't try triggering a LSI when not defined

[Frederic Barrat]

This patch skips [de]asserting a LSI interrupt if the device doesn't
have any LSI defined. Doing so would trigger an assert in
pci_irq_handler().

The PCIE root port implementation in qemu requests a LSI (INTA), but a
subclass may want to change that behavior since it's a valid
configuration. For example on the POWER8/POWER9/POWER10 systems, the
root bridge doesn't request any LSI.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
 hw/pci/pcie.c     | 5 +++--
 hw/pci/pcie_aer.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 67a5d67372..68a62da0b5 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -353,7 +353,7 @@ static void hotplug_event_notify(PCIDevice *dev)
         msix_notify(dev, pcie_cap_flags_get_vector(dev));
     } else if (msi_enabled(dev)) {
         msi_notify(dev, pcie_cap_flags_get_vector(dev));
-    } else {
+    } else if (pci_intx(dev) != -1) {
         pci_set_irq(dev, dev->exp.hpev_notified);
     }
 }
@@ -361,7 +361,8 @@ static void hotplug_event_notify(PCIDevice *dev)
 static void hotplug_event_clear(PCIDevice *dev)
 {
     hotplug_event_update_event_status(dev);
-    if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified) {
+    if (!msix_enabled(dev) && !msi_enabled(dev) && pci_intx(dev) != -1 &&
+        !dev->exp.hpev_notified) {
         pci_irq_deassert(dev);
     }
 }
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index e1a8a88c8c..92bd0530dd 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -290,7 +290,7 @@ static void pcie_aer_root_notify(PCIDevice *dev)
         msix_notify(dev, pcie_aer_root_get_vector(dev));
     } else if (msi_enabled(dev)) {
         msi_notify(dev, pcie_aer_root_get_vector(dev));
-    } else {
+    } else if (pci_intx(dev) != -1) {
         pci_irq_assert(dev);
     }
 }
-- 
2.35.1

Re: [PATCH v2 1/2] pcie: Don't try triggering a LSI when not defined

[Daniel Henrique Barboza]

On 4/8/22 10:13, Frederic Barrat wrote:
> This patch skips [de]asserting a LSI interrupt if the device doesn't
> have any LSI defined. Doing so would trigger an assert in
> pci_irq_handler().
> 
> The PCIE root port implementation in qemu requests a LSI (INTA), but a
> subclass may want to change that behavior since it's a valid
> configuration. For example on the POWER8/POWER9/POWER10 systems, the
> root bridge doesn't request any LSI.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/pci/pcie.c     | 5 +++--
>   hw/pci/pcie_aer.c | 2 +-
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 67a5d67372..68a62da0b5 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -353,7 +353,7 @@ static void hotplug_event_notify(PCIDevice *dev)
>           msix_notify(dev, pcie_cap_flags_get_vector(dev));
>       } else if (msi_enabled(dev)) {
>           msi_notify(dev, pcie_cap_flags_get_vector(dev));
> -    } else {
> +    } else if (pci_intx(dev) != -1) {
>           pci_set_irq(dev, dev->exp.hpev_notified);
>       }
>   }
> @@ -361,7 +361,8 @@ static void hotplug_event_notify(PCIDevice *dev)
>   static void hotplug_event_clear(PCIDevice *dev)
>   {
>       hotplug_event_update_event_status(dev);
> -    if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified) {
> +    if (!msix_enabled(dev) && !msi_enabled(dev) && pci_intx(dev) != -1 &&
> +        !dev->exp.hpev_notified) {
>           pci_irq_deassert(dev);
>       }
>   }
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index e1a8a88c8c..92bd0530dd 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -290,7 +290,7 @@ static void pcie_aer_root_notify(PCIDevice *dev)
>           msix_notify(dev, pcie_aer_root_get_vector(dev));
>       } else if (msi_enabled(dev)) {
>           msi_notify(dev, pcie_aer_root_get_vector(dev));
> -    } else {
> +    } else if (pci_intx(dev) != -1) {
>           pci_irq_assert(dev);
>       }
>   }

Re: [PATCH v2 1/2] pcie: Don't try triggering a LSI when not defined

[Daniel Henrique Barboza]

Michael,

Let me know if you want me to grab this patch from the ppc tree. I'm
going to send a PR in the next few days.


Thanks,


Daniel

On 4/8/22 10:13, Frederic Barrat wrote:
> This patch skips [de]asserting a LSI interrupt if the device doesn't
> have any LSI defined. Doing so would trigger an assert in
> pci_irq_handler().
> 
> The PCIE root port implementation in qemu requests a LSI (INTA), but a
> subclass may want to change that behavior since it's a valid
> configuration. For example on the POWER8/POWER9/POWER10 systems, the
> root bridge doesn't request any LSI.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
>   hw/pci/pcie.c     | 5 +++--
>   hw/pci/pcie_aer.c | 2 +-
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 67a5d67372..68a62da0b5 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -353,7 +353,7 @@ static void hotplug_event_notify(PCIDevice *dev)
>           msix_notify(dev, pcie_cap_flags_get_vector(dev));
>       } else if (msi_enabled(dev)) {
>           msi_notify(dev, pcie_cap_flags_get_vector(dev));
> -    } else {
> +    } else if (pci_intx(dev) != -1) {
>           pci_set_irq(dev, dev->exp.hpev_notified);
>       }
>   }
> @@ -361,7 +361,8 @@ static void hotplug_event_notify(PCIDevice *dev)
>   static void hotplug_event_clear(PCIDevice *dev)
>   {
>       hotplug_event_update_event_status(dev);
> -    if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified) {
> +    if (!msix_enabled(dev) && !msi_enabled(dev) && pci_intx(dev) != -1 &&
> +        !dev->exp.hpev_notified) {
>           pci_irq_deassert(dev);
>       }
>   }
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index e1a8a88c8c..92bd0530dd 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -290,7 +290,7 @@ static void pcie_aer_root_notify(PCIDevice *dev)
>           msix_notify(dev, pcie_aer_root_get_vector(dev));
>       } else if (msi_enabled(dev)) {
>           msi_notify(dev, pcie_aer_root_get_vector(dev));
> -    } else {
> +    } else if (pci_intx(dev) != -1) {
>           pci_irq_assert(dev);
>       }
>   }

Re: [PATCH v2 1/2] pcie: Don't try triggering a LSI when not defined

[Michael S. Tsirkin]

On Fri, Apr 08, 2022 at 03:13:02PM +0200, Frederic Barrat wrote:
> This patch skips [de]asserting a LSI interrupt if the device doesn't
> have any LSI defined. Doing so would trigger an assert in
> pci_irq_handler().
> 
> The PCIE root port implementation in qemu requests a LSI (INTA), but a
> subclass may want to change that behavior since it's a valid
> configuration. For example on the POWER8/POWER9/POWER10 systems, the
> root bridge doesn't request any LSI.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


Feel free to merge with the second patch. Thanks!

> ---
>  hw/pci/pcie.c     | 5 +++--
>  hw/pci/pcie_aer.c | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 67a5d67372..68a62da0b5 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -353,7 +353,7 @@ static void hotplug_event_notify(PCIDevice *dev)
>          msix_notify(dev, pcie_cap_flags_get_vector(dev));
>      } else if (msi_enabled(dev)) {
>          msi_notify(dev, pcie_cap_flags_get_vector(dev));
> -    } else {
> +    } else if (pci_intx(dev) != -1) {
>          pci_set_irq(dev, dev->exp.hpev_notified);
>      }
>  }
> @@ -361,7 +361,8 @@ static void hotplug_event_notify(PCIDevice *dev)
>  static void hotplug_event_clear(PCIDevice *dev)
>  {
>      hotplug_event_update_event_status(dev);
> -    if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified) {
> +    if (!msix_enabled(dev) && !msi_enabled(dev) && pci_intx(dev) != -1 &&
> +        !dev->exp.hpev_notified) {
>          pci_irq_deassert(dev);
>      }
>  }
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index e1a8a88c8c..92bd0530dd 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -290,7 +290,7 @@ static void pcie_aer_root_notify(PCIDevice *dev)
>          msix_notify(dev, pcie_aer_root_get_vector(dev));
>      } else if (msi_enabled(dev)) {
>          msi_notify(dev, pcie_aer_root_get_vector(dev));
> -    } else {
> +    } else if (pci_intx(dev) != -1) {
>          pci_irq_assert(dev);
>      }
>  }
> -- 
> 2.35.1

[PATCH v2 2/2] ppc/pnv: Remove LSI on the PCIE host bridge

[Frederic Barrat]

The phb3/phb4/phb5 root ports inherit from the default PCIE root port
implementation, which requests a LSI interrupt (#INTA). On real
hardware (POWER8/POWER9/POWER10), there is no such LSI. This patch
corrects it so that it matches the hardware.

As a consequence, the device tree previously generated was bogus, as
the root bridge LSI was not properly mapped. On some
implementation (powernv9), it was leading to inconsistent interrupt
controller (xive) data. With this patch, it is now clean.

Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
 hw/pci-host/pnv_phb3.c | 1 +
 hw/pci-host/pnv_phb4.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 6e9aa9d6ac..6a884833a8 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1162,6 +1162,7 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
+    pci_config_set_interrupt_pin(pci->config, 0);
 }
 
 static void pnv_phb3_root_port_class_init(ObjectClass *klass, void *data)
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 11c97e27eb..dd81e940b7 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1772,6 +1772,7 @@ static void pnv_phb4_root_port_reset(DeviceState *dev)
     pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, 0xfff1);
     pci_set_long(conf + PCI_PREF_BASE_UPPER32, 0x1); /* Hack */
     pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0xffffffff);
+    pci_config_set_interrupt_pin(conf, 0);
 }
 
 static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)
-- 
2.35.1

Re: [PATCH v2 2/2] ppc/pnv: Remove LSI on the PCIE host bridge

[Daniel Henrique Barboza]

On 4/8/22 10:13, Frederic Barrat wrote:
> The phb3/phb4/phb5 root ports inherit from the default PCIE root port
> implementation, which requests a LSI interrupt (#INTA). On real
> hardware (POWER8/POWER9/POWER10), there is no such LSI. This patch
> corrects it so that it matches the hardware.
> 
> As a consequence, the device tree previously generated was bogus, as
> the root bridge LSI was not properly mapped. On some
> implementation (powernv9), it was leading to inconsistent interrupt
> controller (xive) data. With this patch, it is now clean.
> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/pci-host/pnv_phb3.c | 1 +
>   hw/pci-host/pnv_phb4.c | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 6e9aa9d6ac..6a884833a8 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1162,6 +1162,7 @@ static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
>           error_propagate(errp, local_err);
>           return;
>       }
> +    pci_config_set_interrupt_pin(pci->config, 0);
>   }
>   
>   static void pnv_phb3_root_port_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 11c97e27eb..dd81e940b7 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1772,6 +1772,7 @@ static void pnv_phb4_root_port_reset(DeviceState *dev)
>       pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, 0xfff1);
>       pci_set_long(conf + PCI_PREF_BASE_UPPER32, 0x1); /* Hack */
>       pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0xffffffff);
> +    pci_config_set_interrupt_pin(conf, 0);
>   }
>   
>   static void pnv_phb4_root_port_realize(DeviceState *dev, Error **errp)

Re: [PATCH v2 0/2] Remove PCIE root bridge LSI on powernv

[Daniel Henrique Barboza]

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel



On 4/8/22 10:13, Frederic Barrat wrote:
> The powernv8/powernv9/powernv10 machines allocate a LSI for their root
> port bridge, which is not the case on real hardware. The default root
> port implementation in qemu requests a LSI. Since the powernv
> implementation derives from it, that's where the LSI is coming
> from. This series fixes it, so that the model matches the hardware.
> 
> However, the code in hw/pci to handle AER and hotplug events assume a
> LSI is defined. It tends to assert/deassert a LSI if MSI or MSIX is
> not enabled. Since we have hardware where that is not true, this patch
> also fixes a few code paths to check if a LSI is configured before
> trying to trigger it.
> 
> 
> Changes from v1:
>   - addressed comments from Daniel
> 
> 
> Frederic Barrat (2):
>    pcie: Don't try triggering a LSI when not defined
>    ppc/pnv: Remove LSI on the PCIE host bridge
> 
>   hw/pci-host/pnv_phb3.c | 1 +
>   hw/pci-host/pnv_phb4.c | 1 +
>   hw/pci/pcie.c          | 5 +++--
>   hw/pci/pcie_aer.c      | 2 +-
>   4 files changed, 6 insertions(+), 3 deletions(-)
>