qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 10/21] ipmi: add SET_SENSOR_READING command


From: Corey Minyard
Subject: Re: [Qemu-devel] [PATCH 10/21] ipmi: add SET_SENSOR_READING command
Date: Wed, 5 Apr 2017 09:41:28 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

On 04/05/2017 07:41 AM, Cédric Le Goater 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 <address@hidden>
---
  hw/ipmi/ipmi_bmc_sim.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 133 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 155561d06614..26036b20d4df 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -45,6 +45,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 */ @@ -1739,6 +1740,137 @@ static void get_sensor_type(IPMIBmcSim *ibs,
      rsp_buffer_push(rsp, sens->evt_reading_type_code);
  }
+static void set_sensor_reading(IPMIBmcSim *ibs,
+                               uint8_t *cmd, unsigned int cmd_len,
+                               RspBuffer *rsp)
+{
+    IPMISensor *sens;
+    uint8_t evd1;
+    uint8_t evd2;
+    uint8_t evd3;
+
+    if ((cmd[2] > MAX_SENSORS) ||

Should be ">=" here, I believe.


+            !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
+        rsp_buffer_set_error(rsp, 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_buffer_set_error(rsp, 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_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }

I think the above section (from the initialization of evd<n>) should be moved to right before the sensor reading handling. Otherwise, on an error you will set the sensor
reading and assertion bits but will return an error from the command and not
send an event. Actually, it may be better to create local copies of the reading and the assertion bits and commit everything together at the end, for reasons
I will talk about later, and because it's more clear.

There is some vagueness in the spec about how to handle the event data bytes if
they are not present in the command but the command says to use them.  I'm
ok with this implementation, but I'm wondering if returning an error wouldn't be
better in this particular case.

The spec is also silent about how to handle setting threshold bits if they are not
present but the value exceeds the thresholds.  I'm not sure what to do here.
My gut says that the event should be generated, but that add a lot of complexity. We should probably at least add a comment about this if we don't do it, since
there is no handling of threshold sensors for event generation below.

+
+    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);
+        }

You should only generate the events if the values change. So you need to save the original values so you can compare. Also, if the command requests that the
BMC generate it's own event, you would need to scan the new values compared
with the old values and send events for each bit that needs it. Again, more
complexity, but we should at least comment that we are not doing something
that may be needed later.

-corey

+    }
+}
static const IPMICmdHandler chassis_cmds[] = {
      [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = { chassis_capabilities },
@@ -1759,6 +1891,7 @@ static const IPMICmdHandler sensor_event_cmds[] = {
      [IPMI_CMD_GET_SENSOR_READING] = { get_sensor_reading, 3 },
      [IPMI_CMD_SET_SENSOR_TYPE] = { set_sensor_type, 5 },
      [IPMI_CMD_GET_SENSOR_TYPE] = { get_sensor_type, 3 },
+    [IPMI_CMD_SET_SENSOR_READING] = { set_sensor_reading, 5 },
  };
  static const IPMINetfn sensor_event_netfn = {
      .cmd_nums = ARRAY_SIZE(sensor_event_cmds),





reply via email to

[Prev in Thread] Current Thread [Next in Thread]