* [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator
@ 2016-01-21 17:18 Cédric Le Goater
2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 1/9] ppc: add IPMI support Cédric Le Goater
` (8 more replies)
0 siblings, 9 replies; 25+ messages in thread
From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw)
To: Corey Minyard
Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum,
Michael S. Tsirkin
Hi,
Here are a few patches adding a couple of IPMI commands to the BMC
simulator.
Changes since v1:
- Added IPMI to ppc. We will need it for the future powernv platform.
- Added some initial cleanups
- Kept FRU, the API extensions to expose SDR and generate events
for later, may be as an extension of this patchset. (Work in progress)
Based on 3db34bf64ab4 and also available here :
https://github.com/legoater/qemu/commits/ipmi
Thanks,
Cédric Le Goater (9):
ppc: add IPMI support
ipmi: replace goto by a return statement
ipmi: replace *_MAXCMD defines
ipmi: introduce a struct ipmi_sdr_compact
ipmi: fix SDR length value
ipmi: add get and set SENSOR_TYPE commands
ipmi: add GET_SYS_RESTART_CAUSE chassis command
ipmi: add ACPI power and GUID commands
ipmi: add SET_SENSOR_READING command (tentative try)
default-configs/ppc64-softmmu.mak | 4 +
hw/ipmi/ipmi_bmc_sim.c | 479 ++++++++++++++++++++++++++------------
include/hw/ipmi/ipmi.h | 45 ++++
3 files changed, 385 insertions(+), 143 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 25+ messages in thread* [Qemu-devel] [PATCH v2 1/9] ppc: add IPMI support 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement Cédric Le Goater ` (7 subsequent siblings) 8 siblings, 0 replies; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin Open PowerNV systems use a BT device to communicate with the BMC. Provide support for it. Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- default-configs/ppc64-softmmu.mak | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak index bb71b23ee78a..d6871f43cbd5 100644 --- a/default-configs/ppc64-softmmu.mak +++ b/default-configs/ppc64-softmmu.mak @@ -7,6 +7,10 @@ CONFIG_VIRTIO_VGA=y CONFIG_ISA_MMIO=y CONFIG_ESCC=y CONFIG_M48T59=y +CONFIG_IPMI=y +CONFIG_IPMI_LOCAL=y +CONFIG_IPMI_EXTERN=y +CONFIG_ISA_IPMI_BT=y CONFIG_SERIAL=y CONFIG_PARALLEL=y CONFIG_I8254=y -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 1/9] ppc: add IPMI support Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-22 5:49 ` Marcel Apfelbaum ` (2 more replies) 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 3/9] ipmi: replace *_MAXCMD defines Cédric Le Goater ` (6 subsequent siblings) 8 siblings, 3 replies; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin Each routine using the IPMI_ADD_RSP_DATA, IPMI_CHECK_CMD_LEN or IPMI_CHECK_RESERVATION macros needs to define a goto label 'out' to handle hidden errors. Using directly a return statement as the same effect and it removes the fact that 'out' needs to be defined. The code exits in ipmi_sim_handle_command() are a little different from the rest and a "possible" error in the macro IPMI_ADD_RSP_DATA is handled before making use of it. This might be a bit excessive as a minimum response len is currently 300 bytes and the patch checks that at least 3 are available. Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++---------------------------------- 1 file changed, 41 insertions(+), 99 deletions(-) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index 0a59e539f549..e42c7e86c344 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -258,7 +258,7 @@ struct IPMIBmcSim { do { \ if (*rsp_len >= max_rsp_len) { \ rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED; \ - goto out; \ + return; \ } \ rsp[(*rsp_len)++] = (b); \ } while (0) @@ -267,7 +267,7 @@ struct IPMIBmcSim { #define IPMI_CHECK_CMD_LEN(l) \ if (cmd_len < l) { \ rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID; \ - goto out; \ + return; \ } /* Check that the reservation in the command is valid. */ @@ -275,7 +275,7 @@ struct IPMIBmcSim { do { \ if ((cmd[off] | (cmd[off + 1] << 8)) != r) { \ rsp[2] = IPMI_CC_INVALID_RESERVATION; \ - goto out; \ + return; \ } \ } while (0) @@ -453,14 +453,12 @@ static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert, } if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) { - goto out; + return; } memcpy(ibs->evtbuf, evt, 16); ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; k->set_atn(s, 1, attn_irq_enabled(ibs)); - out: - return; } static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor, @@ -581,6 +579,11 @@ static void ipmi_sim_handle_command(IPMIBmc *b, /* 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; + goto out; + } + IPMI_ADD_RSP_DATA(cmd[0] | 0x04); IPMI_ADD_RSP_DATA(cmd[1]); IPMI_ADD_RSP_DATA(0); /* Assume success */ @@ -698,8 +701,6 @@ static void chassis_capabilities(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); - out: - return; } static void chassis_status(IPMIBmcSim *ibs, @@ -711,8 +712,6 @@ static void chassis_status(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA(0); IPMI_ADD_RSP_DATA(0); IPMI_ADD_RSP_DATA(0); - out: - return; } static void chassis_control(IPMIBmcSim *ibs, @@ -746,10 +745,8 @@ static void chassis_control(IPMIBmcSim *ibs, break; default: rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } - out: - return; } static void get_device_id(IPMIBmcSim *ibs, @@ -768,8 +765,6 @@ static void get_device_id(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA(ibs->mfg_id[2]); IPMI_ADD_RSP_DATA(ibs->product_id[0]); IPMI_ADD_RSP_DATA(ibs->product_id[1]); - out: - return; } static void set_global_enables(IPMIBmcSim *ibs, uint8_t val) @@ -822,8 +817,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs, { IPMI_CHECK_CMD_LEN(3); set_global_enables(ibs, cmd[2]); - out: - return; } static void get_bmc_global_enables(IPMIBmcSim *ibs, @@ -832,8 +825,6 @@ static void get_bmc_global_enables(IPMIBmcSim *ibs, unsigned int max_rsp_len) { IPMI_ADD_RSP_DATA(ibs->bmc_global_enables); - out: - return; } static void clr_msg_flags(IPMIBmcSim *ibs, @@ -847,8 +838,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs, IPMI_CHECK_CMD_LEN(3); ibs->msg_flags &= ~cmd[2]; k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); - out: - return; } static void get_msg_flags(IPMIBmcSim *ibs, @@ -857,8 +846,6 @@ static void get_msg_flags(IPMIBmcSim *ibs, unsigned int max_rsp_len) { IPMI_ADD_RSP_DATA(ibs->msg_flags); - out: - return; } static void read_evt_msg_buf(IPMIBmcSim *ibs, @@ -872,15 +859,13 @@ static void read_evt_msg_buf(IPMIBmcSim *ibs, if (!(ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL)) { rsp[2] = 0x80; - goto out; + return; } for (i = 0; i < 16; i++) { IPMI_ADD_RSP_DATA(ibs->evtbuf[i]); } ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); - out: - return; } static void get_msg(IPMIBmcSim *ibs, @@ -911,7 +896,7 @@ static void get_msg(IPMIBmcSim *ibs, k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); } - out: +out: qemu_mutex_unlock(&ibs->lock); return; } @@ -942,14 +927,14 @@ 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; - goto out; + return; } IPMI_CHECK_CMD_LEN(10); if (cmd[3] != 0x40) { /* We only emulate a MC at address 0x40. */ rsp[2] = 0x83; /* NAK on write */ - goto out; + return; } cmd += 3; /* Skip the header. */ @@ -961,7 +946,7 @@ static void send_msg(IPMIBmcSim *ibs, */ if (ipmb_checksum(cmd, cmd_len, 0) != 0 || cmd[3] != 0x20) { /* Improper response address */ - goto out; /* No response */ + return; /* No response */ } netfn = cmd[1] >> 2; @@ -971,7 +956,7 @@ static void send_msg(IPMIBmcSim *ibs, if (rqLun != 2) { /* We only support LUN 2 coming back to us. */ - goto out; + return; } msg = g_malloc(sizeof(*msg)); @@ -1011,9 +996,6 @@ static void send_msg(IPMIBmcSim *ibs, ibs->msg_flags |= IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE; k->set_atn(s, 1, attn_irq_enabled(ibs)); qemu_mutex_unlock(&ibs->lock); - - out: - return; } static void do_watchdog_reset(IPMIBmcSim *ibs) @@ -1042,11 +1024,9 @@ static void reset_watchdog_timer(IPMIBmcSim *ibs, { if (!ibs->watchdog_initialized) { rsp[2] = 0x80; - goto out; + return; } do_watchdog_reset(ibs); - out: - return; } static void set_watchdog_timer(IPMIBmcSim *ibs, @@ -1062,7 +1042,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, val = cmd[2] & 0x7; /* Validate use */ if (val == 0 || val > 5) { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } val = cmd[3] & 0x7; /* Validate action */ switch (val) { @@ -1086,7 +1066,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, } if (rsp[2]) { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } val = (cmd[3] >> 4) & 0x7; /* Validate preaction */ @@ -1099,12 +1079,12 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, if (!k->do_hw_op(s, IPMI_SEND_NMI, 1)) { /* NMI not supported. */ rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } default: /* We don't support PRE_SMI */ rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } ibs->watchdog_initialized = 1; @@ -1118,8 +1098,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, } else { ibs->watchdog_running = 0; } - out: - return; } static void get_watchdog_timer(IPMIBmcSim *ibs, @@ -1141,8 +1119,6 @@ static void get_watchdog_timer(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA(0); IPMI_ADD_RSP_DATA(0); } - out: - return; } static void get_sdr_rep_info(IPMIBmcSim *ibs, @@ -1165,8 +1141,6 @@ static void get_sdr_rep_info(IPMIBmcSim *ibs, } /* Only modal support, reserve supported */ IPMI_ADD_RSP_DATA((ibs->sdr.overflow << 7) | 0x22); - out: - return; } static void reserve_sdr_rep(IPMIBmcSim *ibs, @@ -1176,8 +1150,6 @@ static void reserve_sdr_rep(IPMIBmcSim *ibs, { IPMI_ADD_RSP_DATA(ibs->sdr.reservation & 0xff); IPMI_ADD_RSP_DATA((ibs->sdr.reservation >> 8) & 0xff); - out: - return; } static void get_sdr(IPMIBmcSim *ibs, @@ -1196,11 +1168,11 @@ static void get_sdr(IPMIBmcSim *ibs, if (sdr_find_entry(&ibs->sdr, cmd[4] | (cmd[5] << 8), &pos, &nextrec)) { rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; - goto out; + return; } if (cmd[6] > (ibs->sdr.sdr[pos + 4])) { rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; - goto out; + return; } IPMI_ADD_RSP_DATA(nextrec & 0xff); @@ -1212,12 +1184,10 @@ static void get_sdr(IPMIBmcSim *ibs, if ((cmd[7] + *rsp_len) > max_rsp_len) { rsp[2] = IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES; - goto out; + return; } memcpy(rsp + *rsp_len, ibs->sdr.sdr + pos + cmd[6], cmd[7]); *rsp_len += cmd[7]; - out: - return; } static void add_sdr(IPMIBmcSim *ibs, @@ -1229,12 +1199,10 @@ static void add_sdr(IPMIBmcSim *ibs, if (sdr_add_entry(ibs, cmd + 2, cmd_len - 2, &recid)) { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } IPMI_ADD_RSP_DATA(recid & 0xff); IPMI_ADD_RSP_DATA((recid >> 8) & 0xff); - out: - return; } static void clear_sdr_rep(IPMIBmcSim *ibs, @@ -1246,7 +1214,7 @@ static void clear_sdr_rep(IPMIBmcSim *ibs, IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation); if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } if (cmd[7] == 0xaa) { ibs->sdr.next_free = 0; @@ -1258,10 +1226,8 @@ static void clear_sdr_rep(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA(1); /* Erasure complete */ } else { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } - out: - return; } static void get_sel_info(IPMIBmcSim *ibs, @@ -1285,8 +1251,6 @@ static void get_sel_info(IPMIBmcSim *ibs, } /* Only support Reserve SEL */ IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02); - out: - return; } static void reserve_sel(IPMIBmcSim *ibs, @@ -1296,8 +1260,6 @@ static void reserve_sel(IPMIBmcSim *ibs, { IPMI_ADD_RSP_DATA(ibs->sel.reservation & 0xff); IPMI_ADD_RSP_DATA((ibs->sel.reservation >> 8) & 0xff); - out: - return; } static void get_sel_entry(IPMIBmcSim *ibs, @@ -1313,17 +1275,17 @@ static void get_sel_entry(IPMIBmcSim *ibs, } if (ibs->sel.next_free == 0) { rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; - goto out; + return; } if (cmd[6] > 15) { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } if (cmd[7] == 0xff) { cmd[7] = 16; } else if ((cmd[7] + cmd[6]) > 16) { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } else { cmd[7] += cmd[6]; } @@ -1333,7 +1295,7 @@ static void get_sel_entry(IPMIBmcSim *ibs, val = ibs->sel.next_free - 1; } else if (val >= ibs->sel.next_free) { rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; - goto out; + return; } if ((val + 1) == ibs->sel.next_free) { IPMI_ADD_RSP_DATA(0xff); @@ -1345,8 +1307,6 @@ static void get_sel_entry(IPMIBmcSim *ibs, for (; cmd[6] < cmd[7]; cmd[6]++) { IPMI_ADD_RSP_DATA(ibs->sel.sel[val][cmd[6]]); } - out: - return; } static void add_sel_entry(IPMIBmcSim *ibs, @@ -1357,13 +1317,11 @@ static void add_sel_entry(IPMIBmcSim *ibs, IPMI_CHECK_CMD_LEN(18); if (sel_add_event(ibs, cmd + 2)) { rsp[2] = IPMI_CC_OUT_OF_SPACE; - goto out; + return; } /* sel_add_event fills in the record number. */ IPMI_ADD_RSP_DATA(cmd[2]); IPMI_ADD_RSP_DATA(cmd[3]); - out: - return; } static void clear_sel(IPMIBmcSim *ibs, @@ -1375,7 +1333,7 @@ static void clear_sel(IPMIBmcSim *ibs, IPMI_CHECK_RESERVATION(2, ibs->sel.reservation); if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } if (cmd[7] == 0xaa) { ibs->sel.next_free = 0; @@ -1387,10 +1345,8 @@ static void clear_sel(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA(1); /* Erasure complete */ } else { rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } - out: - return; } static void get_sel_time(IPMIBmcSim *ibs, @@ -1407,8 +1363,6 @@ static void get_sel_time(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA((val >> 8) & 0xff); IPMI_ADD_RSP_DATA((val >> 16) & 0xff); IPMI_ADD_RSP_DATA((val >> 24) & 0xff); - out: - return; } static void set_sel_time(IPMIBmcSim *ibs, @@ -1423,8 +1377,6 @@ static void set_sel_time(IPMIBmcSim *ibs, val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24); ipmi_gettime(&now); ibs->sel.time_offset = now.tv_sec - ((long) val); - out: - return; } static void set_sensor_evt_enable(IPMIBmcSim *ibs, @@ -1438,7 +1390,7 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs, if ((cmd[2] > MAX_SENSORS) || !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; - goto out; + return; } sens = ibs->sensors + cmd[2]; switch ((cmd[3] >> 4) & 0x3) { @@ -1474,11 +1426,9 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs, break; case 3: rsp[2] = IPMI_CC_INVALID_DATA_FIELD; - goto out; + return; } IPMI_SENSOR_SET_RET_STATUS(sens, cmd[3]); - out: - return; } static void get_sensor_evt_enable(IPMIBmcSim *ibs, @@ -1492,7 +1442,7 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs, if ((cmd[2] > MAX_SENSORS) || !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; - goto out; + return; } sens = ibs->sensors + cmd[2]; IPMI_ADD_RSP_DATA(IPMI_SENSOR_GET_RET_STATUS(sens)); @@ -1500,8 +1450,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs, 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); - out: - return; } static void rearm_sensor_evts(IPMIBmcSim *ibs, @@ -1515,17 +1463,15 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs, if ((cmd[2] > MAX_SENSORS) || !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; - goto out; + return; } sens = ibs->sensors + cmd[2]; if ((cmd[3] & 0x80) == 0) { /* Just clear everything */ sens->states = 0; - goto out; + return; } - out: - return; } static void get_sensor_evt_status(IPMIBmcSim *ibs, @@ -1539,7 +1485,7 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs, if ((cmd[2] > MAX_SENSORS) || !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; - goto out; + return; } sens = ibs->sensors + cmd[2]; IPMI_ADD_RSP_DATA(sens->reading); @@ -1548,8 +1494,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs, 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); - out: - return; } static void get_sensor_reading(IPMIBmcSim *ibs, @@ -1563,7 +1507,7 @@ static void get_sensor_reading(IPMIBmcSim *ibs, if ((cmd[2] > MAX_SENSORS) || !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; - goto out; + return; } sens = ibs->sensors + cmd[2]; IPMI_ADD_RSP_DATA(sens->reading); @@ -1572,8 +1516,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs, if (IPMI_SENSOR_IS_DISCRETE(sens)) { IPMI_ADD_RSP_DATA((sens->states >> 8) & 0xff); } - out: - return; } static const IPMICmdHandler chassis_cmds[IPMI_NETFN_CHASSIS_MAXCMD] = { -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement Cédric Le Goater @ 2016-01-22 5:49 ` Marcel Apfelbaum 2016-01-22 6:28 ` Greg Kurz 2016-01-22 12:56 ` Corey Minyard 2 siblings, 0 replies; 25+ messages in thread From: Marcel Apfelbaum @ 2016-01-22 5:49 UTC (permalink / raw) To: Cédric Le Goater, Corey Minyard Cc: Greg Kurz, qemu-devel, Michael S. Tsirkin On 01/21/2016 07:18 PM, Cédric Le Goater wrote: > Each routine using the IPMI_ADD_RSP_DATA, IPMI_CHECK_CMD_LEN or > IPMI_CHECK_RESERVATION macros needs to define a goto label 'out' to > handle hidden errors. Using directly a return statement as the same > effect and it removes the fact that 'out' needs to be defined. > > The code exits in ipmi_sim_handle_command() are a little different > from the rest and a "possible" error in the macro IPMI_ADD_RSP_DATA is > handled before making use of it. This might be a bit excessive as a > minimum response len is currently 300 bytes and the patch checks that > at least 3 are available. > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++---------------------------------- > 1 file changed, 41 insertions(+), 99 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 0a59e539f549..e42c7e86c344 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -258,7 +258,7 @@ struct IPMIBmcSim { > do { \ > if (*rsp_len >= max_rsp_len) { \ > rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED; \ > - goto out; \ > + return; \ > } \ > rsp[(*rsp_len)++] = (b); \ > } while (0) > @@ -267,7 +267,7 @@ struct IPMIBmcSim { > #define IPMI_CHECK_CMD_LEN(l) \ > if (cmd_len < l) { \ > rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID; \ > - goto out; \ > + return; \ > } > > /* Check that the reservation in the command is valid. */ > @@ -275,7 +275,7 @@ struct IPMIBmcSim { > do { \ > if ((cmd[off] | (cmd[off + 1] << 8)) != r) { \ > rsp[2] = IPMI_CC_INVALID_RESERVATION; \ > - goto out; \ > + return; \ > } \ > } while (0) > > @@ -453,14 +453,12 @@ static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert, > } > > if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) { > - goto out; > + return; > } > > memcpy(ibs->evtbuf, evt, 16); > ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; > k->set_atn(s, 1, attn_irq_enabled(ibs)); > - out: > - return; > } > > static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor, > @@ -581,6 +579,11 @@ static void ipmi_sim_handle_command(IPMIBmc *b, > > /* 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; > + goto out; > + } > + > IPMI_ADD_RSP_DATA(cmd[0] | 0x04); > IPMI_ADD_RSP_DATA(cmd[1]); > IPMI_ADD_RSP_DATA(0); /* Assume success */ > @@ -698,8 +701,6 @@ static void chassis_capabilities(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > - out: > - return; > } > > static void chassis_status(IPMIBmcSim *ibs, > @@ -711,8 +712,6 @@ static void chassis_status(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > - out: > - return; > } > > static void chassis_control(IPMIBmcSim *ibs, > @@ -746,10 +745,8 @@ static void chassis_control(IPMIBmcSim *ibs, > break; > default: > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_device_id(IPMIBmcSim *ibs, > @@ -768,8 +765,6 @@ static void get_device_id(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(ibs->mfg_id[2]); > IPMI_ADD_RSP_DATA(ibs->product_id[0]); > IPMI_ADD_RSP_DATA(ibs->product_id[1]); > - out: > - return; > } > > static void set_global_enables(IPMIBmcSim *ibs, uint8_t val) > @@ -822,8 +817,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs, > { > IPMI_CHECK_CMD_LEN(3); > set_global_enables(ibs, cmd[2]); > - out: > - return; > } > > static void get_bmc_global_enables(IPMIBmcSim *ibs, > @@ -832,8 +825,6 @@ static void get_bmc_global_enables(IPMIBmcSim *ibs, > unsigned int max_rsp_len) > { > IPMI_ADD_RSP_DATA(ibs->bmc_global_enables); > - out: > - return; > } > > static void clr_msg_flags(IPMIBmcSim *ibs, > @@ -847,8 +838,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs, > IPMI_CHECK_CMD_LEN(3); > ibs->msg_flags &= ~cmd[2]; > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > - out: > - return; > } > > static void get_msg_flags(IPMIBmcSim *ibs, > @@ -857,8 +846,6 @@ static void get_msg_flags(IPMIBmcSim *ibs, > unsigned int max_rsp_len) > { > IPMI_ADD_RSP_DATA(ibs->msg_flags); > - out: > - return; > } > > static void read_evt_msg_buf(IPMIBmcSim *ibs, > @@ -872,15 +859,13 @@ static void read_evt_msg_buf(IPMIBmcSim *ibs, > > if (!(ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL)) { > rsp[2] = 0x80; > - goto out; > + return; > } > for (i = 0; i < 16; i++) { > IPMI_ADD_RSP_DATA(ibs->evtbuf[i]); > } > ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > - out: > - return; > } > > static void get_msg(IPMIBmcSim *ibs, > @@ -911,7 +896,7 @@ static void get_msg(IPMIBmcSim *ibs, > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > } > > - out: > +out: > qemu_mutex_unlock(&ibs->lock); > return; > } > @@ -942,14 +927,14 @@ 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; > - goto out; > + return; > } > > IPMI_CHECK_CMD_LEN(10); > if (cmd[3] != 0x40) { > /* We only emulate a MC at address 0x40. */ > rsp[2] = 0x83; /* NAK on write */ > - goto out; > + return; > } > > cmd += 3; /* Skip the header. */ > @@ -961,7 +946,7 @@ static void send_msg(IPMIBmcSim *ibs, > */ > if (ipmb_checksum(cmd, cmd_len, 0) != 0 || > cmd[3] != 0x20) { /* Improper response address */ > - goto out; /* No response */ > + return; /* No response */ > } > > netfn = cmd[1] >> 2; > @@ -971,7 +956,7 @@ static void send_msg(IPMIBmcSim *ibs, > > if (rqLun != 2) { > /* We only support LUN 2 coming back to us. */ > - goto out; > + return; > } > > msg = g_malloc(sizeof(*msg)); > @@ -1011,9 +996,6 @@ static void send_msg(IPMIBmcSim *ibs, > ibs->msg_flags |= IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE; > k->set_atn(s, 1, attn_irq_enabled(ibs)); > qemu_mutex_unlock(&ibs->lock); > - > - out: > - return; > } > > static void do_watchdog_reset(IPMIBmcSim *ibs) > @@ -1042,11 +1024,9 @@ static void reset_watchdog_timer(IPMIBmcSim *ibs, > { > if (!ibs->watchdog_initialized) { > rsp[2] = 0x80; > - goto out; > + return; > } > do_watchdog_reset(ibs); > - out: > - return; > } > > static void set_watchdog_timer(IPMIBmcSim *ibs, > @@ -1062,7 +1042,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > val = cmd[2] & 0x7; /* Validate use */ > if (val == 0 || val > 5) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > val = cmd[3] & 0x7; /* Validate action */ > switch (val) { > @@ -1086,7 +1066,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > } > if (rsp[2]) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > > val = (cmd[3] >> 4) & 0x7; /* Validate preaction */ > @@ -1099,12 +1079,12 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > if (!k->do_hw_op(s, IPMI_SEND_NMI, 1)) { > /* NMI not supported. */ > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > default: > /* We don't support PRE_SMI */ > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > > ibs->watchdog_initialized = 1; > @@ -1118,8 +1098,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > } else { > ibs->watchdog_running = 0; > } > - out: > - return; > } > > static void get_watchdog_timer(IPMIBmcSim *ibs, > @@ -1141,8 +1119,6 @@ static void get_watchdog_timer(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > } > - out: > - return; > } > > static void get_sdr_rep_info(IPMIBmcSim *ibs, > @@ -1165,8 +1141,6 @@ static void get_sdr_rep_info(IPMIBmcSim *ibs, > } > /* Only modal support, reserve supported */ > IPMI_ADD_RSP_DATA((ibs->sdr.overflow << 7) | 0x22); > - out: > - return; > } > > static void reserve_sdr_rep(IPMIBmcSim *ibs, > @@ -1176,8 +1150,6 @@ static void reserve_sdr_rep(IPMIBmcSim *ibs, > { > IPMI_ADD_RSP_DATA(ibs->sdr.reservation & 0xff); > IPMI_ADD_RSP_DATA((ibs->sdr.reservation >> 8) & 0xff); > - out: > - return; > } > > static void get_sdr(IPMIBmcSim *ibs, > @@ -1196,11 +1168,11 @@ static void get_sdr(IPMIBmcSim *ibs, > if (sdr_find_entry(&ibs->sdr, cmd[4] | (cmd[5] << 8), > &pos, &nextrec)) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if (cmd[6] > (ibs->sdr.sdr[pos + 4])) { > rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; > - goto out; > + return; > } > > IPMI_ADD_RSP_DATA(nextrec & 0xff); > @@ -1212,12 +1184,10 @@ static void get_sdr(IPMIBmcSim *ibs, > > if ((cmd[7] + *rsp_len) > max_rsp_len) { > rsp[2] = IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES; > - goto out; > + return; > } > memcpy(rsp + *rsp_len, ibs->sdr.sdr + pos + cmd[6], cmd[7]); > *rsp_len += cmd[7]; > - out: > - return; > } > > static void add_sdr(IPMIBmcSim *ibs, > @@ -1229,12 +1199,10 @@ static void add_sdr(IPMIBmcSim *ibs, > > if (sdr_add_entry(ibs, cmd + 2, cmd_len - 2, &recid)) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > IPMI_ADD_RSP_DATA(recid & 0xff); > IPMI_ADD_RSP_DATA((recid >> 8) & 0xff); > - out: > - return; > } > > static void clear_sdr_rep(IPMIBmcSim *ibs, > @@ -1246,7 +1214,7 @@ static void clear_sdr_rep(IPMIBmcSim *ibs, > IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation); > if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xaa) { > ibs->sdr.next_free = 0; > @@ -1258,10 +1226,8 @@ static void clear_sdr_rep(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(1); /* Erasure complete */ > } else { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sel_info(IPMIBmcSim *ibs, > @@ -1285,8 +1251,6 @@ static void get_sel_info(IPMIBmcSim *ibs, > } > /* Only support Reserve SEL */ > IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02); > - out: > - return; > } > > static void reserve_sel(IPMIBmcSim *ibs, > @@ -1296,8 +1260,6 @@ static void reserve_sel(IPMIBmcSim *ibs, > { > IPMI_ADD_RSP_DATA(ibs->sel.reservation & 0xff); > IPMI_ADD_RSP_DATA((ibs->sel.reservation >> 8) & 0xff); > - out: > - return; > } > > static void get_sel_entry(IPMIBmcSim *ibs, > @@ -1313,17 +1275,17 @@ static void get_sel_entry(IPMIBmcSim *ibs, > } > if (ibs->sel.next_free == 0) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if (cmd[6] > 15) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xff) { > cmd[7] = 16; > } else if ((cmd[7] + cmd[6]) > 16) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } else { > cmd[7] += cmd[6]; > } > @@ -1333,7 +1295,7 @@ static void get_sel_entry(IPMIBmcSim *ibs, > val = ibs->sel.next_free - 1; > } else if (val >= ibs->sel.next_free) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if ((val + 1) == ibs->sel.next_free) { > IPMI_ADD_RSP_DATA(0xff); > @@ -1345,8 +1307,6 @@ static void get_sel_entry(IPMIBmcSim *ibs, > for (; cmd[6] < cmd[7]; cmd[6]++) { > IPMI_ADD_RSP_DATA(ibs->sel.sel[val][cmd[6]]); > } > - out: > - return; > } > > static void add_sel_entry(IPMIBmcSim *ibs, > @@ -1357,13 +1317,11 @@ static void add_sel_entry(IPMIBmcSim *ibs, > IPMI_CHECK_CMD_LEN(18); > if (sel_add_event(ibs, cmd + 2)) { > rsp[2] = IPMI_CC_OUT_OF_SPACE; > - goto out; > + return; > } > /* sel_add_event fills in the record number. */ > IPMI_ADD_RSP_DATA(cmd[2]); > IPMI_ADD_RSP_DATA(cmd[3]); > - out: > - return; > } > > static void clear_sel(IPMIBmcSim *ibs, > @@ -1375,7 +1333,7 @@ static void clear_sel(IPMIBmcSim *ibs, > IPMI_CHECK_RESERVATION(2, ibs->sel.reservation); > if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xaa) { > ibs->sel.next_free = 0; > @@ -1387,10 +1345,8 @@ static void clear_sel(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(1); /* Erasure complete */ > } else { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sel_time(IPMIBmcSim *ibs, > @@ -1407,8 +1363,6 @@ static void get_sel_time(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA((val >> 8) & 0xff); > IPMI_ADD_RSP_DATA((val >> 16) & 0xff); > IPMI_ADD_RSP_DATA((val >> 24) & 0xff); > - out: > - return; > } > > static void set_sel_time(IPMIBmcSim *ibs, > @@ -1423,8 +1377,6 @@ static void set_sel_time(IPMIBmcSim *ibs, > val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24); > ipmi_gettime(&now); > ibs->sel.time_offset = now.tv_sec - ((long) val); > - out: > - return; > } > > static void set_sensor_evt_enable(IPMIBmcSim *ibs, > @@ -1438,7 +1390,7 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > switch ((cmd[3] >> 4) & 0x3) { > @@ -1474,11 +1426,9 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs, > break; > case 3: > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > IPMI_SENSOR_SET_RET_STATUS(sens, cmd[3]); > - out: > - return; > } > > static void get_sensor_evt_enable(IPMIBmcSim *ibs, > @@ -1492,7 +1442,7 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(IPMI_SENSOR_GET_RET_STATUS(sens)); > @@ -1500,8 +1450,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs, > 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); > - out: > - return; > } > > static void rearm_sensor_evts(IPMIBmcSim *ibs, > @@ -1515,17 +1463,15 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > > if ((cmd[3] & 0x80) == 0) { > /* Just clear everything */ > sens->states = 0; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sensor_evt_status(IPMIBmcSim *ibs, > @@ -1539,7 +1485,7 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(sens->reading); > @@ -1548,8 +1494,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs, > 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); > - out: > - return; > } > > static void get_sensor_reading(IPMIBmcSim *ibs, > @@ -1563,7 +1507,7 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(sens->reading); > @@ -1572,8 +1516,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > if (IPMI_SENSOR_IS_DISCRETE(sens)) { > IPMI_ADD_RSP_DATA((sens->states >> 8) & 0xff); > } > - out: > - return; > } > > static const IPMICmdHandler chassis_cmds[IPMI_NETFN_CHASSIS_MAXCMD] = { > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks for taking care of this, Marcel ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement Cédric Le Goater 2016-01-22 5:49 ` Marcel Apfelbaum @ 2016-01-22 6:28 ` Greg Kurz 2016-01-22 12:56 ` Corey Minyard 2 siblings, 0 replies; 25+ messages in thread From: Greg Kurz @ 2016-01-22 6:28 UTC (permalink / raw) To: Cédric Le Goater Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On Thu, 21 Jan 2016 18:18:47 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > Each routine using the IPMI_ADD_RSP_DATA, IPMI_CHECK_CMD_LEN or > IPMI_CHECK_RESERVATION macros needs to define a goto label 'out' to > handle hidden errors. Using directly a return statement as the same s/as/has > effect and it removes the fact that 'out' needs to be defined. > > The code exits in ipmi_sim_handle_command() are a little different > from the rest and a "possible" error in the macro IPMI_ADD_RSP_DATA is > handled before making use of it. This might be a bit excessive as a > minimum response len is currently 300 bytes and the patch checks that > at least 3 are available. > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++---------------------------------- > 1 file changed, 41 insertions(+), 99 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 0a59e539f549..e42c7e86c344 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -258,7 +258,7 @@ struct IPMIBmcSim { > do { \ > if (*rsp_len >= max_rsp_len) { \ > rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED; \ > - goto out; \ > + return; \ > } \ > rsp[(*rsp_len)++] = (b); \ > } while (0) > @@ -267,7 +267,7 @@ struct IPMIBmcSim { > #define IPMI_CHECK_CMD_LEN(l) \ > if (cmd_len < l) { \ > rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID; \ > - goto out; \ > + return; \ > } > > /* Check that the reservation in the command is valid. */ > @@ -275,7 +275,7 @@ struct IPMIBmcSim { > do { \ > if ((cmd[off] | (cmd[off + 1] << 8)) != r) { \ > rsp[2] = IPMI_CC_INVALID_RESERVATION; \ > - goto out; \ > + return; \ > } \ > } while (0) > > @@ -453,14 +453,12 @@ static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert, > } > > if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) { > - goto out; > + return; > } > > memcpy(ibs->evtbuf, evt, 16); > ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; > k->set_atn(s, 1, attn_irq_enabled(ibs)); > - out: > - return; > } > > static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor, > @@ -581,6 +579,11 @@ static void ipmi_sim_handle_command(IPMIBmc *b, > > /* 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; > + goto out; > + } > + > IPMI_ADD_RSP_DATA(cmd[0] | 0x04); > IPMI_ADD_RSP_DATA(cmd[1]); > IPMI_ADD_RSP_DATA(0); /* Assume success */ > @@ -698,8 +701,6 @@ static void chassis_capabilities(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > - out: > - return; > } > > static void chassis_status(IPMIBmcSim *ibs, > @@ -711,8 +712,6 @@ static void chassis_status(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > - out: > - return; > } > > static void chassis_control(IPMIBmcSim *ibs, > @@ -746,10 +745,8 @@ static void chassis_control(IPMIBmcSim *ibs, > break; > default: > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_device_id(IPMIBmcSim *ibs, > @@ -768,8 +765,6 @@ static void get_device_id(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(ibs->mfg_id[2]); > IPMI_ADD_RSP_DATA(ibs->product_id[0]); > IPMI_ADD_RSP_DATA(ibs->product_id[1]); > - out: > - return; > } > > static void set_global_enables(IPMIBmcSim *ibs, uint8_t val) > @@ -822,8 +817,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs, > { > IPMI_CHECK_CMD_LEN(3); > set_global_enables(ibs, cmd[2]); > - out: > - return; > } > > static void get_bmc_global_enables(IPMIBmcSim *ibs, > @@ -832,8 +825,6 @@ static void get_bmc_global_enables(IPMIBmcSim *ibs, > unsigned int max_rsp_len) > { > IPMI_ADD_RSP_DATA(ibs->bmc_global_enables); > - out: > - return; > } > > static void clr_msg_flags(IPMIBmcSim *ibs, > @@ -847,8 +838,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs, > IPMI_CHECK_CMD_LEN(3); > ibs->msg_flags &= ~cmd[2]; > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > - out: > - return; > } > > static void get_msg_flags(IPMIBmcSim *ibs, > @@ -857,8 +846,6 @@ static void get_msg_flags(IPMIBmcSim *ibs, > unsigned int max_rsp_len) > { > IPMI_ADD_RSP_DATA(ibs->msg_flags); > - out: > - return; > } > > static void read_evt_msg_buf(IPMIBmcSim *ibs, > @@ -872,15 +859,13 @@ static void read_evt_msg_buf(IPMIBmcSim *ibs, > > if (!(ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL)) { > rsp[2] = 0x80; > - goto out; > + return; > } > for (i = 0; i < 16; i++) { > IPMI_ADD_RSP_DATA(ibs->evtbuf[i]); > } > ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > - out: > - return; > } > > static void get_msg(IPMIBmcSim *ibs, > @@ -911,7 +896,7 @@ static void get_msg(IPMIBmcSim *ibs, > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > } > > - out: > +out: > qemu_mutex_unlock(&ibs->lock); > return; > } > @@ -942,14 +927,14 @@ 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; > - goto out; > + return; > } > > IPMI_CHECK_CMD_LEN(10); > if (cmd[3] != 0x40) { > /* We only emulate a MC at address 0x40. */ > rsp[2] = 0x83; /* NAK on write */ > - goto out; > + return; > } > > cmd += 3; /* Skip the header. */ > @@ -961,7 +946,7 @@ static void send_msg(IPMIBmcSim *ibs, > */ > if (ipmb_checksum(cmd, cmd_len, 0) != 0 || > cmd[3] != 0x20) { /* Improper response address */ > - goto out; /* No response */ > + return; /* No response */ > } > > netfn = cmd[1] >> 2; > @@ -971,7 +956,7 @@ static void send_msg(IPMIBmcSim *ibs, > > if (rqLun != 2) { > /* We only support LUN 2 coming back to us. */ > - goto out; > + return; > } > > msg = g_malloc(sizeof(*msg)); > @@ -1011,9 +996,6 @@ static void send_msg(IPMIBmcSim *ibs, > ibs->msg_flags |= IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE; > k->set_atn(s, 1, attn_irq_enabled(ibs)); > qemu_mutex_unlock(&ibs->lock); > - > - out: > - return; > } > > static void do_watchdog_reset(IPMIBmcSim *ibs) > @@ -1042,11 +1024,9 @@ static void reset_watchdog_timer(IPMIBmcSim *ibs, > { > if (!ibs->watchdog_initialized) { > rsp[2] = 0x80; > - goto out; > + return; > } > do_watchdog_reset(ibs); > - out: > - return; > } > > static void set_watchdog_timer(IPMIBmcSim *ibs, > @@ -1062,7 +1042,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > val = cmd[2] & 0x7; /* Validate use */ > if (val == 0 || val > 5) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > val = cmd[3] & 0x7; /* Validate action */ > switch (val) { > @@ -1086,7 +1066,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > } > if (rsp[2]) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > > val = (cmd[3] >> 4) & 0x7; /* Validate preaction */ > @@ -1099,12 +1079,12 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > if (!k->do_hw_op(s, IPMI_SEND_NMI, 1)) { > /* NMI not supported. */ > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > default: > /* We don't support PRE_SMI */ > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > > ibs->watchdog_initialized = 1; > @@ -1118,8 +1098,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > } else { > ibs->watchdog_running = 0; > } > - out: > - return; > } > > static void get_watchdog_timer(IPMIBmcSim *ibs, > @@ -1141,8 +1119,6 @@ static void get_watchdog_timer(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > } > - out: > - return; > } > > static void get_sdr_rep_info(IPMIBmcSim *ibs, > @@ -1165,8 +1141,6 @@ static void get_sdr_rep_info(IPMIBmcSim *ibs, > } > /* Only modal support, reserve supported */ > IPMI_ADD_RSP_DATA((ibs->sdr.overflow << 7) | 0x22); > - out: > - return; > } > > static void reserve_sdr_rep(IPMIBmcSim *ibs, > @@ -1176,8 +1150,6 @@ static void reserve_sdr_rep(IPMIBmcSim *ibs, > { > IPMI_ADD_RSP_DATA(ibs->sdr.reservation & 0xff); > IPMI_ADD_RSP_DATA((ibs->sdr.reservation >> 8) & 0xff); > - out: > - return; > } > > static void get_sdr(IPMIBmcSim *ibs, > @@ -1196,11 +1168,11 @@ static void get_sdr(IPMIBmcSim *ibs, > if (sdr_find_entry(&ibs->sdr, cmd[4] | (cmd[5] << 8), > &pos, &nextrec)) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if (cmd[6] > (ibs->sdr.sdr[pos + 4])) { > rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; > - goto out; > + return; > } > > IPMI_ADD_RSP_DATA(nextrec & 0xff); > @@ -1212,12 +1184,10 @@ static void get_sdr(IPMIBmcSim *ibs, > > if ((cmd[7] + *rsp_len) > max_rsp_len) { > rsp[2] = IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES; > - goto out; > + return; > } > memcpy(rsp + *rsp_len, ibs->sdr.sdr + pos + cmd[6], cmd[7]); > *rsp_len += cmd[7]; > - out: > - return; > } > > static void add_sdr(IPMIBmcSim *ibs, > @@ -1229,12 +1199,10 @@ static void add_sdr(IPMIBmcSim *ibs, > > if (sdr_add_entry(ibs, cmd + 2, cmd_len - 2, &recid)) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > IPMI_ADD_RSP_DATA(recid & 0xff); > IPMI_ADD_RSP_DATA((recid >> 8) & 0xff); > - out: > - return; > } > > static void clear_sdr_rep(IPMIBmcSim *ibs, > @@ -1246,7 +1214,7 @@ static void clear_sdr_rep(IPMIBmcSim *ibs, > IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation); > if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xaa) { > ibs->sdr.next_free = 0; > @@ -1258,10 +1226,8 @@ static void clear_sdr_rep(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(1); /* Erasure complete */ > } else { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sel_info(IPMIBmcSim *ibs, > @@ -1285,8 +1251,6 @@ static void get_sel_info(IPMIBmcSim *ibs, > } > /* Only support Reserve SEL */ > IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02); > - out: > - return; > } > > static void reserve_sel(IPMIBmcSim *ibs, > @@ -1296,8 +1260,6 @@ static void reserve_sel(IPMIBmcSim *ibs, > { > IPMI_ADD_RSP_DATA(ibs->sel.reservation & 0xff); > IPMI_ADD_RSP_DATA((ibs->sel.reservation >> 8) & 0xff); > - out: > - return; > } > > static void get_sel_entry(IPMIBmcSim *ibs, > @@ -1313,17 +1275,17 @@ static void get_sel_entry(IPMIBmcSim *ibs, > } > if (ibs->sel.next_free == 0) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if (cmd[6] > 15) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xff) { > cmd[7] = 16; > } else if ((cmd[7] + cmd[6]) > 16) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } else { > cmd[7] += cmd[6]; > } > @@ -1333,7 +1295,7 @@ static void get_sel_entry(IPMIBmcSim *ibs, > val = ibs->sel.next_free - 1; > } else if (val >= ibs->sel.next_free) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if ((val + 1) == ibs->sel.next_free) { > IPMI_ADD_RSP_DATA(0xff); > @@ -1345,8 +1307,6 @@ static void get_sel_entry(IPMIBmcSim *ibs, > for (; cmd[6] < cmd[7]; cmd[6]++) { > IPMI_ADD_RSP_DATA(ibs->sel.sel[val][cmd[6]]); > } > - out: > - return; > } > > static void add_sel_entry(IPMIBmcSim *ibs, > @@ -1357,13 +1317,11 @@ static void add_sel_entry(IPMIBmcSim *ibs, > IPMI_CHECK_CMD_LEN(18); > if (sel_add_event(ibs, cmd + 2)) { > rsp[2] = IPMI_CC_OUT_OF_SPACE; > - goto out; > + return; > } > /* sel_add_event fills in the record number. */ > IPMI_ADD_RSP_DATA(cmd[2]); > IPMI_ADD_RSP_DATA(cmd[3]); > - out: > - return; > } > > static void clear_sel(IPMIBmcSim *ibs, > @@ -1375,7 +1333,7 @@ static void clear_sel(IPMIBmcSim *ibs, > IPMI_CHECK_RESERVATION(2, ibs->sel.reservation); > if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xaa) { > ibs->sel.next_free = 0; > @@ -1387,10 +1345,8 @@ static void clear_sel(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(1); /* Erasure complete */ > } else { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sel_time(IPMIBmcSim *ibs, > @@ -1407,8 +1363,6 @@ static void get_sel_time(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA((val >> 8) & 0xff); > IPMI_ADD_RSP_DATA((val >> 16) & 0xff); > IPMI_ADD_RSP_DATA((val >> 24) & 0xff); > - out: > - return; > } > > static void set_sel_time(IPMIBmcSim *ibs, > @@ -1423,8 +1377,6 @@ static void set_sel_time(IPMIBmcSim *ibs, > val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24); > ipmi_gettime(&now); > ibs->sel.time_offset = now.tv_sec - ((long) val); > - out: > - return; > } > > static void set_sensor_evt_enable(IPMIBmcSim *ibs, > @@ -1438,7 +1390,7 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > switch ((cmd[3] >> 4) & 0x3) { > @@ -1474,11 +1426,9 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs, > break; > case 3: > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > IPMI_SENSOR_SET_RET_STATUS(sens, cmd[3]); > - out: > - return; > } > > static void get_sensor_evt_enable(IPMIBmcSim *ibs, > @@ -1492,7 +1442,7 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(IPMI_SENSOR_GET_RET_STATUS(sens)); > @@ -1500,8 +1450,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs, > 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); > - out: > - return; > } > > static void rearm_sensor_evts(IPMIBmcSim *ibs, > @@ -1515,17 +1463,15 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > > if ((cmd[3] & 0x80) == 0) { > /* Just clear everything */ > sens->states = 0; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sensor_evt_status(IPMIBmcSim *ibs, > @@ -1539,7 +1485,7 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(sens->reading); > @@ -1548,8 +1494,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs, > 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); > - out: > - return; > } > > static void get_sensor_reading(IPMIBmcSim *ibs, > @@ -1563,7 +1507,7 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(sens->reading); > @@ -1572,8 +1516,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > if (IPMI_SENSOR_IS_DISCRETE(sens)) { > IPMI_ADD_RSP_DATA((sens->states >> 8) & 0xff); > } > - out: > - return; > } > > static const IPMICmdHandler chassis_cmds[IPMI_NETFN_CHASSIS_MAXCMD] = { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement Cédric Le Goater 2016-01-22 5:49 ` Marcel Apfelbaum 2016-01-22 6:28 ` Greg Kurz @ 2016-01-22 12:56 ` Corey Minyard 2 siblings, 0 replies; 25+ messages in thread From: Corey Minyard @ 2016-01-22 12:56 UTC (permalink / raw) To: Cédric Le Goater, Corey Minyard Cc: Michael S. Tsirkin, qemu-devel, Marcel Apfelbaum, Greg Kurz On 01/21/2016 11:18 AM, Cédric Le Goater wrote: > Each routine using the IPMI_ADD_RSP_DATA, IPMI_CHECK_CMD_LEN or > IPMI_CHECK_RESERVATION macros needs to define a goto label 'out' to > handle hidden errors. Using directly a return statement as the same Using a return statement directly has the same.... > effect and it removes the fact that 'out' needs to be defined. > > The code exits in ipmi_sim_handle_command() are a little different > from the rest and a "possible" error in the macro IPMI_ADD_RSP_DATA is > handled before making use of it. This might be a bit excessive as a > minimum response len is currently 300 bytes and the patch checks that > at least 3 are available. Yeah, it seems a little excessive. The compiler should figure out that the value is always false and remove the code. The return in the macro seems to obfuscate the return as much as the goto, but the code does look a lot neater this way. Reviewed-by: Corey Minyard <cminyard@mvista.com> > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++---------------------------------- > 1 file changed, 41 insertions(+), 99 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 0a59e539f549..e42c7e86c344 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -258,7 +258,7 @@ struct IPMIBmcSim { > do { \ > if (*rsp_len >= max_rsp_len) { \ > rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED; \ > - goto out; \ > + return; \ > } \ > rsp[(*rsp_len)++] = (b); \ > } while (0) > @@ -267,7 +267,7 @@ struct IPMIBmcSim { > #define IPMI_CHECK_CMD_LEN(l) \ > if (cmd_len < l) { \ > rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID; \ > - goto out; \ > + return; \ > } > > /* Check that the reservation in the command is valid. */ > @@ -275,7 +275,7 @@ struct IPMIBmcSim { > do { \ > if ((cmd[off] | (cmd[off + 1] << 8)) != r) { \ > rsp[2] = IPMI_CC_INVALID_RESERVATION; \ > - goto out; \ > + return; \ > } \ > } while (0) > > @@ -453,14 +453,12 @@ static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert, > } > > if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) { > - goto out; > + return; > } > > memcpy(ibs->evtbuf, evt, 16); > ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; > k->set_atn(s, 1, attn_irq_enabled(ibs)); > - out: > - return; > } > > static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor, > @@ -581,6 +579,11 @@ static void ipmi_sim_handle_command(IPMIBmc *b, > > /* 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; > + goto out; > + } > + > IPMI_ADD_RSP_DATA(cmd[0] | 0x04); > IPMI_ADD_RSP_DATA(cmd[1]); > IPMI_ADD_RSP_DATA(0); /* Assume success */ > @@ -698,8 +701,6 @@ static void chassis_capabilities(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > IPMI_ADD_RSP_DATA(ibs->parent.slave_addr); > - out: > - return; > } > > static void chassis_status(IPMIBmcSim *ibs, > @@ -711,8 +712,6 @@ static void chassis_status(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > - out: > - return; > } > > static void chassis_control(IPMIBmcSim *ibs, > @@ -746,10 +745,8 @@ static void chassis_control(IPMIBmcSim *ibs, > break; > default: > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_device_id(IPMIBmcSim *ibs, > @@ -768,8 +765,6 @@ static void get_device_id(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(ibs->mfg_id[2]); > IPMI_ADD_RSP_DATA(ibs->product_id[0]); > IPMI_ADD_RSP_DATA(ibs->product_id[1]); > - out: > - return; > } > > static void set_global_enables(IPMIBmcSim *ibs, uint8_t val) > @@ -822,8 +817,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs, > { > IPMI_CHECK_CMD_LEN(3); > set_global_enables(ibs, cmd[2]); > - out: > - return; > } > > static void get_bmc_global_enables(IPMIBmcSim *ibs, > @@ -832,8 +825,6 @@ static void get_bmc_global_enables(IPMIBmcSim *ibs, > unsigned int max_rsp_len) > { > IPMI_ADD_RSP_DATA(ibs->bmc_global_enables); > - out: > - return; > } > > static void clr_msg_flags(IPMIBmcSim *ibs, > @@ -847,8 +838,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs, > IPMI_CHECK_CMD_LEN(3); > ibs->msg_flags &= ~cmd[2]; > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > - out: > - return; > } > > static void get_msg_flags(IPMIBmcSim *ibs, > @@ -857,8 +846,6 @@ static void get_msg_flags(IPMIBmcSim *ibs, > unsigned int max_rsp_len) > { > IPMI_ADD_RSP_DATA(ibs->msg_flags); > - out: > - return; > } > > static void read_evt_msg_buf(IPMIBmcSim *ibs, > @@ -872,15 +859,13 @@ static void read_evt_msg_buf(IPMIBmcSim *ibs, > > if (!(ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL)) { > rsp[2] = 0x80; > - goto out; > + return; > } > for (i = 0; i < 16; i++) { > IPMI_ADD_RSP_DATA(ibs->evtbuf[i]); > } > ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > - out: > - return; > } > > static void get_msg(IPMIBmcSim *ibs, > @@ -911,7 +896,7 @@ static void get_msg(IPMIBmcSim *ibs, > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > } > > - out: > +out: > qemu_mutex_unlock(&ibs->lock); > return; > } > @@ -942,14 +927,14 @@ 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; > - goto out; > + return; > } > > IPMI_CHECK_CMD_LEN(10); > if (cmd[3] != 0x40) { > /* We only emulate a MC at address 0x40. */ > rsp[2] = 0x83; /* NAK on write */ > - goto out; > + return; > } > > cmd += 3; /* Skip the header. */ > @@ -961,7 +946,7 @@ static void send_msg(IPMIBmcSim *ibs, > */ > if (ipmb_checksum(cmd, cmd_len, 0) != 0 || > cmd[3] != 0x20) { /* Improper response address */ > - goto out; /* No response */ > + return; /* No response */ > } > > netfn = cmd[1] >> 2; > @@ -971,7 +956,7 @@ static void send_msg(IPMIBmcSim *ibs, > > if (rqLun != 2) { > /* We only support LUN 2 coming back to us. */ > - goto out; > + return; > } > > msg = g_malloc(sizeof(*msg)); > @@ -1011,9 +996,6 @@ static void send_msg(IPMIBmcSim *ibs, > ibs->msg_flags |= IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE; > k->set_atn(s, 1, attn_irq_enabled(ibs)); > qemu_mutex_unlock(&ibs->lock); > - > - out: > - return; > } > > static void do_watchdog_reset(IPMIBmcSim *ibs) > @@ -1042,11 +1024,9 @@ static void reset_watchdog_timer(IPMIBmcSim *ibs, > { > if (!ibs->watchdog_initialized) { > rsp[2] = 0x80; > - goto out; > + return; > } > do_watchdog_reset(ibs); > - out: > - return; > } > > static void set_watchdog_timer(IPMIBmcSim *ibs, > @@ -1062,7 +1042,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > val = cmd[2] & 0x7; /* Validate use */ > if (val == 0 || val > 5) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > val = cmd[3] & 0x7; /* Validate action */ > switch (val) { > @@ -1086,7 +1066,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > } > if (rsp[2]) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > > val = (cmd[3] >> 4) & 0x7; /* Validate preaction */ > @@ -1099,12 +1079,12 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > if (!k->do_hw_op(s, IPMI_SEND_NMI, 1)) { > /* NMI not supported. */ > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > default: > /* We don't support PRE_SMI */ > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > > ibs->watchdog_initialized = 1; > @@ -1118,8 +1098,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs, > } else { > ibs->watchdog_running = 0; > } > - out: > - return; > } > > static void get_watchdog_timer(IPMIBmcSim *ibs, > @@ -1141,8 +1119,6 @@ static void get_watchdog_timer(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(0); > IPMI_ADD_RSP_DATA(0); > } > - out: > - return; > } > > static void get_sdr_rep_info(IPMIBmcSim *ibs, > @@ -1165,8 +1141,6 @@ static void get_sdr_rep_info(IPMIBmcSim *ibs, > } > /* Only modal support, reserve supported */ > IPMI_ADD_RSP_DATA((ibs->sdr.overflow << 7) | 0x22); > - out: > - return; > } > > static void reserve_sdr_rep(IPMIBmcSim *ibs, > @@ -1176,8 +1150,6 @@ static void reserve_sdr_rep(IPMIBmcSim *ibs, > { > IPMI_ADD_RSP_DATA(ibs->sdr.reservation & 0xff); > IPMI_ADD_RSP_DATA((ibs->sdr.reservation >> 8) & 0xff); > - out: > - return; > } > > static void get_sdr(IPMIBmcSim *ibs, > @@ -1196,11 +1168,11 @@ static void get_sdr(IPMIBmcSim *ibs, > if (sdr_find_entry(&ibs->sdr, cmd[4] | (cmd[5] << 8), > &pos, &nextrec)) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if (cmd[6] > (ibs->sdr.sdr[pos + 4])) { > rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; > - goto out; > + return; > } > > IPMI_ADD_RSP_DATA(nextrec & 0xff); > @@ -1212,12 +1184,10 @@ static void get_sdr(IPMIBmcSim *ibs, > > if ((cmd[7] + *rsp_len) > max_rsp_len) { > rsp[2] = IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES; > - goto out; > + return; > } > memcpy(rsp + *rsp_len, ibs->sdr.sdr + pos + cmd[6], cmd[7]); > *rsp_len += cmd[7]; > - out: > - return; > } > > static void add_sdr(IPMIBmcSim *ibs, > @@ -1229,12 +1199,10 @@ static void add_sdr(IPMIBmcSim *ibs, > > if (sdr_add_entry(ibs, cmd + 2, cmd_len - 2, &recid)) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > IPMI_ADD_RSP_DATA(recid & 0xff); > IPMI_ADD_RSP_DATA((recid >> 8) & 0xff); > - out: > - return; > } > > static void clear_sdr_rep(IPMIBmcSim *ibs, > @@ -1246,7 +1214,7 @@ static void clear_sdr_rep(IPMIBmcSim *ibs, > IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation); > if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xaa) { > ibs->sdr.next_free = 0; > @@ -1258,10 +1226,8 @@ static void clear_sdr_rep(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(1); /* Erasure complete */ > } else { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sel_info(IPMIBmcSim *ibs, > @@ -1285,8 +1251,6 @@ static void get_sel_info(IPMIBmcSim *ibs, > } > /* Only support Reserve SEL */ > IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02); > - out: > - return; > } > > static void reserve_sel(IPMIBmcSim *ibs, > @@ -1296,8 +1260,6 @@ static void reserve_sel(IPMIBmcSim *ibs, > { > IPMI_ADD_RSP_DATA(ibs->sel.reservation & 0xff); > IPMI_ADD_RSP_DATA((ibs->sel.reservation >> 8) & 0xff); > - out: > - return; > } > > static void get_sel_entry(IPMIBmcSim *ibs, > @@ -1313,17 +1275,17 @@ static void get_sel_entry(IPMIBmcSim *ibs, > } > if (ibs->sel.next_free == 0) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if (cmd[6] > 15) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xff) { > cmd[7] = 16; > } else if ((cmd[7] + cmd[6]) > 16) { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } else { > cmd[7] += cmd[6]; > } > @@ -1333,7 +1295,7 @@ static void get_sel_entry(IPMIBmcSim *ibs, > val = ibs->sel.next_free - 1; > } else if (val >= ibs->sel.next_free) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > if ((val + 1) == ibs->sel.next_free) { > IPMI_ADD_RSP_DATA(0xff); > @@ -1345,8 +1307,6 @@ static void get_sel_entry(IPMIBmcSim *ibs, > for (; cmd[6] < cmd[7]; cmd[6]++) { > IPMI_ADD_RSP_DATA(ibs->sel.sel[val][cmd[6]]); > } > - out: > - return; > } > > static void add_sel_entry(IPMIBmcSim *ibs, > @@ -1357,13 +1317,11 @@ static void add_sel_entry(IPMIBmcSim *ibs, > IPMI_CHECK_CMD_LEN(18); > if (sel_add_event(ibs, cmd + 2)) { > rsp[2] = IPMI_CC_OUT_OF_SPACE; > - goto out; > + return; > } > /* sel_add_event fills in the record number. */ > IPMI_ADD_RSP_DATA(cmd[2]); > IPMI_ADD_RSP_DATA(cmd[3]); > - out: > - return; > } > > static void clear_sel(IPMIBmcSim *ibs, > @@ -1375,7 +1333,7 @@ static void clear_sel(IPMIBmcSim *ibs, > IPMI_CHECK_RESERVATION(2, ibs->sel.reservation); > if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > if (cmd[7] == 0xaa) { > ibs->sel.next_free = 0; > @@ -1387,10 +1345,8 @@ static void clear_sel(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(1); /* Erasure complete */ > } else { > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sel_time(IPMIBmcSim *ibs, > @@ -1407,8 +1363,6 @@ static void get_sel_time(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA((val >> 8) & 0xff); > IPMI_ADD_RSP_DATA((val >> 16) & 0xff); > IPMI_ADD_RSP_DATA((val >> 24) & 0xff); > - out: > - return; > } > > static void set_sel_time(IPMIBmcSim *ibs, > @@ -1423,8 +1377,6 @@ static void set_sel_time(IPMIBmcSim *ibs, > val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24); > ipmi_gettime(&now); > ibs->sel.time_offset = now.tv_sec - ((long) val); > - out: > - return; > } > > static void set_sensor_evt_enable(IPMIBmcSim *ibs, > @@ -1438,7 +1390,7 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > switch ((cmd[3] >> 4) & 0x3) { > @@ -1474,11 +1426,9 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs, > break; > case 3: > rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > - goto out; > + return; > } > IPMI_SENSOR_SET_RET_STATUS(sens, cmd[3]); > - out: > - return; > } > > static void get_sensor_evt_enable(IPMIBmcSim *ibs, > @@ -1492,7 +1442,7 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(IPMI_SENSOR_GET_RET_STATUS(sens)); > @@ -1500,8 +1450,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs, > 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); > - out: > - return; > } > > static void rearm_sensor_evts(IPMIBmcSim *ibs, > @@ -1515,17 +1463,15 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > > if ((cmd[3] & 0x80) == 0) { > /* Just clear everything */ > sens->states = 0; > - goto out; > + return; > } > - out: > - return; > } > > static void get_sensor_evt_status(IPMIBmcSim *ibs, > @@ -1539,7 +1485,7 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(sens->reading); > @@ -1548,8 +1494,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs, > 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); > - out: > - return; > } > > static void get_sensor_reading(IPMIBmcSim *ibs, > @@ -1563,7 +1507,7 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > if ((cmd[2] > MAX_SENSORS) || > !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > - goto out; > + return; > } > sens = ibs->sensors + cmd[2]; > IPMI_ADD_RSP_DATA(sens->reading); > @@ -1572,8 +1516,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > if (IPMI_SENSOR_IS_DISCRETE(sens)) { > IPMI_ADD_RSP_DATA((sens->states >> 8) & 0xff); > } > - out: > - return; > } > > static const IPMICmdHandler chassis_cmds[IPMI_NETFN_CHASSIS_MAXCMD] = { ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 3/9] ipmi: replace *_MAXCMD defines 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 1/9] ppc: add IPMI support Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-22 8:05 ` Greg Kurz 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 4/9] ipmi: introduce a struct ipmi_sdr_compact Cédric Le Goater ` (5 subsequent siblings) 8 siblings, 1 reply; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin ARRAY_SIZE() is simple to use and removes the need to pre-define the size of the command arrays. Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- hw/ipmi/ipmi_bmc_sim.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index e42c7e86c344..fc596a548df7 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -30,14 +30,12 @@ #include "qemu/error-report.h" #define IPMI_NETFN_CHASSIS 0x00 -#define IPMI_NETFN_CHASSIS_MAXCMD 0x03 #define IPMI_CMD_GET_CHASSIS_CAPABILITIES 0x00 #define IPMI_CMD_GET_CHASSIS_STATUS 0x01 #define IPMI_CMD_CHASSIS_CONTROL 0x02 #define IPMI_NETFN_SENSOR_EVENT 0x04 -#define IPMI_NETFN_SENSOR_EVENT_MAXCMD 0x2e #define IPMI_CMD_SET_SENSOR_EVT_ENABLE 0x28 #define IPMI_CMD_GET_SENSOR_EVT_ENABLE 0x29 @@ -46,7 +44,6 @@ #define IPMI_CMD_GET_SENSOR_READING 0x2d /* #define IPMI_NETFN_APP 0x06 In ipmi.h */ -#define IPMI_NETFN_APP_MAXCMD 0x36 #define IPMI_CMD_GET_DEVICE_ID 0x01 #define IPMI_CMD_COLD_RESET 0x02 @@ -63,7 +60,6 @@ #define IPMI_CMD_READ_EVT_MSG_BUF 0x35 #define IPMI_NETFN_STORAGE 0x0a -#define IPMI_NETFN_STORAGE_MAXCMD 0x4a #define IPMI_CMD_GET_SDR_REP_INFO 0x20 #define IPMI_CMD_GET_SDR_REP_ALLOC_INFO 0x21 @@ -1518,18 +1514,17 @@ static void get_sensor_reading(IPMIBmcSim *ibs, } } -static const IPMICmdHandler chassis_cmds[IPMI_NETFN_CHASSIS_MAXCMD] = { +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 }; static const IPMINetfn chassis_netfn = { - .cmd_nums = IPMI_NETFN_CHASSIS_MAXCMD, + .cmd_nums = ARRAY_SIZE(chassis_cmds), .cmd_handlers = chassis_cmds }; -static const IPMICmdHandler -sensor_event_cmds[IPMI_NETFN_SENSOR_EVENT_MAXCMD] = { +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, @@ -1537,11 +1532,11 @@ sensor_event_cmds[IPMI_NETFN_SENSOR_EVENT_MAXCMD] = { [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading }; static const IPMINetfn sensor_event_netfn = { - .cmd_nums = IPMI_NETFN_SENSOR_EVENT_MAXCMD, + .cmd_nums = ARRAY_SIZE(sensor_event_cmds), .cmd_handlers = sensor_event_cmds }; -static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { +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, @@ -1557,11 +1552,11 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { [IPMI_CMD_GET_WATCHDOG_TIMER] = get_watchdog_timer, }; static const IPMINetfn app_netfn = { - .cmd_nums = IPMI_NETFN_APP_MAXCMD, + .cmd_nums = ARRAY_SIZE(app_cmds), .cmd_handlers = app_cmds }; -static const IPMICmdHandler storage_cmds[IPMI_NETFN_STORAGE_MAXCMD] = { +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, @@ -1577,7 +1572,7 @@ static const IPMICmdHandler storage_cmds[IPMI_NETFN_STORAGE_MAXCMD] = { }; static const IPMINetfn storage_netfn = { - .cmd_nums = IPMI_NETFN_STORAGE_MAXCMD, + .cmd_nums = ARRAY_SIZE(storage_cmds), .cmd_handlers = storage_cmds }; -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/9] ipmi: replace *_MAXCMD defines 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 3/9] ipmi: replace *_MAXCMD defines Cédric Le Goater @ 2016-01-22 8:05 ` Greg Kurz 0 siblings, 0 replies; 25+ messages in thread From: Greg Kurz @ 2016-01-22 8:05 UTC (permalink / raw) To: Cédric Le Goater Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On Thu, 21 Jan 2016 18:18:48 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > ARRAY_SIZE() is simple to use and removes the need to pre-define > the size of the command arrays. > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- Much nicer ! Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > hw/ipmi/ipmi_bmc_sim.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index e42c7e86c344..fc596a548df7 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -30,14 +30,12 @@ > #include "qemu/error-report.h" > > #define IPMI_NETFN_CHASSIS 0x00 > -#define IPMI_NETFN_CHASSIS_MAXCMD 0x03 > > #define IPMI_CMD_GET_CHASSIS_CAPABILITIES 0x00 > #define IPMI_CMD_GET_CHASSIS_STATUS 0x01 > #define IPMI_CMD_CHASSIS_CONTROL 0x02 > > #define IPMI_NETFN_SENSOR_EVENT 0x04 > -#define IPMI_NETFN_SENSOR_EVENT_MAXCMD 0x2e > > #define IPMI_CMD_SET_SENSOR_EVT_ENABLE 0x28 > #define IPMI_CMD_GET_SENSOR_EVT_ENABLE 0x29 > @@ -46,7 +44,6 @@ > #define IPMI_CMD_GET_SENSOR_READING 0x2d > > /* #define IPMI_NETFN_APP 0x06 In ipmi.h */ > -#define IPMI_NETFN_APP_MAXCMD 0x36 > > #define IPMI_CMD_GET_DEVICE_ID 0x01 > #define IPMI_CMD_COLD_RESET 0x02 > @@ -63,7 +60,6 @@ > #define IPMI_CMD_READ_EVT_MSG_BUF 0x35 > > #define IPMI_NETFN_STORAGE 0x0a > -#define IPMI_NETFN_STORAGE_MAXCMD 0x4a > > #define IPMI_CMD_GET_SDR_REP_INFO 0x20 > #define IPMI_CMD_GET_SDR_REP_ALLOC_INFO 0x21 > @@ -1518,18 +1514,17 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > } > } > > -static const IPMICmdHandler chassis_cmds[IPMI_NETFN_CHASSIS_MAXCMD] = { > +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 > }; > static const IPMINetfn chassis_netfn = { > - .cmd_nums = IPMI_NETFN_CHASSIS_MAXCMD, > + .cmd_nums = ARRAY_SIZE(chassis_cmds), > .cmd_handlers = chassis_cmds > }; > > -static const IPMICmdHandler > -sensor_event_cmds[IPMI_NETFN_SENSOR_EVENT_MAXCMD] = { > +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, > @@ -1537,11 +1532,11 @@ sensor_event_cmds[IPMI_NETFN_SENSOR_EVENT_MAXCMD] = { > [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading > }; > static const IPMINetfn sensor_event_netfn = { > - .cmd_nums = IPMI_NETFN_SENSOR_EVENT_MAXCMD, > + .cmd_nums = ARRAY_SIZE(sensor_event_cmds), > .cmd_handlers = sensor_event_cmds > }; > > -static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { > +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, > @@ -1557,11 +1552,11 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { > [IPMI_CMD_GET_WATCHDOG_TIMER] = get_watchdog_timer, > }; > static const IPMINetfn app_netfn = { > - .cmd_nums = IPMI_NETFN_APP_MAXCMD, > + .cmd_nums = ARRAY_SIZE(app_cmds), > .cmd_handlers = app_cmds > }; > > -static const IPMICmdHandler storage_cmds[IPMI_NETFN_STORAGE_MAXCMD] = { > +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, > @@ -1577,7 +1572,7 @@ static const IPMICmdHandler storage_cmds[IPMI_NETFN_STORAGE_MAXCMD] = { > }; > > static const IPMINetfn storage_netfn = { > - .cmd_nums = IPMI_NETFN_STORAGE_MAXCMD, > + .cmd_nums = ARRAY_SIZE(storage_cmds), > .cmd_handlers = storage_cmds > }; > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 4/9] ipmi: introduce a struct ipmi_sdr_compact 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater ` (2 preceding siblings ...) 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 3/9] ipmi: replace *_MAXCMD defines Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-22 10:49 ` Greg Kurz 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 5/9] ipmi: fix SDR length value Cédric Le Goater ` (4 subsequent siblings) 8 siblings, 1 reply; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin Currently, sdr attributes are identified using byte offsets and this can be a bit confusing. This patch adds a struct ipmi_sdr_compact conforming to the IPMI specs and replaces byte offsets with names. It also introduces and uses a struct ipmi_sdr_header in sections of the code where no assumption is made on the type of SDR. This leave rooms to potential usage of other types in the future. Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- hw/ipmi/ipmi_bmc_sim.c | 65 +++++++++++++++++++++++++++++++------------------- include/hw/ipmi/ipmi.h | 44 ++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 25 deletions(-) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index fc596a548df7..31f990199154 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -323,11 +323,15 @@ static void sdr_inc_reservation(IPMISdr *sdr) static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, unsigned int len, uint16_t *recid) { + struct ipmi_sdr_header *sdrh_entry = (struct ipmi_sdr_header *) entry; + struct ipmi_sdr_header *sdrh = + (struct ipmi_sdr_header *) &ibs->sdr.sdr[ibs->sdr.next_free]; + if ((len < 5) || (len > 255)) { return 1; } - if (entry[4] != len - 5) { + if (sdrh_entry->rec_length != len - 5) { return 1; } @@ -336,10 +340,10 @@ static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, return 1; } - memcpy(ibs->sdr.sdr + ibs->sdr.next_free, entry, len); - ibs->sdr.sdr[ibs->sdr.next_free] = ibs->sdr.next_rec_id & 0xff; - ibs->sdr.sdr[ibs->sdr.next_free+1] = (ibs->sdr.next_rec_id >> 8) & 0xff; - ibs->sdr.sdr[ibs->sdr.next_free+2] = 0x51; /* Conform to IPMI 1.5 spec */ + memcpy(sdrh, entry, len); + sdrh->rec_id[0] = ibs->sdr.next_rec_id & 0xff; + sdrh->rec_id[1] = (ibs->sdr.next_rec_id >> 8) & 0xff; + sdrh->sdr_version = 0x51; /* Conform to IPMI 1.5 spec */ if (recid) { *recid = ibs->sdr.next_rec_id; @@ -357,8 +361,10 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid, unsigned int pos = *retpos; while (pos < sdr->next_free) { - uint16_t trec = sdr->sdr[pos] | (sdr->sdr[pos + 1] << 8); - unsigned int nextpos = pos + sdr->sdr[pos + 4]; + struct ipmi_sdr_header *sdrh = + (struct ipmi_sdr_header *) &sdr->sdr[pos]; + uint16_t trec = ipmi_sdr_recid(sdrh); + unsigned int nextpos = pos + sdrh->rec_length; if (trec == recid) { if (nextrec) { @@ -507,29 +513,32 @@ static void ipmi_init_sensors_from_sdrs(IPMIBmcSim *s) pos = 0; for (i = 0; !sdr_find_entry(&s->sdr, i, &pos, NULL); i++) { - uint8_t *sdr = s->sdr.sdr + pos; - unsigned int len = sdr[4]; + struct ipmi_sdr_compact *sdr = + (struct ipmi_sdr_compact *) &s->sdr.sdr[pos]; + unsigned int len = sdr->header.rec_length; if (len < 20) { continue; } - if ((sdr[3] < 1) || (sdr[3] > 2)) { + if (sdr->header.rec_type != IPMI_SDR_COMPACT_TYPE) { continue; /* Not a sensor SDR we set from */ } - if (sdr[7] > MAX_SENSORS) { + if (sdr->sensor_owner_number > MAX_SENSORS) { continue; } - sens = s->sensors + sdr[7]; + sens = s->sensors + sdr->sensor_owner_number; IPMI_SENSOR_SET_PRESENT(sens, 1); - IPMI_SENSOR_SET_SCAN_ON(sens, (sdr[10] >> 6) & 1); - IPMI_SENSOR_SET_EVENTS_ON(sens, (sdr[10] >> 5) & 1); - sens->assert_suppt = sdr[14] | (sdr[15] << 8); - sens->deassert_suppt = sdr[16] | (sdr[17] << 8); - sens->states_suppt = sdr[18] | (sdr[19] << 8); - sens->sensor_type = sdr[12]; - sens->evt_reading_type_code = sdr[13] & 0x7f; + IPMI_SENSOR_SET_SCAN_ON(sens, (sdr->sensor_init >> 6) & 1); + IPMI_SENSOR_SET_EVENTS_ON(sens, (sdr->sensor_init >> 5) & 1); + sens->assert_suppt = sdr->assert_mask[0] | (sdr->assert_mask[1] << 8); + sens->deassert_suppt = + sdr->deassert_mask[0] | (sdr->deassert_mask[1] << 8); + sens->states_suppt = + sdr->discrete_mask[0] | (sdr->discrete_mask[1] << 8); + sens->sensor_type = sdr->sensor_type; + sens->evt_reading_type_code = sdr->reading_type & 0x7f; /* Enable all the events that are supported. */ sens->assert_enable = sens->assert_suppt; @@ -1155,6 +1164,7 @@ static void get_sdr(IPMIBmcSim *ibs, { unsigned int pos; uint16_t nextrec; + struct ipmi_sdr_header *sdrh; IPMI_CHECK_CMD_LEN(8); if (cmd[6]) { @@ -1166,7 +1176,10 @@ static void get_sdr(IPMIBmcSim *ibs, rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; return; } - if (cmd[6] > (ibs->sdr.sdr[pos + 4])) { + + sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos]; + + if (cmd[6] > sdrh->rec_length) { rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; return; } @@ -1175,7 +1188,7 @@ static void get_sdr(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff); if (cmd[7] == 0xff) { - cmd[7] = ibs->sdr.sdr[pos + 4] - cmd[6]; + cmd[7] = sdrh->rec_length - cmd[6]; } if ((cmd[7] + *rsp_len) > max_rsp_len) { @@ -1644,18 +1657,20 @@ static void ipmi_sim_init(Object *obj) } for (i = 0;;) { + struct ipmi_sdr_header *sdrh; int len; if ((i + 5) > sizeof(init_sdrs)) { - error_report("Problem with recid 0x%4.4x: \n", i); + error_report("Problem with recid 0x%4.4x", i); return; } - len = init_sdrs[i + 4]; - recid = init_sdrs[i] | (init_sdrs[i + 1] << 8); + sdrh = (struct ipmi_sdr_header *) &init_sdrs[i]; + len = sdrh->rec_length; + recid = ipmi_sdr_recid(sdrh); if (recid == 0xffff) { break; } if ((i + len + 5) > sizeof(init_sdrs)) { - error_report("Problem with recid 0x%4.4x\n", i); + error_report("Problem with recid 0x%4.4x", i); return; } sdr_add_entry(ibs, init_sdrs + i, len, NULL); diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h index 32bac0fa8877..7e142e241dcb 100644 --- a/include/hw/ipmi/ipmi.h +++ b/include/hw/ipmi/ipmi.h @@ -210,4 +210,48 @@ IPMIFwInfo *ipmi_next_fwinfo(IPMIFwInfo *current); #define ipmi_debug(fs, ...) #endif +struct ipmi_sdr_header { + uint8_t rec_id[2]; + uint8_t sdr_version; /* 0x51 */ + uint8_t rec_type; + uint8_t rec_length; +}; +#define IPMI_SDR_HEADER_SIZE sizeof(struct ipmi_sdr_header) + +#define ipmi_sdr_recid(sdr) ((sdr)->rec_id[0] | ((sdr)->rec_id[1] << 8)) + +/* + * 43.2 SDR Type 02h. Compact Sensor Record + */ +#define IPMI_SDR_COMPACT_TYPE 2 + +struct ipmi_sdr_compact { + struct ipmi_sdr_header header; + + uint8_t sensor_owner_id; + uint8_t sensor_owner_lun; + uint8_t sensor_owner_number; /* byte 8 */ + uint8_t entity_id; + uint8_t entity_instance; + uint8_t sensor_init; + uint8_t sensor_caps; + uint8_t sensor_type; + uint8_t reading_type; + uint8_t assert_mask[2]; /* byte 16 */ + uint8_t deassert_mask[2]; + uint8_t discrete_mask[2]; + uint8_t sensor_unit1; + uint8_t sensor_unit2; + uint8_t sensor_unit3; + uint8_t sensor_direction[2]; /* byte 24 */ + uint8_t positive_threshold; + uint8_t negative_threshold; + uint8_t reserved[3]; + uint8_t oem; + uint8_t id_str_len; /* byte 32 */ + uint8_t id_string[16]; +}; + +typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)]; + #endif -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/9] ipmi: introduce a struct ipmi_sdr_compact 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 4/9] ipmi: introduce a struct ipmi_sdr_compact Cédric Le Goater @ 2016-01-22 10:49 ` Greg Kurz 2016-01-22 11:10 ` Cédric Le Goater 0 siblings, 1 reply; 25+ messages in thread From: Greg Kurz @ 2016-01-22 10:49 UTC (permalink / raw) To: Cédric Le Goater Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On Thu, 21 Jan 2016 18:18:49 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > Currently, sdr attributes are identified using byte offsets and this > can be a bit confusing. > > This patch adds a struct ipmi_sdr_compact conforming to the IPMI specs > and replaces byte offsets with names. It also introduces and uses a > struct ipmi_sdr_header in sections of the code where no assumption is > made on the type of SDR. This leave rooms to potential usage of other > types in the future. > Turning all these magic numbers into understandable names is definitely a great idea ! See comments below. > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > hw/ipmi/ipmi_bmc_sim.c | 65 +++++++++++++++++++++++++++++++------------------- > include/hw/ipmi/ipmi.h | 44 ++++++++++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+), 25 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index fc596a548df7..31f990199154 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -323,11 +323,15 @@ static void sdr_inc_reservation(IPMISdr *sdr) > static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, > unsigned int len, uint16_t *recid) > { > + struct ipmi_sdr_header *sdrh_entry = (struct ipmi_sdr_header *) entry; This looks like the entry argument should be struct ipmi_sdr_header * and you would not need sdrh_entry. > + struct ipmi_sdr_header *sdrh = > + (struct ipmi_sdr_header *) &ibs->sdr.sdr[ibs->sdr.next_free]; > + > if ((len < 5) || (len > 255)) { > return 1; > } > > - if (entry[4] != len - 5) { > + if (sdrh_entry->rec_length != len - 5) { > return 1; > } > > @@ -336,10 +340,10 @@ static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, > return 1; > } > > - memcpy(ibs->sdr.sdr + ibs->sdr.next_free, entry, len); > - ibs->sdr.sdr[ibs->sdr.next_free] = ibs->sdr.next_rec_id & 0xff; > - ibs->sdr.sdr[ibs->sdr.next_free+1] = (ibs->sdr.next_rec_id >> 8) & 0xff; > - ibs->sdr.sdr[ibs->sdr.next_free+2] = 0x51; /* Conform to IPMI 1.5 spec */ > + memcpy(sdrh, entry, len); > + sdrh->rec_id[0] = ibs->sdr.next_rec_id & 0xff; > + sdrh->rec_id[1] = (ibs->sdr.next_rec_id >> 8) & 0xff; > + sdrh->sdr_version = 0x51; /* Conform to IPMI 1.5 spec */ > > if (recid) { > *recid = ibs->sdr.next_rec_id; > @@ -357,8 +361,10 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid, > unsigned int pos = *retpos; > > while (pos < sdr->next_free) { > - uint16_t trec = sdr->sdr[pos] | (sdr->sdr[pos + 1] << 8); > - unsigned int nextpos = pos + sdr->sdr[pos + 4]; > + struct ipmi_sdr_header *sdrh = > + (struct ipmi_sdr_header *) &sdr->sdr[pos]; > + uint16_t trec = ipmi_sdr_recid(sdrh); > + unsigned int nextpos = pos + sdrh->rec_length; > > if (trec == recid) { > if (nextrec) { > @@ -507,29 +513,32 @@ static void ipmi_init_sensors_from_sdrs(IPMIBmcSim *s) > > pos = 0; > for (i = 0; !sdr_find_entry(&s->sdr, i, &pos, NULL); i++) { > - uint8_t *sdr = s->sdr.sdr + pos; > - unsigned int len = sdr[4]; > + struct ipmi_sdr_compact *sdr = > + (struct ipmi_sdr_compact *) &s->sdr.sdr[pos]; > + unsigned int len = sdr->header.rec_length; > > if (len < 20) { > continue; > } > - if ((sdr[3] < 1) || (sdr[3] > 2)) { > + if (sdr->header.rec_type != IPMI_SDR_COMPACT_TYPE) { > continue; /* Not a sensor SDR we set from */ > } > > - if (sdr[7] > MAX_SENSORS) { > + if (sdr->sensor_owner_number > MAX_SENSORS) { > continue; > } > - sens = s->sensors + sdr[7]; > + sens = s->sensors + sdr->sensor_owner_number; > > IPMI_SENSOR_SET_PRESENT(sens, 1); > - IPMI_SENSOR_SET_SCAN_ON(sens, (sdr[10] >> 6) & 1); > - IPMI_SENSOR_SET_EVENTS_ON(sens, (sdr[10] >> 5) & 1); > - sens->assert_suppt = sdr[14] | (sdr[15] << 8); > - sens->deassert_suppt = sdr[16] | (sdr[17] << 8); > - sens->states_suppt = sdr[18] | (sdr[19] << 8); > - sens->sensor_type = sdr[12]; > - sens->evt_reading_type_code = sdr[13] & 0x7f; > + IPMI_SENSOR_SET_SCAN_ON(sens, (sdr->sensor_init >> 6) & 1); > + IPMI_SENSOR_SET_EVENTS_ON(sens, (sdr->sensor_init >> 5) & 1); > + sens->assert_suppt = sdr->assert_mask[0] | (sdr->assert_mask[1] << 8); > + sens->deassert_suppt = > + sdr->deassert_mask[0] | (sdr->deassert_mask[1] << 8); > + sens->states_suppt = > + sdr->discrete_mask[0] | (sdr->discrete_mask[1] << 8); > + sens->sensor_type = sdr->sensor_type; > + sens->evt_reading_type_code = sdr->reading_type & 0x7f; > > /* Enable all the events that are supported. */ > sens->assert_enable = sens->assert_suppt; > @@ -1155,6 +1164,7 @@ static void get_sdr(IPMIBmcSim *ibs, > { > unsigned int pos; > uint16_t nextrec; > + struct ipmi_sdr_header *sdrh; > > IPMI_CHECK_CMD_LEN(8); > if (cmd[6]) { > @@ -1166,7 +1176,10 @@ static void get_sdr(IPMIBmcSim *ibs, > rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > return; > } > - if (cmd[6] > (ibs->sdr.sdr[pos + 4])) { > + > + sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos]; > + > + if (cmd[6] > sdrh->rec_length) { > rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; > return; > } > @@ -1175,7 +1188,7 @@ static void get_sdr(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff); > > if (cmd[7] == 0xff) { > - cmd[7] = ibs->sdr.sdr[pos + 4] - cmd[6]; > + cmd[7] = sdrh->rec_length - cmd[6]; > } > > if ((cmd[7] + *rsp_len) > max_rsp_len) { > @@ -1644,18 +1657,20 @@ static void ipmi_sim_init(Object *obj) > } > > for (i = 0;;) { > + struct ipmi_sdr_header *sdrh; > int len; > if ((i + 5) > sizeof(init_sdrs)) { > - error_report("Problem with recid 0x%4.4x: \n", i); > + error_report("Problem with recid 0x%4.4x", i); Unrelated change. > return; > } > - len = init_sdrs[i + 4]; > - recid = init_sdrs[i] | (init_sdrs[i + 1] << 8); > + sdrh = (struct ipmi_sdr_header *) &init_sdrs[i]; Maybe init_sdrs could be turned into something which already has explicit struct ipmi_sdr_header fields ? > + len = sdrh->rec_length; > + recid = ipmi_sdr_recid(sdrh); > if (recid == 0xffff) { > break; > } > if ((i + len + 5) > sizeof(init_sdrs)) { > - error_report("Problem with recid 0x%4.4x\n", i); > + error_report("Problem with recid 0x%4.4x", i); Unrelated change. > return; > } > sdr_add_entry(ibs, init_sdrs + i, len, NULL); > diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h > index 32bac0fa8877..7e142e241dcb 100644 > --- a/include/hw/ipmi/ipmi.h > +++ b/include/hw/ipmi/ipmi.h > @@ -210,4 +210,48 @@ IPMIFwInfo *ipmi_next_fwinfo(IPMIFwInfo *current); > #define ipmi_debug(fs, ...) > #endif > > +struct ipmi_sdr_header { > + uint8_t rec_id[2]; > + uint8_t sdr_version; /* 0x51 */ > + uint8_t rec_type; > + uint8_t rec_length; > +}; > +#define IPMI_SDR_HEADER_SIZE sizeof(struct ipmi_sdr_header) > + > +#define ipmi_sdr_recid(sdr) ((sdr)->rec_id[0] | ((sdr)->rec_id[1] << 8)) > + > +/* > + * 43.2 SDR Type 02h. Compact Sensor Record > + */ > +#define IPMI_SDR_COMPACT_TYPE 2 > + > +struct ipmi_sdr_compact { > + struct ipmi_sdr_header header; > + > + uint8_t sensor_owner_id; > + uint8_t sensor_owner_lun; > + uint8_t sensor_owner_number; /* byte 8 */ > + uint8_t entity_id; > + uint8_t entity_instance; > + uint8_t sensor_init; > + uint8_t sensor_caps; > + uint8_t sensor_type; > + uint8_t reading_type; > + uint8_t assert_mask[2]; /* byte 16 */ > + uint8_t deassert_mask[2]; > + uint8_t discrete_mask[2]; > + uint8_t sensor_unit1; > + uint8_t sensor_unit2; > + uint8_t sensor_unit3; > + uint8_t sensor_direction[2]; /* byte 24 */ > + uint8_t positive_threshold; > + uint8_t negative_threshold; > + uint8_t reserved[3]; > + uint8_t oem; > + uint8_t id_str_len; /* byte 32 */ > + uint8_t id_string[16]; > +}; > + > +typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)]; > + > #endif ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/9] ipmi: introduce a struct ipmi_sdr_compact 2016-01-22 10:49 ` Greg Kurz @ 2016-01-22 11:10 ` Cédric Le Goater 0 siblings, 0 replies; 25+ messages in thread From: Cédric Le Goater @ 2016-01-22 11:10 UTC (permalink / raw) To: Greg Kurz; +Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On 01/22/2016 11:49 AM, Greg Kurz wrote: > On Thu, 21 Jan 2016 18:18:49 +0100 > Cédric Le Goater <clg@fr.ibm.com> wrote: > >> Currently, sdr attributes are identified using byte offsets and this >> can be a bit confusing. >> >> This patch adds a struct ipmi_sdr_compact conforming to the IPMI specs >> and replaces byte offsets with names. It also introduces and uses a >> struct ipmi_sdr_header in sections of the code where no assumption is >> made on the type of SDR. This leave rooms to potential usage of other >> types in the future. >> > > Turning all these magic numbers into understandable names is definitely a > great idea ! > > See comments below. > >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >> --- >> hw/ipmi/ipmi_bmc_sim.c | 65 +++++++++++++++++++++++++++++++------------------- >> include/hw/ipmi/ipmi.h | 44 ++++++++++++++++++++++++++++++++++ >> 2 files changed, 84 insertions(+), 25 deletions(-) >> >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >> index fc596a548df7..31f990199154 100644 >> --- a/hw/ipmi/ipmi_bmc_sim.c >> +++ b/hw/ipmi/ipmi_bmc_sim.c >> @@ -323,11 +323,15 @@ static void sdr_inc_reservation(IPMISdr *sdr) >> static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, >> unsigned int len, uint16_t *recid) >> { >> + struct ipmi_sdr_header *sdrh_entry = (struct ipmi_sdr_header *) entry; > > This looks like the entry argument should be struct ipmi_sdr_header * and > you would not need sdrh_entry. Indeed and it improves readability a little more. I will send a fix in next patchset. >> + struct ipmi_sdr_header *sdrh = >> + (struct ipmi_sdr_header *) &ibs->sdr.sdr[ibs->sdr.next_free]; >> + >> if ((len < 5) || (len > 255)) { >> return 1; >> } >> >> - if (entry[4] != len - 5) { >> + if (sdrh_entry->rec_length != len - 5) { >> return 1; >> } >> >> @@ -336,10 +340,10 @@ static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, >> return 1; >> } >> >> - memcpy(ibs->sdr.sdr + ibs->sdr.next_free, entry, len); >> - ibs->sdr.sdr[ibs->sdr.next_free] = ibs->sdr.next_rec_id & 0xff; >> - ibs->sdr.sdr[ibs->sdr.next_free+1] = (ibs->sdr.next_rec_id >> 8) & 0xff; >> - ibs->sdr.sdr[ibs->sdr.next_free+2] = 0x51; /* Conform to IPMI 1.5 spec */ >> + memcpy(sdrh, entry, len); >> + sdrh->rec_id[0] = ibs->sdr.next_rec_id & 0xff; >> + sdrh->rec_id[1] = (ibs->sdr.next_rec_id >> 8) & 0xff; >> + sdrh->sdr_version = 0x51; /* Conform to IPMI 1.5 spec */ >> >> if (recid) { >> *recid = ibs->sdr.next_rec_id; >> @@ -357,8 +361,10 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid, >> unsigned int pos = *retpos; >> >> while (pos < sdr->next_free) { >> - uint16_t trec = sdr->sdr[pos] | (sdr->sdr[pos + 1] << 8); >> - unsigned int nextpos = pos + sdr->sdr[pos + 4]; >> + struct ipmi_sdr_header *sdrh = >> + (struct ipmi_sdr_header *) &sdr->sdr[pos]; >> + uint16_t trec = ipmi_sdr_recid(sdrh); >> + unsigned int nextpos = pos + sdrh->rec_length; >> >> if (trec == recid) { >> if (nextrec) { >> @@ -507,29 +513,32 @@ static void ipmi_init_sensors_from_sdrs(IPMIBmcSim *s) >> >> pos = 0; >> for (i = 0; !sdr_find_entry(&s->sdr, i, &pos, NULL); i++) { >> - uint8_t *sdr = s->sdr.sdr + pos; >> - unsigned int len = sdr[4]; >> + struct ipmi_sdr_compact *sdr = >> + (struct ipmi_sdr_compact *) &s->sdr.sdr[pos]; >> + unsigned int len = sdr->header.rec_length; >> >> if (len < 20) { >> continue; >> } >> - if ((sdr[3] < 1) || (sdr[3] > 2)) { >> + if (sdr->header.rec_type != IPMI_SDR_COMPACT_TYPE) { >> continue; /* Not a sensor SDR we set from */ >> } >> >> - if (sdr[7] > MAX_SENSORS) { >> + if (sdr->sensor_owner_number > MAX_SENSORS) { >> continue; >> } >> - sens = s->sensors + sdr[7]; >> + sens = s->sensors + sdr->sensor_owner_number; >> >> IPMI_SENSOR_SET_PRESENT(sens, 1); >> - IPMI_SENSOR_SET_SCAN_ON(sens, (sdr[10] >> 6) & 1); >> - IPMI_SENSOR_SET_EVENTS_ON(sens, (sdr[10] >> 5) & 1); >> - sens->assert_suppt = sdr[14] | (sdr[15] << 8); >> - sens->deassert_suppt = sdr[16] | (sdr[17] << 8); >> - sens->states_suppt = sdr[18] | (sdr[19] << 8); >> - sens->sensor_type = sdr[12]; >> - sens->evt_reading_type_code = sdr[13] & 0x7f; >> + IPMI_SENSOR_SET_SCAN_ON(sens, (sdr->sensor_init >> 6) & 1); >> + IPMI_SENSOR_SET_EVENTS_ON(sens, (sdr->sensor_init >> 5) & 1); >> + sens->assert_suppt = sdr->assert_mask[0] | (sdr->assert_mask[1] << 8); >> + sens->deassert_suppt = >> + sdr->deassert_mask[0] | (sdr->deassert_mask[1] << 8); >> + sens->states_suppt = >> + sdr->discrete_mask[0] | (sdr->discrete_mask[1] << 8); >> + sens->sensor_type = sdr->sensor_type; >> + sens->evt_reading_type_code = sdr->reading_type & 0x7f; >> >> /* Enable all the events that are supported. */ >> sens->assert_enable = sens->assert_suppt; >> @@ -1155,6 +1164,7 @@ static void get_sdr(IPMIBmcSim *ibs, >> { >> unsigned int pos; >> uint16_t nextrec; >> + struct ipmi_sdr_header *sdrh; >> >> IPMI_CHECK_CMD_LEN(8); >> if (cmd[6]) { >> @@ -1166,7 +1176,10 @@ static void get_sdr(IPMIBmcSim *ibs, >> rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; >> return; >> } >> - if (cmd[6] > (ibs->sdr.sdr[pos + 4])) { >> + >> + sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos]; >> + >> + if (cmd[6] > sdrh->rec_length) { >> rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; >> return; >> } >> @@ -1175,7 +1188,7 @@ static void get_sdr(IPMIBmcSim *ibs, >> IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff); >> >> if (cmd[7] == 0xff) { >> - cmd[7] = ibs->sdr.sdr[pos + 4] - cmd[6]; >> + cmd[7] = sdrh->rec_length - cmd[6]; >> } >> >> if ((cmd[7] + *rsp_len) > max_rsp_len) { >> @@ -1644,18 +1657,20 @@ static void ipmi_sim_init(Object *obj) >> } >> >> for (i = 0;;) { >> + struct ipmi_sdr_header *sdrh; >> int len; >> if ((i + 5) > sizeof(init_sdrs)) { >> - error_report("Problem with recid 0x%4.4x: \n", i); >> + error_report("Problem with recid 0x%4.4x", i); > > Unrelated change. OK. I will pull these changes in a preliminary patch. >> return; >> } >> - len = init_sdrs[i + 4]; >> - recid = init_sdrs[i] | (init_sdrs[i + 1] << 8); >> + sdrh = (struct ipmi_sdr_header *) &init_sdrs[i]; > > Maybe init_sdrs could be turned into something which already has explicit > struct ipmi_sdr_header fields ? I don't necessarily agree here but may be, the name is confusing and should be something like 'sdr_blob', which will make more sense when we load sdrs from a file. init_sdrs contains raw data and this routine loops over it to find enough bytes for a minimal ipmi sdr header, and then enough bytes to add a sdr entry in the BMC table. So we are manipulating bytes all the time until the end when we : sdr_add_entry(ibs, init_sdrs + i, len, NULL); Your suggestion on changing sdr_add_entry() API improves the code a little. Thanks ! C. > >> + len = sdrh->rec_length; >> + recid = ipmi_sdr_recid(sdrh); >> if (recid == 0xffff) { >> break; >> } >> if ((i + len + 5) > sizeof(init_sdrs)) { >> - error_report("Problem with recid 0x%4.4x\n", i); >> + error_report("Problem with recid 0x%4.4x", i); > > Unrelated change. > >> return; >> } >> sdr_add_entry(ibs, init_sdrs + i, len, NULL); >> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h >> index 32bac0fa8877..7e142e241dcb 100644 >> --- a/include/hw/ipmi/ipmi.h >> +++ b/include/hw/ipmi/ipmi.h >> @@ -210,4 +210,48 @@ IPMIFwInfo *ipmi_next_fwinfo(IPMIFwInfo *current); >> #define ipmi_debug(fs, ...) >> #endif >> >> +struct ipmi_sdr_header { >> + uint8_t rec_id[2]; >> + uint8_t sdr_version; /* 0x51 */ >> + uint8_t rec_type; >> + uint8_t rec_length; >> +}; >> +#define IPMI_SDR_HEADER_SIZE sizeof(struct ipmi_sdr_header) >> + >> +#define ipmi_sdr_recid(sdr) ((sdr)->rec_id[0] | ((sdr)->rec_id[1] << 8)) >> + >> +/* >> + * 43.2 SDR Type 02h. Compact Sensor Record >> + */ >> +#define IPMI_SDR_COMPACT_TYPE 2 >> + >> +struct ipmi_sdr_compact { >> + struct ipmi_sdr_header header; >> + >> + uint8_t sensor_owner_id; >> + uint8_t sensor_owner_lun; >> + uint8_t sensor_owner_number; /* byte 8 */ >> + uint8_t entity_id; >> + uint8_t entity_instance; >> + uint8_t sensor_init; >> + uint8_t sensor_caps; >> + uint8_t sensor_type; >> + uint8_t reading_type; >> + uint8_t assert_mask[2]; /* byte 16 */ >> + uint8_t deassert_mask[2]; >> + uint8_t discrete_mask[2]; >> + uint8_t sensor_unit1; >> + uint8_t sensor_unit2; >> + uint8_t sensor_unit3; >> + uint8_t sensor_direction[2]; /* byte 24 */ >> + uint8_t positive_threshold; >> + uint8_t negative_threshold; >> + uint8_t reserved[3]; >> + uint8_t oem; >> + uint8_t id_str_len; /* byte 32 */ >> + uint8_t id_string[16]; >> +}; >> + >> +typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)]; >> + >> #endif > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 5/9] ipmi: fix SDR length value 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater ` (3 preceding siblings ...) 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 4/9] ipmi: introduce a struct ipmi_sdr_compact Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-22 10:56 ` Greg Kurz 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 6/9] ipmi: add get and set SENSOR_TYPE commands Cédric Le Goater ` (3 subsequent siblings) 8 siblings, 1 reply; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin The IPMI BMC simulator populates the SDR table with a set of initial SDRs. The length of each SDR is taken from the record itself (byte 4) which does not include the size of the header. But, the full length (header + data) is required by the sdr_add_entry() routine. Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- hw/ipmi/ipmi_bmc_sim.c | 18 +++++++++--------- include/hw/ipmi/ipmi.h | 1 + 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index 31f990199154..803c7e5130c0 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -327,11 +327,11 @@ static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, struct ipmi_sdr_header *sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[ibs->sdr.next_free]; - if ((len < 5) || (len > 255)) { + if ((len < IPMI_SDR_HEADER_SIZE) || (len > 255)) { return 1; } - if (sdrh_entry->rec_length != len - 5) { + if (ipmi_sdr_length(sdrh_entry) != len) { return 1; } @@ -364,7 +364,7 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid, struct ipmi_sdr_header *sdrh = (struct ipmi_sdr_header *) &sdr->sdr[pos]; uint16_t trec = ipmi_sdr_recid(sdrh); - unsigned int nextpos = pos + sdrh->rec_length; + unsigned int nextpos = pos + ipmi_sdr_length(sdrh); if (trec == recid) { if (nextrec) { @@ -1179,7 +1179,7 @@ static void get_sdr(IPMIBmcSim *ibs, sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos]; - if (cmd[6] > sdrh->rec_length) { + if (cmd[6] > ipmi_sdr_length(sdrh)) { rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; return; } @@ -1188,7 +1188,7 @@ static void get_sdr(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff); if (cmd[7] == 0xff) { - cmd[7] = sdrh->rec_length - cmd[6]; + cmd[7] = ipmi_sdr_length(sdrh) - cmd[6]; } if ((cmd[7] + *rsp_len) > max_rsp_len) { @@ -1659,22 +1659,22 @@ static void ipmi_sim_init(Object *obj) for (i = 0;;) { struct ipmi_sdr_header *sdrh; int len; - if ((i + 5) > sizeof(init_sdrs)) { + 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 = sdrh->rec_length; + len = ipmi_sdr_length(sdrh); recid = ipmi_sdr_recid(sdrh); if (recid == 0xffff) { break; } - if ((i + len + 5) > sizeof(init_sdrs)) { + if ((i + len) > sizeof(init_sdrs)) { error_report("Problem with recid 0x%4.4x", i); return; } sdr_add_entry(ibs, init_sdrs + i, len, NULL); - i += len + 5; + i += len; } ipmi_init_sensors_from_sdrs(ibs); diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h index 7e142e241dcb..74a2b5af9613 100644 --- a/include/hw/ipmi/ipmi.h +++ b/include/hw/ipmi/ipmi.h @@ -219,6 +219,7 @@ struct ipmi_sdr_header { #define IPMI_SDR_HEADER_SIZE sizeof(struct ipmi_sdr_header) #define ipmi_sdr_recid(sdr) ((sdr)->rec_id[0] | ((sdr)->rec_id[1] << 8)) +#define ipmi_sdr_length(sdr) ((sdr)->rec_length + IPMI_SDR_HEADER_SIZE) /* * 43.2 SDR Type 02h. Compact Sensor Record -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/9] ipmi: fix SDR length value 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 5/9] ipmi: fix SDR length value Cédric Le Goater @ 2016-01-22 10:56 ` Greg Kurz 2016-01-22 11:15 ` Cédric Le Goater 0 siblings, 1 reply; 25+ messages in thread From: Greg Kurz @ 2016-01-22 10:56 UTC (permalink / raw) To: Cédric Le Goater Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On Thu, 21 Jan 2016 18:18:50 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > The IPMI BMC simulator populates the SDR table with a set of initial > SDRs. The length of each SDR is taken from the record itself (byte 4) > which does not include the size of the header. But, the full length > (header + data) is required by the sdr_add_entry() routine. > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> The patch is good but IMHO it should come before patch 4 because this is bugfix that could be applied right away, while patch 4 is code cleanup that may need some more discussion. > --- > hw/ipmi/ipmi_bmc_sim.c | 18 +++++++++--------- > include/hw/ipmi/ipmi.h | 1 + > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 31f990199154..803c7e5130c0 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -327,11 +327,11 @@ static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, > struct ipmi_sdr_header *sdrh = > (struct ipmi_sdr_header *) &ibs->sdr.sdr[ibs->sdr.next_free]; > > - if ((len < 5) || (len > 255)) { > + if ((len < IPMI_SDR_HEADER_SIZE) || (len > 255)) { > return 1; > } > > - if (sdrh_entry->rec_length != len - 5) { > + if (ipmi_sdr_length(sdrh_entry) != len) { > return 1; > } > > @@ -364,7 +364,7 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid, > struct ipmi_sdr_header *sdrh = > (struct ipmi_sdr_header *) &sdr->sdr[pos]; > uint16_t trec = ipmi_sdr_recid(sdrh); > - unsigned int nextpos = pos + sdrh->rec_length; > + unsigned int nextpos = pos + ipmi_sdr_length(sdrh); > > if (trec == recid) { > if (nextrec) { > @@ -1179,7 +1179,7 @@ static void get_sdr(IPMIBmcSim *ibs, > > sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos]; > > - if (cmd[6] > sdrh->rec_length) { > + if (cmd[6] > ipmi_sdr_length(sdrh)) { > rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; > return; > } > @@ -1188,7 +1188,7 @@ static void get_sdr(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff); > > if (cmd[7] == 0xff) { > - cmd[7] = sdrh->rec_length - cmd[6]; > + cmd[7] = ipmi_sdr_length(sdrh) - cmd[6]; > } > > if ((cmd[7] + *rsp_len) > max_rsp_len) { > @@ -1659,22 +1659,22 @@ static void ipmi_sim_init(Object *obj) > for (i = 0;;) { > struct ipmi_sdr_header *sdrh; > int len; > - if ((i + 5) > sizeof(init_sdrs)) { > + 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 = sdrh->rec_length; > + len = ipmi_sdr_length(sdrh); > recid = ipmi_sdr_recid(sdrh); > if (recid == 0xffff) { > break; > } > - if ((i + len + 5) > sizeof(init_sdrs)) { > + if ((i + len) > sizeof(init_sdrs)) { > error_report("Problem with recid 0x%4.4x", i); > return; > } > sdr_add_entry(ibs, init_sdrs + i, len, NULL); > - i += len + 5; > + i += len; > } > > ipmi_init_sensors_from_sdrs(ibs); > diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h > index 7e142e241dcb..74a2b5af9613 100644 > --- a/include/hw/ipmi/ipmi.h > +++ b/include/hw/ipmi/ipmi.h > @@ -219,6 +219,7 @@ struct ipmi_sdr_header { > #define IPMI_SDR_HEADER_SIZE sizeof(struct ipmi_sdr_header) > > #define ipmi_sdr_recid(sdr) ((sdr)->rec_id[0] | ((sdr)->rec_id[1] << 8)) > +#define ipmi_sdr_length(sdr) ((sdr)->rec_length + IPMI_SDR_HEADER_SIZE) > > /* > * 43.2 SDR Type 02h. Compact Sensor Record ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/9] ipmi: fix SDR length value 2016-01-22 10:56 ` Greg Kurz @ 2016-01-22 11:15 ` Cédric Le Goater 0 siblings, 0 replies; 25+ messages in thread From: Cédric Le Goater @ 2016-01-22 11:15 UTC (permalink / raw) To: Greg Kurz; +Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On 01/22/2016 11:56 AM, Greg Kurz wrote: > On Thu, 21 Jan 2016 18:18:50 +0100 > Cédric Le Goater <clg@fr.ibm.com> wrote: >> The IPMI BMC simulator populates the SDR table with a set of initial >> SDRs. The length of each SDR is taken from the record itself (byte 4) >> which does not include the size of the header. But, the full length >> (header + data) is required by the sdr_add_entry() routine. >> >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > > The patch is good but IMHO it should come before patch 4 because this is bugfix > that could be applied right away, while patch 4 is code cleanup that may need > some more discussion. OK. I am fine with that. It should be the patch from v1. Thanks, C. >> --- >> hw/ipmi/ipmi_bmc_sim.c | 18 +++++++++--------- >> include/hw/ipmi/ipmi.h | 1 + >> 2 files changed, 10 insertions(+), 9 deletions(-) >> >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >> index 31f990199154..803c7e5130c0 100644 >> --- a/hw/ipmi/ipmi_bmc_sim.c >> +++ b/hw/ipmi/ipmi_bmc_sim.c >> @@ -327,11 +327,11 @@ static int sdr_add_entry(IPMIBmcSim *ibs, const uint8_t *entry, >> struct ipmi_sdr_header *sdrh = >> (struct ipmi_sdr_header *) &ibs->sdr.sdr[ibs->sdr.next_free]; >> >> - if ((len < 5) || (len > 255)) { >> + if ((len < IPMI_SDR_HEADER_SIZE) || (len > 255)) { >> return 1; >> } >> >> - if (sdrh_entry->rec_length != len - 5) { >> + if (ipmi_sdr_length(sdrh_entry) != len) { >> return 1; >> } >> >> @@ -364,7 +364,7 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid, >> struct ipmi_sdr_header *sdrh = >> (struct ipmi_sdr_header *) &sdr->sdr[pos]; >> uint16_t trec = ipmi_sdr_recid(sdrh); >> - unsigned int nextpos = pos + sdrh->rec_length; >> + unsigned int nextpos = pos + ipmi_sdr_length(sdrh); >> >> if (trec == recid) { >> if (nextrec) { >> @@ -1179,7 +1179,7 @@ static void get_sdr(IPMIBmcSim *ibs, >> >> sdrh = (struct ipmi_sdr_header *) &ibs->sdr.sdr[pos]; >> >> - if (cmd[6] > sdrh->rec_length) { >> + if (cmd[6] > ipmi_sdr_length(sdrh)) { >> rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; >> return; >> } >> @@ -1188,7 +1188,7 @@ static void get_sdr(IPMIBmcSim *ibs, >> IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff); >> >> if (cmd[7] == 0xff) { >> - cmd[7] = sdrh->rec_length - cmd[6]; >> + cmd[7] = ipmi_sdr_length(sdrh) - cmd[6]; >> } >> >> if ((cmd[7] + *rsp_len) > max_rsp_len) { >> @@ -1659,22 +1659,22 @@ static void ipmi_sim_init(Object *obj) >> for (i = 0;;) { >> struct ipmi_sdr_header *sdrh; >> int len; >> - if ((i + 5) > sizeof(init_sdrs)) { >> + 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 = sdrh->rec_length; >> + len = ipmi_sdr_length(sdrh); >> recid = ipmi_sdr_recid(sdrh); >> if (recid == 0xffff) { >> break; >> } >> - if ((i + len + 5) > sizeof(init_sdrs)) { >> + if ((i + len) > sizeof(init_sdrs)) { >> error_report("Problem with recid 0x%4.4x", i); >> return; >> } >> sdr_add_entry(ibs, init_sdrs + i, len, NULL); >> - i += len + 5; >> + i += len; >> } >> >> ipmi_init_sensors_from_sdrs(ibs); >> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h >> index 7e142e241dcb..74a2b5af9613 100644 >> --- a/include/hw/ipmi/ipmi.h >> +++ b/include/hw/ipmi/ipmi.h >> @@ -219,6 +219,7 @@ struct ipmi_sdr_header { >> #define IPMI_SDR_HEADER_SIZE sizeof(struct ipmi_sdr_header) >> >> #define ipmi_sdr_recid(sdr) ((sdr)->rec_id[0] | ((sdr)->rec_id[1] << 8)) >> +#define ipmi_sdr_length(sdr) ((sdr)->rec_length + IPMI_SDR_HEADER_SIZE) >> >> /* >> * 43.2 SDR Type 02h. Compact Sensor Record > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 6/9] ipmi: add get and set SENSOR_TYPE commands 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater ` (4 preceding siblings ...) 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 5/9] ipmi: fix SDR length value Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-22 11:07 ` Greg Kurz 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 7/9] ipmi: add GET_SYS_RESTART_CAUSE chassis command Cédric Le Goater ` (2 subsequent siblings) 8 siblings, 1 reply; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> Acked-by: Corey Minyard <cminyard@mvista.com> --- hw/ipmi/ipmi_bmc_sim.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index 803c7e5130c0..7c0f2a1d9799 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -42,6 +42,8 @@ #define IPMI_CMD_REARM_SENSOR_EVTS 0x2a #define IPMI_CMD_GET_SENSOR_EVT_STATUS 0x2b #define IPMI_CMD_GET_SENSOR_READING 0x2d +#define IPMI_CMD_SET_SENSOR_TYPE 0x2e +#define IPMI_CMD_GET_SENSOR_TYPE 0x2f /* #define IPMI_NETFN_APP 0x06 In ipmi.h */ @@ -1527,6 +1529,45 @@ static void get_sensor_reading(IPMIBmcSim *ibs, } } +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) +{ + 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; + return; + } + sens = ibs->sensors + cmd[2]; + sens->sensor_type = cmd[3]; + sens->evt_reading_type_code = cmd[4] & 0x7f; +} + +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) +{ + 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; + return; + } + sens = ibs->sensors + cmd[2]; + IPMI_ADD_RSP_DATA(sens->sensor_type); + IPMI_ADD_RSP_DATA(sens->evt_reading_type_code); +} + + static const IPMICmdHandler chassis_cmds[] = { [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities, [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status, @@ -1542,7 +1583,9 @@ static const IPMICmdHandler sensor_event_cmds[] = { [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_GET_SENSOR_READING] = get_sensor_reading, + [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type, + [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type, }; static const IPMINetfn sensor_event_netfn = { .cmd_nums = ARRAY_SIZE(sensor_event_cmds), -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/9] ipmi: add get and set SENSOR_TYPE commands 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 6/9] ipmi: add get and set SENSOR_TYPE commands Cédric Le Goater @ 2016-01-22 11:07 ` Greg Kurz 2016-01-22 11:13 ` Cédric Le Goater 0 siblings, 1 reply; 25+ messages in thread From: Greg Kurz @ 2016-01-22 11:07 UTC (permalink / raw) To: Cédric Le Goater Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On Thu, 21 Jan 2016 18:18:51 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > Acked-by: Corey Minyard <cminyard@mvista.com> > --- Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com> Just two nits below. > hw/ipmi/ipmi_bmc_sim.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 803c7e5130c0..7c0f2a1d9799 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -42,6 +42,8 @@ > #define IPMI_CMD_REARM_SENSOR_EVTS 0x2a > #define IPMI_CMD_GET_SENSOR_EVT_STATUS 0x2b > #define IPMI_CMD_GET_SENSOR_READING 0x2d > +#define IPMI_CMD_SET_SENSOR_TYPE 0x2e > +#define IPMI_CMD_GET_SENSOR_TYPE 0x2f > > /* #define IPMI_NETFN_APP 0x06 In ipmi.h */ > > @@ -1527,6 +1529,45 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > } > } > > +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) > +{ > + IPMISensor *sens; > + > + > + IPMI_CHECK_CMD_LEN(5); > + if ((cmd[2] > MAX_SENSORS) || This has been a recurring remark on many patches lately, and all the people don't necessarily agree but the extra parenthesis are not needed here... > + !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > + rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > + return; > + } > + sens = ibs->sensors + cmd[2]; > + sens->sensor_type = cmd[3]; > + sens->evt_reading_type_code = cmd[4] & 0x7f; > +} > + > +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) > +{ > + IPMISensor *sens; > + > + > + IPMI_CHECK_CMD_LEN(3); > + if ((cmd[2] > MAX_SENSORS) || and here. > + !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > + rsp[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); > +} > + > + > static const IPMICmdHandler chassis_cmds[] = { > [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities, > [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status, > @@ -1542,7 +1583,9 @@ static const IPMICmdHandler sensor_event_cmds[] = { > [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_GET_SENSOR_READING] = get_sensor_reading, > + [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type, > + [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type, > }; > static const IPMINetfn sensor_event_netfn = { > .cmd_nums = ARRAY_SIZE(sensor_event_cmds), ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/9] ipmi: add get and set SENSOR_TYPE commands 2016-01-22 11:07 ` Greg Kurz @ 2016-01-22 11:13 ` Cédric Le Goater 0 siblings, 0 replies; 25+ messages in thread From: Cédric Le Goater @ 2016-01-22 11:13 UTC (permalink / raw) To: Greg Kurz; +Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On 01/22/2016 12:07 PM, Greg Kurz wrote: > On Thu, 21 Jan 2016 18:18:51 +0100 > Cédric Le Goater <clg@fr.ibm.com> wrote: > >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >> Acked-by: Corey Minyard <cminyard@mvista.com> >> --- > > Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > Just two nits below. > >> hw/ipmi/ipmi_bmc_sim.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 44 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >> index 803c7e5130c0..7c0f2a1d9799 100644 >> --- a/hw/ipmi/ipmi_bmc_sim.c >> +++ b/hw/ipmi/ipmi_bmc_sim.c >> @@ -42,6 +42,8 @@ >> #define IPMI_CMD_REARM_SENSOR_EVTS 0x2a >> #define IPMI_CMD_GET_SENSOR_EVT_STATUS 0x2b >> #define IPMI_CMD_GET_SENSOR_READING 0x2d >> +#define IPMI_CMD_SET_SENSOR_TYPE 0x2e >> +#define IPMI_CMD_GET_SENSOR_TYPE 0x2f >> >> /* #define IPMI_NETFN_APP 0x06 In ipmi.h */ >> >> @@ -1527,6 +1529,45 @@ static void get_sensor_reading(IPMIBmcSim *ibs, >> } >> } >> >> +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) >> +{ >> + IPMISensor *sens; >> + >> + >> + IPMI_CHECK_CMD_LEN(5); >> + if ((cmd[2] > MAX_SENSORS) || > > This has been a recurring remark on many patches lately, and all the people > don't necessarily agree but the extra parenthesis are not needed here... Damn. Am I contaminated ? :) C. >> + !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { >> + rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; >> + return; >> + } >> + sens = ibs->sensors + cmd[2]; >> + sens->sensor_type = cmd[3]; >> + sens->evt_reading_type_code = cmd[4] & 0x7f; >> +} >> + >> +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) >> +{ >> + IPMISensor *sens; >> + >> + >> + IPMI_CHECK_CMD_LEN(3); >> + if ((cmd[2] > MAX_SENSORS) || > > and here. > >> + !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { >> + rsp[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); >> +} >> + >> + >> static const IPMICmdHandler chassis_cmds[] = { >> [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities, >> [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status, >> @@ -1542,7 +1583,9 @@ static const IPMICmdHandler sensor_event_cmds[] = { >> [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_GET_SENSOR_READING] = get_sensor_reading, >> + [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type, >> + [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type, >> }; >> static const IPMINetfn sensor_event_netfn = { >> .cmd_nums = ARRAY_SIZE(sensor_event_cmds), > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 7/9] ipmi: add GET_SYS_RESTART_CAUSE chassis command 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater ` (5 preceding siblings ...) 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 6/9] ipmi: add get and set SENSOR_TYPE commands Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-22 11:09 ` Greg Kurz 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 9/9] ipmi: add SET_SENSOR_READING command (tentative try) Cédric Le Goater 8 siblings, 1 reply; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin This is a simulator. Just return an unknown cause (0). Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> Acked-by: Corey Minyard <cminyard@mvista.com> --- hw/ipmi/ipmi_bmc_sim.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index 7c0f2a1d9799..e882af3f1b40 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -34,6 +34,7 @@ #define IPMI_CMD_GET_CHASSIS_CAPABILITIES 0x00 #define IPMI_CMD_GET_CHASSIS_STATUS 0x01 #define IPMI_CMD_CHASSIS_CONTROL 0x02 +#define IPMI_CMD_GET_SYS_RESTART_CAUSE 0x09 #define IPMI_NETFN_SENSOR_EVENT 0x04 @@ -197,6 +198,8 @@ struct IPMIBmcSim { uint8_t mfg_id[3]; uint8_t product_id[2]; + uint8_t restart_cause; + IPMISel sel; IPMISdr sdr; IPMISensor sensors[MAX_SENSORS]; @@ -756,6 +759,15 @@ static void chassis_control(IPMIBmcSim *ibs, } } +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) +{ + IPMI_ADD_RSP_DATA(ibs->restart_cause & 0xf); /* Restart Cause */ + IPMI_ADD_RSP_DATA(0); /* Channel 0 */ +} + static void get_device_id(IPMIBmcSim *ibs, uint8_t *cmd, unsigned int cmd_len, uint8_t *rsp, unsigned int *rsp_len, @@ -1571,7 +1583,8 @@ static void get_sensor_type(IPMIBmcSim *ibs, 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_CHASSIS_CONTROL] = chassis_control, + [IPMI_CMD_GET_SYS_RESTART_CAUSE] = chassis_get_sys_restart_cause }; static const IPMINetfn chassis_netfn = { .cmd_nums = ARRAY_SIZE(chassis_cmds), @@ -1692,6 +1705,7 @@ static void ipmi_sim_init(Object *obj) ibs->bmc_global_enables = (1 << IPMI_BMC_EVENT_LOG_BIT); ibs->device_id = 0x20; ibs->ipmi_version = 0x02; /* IPMI 2.0 */ + ibs->restart_cause = 0; for (i = 0; i < 4; i++) { ibs->sel.last_addition[i] = 0xff; ibs->sel.last_clear[i] = 0xff; -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/9] ipmi: add GET_SYS_RESTART_CAUSE chassis command 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 7/9] ipmi: add GET_SYS_RESTART_CAUSE chassis command Cédric Le Goater @ 2016-01-22 11:09 ` Greg Kurz 0 siblings, 0 replies; 25+ messages in thread From: Greg Kurz @ 2016-01-22 11:09 UTC (permalink / raw) To: Cédric Le Goater Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On Thu, 21 Jan 2016 18:18:52 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > This is a simulator. Just return an unknown cause (0). > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > Acked-by: Corey Minyard <cminyard@mvista.com> > --- Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > hw/ipmi/ipmi_bmc_sim.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 7c0f2a1d9799..e882af3f1b40 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -34,6 +34,7 @@ > #define IPMI_CMD_GET_CHASSIS_CAPABILITIES 0x00 > #define IPMI_CMD_GET_CHASSIS_STATUS 0x01 > #define IPMI_CMD_CHASSIS_CONTROL 0x02 > +#define IPMI_CMD_GET_SYS_RESTART_CAUSE 0x09 > > #define IPMI_NETFN_SENSOR_EVENT 0x04 > > @@ -197,6 +198,8 @@ struct IPMIBmcSim { > uint8_t mfg_id[3]; > uint8_t product_id[2]; > > + uint8_t restart_cause; > + > IPMISel sel; > IPMISdr sdr; > IPMISensor sensors[MAX_SENSORS]; > @@ -756,6 +759,15 @@ static void chassis_control(IPMIBmcSim *ibs, > } > } > > +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) > +{ > + IPMI_ADD_RSP_DATA(ibs->restart_cause & 0xf); /* Restart Cause */ > + IPMI_ADD_RSP_DATA(0); /* Channel 0 */ > +} > + > static void get_device_id(IPMIBmcSim *ibs, > uint8_t *cmd, unsigned int cmd_len, > uint8_t *rsp, unsigned int *rsp_len, > @@ -1571,7 +1583,8 @@ static void get_sensor_type(IPMIBmcSim *ibs, > 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_CHASSIS_CONTROL] = chassis_control, > + [IPMI_CMD_GET_SYS_RESTART_CAUSE] = chassis_get_sys_restart_cause > }; > static const IPMINetfn chassis_netfn = { > .cmd_nums = ARRAY_SIZE(chassis_cmds), > @@ -1692,6 +1705,7 @@ static void ipmi_sim_init(Object *obj) > ibs->bmc_global_enables = (1 << IPMI_BMC_EVENT_LOG_BIT); > ibs->device_id = 0x20; > ibs->ipmi_version = 0x02; /* IPMI 2.0 */ > + ibs->restart_cause = 0; > for (i = 0; i < 4; i++) { > ibs->sel.last_addition[i] = 0xff; > ibs->sel.last_clear[i] = 0xff; ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater ` (6 preceding siblings ...) 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 7/9] ipmi: add GET_SYS_RESTART_CAUSE chassis command Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-22 11:24 ` Greg Kurz 2016-01-22 13:04 ` Corey Minyard 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 9/9] ipmi: add SET_SENSOR_READING command (tentative try) Cédric Le Goater 8 siblings, 2 replies; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- Changes since v1: - added ACPI to command names. hw/ipmi/ipmi_bmc_sim.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index e882af3f1b40..53c75cb21c1a 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -25,6 +25,7 @@ #include <stdio.h> #include <string.h> #include <stdint.h> +#include "sysemu/sysemu.h" #include "qemu/timer.h" #include "hw/ipmi/ipmi.h" #include "qemu/error-report.h" @@ -51,6 +52,9 @@ #define IPMI_CMD_GET_DEVICE_ID 0x01 #define IPMI_CMD_COLD_RESET 0x02 #define IPMI_CMD_WARM_RESET 0x03 +#define IPMI_CMD_SET_ACPI_POWER_STATE 0x06 +#define IPMI_CMD_GET_ACPI_POWER_STATE 0x07 +#define IPMI_CMD_GET_DEVICE_GUID 0x08 #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 @@ -200,6 +204,9 @@ struct IPMIBmcSim { uint8_t restart_cause; + uint8_t acpi_power_state[2]; + uint8_t uuid[16]; + IPMISel sel; IPMISdr sdr; IPMISensor sensors[MAX_SENSORS]; @@ -828,6 +835,36 @@ static void warm_reset(IPMIBmcSim *ibs, k->reset(s, false); } } +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) +{ + IPMI_CHECK_CMD_LEN(4); + 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) +{ + IPMI_ADD_RSP_DATA(ibs->acpi_power_state[0]); + IPMI_ADD_RSP_DATA(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) +{ + unsigned int i; + + for (i = 0; i < 16; i++) { + IPMI_ADD_RSP_DATA(ibs->uuid[i]); + } +} static void set_bmc_global_enables(IPMIBmcSim *ibs, uint8_t *cmd, unsigned int cmd_len, @@ -1609,6 +1646,9 @@ 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, @@ -1734,6 +1774,15 @@ static void ipmi_sim_init(Object *obj) i += len; } + ibs->acpi_power_state[0] = 0; + ibs->acpi_power_state[1] = 0; + + if (qemu_uuid_set) { + memcpy(&ibs->uuid, qemu_uuid, 16); + } else { + memset(&ibs->uuid, 0, 16); + } + ipmi_init_sensors_from_sdrs(ibs); register_cmds(ibs); -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands Cédric Le Goater @ 2016-01-22 11:24 ` Greg Kurz 2016-01-22 11:58 ` Cédric Le Goater 2016-01-22 13:04 ` Corey Minyard 1 sibling, 1 reply; 25+ messages in thread From: Greg Kurz @ 2016-01-22 11:24 UTC (permalink / raw) To: Cédric Le Goater Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On Thu, 21 Jan 2016 18:18:53 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > > Changes since v1: > - added ACPI to command names. > > hw/ipmi/ipmi_bmc_sim.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index e882af3f1b40..53c75cb21c1a 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -25,6 +25,7 @@ > #include <stdio.h> > #include <string.h> > #include <stdint.h> > +#include "sysemu/sysemu.h" > #include "qemu/timer.h" > #include "hw/ipmi/ipmi.h" > #include "qemu/error-report.h" > @@ -51,6 +52,9 @@ > #define IPMI_CMD_GET_DEVICE_ID 0x01 > #define IPMI_CMD_COLD_RESET 0x02 > #define IPMI_CMD_WARM_RESET 0x03 > +#define IPMI_CMD_SET_ACPI_POWER_STATE 0x06 > +#define IPMI_CMD_GET_ACPI_POWER_STATE 0x07 > +#define IPMI_CMD_GET_DEVICE_GUID 0x08 > #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 > #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 > #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 > @@ -200,6 +204,9 @@ struct IPMIBmcSim { > > uint8_t restart_cause; > > + uint8_t acpi_power_state[2]; > + uint8_t uuid[16]; > + > IPMISel sel; > IPMISdr sdr; > IPMISensor sensors[MAX_SENSORS]; > @@ -828,6 +835,36 @@ static void warm_reset(IPMIBmcSim *ibs, > k->reset(s, false); > } > } > +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) > +{ > + IPMI_CHECK_CMD_LEN(4); > + 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) > +{ > + IPMI_ADD_RSP_DATA(ibs->acpi_power_state[0]); > + IPMI_ADD_RSP_DATA(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) > +{ > + unsigned int i; > + > + for (i = 0; i < 16; i++) { > + IPMI_ADD_RSP_DATA(ibs->uuid[i]); > + } > +} > > static void set_bmc_global_enables(IPMIBmcSim *ibs, > uint8_t *cmd, unsigned int cmd_len, > @@ -1609,6 +1646,9 @@ 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, > @@ -1734,6 +1774,15 @@ static void ipmi_sim_init(Object *obj) > i += len; > } > > + ibs->acpi_power_state[0] = 0; > + ibs->acpi_power_state[1] = 0; > + > + if (qemu_uuid_set) { > + memcpy(&ibs->uuid, qemu_uuid, 16); > + } else { > + memset(&ibs->uuid, 0, 16); > + } > + Sorry if this is a dumb question: why does the VM's UUID gets copied here ? > ipmi_init_sensors_from_sdrs(ibs); > register_cmds(ibs); > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands 2016-01-22 11:24 ` Greg Kurz @ 2016-01-22 11:58 ` Cédric Le Goater 0 siblings, 0 replies; 25+ messages in thread From: Cédric Le Goater @ 2016-01-22 11:58 UTC (permalink / raw) To: Greg Kurz; +Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On 01/22/2016 12:24 PM, Greg Kurz wrote: > On Thu, 21 Jan 2016 18:18:53 +0100 > Cédric Le Goater <clg@fr.ibm.com> wrote: > >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >> --- >> >> Changes since v1: >> - added ACPI to command names. >> >> hw/ipmi/ipmi_bmc_sim.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >> index e882af3f1b40..53c75cb21c1a 100644 >> --- a/hw/ipmi/ipmi_bmc_sim.c >> +++ b/hw/ipmi/ipmi_bmc_sim.c >> @@ -25,6 +25,7 @@ >> #include <stdio.h> >> #include <string.h> >> #include <stdint.h> >> +#include "sysemu/sysemu.h" >> #include "qemu/timer.h" >> #include "hw/ipmi/ipmi.h" >> #include "qemu/error-report.h" >> @@ -51,6 +52,9 @@ >> #define IPMI_CMD_GET_DEVICE_ID 0x01 >> #define IPMI_CMD_COLD_RESET 0x02 >> #define IPMI_CMD_WARM_RESET 0x03 >> +#define IPMI_CMD_SET_ACPI_POWER_STATE 0x06 >> +#define IPMI_CMD_GET_ACPI_POWER_STATE 0x07 >> +#define IPMI_CMD_GET_DEVICE_GUID 0x08 >> #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 >> #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 >> #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 >> @@ -200,6 +204,9 @@ struct IPMIBmcSim { >> >> uint8_t restart_cause; >> >> + uint8_t acpi_power_state[2]; >> + uint8_t uuid[16]; >> + >> IPMISel sel; >> IPMISdr sdr; >> IPMISensor sensors[MAX_SENSORS]; >> @@ -828,6 +835,36 @@ static void warm_reset(IPMIBmcSim *ibs, >> k->reset(s, false); >> } >> } >> +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) >> +{ >> + IPMI_CHECK_CMD_LEN(4); >> + 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) >> +{ >> + IPMI_ADD_RSP_DATA(ibs->acpi_power_state[0]); >> + IPMI_ADD_RSP_DATA(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) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < 16; i++) { >> + IPMI_ADD_RSP_DATA(ibs->uuid[i]); >> + } >> +} >> >> static void set_bmc_global_enables(IPMIBmcSim *ibs, >> uint8_t *cmd, unsigned int cmd_len, >> @@ -1609,6 +1646,9 @@ 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, >> @@ -1734,6 +1774,15 @@ static void ipmi_sim_init(Object *obj) >> i += len; >> } >> >> + ibs->acpi_power_state[0] = 0; >> + ibs->acpi_power_state[1] = 0; >> + >> + if (qemu_uuid_set) { >> + memcpy(&ibs->uuid, qemu_uuid, 16); >> + } else { >> + memset(&ibs->uuid, 0, 16); >> + } >> + > > Sorry if this is a dumb question: why does the VM's UUID gets copied here ? >From the specs (20.8 Get Device GUID Command), the command needs to return a GUID (Globally Unique ID), or UUID, that should never change over the lifetime of the device. qemu_uuid looked like a good candidate to start with but we could use a property also if needed. Thanks, C. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands Cédric Le Goater 2016-01-22 11:24 ` Greg Kurz @ 2016-01-22 13:04 ` Corey Minyard 1 sibling, 0 replies; 25+ messages in thread From: Corey Minyard @ 2016-01-22 13:04 UTC (permalink / raw) To: Cédric Le Goater, Corey Minyard Cc: Michael S. Tsirkin, qemu-devel, Marcel Apfelbaum, Greg Kurz On 01/21/2016 11:18 AM, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > > Changes since v1: > - added ACPI to command names. Thanks. Acked-by: Corey Minyard <cminyard@mvista.com> > > hw/ipmi/ipmi_bmc_sim.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index e882af3f1b40..53c75cb21c1a 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -25,6 +25,7 @@ > #include <stdio.h> > #include <string.h> > #include <stdint.h> > +#include "sysemu/sysemu.h" > #include "qemu/timer.h" > #include "hw/ipmi/ipmi.h" > #include "qemu/error-report.h" > @@ -51,6 +52,9 @@ > #define IPMI_CMD_GET_DEVICE_ID 0x01 > #define IPMI_CMD_COLD_RESET 0x02 > #define IPMI_CMD_WARM_RESET 0x03 > +#define IPMI_CMD_SET_ACPI_POWER_STATE 0x06 > +#define IPMI_CMD_GET_ACPI_POWER_STATE 0x07 > +#define IPMI_CMD_GET_DEVICE_GUID 0x08 > #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 > #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 > #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 > @@ -200,6 +204,9 @@ struct IPMIBmcSim { > > uint8_t restart_cause; > > + uint8_t acpi_power_state[2]; > + uint8_t uuid[16]; > + > IPMISel sel; > IPMISdr sdr; > IPMISensor sensors[MAX_SENSORS]; > @@ -828,6 +835,36 @@ static void warm_reset(IPMIBmcSim *ibs, > k->reset(s, false); > } > } > +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) > +{ > + IPMI_CHECK_CMD_LEN(4); > + 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) > +{ > + IPMI_ADD_RSP_DATA(ibs->acpi_power_state[0]); > + IPMI_ADD_RSP_DATA(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) > +{ > + unsigned int i; > + > + for (i = 0; i < 16; i++) { > + IPMI_ADD_RSP_DATA(ibs->uuid[i]); > + } > +} > > static void set_bmc_global_enables(IPMIBmcSim *ibs, > uint8_t *cmd, unsigned int cmd_len, > @@ -1609,6 +1646,9 @@ 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, > @@ -1734,6 +1774,15 @@ static void ipmi_sim_init(Object *obj) > i += len; > } > > + ibs->acpi_power_state[0] = 0; > + ibs->acpi_power_state[1] = 0; > + > + if (qemu_uuid_set) { > + memcpy(&ibs->uuid, qemu_uuid, 16); > + } else { > + memset(&ibs->uuid, 0, 16); > + } > + > ipmi_init_sensors_from_sdrs(ibs); > register_cmds(ibs); > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 9/9] ipmi: add SET_SENSOR_READING command (tentative try) 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater ` (7 preceding siblings ...) 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands Cédric Le Goater @ 2016-01-21 17:18 ` Cédric Le Goater 2016-01-22 11:28 ` Greg Kurz 8 siblings, 1 reply; 25+ messages in thread From: Cédric Le Goater @ 2016-01-21 17:18 UTC (permalink / raw) To: Corey Minyard Cc: Cédric Le Goater, Greg Kurz, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin SET_SENSOR_READING is a complex IPMI command (IPMI spec : "35.17 Set Sensor Reading And Event Status Command"). Here is a very minimum framework fitting the Open PowerNV platform needs. This command is used on this platform to set the "System Firmware Progress" sensor and the "Boot Count" sensor. Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> Acked-by: Corey Minyard <cminyard@mvista.com> --- hw/ipmi/ipmi_bmc_sim.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index 53c75cb21c1a..0aa7e67ae217 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -46,6 +46,7 @@ #define IPMI_CMD_GET_SENSOR_READING 0x2d #define IPMI_CMD_SET_SENSOR_TYPE 0x2e #define IPMI_CMD_GET_SENSOR_TYPE 0x2f +#define IPMI_CMD_SET_SENSOR_READING 0x30 /* #define IPMI_NETFN_APP 0x06 In ipmi.h */ @@ -1616,6 +1617,139 @@ static void get_sensor_type(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA(sens->evt_reading_type_code); } +static void set_sensor_reading(IPMIBmcSim *ibs, + uint8_t *cmd, unsigned int cmd_len, + uint8_t *rsp, unsigned int *rsp_len, + unsigned int max_rsp_len) +{ + IPMISensor *sens; + uint8_t evd1; + uint8_t evd2; + uint8_t evd3; + + 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; + return; + } + + sens = ibs->sensors + cmd[2]; + + /* Sensor Reading operation */ + switch ((cmd[3]) & 0x3) { + case 0: /* Do not change */ + break; + case 1: /* write given value to sensor reading byte */ + sens->reading = cmd[4]; + break; + case 2: + case 3: + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; + return; + } + + /* Deassertion bits operation */ + switch ((cmd[3] >> 2) & 0x3) { + case 0: /* Do not change */ + break; + case 1: /* write given value */ + if (cmd_len > 7) { + sens->deassert_states = cmd[7]; + } + if (cmd_len > 8) { + sens->deassert_states = cmd[8] << 8; + } + + case 2: /* mask on */ + if (cmd_len > 7) { + sens->deassert_states |= cmd[7]; + } + if (cmd_len > 8) { + sens->deassert_states |= cmd[8] << 8; + } + break; + case 3: /* mask off */ + if (cmd_len > 7) { + sens->deassert_states &= cmd[7]; + } + if (cmd_len > 8) { + sens->deassert_states &= (cmd[8] << 8); + } + break; + } + + /* Assertion bits operation */ + switch ((cmd[3] >> 4) & 0x3) { + case 0: /* Do not change */ + break; + case 1: /* write given value */ + if (cmd_len > 5) { + sens->assert_states = cmd[5]; + } + if (cmd_len > 6) { + sens->assert_states = cmd[6] << 8; + } + + case 2: /* mask on */ + if (cmd_len > 5) { + sens->assert_states |= cmd[5]; + } + if (cmd_len > 6) { + sens->assert_states |= cmd[6] << 8; + } + break; + case 3: /* mask off */ + if (cmd_len > 5) { + sens->assert_states &= cmd[5]; + } + if (cmd_len > 6) { + sens->assert_states &= (cmd[6] << 8); + } + break; + } + + evd1 = evd2 = evd3 = 0x0; + if (cmd_len > 9) { + evd1 = cmd[9]; + } + if (cmd_len > 10) { + evd2 = cmd[10]; + } + if (cmd_len > 11) { + evd3 = cmd[11]; + } + + /* Event Data Bytes operation */ + switch ((cmd[3] >> 6) & 0x3) { + case 0: /* Do not use the event data in message */ + evd1 = evd2 = evd3 = 0x0; + break; + case 1: /* Write given values to event data bytes excluding bits + * [3:0] Event Data 1. */ + evd1 &= 0xf0; + break; + case 2: /* Write given values to event data bytes including bits + * [3:0] Event Data 1. */ + break; + case 3: + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; + return; + } + + if (IPMI_SENSOR_IS_DISCRETE(sens)) { + unsigned int bit = evd1 & 0xf; + uint16_t mask = (1 << bit); + + if (sens->assert_states & mask & sens->assert_enable) { + gen_event(ibs, cmd[2], 0, evd1, evd2, evd3); + } + + if (sens->deassert_states & mask & sens->deassert_enable) { + gen_event(ibs, cmd[2], 1, evd1, evd2, evd3); + } + } +} static const IPMICmdHandler chassis_cmds[] = { [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities, @@ -1636,6 +1770,7 @@ static const IPMICmdHandler sensor_event_cmds[] = { [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_READING] = set_sensor_reading }; static const IPMINetfn sensor_event_netfn = { .cmd_nums = ARRAY_SIZE(sensor_event_cmds), -- 2.1.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 9/9] ipmi: add SET_SENSOR_READING command (tentative try) 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 9/9] ipmi: add SET_SENSOR_READING command (tentative try) Cédric Le Goater @ 2016-01-22 11:28 ` Greg Kurz 0 siblings, 0 replies; 25+ messages in thread From: Greg Kurz @ 2016-01-22 11:28 UTC (permalink / raw) To: Cédric Le Goater Cc: Corey Minyard, qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin On Thu, 21 Jan 2016 18:18:54 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > SET_SENSOR_READING is a complex IPMI command (IPMI spec : "35.17 Set > Sensor Reading And Event Status Command"). Here is a very minimum > framework fitting the Open PowerNV platform needs. This command is > used on this platform to set the "System Firmware Progress" sensor and > the "Boot Count" sensor. > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > Acked-by: Corey Minyard <cminyard@mvista.com> > --- Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com> Just one "parenthesitis" attack... > hw/ipmi/ipmi_bmc_sim.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 135 insertions(+) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 53c75cb21c1a..0aa7e67ae217 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -46,6 +46,7 @@ > #define IPMI_CMD_GET_SENSOR_READING 0x2d > #define IPMI_CMD_SET_SENSOR_TYPE 0x2e > #define IPMI_CMD_GET_SENSOR_TYPE 0x2f > +#define IPMI_CMD_SET_SENSOR_READING 0x30 > > /* #define IPMI_NETFN_APP 0x06 In ipmi.h */ > > @@ -1616,6 +1617,139 @@ static void get_sensor_type(IPMIBmcSim *ibs, > IPMI_ADD_RSP_DATA(sens->evt_reading_type_code); > } > > +static void set_sensor_reading(IPMIBmcSim *ibs, > + uint8_t *cmd, unsigned int cmd_len, > + uint8_t *rsp, unsigned int *rsp_len, > + unsigned int max_rsp_len) > +{ > + IPMISensor *sens; > + uint8_t evd1; > + uint8_t evd2; > + uint8_t evd3; > + > + IPMI_CHECK_CMD_LEN(5); > + if ((cmd[2] > MAX_SENSORS) || Here ! :) > + !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > + rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; > + return; > + } > + > + sens = ibs->sensors + cmd[2]; > + > + /* Sensor Reading operation */ > + switch ((cmd[3]) & 0x3) { > + case 0: /* Do not change */ > + break; > + case 1: /* write given value to sensor reading byte */ > + sens->reading = cmd[4]; > + break; > + case 2: > + case 3: > + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > + return; > + } > + > + /* Deassertion bits operation */ > + switch ((cmd[3] >> 2) & 0x3) { > + case 0: /* Do not change */ > + break; > + case 1: /* write given value */ > + if (cmd_len > 7) { > + sens->deassert_states = cmd[7]; > + } > + if (cmd_len > 8) { > + sens->deassert_states = cmd[8] << 8; > + } > + > + case 2: /* mask on */ > + if (cmd_len > 7) { > + sens->deassert_states |= cmd[7]; > + } > + if (cmd_len > 8) { > + sens->deassert_states |= cmd[8] << 8; > + } > + break; > + case 3: /* mask off */ > + if (cmd_len > 7) { > + sens->deassert_states &= cmd[7]; > + } > + if (cmd_len > 8) { > + sens->deassert_states &= (cmd[8] << 8); > + } > + break; > + } > + > + /* Assertion bits operation */ > + switch ((cmd[3] >> 4) & 0x3) { > + case 0: /* Do not change */ > + break; > + case 1: /* write given value */ > + if (cmd_len > 5) { > + sens->assert_states = cmd[5]; > + } > + if (cmd_len > 6) { > + sens->assert_states = cmd[6] << 8; > + } > + > + case 2: /* mask on */ > + if (cmd_len > 5) { > + sens->assert_states |= cmd[5]; > + } > + if (cmd_len > 6) { > + sens->assert_states |= cmd[6] << 8; > + } > + break; > + case 3: /* mask off */ > + if (cmd_len > 5) { > + sens->assert_states &= cmd[5]; > + } > + if (cmd_len > 6) { > + sens->assert_states &= (cmd[6] << 8); > + } > + break; > + } > + > + evd1 = evd2 = evd3 = 0x0; > + if (cmd_len > 9) { > + evd1 = cmd[9]; > + } > + if (cmd_len > 10) { > + evd2 = cmd[10]; > + } > + if (cmd_len > 11) { > + evd3 = cmd[11]; > + } > + > + /* Event Data Bytes operation */ > + switch ((cmd[3] >> 6) & 0x3) { > + case 0: /* Do not use the event data in message */ > + evd1 = evd2 = evd3 = 0x0; > + break; > + case 1: /* Write given values to event data bytes excluding bits > + * [3:0] Event Data 1. */ > + evd1 &= 0xf0; > + break; > + case 2: /* Write given values to event data bytes including bits > + * [3:0] Event Data 1. */ > + break; > + case 3: > + rsp[2] = IPMI_CC_INVALID_DATA_FIELD; > + return; > + } > + > + if (IPMI_SENSOR_IS_DISCRETE(sens)) { > + unsigned int bit = evd1 & 0xf; > + uint16_t mask = (1 << bit); > + > + if (sens->assert_states & mask & sens->assert_enable) { > + gen_event(ibs, cmd[2], 0, evd1, evd2, evd3); > + } > + > + if (sens->deassert_states & mask & sens->deassert_enable) { > + gen_event(ibs, cmd[2], 1, evd1, evd2, evd3); > + } > + } > +} > > static const IPMICmdHandler chassis_cmds[] = { > [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities, > @@ -1636,6 +1770,7 @@ static const IPMICmdHandler sensor_event_cmds[] = { > [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_READING] = set_sensor_reading > }; > static const IPMINetfn sensor_event_netfn = { > .cmd_nums = ARRAY_SIZE(sensor_event_cmds), ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2016-01-22 13:05 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-21 17:18 [Qemu-devel] [PATCH v2 0/9] ipmi: a couple of enhancements to the BMC simulator Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 1/9] ppc: add IPMI support Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement Cédric Le Goater 2016-01-22 5:49 ` Marcel Apfelbaum 2016-01-22 6:28 ` Greg Kurz 2016-01-22 12:56 ` Corey Minyard 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 3/9] ipmi: replace *_MAXCMD defines Cédric Le Goater 2016-01-22 8:05 ` Greg Kurz 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 4/9] ipmi: introduce a struct ipmi_sdr_compact Cédric Le Goater 2016-01-22 10:49 ` Greg Kurz 2016-01-22 11:10 ` Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 5/9] ipmi: fix SDR length value Cédric Le Goater 2016-01-22 10:56 ` Greg Kurz 2016-01-22 11:15 ` Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 6/9] ipmi: add get and set SENSOR_TYPE commands Cédric Le Goater 2016-01-22 11:07 ` Greg Kurz 2016-01-22 11:13 ` Cédric Le Goater 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 7/9] ipmi: add GET_SYS_RESTART_CAUSE chassis command Cédric Le Goater 2016-01-22 11:09 ` Greg Kurz 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 8/9] ipmi: add ACPI power and GUID commands Cédric Le Goater 2016-01-22 11:24 ` Greg Kurz 2016-01-22 11:58 ` Cédric Le Goater 2016-01-22 13:04 ` Corey Minyard 2016-01-21 17:18 ` [Qemu-devel] [PATCH v2 9/9] ipmi: add SET_SENSOR_READING command (tentative try) Cédric Le Goater 2016-01-22 11:28 ` Greg Kurz
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.