This requires some adjustments to callers to avoid possible behaviour
changes for PCI devices.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/ipmi/ipmi.h | 5 +++++
hw/acpi/ipmi.c | 2 +-
hw/ipmi/isa_ipmi_bt.c | 1 +
hw/ipmi/isa_ipmi_kcs.c | 1 +
hw/ipmi/pci_ipmi_bt.c | 12 ++++++++++++
hw/ipmi/pci_ipmi_kcs.c | 11 +++++++++++
hw/smbios/smbios_type_38.c | 6 +++++-
7 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index 77a7213ed93..71c4efac8cd 100644
--- a/include/hw/ipmi/ipmi.h
+++ b/include/hw/ipmi/ipmi.h
@@ -90,6 +90,11 @@ typedef struct IPMIFwInfo {
} memspace;
int interrupt_number;
+ enum {
+ IPMI_NO_IRQ = 0,
+ IPMI_ISA_IRQ,
+ IPMI_PCI_IRQ,
+ } irq;
enum {
IPMI_LEVEL_IRQ,
IPMI_EDGE_IRQ
diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
index a20e57d465c..c81cbd2f158 100644
--- a/hw/acpi/ipmi.c
+++ b/hw/acpi/ipmi.c
@@ -55,7 +55,7 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
abort();
}
- if (info->interrupt_number) {
+ if (info->irq == IPMI_ISA_IRQ && info->interrupt_number) {
aml_append(crs, aml_irq_no_flags(info->interrupt_number));
}
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index a1b66d5ee82..b5556436b82 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -49,6 +49,7 @@ static void isa_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
ISAIPMIBTDevice *iib = ISA_IPMI_BT(ii);
ipmi_bt_get_fwinfo(&iib->bt, info);
+ info->irq = IPMI_ISA_IRQ;
info->interrupt_number = iib->isairq;
info->i2c_slave_address = iib->bt.bmc->slave_addr;
info->uuid = iib->uuid;
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index d9ebdd5371f..326115f51bb 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -49,6 +49,7 @@ static void isa_ipmi_kcs_get_fwinfo(IPMIInterface *ii, IPMIFwInfo *info)
ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(ii);
ipmi_kcs_get_fwinfo(&iik->kcs, info);
+ info->irq = IPMI_ISA_IRQ;
info->interrupt_number = iik->isairq;
info->uuid = iik->uuid;
}
diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
index a3b742d22c9..33ff7190ee8 100644
--- a/hw/ipmi/pci_ipmi_bt.c
+++ b/hw/ipmi/pci_ipmi_bt.c
@@ -38,6 +38,17 @@ struct PCIIPMIBTDevice {
uint32_t uuid;
};
+static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
+{
+ PCIIPMIBTDevice *pib = PCI_IPMI_BT(ii);
+
+ ipmi_bt_get_fwinfo(&pib->bt, info);
+ info->irq = IPMI_PCI_IRQ;
+ info->interrupt_number = pci_intx(&pib->dev);
+ info->i2c_slave_address = pib->bt.bmc->slave_addr;
+ info->uuid = pib->uuid;
+}
+
static void pci_ipmi_raise_irq(IPMIBT *ib)
{
PCIIPMIBTDevice *pib = ib->opaque;
@@ -125,6 +136,7 @@ static void pci_ipmi_bt_class_init(ObjectClass *oc, void *data)
iic->get_backend_data = pci_ipmi_bt_get_backend_data;
ipmi_bt_class_init(iic);
+ iic->get_fwinfo = pci_ipmi_bt_get_fwinfo;
}
static const TypeInfo pci_ipmi_bt_info = {
diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
index 05ba97ec58f..6673b2088ef 100644
--- a/hw/ipmi/pci_ipmi_kcs.c
+++ b/hw/ipmi/pci_ipmi_kcs.c
@@ -38,6 +38,16 @@ struct PCIIPMIKCSDevice {
uint32_t uuid;
};
+static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
+{
+ PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(ii);
+
+ ipmi_kcs_get_fwinfo(&pik->kcs, info);
+ info->irq = IPMI_PCI_IRQ;
+ info->interrupt_number = pci_intx(&pik->dev);
+ info->uuid = pik->uuid;
+}
+
static void pci_ipmi_raise_irq(IPMIKCS *ik)
{
PCIIPMIKCSDevice *pik = ik->opaque;
@@ -125,6 +135,7 @@ static void pci_ipmi_kcs_class_init(ObjectClass *oc, void *data)
iic->get_backend_data = pci_ipmi_kcs_get_backend_data;
ipmi_kcs_class_init(iic);
+ iic->get_fwinfo = pci_ipmi_kcs_get_fwinfo;
}
static const TypeInfo pci_ipmi_kcs_info = {
diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
index 168b886647d..2823929c258 100644
--- a/hw/smbios/smbios_type_38.c
+++ b/hw/smbios/smbios_type_38.c
@@ -72,7 +72,11 @@ static void smbios_build_one_type_38(IPMIFwInfo *info)
" SMBIOS, ignoring this entry.", info->register_spacing);
return;
}
- t->interrupt_number = info->interrupt_number;
+ if (info->irq == IPMI_ISA_IRQ) {
+ t->interrupt_number = info->interrupt_number;
+ } else {
+ t->interrupt_number = 0;
+ }
SMBIOS_BUILD_TABLE_POST;
}
--
2.47.1On Tue, Apr 01, 2025 at 09:44:09PM +1000, Nicholas Piggin wrote:
> This requires some adjustments to callers to avoid possible behaviour
> changes for PCI devices.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> include/hw/ipmi/ipmi.h | 5 +++++
> hw/acpi/ipmi.c | 2 +-
> hw/ipmi/isa_ipmi_bt.c | 1 +
> hw/ipmi/isa_ipmi_kcs.c | 1 +
> hw/ipmi/pci_ipmi_bt.c | 12 ++++++++++++
> hw/ipmi/pci_ipmi_kcs.c | 11 +++++++++++
> hw/smbios/smbios_type_38.c | 6 +++++-
> 7 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
> index 77a7213ed93..71c4efac8cd 100644
> --- a/include/hw/ipmi/ipmi.h
> +++ b/include/hw/ipmi/ipmi.h
> @@ -90,6 +90,11 @@ typedef struct IPMIFwInfo {
> } memspace;
>
> int interrupt_number;
> + enum {
> + IPMI_NO_IRQ = 0,
> + IPMI_ISA_IRQ,
> + IPMI_PCI_IRQ,
> + } irq;
In addition to Phillippe's comment, can you name this "irq_source" to
make it clear what it is?
Overall this patch set looks good.
-corey
> enum {
> IPMI_LEVEL_IRQ,
> IPMI_EDGE_IRQ
> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> index a20e57d465c..c81cbd2f158 100644
> --- a/hw/acpi/ipmi.c
> +++ b/hw/acpi/ipmi.c
> @@ -55,7 +55,7 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
> abort();
> }
>
> - if (info->interrupt_number) {
> + if (info->irq == IPMI_ISA_IRQ && info->interrupt_number) {
> aml_append(crs, aml_irq_no_flags(info->interrupt_number));
> }
>
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index a1b66d5ee82..b5556436b82 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -49,6 +49,7 @@ static void isa_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
> ISAIPMIBTDevice *iib = ISA_IPMI_BT(ii);
>
> ipmi_bt_get_fwinfo(&iib->bt, info);
> + info->irq = IPMI_ISA_IRQ;
> info->interrupt_number = iib->isairq;
> info->i2c_slave_address = iib->bt.bmc->slave_addr;
> info->uuid = iib->uuid;
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index d9ebdd5371f..326115f51bb 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -49,6 +49,7 @@ static void isa_ipmi_kcs_get_fwinfo(IPMIInterface *ii, IPMIFwInfo *info)
> ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(ii);
>
> ipmi_kcs_get_fwinfo(&iik->kcs, info);
> + info->irq = IPMI_ISA_IRQ;
> info->interrupt_number = iik->isairq;
> info->uuid = iik->uuid;
> }
> diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
> index a3b742d22c9..33ff7190ee8 100644
> --- a/hw/ipmi/pci_ipmi_bt.c
> +++ b/hw/ipmi/pci_ipmi_bt.c
> @@ -38,6 +38,17 @@ struct PCIIPMIBTDevice {
> uint32_t uuid;
> };
>
> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
> +{
> + PCIIPMIBTDevice *pib = PCI_IPMI_BT(ii);
> +
> + ipmi_bt_get_fwinfo(&pib->bt, info);
> + info->irq = IPMI_PCI_IRQ;
> + info->interrupt_number = pci_intx(&pib->dev);
> + info->i2c_slave_address = pib->bt.bmc->slave_addr;
> + info->uuid = pib->uuid;
> +}
> +
> static void pci_ipmi_raise_irq(IPMIBT *ib)
> {
> PCIIPMIBTDevice *pib = ib->opaque;
> @@ -125,6 +136,7 @@ static void pci_ipmi_bt_class_init(ObjectClass *oc, void *data)
>
> iic->get_backend_data = pci_ipmi_bt_get_backend_data;
> ipmi_bt_class_init(iic);
> + iic->get_fwinfo = pci_ipmi_bt_get_fwinfo;
> }
>
> static const TypeInfo pci_ipmi_bt_info = {
> diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
> index 05ba97ec58f..6673b2088ef 100644
> --- a/hw/ipmi/pci_ipmi_kcs.c
> +++ b/hw/ipmi/pci_ipmi_kcs.c
> @@ -38,6 +38,16 @@ struct PCIIPMIKCSDevice {
> uint32_t uuid;
> };
>
> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
> +{
> + PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(ii);
> +
> + ipmi_kcs_get_fwinfo(&pik->kcs, info);
> + info->irq = IPMI_PCI_IRQ;
> + info->interrupt_number = pci_intx(&pik->dev);
> + info->uuid = pik->uuid;
> +}
> +
> static void pci_ipmi_raise_irq(IPMIKCS *ik)
> {
> PCIIPMIKCSDevice *pik = ik->opaque;
> @@ -125,6 +135,7 @@ static void pci_ipmi_kcs_class_init(ObjectClass *oc, void *data)
>
> iic->get_backend_data = pci_ipmi_kcs_get_backend_data;
> ipmi_kcs_class_init(iic);
> + iic->get_fwinfo = pci_ipmi_kcs_get_fwinfo;
> }
>
> static const TypeInfo pci_ipmi_kcs_info = {
> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
> index 168b886647d..2823929c258 100644
> --- a/hw/smbios/smbios_type_38.c
> +++ b/hw/smbios/smbios_type_38.c
> @@ -72,7 +72,11 @@ static void smbios_build_one_type_38(IPMIFwInfo *info)
> " SMBIOS, ignoring this entry.", info->register_spacing);
> return;
> }
> - t->interrupt_number = info->interrupt_number;
> + if (info->irq == IPMI_ISA_IRQ) {
> + t->interrupt_number = info->interrupt_number;
> + } else {
> + t->interrupt_number = 0;
> + }
>
> SMBIOS_BUILD_TABLE_POST;
> }
> --
> 2.47.1
>
On Tue Apr 1, 2025 at 11:09 PM AEST, Corey Minyard wrote:
> On Tue, Apr 01, 2025 at 09:44:09PM +1000, Nicholas Piggin wrote:
>> This requires some adjustments to callers to avoid possible behaviour
>> changes for PCI devices.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> include/hw/ipmi/ipmi.h | 5 +++++
>> hw/acpi/ipmi.c | 2 +-
>> hw/ipmi/isa_ipmi_bt.c | 1 +
>> hw/ipmi/isa_ipmi_kcs.c | 1 +
>> hw/ipmi/pci_ipmi_bt.c | 12 ++++++++++++
>> hw/ipmi/pci_ipmi_kcs.c | 11 +++++++++++
>> hw/smbios/smbios_type_38.c | 6 +++++-
>> 7 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
>> index 77a7213ed93..71c4efac8cd 100644
>> --- a/include/hw/ipmi/ipmi.h
>> +++ b/include/hw/ipmi/ipmi.h
>> @@ -90,6 +90,11 @@ typedef struct IPMIFwInfo {
>> } memspace;
>>
>> int interrupt_number;
>> + enum {
>> + IPMI_NO_IRQ = 0,
>> + IPMI_ISA_IRQ,
>> + IPMI_PCI_IRQ,
>> + } irq;
>
> In addition to Phillippe's comment, can you name this "irq_source" to
> make it clear what it is?
Sure.
I wonder now looking at fwinfo if I should put protocol in there
too instead of in the class.
Thanks,
Nick
Hi Nick,
On 1/4/25 13:44, Nicholas Piggin wrote:
> This requires some adjustments to callers to avoid possible behaviour
> changes for PCI devices.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> include/hw/ipmi/ipmi.h | 5 +++++
> hw/acpi/ipmi.c | 2 +-
> hw/ipmi/isa_ipmi_bt.c | 1 +
> hw/ipmi/isa_ipmi_kcs.c | 1 +
> hw/ipmi/pci_ipmi_bt.c | 12 ++++++++++++
> hw/ipmi/pci_ipmi_kcs.c | 11 +++++++++++
> hw/smbios/smbios_type_38.c | 6 +++++-
> 7 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
> index 77a7213ed93..71c4efac8cd 100644
> --- a/include/hw/ipmi/ipmi.h
> +++ b/include/hw/ipmi/ipmi.h
> @@ -90,6 +90,11 @@ typedef struct IPMIFwInfo {
> } memspace;
>
> int interrupt_number;
> + enum {
> + IPMI_NO_IRQ = 0,
> + IPMI_ISA_IRQ,
> + IPMI_PCI_IRQ,
> + } irq;
> enum {
> IPMI_LEVEL_IRQ,
> IPMI_EDGE_IRQ
> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> index a20e57d465c..c81cbd2f158 100644
> --- a/hw/acpi/ipmi.c
> +++ b/hw/acpi/ipmi.c
> @@ -55,7 +55,7 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
> abort();
> }
>
> - if (info->interrupt_number) {
> + if (info->irq == IPMI_ISA_IRQ && info->interrupt_number) {
> aml_append(crs, aml_irq_no_flags(info->interrupt_number));
> }
>
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index a1b66d5ee82..b5556436b82 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -49,6 +49,7 @@ static void isa_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
> ISAIPMIBTDevice *iib = ISA_IPMI_BT(ii);
>
> ipmi_bt_get_fwinfo(&iib->bt, info);
> + info->irq = IPMI_ISA_IRQ;
> info->interrupt_number = iib->isairq;
> info->i2c_slave_address = iib->bt.bmc->slave_addr;
> info->uuid = iib->uuid;
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index d9ebdd5371f..326115f51bb 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -49,6 +49,7 @@ static void isa_ipmi_kcs_get_fwinfo(IPMIInterface *ii, IPMIFwInfo *info)
> ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(ii);
>
> ipmi_kcs_get_fwinfo(&iik->kcs, info);
> + info->irq = IPMI_ISA_IRQ;
> info->interrupt_number = iik->isairq;
> info->uuid = iik->uuid;
> }
> diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
> index a3b742d22c9..33ff7190ee8 100644
> --- a/hw/ipmi/pci_ipmi_bt.c
> +++ b/hw/ipmi/pci_ipmi_bt.c
> @@ -38,6 +38,17 @@ struct PCIIPMIBTDevice {
> uint32_t uuid;
> };
>
> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
> +{
> + PCIIPMIBTDevice *pib = PCI_IPMI_BT(ii);
> +
> + ipmi_bt_get_fwinfo(&pib->bt, info);
> + info->irq = IPMI_PCI_IRQ;
> + info->interrupt_number = pci_intx(&pib->dev);
> + info->i2c_slave_address = pib->bt.bmc->slave_addr;
> + info->uuid = pib->uuid;
> +}
> +
> static void pci_ipmi_raise_irq(IPMIBT *ib)
> {
> PCIIPMIBTDevice *pib = ib->opaque;
> @@ -125,6 +136,7 @@ static void pci_ipmi_bt_class_init(ObjectClass *oc, void *data)
>
> iic->get_backend_data = pci_ipmi_bt_get_backend_data;
> ipmi_bt_class_init(iic);
> + iic->get_fwinfo = pci_ipmi_bt_get_fwinfo;
> }
>
> static const TypeInfo pci_ipmi_bt_info = {
> diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
> index 05ba97ec58f..6673b2088ef 100644
> --- a/hw/ipmi/pci_ipmi_kcs.c
> +++ b/hw/ipmi/pci_ipmi_kcs.c
> @@ -38,6 +38,16 @@ struct PCIIPMIKCSDevice {
> uint32_t uuid;
> };
>
> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
> +{
> + PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(ii);
> +
> + ipmi_kcs_get_fwinfo(&pik->kcs, info);
> + info->irq = IPMI_PCI_IRQ;
> + info->interrupt_number = pci_intx(&pik->dev);
> + info->uuid = pik->uuid;
> +}
> +
> static void pci_ipmi_raise_irq(IPMIKCS *ik)
> {
> PCIIPMIKCSDevice *pik = ik->opaque;
> @@ -125,6 +135,7 @@ static void pci_ipmi_kcs_class_init(ObjectClass *oc, void *data)
>
> iic->get_backend_data = pci_ipmi_kcs_get_backend_data;
> ipmi_kcs_class_init(iic);
> + iic->get_fwinfo = pci_ipmi_kcs_get_fwinfo;
> }
>
> static const TypeInfo pci_ipmi_kcs_info = {
> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
> index 168b886647d..2823929c258 100644
> --- a/hw/smbios/smbios_type_38.c
> +++ b/hw/smbios/smbios_type_38.c
> @@ -72,7 +72,11 @@ static void smbios_build_one_type_38(IPMIFwInfo *info)
> " SMBIOS, ignoring this entry.", info->register_spacing);
> return;
> }
> - t->interrupt_number = info->interrupt_number;
> + if (info->irq == IPMI_ISA_IRQ) {
> + t->interrupt_number = info->interrupt_number;
> + } else {
> + t->interrupt_number = 0;
Can you explain why use 0 for PCI?
> + }
>
> SMBIOS_BUILD_TABLE_POST;
> }
On Tue Apr 1, 2025 at 9:57 PM AEST, Philippe Mathieu-Daudé wrote:
> Hi Nick,
>
> On 1/4/25 13:44, Nicholas Piggin wrote:
>> This requires some adjustments to callers to avoid possible behaviour
>> changes for PCI devices.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> include/hw/ipmi/ipmi.h | 5 +++++
>> hw/acpi/ipmi.c | 2 +-
>> hw/ipmi/isa_ipmi_bt.c | 1 +
>> hw/ipmi/isa_ipmi_kcs.c | 1 +
>> hw/ipmi/pci_ipmi_bt.c | 12 ++++++++++++
>> hw/ipmi/pci_ipmi_kcs.c | 11 +++++++++++
>> hw/smbios/smbios_type_38.c | 6 +++++-
>> 7 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
>> index 77a7213ed93..71c4efac8cd 100644
>> --- a/include/hw/ipmi/ipmi.h
>> +++ b/include/hw/ipmi/ipmi.h
>> @@ -90,6 +90,11 @@ typedef struct IPMIFwInfo {
>> } memspace;
>>
>> int interrupt_number;
>> + enum {
>> + IPMI_NO_IRQ = 0,
>> + IPMI_ISA_IRQ,
>> + IPMI_PCI_IRQ,
>> + } irq;
>> enum {
>> IPMI_LEVEL_IRQ,
>> IPMI_EDGE_IRQ
>> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
>> index a20e57d465c..c81cbd2f158 100644
>> --- a/hw/acpi/ipmi.c
>> +++ b/hw/acpi/ipmi.c
>> @@ -55,7 +55,7 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>> abort();
>> }
>>
>> - if (info->interrupt_number) {
>> + if (info->irq == IPMI_ISA_IRQ && info->interrupt_number) {
>> aml_append(crs, aml_irq_no_flags(info->interrupt_number));
>> }
>>
>> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
>> index a1b66d5ee82..b5556436b82 100644
>> --- a/hw/ipmi/isa_ipmi_bt.c
>> +++ b/hw/ipmi/isa_ipmi_bt.c
>> @@ -49,6 +49,7 @@ static void isa_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>> ISAIPMIBTDevice *iib = ISA_IPMI_BT(ii);
>>
>> ipmi_bt_get_fwinfo(&iib->bt, info);
>> + info->irq = IPMI_ISA_IRQ;
>> info->interrupt_number = iib->isairq;
>> info->i2c_slave_address = iib->bt.bmc->slave_addr;
>> info->uuid = iib->uuid;
>> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
>> index d9ebdd5371f..326115f51bb 100644
>> --- a/hw/ipmi/isa_ipmi_kcs.c
>> +++ b/hw/ipmi/isa_ipmi_kcs.c
>> @@ -49,6 +49,7 @@ static void isa_ipmi_kcs_get_fwinfo(IPMIInterface *ii, IPMIFwInfo *info)
>> ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(ii);
>>
>> ipmi_kcs_get_fwinfo(&iik->kcs, info);
>> + info->irq = IPMI_ISA_IRQ;
>> info->interrupt_number = iik->isairq;
>> info->uuid = iik->uuid;
>> }
>> diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
>> index a3b742d22c9..33ff7190ee8 100644
>> --- a/hw/ipmi/pci_ipmi_bt.c
>> +++ b/hw/ipmi/pci_ipmi_bt.c
>> @@ -38,6 +38,17 @@ struct PCIIPMIBTDevice {
>> uint32_t uuid;
>> };
>>
>> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>> +{
>> + PCIIPMIBTDevice *pib = PCI_IPMI_BT(ii);
>> +
>> + ipmi_bt_get_fwinfo(&pib->bt, info);
>> + info->irq = IPMI_PCI_IRQ;
>> + info->interrupt_number = pci_intx(&pib->dev);
>> + info->i2c_slave_address = pib->bt.bmc->slave_addr;
>> + info->uuid = pib->uuid;
>> +}
>> +
>> static void pci_ipmi_raise_irq(IPMIBT *ib)
>> {
>> PCIIPMIBTDevice *pib = ib->opaque;
>> @@ -125,6 +136,7 @@ static void pci_ipmi_bt_class_init(ObjectClass *oc, void *data)
>>
>> iic->get_backend_data = pci_ipmi_bt_get_backend_data;
>> ipmi_bt_class_init(iic);
>> + iic->get_fwinfo = pci_ipmi_bt_get_fwinfo;
>> }
>>
>> static const TypeInfo pci_ipmi_bt_info = {
>> diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
>> index 05ba97ec58f..6673b2088ef 100644
>> --- a/hw/ipmi/pci_ipmi_kcs.c
>> +++ b/hw/ipmi/pci_ipmi_kcs.c
>> @@ -38,6 +38,16 @@ struct PCIIPMIKCSDevice {
>> uint32_t uuid;
>> };
>>
>> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>> +{
>> + PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(ii);
>> +
>> + ipmi_kcs_get_fwinfo(&pik->kcs, info);
>> + info->irq = IPMI_PCI_IRQ;
>> + info->interrupt_number = pci_intx(&pik->dev);
>> + info->uuid = pik->uuid;
>> +}
>> +
>> static void pci_ipmi_raise_irq(IPMIKCS *ik)
>> {
>> PCIIPMIKCSDevice *pik = ik->opaque;
>> @@ -125,6 +135,7 @@ static void pci_ipmi_kcs_class_init(ObjectClass *oc, void *data)
>>
>> iic->get_backend_data = pci_ipmi_kcs_get_backend_data;
>> ipmi_kcs_class_init(iic);
>> + iic->get_fwinfo = pci_ipmi_kcs_get_fwinfo;
>> }
>>
>> static const TypeInfo pci_ipmi_kcs_info = {
>> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
>> index 168b886647d..2823929c258 100644
>> --- a/hw/smbios/smbios_type_38.c
>> +++ b/hw/smbios/smbios_type_38.c
>> @@ -72,7 +72,11 @@ static void smbios_build_one_type_38(IPMIFwInfo *info)
>> " SMBIOS, ignoring this entry.", info->register_spacing);
>> return;
>> }
>> - t->interrupt_number = info->interrupt_number;
>> + if (info->irq == IPMI_ISA_IRQ) {
>> + t->interrupt_number = info->interrupt_number;
>> + } else {
>> + t->interrupt_number = 0;
>
> Can you explain why use 0 for PCI?
To avoid changes to other callers after this patch. Previously PCI
devices would leave interrupt_number as 0, not sure if such devices
are relevant here. If there is a better approach I would take it.
Thanks,
Nick
On 1/4/25 15:27, Nicholas Piggin wrote:
> On Tue Apr 1, 2025 at 9:57 PM AEST, Philippe Mathieu-Daudé wrote:
>> Hi Nick,
>>
>> On 1/4/25 13:44, Nicholas Piggin wrote:
>>> This requires some adjustments to callers to avoid possible behaviour
>>> changes for PCI devices.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> include/hw/ipmi/ipmi.h | 5 +++++
>>> hw/acpi/ipmi.c | 2 +-
>>> hw/ipmi/isa_ipmi_bt.c | 1 +
>>> hw/ipmi/isa_ipmi_kcs.c | 1 +
>>> hw/ipmi/pci_ipmi_bt.c | 12 ++++++++++++
>>> hw/ipmi/pci_ipmi_kcs.c | 11 +++++++++++
>>> hw/smbios/smbios_type_38.c | 6 +++++-
>>> 7 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
>>> index 77a7213ed93..71c4efac8cd 100644
>>> --- a/include/hw/ipmi/ipmi.h
>>> +++ b/include/hw/ipmi/ipmi.h
>>> @@ -90,6 +90,11 @@ typedef struct IPMIFwInfo {
>>> } memspace;
>>>
>>> int interrupt_number;
>>> + enum {
>>> + IPMI_NO_IRQ = 0,
>>> + IPMI_ISA_IRQ,
>>> + IPMI_PCI_IRQ,
>>> + } irq;
>>> enum {
>>> IPMI_LEVEL_IRQ,
>>> IPMI_EDGE_IRQ
>>> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
>>> index a20e57d465c..c81cbd2f158 100644
>>> --- a/hw/acpi/ipmi.c
>>> +++ b/hw/acpi/ipmi.c
>>> @@ -55,7 +55,7 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>>> abort();
>>> }
>>>
>>> - if (info->interrupt_number) {
>>> + if (info->irq == IPMI_ISA_IRQ && info->interrupt_number) {
>>> aml_append(crs, aml_irq_no_flags(info->interrupt_number));
>>> }
>>>
>>> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
>>> index a1b66d5ee82..b5556436b82 100644
>>> --- a/hw/ipmi/isa_ipmi_bt.c
>>> +++ b/hw/ipmi/isa_ipmi_bt.c
>>> @@ -49,6 +49,7 @@ static void isa_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>>> ISAIPMIBTDevice *iib = ISA_IPMI_BT(ii);
>>>
>>> ipmi_bt_get_fwinfo(&iib->bt, info);
>>> + info->irq = IPMI_ISA_IRQ;
>>> info->interrupt_number = iib->isairq;
>>> info->i2c_slave_address = iib->bt.bmc->slave_addr;
>>> info->uuid = iib->uuid;
>>> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
>>> index d9ebdd5371f..326115f51bb 100644
>>> --- a/hw/ipmi/isa_ipmi_kcs.c
>>> +++ b/hw/ipmi/isa_ipmi_kcs.c
>>> @@ -49,6 +49,7 @@ static void isa_ipmi_kcs_get_fwinfo(IPMIInterface *ii, IPMIFwInfo *info)
>>> ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(ii);
>>>
>>> ipmi_kcs_get_fwinfo(&iik->kcs, info);
>>> + info->irq = IPMI_ISA_IRQ;
>>> info->interrupt_number = iik->isairq;
>>> info->uuid = iik->uuid;
>>> }
>>> diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
>>> index a3b742d22c9..33ff7190ee8 100644
>>> --- a/hw/ipmi/pci_ipmi_bt.c
>>> +++ b/hw/ipmi/pci_ipmi_bt.c
>>> @@ -38,6 +38,17 @@ struct PCIIPMIBTDevice {
>>> uint32_t uuid;
>>> };
>>>
>>> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>>> +{
>>> + PCIIPMIBTDevice *pib = PCI_IPMI_BT(ii);
>>> +
>>> + ipmi_bt_get_fwinfo(&pib->bt, info);
>>> + info->irq = IPMI_PCI_IRQ;
>>> + info->interrupt_number = pci_intx(&pib->dev);
>>> + info->i2c_slave_address = pib->bt.bmc->slave_addr;
>>> + info->uuid = pib->uuid;
>>> +}
>>> +
>>> static void pci_ipmi_raise_irq(IPMIBT *ib)
>>> {
>>> PCIIPMIBTDevice *pib = ib->opaque;
>>> @@ -125,6 +136,7 @@ static void pci_ipmi_bt_class_init(ObjectClass *oc, void *data)
>>>
>>> iic->get_backend_data = pci_ipmi_bt_get_backend_data;
>>> ipmi_bt_class_init(iic);
>>> + iic->get_fwinfo = pci_ipmi_bt_get_fwinfo;
>>> }
>>>
>>> static const TypeInfo pci_ipmi_bt_info = {
>>> diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
>>> index 05ba97ec58f..6673b2088ef 100644
>>> --- a/hw/ipmi/pci_ipmi_kcs.c
>>> +++ b/hw/ipmi/pci_ipmi_kcs.c
>>> @@ -38,6 +38,16 @@ struct PCIIPMIKCSDevice {
>>> uint32_t uuid;
>>> };
>>>
>>> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>>> +{
>>> + PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(ii);
>>> +
>>> + ipmi_kcs_get_fwinfo(&pik->kcs, info);
>>> + info->irq = IPMI_PCI_IRQ;
>>> + info->interrupt_number = pci_intx(&pik->dev);
>>> + info->uuid = pik->uuid;
>>> +}
>>> +
>>> static void pci_ipmi_raise_irq(IPMIKCS *ik)
>>> {
>>> PCIIPMIKCSDevice *pik = ik->opaque;
>>> @@ -125,6 +135,7 @@ static void pci_ipmi_kcs_class_init(ObjectClass *oc, void *data)
>>>
>>> iic->get_backend_data = pci_ipmi_kcs_get_backend_data;
>>> ipmi_kcs_class_init(iic);
>>> + iic->get_fwinfo = pci_ipmi_kcs_get_fwinfo;
>>> }
>>>
>>> static const TypeInfo pci_ipmi_kcs_info = {
>>> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
>>> index 168b886647d..2823929c258 100644
>>> --- a/hw/smbios/smbios_type_38.c
>>> +++ b/hw/smbios/smbios_type_38.c
>>> @@ -72,7 +72,11 @@ static void smbios_build_one_type_38(IPMIFwInfo *info)
>>> " SMBIOS, ignoring this entry.", info->register_spacing);
>>> return;
>>> }
>>> - t->interrupt_number = info->interrupt_number;
>>> + if (info->irq == IPMI_ISA_IRQ) {
>>> + t->interrupt_number = info->interrupt_number;
>>> + } else {
>>> + t->interrupt_number = 0;
>>
>> Can you explain why use 0 for PCI?
>
> To avoid changes to other callers after this patch. Previously PCI
> devices would leave interrupt_number as 0, not sure if such devices
> are relevant here. If there is a better approach I would take it.
OK, this is what I thought but I felt I could have missed something.
Thanks!
© 2016 - 2025 Red Hat, Inc.