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

[Frederic Barrat <fbarrat@linux.ibm.com>]

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