[PATCH 0/3] ipmi: bmc-sim improvements

[PATCH 0/3] 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.

Thanks,
Nick

Nicholas Piggin (3):
  ipmi/bmc-sim: implement watchdog dont log flag
  ipmi/bmc-sim: add error handling for 'Set BMC Global Enables' command
  ipmi/bmc-sim: Add 'Get Channel Info' command

 include/hw/ipmi/ipmi.h | 14 ++++++++
 hw/ipmi/ipmi_bmc_sim.c | 73 +++++++++++++++++++++++++++++++++++++++---
 hw/ipmi/ipmi_bt.c      |  2 ++
 hw/ipmi/ipmi_kcs.c     |  1 +
 4 files changed, 86 insertions(+), 4 deletions(-)

-- 
2.47.1

[PATCH 1/3] 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 6157ac71201..32161044c0b 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -733,7 +733,12 @@ 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));
+
+    /* Log the expiry if the don't log bit is clear */
+    if (!IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs)) {
+        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,
-- 
2.47.1

[PATCH 2/3] 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 32161044c0b..8c3313aa65f 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -234,6 +234,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
@@ -930,7 +931,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

[PATCH 3/3] 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 | 14 +++++++++++
 hw/ipmi/ipmi_bmc_sim.c | 56 ++++++++++++++++++++++++++++++++++++++++--
 hw/ipmi/ipmi_bt.c      |  2 ++
 hw/ipmi/ipmi_kcs.c     |  1 +
 4 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index 77a7213ed93..5f01a50cd86 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_CH_MEDIUM_IPMB              0x01
+#define IPMI_CH_MEDIUM_SYSTEM            0x0c
+#define IPMI_CH_PROTOCOL_IPMB            0x01
+#define IPMI_CH_PROTOCOL_KCS             0x05
+#define IPMI_CH_PROTOCOL_BT_15           0x08
+
 #define IPMI_CC_INVALID_CMD                              0xc1
 #define IPMI_CC_COMMAND_INVALID_FOR_LUN                  0xc2
 #define IPMI_CC_TIMEOUT                                  0xc3
@@ -170,6 +179,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 8c3313aa65f..9198f854bd9 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
 
@@ -1033,8 +1034,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;
     }
@@ -1232,6 +1233,56 @@ 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[1] & 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 supported */
+        int i;
+        for (i = 0; i < 8; i++) {
+            rsp_buffer_push(rsp, 0x00);
+        }
+        return;
+    }
+
+    if (ch == IPMI_CHANNEL_IPMB) {
+        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_IPMB);
+        rsp_buffer_push(rsp, IPMI_CH_PROTOCOL_IPMB);
+    } else { /* IPMI_CHANNEL_SYSTEM */
+        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_SYSTEM);
+        rsp_buffer_push(rsp, k->protocol);
+    }
+
+    rsp_buffer_push(rsp, 0x00); /* Session-less */
+
+    /* IPMI Vendor ID */
+    rsp_buffer_push(rsp, 0xf2);
+    rsp_buffer_push(rsp, 0x1b);
+    rsp_buffer_push(rsp, 0x00);
+
+    if (ch == IPMI_CHANNEL_SYSTEM) {
+        /* IRQ assigned by ACPI/PnP (XXX?) */
+        rsp_buffer_push(rsp, 0x60);
+        rsp_buffer_push(rsp, 0x60);
+    } 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)
@@ -2028,6 +2079,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..d639c151c4d 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_CH_PROTOCOL_BT_15;
 }
diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c
index c15977cab4c..8af7698286d 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_CH_PROTOCOL_KCS;
 }
-- 
2.47.1

Re: [PATCH 1/3] ipmi/bmc-sim: implement watchdog dont log flag

[Corey Minyard]

On Mon, Mar 31, 2025 at 10:57:22PM +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 | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 6157ac71201..32161044c0b 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -733,7 +733,12 @@ 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));
> +
> +    /* Log the expiry if the don't log bit is clear */
> +    if (!IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs)) {
> +        ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
> +    }
> +

Are you sure this is correct?  The spec doesn't say what this means, but
I would assume this means "Don't add a system log" not "Don't set the
expiry happened bit".

>      switch (IPMI_BMC_WATCHDOG_GET_ACTION(ibs)) {
>      case IPMI_BMC_WATCHDOG_ACTION_NONE:
>          sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 0, 1,
> -- 
> 2.47.1
> 

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

[Corey Minyard]

On Mon, Mar 31, 2025 at 10:57:24PM +1000, Nicholas Piggin wrote:
> Linux issues this command when booting a powernv machine.

This is good, just a couple of nits.

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  include/hw/ipmi/ipmi.h | 14 +++++++++++
>  hw/ipmi/ipmi_bmc_sim.c | 56 ++++++++++++++++++++++++++++++++++++++++--
>  hw/ipmi/ipmi_bt.c      |  2 ++
>  hw/ipmi/ipmi_kcs.c     |  1 +
>  4 files changed, 71 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
> index 77a7213ed93..5f01a50cd86 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_CH_MEDIUM_IPMB              0x01
> +#define IPMI_CH_MEDIUM_SYSTEM            0x0c
> +#define IPMI_CH_PROTOCOL_IPMB            0x01
> +#define IPMI_CH_PROTOCOL_KCS             0x05
> +#define IPMI_CH_PROTOCOL_BT_15           0x08

I know it's picky, but could you spell out CHANNEL here?

> +
>  #define IPMI_CC_INVALID_CMD                              0xc1
>  #define IPMI_CC_COMMAND_INVALID_FOR_LUN                  0xc2
>  #define IPMI_CC_TIMEOUT                                  0xc3
> @@ -170,6 +179,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 8c3313aa65f..9198f854bd9 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
>  
> @@ -1033,8 +1034,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;
>      }
> @@ -1232,6 +1233,56 @@ 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[1] & 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 supported */

I think that an all zero response is a valid response.  I think you
should return a IPMI_CC_INVALID_DATA_FIELD instead, right?

> +        int i;
> +        for (i = 0; i < 8; i++) {
> +            rsp_buffer_push(rsp, 0x00);
> +        }
> +        return;
> +    }
> +
> +    if (ch == IPMI_CHANNEL_IPMB) {
> +        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_IPMB);
> +        rsp_buffer_push(rsp, IPMI_CH_PROTOCOL_IPMB);
> +    } else { /* IPMI_CHANNEL_SYSTEM */
> +        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_SYSTEM);
> +        rsp_buffer_push(rsp, k->protocol);
> +    }
> +
> +    rsp_buffer_push(rsp, 0x00); /* Session-less */
> +
> +    /* IPMI Vendor ID */
> +    rsp_buffer_push(rsp, 0xf2);
> +    rsp_buffer_push(rsp, 0x1b);
> +    rsp_buffer_push(rsp, 0x00);

Where does this come from?

> +
> +    if (ch == IPMI_CHANNEL_SYSTEM) {
> +        /* IRQ assigned by ACPI/PnP (XXX?) */
> +        rsp_buffer_push(rsp, 0x60);
> +        rsp_buffer_push(rsp, 0x60);

The interrupt should be available.  For the isa versions there is a
get_fwinfo function pointer that you can fetch this with.  For PCI it's
more complicated, unfortunately.

-corey

> +    } 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)
> @@ -2028,6 +2079,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..d639c151c4d 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_CH_PROTOCOL_BT_15;
>  }
> diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c
> index c15977cab4c..8af7698286d 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_CH_PROTOCOL_KCS;
>  }
> -- 
> 2.47.1
> 

Re: [PATCH 1/3] ipmi/bmc-sim: implement watchdog dont log flag

[Nicholas Piggin]

On Mon Mar 31, 2025 at 11:13 PM AEST, Corey Minyard wrote:
> On Mon, Mar 31, 2025 at 10:57:22PM +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 | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index 6157ac71201..32161044c0b 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -733,7 +733,12 @@ 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));
>> +
>> +    /* Log the expiry if the don't log bit is clear */
>> +    if (!IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs)) {
>> +        ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
>> +    }
>> +
>
> Are you sure this is correct?  The spec doesn't say what this means, but
> I would assume this means "Don't add a system log" not "Don't set the
> expiry happened bit".

From IPMI spec, Set Watchdog Timer command timer use field of byte 1
says "timer use (logged on expiration when “don’t log” bit = 0b)".
But it also says it should disable the timeout sensor event logging.
I missed that part, I will see if I can make that work.

Thanks,
Nick

Re: [PATCH 1/3] ipmi/bmc-sim: implement watchdog dont log flag

[Corey Minyard]

On Tue, Apr 01, 2025 at 08:37:19AM +1000, Nicholas Piggin wrote:
> On Mon Mar 31, 2025 at 11:13 PM AEST, Corey Minyard wrote:
> > On Mon, Mar 31, 2025 at 10:57:22PM +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 | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> >> index 6157ac71201..32161044c0b 100644
> >> --- a/hw/ipmi/ipmi_bmc_sim.c
> >> +++ b/hw/ipmi/ipmi_bmc_sim.c
> >> @@ -733,7 +733,12 @@ 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));
> >> +
> >> +    /* Log the expiry if the don't log bit is clear */
> >> +    if (!IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs)) {
> >> +        ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
> >> +    }
> >> +
> >
> > Are you sure this is correct?  The spec doesn't say what this means, but
> > I would assume this means "Don't add a system log" not "Don't set the
> > expiry happened bit".
> 
> From IPMI spec, Set Watchdog Timer command timer use field of byte 1
> says "timer use (logged on expiration when “don’t log” bit = 0b)".
> But it also says it should disable the timeout sensor event logging.
> I missed that part, I will see if I can make that work.

It doesn't currently add an event to the log, I don't think.  If you
want to add that, it's fine.

However, as it is, your change will cause the Get Watchdog Timer command
to return the wrong value for Timer Use Expiration flags.  It's not what
you want to do.

What bug are you trying to solve?

-corey

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

[Nicholas Piggin]

On Mon Mar 31, 2025 at 11:25 PM AEST, Corey Minyard wrote:
> On Mon, Mar 31, 2025 at 10:57:24PM +1000, Nicholas Piggin wrote:
>> Linux issues this command when booting a powernv machine.
>
> This is good, just a couple of nits.
>
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  include/hw/ipmi/ipmi.h | 14 +++++++++++
>>  hw/ipmi/ipmi_bmc_sim.c | 56 ++++++++++++++++++++++++++++++++++++++++--
>>  hw/ipmi/ipmi_bt.c      |  2 ++
>>  hw/ipmi/ipmi_kcs.c     |  1 +
>>  4 files changed, 71 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
>> index 77a7213ed93..5f01a50cd86 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_CH_MEDIUM_IPMB              0x01
>> +#define IPMI_CH_MEDIUM_SYSTEM            0x0c
>> +#define IPMI_CH_PROTOCOL_IPMB            0x01
>> +#define IPMI_CH_PROTOCOL_KCS             0x05
>> +#define IPMI_CH_PROTOCOL_BT_15           0x08
>
> I know it's picky, but could you spell out CHANNEL here?

Sure.

>> +
>>  #define IPMI_CC_INVALID_CMD                              0xc1
>>  #define IPMI_CC_COMMAND_INVALID_FOR_LUN                  0xc2
>>  #define IPMI_CC_TIMEOUT                                  0xc3
>> @@ -170,6 +179,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 8c3313aa65f..9198f854bd9 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
>>  
>> @@ -1033,8 +1034,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;
>>      }
>> @@ -1232,6 +1233,56 @@ 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[1] & 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 supported */
>
> I think that an all zero response is a valid response.  I think you
> should return a IPMI_CC_INVALID_DATA_FIELD instead, right?

I can't remember, I dug the patch out from a while ago. I can't actually
see anywhere it is made clear in the spec, do you? I agree an invalid
error sounds better. I'll try to see how ipmi tools handles it.

>> +        int i;
>> +        for (i = 0; i < 8; i++) {
>> +            rsp_buffer_push(rsp, 0x00);
>> +        }
>> +        return;
>> +    }
>> +
>> +    if (ch == IPMI_CHANNEL_IPMB) {
>> +        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_IPMB);
>> +        rsp_buffer_push(rsp, IPMI_CH_PROTOCOL_IPMB);
>> +    } else { /* IPMI_CHANNEL_SYSTEM */
>> +        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_SYSTEM);
>> +        rsp_buffer_push(rsp, k->protocol);
>> +    }
>> +
>> +    rsp_buffer_push(rsp, 0x00); /* Session-less */
>> +
>> +    /* IPMI Vendor ID */
>> +    rsp_buffer_push(rsp, 0xf2);
>> +    rsp_buffer_push(rsp, 0x1b);
>> +    rsp_buffer_push(rsp, 0x00);
>
> Where does this come from?

IPMI spec Get Channel Info Command, search "IPMI Enterprise Number"
From my reading, all channel info responses contain this.

>> +
>> +    if (ch == IPMI_CHANNEL_SYSTEM) {
>> +        /* IRQ assigned by ACPI/PnP (XXX?) */
>> +        rsp_buffer_push(rsp, 0x60);
>> +        rsp_buffer_push(rsp, 0x60);
>
> The interrupt should be available.  For the isa versions there is a
> get_fwinfo function pointer that you can fetch this with.  For PCI it's
> more complicated, unfortunately.

These are for the two interrupts. QEMU seems to tie both to the
same line, I guess that's okay?

That interface looks good, but what I was concerned about is whether
that implies the irq is hard coded or whether the platform can assign
it, does it mean unassigned? I don't know a lot about irq routing or
what IPMI clients would use it for.

Anyhow I'll respin with changes.

Thanks,
Nick

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

[Corey Minyard]

On Tue, Apr 01, 2025 at 09:42:01AM +1000, Nicholas Piggin wrote:
> On Mon Mar 31, 2025 at 11:25 PM AEST, Corey Minyard wrote:
> > On Mon, Mar 31, 2025 at 10:57:24PM +1000, Nicholas Piggin wrote:
> >> +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[1] & 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 supported */
> >
> > I think that an all zero response is a valid response.  I think you
> > should return a IPMI_CC_INVALID_DATA_FIELD instead, right?
> 
> I can't remember, I dug the patch out from a while ago. I can't actually
> see anywhere it is made clear in the spec, do you? I agree an invalid
> error sounds better. I'll try to see how ipmi tools handles it.

I'm fairly sure an error response is ok.  Please pursue that.

> 
> >> +        int i;
> >> +        for (i = 0; i < 8; i++) {
> >> +            rsp_buffer_push(rsp, 0x00);
> >> +        }
> >> +        return;
> >> +    }
> >> +
> >> +    if (ch == IPMI_CHANNEL_IPMB) {
> >> +        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_IPMB);
> >> +        rsp_buffer_push(rsp, IPMI_CH_PROTOCOL_IPMB);
> >> +    } else { /* IPMI_CHANNEL_SYSTEM */
> >> +        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_SYSTEM);
> >> +        rsp_buffer_push(rsp, k->protocol);
> >> +    }
> >> +
> >> +    rsp_buffer_push(rsp, 0x00); /* Session-less */
> >> +
> >> +    /* IPMI Vendor ID */
> >> +    rsp_buffer_push(rsp, 0xf2);
> >> +    rsp_buffer_push(rsp, 0x1b);
> >> +    rsp_buffer_push(rsp, 0x00);
> >
> > Where does this come from?
> 
> IPMI spec Get Channel Info Command, search "IPMI Enterprise Number"
> From my reading, all channel info responses contain this.

Oh, sorry, I should have read on this.  This is fine.

> 
> >> +
> >> +    if (ch == IPMI_CHANNEL_SYSTEM) {
> >> +        /* IRQ assigned by ACPI/PnP (XXX?) */
> >> +        rsp_buffer_push(rsp, 0x60);
> >> +        rsp_buffer_push(rsp, 0x60);
> >
> > The interrupt should be available.  For the isa versions there is a
> > get_fwinfo function pointer that you can fetch this with.  For PCI it's
> > more complicated, unfortunately.
> 
> These are for the two interrupts. QEMU seems to tie both to the
> same line, I guess that's okay?

Yes, they are the same.

> 
> That interface looks good, but what I was concerned about is whether
> that implies the irq is hard coded or whether the platform can assign
> it, does it mean unassigned? I don't know a lot about irq routing or
> what IPMI clients would use it for.

For isa-based devices, it's hard-coded by the configuration.  That one
should be easy to get.

For PCI, I'm not so sure.  It would take some research to figure it out.

> 
> Anyhow I'll respin with changes.

Thanks,

-corey

> 
> Thanks,
> Nick

Re: [PATCH 1/3] ipmi/bmc-sim: implement watchdog dont log flag

[Corey Minyard]

On Mon, Mar 31, 2025 at 06:03:11PM -0500, Corey Minyard wrote:
> On Tue, Apr 01, 2025 at 08:37:19AM +1000, Nicholas Piggin wrote:
> > On Mon Mar 31, 2025 at 11:13 PM AEST, Corey Minyard wrote:
> > > On Mon, Mar 31, 2025 at 10:57:22PM +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 | 7 ++++++-
> > >>  1 file changed, 6 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> > >> index 6157ac71201..32161044c0b 100644
> > >> --- a/hw/ipmi/ipmi_bmc_sim.c
> > >> +++ b/hw/ipmi/ipmi_bmc_sim.c
> > >> @@ -733,7 +733,12 @@ 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));
> > >> +
> > >> +    /* Log the expiry if the don't log bit is clear */
> > >> +    if (!IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs)) {
> > >> +        ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
> > >> +    }
> > >> +
> > >
> > > Are you sure this is correct?  The spec doesn't say what this means, but
> > > I would assume this means "Don't add a system log" not "Don't set the
> > > expiry happened bit".
> > 
> > From IPMI spec, Set Watchdog Timer command timer use field of byte 1
> > says "timer use (logged on expiration when “don’t log” bit = 0b)".
> > But it also says it should disable the timeout sensor event logging.
> > I missed that part, I will see if I can make that work.
> 
> It doesn't currently add an event to the log, I don't think.  If you
> want to add that, it's fine.

Actually, I'm wrong here.  It does generate an event through the sensor
handling, the sensor_set_discrete_bit() function will cause an event to
be generated.  However, you can't just not call that function, you need
the event set.  I think the best way would be to add a "do_log"
parameter to that function to suppress the log in this case.

-corey

> 
> However, as it is, your change will cause the Get Watchdog Timer command
> to return the wrong value for Timer Use Expiration flags.  It's not what
> you want to do.
> 
> What bug are you trying to solve?
> 
> -corey