* [PATCH v2 1/5] ipmi/pci-ipmi-bt: Rename copy-paste variables
2025-04-01 11:44 [PATCH v2 0/5] ipmi: bmc-sim improvements Nicholas Piggin
@ 2025-04-01 11:44 ` Nicholas Piggin
2025-04-01 11:49 ` Philippe Mathieu-Daudé
2025-04-01 11:44 ` [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices Nicholas Piggin
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Nicholas Piggin @ 2025-04-01 11:44 UTC (permalink / raw)
To: Corey Minyard; +Cc: Nicholas Piggin, qemu-devel
IPMI drivers use p/k suffix in variable names depending on bt or kcs.
The pci bt driver must have come from the kcs driver because it's
still using k suffixes in some cases. Rename.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/ipmi/pci_ipmi_bt.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
index afeea6f3031..a3b742d22c9 100644
--- a/hw/ipmi/pci_ipmi_bt.c
+++ b/hw/ipmi/pci_ipmi_bt.c
@@ -38,49 +38,49 @@ struct PCIIPMIBTDevice {
uint32_t uuid;
};
-static void pci_ipmi_raise_irq(IPMIBT *ik)
+static void pci_ipmi_raise_irq(IPMIBT *ib)
{
- PCIIPMIBTDevice *pik = ik->opaque;
+ PCIIPMIBTDevice *pib = ib->opaque;
- pci_set_irq(&pik->dev, true);
+ pci_set_irq(&pib->dev, true);
}
-static void pci_ipmi_lower_irq(IPMIBT *ik)
+static void pci_ipmi_lower_irq(IPMIBT *ib)
{
- PCIIPMIBTDevice *pik = ik->opaque;
+ PCIIPMIBTDevice *pib = ib->opaque;
- pci_set_irq(&pik->dev, false);
+ pci_set_irq(&pib->dev, false);
}
static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
{
Error *err = NULL;
- PCIIPMIBTDevice *pik = PCI_IPMI_BT(pd);
+ PCIIPMIBTDevice *pib = PCI_IPMI_BT(pd);
IPMIInterface *ii = IPMI_INTERFACE(pd);
IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
- if (!pik->bt.bmc) {
+ if (!pib->bt.bmc) {
error_setg(errp, "IPMI device requires a bmc attribute to be set");
return;
}
- pik->uuid = ipmi_next_uuid();
+ pib->uuid = ipmi_next_uuid();
- pik->bt.bmc->intf = ii;
- pik->bt.opaque = pik;
+ pib->bt.bmc->intf = ii;
+ pib->bt.opaque = pib;
pci_config_set_prog_interface(pd->config, 0x02); /* BT */
pci_config_set_interrupt_pin(pd->config, 0x01);
- pik->bt.use_irq = 1;
- pik->bt.raise_irq = pci_ipmi_raise_irq;
- pik->bt.lower_irq = pci_ipmi_lower_irq;
+ pib->bt.use_irq = 1;
+ pib->bt.raise_irq = pci_ipmi_raise_irq;
+ pib->bt.lower_irq = pci_ipmi_lower_irq;
iic->init(ii, 8, &err);
if (err) {
error_propagate(errp, err);
return;
}
- pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, &pik->bt.io);
+ pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, &pib->bt.io);
}
const VMStateDescription vmstate_PCIIPMIBTDevice = {
@@ -96,16 +96,16 @@ const VMStateDescription vmstate_PCIIPMIBTDevice = {
static void pci_ipmi_bt_instance_init(Object *obj)
{
- PCIIPMIBTDevice *pik = PCI_IPMI_BT(obj);
+ PCIIPMIBTDevice *pib = PCI_IPMI_BT(obj);
- ipmi_bmc_find_and_link(obj, (Object **) &pik->bt.bmc);
+ ipmi_bmc_find_and_link(obj, (Object **) &pib->bt.bmc);
}
static void *pci_ipmi_bt_get_backend_data(IPMIInterface *ii)
{
- PCIIPMIBTDevice *pik = PCI_IPMI_BT(ii);
+ PCIIPMIBTDevice *pib = PCI_IPMI_BT(ii);
- return &pik->bt;
+ return &pib->bt;
}
static void pci_ipmi_bt_class_init(ObjectClass *oc, void *data)
--
2.47.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/5] ipmi/pci-ipmi-bt: Rename copy-paste variables
2025-04-01 11:44 ` [PATCH v2 1/5] ipmi/pci-ipmi-bt: Rename copy-paste variables Nicholas Piggin
@ 2025-04-01 11:49 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-01 11:49 UTC (permalink / raw)
To: Nicholas Piggin, Corey Minyard; +Cc: qemu-devel
On 1/4/25 13:44, Nicholas Piggin wrote:
> IPMI drivers use p/k suffix in variable names depending on bt or kcs.
> The pci bt driver must have come from the kcs driver because it's
> still using k suffixes in some cases. Rename.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/ipmi/pci_ipmi_bt.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices
2025-04-01 11:44 [PATCH v2 0/5] ipmi: bmc-sim improvements Nicholas Piggin
2025-04-01 11:44 ` [PATCH v2 1/5] ipmi/pci-ipmi-bt: Rename copy-paste variables Nicholas Piggin
@ 2025-04-01 11:44 ` Nicholas Piggin
2025-04-01 11:57 ` Philippe Mathieu-Daudé
2025-04-01 13:09 ` Corey Minyard
2025-04-01 11:44 ` [PATCH v2 3/5] ipmi/bmc-sim: Add 'Get Channel Info' command Nicholas Piggin
` (2 subsequent siblings)
4 siblings, 2 replies; 14+ messages in thread
From: Nicholas Piggin @ 2025-04-01 11:44 UTC (permalink / raw)
To: Corey Minyard; +Cc: Nicholas Piggin, qemu-devel
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.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices
2025-04-01 11:44 ` [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices Nicholas Piggin
@ 2025-04-01 11:57 ` Philippe Mathieu-Daudé
2025-04-01 13:27 ` Nicholas Piggin
2025-04-01 13:09 ` Corey Minyard
1 sibling, 1 reply; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-01 11:57 UTC (permalink / raw)
To: Nicholas Piggin, Corey Minyard; +Cc: qemu-devel
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;
> }
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices
2025-04-01 11:57 ` Philippe Mathieu-Daudé
@ 2025-04-01 13:27 ` Nicholas Piggin
2025-04-01 14:55 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 14+ messages in thread
From: Nicholas Piggin @ 2025-04-01 13:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Corey Minyard; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices
2025-04-01 13:27 ` Nicholas Piggin
@ 2025-04-01 14:55 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-01 14:55 UTC (permalink / raw)
To: Nicholas Piggin, Corey Minyard; +Cc: qemu-devel
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!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices
2025-04-01 11:44 ` [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices Nicholas Piggin
2025-04-01 11:57 ` Philippe Mathieu-Daudé
@ 2025-04-01 13:09 ` Corey Minyard
2025-04-01 13:29 ` Nicholas Piggin
1 sibling, 1 reply; 14+ messages in thread
From: Corey Minyard @ 2025-04-01 13:09 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Corey Minyard, qemu-devel
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?
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices
2025-04-01 13:09 ` Corey Minyard
@ 2025-04-01 13:29 ` Nicholas Piggin
0 siblings, 0 replies; 14+ messages in thread
From: Nicholas Piggin @ 2025-04-01 13:29 UTC (permalink / raw)
To: corey; +Cc: Corey Minyard, qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/5] ipmi/bmc-sim: Add 'Get Channel Info' command
2025-04-01 11:44 [PATCH v2 0/5] ipmi: bmc-sim improvements Nicholas Piggin
2025-04-01 11:44 ` [PATCH v2 1/5] ipmi/pci-ipmi-bt: Rename copy-paste variables Nicholas Piggin
2025-04-01 11:44 ` [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices Nicholas Piggin
@ 2025-04-01 11:44 ` Nicholas Piggin
2025-04-01 11:44 ` [PATCH v2 4/5] ipmi/bmc-sim: implement watchdog dont log flag Nicholas Piggin
2025-04-01 11:44 ` [PATCH v2 5/5] ipmi/bmc-sim: add error handling for 'Set BMC Global Enables' command Nicholas Piggin
4 siblings, 0 replies; 14+ messages in thread
From: Nicholas Piggin @ 2025-04-01 11:44 UTC (permalink / raw)
To: Corey Minyard; +Cc: Nicholas Piggin, qemu-devel
Linux issues this command when booting a powernv machine.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/ipmi/ipmi.h | 14 +++++++++
hw/ipmi/ipmi_bmc_sim.c | 65 ++++++++++++++++++++++++++++++++++++++++--
hw/ipmi/ipmi_bt.c | 2 ++
hw/ipmi/ipmi_kcs.c | 1 +
4 files changed, 80 insertions(+), 2 deletions(-)
diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index 71c4efac8cd..d1af660012a 100644
--- a/include/hw/ipmi/ipmi.h
+++ b/include/hw/ipmi/ipmi.h
@@ -41,6 +41,15 @@ enum ipmi_op {
IPMI_SEND_NMI
};
+/* Channel properties */
+#define IPMI_CHANNEL_IPMB 0x00
+#define IPMI_CHANNEL_SYSTEM 0x0f
+#define IPMI_CHANNEL_MEDIUM_IPMB 0x01
+#define IPMI_CHANNEL_MEDIUM_SYSTEM 0x0c
+#define IPMI_CHANNEL_PROTOCOL_IPMB 0x01
+#define IPMI_CHANNEL_PROTOCOL_KCS 0x05
+#define IPMI_CHANNEL_PROTOCOL_BT_15 0x08
+
#define IPMI_CC_INVALID_CMD 0xc1
#define IPMI_CC_COMMAND_INVALID_FOR_LUN 0xc2
#define IPMI_CC_TIMEOUT 0xc3
@@ -175,6 +184,11 @@ struct IPMIInterfaceClass {
* Return the firmware info for a device.
*/
void (*get_fwinfo)(struct IPMIInterface *s, IPMIFwInfo *info);
+
+ /*
+ * IPMI channel protocol type number.
+ */
+ uint8_t protocol;
};
/*
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 6157ac71201..216bf8ff7f0 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -70,6 +70,7 @@
#define IPMI_CMD_GET_MSG 0x33
#define IPMI_CMD_SEND_MSG 0x34
#define IPMI_CMD_READ_EVT_MSG_BUF 0x35
+#define IPMI_CMD_GET_CHANNEL_INFO 0x42
#define IPMI_NETFN_STORAGE 0x0a
@@ -1020,8 +1021,8 @@ static void send_msg(IPMIBmcSim *ibs,
uint8_t *buf;
uint8_t netfn, rqLun, rsLun, rqSeq;
- if (cmd[2] != 0) {
- /* We only handle channel 0 with no options */
+ if (cmd[2] != IPMI_CHANNEL_IPMB) {
+ /* We only handle channel 0h (IPMB) with no options */
rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
return;
}
@@ -1219,6 +1220,65 @@ static void get_watchdog_timer(IPMIBmcSim *ibs,
}
}
+static void get_channel_info(IPMIBmcSim *ibs,
+ uint8_t *cmd, unsigned int cmd_len,
+ RspBuffer *rsp)
+{
+ IPMIInterface *s = ibs->parent.intf;
+ IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
+ uint8_t ch = cmd[2] & 0x0f;
+
+ /* Only define channel 0h (IPMB) and Fh (system interface) */
+
+ if (ch == 0x0e) { /* "This channel" */
+ ch = IPMI_CHANNEL_SYSTEM;
+ }
+ rsp_buffer_push(rsp, ch);
+
+ if (ch != IPMI_CHANNEL_IPMB && ch != IPMI_CHANNEL_SYSTEM) {
+ /* Not a supported channel */
+ rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+ return;
+ }
+
+ if (ch == IPMI_CHANNEL_IPMB) {
+ rsp_buffer_push(rsp, IPMI_CHANNEL_MEDIUM_IPMB);
+ rsp_buffer_push(rsp, IPMI_CHANNEL_PROTOCOL_IPMB);
+ } else { /* IPMI_CHANNEL_SYSTEM */
+ rsp_buffer_push(rsp, IPMI_CHANNEL_MEDIUM_SYSTEM);
+ rsp_buffer_push(rsp, k->protocol);
+ }
+
+ rsp_buffer_push(rsp, 0x00); /* Session-less */
+
+ /* IPMI Enterprise Number for Vendor ID */
+ rsp_buffer_push(rsp, 0xf2);
+ rsp_buffer_push(rsp, 0x1b);
+ rsp_buffer_push(rsp, 0x00);
+
+ if (ch == IPMI_CHANNEL_SYSTEM) {
+ IPMIFwInfo info = {};
+ uint8_t irq;
+
+ k->get_fwinfo(s, &info);
+ if (info.irq == IPMI_ISA_IRQ) {
+ irq = info.interrupt_number;
+ } else if (info.irq == IPMI_PCI_IRQ) {
+ irq = 0x10 + info.interrupt_number;
+ } else {
+ irq = 0xff; /* no interrupt / unspecified */
+ }
+
+ /* Both interrupts use the same irq number */
+ rsp_buffer_push(rsp, irq);
+ rsp_buffer_push(rsp, irq);
+ } else {
+ /* Reserved */
+ rsp_buffer_push(rsp, 0x00);
+ rsp_buffer_push(rsp, 0x00);
+ }
+}
+
static void get_sdr_rep_info(IPMIBmcSim *ibs,
uint8_t *cmd, unsigned int cmd_len,
RspBuffer *rsp)
@@ -2015,6 +2075,7 @@ static const IPMICmdHandler app_cmds[] = {
[IPMI_CMD_RESET_WATCHDOG_TIMER] = { reset_watchdog_timer },
[IPMI_CMD_SET_WATCHDOG_TIMER] = { set_watchdog_timer, 8 },
[IPMI_CMD_GET_WATCHDOG_TIMER] = { get_watchdog_timer },
+ [IPMI_CMD_GET_CHANNEL_INFO] = { get_channel_info, 3 },
};
static const IPMINetfn app_netfn = {
.cmd_nums = ARRAY_SIZE(app_cmds),
diff --git a/hw/ipmi/ipmi_bt.c b/hw/ipmi/ipmi_bt.c
index 583fc64730c..79311c41f2b 100644
--- a/hw/ipmi/ipmi_bt.c
+++ b/hw/ipmi/ipmi_bt.c
@@ -434,4 +434,6 @@ void ipmi_bt_class_init(IPMIInterfaceClass *iic)
iic->handle_if_event = ipmi_bt_handle_event;
iic->set_irq_enable = ipmi_bt_set_irq_enable;
iic->reset = ipmi_bt_handle_reset;
+ /* BT System Interface Format, IPMI v1.5 */
+ iic->protocol = IPMI_CHANNEL_PROTOCOL_BT_15;
}
diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c
index c15977cab4c..618a33c581f 100644
--- a/hw/ipmi/ipmi_kcs.c
+++ b/hw/ipmi/ipmi_kcs.c
@@ -420,4 +420,5 @@ void ipmi_kcs_class_init(IPMIInterfaceClass *iic)
iic->handle_rsp = ipmi_kcs_handle_rsp;
iic->handle_if_event = ipmi_kcs_handle_event;
iic->set_irq_enable = ipmi_kcs_set_irq_enable;
+ iic->protocol = IPMI_CHANNEL_PROTOCOL_KCS;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 4/5] ipmi/bmc-sim: implement watchdog dont log flag
2025-04-01 11:44 [PATCH v2 0/5] ipmi: bmc-sim improvements Nicholas Piggin
` (2 preceding siblings ...)
2025-04-01 11:44 ` [PATCH v2 3/5] ipmi/bmc-sim: Add 'Get Channel Info' command Nicholas Piggin
@ 2025-04-01 11:44 ` Nicholas Piggin
2025-04-01 13:05 ` Corey Minyard
2025-04-01 11:44 ` [PATCH v2 5/5] ipmi/bmc-sim: add error handling for 'Set BMC Global Enables' command Nicholas Piggin
4 siblings, 1 reply; 14+ messages in thread
From: Nicholas Piggin @ 2025-04-01 11:44 UTC (permalink / raw)
To: Corey Minyard; +Cc: Nicholas Piggin, qemu-devel
If the dont-log flag is set in the 'timer use' field for the
'set watchdog' command, a watchdog timeout will not get logged as
a timer use expiration.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/ipmi/ipmi_bmc_sim.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 216bf8ff7f0..3cefc69f8a8 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -514,7 +514,8 @@ static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert,
static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
unsigned int bit, unsigned int val,
- uint8_t evd1, uint8_t evd2, uint8_t evd3)
+ uint8_t evd1, uint8_t evd2, uint8_t evd3,
+ bool do_log)
{
IPMISensor *sens;
uint16_t mask;
@@ -534,7 +535,7 @@ static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
return; /* Already asserted */
}
sens->assert_states |= mask & sens->assert_suppt;
- if (sens->assert_enable & mask & sens->assert_states) {
+ if (do_log && (sens->assert_enable & mask & sens->assert_states)) {
/* Send an event on assert */
gen_event(ibs, sensor, 0, evd1, evd2, evd3);
}
@@ -544,7 +545,7 @@ static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
return; /* Already deasserted */
}
sens->deassert_states |= mask & sens->deassert_suppt;
- if (sens->deassert_enable & mask & sens->deassert_states) {
+ if (do_log && (sens->deassert_enable & mask & sens->deassert_states)) {
/* Send an event on deassert */
gen_event(ibs, sensor, 1, evd1, evd2, evd3);
}
@@ -700,6 +701,7 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
{
IPMIInterface *s = ibs->parent.intf;
IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
+ bool do_log = !IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs);
if (!ibs->watchdog_running) {
goto out;
@@ -711,14 +713,16 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
ibs->msg_flags |= IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK;
k->do_hw_op(s, IPMI_SEND_NMI, 0);
sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 8, 1,
- 0xc8, (2 << 4) | 0xf, 0xff);
+ 0xc8, (2 << 4) | 0xf, 0xff,
+ do_log);
break;
case IPMI_BMC_WATCHDOG_PRE_MSG_INT:
ibs->msg_flags |= IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK;
k->set_atn(s, 1, attn_irq_enabled(ibs));
sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 8, 1,
- 0xc8, (3 << 4) | 0xf, 0xff);
+ 0xc8, (3 << 4) | 0xf, 0xff,
+ do_log);
break;
default:
@@ -734,28 +738,36 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
do_full_expiry:
ibs->watchdog_running = 0; /* Stop the watchdog on a timeout */
- ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
+
+ if (do_log) {
+ ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
+ }
+
switch (IPMI_BMC_WATCHDOG_GET_ACTION(ibs)) {
case IPMI_BMC_WATCHDOG_ACTION_NONE:
sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 0, 1,
- 0xc0, ibs->watchdog_use & 0xf, 0xff);
+ 0xc0, ibs->watchdog_use & 0xf, 0xff,
+ do_log);
break;
case IPMI_BMC_WATCHDOG_ACTION_RESET:
sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 1, 1,
- 0xc1, ibs->watchdog_use & 0xf, 0xff);
+ 0xc1, ibs->watchdog_use & 0xf, 0xff,
+ do_log);
k->do_hw_op(s, IPMI_RESET_CHASSIS, 0);
break;
case IPMI_BMC_WATCHDOG_ACTION_POWER_DOWN:
sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 2, 1,
- 0xc2, ibs->watchdog_use & 0xf, 0xff);
+ 0xc2, ibs->watchdog_use & 0xf, 0xff,
+ do_log);
k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
break;
case IPMI_BMC_WATCHDOG_ACTION_POWER_CYCLE:
sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 2, 1,
- 0xc3, ibs->watchdog_use & 0xf, 0xff);
+ 0xc3, ibs->watchdog_use & 0xf, 0xff,
+ do_log);
k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 0);
break;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 4/5] ipmi/bmc-sim: implement watchdog dont log flag
2025-04-01 11:44 ` [PATCH v2 4/5] ipmi/bmc-sim: implement watchdog dont log flag Nicholas Piggin
@ 2025-04-01 13:05 ` Corey Minyard
2025-04-01 13:28 ` Nicholas Piggin
0 siblings, 1 reply; 14+ messages in thread
From: Corey Minyard @ 2025-04-01 13:05 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Corey Minyard, qemu-devel
On Tue, Apr 01, 2025 at 09:44:11PM +1000, Nicholas Piggin wrote:
> If the dont-log flag is set in the 'timer use' field for the
> 'set watchdog' command, a watchdog timeout will not get logged as
> a timer use expiration.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/ipmi/ipmi_bmc_sim.c | 32 ++++++++++++++++++++++----------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 216bf8ff7f0..3cefc69f8a8 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -514,7 +514,8 @@ static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert,
>
> static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
> unsigned int bit, unsigned int val,
> - uint8_t evd1, uint8_t evd2, uint8_t evd3)
> + uint8_t evd1, uint8_t evd2, uint8_t evd3,
> + bool do_log)
> {
> IPMISensor *sens;
> uint16_t mask;
> @@ -534,7 +535,7 @@ static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
> return; /* Already asserted */
> }
> sens->assert_states |= mask & sens->assert_suppt;
> - if (sens->assert_enable & mask & sens->assert_states) {
> + if (do_log && (sens->assert_enable & mask & sens->assert_states)) {
> /* Send an event on assert */
> gen_event(ibs, sensor, 0, evd1, evd2, evd3);
> }
> @@ -544,7 +545,7 @@ static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
> return; /* Already deasserted */
> }
> sens->deassert_states |= mask & sens->deassert_suppt;
> - if (sens->deassert_enable & mask & sens->deassert_states) {
> + if (do_log && (sens->deassert_enable & mask & sens->deassert_states)) {
> /* Send an event on deassert */
> gen_event(ibs, sensor, 1, evd1, evd2, evd3);
> }
> @@ -700,6 +701,7 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
> {
> IPMIInterface *s = ibs->parent.intf;
> IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
> + bool do_log = !IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs);
>
> if (!ibs->watchdog_running) {
> goto out;
> @@ -711,14 +713,16 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
> ibs->msg_flags |= IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK;
> k->do_hw_op(s, IPMI_SEND_NMI, 0);
> sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 8, 1,
> - 0xc8, (2 << 4) | 0xf, 0xff);
> + 0xc8, (2 << 4) | 0xf, 0xff,
> + do_log);
> break;
>
> case IPMI_BMC_WATCHDOG_PRE_MSG_INT:
> ibs->msg_flags |= IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK;
> k->set_atn(s, 1, attn_irq_enabled(ibs));
> sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 8, 1,
> - 0xc8, (3 << 4) | 0xf, 0xff);
> + 0xc8, (3 << 4) | 0xf, 0xff,
> + do_log);
> break;
>
> default:
> @@ -734,28 +738,36 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
>
> do_full_expiry:
> ibs->watchdog_running = 0; /* Stop the watchdog on a timeout */
> - ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
> +
> + if (do_log) {
> + ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
> + }
> +
This change needs to be removed. watchdog_expired has nothing to do
with logs, it's a field reporting in another message.
Other than that, this is good.
-corey
> switch (IPMI_BMC_WATCHDOG_GET_ACTION(ibs)) {
> case IPMI_BMC_WATCHDOG_ACTION_NONE:
> sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 0, 1,
> - 0xc0, ibs->watchdog_use & 0xf, 0xff);
> + 0xc0, ibs->watchdog_use & 0xf, 0xff,
> + do_log);
> break;
>
> case IPMI_BMC_WATCHDOG_ACTION_RESET:
> sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 1, 1,
> - 0xc1, ibs->watchdog_use & 0xf, 0xff);
> + 0xc1, ibs->watchdog_use & 0xf, 0xff,
> + do_log);
> k->do_hw_op(s, IPMI_RESET_CHASSIS, 0);
> break;
>
> case IPMI_BMC_WATCHDOG_ACTION_POWER_DOWN:
> sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 2, 1,
> - 0xc2, ibs->watchdog_use & 0xf, 0xff);
> + 0xc2, ibs->watchdog_use & 0xf, 0xff,
> + do_log);
> k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
> break;
>
> case IPMI_BMC_WATCHDOG_ACTION_POWER_CYCLE:
> sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 2, 1,
> - 0xc3, ibs->watchdog_use & 0xf, 0xff);
> + 0xc3, ibs->watchdog_use & 0xf, 0xff,
> + do_log);
> k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 0);
> break;
> }
> --
> 2.47.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 4/5] ipmi/bmc-sim: implement watchdog dont log flag
2025-04-01 13:05 ` Corey Minyard
@ 2025-04-01 13:28 ` Nicholas Piggin
0 siblings, 0 replies; 14+ messages in thread
From: Nicholas Piggin @ 2025-04-01 13:28 UTC (permalink / raw)
To: corey; +Cc: Corey Minyard, qemu-devel
On Tue Apr 1, 2025 at 11:05 PM AEST, Corey Minyard wrote:
> On Tue, Apr 01, 2025 at 09:44:11PM +1000, Nicholas Piggin wrote:
>> If the dont-log flag is set in the 'timer use' field for the
>> 'set watchdog' command, a watchdog timeout will not get logged as
>> a timer use expiration.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> hw/ipmi/ipmi_bmc_sim.c | 32 ++++++++++++++++++++++----------
>> 1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index 216bf8ff7f0..3cefc69f8a8 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -514,7 +514,8 @@ static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert,
>>
>> static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
>> unsigned int bit, unsigned int val,
>> - uint8_t evd1, uint8_t evd2, uint8_t evd3)
>> + uint8_t evd1, uint8_t evd2, uint8_t evd3,
>> + bool do_log)
>> {
>> IPMISensor *sens;
>> uint16_t mask;
>> @@ -534,7 +535,7 @@ static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
>> return; /* Already asserted */
>> }
>> sens->assert_states |= mask & sens->assert_suppt;
>> - if (sens->assert_enable & mask & sens->assert_states) {
>> + if (do_log && (sens->assert_enable & mask & sens->assert_states)) {
>> /* Send an event on assert */
>> gen_event(ibs, sensor, 0, evd1, evd2, evd3);
>> }
>> @@ -544,7 +545,7 @@ static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
>> return; /* Already deasserted */
>> }
>> sens->deassert_states |= mask & sens->deassert_suppt;
>> - if (sens->deassert_enable & mask & sens->deassert_states) {
>> + if (do_log && (sens->deassert_enable & mask & sens->deassert_states)) {
>> /* Send an event on deassert */
>> gen_event(ibs, sensor, 1, evd1, evd2, evd3);
>> }
>> @@ -700,6 +701,7 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
>> {
>> IPMIInterface *s = ibs->parent.intf;
>> IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>> + bool do_log = !IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs);
>>
>> if (!ibs->watchdog_running) {
>> goto out;
>> @@ -711,14 +713,16 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
>> ibs->msg_flags |= IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK;
>> k->do_hw_op(s, IPMI_SEND_NMI, 0);
>> sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 8, 1,
>> - 0xc8, (2 << 4) | 0xf, 0xff);
>> + 0xc8, (2 << 4) | 0xf, 0xff,
>> + do_log);
>> break;
>>
>> case IPMI_BMC_WATCHDOG_PRE_MSG_INT:
>> ibs->msg_flags |= IPMI_BMC_MSG_FLAG_WATCHDOG_TIMEOUT_MASK;
>> k->set_atn(s, 1, attn_irq_enabled(ibs));
>> sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 8, 1,
>> - 0xc8, (3 << 4) | 0xf, 0xff);
>> + 0xc8, (3 << 4) | 0xf, 0xff,
>> + do_log);
>> break;
>>
>> default:
>> @@ -734,28 +738,36 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
>>
>> do_full_expiry:
>> ibs->watchdog_running = 0; /* Stop the watchdog on a timeout */
>> - ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
>> +
>> + if (do_log) {
>> + ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
>> + }
>> +
>
> This change needs to be removed. watchdog_expired has nothing to do
> with logs, it's a field reporting in another message.
Okay you're right, I read through the spec again and yes I was
mistaken on this field.
Thanks,
Nick
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 5/5] ipmi/bmc-sim: add error handling for 'Set BMC Global Enables' command
2025-04-01 11:44 [PATCH v2 0/5] ipmi: bmc-sim improvements Nicholas Piggin
` (3 preceding siblings ...)
2025-04-01 11:44 ` [PATCH v2 4/5] ipmi/bmc-sim: implement watchdog dont log flag Nicholas Piggin
@ 2025-04-01 11:44 ` Nicholas Piggin
4 siblings, 0 replies; 14+ messages in thread
From: Nicholas Piggin @ 2025-04-01 11:44 UTC (permalink / raw)
To: Corey Minyard; +Cc: Nicholas Piggin, qemu-devel
Mask out unsupported bits and return failure if attempting to set
any. This is not required by the IPMI spec, but it does require that
system software not change bits it isn't aware of.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/ipmi/ipmi_bmc_sim.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 3cefc69f8a8..794b9ed5b12 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -235,6 +235,7 @@ struct IPMIBmcSim {
#define IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE_SET(s) \
(IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE & (s)->msg_flags)
+#define IPMI_BMC_GLOBAL_ENABLES_SUPPORTED 0x0f
#define IPMI_BMC_RCV_MSG_QUEUE_INT_BIT 0
#define IPMI_BMC_EVBUF_FULL_INT_BIT 1
#define IPMI_BMC_EVENT_MSG_BUF_BIT 2
@@ -938,7 +939,14 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs,
uint8_t *cmd, unsigned int cmd_len,
RspBuffer *rsp)
{
- set_global_enables(ibs, cmd[2]);
+ uint8_t val = cmd[2];
+
+ if (val & ~IPMI_BMC_GLOBAL_ENABLES_SUPPORTED) {
+ rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+ return;
+ }
+
+ set_global_enables(ibs, val);
}
static void get_bmc_global_enables(IPMIBmcSim *ibs,
--
2.47.1
^ permalink raw reply related [flat|nested] 14+ messages in thread