| Message ID | 20220321153357.165775-2-fbarrat@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove PCIE root bridge LSI on powernv | expand |
On 3/21/22 12:33, 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> > --- I assume that it's easier to handle just the codepaths that powernv PHBs uses rather than handling all instances where pci_irq_handler() would be asserting without LSIs. Patch LGTM. Small nits below: > hw/pci/pcie.c | 8 ++++++-- > hw/pci/pcie_aer.c | 4 +++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 67a5d67372..71c5194b80 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -354,7 +354,9 @@ static void hotplug_event_notify(PCIDevice *dev) > } else if (msi_enabled(dev)) { > msi_notify(dev, pcie_cap_flags_get_vector(dev)); > } else { > - pci_set_irq(dev, dev->exp.hpev_notified); > + if (pci_intx(dev) != -1) { > + pci_set_irq(dev, dev->exp.hpev_notified); > + } Since you're not doing anything unless the condition is met, you can use 'else if' like it's done in the other conditionals: if (msix_enabled(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 if (pci_intx(dev) != -1) { pci_set_irq(dev, dev->exp.hpev_notified); } > } > } > > @@ -362,7 +364,9 @@ static void hotplug_event_clear(PCIDevice *dev) > { > hotplug_event_update_event_status(dev); > if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified) { > - pci_irq_deassert(dev); > + if (pci_intx(dev) != -1) { > + pci_irq_deassert(dev); > + } > } Similar comment here: if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified && pci_intx(dev) != -1) { pci_irq_deassert(dev); } > } > > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c > index e1a8a88c8c..d936bfca20 100644 > --- a/hw/pci/pcie_aer.c > +++ b/hw/pci/pcie_aer.c > @@ -291,7 +291,9 @@ static void pcie_aer_root_notify(PCIDevice *dev) > } else if (msi_enabled(dev)) { > msi_notify(dev, pcie_aer_root_get_vector(dev)); > } else { > - pci_irq_assert(dev); > + if (pci_intx(dev) != -1) { > + pci_irq_assert(dev); > + } And here: if (msix_enabled(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 if (pci_intx(dev) != -1) { pci_irq_assert(dev); } Thanks, Daniel > } > } >
On 24/03/2022 14:07, Daniel Henrique Barboza wrote: > > > On 3/21/22 12:33, 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> >> --- > > I assume that it's easier to handle just the codepaths that powernv PHBs > uses > rather than handling all instances where pci_irq_handler() would be > asserting > without LSIs. The real reason is that the LSI is added when we realize the TYPE_PCIE_ROOT_PORT object. See rp_realize(). So I'm only trying to fix the code paths that can be called from a subclass of TYPE_PCIE_ROOT_PORT which would choose to override the "interrupt pin" setting in the config space. I believe they are all covered by this patch. The assert() in pci_irq_handler() is there for a reason and I don't want to mess with that. Fred > > > Patch LGTM. Small nits below: > >> hw/pci/pcie.c | 8 ++++++-- >> hw/pci/pcie_aer.c | 4 +++- >> 2 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 67a5d67372..71c5194b80 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -354,7 +354,9 @@ static void hotplug_event_notify(PCIDevice *dev) >> } else if (msi_enabled(dev)) { >> msi_notify(dev, pcie_cap_flags_get_vector(dev)); >> } else { >> - pci_set_irq(dev, dev->exp.hpev_notified); >> + if (pci_intx(dev) != -1) { >> + pci_set_irq(dev, dev->exp.hpev_notified); >> + } > > > Since you're not doing anything unless the condition is met, you can use > 'else if' > like it's done in the other conditionals: > > > if (msix_enabled(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 if (pci_intx(dev) != -1) { > pci_set_irq(dev, dev->exp.hpev_notified); > } > > > >> } >> } >> @@ -362,7 +364,9 @@ static void hotplug_event_clear(PCIDevice *dev) >> { >> hotplug_event_update_event_status(dev); >> if (!msix_enabled(dev) && !msi_enabled(dev) && >> !dev->exp.hpev_notified) { >> - pci_irq_deassert(dev); >> + if (pci_intx(dev) != -1) { >> + pci_irq_deassert(dev); >> + } >> } > > Similar comment here: > > if (!msix_enabled(dev) && !msi_enabled(dev) && > !dev->exp.hpev_notified && > pci_intx(dev) != -1) { > pci_irq_deassert(dev); > } > > > >> } >> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c >> index e1a8a88c8c..d936bfca20 100644 >> --- a/hw/pci/pcie_aer.c >> +++ b/hw/pci/pcie_aer.c >> @@ -291,7 +291,9 @@ static void pcie_aer_root_notify(PCIDevice *dev) >> } else if (msi_enabled(dev)) { >> msi_notify(dev, pcie_aer_root_get_vector(dev)); >> } else { >> - pci_irq_assert(dev); >> + if (pci_intx(dev) != -1) { >> + pci_irq_assert(dev); >> + } > > > And here: > > if (msix_enabled(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 if (pci_intx(dev) != -1) { > pci_irq_assert(dev); > } > > > > Thanks, > > > Daniel > >> } >> }
On 3/24/22 10:47, Frederic Barrat wrote: > > > On 24/03/2022 14:07, Daniel Henrique Barboza wrote: >> >> >> On 3/21/22 12:33, 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> >>> --- >> >> I assume that it's easier to handle just the codepaths that powernv PHBs uses >> rather than handling all instances where pci_irq_handler() would be asserting >> without LSIs. > > > The real reason is that the LSI is added when we realize the TYPE_PCIE_ROOT_PORT object. See rp_realize(). So I'm only trying to fix the code paths that can be called from a subclass of TYPE_PCIE_ROOT_PORT which would choose to override the "interrupt pin" setting in the config space. I believe they are all covered by this patch. > The assert() in pci_irq_handler() is there for a reason and I don't want to mess with that. Yes, handling this situation inside pci_irq_handler() would require changing the "assert(0 <= irq_num && irq_num < PCI_NUM_PINS)" assert or doing some sanity before it to avoid the trigger. Since this is a common code used everywhere we're better of doing minimalist changes as you're doing. We can reevaluate this design if more machines/devices start to get in the same situation we have now with powernv PHBs. Daniel > > Fred > > >> >> >> Patch LGTM. Small nits below: >> >>> hw/pci/pcie.c | 8 ++++++-- >>> hw/pci/pcie_aer.c | 4 +++- >>> 2 files changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >>> index 67a5d67372..71c5194b80 100644 >>> --- a/hw/pci/pcie.c >>> +++ b/hw/pci/pcie.c >>> @@ -354,7 +354,9 @@ static void hotplug_event_notify(PCIDevice *dev) >>> } else if (msi_enabled(dev)) { >>> msi_notify(dev, pcie_cap_flags_get_vector(dev)); >>> } else { >>> - pci_set_irq(dev, dev->exp.hpev_notified); >>> + if (pci_intx(dev) != -1) { >>> + pci_set_irq(dev, dev->exp.hpev_notified); >>> + } >> >> >> Since you're not doing anything unless the condition is met, you can use 'else if' >> like it's done in the other conditionals: >> >> >> if (msix_enabled(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 if (pci_intx(dev) != -1) { >> pci_set_irq(dev, dev->exp.hpev_notified); >> } >> >> >> >>> } >>> } >>> @@ -362,7 +364,9 @@ static void hotplug_event_clear(PCIDevice *dev) >>> { >>> hotplug_event_update_event_status(dev); >>> if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified) { >>> - pci_irq_deassert(dev); >>> + if (pci_intx(dev) != -1) { >>> + pci_irq_deassert(dev); >>> + } >>> } >> >> Similar comment here: >> >> if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified && >> pci_intx(dev) != -1) { >> pci_irq_deassert(dev); >> } >> >> >> >>> } >>> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c >>> index e1a8a88c8c..d936bfca20 100644 >>> --- a/hw/pci/pcie_aer.c >>> +++ b/hw/pci/pcie_aer.c >>> @@ -291,7 +291,9 @@ static void pcie_aer_root_notify(PCIDevice *dev) >>> } else if (msi_enabled(dev)) { >>> msi_notify(dev, pcie_aer_root_get_vector(dev)); >>> } else { >>> - pci_irq_assert(dev); >>> + if (pci_intx(dev) != -1) { >>> + pci_irq_assert(dev); >>> + } >> >> >> And here: >> >> if (msix_enabled(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 if (pci_intx(dev) != -1) { >> pci_irq_assert(dev); >> } >> >> >> >> Thanks, >> >> >> Daniel >> >>> } >>> }
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 67a5d67372..71c5194b80 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -354,7 +354,9 @@ static void hotplug_event_notify(PCIDevice *dev) } else if (msi_enabled(dev)) { msi_notify(dev, pcie_cap_flags_get_vector(dev)); } else { - pci_set_irq(dev, dev->exp.hpev_notified); + if (pci_intx(dev) != -1) { + pci_set_irq(dev, dev->exp.hpev_notified); + } } } @@ -362,7 +364,9 @@ static void hotplug_event_clear(PCIDevice *dev) { hotplug_event_update_event_status(dev); if (!msix_enabled(dev) && !msi_enabled(dev) && !dev->exp.hpev_notified) { - pci_irq_deassert(dev); + if (pci_intx(dev) != -1) { + pci_irq_deassert(dev); + } } } diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c index e1a8a88c8c..d936bfca20 100644 --- a/hw/pci/pcie_aer.c +++ b/hw/pci/pcie_aer.c @@ -291,7 +291,9 @@ static void pcie_aer_root_notify(PCIDevice *dev) } else if (msi_enabled(dev)) { msi_notify(dev, pcie_aer_root_get_vector(dev)); } else { - pci_irq_assert(dev); + if (pci_intx(dev) != -1) { + pci_irq_assert(dev); + } } }
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 | 8 ++++++-- hw/pci/pcie_aer.c | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-)