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(-)
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);
+ }
}
}
--
2.35.1
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
>>
>>> }
>>> }
© 2016 - 2026 Red Hat, Inc.