[PATCH] ipmi: add SET_SENSOR_READING command

Cédric Le Goater posted 1 patch 4 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191118092429.16149-1-clg@kaod.org
Test asan failed
Test checkpatch failed
Test FreeBSD failed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 failed
Maintainers: Corey Minyard <minyard@acm.org>
hw/ipmi/ipmi_bmc_sim.c | 223 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 223 insertions(+)
[PATCH] ipmi: add SET_SENSOR_READING command
Posted by Cédric Le Goater 4 years, 4 months ago
SET_SENSOR_READING is a complex IPMI command (see IPMI spec 35.17)
which enables the host software to set the reading value and the event
status of sensors supporting it.

Below is a proposal for all the operations (reading, assert, deassert,
event data) with the following limitations :

 - No event are generated for threshold-based sensors.
 - The case in which the BMC needs to generate its own events is not
   supported.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 223 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 223 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 6670cf039d1a..b6cf7b123f74 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -49,6 +49,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 */
 
@@ -1747,6 +1748,227 @@ static void get_sensor_type(IPMIBmcSim *ibs,
     rsp_buffer_push(rsp, sens->evt_reading_type_code);
 }
 
+/*
+ * bytes   parameter
+ *    1    sensor number
+ *    2    operation (see below for bits meaning)
+ *    3    sensor reading
+ *  4:5    assertion states (optional)
+ *  6:7    deassertion states (optional)
+ *  8:10   event data 1,2,3 (optional)
+ */
+static void set_sensor_reading(IPMIBmcSim *ibs,
+                               uint8_t *cmd, unsigned int cmd_len,
+                               RspBuffer *rsp)
+{
+    IPMISensor *sens;
+    uint8_t evd1 = 0;
+    uint8_t evd2 = 0;
+    uint8_t evd3 = 0;
+    uint8_t new_reading = 0;
+    uint16_t new_assert_states = 0;
+    uint16_t new_deassert_states = 0;
+    bool change_reading = false;
+    bool change_assert = false;
+    bool change_deassert = false;
+    enum {
+        SENSOR_GEN_EVENT_NONE,
+        SENSOR_GEN_EVENT_DATA,
+        SENSOR_GEN_EVENT_BMC,
+    } do_gen_event = SENSOR_GEN_EVENT_NONE;
+
+    if ((cmd[2] >= MAX_SENSORS) ||
+            !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];
+
+    /* [1:0] Sensor Reading operation */
+    switch ((cmd[3]) & 0x3) {
+    case 0: /* Do not change */
+        break;
+    case 1: /* write given value to sensor reading byte */
+        new_reading = cmd[4];
+        if (sens->reading != new_reading) {
+            change_reading = true;
+        }
+        break;
+    case 2:
+    case 3:
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }
+
+    /* [3:2] Deassertion bits operation */
+    switch ((cmd[3] >> 2) & 0x3) {
+    case 0: /* Do not change */
+        break;
+    case 1: /* write given value */
+        if (cmd_len > 7) {
+            new_deassert_states = cmd[7];
+            change_deassert = true;
+        }
+        if (cmd_len > 8) {
+            new_deassert_states |= (cmd[8] << 8);
+        }
+        break;
+
+    case 2: /* mask on */
+        if (cmd_len > 7) {
+            new_deassert_states = (sens->deassert_states | cmd[7]);
+            change_deassert = true;
+        }
+        if (cmd_len > 8) {
+            new_deassert_states |= (sens->deassert_states | (cmd[8] << 8));
+        }
+        break;
+
+    case 3: /* mask off */
+        if (cmd_len > 7) {
+            new_deassert_states = (sens->deassert_states & cmd[7]);
+            change_deassert = true;
+        }
+        if (cmd_len > 8) {
+            new_deassert_states |= (sens->deassert_states & (cmd[8] << 8));
+        }
+        break;
+    }
+
+    if (change_deassert && (new_deassert_states == sens->deassert_states)) {
+        change_deassert = false;
+    }
+
+    /* [5:4] Assertion bits operation */
+    switch ((cmd[3] >> 4) & 0x3) {
+    case 0: /* Do not change */
+        break;
+    case 1: /* write given value */
+        if (cmd_len > 5) {
+            new_assert_states = cmd[5];
+            change_assert = true;
+        }
+        if (cmd_len > 6) {
+            new_assert_states |= (cmd[6] << 8);
+        }
+        break;
+
+    case 2: /* mask on */
+        if (cmd_len > 5) {
+            new_assert_states = (sens->assert_states | cmd[5]);
+            change_assert = true;
+        }
+        if (cmd_len > 6) {
+            new_assert_states |= (sens->assert_states | (cmd[6] << 8));
+        }
+        break;
+
+    case 3: /* mask off */
+        if (cmd_len > 5) {
+            new_assert_states = (sens->assert_states & cmd[5]);
+            change_assert = true;
+        }
+        if (cmd_len > 6) {
+            new_assert_states |= (sens->assert_states & (cmd[6] << 8));
+        }
+        break;
+    }
+
+    if (change_assert && (new_assert_states == sens->assert_states)) {
+        change_assert = false;
+    }
+
+    if (cmd_len > 9) {
+        evd1 = cmd[9];
+    }
+    if (cmd_len > 10) {
+        evd2 = cmd[10];
+    }
+    if (cmd_len > 11) {
+        evd3 = cmd[11];
+    }
+
+    /* [7:6] Event Data Bytes operation */
+    switch ((cmd[3] >> 6) & 0x3) {
+    case 0: /*
+             * Don’t use Event Data bytes from this command. BMC will
+             * generate it's own Event Data bytes based on its sensor
+             * implementation.
+             */
+        evd1 = evd2 = evd3 = 0x0;
+        do_gen_event = SENSOR_GEN_EVENT_BMC;
+        break;
+    case 1: /*
+             * Write given values to event data bytes including bits
+             * [3:0] Event Data 1.
+             */
+        do_gen_event = SENSOR_GEN_EVENT_DATA;
+        break;
+    case 2: /*
+             * Write given values to event data bytes excluding bits
+             * [3:0] Event Data 1.
+             */
+        evd1 &= 0xf0;
+        do_gen_event = SENSOR_GEN_EVENT_DATA;
+        break;
+    case 3:
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }
+
+    /*
+     * Event Data Bytes operation and parameter are inconsistent. The
+     * Specs are not clear on that topic but generating an error seems
+     * correct.
+     */
+    if (do_gen_event == SENSOR_GEN_EVENT_DATA && cmd_len < 10) {
+        rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
+        return;
+    }
+
+    /* commit values */
+    if (change_reading) {
+        sens->reading = new_reading;
+    }
+
+    if (change_assert) {
+        sens->assert_states = new_assert_states;
+    }
+
+    if (change_deassert) {
+        sens->deassert_states = new_deassert_states;
+    }
+
+    /* TODO: handle threshold sensor */
+    if (!IPMI_SENSOR_IS_DISCRETE(sens)) {
+        return;
+    }
+
+    switch (do_gen_event) {
+    case SENSOR_GEN_EVENT_DATA: {
+        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);
+        }
+    }
+        break;
+    case SENSOR_GEN_EVENT_BMC:
+        /*
+         * TODO: generate event and event data bytes depending on the
+         * sensor
+         */
+        break;
+    case SENSOR_GEN_EVENT_NONE:
+        break;
+    }
+}
 
 static const IPMICmdHandler chassis_cmds[] = {
     [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = { chassis_capabilities },
@@ -1768,6 +1990,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),
-- 
2.21.0


Re: [PATCH] ipmi: add SET_SENSOR_READING command
Posted by Corey Minyard 4 years, 4 months ago
On Mon, Nov 18, 2019 at 10:24:29AM +0100, Cédric Le Goater wrote:
> SET_SENSOR_READING is a complex IPMI command (see IPMI spec 35.17)
> which enables the host software to set the reading value and the event
> status of sensors supporting it.
> 
> Below is a proposal for all the operations (reading, assert, deassert,
> event data) with the following limitations :
> 
>  - No event are generated for threshold-based sensors.
>  - The case in which the BMC needs to generate its own events is not
>    supported.

Ok, I've included this in my tree.  I made one small change mentioned
below.  Beyond that, I think you could make this function shorter, but I
think that would actually make it harder to understand.  Breaking it
into multiple functions doesn't make sense to me, either.

If you are including this in the ppc tree:

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

with the change below and I can remove it from mine.

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Corey Minyard <cminyard@mvista.com>
> ---
> +
> +    switch (do_gen_event) {
> +    case SENSOR_GEN_EVENT_DATA: {
> +        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);
> +        }
> +    }
> +        break;

I moved this break statement above the brace before it to keep the
indention consistent.  It just screwed with my brain too much :).

I looked and there is nothing in the coding style about this, and I
found this done in three different ways:

  case x: {  /* in vl.c */
      ....
      break;
  }
  case y: /* in thunk.c */
      {
           ....
      }
      break;
  case z: /* In vl.c */
  {
      ....
      break;
  }

Oddly enough, I didn't find anything about this in the Linux coding
style document, either (I was curious).  One could argue, I suppose,
that according to the "Block structure" section in the qemu style and
the similar section in the Linux style that the first is correct,
but then case statements violate the "Every indented statement is
braced" statement in the qemu style.  This has always bugged me in
C, sorry for the diatribe on this.

-corey

> +    case SENSOR_GEN_EVENT_BMC:

Re: [PATCH] ipmi: add SET_SENSOR_READING command
Posted by Cédric Le Goater 4 years, 4 months ago
On 22/11/2019 15:28, Corey Minyard wrote:
> On Mon, Nov 18, 2019 at 10:24:29AM +0100, Cédric Le Goater wrote:
>> SET_SENSOR_READING is a complex IPMI command (see IPMI spec 35.17)
>> which enables the host software to set the reading value and the event
>> status of sensors supporting it.
>>
>> Below is a proposal for all the operations (reading, assert, deassert,
>> event data) with the following limitations :
>>
>>  - No event are generated for threshold-based sensors.
>>  - The case in which the BMC needs to generate its own events is not
>>    supported.
> 
> Ok, I've included this in my tree.  I made one small change mentioned
> below.  Beyond that, I think you could make this function shorter, but I
> think that would actually make it harder to understand.  Breaking it
> into multiple functions doesn't make sense to me, either.
> 
> If you are including this in the ppc tree:
> 
> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
> with the change below and I can remove it from mine.

I don't think there is a strong need to have it in the PPC tree. It's 
a stand alone function adding an extra IPMI command.


>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Corey Minyard <cminyard@mvista.com>
>> ---
>> +
>> +    switch (do_gen_event) {
>> +    case SENSOR_GEN_EVENT_DATA: {
>> +        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);
>> +        }
>> +    }
>> +        break;
> 
> I moved this break statement above the brace before it to keep the
> indention consistent.  It just screwed with my brain too much :).
>
> I looked and there is nothing in the coding style about this, and I
> found this done in three different ways:
> 
>   case x: {  /* in vl.c */
>       ....
>       break;
>   }
>   case y: /* in thunk.c */
>       {
>            ....
>       }
>       break;
>   case z: /* In vl.c */
>   {
>       ....
>       break;
>   }
> 
> Oddly enough, I didn't find anything about this in the Linux coding
> style document, either (I was curious).  One could argue, I suppose,
> that according to the "Block structure" section in the qemu style and
> the similar section in the Linux style that the first is correct,
> but then case statements violate the "Every indented statement is
> braced" statement in the qemu style.  This has always bugged me in
> C, sorry for the diatribe on this.

Thanks,

C. 

> 
> -corey
> 
>> +    case SENSOR_GEN_EVENT_BMC: