[PATCH v3 0/5] ipmi: bmc-sim improvements

[PATCH v3 0/5] ipmi: bmc-sim improvements

[Nicholas Piggin]

These little things came up when looking at behaviour of IPMI with
the bmc-sim implementation running the ppc powernv machine, and
trying to clean up error messages and missing features.

Since v1 (thanks to Corey for review and suggestions):
- Added fwinfo to PCI devices
- Report interrupt number in Get Channel Info for ISA, PCI, and
  unknown/unassigned.
- Fix error reporting for Get Channel Info unsupported channels.
  Verify it is the correct error code that ipmitool looks for
  https://github.com/ipmitool/ipmitool/blob/master/lib/ipmi_channel.c#L256C16-L256C45
- Change _CH_ to _CHANNEL_ in some defines names.
- Also avoid adding event logs with watchdog don't log flag.

Since v2:
- Don't log watchog flag should not apply to watchdog expiry
  field.
- Moved protocol type field from class to IPMIFwInfo.
- Rename new FwInfo member irq to irq_source.
- Add comments about handling PCI devices to  existing callers
  of ->fwinfo

Thanks,
Nick


Nicholas Piggin (5):
  ipmi/pci-ipmi-bt: Rename copy-paste variables
  ipmi: add fwinfo to pci ipmi devices
  ipmi/bmc-sim: Add 'Get Channel Info' command
  ipmi/bmc-sim: implement watchdog dont log flag
  ipmi/bmc-sim: add error handling for 'Set BMC Global Enables' command

 include/hw/ipmi/ipmi.h     |  15 ++++++
 hw/acpi/ipmi.c             |   3 +-
 hw/ipmi/ipmi_bmc_sim.c     | 104 ++++++++++++++++++++++++++++++++-----
 hw/ipmi/ipmi_bt.c          |   2 +
 hw/ipmi/ipmi_kcs.c         |   1 +
 hw/ipmi/isa_ipmi_bt.c      |   1 +
 hw/ipmi/isa_ipmi_kcs.c     |   1 +
 hw/ipmi/pci_ipmi_bt.c      |  50 +++++++++++-------
 hw/ipmi/pci_ipmi_kcs.c     |  11 ++++
 hw/smbios/smbios_type_38.c |   7 ++-
 10 files changed, 162 insertions(+), 33 deletions(-)

-- 
2.47.1

[PATCH v3 1/5] ipmi/pci-ipmi-bt: Rename copy-paste variables

[Nicholas Piggin]

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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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

[PATCH v3 2/5] ipmi: add fwinfo to pci ipmi devices

[Nicholas Piggin]

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             |  3 ++-
 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 |  7 ++++++-
 7 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index 77a7213ed93..c8ef04856e1 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_source;
     enum {
         IPMI_LEVEL_IRQ,
         IPMI_EDGE_IRQ
diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
index a20e57d465c..39f8f2f6d68 100644
--- a/hw/acpi/ipmi.c
+++ b/hw/acpi/ipmi.c
@@ -55,7 +55,8 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
         abort();
     }
 
-    if (info->interrupt_number) {
+    /* Should PCI interrupts also be appended? */
+    if (info->irq_source == 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..4a6496b5980 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_source = 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..31d16aba74e 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_source = 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..7ba8b3ab965 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_source = 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..c817f1e8808 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_source = 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..e9b856fcd96 100644
--- a/hw/smbios/smbios_type_38.c
+++ b/hw/smbios/smbios_type_38.c
@@ -72,7 +72,12 @@ 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_source == IPMI_ISA_IRQ) {
+        t->interrupt_number = info->interrupt_number;
+    } else {
+        /* TODO: How to handle PCI? */
+        t->interrupt_number = 0;
+    }
 
     SMBIOS_BUILD_TABLE_POST;
 }
-- 
2.47.1

[PATCH v3 3/5] ipmi/bmc-sim: Add 'Get Channel Info' command

[Nicholas Piggin]

Linux issues this command when booting a powernv machine.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/hw/ipmi/ipmi.h | 10 +++++++
 hw/ipmi/ipmi_bmc_sim.c | 68 ++++++++++++++++++++++++++++++++++++++++--
 hw/ipmi/ipmi_bt.c      |  2 ++
 hw/ipmi/ipmi_kcs.c     |  1 +
 4 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index c8ef04856e1..802a2febb07 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
@@ -76,6 +85,7 @@ typedef struct IPMIFwInfo {
     int interface_type;
     uint8_t ipmi_spec_major_revision;
     uint8_t ipmi_spec_minor_revision;
+    uint8_t ipmi_channel_protocol;
     uint8_t i2c_slave_address;
     uint32_t uuid;
 
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 6157ac71201..d63f2348ba1 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,68 @@ 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);
+    IPMIFwInfo info = {};
+    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 (k->get_fwinfo) {
+        k->get_fwinfo(s, &info);
+    }
+
+    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, info.ipmi_channel_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) {
+        uint8_t irq;
+
+        if (info.irq_source == IPMI_ISA_IRQ) {
+            irq = info.interrupt_number;
+        } else if (info.irq_source == 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 +2078,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..28cf6ab2185 100644
--- a/hw/ipmi/ipmi_bt.c
+++ b/hw/ipmi/ipmi_bt.c
@@ -419,6 +419,8 @@ void ipmi_bt_get_fwinfo(struct IPMIBT *ib, IPMIFwInfo *info)
     info->interface_type = IPMI_SMBIOS_BT;
     info->ipmi_spec_major_revision = 2;
     info->ipmi_spec_minor_revision = 0;
+    /* BT System Interface Format, IPMI v1.5 */
+    info->ipmi_channel_protocol = IPMI_CHANNEL_PROTOCOL_BT_15;
     info->base_address = ib->io_base;
     info->register_length = ib->io_length;
     info->register_spacing = 1;
diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c
index c15977cab4c..578dd7cef34 100644
--- a/hw/ipmi/ipmi_kcs.c
+++ b/hw/ipmi/ipmi_kcs.c
@@ -405,6 +405,7 @@ void ipmi_kcs_get_fwinfo(IPMIKCS *ik, IPMIFwInfo *info)
     info->interface_type = IPMI_SMBIOS_KCS;
     info->ipmi_spec_major_revision = 2;
     info->ipmi_spec_minor_revision = 0;
+    info->ipmi_channel_protocol = IPMI_CHANNEL_PROTOCOL_KCS;
     info->base_address = ik->io_base;
     info->i2c_slave_address = ik->bmc->slave_addr;
     info->register_length = ik->io_length;
-- 
2.47.1

[PATCH v3 4/5] ipmi/bmc-sim: implement watchdog dont log flag

[Nicholas Piggin]

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 | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index d63f2348ba1..98cfab6a455 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:
@@ -738,24 +742,28 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *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

[PATCH v3 5/5] ipmi/bmc-sim: add error handling for 'Set BMC Global Enables' command

[Nicholas Piggin]

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 98cfab6a455..3c008a3a603 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
@@ -934,7 +935,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

Re: [PATCH v3 2/5] ipmi: add fwinfo to pci ipmi devices

[Philippe Mathieu-Daudé]

On 1/4/25 16:01, 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             |  3 ++-
>   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 |  7 ++++++-
>   7 files changed, 38 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 0/5] ipmi: bmc-sim improvements

[Corey Minyard]

On Wed, Apr 02, 2025 at 12:01:47AM +1000, Nicholas Piggin wrote:
> These little things came up when looking at behaviour of IPMI with
> the bmc-sim implementation running the ppc powernv machine, and
> trying to clean up error messages and missing features.

This all looks good to me.  Thanks to Philippe for reviewing, too.

I can give you an

Acked-by: Corey Minyard <cminyard@mvista.com>

if you are working on this for your own tree, or I can take it into
mine.

-corey

> 
> Since v1 (thanks to Corey for review and suggestions):
> - Added fwinfo to PCI devices
> - Report interrupt number in Get Channel Info for ISA, PCI, and
>   unknown/unassigned.
> - Fix error reporting for Get Channel Info unsupported channels.
>   Verify it is the correct error code that ipmitool looks for
>   https://github.com/ipmitool/ipmitool/blob/master/lib/ipmi_channel.c#L256C16-L256C45
> - Change _CH_ to _CHANNEL_ in some defines names.
> - Also avoid adding event logs with watchdog don't log flag.
> 
> Since v2:
> - Don't log watchog flag should not apply to watchdog expiry
>   field.
> - Moved protocol type field from class to IPMIFwInfo.
> - Rename new FwInfo member irq to irq_source.
> - Add comments about handling PCI devices to  existing callers
>   of ->fwinfo
> 
> Thanks,
> Nick
> 
> 
> Nicholas Piggin (5):
>   ipmi/pci-ipmi-bt: Rename copy-paste variables
>   ipmi: add fwinfo to pci ipmi devices
>   ipmi/bmc-sim: Add 'Get Channel Info' command
>   ipmi/bmc-sim: implement watchdog dont log flag
>   ipmi/bmc-sim: add error handling for 'Set BMC Global Enables' command
> 
>  include/hw/ipmi/ipmi.h     |  15 ++++++
>  hw/acpi/ipmi.c             |   3 +-
>  hw/ipmi/ipmi_bmc_sim.c     | 104 ++++++++++++++++++++++++++++++++-----
>  hw/ipmi/ipmi_bt.c          |   2 +
>  hw/ipmi/ipmi_kcs.c         |   1 +
>  hw/ipmi/isa_ipmi_bt.c      |   1 +
>  hw/ipmi/isa_ipmi_kcs.c     |   1 +
>  hw/ipmi/pci_ipmi_bt.c      |  50 +++++++++++-------
>  hw/ipmi/pci_ipmi_kcs.c     |  11 ++++
>  hw/smbios/smbios_type_38.c |   7 ++-
>  10 files changed, 162 insertions(+), 33 deletions(-)
> 
> -- 
> 2.47.1
> 

Re: [PATCH v3 0/5] ipmi: bmc-sim improvements

[Nicholas Piggin]

On Wed Apr 2, 2025 at 5:17 AM AEST, Corey Minyard wrote:
> On Wed, Apr 02, 2025 at 12:01:47AM +1000, Nicholas Piggin wrote:
>> These little things came up when looking at behaviour of IPMI with
>> the bmc-sim implementation running the ppc powernv machine, and
>> trying to clean up error messages and missing features.
>
> This all looks good to me.  Thanks to Philippe for reviewing, too.
>
> I can give you an
>
> Acked-by: Corey Minyard <cminyard@mvista.com>
>
> if you are working on this for your own tree, or I can take it into
> mine.

Hey Corey,

Thanks for all the review, and sorry I missed your question...

I don't have anything further in my tree, I don't have an
immediate need for it, it was just tidying up a few errors
and warnings I noticed.

I'd be happy for you to take it in your tree and send it up
when it suits you.

Thanks,
Nick

Re: [PATCH v3 0/5] ipmi: bmc-sim improvements

[Corey Minyard]

On Fri, Apr 11, 2025 at 04:25:10PM +1000, Nicholas Piggin wrote:
> On Wed Apr 2, 2025 at 5:17 AM AEST, Corey Minyard wrote:
> > On Wed, Apr 02, 2025 at 12:01:47AM +1000, Nicholas Piggin wrote:
> >> These little things came up when looking at behaviour of IPMI with
> >> the bmc-sim implementation running the ppc powernv machine, and
> >> trying to clean up error messages and missing features.
> >
> > This all looks good to me.  Thanks to Philippe for reviewing, too.
> >
> > I can give you an
> >
> > Acked-by: Corey Minyard <cminyard@mvista.com>
> >
> > if you are working on this for your own tree, or I can take it into
> > mine.
> 
> Hey Corey,
> 
> Thanks for all the review, and sorry I missed your question...
> 
> I don't have anything further in my tree, I don't have an
> immediate need for it, it was just tidying up a few errors
> and warnings I noticed.
> 
> I'd be happy for you to take it in your tree and send it up
> when it suits you.

Ok, it's in my tree.  I had to fix up pci_ipmi_kcs.c, it defined
pci_ipmi_bt_get_fwinfo, not pci_ipmi_kcs_get_fwinfo.

-corey

> 
> Thanks,
> Nick

Re: [PATCH v3 0/5] ipmi: bmc-sim improvements

[Nicholas Piggin]

On Sat Apr 12, 2025 at 4:20 AM AEST, Corey Minyard wrote:
> On Fri, Apr 11, 2025 at 04:25:10PM +1000, Nicholas Piggin wrote:
>> On Wed Apr 2, 2025 at 5:17 AM AEST, Corey Minyard wrote:
>> > On Wed, Apr 02, 2025 at 12:01:47AM +1000, Nicholas Piggin wrote:
>> >> These little things came up when looking at behaviour of IPMI with
>> >> the bmc-sim implementation running the ppc powernv machine, and
>> >> trying to clean up error messages and missing features.
>> >
>> > This all looks good to me.  Thanks to Philippe for reviewing, too.
>> >
>> > I can give you an
>> >
>> > Acked-by: Corey Minyard <cminyard@mvista.com>
>> >
>> > if you are working on this for your own tree, or I can take it into
>> > mine.
>> 
>> Hey Corey,
>> 
>> Thanks for all the review, and sorry I missed your question...
>> 
>> I don't have anything further in my tree, I don't have an
>> immediate need for it, it was just tidying up a few errors
>> and warnings I noticed.
>> 
>> I'd be happy for you to take it in your tree and send it up
>> when it suits you.
>
> Ok, it's in my tree.  I had to fix up pci_ipmi_kcs.c, it defined
> pci_ipmi_bt_get_fwinfo, not pci_ipmi_kcs_get_fwinfo.

Thanks, I noiced that just now too I thought I'd had a config
that built it but clearly not :( Sorry.

Thanks,
Nick