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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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

>       }
>   }
>