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

[Daniel Henrique Barboza]

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
> >
> > >       }
> > >   }