All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2)
@ 2016-03-10 14:03 Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro Cédric Le Goater
                   ` (9 more replies)
  0 siblings, 10 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:03 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

Hello,

The first patches are cleanups and prepare ground for an extension of
the BMC simulator providing a SDR loader using a file. A simple FRU
support comes next.

Changes since v2:

 - changed 'struct rsp_buffer' to 'RspBuffer' 
 - reworked ipmi_sim_handle_command() to introduce a
   ipmi_get_handler() helper.
    
Changes since v1:

  - Added initial cleanups removing the macros implicitely making use
    of local variables in the command handlers  
  - Fixed property naming
  - Kept the API extensions to expose SDR and generate events for
    later

Based on 9c279bec754a and also available here  :

  https://github.com/legoater/qemu/commits/ipmi

Thanks,

C.

Cédric Le Goater (10):
  ipmi: remove IPMI_CHECK_CMD_LEN() macro
  ipmi: replace IPMI_ADD_RSP_DATA() macro with inline helpers
  ipmi: remove IPMI_CHECK_RESERVATION() macro
  ipmi: add rsp_buffer_set_error() helper
  ipmi: add a realize function to the device class
  ipmi: use a function to initialize the SDR table
  ipmi: remove the need of an ending record in the SDR table
  ipmi: add some local variables in ipmi_sdr_init
  ipmi: use a file to load SDRs
  ipmi: provide support for FRUs

 hw/ipmi/ipmi_bmc_sim.c | 855 ++++++++++++++++++++++++++++---------------------
 qemu-options.hx        |  15 +-
 2 files changed, 506 insertions(+), 364 deletions(-)

-- 
2.1.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
@ 2016-03-10 14:03 ` Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 02/10] ipmi: replace IPMI_ADD_RSP_DATA() macro with inline helpers Cédric Le Goater
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:03 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

Most IPMI command handlers in the BMC simulator start with a call to
the macro IPMI_CHECK_CMD_LEN() which verifies that a minimal number of
arguments expected by the command are indeed available. To achieve
this task, the macro implicitly uses local variables which is
misleading in the code.

This patch adds a 'cmd_len_min' attribute to the struct IPMICmdHandler
defining the minimal number of arguments expected by the command and
moves this check in the global command handler ipmi_sim_handle_command().

To clarify the checks being done on the received command, the patch
introduces a helper ipmi_get_handler().

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
---

 Changes since v2:

 - introduced ipmi_get_handler() helper.
 
 hw/ipmi/ipmi_bmc_sim.c |  164 +++++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 80 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -155,10 +155,15 @@ typedef struct IPMISensor {
 typedef struct IPMIBmcSim IPMIBmcSim;
 
 #define MAX_NETFNS 64
-typedef void (*IPMICmdHandler)(IPMIBmcSim *s,
-                               uint8_t *cmd, unsigned int cmd_len,
-                               uint8_t *rsp, unsigned int *rsp_len,
-                               unsigned int max_rsp_len);
+
+typedef struct IPMICmdHandler {
+    void (*cmd_handler)(IPMIBmcSim *s,
+                        uint8_t *cmd, unsigned int cmd_len,
+                        uint8_t *rsp, unsigned int *rsp_len,
+                        unsigned int max_rsp_len);
+    unsigned int cmd_len_min;
+} IPMICmdHandler;
+
 typedef struct IPMINetfn {
     unsigned int cmd_nums;
     const IPMICmdHandler *cmd_handlers;
@@ -269,13 +274,6 @@ struct IPMIBmcSim {
         rsp[(*rsp_len)++] = (b);                           \
     } while (0)
 
-/* Verify that the received command is a certain length. */
-#define IPMI_CHECK_CMD_LEN(l) \
-    if (cmd_len < l) {                                     \
-        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;      \
-        return; \
-    }
-
 /* Check that the reservation in the command is valid. */
 #define IPMI_CHECK_RESERVATION(off, r) \
     do {                                                   \
@@ -566,6 +564,28 @@ static int ipmi_register_netfn(IPMIBmcSi
     return 0;
 }
 
+static const IPMICmdHandler *ipmi_get_handler(IPMIBmcSim *ibs,
+                                              unsigned int netfn,
+                                              unsigned int cmd)
+{
+    const IPMICmdHandler *hdl;
+
+    if (netfn & 1 || netfn >= MAX_NETFNS || !ibs->netfns[netfn / 2]) {
+        return NULL;
+    }
+
+    if (cmd >= ibs->netfns[netfn / 2]->cmd_nums) {
+        return NULL;
+    }
+
+    hdl = &ibs->netfns[netfn / 2]->cmd_handlers[cmd];
+    if (!hdl->cmd_handler) {
+        return NULL;
+    }
+
+    return hdl;
+}
+
 static void next_timeout(IPMIBmcSim *ibs)
 {
     int64_t next;
@@ -586,11 +606,11 @@ static void ipmi_sim_handle_command(IPMI
     IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
-    unsigned int netfn;
     uint8_t rsp[MAX_IPMI_MSG_SIZE];
     unsigned int rsp_len_holder = 0;
     unsigned int *rsp_len = &rsp_len_holder;
     unsigned int max_rsp_len = sizeof(rsp);
+    const IPMICmdHandler *hdl;
 
     /* Set up the response, set the low bit of NETFN. */
     /* Note that max_rsp_len must be at least 3 */
@@ -619,18 +639,18 @@ static void ipmi_sim_handle_command(IPMI
         goto out;
     }
 
-    netfn = cmd[0] >> 2;
-
-    /* Odd netfns are not valid, make sure the command is registered */
-    if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
-                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
-                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
+    hdl = ipmi_get_handler(ibs, cmd[0] >> 2, cmd[1]);
+    if (!hdl) {
         rsp[2] = IPMI_CC_INVALID_CMD;
         goto out;
     }
 
-    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
-                                                max_rsp_len);
+    if (cmd_len < hdl->cmd_len_min) {
+        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        goto out;
+    }
+
+    hdl->cmd_handler(ibs, cmd, cmd_len, rsp, rsp_len, max_rsp_len);
 
  out:
     k->handle_rsp(s, msg_id, rsp, *rsp_len);
@@ -737,7 +757,6 @@ static void chassis_control(IPMIBmcSim *
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
 
-    IPMI_CHECK_CMD_LEN(3);
     switch (cmd[2] & 0xf) {
     case 0: /* power down */
         rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
@@ -838,7 +857,6 @@ static void set_acpi_power_state(IPMIBmc
                           uint8_t *rsp, unsigned int *rsp_len,
                           unsigned int max_rsp_len)
 {
-    IPMI_CHECK_CMD_LEN(4);
     ibs->acpi_power_state[0] = cmd[2];
     ibs->acpi_power_state[1] = cmd[3];
 }
@@ -869,7 +887,6 @@ static void set_bmc_global_enables(IPMIB
                                    uint8_t *rsp, unsigned int *rsp_len,
                                    unsigned int max_rsp_len)
 {
-    IPMI_CHECK_CMD_LEN(3);
     set_global_enables(ibs, cmd[2]);
 }
 
@@ -889,7 +906,6 @@ static void clr_msg_flags(IPMIBmcSim *ib
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
 
-    IPMI_CHECK_CMD_LEN(3);
     ibs->msg_flags &= ~cmd[2];
     k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
 }
@@ -976,15 +992,17 @@ static void send_msg(IPMIBmcSim *ibs,
     uint8_t *buf;
     uint8_t netfn, rqLun, rsLun, rqSeq;
 
-    IPMI_CHECK_CMD_LEN(3);
-
     if (cmd[2] != 0) {
         /* We only handle channel 0 with no options */
         rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
 
-    IPMI_CHECK_CMD_LEN(10);
+    if (cmd_len < 10) {
+        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        return;
+    }
+
     if (cmd[3] != 0x40) {
         /* We only emulate a MC at address 0x40. */
         rsp[2] = 0x83; /* NAK on write */
@@ -1092,7 +1110,6 @@ static void set_watchdog_timer(IPMIBmcSi
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
     unsigned int val;
 
-    IPMI_CHECK_CMD_LEN(8);
     val = cmd[2] & 0x7; /* Validate use */
     if (val == 0 || val > 5) {
         rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
@@ -1217,7 +1234,6 @@ static void get_sdr(IPMIBmcSim *ibs,
     uint16_t nextrec;
     struct ipmi_sdr_header *sdrh;
 
-    IPMI_CHECK_CMD_LEN(8);
     if (cmd[6]) {
         IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
     }
@@ -1271,7 +1287,6 @@ static void clear_sdr_rep(IPMIBmcSim *ib
                           uint8_t *rsp, unsigned int *rsp_len,
                           unsigned int max_rsp_len)
 {
-    IPMI_CHECK_CMD_LEN(8);
     IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
     if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
         rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
@@ -1330,7 +1345,6 @@ static void get_sel_entry(IPMIBmcSim *ib
 {
     unsigned int val;
 
-    IPMI_CHECK_CMD_LEN(8);
     if (cmd[6]) {
         IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
     }
@@ -1375,7 +1389,6 @@ static void add_sel_entry(IPMIBmcSim *ib
                           uint8_t *rsp, unsigned int *rsp_len,
                           unsigned int max_rsp_len)
 {
-    IPMI_CHECK_CMD_LEN(18);
     if (sel_add_event(ibs, cmd + 2)) {
         rsp[2] = IPMI_CC_OUT_OF_SPACE;
         return;
@@ -1390,7 +1403,6 @@ static void clear_sel(IPMIBmcSim *ibs,
                       uint8_t *rsp, unsigned int *rsp_len,
                       unsigned int max_rsp_len)
 {
-    IPMI_CHECK_CMD_LEN(8);
     IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
     if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
         rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
@@ -1434,7 +1446,6 @@ static void set_sel_time(IPMIBmcSim *ibs
     uint32_t val;
     struct ipmi_time now;
 
-    IPMI_CHECK_CMD_LEN(6);
     val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
     ipmi_gettime(&now);
     ibs->sel.time_offset = now.tv_sec - ((long) val);
@@ -1447,7 +1458,6 @@ static void set_sensor_evt_enable(IPMIBm
 {
     IPMISensor *sens;
 
-    IPMI_CHECK_CMD_LEN(4);
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
         rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
@@ -1499,7 +1509,6 @@ static void get_sensor_evt_enable(IPMIBm
 {
     IPMISensor *sens;
 
-    IPMI_CHECK_CMD_LEN(3);
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
         rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
@@ -1520,7 +1529,6 @@ static void rearm_sensor_evts(IPMIBmcSim
 {
     IPMISensor *sens;
 
-    IPMI_CHECK_CMD_LEN(4);
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
         rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
@@ -1542,7 +1550,6 @@ static void get_sensor_evt_status(IPMIBm
 {
     IPMISensor *sens;
 
-    IPMI_CHECK_CMD_LEN(3);
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
         rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
@@ -1564,7 +1571,6 @@ static void get_sensor_reading(IPMIBmcSi
 {
     IPMISensor *sens;
 
-    IPMI_CHECK_CMD_LEN(3);
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
         rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
@@ -1587,7 +1593,6 @@ static void set_sensor_type(IPMIBmcSim *
     IPMISensor *sens;
 
 
-    IPMI_CHECK_CMD_LEN(5);
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
         rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
@@ -1606,7 +1611,6 @@ static void get_sensor_type(IPMIBmcSim *
     IPMISensor *sens;
 
 
-    IPMI_CHECK_CMD_LEN(3);
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
         rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
@@ -1619,10 +1623,10 @@ static void get_sensor_type(IPMIBmcSim *
 
 
 static const IPMICmdHandler chassis_cmds[] = {
-    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities,
-    [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status,
-    [IPMI_CMD_CHASSIS_CONTROL] = chassis_control,
-    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = chassis_get_sys_restart_cause
+    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = { chassis_capabilities },
+    [IPMI_CMD_GET_CHASSIS_STATUS] = { chassis_status },
+    [IPMI_CMD_CHASSIS_CONTROL] = { chassis_control, 3 },
+    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = { chassis_get_sys_restart_cause }
 };
 static const IPMINetfn chassis_netfn = {
     .cmd_nums = ARRAY_SIZE(chassis_cmds),
@@ -1630,13 +1634,13 @@ static const IPMINetfn chassis_netfn = {
 };
 
 static const IPMICmdHandler sensor_event_cmds[] = {
-    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = set_sensor_evt_enable,
-    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = get_sensor_evt_enable,
-    [IPMI_CMD_REARM_SENSOR_EVTS] = rearm_sensor_evts,
-    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = get_sensor_evt_status,
-    [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading,
-    [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type,
-    [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type,
+    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
+    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
+    [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
+    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = { get_sensor_evt_status, 3 },
+    [IPMI_CMD_GET_SENSOR_READING] = { get_sensor_reading, 3 },
+    [IPMI_CMD_SET_SENSOR_TYPE] = { set_sensor_type, 5 },
+    [IPMI_CMD_GET_SENSOR_TYPE] = { get_sensor_type, 3 },
 };
 static const IPMINetfn sensor_event_netfn = {
     .cmd_nums = ARRAY_SIZE(sensor_event_cmds),
@@ -1644,22 +1648,22 @@ static const IPMINetfn sensor_event_netf
 };
 
 static const IPMICmdHandler app_cmds[] = {
-    [IPMI_CMD_GET_DEVICE_ID] = get_device_id,
-    [IPMI_CMD_COLD_RESET] = cold_reset,
-    [IPMI_CMD_WARM_RESET] = warm_reset,
-    [IPMI_CMD_SET_ACPI_POWER_STATE] = set_acpi_power_state,
-    [IPMI_CMD_GET_ACPI_POWER_STATE] = get_acpi_power_state,
-    [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid,
-    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables,
-    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables,
-    [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags,
-    [IPMI_CMD_GET_MSG_FLAGS] = get_msg_flags,
-    [IPMI_CMD_GET_MSG] = get_msg,
-    [IPMI_CMD_SEND_MSG] = send_msg,
-    [IPMI_CMD_READ_EVT_MSG_BUF] = read_evt_msg_buf,
-    [IPMI_CMD_RESET_WATCHDOG_TIMER] = reset_watchdog_timer,
-    [IPMI_CMD_SET_WATCHDOG_TIMER] = set_watchdog_timer,
-    [IPMI_CMD_GET_WATCHDOG_TIMER] = get_watchdog_timer,
+    [IPMI_CMD_GET_DEVICE_ID] = { get_device_id },
+    [IPMI_CMD_COLD_RESET] = { cold_reset },
+    [IPMI_CMD_WARM_RESET] = { warm_reset },
+    [IPMI_CMD_SET_ACPI_POWER_STATE] = { set_acpi_power_state, 4 },
+    [IPMI_CMD_GET_ACPI_POWER_STATE] = { get_acpi_power_state },
+    [IPMI_CMD_GET_DEVICE_GUID] = { get_device_guid },
+    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = { set_bmc_global_enables, 3 },
+    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = { get_bmc_global_enables },
+    [IPMI_CMD_CLR_MSG_FLAGS] = { clr_msg_flags, 3 },
+    [IPMI_CMD_GET_MSG_FLAGS] = { get_msg_flags },
+    [IPMI_CMD_GET_MSG] = { get_msg },
+    [IPMI_CMD_SEND_MSG] = { send_msg, 3 },
+    [IPMI_CMD_READ_EVT_MSG_BUF] = { read_evt_msg_buf },
+    [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 },
 };
 static const IPMINetfn app_netfn = {
     .cmd_nums = ARRAY_SIZE(app_cmds),
@@ -1667,18 +1671,18 @@ static const IPMINetfn app_netfn = {
 };
 
 static const IPMICmdHandler storage_cmds[] = {
-    [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
-    [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
-    [IPMI_CMD_GET_SDR] = get_sdr,
-    [IPMI_CMD_ADD_SDR] = add_sdr,
-    [IPMI_CMD_CLEAR_SDR_REP] = clear_sdr_rep,
-    [IPMI_CMD_GET_SEL_INFO] = get_sel_info,
-    [IPMI_CMD_RESERVE_SEL] = reserve_sel,
-    [IPMI_CMD_GET_SEL_ENTRY] = get_sel_entry,
-    [IPMI_CMD_ADD_SEL_ENTRY] = add_sel_entry,
-    [IPMI_CMD_CLEAR_SEL] = clear_sel,
-    [IPMI_CMD_GET_SEL_TIME] = get_sel_time,
-    [IPMI_CMD_SET_SEL_TIME] = set_sel_time,
+    [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
+    [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
+    [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
+    [IPMI_CMD_ADD_SDR] = { add_sdr },
+    [IPMI_CMD_CLEAR_SDR_REP] = { clear_sdr_rep, 8 },
+    [IPMI_CMD_GET_SEL_INFO] = { get_sel_info },
+    [IPMI_CMD_RESERVE_SEL] = { reserve_sel },
+    [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
+    [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
+    [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
+    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
+    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
 };
 
 static const IPMINetfn storage_netfn = {

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 02/10] ipmi: replace IPMI_ADD_RSP_DATA() macro with inline helpers
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro Cédric Le Goater
@ 2016-03-10 14:03 ` Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 03/10] ipmi: remove IPMI_CHECK_RESERVATION() macro Cédric Le Goater
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:03 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

The IPMI command handlers in the BMC simulator use a macro
IPMI_ADD_RSP_DATA() to push bytes in a response buffer. The macro
hides the fact that it implicitly uses variables local to the handler,
which is misleading.

This patch introduces a simple 'struct RspBuffer' and inlined helper
routines to store byte(s) in a response buffer. rsp_buffer_push()
replaces the macro IPMI_ADD_RSP_DATA() and rsp_buffer_pushmore() is
new helper to push multiple bytes. The latest is used in the command
handlers get_msg() and get_sdr() which are manipulating the buffer
directly.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
---

Changes since v2:

 - changed 'struct rsp_buffer' to 'RspBuffer'
 
 hw/ipmi/ipmi_bmc_sim.c |  479 +++++++++++++++++++++++--------------------------
 1 file changed, 227 insertions(+), 252 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -153,14 +153,14 @@ typedef struct IPMISensor {
 #define IPMI_WATCHDOG_SENSOR 0
 
 typedef struct IPMIBmcSim IPMIBmcSim;
+typedef struct RspBuffer RspBuffer;
 
 #define MAX_NETFNS 64
 
 typedef struct IPMICmdHandler {
     void (*cmd_handler)(IPMIBmcSim *s,
                         uint8_t *cmd, unsigned int cmd_len,
-                        uint8_t *rsp, unsigned int *rsp_len,
-                        unsigned int max_rsp_len);
+                        RspBuffer *rsp);
     unsigned int cmd_len_min;
 } IPMICmdHandler;
 
@@ -263,22 +263,40 @@ struct IPMIBmcSim {
 #define IPMI_BMC_WATCHDOG_ACTION_POWER_DOWN      2
 #define IPMI_BMC_WATCHDOG_ACTION_POWER_CYCLE     3
 
+struct RspBuffer {
+    uint8_t buffer[MAX_IPMI_MSG_SIZE];
+    unsigned int len;
+};
+
+#define RSP_BUFFER_INITIALIZER { }
 
 /* Add a byte to the response. */
-#define IPMI_ADD_RSP_DATA(b) \
-    do {                                                   \
-        if (*rsp_len >= max_rsp_len) {                     \
-            rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;       \
-            return;                                        \
-        }                                                  \
-        rsp[(*rsp_len)++] = (b);                           \
-    } while (0)
+static inline void rsp_buffer_push(RspBuffer *rsp, uint8_t byte)
+{
+    if (rsp->len >= sizeof(rsp->buffer)) {
+        rsp->buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+        return;
+    }
+    rsp->buffer[rsp->len++] = byte;
+}
+
+static inline void rsp_buffer_pushmore(RspBuffer *rsp, uint8_t *bytes,
+                                       unsigned int n)
+{
+    if (rsp->len + n >= sizeof(rsp->buffer)) {
+        rsp->buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+        return;
+    }
+
+    memcpy(&rsp->buffer[rsp->len], bytes, n);
+    rsp->len += n;
+}
 
 /* Check that the reservation in the command is valid. */
 #define IPMI_CHECK_RESERVATION(off, r) \
     do {                                                   \
         if ((cmd[off] | (cmd[off + 1] << 8)) != r) {       \
-            rsp[2] = IPMI_CC_INVALID_RESERVATION;          \
+            rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;     \
             return;                                        \
         }                                                  \
     } while (0)
@@ -606,54 +624,51 @@ static void ipmi_sim_handle_command(IPMI
     IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
-    uint8_t rsp[MAX_IPMI_MSG_SIZE];
-    unsigned int rsp_len_holder = 0;
-    unsigned int *rsp_len = &rsp_len_holder;
-    unsigned int max_rsp_len = sizeof(rsp);
     const IPMICmdHandler *hdl;
+    RspBuffer rsp = RSP_BUFFER_INITIALIZER;
 
     /* Set up the response, set the low bit of NETFN. */
     /* Note that max_rsp_len must be at least 3 */
-    if (max_rsp_len < 3) {
-        rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+    if (sizeof(rsp.buffer) < 3) {
+        rsp.buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
         goto out;
     }
 
-    IPMI_ADD_RSP_DATA(cmd[0] | 0x04);
-    IPMI_ADD_RSP_DATA(cmd[1]);
-    IPMI_ADD_RSP_DATA(0); /* Assume success */
+    rsp_buffer_push(&rsp, cmd[0] | 0x04);
+    rsp_buffer_push(&rsp, cmd[1]);
+    rsp_buffer_push(&rsp, 0); /* Assume success */
 
     /* If it's too short or it was truncated, return an error. */
     if (cmd_len < 2) {
-        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        rsp.buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
         goto out;
     }
     if (cmd_len > max_cmd_len) {
-        rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+        rsp.buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
         goto out;
     }
 
     if ((cmd[0] & 0x03) != 0) {
         /* Only have stuff on LUN 0 */
-        rsp[2] = IPMI_CC_COMMAND_INVALID_FOR_LUN;
+        rsp.buffer[2] = IPMI_CC_COMMAND_INVALID_FOR_LUN;
         goto out;
     }
 
     hdl = ipmi_get_handler(ibs, cmd[0] >> 2, cmd[1]);
     if (!hdl) {
-        rsp[2] = IPMI_CC_INVALID_CMD;
+        rsp.buffer[2] = IPMI_CC_INVALID_CMD;
         goto out;
     }
 
     if (cmd_len < hdl->cmd_len_min) {
-        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        rsp.buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
         goto out;
     }
 
-    hdl->cmd_handler(ibs, cmd, cmd_len, rsp, rsp_len, max_rsp_len);
+    hdl->cmd_handler(ibs, cmd, cmd_len, &rsp);
 
  out:
-    k->handle_rsp(s, msg_id, rsp, *rsp_len);
+    k->handle_rsp(s, msg_id, rsp.buffer, rsp.len);
 
     next_timeout(ibs);
 }
@@ -728,86 +743,82 @@ static void ipmi_sim_handle_timeout(IPMI
 
 static void chassis_capabilities(IPMIBmcSim *ibs,
                                  uint8_t *cmd, unsigned int cmd_len,
-                                 uint8_t *rsp, unsigned int *rsp_len,
-                                 unsigned int max_rsp_len)
+                                 RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(0);
-    IPMI_ADD_RSP_DATA(ibs->parent.slave_addr);
-    IPMI_ADD_RSP_DATA(ibs->parent.slave_addr);
-    IPMI_ADD_RSP_DATA(ibs->parent.slave_addr);
-    IPMI_ADD_RSP_DATA(ibs->parent.slave_addr);
+    rsp_buffer_push(rsp, 0);
+    rsp_buffer_push(rsp, ibs->parent.slave_addr);
+    rsp_buffer_push(rsp, ibs->parent.slave_addr);
+    rsp_buffer_push(rsp, ibs->parent.slave_addr);
+    rsp_buffer_push(rsp, ibs->parent.slave_addr);
 }
 
 static void chassis_status(IPMIBmcSim *ibs,
                            uint8_t *cmd, unsigned int cmd_len,
-                           uint8_t *rsp, unsigned int *rsp_len,
-                           unsigned int max_rsp_len)
+                           RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(0x61); /* Unknown power restore, power is on */
-    IPMI_ADD_RSP_DATA(0);
-    IPMI_ADD_RSP_DATA(0);
-    IPMI_ADD_RSP_DATA(0);
+    rsp_buffer_push(rsp, 0x61); /* Unknown power restore, power is on */
+    rsp_buffer_push(rsp, 0);
+    rsp_buffer_push(rsp, 0);
+    rsp_buffer_push(rsp, 0);
 }
 
 static void chassis_control(IPMIBmcSim *ibs,
                             uint8_t *cmd, unsigned int cmd_len,
-                            uint8_t *rsp, unsigned int *rsp_len,
-                            unsigned int max_rsp_len)
+                            RspBuffer *rsp)
 {
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
 
     switch (cmd[2] & 0xf) {
     case 0: /* power down */
-        rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
+        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
         break;
     case 1: /* power up */
-        rsp[2] = k->do_hw_op(s, IPMI_POWERON_CHASSIS, 0);
+        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWERON_CHASSIS, 0);
         break;
     case 2: /* power cycle */
-        rsp[2] = k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 0);
+        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 0);
         break;
     case 3: /* hard reset */
-        rsp[2] = k->do_hw_op(s, IPMI_RESET_CHASSIS, 0);
+        rsp->buffer[2] = k->do_hw_op(s, IPMI_RESET_CHASSIS, 0);
         break;
     case 4: /* pulse diagnostic interrupt */
-        rsp[2] = k->do_hw_op(s, IPMI_PULSE_DIAG_IRQ, 0);
+        rsp->buffer[2] = k->do_hw_op(s, IPMI_PULSE_DIAG_IRQ, 0);
         break;
     case 5: /* soft shutdown via ACPI by overtemp emulation */
-        rsp[2] = k->do_hw_op(s,
+        rsp->buffer[2] = k->do_hw_op(s,
                              IPMI_SHUTDOWN_VIA_ACPI_OVERTEMP, 0);
         break;
     default:
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
 }
 
 static void chassis_get_sys_restart_cause(IPMIBmcSim *ibs,
                            uint8_t *cmd, unsigned int cmd_len,
-                           uint8_t *rsp, unsigned int *rsp_len,
-                           unsigned int max_rsp_len)
+                           RspBuffer *rsp)
+
 {
-    IPMI_ADD_RSP_DATA(ibs->restart_cause & 0xf); /* Restart Cause */
-    IPMI_ADD_RSP_DATA(0);  /* Channel 0 */
+    rsp_buffer_push(rsp, ibs->restart_cause & 0xf); /* Restart Cause */
+    rsp_buffer_push(rsp, 0);  /* Channel 0 */
 }
 
 static void get_device_id(IPMIBmcSim *ibs,
                           uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                          RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(ibs->device_id);
-    IPMI_ADD_RSP_DATA(ibs->device_rev & 0xf);
-    IPMI_ADD_RSP_DATA(ibs->fwrev1 & 0x7f);
-    IPMI_ADD_RSP_DATA(ibs->fwrev2);
-    IPMI_ADD_RSP_DATA(ibs->ipmi_version);
-    IPMI_ADD_RSP_DATA(0x07); /* sensor, SDR, and SEL. */
-    IPMI_ADD_RSP_DATA(ibs->mfg_id[0]);
-    IPMI_ADD_RSP_DATA(ibs->mfg_id[1]);
-    IPMI_ADD_RSP_DATA(ibs->mfg_id[2]);
-    IPMI_ADD_RSP_DATA(ibs->product_id[0]);
-    IPMI_ADD_RSP_DATA(ibs->product_id[1]);
+    rsp_buffer_push(rsp, ibs->device_id);
+    rsp_buffer_push(rsp, ibs->device_rev & 0xf);
+    rsp_buffer_push(rsp, ibs->fwrev1 & 0x7f);
+    rsp_buffer_push(rsp, ibs->fwrev2);
+    rsp_buffer_push(rsp, ibs->ipmi_version);
+    rsp_buffer_push(rsp, 0x07); /* sensor, SDR, and SEL. */
+    rsp_buffer_push(rsp, ibs->mfg_id[0]);
+    rsp_buffer_push(rsp, ibs->mfg_id[1]);
+    rsp_buffer_push(rsp, ibs->mfg_id[2]);
+    rsp_buffer_push(rsp, ibs->product_id[0]);
+    rsp_buffer_push(rsp, ibs->product_id[1]);
 }
 
 static void set_global_enables(IPMIBmcSim *ibs, uint8_t val)
@@ -826,8 +837,7 @@ static void set_global_enables(IPMIBmcSi
 
 static void cold_reset(IPMIBmcSim *ibs,
                        uint8_t *cmd, unsigned int cmd_len,
-                       uint8_t *rsp, unsigned int *rsp_len,
-                       unsigned int max_rsp_len)
+                       RspBuffer *rsp)
 {
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
@@ -842,8 +852,7 @@ static void cold_reset(IPMIBmcSim *ibs,
 
 static void warm_reset(IPMIBmcSim *ibs,
                        uint8_t *cmd, unsigned int cmd_len,
-                       uint8_t *rsp, unsigned int *rsp_len,
-                       unsigned int max_rsp_len)
+                       RspBuffer *rsp)
 {
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
@@ -853,55 +862,49 @@ static void warm_reset(IPMIBmcSim *ibs,
     }
 }
 static void set_acpi_power_state(IPMIBmcSim *ibs,
-                          uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                                 uint8_t *cmd, unsigned int cmd_len,
+                                 RspBuffer *rsp)
 {
     ibs->acpi_power_state[0] = cmd[2];
     ibs->acpi_power_state[1] = cmd[3];
 }
 
 static void get_acpi_power_state(IPMIBmcSim *ibs,
-                          uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                                 uint8_t *cmd, unsigned int cmd_len,
+                                 RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(ibs->acpi_power_state[0]);
-    IPMI_ADD_RSP_DATA(ibs->acpi_power_state[1]);
+    rsp_buffer_push(rsp, ibs->acpi_power_state[0]);
+    rsp_buffer_push(rsp, ibs->acpi_power_state[1]);
 }
 
 static void get_device_guid(IPMIBmcSim *ibs,
-                          uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                            uint8_t *cmd, unsigned int cmd_len,
+                            RspBuffer *rsp)
 {
     unsigned int i;
 
     for (i = 0; i < 16; i++) {
-        IPMI_ADD_RSP_DATA(ibs->uuid[i]);
+        rsp_buffer_push(rsp, ibs->uuid[i]);
     }
 }
 
 static void set_bmc_global_enables(IPMIBmcSim *ibs,
                                    uint8_t *cmd, unsigned int cmd_len,
-                                   uint8_t *rsp, unsigned int *rsp_len,
-                                   unsigned int max_rsp_len)
+                                   RspBuffer *rsp)
 {
     set_global_enables(ibs, cmd[2]);
 }
 
 static void get_bmc_global_enables(IPMIBmcSim *ibs,
                                    uint8_t *cmd, unsigned int cmd_len,
-                                   uint8_t *rsp, unsigned int *rsp_len,
-                                   unsigned int max_rsp_len)
+                                   RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(ibs->bmc_global_enables);
+    rsp_buffer_push(rsp, ibs->bmc_global_enables);
 }
 
 static void clr_msg_flags(IPMIBmcSim *ibs,
                           uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                          RspBuffer *rsp)
 {
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
@@ -912,27 +915,25 @@ static void clr_msg_flags(IPMIBmcSim *ib
 
 static void get_msg_flags(IPMIBmcSim *ibs,
                           uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                          RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(ibs->msg_flags);
+    rsp_buffer_push(rsp, ibs->msg_flags);
 }
 
 static void read_evt_msg_buf(IPMIBmcSim *ibs,
                              uint8_t *cmd, unsigned int cmd_len,
-                             uint8_t *rsp, unsigned int *rsp_len,
-                            unsigned int max_rsp_len)
+                             RspBuffer *rsp)
 {
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
     unsigned int i;
 
     if (!(ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL)) {
-        rsp[2] = 0x80;
+        rsp->buffer[2] = 0x80;
         return;
     }
     for (i = 0; i < 16; i++) {
-        IPMI_ADD_RSP_DATA(ibs->evtbuf[i]);
+        rsp_buffer_push(rsp, ibs->evtbuf[i]);
     }
     ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_EVT_BUF_FULL;
     k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
@@ -940,21 +941,18 @@ static void read_evt_msg_buf(IPMIBmcSim
 
 static void get_msg(IPMIBmcSim *ibs,
                     uint8_t *cmd, unsigned int cmd_len,
-                    uint8_t *rsp, unsigned int *rsp_len,
-                    unsigned int max_rsp_len)
+                    RspBuffer *rsp)
 {
     IPMIRcvBufEntry *msg;
 
     qemu_mutex_lock(&ibs->lock);
     if (QTAILQ_EMPTY(&ibs->rcvbufs)) {
-        rsp[2] = 0x80; /* Queue empty */
+        rsp->buffer[2] = 0x80; /* Queue empty */
         goto out;
     }
-    rsp[3] = 0; /* Channel 0 */
-    *rsp_len += 1;
+    rsp_buffer_push(rsp, 0); /* Channel 0 */
     msg = QTAILQ_FIRST(&ibs->rcvbufs);
-    memcpy(rsp + 4, msg->buf, msg->len);
-    *rsp_len += msg->len;
+    rsp_buffer_pushmore(rsp, msg->buf, msg->len);
     QTAILQ_REMOVE(&ibs->rcvbufs, msg, entry);
     g_free(msg);
 
@@ -983,8 +981,7 @@ ipmb_checksum(unsigned char *data, int s
 
 static void send_msg(IPMIBmcSim *ibs,
                      uint8_t *cmd, unsigned int cmd_len,
-                     uint8_t *rsp, unsigned int *rsp_len,
-                     unsigned int max_rsp_len)
+                     RspBuffer *rsp)
 {
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
@@ -994,18 +991,18 @@ static void send_msg(IPMIBmcSim *ibs,
 
     if (cmd[2] != 0) {
         /* We only handle channel 0 with no options */
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
 
     if (cmd_len < 10) {
-        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        rsp->buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
         return;
     }
 
     if (cmd[3] != 0x40) {
         /* We only emulate a MC at address 0x40. */
-        rsp[2] = 0x83; /* NAK on write */
+        rsp->buffer[2] = 0x83; /* NAK on write */
         return;
     }
 
@@ -1091,11 +1088,10 @@ static void do_watchdog_reset(IPMIBmcSim
 
 static void reset_watchdog_timer(IPMIBmcSim *ibs,
                                  uint8_t *cmd, unsigned int cmd_len,
-                                 uint8_t *rsp, unsigned int *rsp_len,
-                                 unsigned int max_rsp_len)
+                                 RspBuffer *rsp)
 {
     if (!ibs->watchdog_initialized) {
-        rsp[2] = 0x80;
+        rsp->buffer[2] = 0x80;
         return;
     }
     do_watchdog_reset(ibs);
@@ -1103,8 +1099,7 @@ static void reset_watchdog_timer(IPMIBmc
 
 static void set_watchdog_timer(IPMIBmcSim *ibs,
                                uint8_t *cmd, unsigned int cmd_len,
-                               uint8_t *rsp, unsigned int *rsp_len,
-                               unsigned int max_rsp_len)
+                               RspBuffer *rsp)
 {
     IPMIInterface *s = ibs->parent.intf;
     IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
@@ -1112,7 +1107,7 @@ static void set_watchdog_timer(IPMIBmcSi
 
     val = cmd[2] & 0x7; /* Validate use */
     if (val == 0 || val > 5) {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
     val = cmd[3] & 0x7; /* Validate action */
@@ -1121,22 +1116,22 @@ static void set_watchdog_timer(IPMIBmcSi
         break;
 
     case IPMI_BMC_WATCHDOG_ACTION_RESET:
-        rsp[2] = k->do_hw_op(s, IPMI_RESET_CHASSIS, 1);
+        rsp->buffer[2] = k->do_hw_op(s, IPMI_RESET_CHASSIS, 1);
         break;
 
     case IPMI_BMC_WATCHDOG_ACTION_POWER_DOWN:
-        rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 1);
+        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 1);
         break;
 
     case IPMI_BMC_WATCHDOG_ACTION_POWER_CYCLE:
-        rsp[2] = k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 1);
+        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 1);
         break;
 
     default:
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
     }
-    if (rsp[2]) {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+    if (rsp->buffer[2]) {
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
 
@@ -1149,14 +1144,14 @@ static void set_watchdog_timer(IPMIBmcSi
     case IPMI_BMC_WATCHDOG_PRE_NMI:
         if (!k->do_hw_op(s, IPMI_SEND_NMI, 1)) {
             /* NMI not supported. */
-            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+            rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
             return;
         }
         break;
 
     default:
         /* We don't support PRE_SMI */
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
 
@@ -1175,60 +1170,56 @@ static void set_watchdog_timer(IPMIBmcSi
 
 static void get_watchdog_timer(IPMIBmcSim *ibs,
                                uint8_t *cmd, unsigned int cmd_len,
-                               uint8_t *rsp, unsigned int *rsp_len,
-                               unsigned int max_rsp_len)
+                               RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(ibs->watchdog_use);
-    IPMI_ADD_RSP_DATA(ibs->watchdog_action);
-    IPMI_ADD_RSP_DATA(ibs->watchdog_pretimeout);
-    IPMI_ADD_RSP_DATA(ibs->watchdog_expired);
+    rsp_buffer_push(rsp, ibs->watchdog_use);
+    rsp_buffer_push(rsp, ibs->watchdog_action);
+    rsp_buffer_push(rsp, ibs->watchdog_pretimeout);
+    rsp_buffer_push(rsp, ibs->watchdog_expired);
     if (ibs->watchdog_running) {
         long timeout;
         timeout = ((ibs->watchdog_expiry - ipmi_getmonotime() + 50000000)
                    / 100000000);
-        IPMI_ADD_RSP_DATA(timeout & 0xff);
-        IPMI_ADD_RSP_DATA((timeout >> 8) & 0xff);
+        rsp_buffer_push(rsp, timeout & 0xff);
+        rsp_buffer_push(rsp, (timeout >> 8) & 0xff);
     } else {
-        IPMI_ADD_RSP_DATA(0);
-        IPMI_ADD_RSP_DATA(0);
+        rsp_buffer_push(rsp, 0);
+        rsp_buffer_push(rsp, 0);
     }
 }
 
 static void get_sdr_rep_info(IPMIBmcSim *ibs,
                              uint8_t *cmd, unsigned int cmd_len,
-                             uint8_t *rsp, unsigned int *rsp_len,
-                             unsigned int max_rsp_len)
+                             RspBuffer *rsp)
 {
     unsigned int i;
 
-    IPMI_ADD_RSP_DATA(0x51); /* Conform to IPMI 1.5 spec */
-    IPMI_ADD_RSP_DATA(ibs->sdr.next_rec_id & 0xff);
-    IPMI_ADD_RSP_DATA((ibs->sdr.next_rec_id >> 8) & 0xff);
-    IPMI_ADD_RSP_DATA((MAX_SDR_SIZE - ibs->sdr.next_free) & 0xff);
-    IPMI_ADD_RSP_DATA(((MAX_SDR_SIZE - ibs->sdr.next_free) >> 8) & 0xff);
+    rsp_buffer_push(rsp, 0x51); /* Conform to IPMI 1.5 spec */
+    rsp_buffer_push(rsp, ibs->sdr.next_rec_id & 0xff);
+    rsp_buffer_push(rsp, (ibs->sdr.next_rec_id >> 8) & 0xff);
+    rsp_buffer_push(rsp, (MAX_SDR_SIZE - ibs->sdr.next_free) & 0xff);
+    rsp_buffer_push(rsp, ((MAX_SDR_SIZE - ibs->sdr.next_free) >> 8) & 0xff);
     for (i = 0; i < 4; i++) {
-        IPMI_ADD_RSP_DATA(ibs->sdr.last_addition[i]);
+        rsp_buffer_push(rsp, ibs->sdr.last_addition[i]);
     }
     for (i = 0; i < 4; i++) {
-        IPMI_ADD_RSP_DATA(ibs->sdr.last_clear[i]);
+        rsp_buffer_push(rsp, ibs->sdr.last_clear[i]);
     }
     /* Only modal support, reserve supported */
-    IPMI_ADD_RSP_DATA((ibs->sdr.overflow << 7) | 0x22);
+    rsp_buffer_push(rsp, (ibs->sdr.overflow << 7) | 0x22);
 }
 
 static void reserve_sdr_rep(IPMIBmcSim *ibs,
                             uint8_t *cmd, unsigned int cmd_len,
-                            uint8_t *rsp, unsigned int *rsp_len,
-                            unsigned int max_rsp_len)
+                            RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(ibs->sdr.reservation & 0xff);
-    IPMI_ADD_RSP_DATA((ibs->sdr.reservation >> 8) & 0xff);
+    rsp_buffer_push(rsp, ibs->sdr.reservation & 0xff);
+    rsp_buffer_push(rsp, (ibs->sdr.reservation >> 8) & 0xff);
 }
 
 static void get_sdr(IPMIBmcSim *ibs,
                     uint8_t *cmd, unsigned int cmd_len,
-                    uint8_t *rsp, unsigned int *rsp_len,
-                    unsigned int max_rsp_len)
+                    RspBuffer *rsp)
 {
     unsigned int pos;
     uint16_t nextrec;
@@ -1240,108 +1231,103 @@ static void get_sdr(IPMIBmcSim *ibs,
     pos = 0;
     if (sdr_find_entry(&ibs->sdr, cmd[4] | (cmd[5] << 8),
                        &pos, &nextrec)) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
 
     sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos];
 
     if (cmd[6] > ipmi_sdr_length(sdrh)) {
-        rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE;
+        rsp->buffer[2] = IPMI_CC_PARM_OUT_OF_RANGE;
         return;
     }
 
-    IPMI_ADD_RSP_DATA(nextrec & 0xff);
-    IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff);
+    rsp_buffer_push(rsp, nextrec & 0xff);
+    rsp_buffer_push(rsp, (nextrec >> 8) & 0xff);
 
     if (cmd[7] == 0xff) {
         cmd[7] = ipmi_sdr_length(sdrh) - cmd[6];
     }
 
-    if ((cmd[7] + *rsp_len) > max_rsp_len) {
-        rsp[2] = IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES;
+    if ((cmd[7] + rsp->len) > sizeof(rsp->buffer)) {
+        rsp->buffer[2] = IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES;
         return;
     }
-    memcpy(rsp + *rsp_len, ibs->sdr.sdr + pos + cmd[6], cmd[7]);
-    *rsp_len += cmd[7];
+
+    rsp_buffer_pushmore(rsp, ibs->sdr.sdr + pos + cmd[6], cmd[7]);
 }
 
 static void add_sdr(IPMIBmcSim *ibs,
                     uint8_t *cmd, unsigned int cmd_len,
-                    uint8_t *rsp, unsigned int *rsp_len,
-                    unsigned int max_rsp_len)
+                    RspBuffer *rsp)
 {
     uint16_t recid;
     struct ipmi_sdr_header *sdrh = (struct ipmi_sdr_header *) cmd + 2;
 
     if (sdr_add_entry(ibs, sdrh, cmd_len - 2, &recid)) {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
-    IPMI_ADD_RSP_DATA(recid & 0xff);
-    IPMI_ADD_RSP_DATA((recid >> 8) & 0xff);
+    rsp_buffer_push(rsp, recid & 0xff);
+    rsp_buffer_push(rsp, (recid >> 8) & 0xff);
 }
 
 static void clear_sdr_rep(IPMIBmcSim *ibs,
                           uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                          RspBuffer *rsp)
 {
     IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
     if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
     if (cmd[7] == 0xaa) {
         ibs->sdr.next_free = 0;
         ibs->sdr.overflow = 0;
         set_timestamp(ibs, ibs->sdr.last_clear);
-        IPMI_ADD_RSP_DATA(1); /* Erasure complete */
+        rsp_buffer_push(rsp, 1); /* Erasure complete */
         sdr_inc_reservation(&ibs->sdr);
     } else if (cmd[7] == 0) {
-        IPMI_ADD_RSP_DATA(1); /* Erasure complete */
+        rsp_buffer_push(rsp, 1); /* Erasure complete */
     } else {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
 }
 
 static void get_sel_info(IPMIBmcSim *ibs,
                          uint8_t *cmd, unsigned int cmd_len,
-                         uint8_t *rsp, unsigned int *rsp_len,
-                         unsigned int max_rsp_len)
+                         RspBuffer *rsp)
 {
     unsigned int i, val;
 
-    IPMI_ADD_RSP_DATA(0x51); /* Conform to IPMI 1.5 */
-    IPMI_ADD_RSP_DATA(ibs->sel.next_free & 0xff);
-    IPMI_ADD_RSP_DATA((ibs->sel.next_free >> 8) & 0xff);
+    rsp_buffer_push(rsp, 0x51); /* Conform to IPMI 1.5 */
+    rsp_buffer_push(rsp, ibs->sel.next_free & 0xff);
+    rsp_buffer_push(rsp, (ibs->sel.next_free >> 8) & 0xff);
     val = (MAX_SEL_SIZE - ibs->sel.next_free) * 16;
-    IPMI_ADD_RSP_DATA(val & 0xff);
-    IPMI_ADD_RSP_DATA((val >> 8) & 0xff);
+    rsp_buffer_push(rsp, val & 0xff);
+    rsp_buffer_push(rsp, (val >> 8) & 0xff);
     for (i = 0; i < 4; i++) {
-        IPMI_ADD_RSP_DATA(ibs->sel.last_addition[i]);
+        rsp_buffer_push(rsp, ibs->sel.last_addition[i]);
     }
     for (i = 0; i < 4; i++) {
-        IPMI_ADD_RSP_DATA(ibs->sel.last_clear[i]);
+        rsp_buffer_push(rsp, ibs->sel.last_clear[i]);
     }
     /* Only support Reserve SEL */
-    IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
+    rsp_buffer_push(rsp, (ibs->sel.overflow << 7) | 0x02);
 }
 
 static void reserve_sel(IPMIBmcSim *ibs,
                         uint8_t *cmd, unsigned int cmd_len,
-                        uint8_t *rsp, unsigned int *rsp_len,
-                        unsigned int max_rsp_len)
+                        RspBuffer *rsp)
 {
-    IPMI_ADD_RSP_DATA(ibs->sel.reservation & 0xff);
-    IPMI_ADD_RSP_DATA((ibs->sel.reservation >> 8) & 0xff);
+    rsp_buffer_push(rsp, ibs->sel.reservation & 0xff);
+    rsp_buffer_push(rsp, (ibs->sel.reservation >> 8) & 0xff);
 }
 
 static void get_sel_entry(IPMIBmcSim *ibs,
                           uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                          RspBuffer *rsp)
 {
     unsigned int val;
 
@@ -1349,17 +1335,17 @@ static void get_sel_entry(IPMIBmcSim *ib
         IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
     }
     if (ibs->sel.next_free == 0) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     if (cmd[6] > 15) {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
     if (cmd[7] == 0xff) {
         cmd[7] = 16;
     } else if ((cmd[7] + cmd[6]) > 16) {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     } else {
         cmd[7] += cmd[6];
@@ -1369,79 +1355,75 @@ static void get_sel_entry(IPMIBmcSim *ib
     if (val == 0xffff) {
         val = ibs->sel.next_free - 1;
     } else if (val >= ibs->sel.next_free) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     if ((val + 1) == ibs->sel.next_free) {
-        IPMI_ADD_RSP_DATA(0xff);
-        IPMI_ADD_RSP_DATA(0xff);
+        rsp_buffer_push(rsp, 0xff);
+        rsp_buffer_push(rsp, 0xff);
     } else {
-        IPMI_ADD_RSP_DATA((val + 1) & 0xff);
-        IPMI_ADD_RSP_DATA(((val + 1) >> 8) & 0xff);
+        rsp_buffer_push(rsp, (val + 1) & 0xff);
+        rsp_buffer_push(rsp, ((val + 1) >> 8) & 0xff);
     }
     for (; cmd[6] < cmd[7]; cmd[6]++) {
-        IPMI_ADD_RSP_DATA(ibs->sel.sel[val][cmd[6]]);
+        rsp_buffer_push(rsp, ibs->sel.sel[val][cmd[6]]);
     }
 }
 
 static void add_sel_entry(IPMIBmcSim *ibs,
                           uint8_t *cmd, unsigned int cmd_len,
-                          uint8_t *rsp, unsigned int *rsp_len,
-                          unsigned int max_rsp_len)
+                          RspBuffer *rsp)
 {
     if (sel_add_event(ibs, cmd + 2)) {
-        rsp[2] = IPMI_CC_OUT_OF_SPACE;
+        rsp->buffer[2] = IPMI_CC_OUT_OF_SPACE;
         return;
     }
     /* sel_add_event fills in the record number. */
-    IPMI_ADD_RSP_DATA(cmd[2]);
-    IPMI_ADD_RSP_DATA(cmd[3]);
+    rsp_buffer_push(rsp, cmd[2]);
+    rsp_buffer_push(rsp, cmd[3]);
 }
 
 static void clear_sel(IPMIBmcSim *ibs,
                       uint8_t *cmd, unsigned int cmd_len,
-                      uint8_t *rsp, unsigned int *rsp_len,
-                      unsigned int max_rsp_len)
+                      RspBuffer *rsp)
 {
     IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
     if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
     if (cmd[7] == 0xaa) {
         ibs->sel.next_free = 0;
         ibs->sel.overflow = 0;
         set_timestamp(ibs, ibs->sdr.last_clear);
-        IPMI_ADD_RSP_DATA(1); /* Erasure complete */
+        rsp_buffer_push(rsp, 1); /* Erasure complete */
         sel_inc_reservation(&ibs->sel);
     } else if (cmd[7] == 0) {
-        IPMI_ADD_RSP_DATA(1); /* Erasure complete */
+        rsp_buffer_push(rsp, 1); /* Erasure complete */
     } else {
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
 }
 
 static void get_sel_time(IPMIBmcSim *ibs,
                          uint8_t *cmd, unsigned int cmd_len,
-                         uint8_t *rsp, unsigned int *rsp_len,
-                         unsigned int max_rsp_len)
+                         RspBuffer *rsp)
 {
     uint32_t val;
     struct ipmi_time now;
 
     ipmi_gettime(&now);
     val = now.tv_sec + ibs->sel.time_offset;
-    IPMI_ADD_RSP_DATA(val & 0xff);
-    IPMI_ADD_RSP_DATA((val >> 8) & 0xff);
-    IPMI_ADD_RSP_DATA((val >> 16) & 0xff);
-    IPMI_ADD_RSP_DATA((val >> 24) & 0xff);
+    rsp_buffer_push(rsp, val & 0xff);
+    rsp_buffer_push(rsp, (val >> 8) & 0xff);
+    rsp_buffer_push(rsp, (val >> 16) & 0xff);
+    rsp_buffer_push(rsp, (val >> 24) & 0xff);
 }
 
 static void set_sel_time(IPMIBmcSim *ibs,
                          uint8_t *cmd, unsigned int cmd_len,
-                         uint8_t *rsp, unsigned int *rsp_len,
-                         unsigned int max_rsp_len)
+                         RspBuffer *rsp)
 {
     uint32_t val;
     struct ipmi_time now;
@@ -1453,14 +1435,13 @@ static void set_sel_time(IPMIBmcSim *ibs
 
 static void set_sensor_evt_enable(IPMIBmcSim *ibs,
                                   uint8_t *cmd, unsigned int cmd_len,
-                                  uint8_t *rsp, unsigned int *rsp_len,
-                                  unsigned int max_rsp_len)
+                                  RspBuffer *rsp)
 {
     IPMISensor *sens;
 
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1496,7 +1477,7 @@ static void set_sensor_evt_enable(IPMIBm
         }
         break;
     case 3:
-        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
     }
     IPMI_SENSOR_SET_RET_STATUS(sens, cmd[3]);
@@ -1504,34 +1485,32 @@ static void set_sensor_evt_enable(IPMIBm
 
 static void get_sensor_evt_enable(IPMIBmcSim *ibs,
                                   uint8_t *cmd, unsigned int cmd_len,
-                                  uint8_t *rsp, unsigned int *rsp_len,
-                                  unsigned int max_rsp_len)
+                                  RspBuffer *rsp)
 {
     IPMISensor *sens;
 
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     sens = ibs->sensors + cmd[2];
-    IPMI_ADD_RSP_DATA(IPMI_SENSOR_GET_RET_STATUS(sens));
-    IPMI_ADD_RSP_DATA(sens->assert_enable & 0xff);
-    IPMI_ADD_RSP_DATA((sens->assert_enable >> 8) & 0xff);
-    IPMI_ADD_RSP_DATA(sens->deassert_enable & 0xff);
-    IPMI_ADD_RSP_DATA((sens->deassert_enable >> 8) & 0xff);
+    rsp_buffer_push(rsp, IPMI_SENSOR_GET_RET_STATUS(sens));
+    rsp_buffer_push(rsp, sens->assert_enable & 0xff);
+    rsp_buffer_push(rsp, (sens->assert_enable >> 8) & 0xff);
+    rsp_buffer_push(rsp, sens->deassert_enable & 0xff);
+    rsp_buffer_push(rsp, (sens->deassert_enable >> 8) & 0xff);
 }
 
 static void rearm_sensor_evts(IPMIBmcSim *ibs,
                               uint8_t *cmd, unsigned int cmd_len,
-                              uint8_t *rsp, unsigned int *rsp_len,
-                              unsigned int max_rsp_len)
+                              RspBuffer *rsp)
 {
     IPMISensor *sens;
 
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1545,57 +1524,54 @@ static void rearm_sensor_evts(IPMIBmcSim
 
 static void get_sensor_evt_status(IPMIBmcSim *ibs,
                                   uint8_t *cmd, unsigned int cmd_len,
-                                  uint8_t *rsp, unsigned int *rsp_len,
-                                  unsigned int max_rsp_len)
+                                  RspBuffer *rsp)
 {
     IPMISensor *sens;
 
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     sens = ibs->sensors + cmd[2];
-    IPMI_ADD_RSP_DATA(sens->reading);
-    IPMI_ADD_RSP_DATA(IPMI_SENSOR_GET_RET_STATUS(sens));
-    IPMI_ADD_RSP_DATA(sens->assert_states & 0xff);
-    IPMI_ADD_RSP_DATA((sens->assert_states >> 8) & 0xff);
-    IPMI_ADD_RSP_DATA(sens->deassert_states & 0xff);
-    IPMI_ADD_RSP_DATA((sens->deassert_states >> 8) & 0xff);
+    rsp_buffer_push(rsp, sens->reading);
+    rsp_buffer_push(rsp, IPMI_SENSOR_GET_RET_STATUS(sens));
+    rsp_buffer_push(rsp, sens->assert_states & 0xff);
+    rsp_buffer_push(rsp, (sens->assert_states >> 8) & 0xff);
+    rsp_buffer_push(rsp, sens->deassert_states & 0xff);
+    rsp_buffer_push(rsp, (sens->deassert_states >> 8) & 0xff);
 }
 
 static void get_sensor_reading(IPMIBmcSim *ibs,
                                uint8_t *cmd, unsigned int cmd_len,
-                               uint8_t *rsp, unsigned int *rsp_len,
-                               unsigned int max_rsp_len)
+                               RspBuffer *rsp)
 {
     IPMISensor *sens;
 
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     sens = ibs->sensors + cmd[2];
-    IPMI_ADD_RSP_DATA(sens->reading);
-    IPMI_ADD_RSP_DATA(IPMI_SENSOR_GET_RET_STATUS(sens));
-    IPMI_ADD_RSP_DATA(sens->states & 0xff);
+    rsp_buffer_push(rsp, sens->reading);
+    rsp_buffer_push(rsp, IPMI_SENSOR_GET_RET_STATUS(sens));
+    rsp_buffer_push(rsp, sens->states & 0xff);
     if (IPMI_SENSOR_IS_DISCRETE(sens)) {
-        IPMI_ADD_RSP_DATA((sens->states >> 8) & 0xff);
+        rsp_buffer_push(rsp, (sens->states >> 8) & 0xff);
     }
 }
 
 static void set_sensor_type(IPMIBmcSim *ibs,
-                               uint8_t *cmd, unsigned int cmd_len,
-                               uint8_t *rsp, unsigned int *rsp_len,
-                               unsigned int max_rsp_len)
+                            uint8_t *cmd, unsigned int cmd_len,
+                            RspBuffer *rsp)
 {
     IPMISensor *sens;
 
 
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1604,21 +1580,20 @@ static void set_sensor_type(IPMIBmcSim *
 }
 
 static void get_sensor_type(IPMIBmcSim *ibs,
-                               uint8_t *cmd, unsigned int cmd_len,
-                               uint8_t *rsp, unsigned int *rsp_len,
-                               unsigned int max_rsp_len)
+                            uint8_t *cmd, unsigned int cmd_len,
+                            RspBuffer *rsp)
 {
     IPMISensor *sens;
 
 
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
         return;
     }
     sens = ibs->sensors + cmd[2];
-    IPMI_ADD_RSP_DATA(sens->sensor_type);
-    IPMI_ADD_RSP_DATA(sens->evt_reading_type_code);
+    rsp_buffer_push(rsp, sens->sensor_type);
+    rsp_buffer_push(rsp, sens->evt_reading_type_code);
 }
 
 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 03/10] ipmi: remove IPMI_CHECK_RESERVATION() macro
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 02/10] ipmi: replace IPMI_ADD_RSP_DATA() macro with inline helpers Cédric Le Goater
@ 2016-03-10 14:03 ` Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 04/10] ipmi: add rsp_buffer_set_error() helper Cédric Le Goater
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:03 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

Some IPMI command handlers in the BMC simulator use a macro
IPMI_CHECK_RESERVATION() to check a SDR reservation but the macro
implicitly uses local variables. This patch simply removes it.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/ipmi_bmc_sim.c |   33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -292,16 +292,6 @@ static inline void rsp_buffer_pushmore(R
     rsp->len += n;
 }
 
-/* Check that the reservation in the command is valid. */
-#define IPMI_CHECK_RESERVATION(off, r) \
-    do {                                                   \
-        if ((cmd[off] | (cmd[off + 1] << 8)) != r) {       \
-            rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;     \
-            return;                                        \
-        }                                                  \
-    } while (0)
-
-
 static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs);
 
 static void ipmi_gettime(struct ipmi_time *time)
@@ -1226,8 +1216,12 @@ static void get_sdr(IPMIBmcSim *ibs,
     struct ipmi_sdr_header *sdrh;
 
     if (cmd[6]) {
-        IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
+        if ((cmd[2] | (cmd[3] << 8)) != ibs->sdr.reservation) {
+            rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;
+            return;
+        }
     }
+
     pos = 0;
     if (sdr_find_entry(&ibs->sdr, cmd[4] | (cmd[5] << 8),
                        &pos, &nextrec)) {
@@ -1276,7 +1270,11 @@ static void clear_sdr_rep(IPMIBmcSim *ib
                           uint8_t *cmd, unsigned int cmd_len,
                           RspBuffer *rsp)
 {
-    IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
+    if ((cmd[2] | (cmd[3] << 8)) != ibs->sdr.reservation) {
+        rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;
+        return;
+    }
+
     if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
         rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;
@@ -1332,7 +1330,10 @@ static void get_sel_entry(IPMIBmcSim *ib
     unsigned int val;
 
     if (cmd[6]) {
-        IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
+        if ((cmd[2] | (cmd[3] << 8)) != ibs->sel.reservation) {
+            rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;
+            return;
+        }
     }
     if (ibs->sel.next_free == 0) {
         rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
@@ -1387,7 +1388,11 @@ static void clear_sel(IPMIBmcSim *ibs,
                       uint8_t *cmd, unsigned int cmd_len,
                       RspBuffer *rsp)
 {
-    IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
+    if ((cmd[2] | (cmd[3] << 8)) != ibs->sel.reservation) {
+        rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;
+        return;
+    }
+
     if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
         rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
         return;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 04/10] ipmi: add rsp_buffer_set_error() helper
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
                   ` (2 preceding siblings ...)
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 03/10] ipmi: remove IPMI_CHECK_RESERVATION() macro Cédric Le Goater
@ 2016-03-10 14:03 ` Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 05/10] ipmi: add a realize function to the device class Cédric Le Goater
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:03 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

The third byte in the response buffer of an IPMI command holds the
error code. In many IPMI command handlers, this byte is updated
directly. This patch adds a helper routine to clarify why this byte is
being used.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
---

Changes since v2:

 - changed 'struct rsp_buffer' to 'RspBuffer'
 
 hw/ipmi/ipmi_bmc_sim.c |  115 +++++++++++++++++++++++++------------------------
 1 file changed, 60 insertions(+), 55 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -270,11 +270,16 @@ struct RspBuffer {
 
 #define RSP_BUFFER_INITIALIZER { }
 
+static inline void rsp_buffer_set_error(RspBuffer *rsp, uint8_t byte)
+{
+    rsp->buffer[2] = byte;
+}
+
 /* Add a byte to the response. */
 static inline void rsp_buffer_push(RspBuffer *rsp, uint8_t byte)
 {
     if (rsp->len >= sizeof(rsp->buffer)) {
-        rsp->buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQUEST_DATA_TRUNCATED);
         return;
     }
     rsp->buffer[rsp->len++] = byte;
@@ -284,7 +289,7 @@ static inline void rsp_buffer_pushmore(R
                                        unsigned int n)
 {
     if (rsp->len + n >= sizeof(rsp->buffer)) {
-        rsp->buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQUEST_DATA_TRUNCATED);
         return;
     }
 
@@ -620,7 +625,7 @@ static void ipmi_sim_handle_command(IPMI
     /* Set up the response, set the low bit of NETFN. */
     /* Note that max_rsp_len must be at least 3 */
     if (sizeof(rsp.buffer) < 3) {
-        rsp.buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+        rsp_buffer_set_error(&rsp, IPMI_CC_REQUEST_DATA_TRUNCATED);
         goto out;
     }
 
@@ -630,28 +635,28 @@ static void ipmi_sim_handle_command(IPMI
 
     /* If it's too short or it was truncated, return an error. */
     if (cmd_len < 2) {
-        rsp.buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        rsp_buffer_set_error(&rsp, IPMI_CC_REQUEST_DATA_LENGTH_INVALID);
         goto out;
     }
     if (cmd_len > max_cmd_len) {
-        rsp.buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+        rsp_buffer_set_error(&rsp, IPMI_CC_REQUEST_DATA_TRUNCATED);
         goto out;
     }
 
     if ((cmd[0] & 0x03) != 0) {
         /* Only have stuff on LUN 0 */
-        rsp.buffer[2] = IPMI_CC_COMMAND_INVALID_FOR_LUN;
+        rsp_buffer_set_error(&rsp, IPMI_CC_COMMAND_INVALID_FOR_LUN);
         goto out;
     }
 
     hdl = ipmi_get_handler(ibs, cmd[0] >> 2, cmd[1]);
     if (!hdl) {
-        rsp.buffer[2] = IPMI_CC_INVALID_CMD;
+        rsp_buffer_set_error(&rsp, IPMI_CC_INVALID_CMD);
         goto out;
     }
 
     if (cmd_len < hdl->cmd_len_min) {
-        rsp.buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        rsp_buffer_set_error(&rsp, IPMI_CC_REQUEST_DATA_LENGTH_INVALID);
         goto out;
     }
 
@@ -761,26 +766,26 @@ static void chassis_control(IPMIBmcSim *
 
     switch (cmd[2] & 0xf) {
     case 0: /* power down */
-        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0));
         break;
     case 1: /* power up */
-        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWERON_CHASSIS, 0);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_POWERON_CHASSIS, 0));
         break;
     case 2: /* power cycle */
-        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 0);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 0));
         break;
     case 3: /* hard reset */
-        rsp->buffer[2] = k->do_hw_op(s, IPMI_RESET_CHASSIS, 0);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_RESET_CHASSIS, 0));
         break;
     case 4: /* pulse diagnostic interrupt */
-        rsp->buffer[2] = k->do_hw_op(s, IPMI_PULSE_DIAG_IRQ, 0);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_PULSE_DIAG_IRQ, 0));
         break;
     case 5: /* soft shutdown via ACPI by overtemp emulation */
-        rsp->buffer[2] = k->do_hw_op(s,
-                             IPMI_SHUTDOWN_VIA_ACPI_OVERTEMP, 0);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s,
+                                          IPMI_SHUTDOWN_VIA_ACPI_OVERTEMP, 0));
         break;
     default:
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
 }
@@ -919,7 +924,7 @@ static void read_evt_msg_buf(IPMIBmcSim
     unsigned int i;
 
     if (!(ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL)) {
-        rsp->buffer[2] = 0x80;
+        rsp_buffer_set_error(rsp, 0x80);
         return;
     }
     for (i = 0; i < 16; i++) {
@@ -937,7 +942,7 @@ static void get_msg(IPMIBmcSim *ibs,
 
     qemu_mutex_lock(&ibs->lock);
     if (QTAILQ_EMPTY(&ibs->rcvbufs)) {
-        rsp->buffer[2] = 0x80; /* Queue empty */
+        rsp_buffer_set_error(rsp, 0x80); /* Queue empty */
         goto out;
     }
     rsp_buffer_push(rsp, 0); /* Channel 0 */
@@ -981,18 +986,18 @@ static void send_msg(IPMIBmcSim *ibs,
 
     if (cmd[2] != 0) {
         /* We only handle channel 0 with no options */
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
 
     if (cmd_len < 10) {
-        rsp->buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQUEST_DATA_LENGTH_INVALID);
         return;
     }
 
     if (cmd[3] != 0x40) {
         /* We only emulate a MC at address 0x40. */
-        rsp->buffer[2] = 0x83; /* NAK on write */
+        rsp_buffer_set_error(rsp, 0x83); /* NAK on write */
         return;
     }
 
@@ -1081,7 +1086,7 @@ static void reset_watchdog_timer(IPMIBmc
                                  RspBuffer *rsp)
 {
     if (!ibs->watchdog_initialized) {
-        rsp->buffer[2] = 0x80;
+        rsp_buffer_set_error(rsp, 0x80);
         return;
     }
     do_watchdog_reset(ibs);
@@ -1097,7 +1102,7 @@ static void set_watchdog_timer(IPMIBmcSi
 
     val = cmd[2] & 0x7; /* Validate use */
     if (val == 0 || val > 5) {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
     val = cmd[3] & 0x7; /* Validate action */
@@ -1106,22 +1111,22 @@ static void set_watchdog_timer(IPMIBmcSi
         break;
 
     case IPMI_BMC_WATCHDOG_ACTION_RESET:
-        rsp->buffer[2] = k->do_hw_op(s, IPMI_RESET_CHASSIS, 1);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_RESET_CHASSIS, 1));
         break;
 
     case IPMI_BMC_WATCHDOG_ACTION_POWER_DOWN:
-        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 1);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 1));
         break;
 
     case IPMI_BMC_WATCHDOG_ACTION_POWER_CYCLE:
-        rsp->buffer[2] = k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 1);
+        rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 1));
         break;
 
     default:
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
     }
     if (rsp->buffer[2]) {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
 
@@ -1134,14 +1139,14 @@ static void set_watchdog_timer(IPMIBmcSi
     case IPMI_BMC_WATCHDOG_PRE_NMI:
         if (!k->do_hw_op(s, IPMI_SEND_NMI, 1)) {
             /* NMI not supported. */
-            rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+            rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
             return;
         }
         break;
 
     default:
         /* We don't support PRE_SMI */
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
 
@@ -1217,7 +1222,7 @@ static void get_sdr(IPMIBmcSim *ibs,
 
     if (cmd[6]) {
         if ((cmd[2] | (cmd[3] << 8)) != ibs->sdr.reservation) {
-            rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;
+            rsp_buffer_set_error(rsp, IPMI_CC_INVALID_RESERVATION);
             return;
         }
     }
@@ -1225,14 +1230,14 @@ static void get_sdr(IPMIBmcSim *ibs,
     pos = 0;
     if (sdr_find_entry(&ibs->sdr, cmd[4] | (cmd[5] << 8),
                        &pos, &nextrec)) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
 
     sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos];
 
     if (cmd[6] > ipmi_sdr_length(sdrh)) {
-        rsp->buffer[2] = IPMI_CC_PARM_OUT_OF_RANGE;
+        rsp_buffer_set_error(rsp, IPMI_CC_PARM_OUT_OF_RANGE);
         return;
     }
 
@@ -1244,7 +1249,7 @@ static void get_sdr(IPMIBmcSim *ibs,
     }
 
     if ((cmd[7] + rsp->len) > sizeof(rsp->buffer)) {
-        rsp->buffer[2] = IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES;
+        rsp_buffer_set_error(rsp, IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES);
         return;
     }
 
@@ -1259,7 +1264,7 @@ static void add_sdr(IPMIBmcSim *ibs,
     struct ipmi_sdr_header *sdrh = (struct ipmi_sdr_header *) cmd + 2;
 
     if (sdr_add_entry(ibs, sdrh, cmd_len - 2, &recid)) {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
     rsp_buffer_push(rsp, recid & 0xff);
@@ -1271,12 +1276,12 @@ static void clear_sdr_rep(IPMIBmcSim *ib
                           RspBuffer *rsp)
 {
     if ((cmd[2] | (cmd[3] << 8)) != ibs->sdr.reservation) {
-        rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_RESERVATION);
         return;
     }
 
     if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
     if (cmd[7] == 0xaa) {
@@ -1288,7 +1293,7 @@ static void clear_sdr_rep(IPMIBmcSim *ib
     } else if (cmd[7] == 0) {
         rsp_buffer_push(rsp, 1); /* Erasure complete */
     } else {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
 }
@@ -1331,22 +1336,22 @@ static void get_sel_entry(IPMIBmcSim *ib
 
     if (cmd[6]) {
         if ((cmd[2] | (cmd[3] << 8)) != ibs->sel.reservation) {
-            rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;
+            rsp_buffer_set_error(rsp, IPMI_CC_INVALID_RESERVATION);
             return;
         }
     }
     if (ibs->sel.next_free == 0) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     if (cmd[6] > 15) {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
     if (cmd[7] == 0xff) {
         cmd[7] = 16;
     } else if ((cmd[7] + cmd[6]) > 16) {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     } else {
         cmd[7] += cmd[6];
@@ -1356,7 +1361,7 @@ static void get_sel_entry(IPMIBmcSim *ib
     if (val == 0xffff) {
         val = ibs->sel.next_free - 1;
     } else if (val >= ibs->sel.next_free) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     if ((val + 1) == ibs->sel.next_free) {
@@ -1376,7 +1381,7 @@ static void add_sel_entry(IPMIBmcSim *ib
                           RspBuffer *rsp)
 {
     if (sel_add_event(ibs, cmd + 2)) {
-        rsp->buffer[2] = IPMI_CC_OUT_OF_SPACE;
+        rsp_buffer_set_error(rsp, IPMI_CC_OUT_OF_SPACE);
         return;
     }
     /* sel_add_event fills in the record number. */
@@ -1389,12 +1394,12 @@ static void clear_sel(IPMIBmcSim *ibs,
                       RspBuffer *rsp)
 {
     if ((cmd[2] | (cmd[3] << 8)) != ibs->sel.reservation) {
-        rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_RESERVATION);
         return;
     }
 
     if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
     if (cmd[7] == 0xaa) {
@@ -1406,7 +1411,7 @@ static void clear_sel(IPMIBmcSim *ibs,
     } else if (cmd[7] == 0) {
         rsp_buffer_push(rsp, 1); /* Erasure complete */
     } else {
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
 }
@@ -1446,7 +1451,7 @@ static void set_sensor_evt_enable(IPMIBm
 
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1482,7 +1487,7 @@ static void set_sensor_evt_enable(IPMIBm
         }
         break;
     case 3:
-        rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
         return;
     }
     IPMI_SENSOR_SET_RET_STATUS(sens, cmd[3]);
@@ -1496,7 +1501,7 @@ static void get_sensor_evt_enable(IPMIBm
 
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1515,7 +1520,7 @@ static void rearm_sensor_evts(IPMIBmcSim
 
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1535,7 +1540,7 @@ static void get_sensor_evt_status(IPMIBm
 
     if ((cmd[2] >= MAX_SENSORS) ||
         !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1555,7 +1560,7 @@ static void get_sensor_reading(IPMIBmcSi
 
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1576,7 +1581,7 @@ static void set_sensor_type(IPMIBmcSim *
 
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     sens = ibs->sensors + cmd[2];
@@ -1593,7 +1598,7 @@ static void get_sensor_type(IPMIBmcSim *
 
     if ((cmd[2] >= MAX_SENSORS) ||
             !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
-        rsp->buffer[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
+        rsp_buffer_set_error(rsp, IPMI_CC_REQ_ENTRY_NOT_PRESENT);
         return;
     }
     sens = ibs->sensors + cmd[2];

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 05/10] ipmi: add a realize function to the device class
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
                   ` (3 preceding siblings ...)
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 04/10] ipmi: add rsp_buffer_set_error() helper Cédric Le Goater
@ 2016-03-10 14:03 ` Cédric Le Goater
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 06/10] ipmi: use a function to initialize the SDR table Cédric Le Goater
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:03 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

This will be useful to define and use properties when the object is
instantiated.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---

Changes since v1:

 - removed empty properties.

 hw/ipmi/ipmi_bmc_sim.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -1722,9 +1722,9 @@ static const VMStateDescription vmstate_
     }
 };
 
-static void ipmi_sim_init(Object *obj)
+static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 {
-    IPMIBmc *b = IPMI_BMC(obj);
+    IPMIBmc *b = IPMI_BMC(dev);
     unsigned int i;
     unsigned int recid;
     IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
@@ -1783,8 +1783,10 @@ static void ipmi_sim_init(Object *obj)
 
 static void ipmi_sim_class_init(ObjectClass *oc, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(oc);
     IPMIBmcClass *bk = IPMI_BMC_CLASS(oc);
 
+    dc->realize = ipmi_sim_realize;
     bk->handle_command = ipmi_sim_handle_command;
 }
 
@@ -1792,7 +1794,6 @@ static const TypeInfo ipmi_sim_type = {
     .name          = TYPE_IPMI_BMC_SIMULATOR,
     .parent        = TYPE_IPMI_BMC,
     .instance_size = sizeof(IPMIBmcSim),
-    .instance_init = ipmi_sim_init,
     .class_init    = ipmi_sim_class_init,
 };
 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 06/10] ipmi: use a function to initialize the SDR table
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
                   ` (4 preceding siblings ...)
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 05/10] ipmi: add a realize function to the device class Cédric Le Goater
@ 2016-03-10 14:03 ` Cédric Le Goater
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 07/10] ipmi: remove the need of an ending record in " Cédric Le Goater
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:03 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

This patch moves the code section initializing the sdrs in its own
routine to prepare ground for changes in the subsequent patches.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/ipmi_bmc_sim.c |   49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -1694,6 +1694,33 @@ static const uint8_t init_sdrs[] = {
     0xff, 0xff, 0x00, 0x00, 0x00
 };
 
+static void ipmi_sdr_init(IPMIBmcSim *ibs)
+{
+    unsigned int i;
+    unsigned int recid;
+
+    for (i = 0;;) {
+        struct ipmi_sdr_header *sdrh;
+        int len;
+        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
+            error_report("Problem with recid 0x%4.4x", i);
+            return;
+        }
+        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
+        len = ipmi_sdr_length(sdrh);
+        recid = ipmi_sdr_recid(sdrh);
+        if (recid == 0xffff) {
+            break;
+        }
+        if ((i + len) > sizeof(init_sdrs)) {
+            error_report("Problem with recid 0x%4.4x", i);
+            return;
+        }
+        sdr_add_entry(ibs, sdrh, len, NULL);
+        i += len;
+    }
+}
+
 static const VMStateDescription vmstate_ipmi_sim = {
     .name = TYPE_IPMI_BMC_SIMULATOR,
     .version_id = 1,
@@ -1726,7 +1753,6 @@ static void ipmi_sim_realize(DeviceState
 {
     IPMIBmc *b = IPMI_BMC(dev);
     unsigned int i;
-    unsigned int recid;
     IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
 
     qemu_mutex_init(&ibs->lock);
@@ -1743,26 +1769,7 @@ static void ipmi_sim_realize(DeviceState
         ibs->sdr.last_clear[i] = 0xff;
     }
 
-    for (i = 0;;) {
-        struct ipmi_sdr_header *sdrh;
-        int len;
-        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
-            error_report("Problem with recid 0x%4.4x", i);
-            return;
-        }
-        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
-        len = ipmi_sdr_length(sdrh);
-        recid = ipmi_sdr_recid(sdrh);
-        if (recid == 0xffff) {
-            break;
-        }
-        if ((i + len) > sizeof(init_sdrs)) {
-            error_report("Problem with recid 0x%4.4x", i);
-            return;
-        }
-        sdr_add_entry(ibs, sdrh, len, NULL);
-        i += len;
-    }
+    ipmi_sdr_init(ibs);
 
     ibs->acpi_power_state[0] = 0;
     ibs->acpi_power_state[1] = 0;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 07/10] ipmi: remove the need of an ending record in the SDR table
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
                   ` (5 preceding siblings ...)
  2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 06/10] ipmi: use a function to initialize the SDR table Cédric Le Goater
@ 2016-03-10 14:04 ` Cédric Le Goater
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 08/10] ipmi: add some local variables in ipmi_sdr_init Cédric Le Goater
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:04 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

Currently, the code initializing the sdr table relies on an ending
record with a recid of 0xffff. This patch changes the loop to use the
sdr size as a breaking condition.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/ipmi/ipmi_bmc_sim.c |   13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -1690,34 +1690,27 @@ static const uint8_t init_sdrs[] = {
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc8,
     'W',  'a',  't',  'c',  'h',  'd',  'o',  'g',
-    /* End */
-    0xff, 0xff, 0x00, 0x00, 0x00
 };
 
 static void ipmi_sdr_init(IPMIBmcSim *ibs)
 {
     unsigned int i;
-    unsigned int recid;
+    int len;
 
-    for (i = 0;;) {
+    for (i = 0; i < sizeof(init_sdrs); i += len) {
         struct ipmi_sdr_header *sdrh;
-        int len;
+
         if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
             error_report("Problem with recid 0x%4.4x", i);
             return;
         }
         sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
         len = ipmi_sdr_length(sdrh);
-        recid = ipmi_sdr_recid(sdrh);
-        if (recid == 0xffff) {
-            break;
-        }
         if ((i + len) > sizeof(init_sdrs)) {
             error_report("Problem with recid 0x%4.4x", i);
             return;
         }
         sdr_add_entry(ibs, sdrh, len, NULL);
-        i += len;
     }
 }
 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 08/10] ipmi: add some local variables in ipmi_sdr_init
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
                   ` (6 preceding siblings ...)
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 07/10] ipmi: remove the need of an ending record in " Cédric Le Goater
@ 2016-03-10 14:04 ` Cédric Le Goater
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 09/10] ipmi: use a file to load SDRs Cédric Le Goater
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs Cédric Le Goater
  9 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:04 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

This patch adds a couple of variables to manipulate the raw sdr
entries. The const attribute is also removed on init_sdrs. This will
ease the introduction of a sdr loader using a file.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/ipmi/ipmi_bmc_sim.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -1683,7 +1683,7 @@ static void register_cmds(IPMIBmcSim *s)
     ipmi_register_netfn(s, IPMI_NETFN_STORAGE, &storage_netfn);
 }
 
-static const uint8_t init_sdrs[] = {
+static uint8_t init_sdrs[] = {
     /* Watchdog device */
     0x00, 0x00, 0x51, 0x02,   35, 0x20, 0x00, 0x00,
     0x23, 0x01, 0x63, 0x00, 0x23, 0x6f, 0x0f, 0x01,
@@ -1696,17 +1696,22 @@ static void ipmi_sdr_init(IPMIBmcSim *ib
 {
     unsigned int i;
     int len;
+    size_t sdrs_size;
+    uint8_t *sdrs;
 
-    for (i = 0; i < sizeof(init_sdrs); i += len) {
+    sdrs_size = sizeof(init_sdrs);
+    sdrs = init_sdrs;
+
+    for (i = 0; i < sdrs_size; i += len) {
         struct ipmi_sdr_header *sdrh;
 
-        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
+        if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
             error_report("Problem with recid 0x%4.4x", i);
             return;
         }
-        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
+        sdrh = (struct ipmi_sdr_header *) &sdrs[i];
         len = ipmi_sdr_length(sdrh);
-        if ((i + len) > sizeof(init_sdrs)) {
+        if (i + len > sdrs_size) {
             error_report("Problem with recid 0x%4.4x", i);
             return;
         }

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 09/10] ipmi: use a file to load SDRs
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
                   ` (7 preceding siblings ...)
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 08/10] ipmi: add some local variables in ipmi_sdr_init Cédric Le Goater
@ 2016-03-10 14:04 ` Cédric Le Goater
  2016-03-15 14:30   ` Michael S. Tsirkin
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs Cédric Le Goater
  9 siblings, 1 reply; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:04 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

The IPMI BMC simulator populates the sdr/sensor tables with a minimal
set of entries (Watchdog). But some qemu platforms might want to use
extra entries for their custom needs.

This patch modifies slighty the initializing routine to take into
account a larger set read from a file. The name of the file to use is
defined through a new 'sdr' property of the simulator device.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---

Changes since v1:

 - log an error if file does not exist.
 - change property name to 'sdrfile'
 - add documentation of new properties
 
 hw/ipmi/ipmi_bmc_sim.c |   23 +++++++++++++++++++++--
 qemu-options.hx        |   11 ++++++++++-
 2 files changed, 31 insertions(+), 3 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -27,6 +27,7 @@
 #include "qemu/timer.h"
 #include "hw/ipmi/ipmi.h"
 #include "qemu/error-report.h"
+#include "hw/loader.h"
 
 #define IPMI_NETFN_CHASSIS            0x00
 
@@ -213,6 +214,7 @@ struct IPMIBmcSim {
     IPMISel sel;
     IPMISdr sdr;
     IPMISensor sensors[MAX_SENSORS];
+    char *sdr_filename;
 
     /* Odd netfns are for responses, so we only need the even ones. */
     const IPMINetfn *netfns[MAX_NETFNS / 2];
@@ -1701,22 +1703,33 @@ static void ipmi_sdr_init(IPMIBmcSim *ib
 
     sdrs_size = sizeof(init_sdrs);
     sdrs = init_sdrs;
+    if (ibs->sdr_filename &&
+        !g_file_get_contents(ibs->sdr_filename, (gchar **) &sdrs, &sdrs_size,
+                             NULL)) {
+        error_report("failed to load sdr file '%s'", ibs->sdr_filename);
+        sdrs_size = sizeof(init_sdrs);
+        sdrs = init_sdrs;
+    }
 
     for (i = 0; i < sdrs_size; i += len) {
         struct ipmi_sdr_header *sdrh;
 
         if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
             error_report("Problem with recid 0x%4.4x", i);
-            return;
+            break;
         }
         sdrh = (struct ipmi_sdr_header *) &sdrs[i];
         len = ipmi_sdr_length(sdrh);
         if (i + len > sdrs_size) {
             error_report("Problem with recid 0x%4.4x", i);
-            return;
+            break;
         }
         sdr_add_entry(ibs, sdrh, len, NULL);
     }
+
+    if (sdrs != init_sdrs) {
+        g_free(sdrs);
+    }
 }
 
 static const VMStateDescription vmstate_ipmi_sim = {
@@ -1786,12 +1799,18 @@ static void ipmi_sim_realize(DeviceState
     vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
 }
 
+static Property ipmi_sim_properties[] = {
+    DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void ipmi_sim_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     IPMIBmcClass *bk = IPMI_BMC_CLASS(oc);
 
     dc->realize = ipmi_sim_realize;
+    dc->props = ipmi_sim_properties;
     bk->handle_command = ipmi_sim_handle_command;
 }
 
Index: qemu-powernv.git/qemu-options.hx
===================================================================
--- qemu-powernv.git.orig/qemu-options.hx
+++ qemu-powernv.git/qemu-options.hx
@@ -387,7 +387,7 @@ possible drivers and properties, use @co
 @code{-device @var{driver},help}.
 
 Some drivers are:
-@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}]
+@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}]
 
 Add an IPMI BMC.  This is a simulation of a hardware management
 interface processor that normally sits on a system.  It provides
@@ -399,6 +399,15 @@ This address is the BMC's address on the
 controllers.  If you don't know what this means, it is safe to ignore
 it.
 
+@table @option
+@item bmc=@var{id}
+The BMC to connect to, one of ipmi-bmc-sim or ipmi-bmc-extern above.
+@item slave_addr=@var{val}
+Define slave address to use for the BMC.  The default is 0x20.
+@item sdrfile=@var{file}
+file containing raw Sensor Data Records (SDR) data.  The default is none.
+@end table
+
 @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
 
 Add a connection to an external IPMI BMC simulator.  Instead of

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs
  2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
                   ` (8 preceding siblings ...)
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 09/10] ipmi: use a file to load SDRs Cédric Le Goater
@ 2016-03-10 14:04 ` Cédric Le Goater
  2016-03-15 14:30   ` Michael S. Tsirkin
  9 siblings, 1 reply; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-10 14:04 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Marcel Apfelbaum, Cédric Le Goater, qemu-devel, Michael S. Tsirkin

This patch provides a simple FRU support for the BMC simulator. FRUs
are loaded from a file which name is specified in the object
properties, each entry having a fixed size, also specified in the
properties. If the file is unknown or not accessible for some reason,
a unique entry of 1024 bytes is created as a default. Just enough to
start some simulation.

These commands complies with the IPMI spec : "34. FRU Inventory Device
Commands".

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
---

Changes since v2:

 - changed 'struct rsp_buffer' to 'RspBuffer'
 
Changes since v1:

 - change property name to 'fruareasize' and 'frudatafile'
 - add documentation of new properties
 
 hw/ipmi/ipmi_bmc_sim.c |  131 +++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx        |    8 ++
 2 files changed, 137 insertions(+), 2 deletions(-)

Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
===================================================================
--- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
+++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
@@ -80,6 +80,9 @@
 #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
 #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
 #define IPMI_CMD_RUN_INIT_AGENT           0x2C
+#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
+#define IPMI_CMD_READ_FRU_DATA            0x11
+#define IPMI_CMD_WRITE_FRU_DATA           0x12
 #define IPMI_CMD_GET_SEL_INFO             0x40
 #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
 #define IPMI_CMD_RESERVE_SEL              0x42
@@ -122,6 +125,13 @@ typedef struct IPMISdr {
     uint8_t overflow;
 } IPMISdr;
 
+typedef struct IPMIFru {
+    char *filename;
+    unsigned int nentries;
+    uint16_t areasize;
+    uint8_t *data;
+} IPMIFru;
+
 typedef struct IPMISensor {
     uint8_t status;
     uint8_t reading;
@@ -213,6 +223,7 @@ struct IPMIBmcSim {
 
     IPMISel sel;
     IPMISdr sdr;
+    IPMIFru fru;
     IPMISensor sensors[MAX_SENSORS];
     char *sdr_filename;
 
@@ -1322,6 +1333,94 @@ static void get_sel_info(IPMIBmcSim *ibs
     rsp_buffer_push(rsp, (ibs->sel.overflow << 7) | 0x02);
 }
 
+static void get_fru_area_info(IPMIBmcSim *ibs,
+                         uint8_t *cmd, unsigned int cmd_len,
+                         RspBuffer *rsp)
+{
+    uint8_t fruid;
+    uint16_t fru_entry_size;
+
+    fruid = cmd[2];
+
+    if (fruid >= ibs->fru.nentries) {
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }
+
+    fru_entry_size = ibs->fru.areasize;
+
+    rsp_buffer_push(rsp, fru_entry_size & 0xff);
+    rsp_buffer_push(rsp, fru_entry_size >> 8 & 0xff);
+    rsp_buffer_push(rsp, 0x0);
+}
+
+#define min(x, y) ((x) < (y) ? (x) : (y))
+#define max(x, y) ((x) > (y) ? (x) : (y))
+
+static void read_fru_data(IPMIBmcSim *ibs,
+                         uint8_t *cmd, unsigned int cmd_len,
+                         RspBuffer *rsp)
+{
+    uint8_t fruid;
+    uint16_t offset;
+    int i;
+    uint8_t *fru_entry;
+    unsigned int count;
+
+    fruid = cmd[2];
+    offset = (cmd[3] | cmd[4] << 8);
+
+    if (fruid >= ibs->fru.nentries) {
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }
+
+    if (offset >= ibs->fru.areasize - 1) {
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }
+
+    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
+
+    count = min(cmd[5], ibs->fru.areasize - offset);
+
+    rsp_buffer_push(rsp, count & 0xff);
+    for (i = 0; i < count; i++) {
+        rsp_buffer_push(rsp, fru_entry[offset + i]);
+    }
+}
+
+static void write_fru_data(IPMIBmcSim *ibs,
+                         uint8_t *cmd, unsigned int cmd_len,
+                         RspBuffer *rsp)
+{
+    uint8_t fruid;
+    uint16_t offset;
+    uint8_t *fru_entry;
+    unsigned int count;
+
+    fruid = cmd[2];
+    offset = (cmd[3] | cmd[4] << 8);
+
+    if (fruid >= ibs->fru.nentries) {
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }
+
+    if (offset >= ibs->fru.areasize - 1) {
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }
+
+    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
+
+    count = min(cmd_len - 5, ibs->fru.areasize - offset);
+
+    memcpy(fru_entry + offset, cmd + 5, count);
+
+    rsp_buffer_push(rsp, count & 0xff);
+}
+
 static void reserve_sel(IPMIBmcSim *ibs,
                         uint8_t *cmd, unsigned int cmd_len,
                         RspBuffer *rsp)
@@ -1658,6 +1757,9 @@ static const IPMINetfn app_netfn = {
 };
 
 static const IPMICmdHandler storage_cmds[] = {
+    [IPMI_CMD_GET_FRU_AREA_INFO] = { get_fru_area_info, 3 },
+    [IPMI_CMD_READ_FRU_DATA] = { read_fru_data, 5 },
+    [IPMI_CMD_WRITE_FRU_DATA] = { write_fru_data, 5 },
     [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
     [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
     [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
@@ -1760,6 +1862,31 @@ static const VMStateDescription vmstate_
     }
 };
 
+static void ipmi_fru_init(IPMIFru *fru)
+{
+    int fsize;
+    int size = 0;
+
+    fsize = get_image_size(fru->filename);
+    if (fsize > 0) {
+        size = QEMU_ALIGN_UP(fsize, fru->areasize);
+        fru->data = g_malloc0(size);
+        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
+            error_report("Could not load file '%s'", fru->filename);
+            g_free(fru->data);
+            fru->data = NULL;
+        }
+    }
+
+    if (!fru->data) {
+        /* give one default FRU */
+        size = fru->areasize;
+        fru->data = g_malloc0(size);
+    }
+
+    fru->nentries = size / fru->areasize;
+}
+
 static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 {
     IPMIBmc *b = IPMI_BMC(dev);
@@ -1782,6 +1909,8 @@ static void ipmi_sim_realize(DeviceState
 
     ipmi_sdr_init(ibs);
 
+    ipmi_fru_init(&ibs->fru);
+
     ibs->acpi_power_state[0] = 0;
     ibs->acpi_power_state[1] = 0;
 
@@ -1800,6 +1929,8 @@ static void ipmi_sim_realize(DeviceState
 }
 
 static Property ipmi_sim_properties[] = {
+    DEFINE_PROP_UINT16("fruareasize", IPMIBmcSim, fru.areasize, 1024),
+    DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, fru.filename),
     DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
     DEFINE_PROP_END_OF_LIST(),
 };
Index: qemu-powernv.git/qemu-options.hx
===================================================================
--- qemu-powernv.git.orig/qemu-options.hx
+++ qemu-powernv.git/qemu-options.hx
@@ -387,7 +387,7 @@ possible drivers and properties, use @co
 @code{-device @var{driver},help}.
 
 Some drivers are:
-@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}]
+@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
 
 Add an IPMI BMC.  This is a simulation of a hardware management
 interface processor that normally sits on a system.  It provides
@@ -405,7 +405,11 @@ The BMC to connect to, one of ipmi-bmc-s
 @item slave_addr=@var{val}
 Define slave address to use for the BMC.  The default is 0x20.
 @item sdrfile=@var{file}
-file containing raw Sensor Data Records (SDR) data.  The default is none.
+file containing raw Sensor Data Records (SDR) data. The default is none.
+@item fruareasize=@var{val}
+size of a Field Replaceable Unit (FRU) area.  The default is 1024.
+@item frudatafile=@var{file}
+file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
 @end table
 
 @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs Cédric Le Goater
@ 2016-03-15 14:30   ` Michael S. Tsirkin
  2016-03-15 14:36     ` Cédric Le Goater
  2016-06-08  7:25     ` Cédric Le Goater
  0 siblings, 2 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2016-03-15 14:30 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: Marcel Apfelbaum, Corey Minyard, qemu-devel

On Thu, Mar 10, 2016 at 03:04:03PM +0100, Cédric Le Goater wrote:
> This patch provides a simple FRU support for the BMC simulator. FRUs
> are loaded from a file which name is specified in the object
> properties, each entry having a fixed size, also specified in the
> properties. If the file is unknown or not accessible for some reason,
> a unique entry of 1024 bytes is created as a default. Just enough to
> start some simulation.
> 
> These commands complies with the IPMI spec : "34. FRU Inventory Device
> Commands".
> 
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> Acked-by: Corey Minyard <cminyard@mvista.com>

Since pull failed, I decided to hold this off until after release.
Loading new files needs some thought.

> ---
> 
> Changes since v2:
> 
>  - changed 'struct rsp_buffer' to 'RspBuffer'
>  
> Changes since v1:
> 
>  - change property name to 'fruareasize' and 'frudatafile'
>  - add documentation of new properties
>  
>  hw/ipmi/ipmi_bmc_sim.c |  131 +++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx        |    8 ++
>  2 files changed, 137 insertions(+), 2 deletions(-)
> 
> Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
> ===================================================================
> --- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
> +++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
> @@ -80,6 +80,9 @@
>  #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>  #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>  #define IPMI_CMD_RUN_INIT_AGENT           0x2C
> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
> +#define IPMI_CMD_READ_FRU_DATA            0x11
> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
>  #define IPMI_CMD_GET_SEL_INFO             0x40
>  #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>  #define IPMI_CMD_RESERVE_SEL              0x42
> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
>      uint8_t overflow;
>  } IPMISdr;
>  
> +typedef struct IPMIFru {
> +    char *filename;
> +    unsigned int nentries;
> +    uint16_t areasize;
> +    uint8_t *data;
> +} IPMIFru;
> +
>  typedef struct IPMISensor {
>      uint8_t status;
>      uint8_t reading;
> @@ -213,6 +223,7 @@ struct IPMIBmcSim {
>  
>      IPMISel sel;
>      IPMISdr sdr;
> +    IPMIFru fru;
>      IPMISensor sensors[MAX_SENSORS];
>      char *sdr_filename;
>  
> @@ -1322,6 +1333,94 @@ static void get_sel_info(IPMIBmcSim *ibs
>      rsp_buffer_push(rsp, (ibs->sel.overflow << 7) | 0x02);
>  }
>  
> +static void get_fru_area_info(IPMIBmcSim *ibs,
> +                         uint8_t *cmd, unsigned int cmd_len,
> +                         RspBuffer *rsp)
> +{
> +    uint8_t fruid;
> +    uint16_t fru_entry_size;
> +
> +    fruid = cmd[2];
> +
> +    if (fruid >= ibs->fru.nentries) {
> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> +        return;
> +    }
> +
> +    fru_entry_size = ibs->fru.areasize;
> +
> +    rsp_buffer_push(rsp, fru_entry_size & 0xff);
> +    rsp_buffer_push(rsp, fru_entry_size >> 8 & 0xff);
> +    rsp_buffer_push(rsp, 0x0);
> +}
> +
> +#define min(x, y) ((x) < (y) ? (x) : (y))
> +#define max(x, y) ((x) > (y) ? (x) : (y))
> +
> +static void read_fru_data(IPMIBmcSim *ibs,
> +                         uint8_t *cmd, unsigned int cmd_len,
> +                         RspBuffer *rsp)
> +{
> +    uint8_t fruid;
> +    uint16_t offset;
> +    int i;
> +    uint8_t *fru_entry;
> +    unsigned int count;
> +
> +    fruid = cmd[2];
> +    offset = (cmd[3] | cmd[4] << 8);
> +
> +    if (fruid >= ibs->fru.nentries) {
> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> +        return;
> +    }
> +
> +    if (offset >= ibs->fru.areasize - 1) {
> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> +        return;
> +    }
> +
> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
> +
> +    count = min(cmd[5], ibs->fru.areasize - offset);
> +
> +    rsp_buffer_push(rsp, count & 0xff);
> +    for (i = 0; i < count; i++) {
> +        rsp_buffer_push(rsp, fru_entry[offset + i]);
> +    }
> +}
> +
> +static void write_fru_data(IPMIBmcSim *ibs,
> +                         uint8_t *cmd, unsigned int cmd_len,
> +                         RspBuffer *rsp)
> +{
> +    uint8_t fruid;
> +    uint16_t offset;
> +    uint8_t *fru_entry;
> +    unsigned int count;
> +
> +    fruid = cmd[2];
> +    offset = (cmd[3] | cmd[4] << 8);
> +
> +    if (fruid >= ibs->fru.nentries) {
> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> +        return;
> +    }
> +
> +    if (offset >= ibs->fru.areasize - 1) {
> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> +        return;
> +    }
> +
> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
> +
> +    count = min(cmd_len - 5, ibs->fru.areasize - offset);
> +
> +    memcpy(fru_entry + offset, cmd + 5, count);
> +
> +    rsp_buffer_push(rsp, count & 0xff);
> +}
> +
>  static void reserve_sel(IPMIBmcSim *ibs,
>                          uint8_t *cmd, unsigned int cmd_len,
>                          RspBuffer *rsp)
> @@ -1658,6 +1757,9 @@ static const IPMINetfn app_netfn = {
>  };
>  
>  static const IPMICmdHandler storage_cmds[] = {
> +    [IPMI_CMD_GET_FRU_AREA_INFO] = { get_fru_area_info, 3 },
> +    [IPMI_CMD_READ_FRU_DATA] = { read_fru_data, 5 },
> +    [IPMI_CMD_WRITE_FRU_DATA] = { write_fru_data, 5 },
>      [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
>      [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
>      [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
> @@ -1760,6 +1862,31 @@ static const VMStateDescription vmstate_
>      }
>  };
>  
> +static void ipmi_fru_init(IPMIFru *fru)
> +{
> +    int fsize;
> +    int size = 0;
> +
> +    fsize = get_image_size(fru->filename);
> +    if (fsize > 0) {
> +        size = QEMU_ALIGN_UP(fsize, fru->areasize);
> +        fru->data = g_malloc0(size);
> +        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
> +            error_report("Could not load file '%s'", fru->filename);
> +            g_free(fru->data);
> +            fru->data = NULL;
> +        }
> +    }
> +
> +    if (!fru->data) {
> +        /* give one default FRU */
> +        size = fru->areasize;
> +        fru->data = g_malloc0(size);
> +    }
> +
> +    fru->nentries = size / fru->areasize;
> +}
> +
>  static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>  {
>      IPMIBmc *b = IPMI_BMC(dev);
> @@ -1782,6 +1909,8 @@ static void ipmi_sim_realize(DeviceState
>  
>      ipmi_sdr_init(ibs);
>  
> +    ipmi_fru_init(&ibs->fru);
> +
>      ibs->acpi_power_state[0] = 0;
>      ibs->acpi_power_state[1] = 0;
>  
> @@ -1800,6 +1929,8 @@ static void ipmi_sim_realize(DeviceState
>  }
>  
>  static Property ipmi_sim_properties[] = {
> +    DEFINE_PROP_UINT16("fruareasize", IPMIBmcSim, fru.areasize, 1024),
> +    DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, fru.filename),
>      DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> Index: qemu-powernv.git/qemu-options.hx
> ===================================================================
> --- qemu-powernv.git.orig/qemu-options.hx
> +++ qemu-powernv.git/qemu-options.hx
> @@ -387,7 +387,7 @@ possible drivers and properties, use @co
>  @code{-device @var{driver},help}.
>  
>  Some drivers are:
> -@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}]
> +@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
>  
>  Add an IPMI BMC.  This is a simulation of a hardware management
>  interface processor that normally sits on a system.  It provides
> @@ -405,7 +405,11 @@ The BMC to connect to, one of ipmi-bmc-s
>  @item slave_addr=@var{val}
>  Define slave address to use for the BMC.  The default is 0x20.
>  @item sdrfile=@var{file}
> -file containing raw Sensor Data Records (SDR) data.  The default is none.
> +file containing raw Sensor Data Records (SDR) data. The default is none.
> +@item fruareasize=@var{val}
> +size of a Field Replaceable Unit (FRU) area.  The default is 1024.
> +@item frudatafile=@var{file}
> +file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
>  @end table
>  
>  @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 09/10] ipmi: use a file to load SDRs
  2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 09/10] ipmi: use a file to load SDRs Cédric Le Goater
@ 2016-03-15 14:30   ` Michael S. Tsirkin
  0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2016-03-15 14:30 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: Marcel Apfelbaum, Corey Minyard, qemu-devel

On Thu, Mar 10, 2016 at 03:04:02PM +0100, Cédric Le Goater wrote:
> The IPMI BMC simulator populates the sdr/sensor tables with a minimal
> set of entries (Watchdog). But some qemu platforms might want to use
> extra entries for their custom needs.
> 
> This patch modifies slighty the initializing routine to take into
> account a larger set read from a file. The name of the file to use is
> defined through a new 'sdr' property of the simulator device.
> 
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Same thing - I prefer deferring this until 2.7
as loading files needs to be done carefully.

> ---
> 
> Changes since v1:
> 
>  - log an error if file does not exist.
>  - change property name to 'sdrfile'
>  - add documentation of new properties
>  
>  hw/ipmi/ipmi_bmc_sim.c |   23 +++++++++++++++++++++--
>  qemu-options.hx        |   11 ++++++++++-
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
> ===================================================================
> --- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
> +++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
> @@ -27,6 +27,7 @@
>  #include "qemu/timer.h"
>  #include "hw/ipmi/ipmi.h"
>  #include "qemu/error-report.h"
> +#include "hw/loader.h"
>  
>  #define IPMI_NETFN_CHASSIS            0x00
>  
> @@ -213,6 +214,7 @@ struct IPMIBmcSim {
>      IPMISel sel;
>      IPMISdr sdr;
>      IPMISensor sensors[MAX_SENSORS];
> +    char *sdr_filename;
>  
>      /* Odd netfns are for responses, so we only need the even ones. */
>      const IPMINetfn *netfns[MAX_NETFNS / 2];
> @@ -1701,22 +1703,33 @@ static void ipmi_sdr_init(IPMIBmcSim *ib
>  
>      sdrs_size = sizeof(init_sdrs);
>      sdrs = init_sdrs;
> +    if (ibs->sdr_filename &&
> +        !g_file_get_contents(ibs->sdr_filename, (gchar **) &sdrs, &sdrs_size,
> +                             NULL)) {
> +        error_report("failed to load sdr file '%s'", ibs->sdr_filename);
> +        sdrs_size = sizeof(init_sdrs);
> +        sdrs = init_sdrs;
> +    }
>  
>      for (i = 0; i < sdrs_size; i += len) {
>          struct ipmi_sdr_header *sdrh;
>  
>          if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
>              error_report("Problem with recid 0x%4.4x", i);
> -            return;
> +            break;
>          }
>          sdrh = (struct ipmi_sdr_header *) &sdrs[i];
>          len = ipmi_sdr_length(sdrh);
>          if (i + len > sdrs_size) {
>              error_report("Problem with recid 0x%4.4x", i);
> -            return;
> +            break;
>          }
>          sdr_add_entry(ibs, sdrh, len, NULL);
>      }
> +
> +    if (sdrs != init_sdrs) {
> +        g_free(sdrs);
> +    }
>  }
>  
>  static const VMStateDescription vmstate_ipmi_sim = {
> @@ -1786,12 +1799,18 @@ static void ipmi_sim_realize(DeviceState
>      vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
>  }
>  
> +static Property ipmi_sim_properties[] = {
> +    DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ipmi_sim_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      IPMIBmcClass *bk = IPMI_BMC_CLASS(oc);
>  
>      dc->realize = ipmi_sim_realize;
> +    dc->props = ipmi_sim_properties;
>      bk->handle_command = ipmi_sim_handle_command;
>  }
>  
> Index: qemu-powernv.git/qemu-options.hx
> ===================================================================
> --- qemu-powernv.git.orig/qemu-options.hx
> +++ qemu-powernv.git/qemu-options.hx
> @@ -387,7 +387,7 @@ possible drivers and properties, use @co
>  @code{-device @var{driver},help}.
>  
>  Some drivers are:
> -@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}]
> +@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}]
>  
>  Add an IPMI BMC.  This is a simulation of a hardware management
>  interface processor that normally sits on a system.  It provides
> @@ -399,6 +399,15 @@ This address is the BMC's address on the
>  controllers.  If you don't know what this means, it is safe to ignore
>  it.
>  
> +@table @option
> +@item bmc=@var{id}
> +The BMC to connect to, one of ipmi-bmc-sim or ipmi-bmc-extern above.
> +@item slave_addr=@var{val}
> +Define slave address to use for the BMC.  The default is 0x20.
> +@item sdrfile=@var{file}
> +file containing raw Sensor Data Records (SDR) data.  The default is none.
> +@end table
> +
>  @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
>  
>  Add a connection to an external IPMI BMC simulator.  Instead of

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs
  2016-03-15 14:30   ` Michael S. Tsirkin
@ 2016-03-15 14:36     ` Cédric Le Goater
  2016-03-15 14:42       ` Michael S. Tsirkin
  2016-06-08  7:25     ` Cédric Le Goater
  1 sibling, 1 reply; 16+ messages in thread
From: Cédric Le Goater @ 2016-03-15 14:36 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, Corey Minyard, qemu-devel

On 03/15/2016 03:30 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 10, 2016 at 03:04:03PM +0100, Cédric Le Goater wrote:
>> This patch provides a simple FRU support for the BMC simulator. FRUs
>> are loaded from a file which name is specified in the object
>> properties, each entry having a fixed size, also specified in the
>> properties. If the file is unknown or not accessible for some reason,
>> a unique entry of 1024 bytes is created as a default. Just enough to
>> start some simulation.
>>
>> These commands complies with the IPMI spec : "34. FRU Inventory Device
>> Commands".
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
> Since pull failed, I decided to hold this off until after release.
> Loading new files needs some thought.

OK. See below for the build break.

>> ---
>>
>> Changes since v2:
>>
>>  - changed 'struct rsp_buffer' to 'RspBuffer'
>>  
>> Changes since v1:
>>
>>  - change property name to 'fruareasize' and 'frudatafile'
>>  - add documentation of new properties
>>  
>>  hw/ipmi/ipmi_bmc_sim.c |  131 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-options.hx        |    8 ++
>>  2 files changed, 137 insertions(+), 2 deletions(-)
>>
>> Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
>> ===================================================================
>> --- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
>> +++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
>> @@ -80,6 +80,9 @@
>>  #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>>  #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>>  #define IPMI_CMD_RUN_INIT_AGENT           0x2C
>> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
>> +#define IPMI_CMD_READ_FRU_DATA            0x11
>> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
>>  #define IPMI_CMD_GET_SEL_INFO             0x40
>>  #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>>  #define IPMI_CMD_RESERVE_SEL              0x42
>> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
>>      uint8_t overflow;
>>  } IPMISdr;
>>  
>> +typedef struct IPMIFru {
>> +    char *filename;
>> +    unsigned int nentries;
>> +    uint16_t areasize;
>> +    uint8_t *data;
>> +} IPMIFru;
>> +
>>  typedef struct IPMISensor {
>>      uint8_t status;
>>      uint8_t reading;
>> @@ -213,6 +223,7 @@ struct IPMIBmcSim {
>>  
>>      IPMISel sel;
>>      IPMISdr sdr;
>> +    IPMIFru fru;
>>      IPMISensor sensors[MAX_SENSORS];
>>      char *sdr_filename;
>>  
>> @@ -1322,6 +1333,94 @@ static void get_sel_info(IPMIBmcSim *ibs
>>      rsp_buffer_push(rsp, (ibs->sel.overflow << 7) | 0x02);
>>  }
>>  
>> +static void get_fru_area_info(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         RspBuffer *rsp)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t fru_entry_size;
>> +
>> +    fruid = cmd[2];
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    fru_entry_size = ibs->fru.areasize;
>> +
>> +    rsp_buffer_push(rsp, fru_entry_size & 0xff);
>> +    rsp_buffer_push(rsp, fru_entry_size >> 8 & 0xff);
>> +    rsp_buffer_push(rsp, 0x0);
>> +}
>> +
>> +#define min(x, y) ((x) < (y) ? (x) : (y))
>> +#define max(x, y) ((x) > (y) ? (x) : (y))


Do you want a resend to fix these ^ and use the MIN and MAX macros
from osdep.h ?

C.

>> +static void read_fru_data(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         RspBuffer *rsp)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t offset;
>> +    int i;
>> +    uint8_t *fru_entry;
>> +    unsigned int count;
>> +
>> +    fruid = cmd[2];
>> +    offset = (cmd[3] | cmd[4] << 8);
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    if (offset >= ibs->fru.areasize - 1) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
>> +
>> +    count = min(cmd[5], ibs->fru.areasize - offset);
>> +
>> +    rsp_buffer_push(rsp, count & 0xff);
>> +    for (i = 0; i < count; i++) {
>> +        rsp_buffer_push(rsp, fru_entry[offset + i]);
>> +    }
>> +}
>> +
>> +static void write_fru_data(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         RspBuffer *rsp)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t offset;
>> +    uint8_t *fru_entry;
>> +    unsigned int count;
>> +
>> +    fruid = cmd[2];
>> +    offset = (cmd[3] | cmd[4] << 8);
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    if (offset >= ibs->fru.areasize - 1) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
>> +
>> +    count = min(cmd_len - 5, ibs->fru.areasize - offset);
>> +
>> +    memcpy(fru_entry + offset, cmd + 5, count);
>> +
>> +    rsp_buffer_push(rsp, count & 0xff);
>> +}
>> +
>>  static void reserve_sel(IPMIBmcSim *ibs,
>>                          uint8_t *cmd, unsigned int cmd_len,
>>                          RspBuffer *rsp)
>> @@ -1658,6 +1757,9 @@ static const IPMINetfn app_netfn = {
>>  };
>>  
>>  static const IPMICmdHandler storage_cmds[] = {
>> +    [IPMI_CMD_GET_FRU_AREA_INFO] = { get_fru_area_info, 3 },
>> +    [IPMI_CMD_READ_FRU_DATA] = { read_fru_data, 5 },
>> +    [IPMI_CMD_WRITE_FRU_DATA] = { write_fru_data, 5 },
>>      [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
>>      [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
>>      [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
>> @@ -1760,6 +1862,31 @@ static const VMStateDescription vmstate_
>>      }
>>  };
>>  
>> +static void ipmi_fru_init(IPMIFru *fru)
>> +{
>> +    int fsize;
>> +    int size = 0;
>> +
>> +    fsize = get_image_size(fru->filename);
>> +    if (fsize > 0) {
>> +        size = QEMU_ALIGN_UP(fsize, fru->areasize);
>> +        fru->data = g_malloc0(size);
>> +        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
>> +            error_report("Could not load file '%s'", fru->filename);
>> +            g_free(fru->data);
>> +            fru->data = NULL;
>> +        }
>> +    }
>> +
>> +    if (!fru->data) {
>> +        /* give one default FRU */
>> +        size = fru->areasize;
>> +        fru->data = g_malloc0(size);
>> +    }
>> +
>> +    fru->nentries = size / fru->areasize;
>> +}
>> +
>>  static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>  {
>>      IPMIBmc *b = IPMI_BMC(dev);
>> @@ -1782,6 +1909,8 @@ static void ipmi_sim_realize(DeviceState
>>  
>>      ipmi_sdr_init(ibs);
>>  
>> +    ipmi_fru_init(&ibs->fru);
>> +
>>      ibs->acpi_power_state[0] = 0;
>>      ibs->acpi_power_state[1] = 0;
>>  
>> @@ -1800,6 +1929,8 @@ static void ipmi_sim_realize(DeviceState
>>  }
>>  
>>  static Property ipmi_sim_properties[] = {
>> +    DEFINE_PROP_UINT16("fruareasize", IPMIBmcSim, fru.areasize, 1024),
>> +    DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, fru.filename),
>>      DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>> Index: qemu-powernv.git/qemu-options.hx
>> ===================================================================
>> --- qemu-powernv.git.orig/qemu-options.hx
>> +++ qemu-powernv.git/qemu-options.hx
>> @@ -387,7 +387,7 @@ possible drivers and properties, use @co
>>  @code{-device @var{driver},help}.
>>  
>>  Some drivers are:
>> -@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}]
>> +@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
>>  
>>  Add an IPMI BMC.  This is a simulation of a hardware management
>>  interface processor that normally sits on a system.  It provides
>> @@ -405,7 +405,11 @@ The BMC to connect to, one of ipmi-bmc-s
>>  @item slave_addr=@var{val}
>>  Define slave address to use for the BMC.  The default is 0x20.
>>  @item sdrfile=@var{file}
>> -file containing raw Sensor Data Records (SDR) data.  The default is none.
>> +file containing raw Sensor Data Records (SDR) data. The default is none.
>> +@item fruareasize=@var{val}
>> +size of a Field Replaceable Unit (FRU) area.  The default is 1024.
>> +@item frudatafile=@var{file}
>> +file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
>>  @end table
>>  
>>  @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs
  2016-03-15 14:36     ` Cédric Le Goater
@ 2016-03-15 14:42       ` Michael S. Tsirkin
  0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2016-03-15 14:42 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: Marcel Apfelbaum, Corey Minyard, qemu-devel

On Tue, Mar 15, 2016 at 03:36:01PM +0100, Cédric Le Goater wrote:
> On 03/15/2016 03:30 PM, Michael S. Tsirkin wrote:
> > On Thu, Mar 10, 2016 at 03:04:03PM +0100, Cédric Le Goater wrote:
> >> This patch provides a simple FRU support for the BMC simulator. FRUs
> >> are loaded from a file which name is specified in the object
> >> properties, each entry having a fixed size, also specified in the
> >> properties. If the file is unknown or not accessible for some reason,
> >> a unique entry of 1024 bytes is created as a default. Just enough to
> >> start some simulation.
> >>
> >> These commands complies with the IPMI spec : "34. FRU Inventory Device
> >> Commands".
> >>
> >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> >> Acked-by: Corey Minyard <cminyard@mvista.com>
> > 
> > Since pull failed, I decided to hold this off until after release.
> > Loading new files needs some thought.
> 
> OK. See below for the build break.
> 
> >> ---
> >>
> >> Changes since v2:
> >>
> >>  - changed 'struct rsp_buffer' to 'RspBuffer'
> >>  
> >> Changes since v1:
> >>
> >>  - change property name to 'fruareasize' and 'frudatafile'
> >>  - add documentation of new properties
> >>  
> >>  hw/ipmi/ipmi_bmc_sim.c |  131 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  qemu-options.hx        |    8 ++
> >>  2 files changed, 137 insertions(+), 2 deletions(-)
> >>
> >> Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
> >> ===================================================================
> >> --- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
> >> +++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
> >> @@ -80,6 +80,9 @@
> >>  #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
> >>  #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
> >>  #define IPMI_CMD_RUN_INIT_AGENT           0x2C
> >> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
> >> +#define IPMI_CMD_READ_FRU_DATA            0x11
> >> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
> >>  #define IPMI_CMD_GET_SEL_INFO             0x40
> >>  #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
> >>  #define IPMI_CMD_RESERVE_SEL              0x42
> >> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
> >>      uint8_t overflow;
> >>  } IPMISdr;
> >>  
> >> +typedef struct IPMIFru {
> >> +    char *filename;
> >> +    unsigned int nentries;
> >> +    uint16_t areasize;
> >> +    uint8_t *data;
> >> +} IPMIFru;
> >> +
> >>  typedef struct IPMISensor {
> >>      uint8_t status;
> >>      uint8_t reading;
> >> @@ -213,6 +223,7 @@ struct IPMIBmcSim {
> >>  
> >>      IPMISel sel;
> >>      IPMISdr sdr;
> >> +    IPMIFru fru;
> >>      IPMISensor sensors[MAX_SENSORS];
> >>      char *sdr_filename;
> >>  
> >> @@ -1322,6 +1333,94 @@ static void get_sel_info(IPMIBmcSim *ibs
> >>      rsp_buffer_push(rsp, (ibs->sel.overflow << 7) | 0x02);
> >>  }
> >>  
> >> +static void get_fru_area_info(IPMIBmcSim *ibs,
> >> +                         uint8_t *cmd, unsigned int cmd_len,
> >> +                         RspBuffer *rsp)
> >> +{
> >> +    uint8_t fruid;
> >> +    uint16_t fru_entry_size;
> >> +
> >> +    fruid = cmd[2];
> >> +
> >> +    if (fruid >= ibs->fru.nentries) {
> >> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> >> +        return;
> >> +    }
> >> +
> >> +    fru_entry_size = ibs->fru.areasize;
> >> +
> >> +    rsp_buffer_push(rsp, fru_entry_size & 0xff);
> >> +    rsp_buffer_push(rsp, fru_entry_size >> 8 & 0xff);
> >> +    rsp_buffer_push(rsp, 0x0);
> >> +}
> >> +
> >> +#define min(x, y) ((x) < (y) ? (x) : (y))
> >> +#define max(x, y) ((x) > (y) ? (x) : (y))
> 
> 
> Do you want a resend to fix these ^ and use the MIN and MAX macros
> from osdep.h ?
> 
> C.

After the release pls.

> >> +static void read_fru_data(IPMIBmcSim *ibs,
> >> +                         uint8_t *cmd, unsigned int cmd_len,
> >> +                         RspBuffer *rsp)
> >> +{
> >> +    uint8_t fruid;
> >> +    uint16_t offset;
> >> +    int i;
> >> +    uint8_t *fru_entry;
> >> +    unsigned int count;
> >> +
> >> +    fruid = cmd[2];
> >> +    offset = (cmd[3] | cmd[4] << 8);
> >> +
> >> +    if (fruid >= ibs->fru.nentries) {
> >> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> >> +        return;
> >> +    }
> >> +
> >> +    if (offset >= ibs->fru.areasize - 1) {
> >> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> >> +        return;
> >> +    }
> >> +
> >> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
> >> +
> >> +    count = min(cmd[5], ibs->fru.areasize - offset);
> >> +
> >> +    rsp_buffer_push(rsp, count & 0xff);
> >> +    for (i = 0; i < count; i++) {
> >> +        rsp_buffer_push(rsp, fru_entry[offset + i]);
> >> +    }
> >> +}
> >> +
> >> +static void write_fru_data(IPMIBmcSim *ibs,
> >> +                         uint8_t *cmd, unsigned int cmd_len,
> >> +                         RspBuffer *rsp)
> >> +{
> >> +    uint8_t fruid;
> >> +    uint16_t offset;
> >> +    uint8_t *fru_entry;
> >> +    unsigned int count;
> >> +
> >> +    fruid = cmd[2];
> >> +    offset = (cmd[3] | cmd[4] << 8);
> >> +
> >> +    if (fruid >= ibs->fru.nentries) {
> >> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> >> +        return;
> >> +    }
> >> +
> >> +    if (offset >= ibs->fru.areasize - 1) {
> >> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
> >> +        return;
> >> +    }
> >> +
> >> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
> >> +
> >> +    count = min(cmd_len - 5, ibs->fru.areasize - offset);
> >> +
> >> +    memcpy(fru_entry + offset, cmd + 5, count);
> >> +
> >> +    rsp_buffer_push(rsp, count & 0xff);
> >> +}
> >> +
> >>  static void reserve_sel(IPMIBmcSim *ibs,
> >>                          uint8_t *cmd, unsigned int cmd_len,
> >>                          RspBuffer *rsp)
> >> @@ -1658,6 +1757,9 @@ static const IPMINetfn app_netfn = {
> >>  };
> >>  
> >>  static const IPMICmdHandler storage_cmds[] = {
> >> +    [IPMI_CMD_GET_FRU_AREA_INFO] = { get_fru_area_info, 3 },
> >> +    [IPMI_CMD_READ_FRU_DATA] = { read_fru_data, 5 },
> >> +    [IPMI_CMD_WRITE_FRU_DATA] = { write_fru_data, 5 },
> >>      [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
> >>      [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
> >>      [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
> >> @@ -1760,6 +1862,31 @@ static const VMStateDescription vmstate_
> >>      }
> >>  };
> >>  
> >> +static void ipmi_fru_init(IPMIFru *fru)
> >> +{
> >> +    int fsize;
> >> +    int size = 0;
> >> +
> >> +    fsize = get_image_size(fru->filename);
> >> +    if (fsize > 0) {
> >> +        size = QEMU_ALIGN_UP(fsize, fru->areasize);
> >> +        fru->data = g_malloc0(size);
> >> +        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
> >> +            error_report("Could not load file '%s'", fru->filename);
> >> +            g_free(fru->data);
> >> +            fru->data = NULL;
> >> +        }
> >> +    }
> >> +
> >> +    if (!fru->data) {
> >> +        /* give one default FRU */
> >> +        size = fru->areasize;
> >> +        fru->data = g_malloc0(size);
> >> +    }
> >> +
> >> +    fru->nentries = size / fru->areasize;
> >> +}
> >> +
> >>  static void ipmi_sim_realize(DeviceState *dev, Error **errp)
> >>  {
> >>      IPMIBmc *b = IPMI_BMC(dev);
> >> @@ -1782,6 +1909,8 @@ static void ipmi_sim_realize(DeviceState
> >>  
> >>      ipmi_sdr_init(ibs);
> >>  
> >> +    ipmi_fru_init(&ibs->fru);
> >> +
> >>      ibs->acpi_power_state[0] = 0;
> >>      ibs->acpi_power_state[1] = 0;
> >>  
> >> @@ -1800,6 +1929,8 @@ static void ipmi_sim_realize(DeviceState
> >>  }
> >>  
> >>  static Property ipmi_sim_properties[] = {
> >> +    DEFINE_PROP_UINT16("fruareasize", IPMIBmcSim, fru.areasize, 1024),
> >> +    DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, fru.filename),
> >>      DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >> Index: qemu-powernv.git/qemu-options.hx
> >> ===================================================================
> >> --- qemu-powernv.git.orig/qemu-options.hx
> >> +++ qemu-powernv.git/qemu-options.hx
> >> @@ -387,7 +387,7 @@ possible drivers and properties, use @co
> >>  @code{-device @var{driver},help}.
> >>  
> >>  Some drivers are:
> >> -@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}]
> >> +@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
> >>  
> >>  Add an IPMI BMC.  This is a simulation of a hardware management
> >>  interface processor that normally sits on a system.  It provides
> >> @@ -405,7 +405,11 @@ The BMC to connect to, one of ipmi-bmc-s
> >>  @item slave_addr=@var{val}
> >>  Define slave address to use for the BMC.  The default is 0x20.
> >>  @item sdrfile=@var{file}
> >> -file containing raw Sensor Data Records (SDR) data.  The default is none.
> >> +file containing raw Sensor Data Records (SDR) data. The default is none.
> >> +@item fruareasize=@var{val}
> >> +size of a Field Replaceable Unit (FRU) area.  The default is 1024.
> >> +@item frudatafile=@var{file}
> >> +file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
> >>  @end table
> >>  
> >>  @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
> > 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs
  2016-03-15 14:30   ` Michael S. Tsirkin
  2016-03-15 14:36     ` Cédric Le Goater
@ 2016-06-08  7:25     ` Cédric Le Goater
  1 sibling, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-06-08  7:25 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum

On 03/15/2016 03:30 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 10, 2016 at 03:04:03PM +0100, Cédric Le Goater wrote:
>> This patch provides a simple FRU support for the BMC simulator. FRUs
>> are loaded from a file which name is specified in the object
>> properties, each entry having a fixed size, also specified in the
>> properties. If the file is unknown or not accessible for some reason,
>> a unique entry of 1024 bytes is created as a default. Just enough to
>> start some simulation.
>>
>> These commands complies with the IPMI spec : "34. FRU Inventory Device
>> Commands".
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
> Since pull failed, I decided to hold this off until after release.
> Loading new files needs some thought.

So, using a file made the code and the api much simpler but I agree 
one should find a way to provide them. 

You can simply steal the SDR from a real system with :

	 ipmitool sdr dump <file>

I didn't see a similar simple command line to dump for the FRU but 
raw commands should do.

We could also provide a simple tool to generate files providing a 
minimum, which is exactly what the static array in the ipmi-bmc-sim 
does.

Please advise. 

Thanks,

C. 

>> ---
>>
>> Changes since v2:
>>
>>  - changed 'struct rsp_buffer' to 'RspBuffer'
>>  
>> Changes since v1:
>>
>>  - change property name to 'fruareasize' and 'frudatafile'
>>  - add documentation of new properties
>>  
>>  hw/ipmi/ipmi_bmc_sim.c |  131 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-options.hx        |    8 ++
>>  2 files changed, 137 insertions(+), 2 deletions(-)
>>
>> Index: qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
>> ===================================================================
>> --- qemu-powernv.git.orig/hw/ipmi/ipmi_bmc_sim.c
>> +++ qemu-powernv.git/hw/ipmi/ipmi_bmc_sim.c
>> @@ -80,6 +80,9 @@
>>  #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>>  #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>>  #define IPMI_CMD_RUN_INIT_AGENT           0x2C
>> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
>> +#define IPMI_CMD_READ_FRU_DATA            0x11
>> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
>>  #define IPMI_CMD_GET_SEL_INFO             0x40
>>  #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>>  #define IPMI_CMD_RESERVE_SEL              0x42
>> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
>>      uint8_t overflow;
>>  } IPMISdr;
>>  
>> +typedef struct IPMIFru {
>> +    char *filename;
>> +    unsigned int nentries;
>> +    uint16_t areasize;
>> +    uint8_t *data;
>> +} IPMIFru;
>> +
>>  typedef struct IPMISensor {
>>      uint8_t status;
>>      uint8_t reading;
>> @@ -213,6 +223,7 @@ struct IPMIBmcSim {
>>  
>>      IPMISel sel;
>>      IPMISdr sdr;
>> +    IPMIFru fru;
>>      IPMISensor sensors[MAX_SENSORS];
>>      char *sdr_filename;
>>  
>> @@ -1322,6 +1333,94 @@ static void get_sel_info(IPMIBmcSim *ibs
>>      rsp_buffer_push(rsp, (ibs->sel.overflow << 7) | 0x02);
>>  }
>>  
>> +static void get_fru_area_info(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         RspBuffer *rsp)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t fru_entry_size;
>> +
>> +    fruid = cmd[2];
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    fru_entry_size = ibs->fru.areasize;
>> +
>> +    rsp_buffer_push(rsp, fru_entry_size & 0xff);
>> +    rsp_buffer_push(rsp, fru_entry_size >> 8 & 0xff);
>> +    rsp_buffer_push(rsp, 0x0);
>> +}
>> +
>> +#define min(x, y) ((x) < (y) ? (x) : (y))
>> +#define max(x, y) ((x) > (y) ? (x) : (y))
>> +
>> +static void read_fru_data(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         RspBuffer *rsp)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t offset;
>> +    int i;
>> +    uint8_t *fru_entry;
>> +    unsigned int count;
>> +
>> +    fruid = cmd[2];
>> +    offset = (cmd[3] | cmd[4] << 8);
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    if (offset >= ibs->fru.areasize - 1) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
>> +
>> +    count = min(cmd[5], ibs->fru.areasize - offset);
>> +
>> +    rsp_buffer_push(rsp, count & 0xff);
>> +    for (i = 0; i < count; i++) {
>> +        rsp_buffer_push(rsp, fru_entry[offset + i]);
>> +    }
>> +}
>> +
>> +static void write_fru_data(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         RspBuffer *rsp)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t offset;
>> +    uint8_t *fru_entry;
>> +    unsigned int count;
>> +
>> +    fruid = cmd[2];
>> +    offset = (cmd[3] | cmd[4] << 8);
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    if (offset >= ibs->fru.areasize - 1) {
>> +        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
>> +        return;
>> +    }
>> +
>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.areasize];
>> +
>> +    count = min(cmd_len - 5, ibs->fru.areasize - offset);
>> +
>> +    memcpy(fru_entry + offset, cmd + 5, count);
>> +
>> +    rsp_buffer_push(rsp, count & 0xff);
>> +}
>> +
>>  static void reserve_sel(IPMIBmcSim *ibs,
>>                          uint8_t *cmd, unsigned int cmd_len,
>>                          RspBuffer *rsp)
>> @@ -1658,6 +1757,9 @@ static const IPMINetfn app_netfn = {
>>  };
>>  
>>  static const IPMICmdHandler storage_cmds[] = {
>> +    [IPMI_CMD_GET_FRU_AREA_INFO] = { get_fru_area_info, 3 },
>> +    [IPMI_CMD_READ_FRU_DATA] = { read_fru_data, 5 },
>> +    [IPMI_CMD_WRITE_FRU_DATA] = { write_fru_data, 5 },
>>      [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
>>      [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
>>      [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
>> @@ -1760,6 +1862,31 @@ static const VMStateDescription vmstate_
>>      }
>>  };
>>  
>> +static void ipmi_fru_init(IPMIFru *fru)
>> +{
>> +    int fsize;
>> +    int size = 0;
>> +
>> +    fsize = get_image_size(fru->filename);
>> +    if (fsize > 0) {
>> +        size = QEMU_ALIGN_UP(fsize, fru->areasize);
>> +        fru->data = g_malloc0(size);
>> +        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
>> +            error_report("Could not load file '%s'", fru->filename);
>> +            g_free(fru->data);
>> +            fru->data = NULL;
>> +        }
>> +    }
>> +
>> +    if (!fru->data) {
>> +        /* give one default FRU */
>> +        size = fru->areasize;
>> +        fru->data = g_malloc0(size);
>> +    }
>> +
>> +    fru->nentries = size / fru->areasize;
>> +}
>> +
>>  static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>  {
>>      IPMIBmc *b = IPMI_BMC(dev);
>> @@ -1782,6 +1909,8 @@ static void ipmi_sim_realize(DeviceState
>>  
>>      ipmi_sdr_init(ibs);
>>  
>> +    ipmi_fru_init(&ibs->fru);
>> +
>>      ibs->acpi_power_state[0] = 0;
>>      ibs->acpi_power_state[1] = 0;
>>  
>> @@ -1800,6 +1929,8 @@ static void ipmi_sim_realize(DeviceState
>>  }
>>  
>>  static Property ipmi_sim_properties[] = {
>> +    DEFINE_PROP_UINT16("fruareasize", IPMIBmcSim, fru.areasize, 1024),
>> +    DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, fru.filename),
>>      DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>> Index: qemu-powernv.git/qemu-options.hx
>> ===================================================================
>> --- qemu-powernv.git.orig/qemu-options.hx
>> +++ qemu-powernv.git/qemu-options.hx
>> @@ -387,7 +387,7 @@ possible drivers and properties, use @co
>>  @code{-device @var{driver},help}.
>>  
>>  Some drivers are:
>> -@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}]
>> +@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
>>  
>>  Add an IPMI BMC.  This is a simulation of a hardware management
>>  interface processor that normally sits on a system.  It provides
>> @@ -405,7 +405,11 @@ The BMC to connect to, one of ipmi-bmc-s
>>  @item slave_addr=@var{val}
>>  Define slave address to use for the BMC.  The default is 0x20.
>>  @item sdrfile=@var{file}
>> -file containing raw Sensor Data Records (SDR) data.  The default is none.
>> +file containing raw Sensor Data Records (SDR) data. The default is none.
>> +@item fruareasize=@var{val}
>> +size of a Field Replaceable Unit (FRU) area.  The default is 1024.
>> +@item frudatafile=@var{file}
>> +file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
>>  @end table
>>  
>>  @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-06-08  7:25 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-10 14:03 [Qemu-devel] [PATCH v3 00/10] ipmi: a couple of enhancements to the BMC simulator (round 2) Cédric Le Goater
2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro Cédric Le Goater
2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 02/10] ipmi: replace IPMI_ADD_RSP_DATA() macro with inline helpers Cédric Le Goater
2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 03/10] ipmi: remove IPMI_CHECK_RESERVATION() macro Cédric Le Goater
2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 04/10] ipmi: add rsp_buffer_set_error() helper Cédric Le Goater
2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 05/10] ipmi: add a realize function to the device class Cédric Le Goater
2016-03-10 14:03 ` [Qemu-devel] [PATCH v3 06/10] ipmi: use a function to initialize the SDR table Cédric Le Goater
2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 07/10] ipmi: remove the need of an ending record in " Cédric Le Goater
2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 08/10] ipmi: add some local variables in ipmi_sdr_init Cédric Le Goater
2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 09/10] ipmi: use a file to load SDRs Cédric Le Goater
2016-03-15 14:30   ` Michael S. Tsirkin
2016-03-10 14:04 ` [Qemu-devel] [PATCH v3 10/10] ipmi: provide support for FRUs Cédric Le Goater
2016-03-15 14:30   ` Michael S. Tsirkin
2016-03-15 14:36     ` Cédric Le Goater
2016-03-15 14:42       ` Michael S. Tsirkin
2016-06-08  7:25     ` Cédric Le Goater

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.