[Qemu-devel] [PATCH v2 0/3] ipmi: Fix SEL get/set time commands

[Qemu-devel] [PATCH v2 0/3] ipmi: Fix SEL get/set time commands

[minyard]

Resend, with the style issues and the problem that Cédric pointed
out.
Thanks Cédric, for the review.

Changes from v1:

 * Get the data from the proper offset in the command array.
 * Use {} on all if statements.

[Qemu-devel] [PATCH v2 1/3] ipmi: Fix SEL get/set time commands

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The minimum message size was on the wrong commands, for getting
the time it's zero and for setting the time it's 6.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 277c28c..cc068f2 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1802,8 +1802,8 @@ static const IPMICmdHandler storage_cmds[] = {
     [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
     [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
     [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
-    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
-    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
+    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time },
+    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time, 6 },
 };
 
 static const IPMINetfn storage_netfn = {
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/3] ipmi: Don't set the timestamp on add events that don't have it

[minyard]

From: Corey Minyard <cminyard@mvista.com>

According to the spec, from section "32.3 OEM SEL Record - Type
E0h-FFh", event types from 0x0e to 0xff do not have a timestamp.
So don't set it when adding those types.  This required putting
the timestamp in a temporary buffer, since it's still required
to set the last addition time.

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

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index cc068f2..a0bbfd5 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -443,16 +443,21 @@ static void sel_inc_reservation(IPMISel *sel)
 /* Returns 1 if the SEL is full and can't hold the event. */
 static int sel_add_event(IPMIBmcSim *ibs, uint8_t *event)
 {
+    uint8_t ts[4];
+
     event[0] = 0xff;
     event[1] = 0xff;
-    set_timestamp(ibs, event + 3);
+    set_timestamp(ibs, ts);
+    if (event[2] < 0xe0) { /* Don't set timestamps for type 0xe0-0xff. */
+        memcpy(event + 3, ts, 4);
+    }
     if (ibs->sel.next_free == MAX_SEL_SIZE) {
         ibs->sel.overflow = 1;
         return 1;
     }
     event[0] = ibs->sel.next_free & 0xff;
     event[1] = (ibs->sel.next_free >> 8) & 0xff;
-    memcpy(ibs->sel.last_addition, event + 3, 4);
+    memcpy(ibs->sel.last_addition, ts, 4);
     memcpy(ibs->sel.sel[ibs->sel.next_free], event, 16);
     ibs->sel.next_free++;
     sel_inc_reservation(&ibs->sel);
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This lets an event be added to the SEL as if a sensor had generated
it.  The OpenIPMI driver uses it for storing panic event information.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index a0bbfd5..e84d710 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -38,6 +38,7 @@
 
 #define IPMI_NETFN_SENSOR_EVENT       0x04
 
+#define IPMI_CMD_PLATFORM_EVENT_MSG       0x02
 #define IPMI_CMD_SET_SENSOR_EVT_ENABLE    0x28
 #define IPMI_CMD_GET_SENSOR_EVT_ENABLE    0x29
 #define IPMI_CMD_REARM_SENSOR_EVTS        0x2a
@@ -1581,6 +1582,28 @@ static void set_sel_time(IPMIBmcSim *ibs,
     ibs->sel.time_offset = now.tv_sec - ((long) val);
 }
 
+static void platform_event_msg(IPMIBmcSim *ibs,
+                               uint8_t *cmd, unsigned int cmd_len,
+                               RspBuffer *rsp)
+{
+    uint8_t event[16];
+
+    event[2] = 2; /* System event record */
+    event[7] = cmd[2]; /* Generator ID */
+    event[8] = 0;
+    event[9] = cmd[3]; /* EvMRev */
+    event[10] = cmd[4]; /* Sensor type */
+    event[11] = cmd[5]; /* Sensor number */
+    event[12] = cmd[6]; /* Event dir / Event type */
+    event[13] = cmd[7]; /* Event data 1 */
+    event[14] = cmd[8]; /* Event data 2 */
+    event[15] = cmd[9]; /* Event data 3 */
+
+    if (sel_add_event(ibs, event)) {
+        rsp_buffer_set_error(rsp, IPMI_CC_OUT_OF_SPACE);
+    }
+}
+
 static void set_sensor_evt_enable(IPMIBmcSim *ibs,
                                   uint8_t *cmd, unsigned int cmd_len,
                                   RspBuffer *rsp)
@@ -1757,6 +1780,7 @@ static const IPMINetfn chassis_netfn = {
 };
 
 static const IPMICmdHandler sensor_event_cmds[] = {
+    [IPMI_CMD_PLATFORM_EVENT_MSG] = { platform_event_msg, 10 },
     [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
     [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
     [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command

[Philippe Mathieu-Daudé]

Hi Corey,

On 08/20/2017 06:46 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This lets an event be added to the SEL as if a sensor had generated
> it.  The OpenIPMI driver uses it for storing panic event information.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index a0bbfd5..e84d710 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -38,6 +38,7 @@
>   
>   #define IPMI_NETFN_SENSOR_EVENT       0x04
>   
> +#define IPMI_CMD_PLATFORM_EVENT_MSG       0x02
>   #define IPMI_CMD_SET_SENSOR_EVT_ENABLE    0x28
>   #define IPMI_CMD_GET_SENSOR_EVT_ENABLE    0x29
>   #define IPMI_CMD_REARM_SENSOR_EVTS        0x2a
> @@ -1581,6 +1582,28 @@ static void set_sel_time(IPMIBmcSim *ibs,
>       ibs->sel.time_offset = now.tv_sec - ((long) val);
>   }
>   
> +static void platform_event_msg(IPMIBmcSim *ibs,
> +                               uint8_t *cmd, unsigned int cmd_len,
> +                               RspBuffer *rsp)
> +{
> +    uint8_t event[16];

Watch out, here you are using uninitialized stack,

> +
> +    event[2] = 2; /* System event record */
> +    event[7] = cmd[2]; /* Generator ID */
> +    event[8] = 0;
> +    event[9] = cmd[3]; /* EvMRev */
> +    event[10] = cmd[4]; /* Sensor type */
> +    event[11] = cmd[5]; /* Sensor number */
> +    event[12] = cmd[6]; /* Event dir / Event type */
> +    event[13] = cmd[7]; /* Event data 1 */
> +    event[14] = cmd[8]; /* Event data 2 */
> +    event[15] = cmd[9]; /* Event data 3 */
> +
> +    if (sel_add_event(ibs, event)) {

So here you have event[0,1,3-6] as crap/random.

Also it'd be more efficient to use

	memcpy(&event[9], &cmd[3], sizeof(event) - 9);

> +        rsp_buffer_set_error(rsp, IPMI_CC_OUT_OF_SPACE);
> +    }
> +}
> +
>   static void set_sensor_evt_enable(IPMIBmcSim *ibs,
>                                     uint8_t *cmd, unsigned int cmd_len,
>                                     RspBuffer *rsp)
> @@ -1757,6 +1780,7 @@ static const IPMINetfn chassis_netfn = {
>   };
>   
>   static const IPMICmdHandler sensor_event_cmds[] = {
> +    [IPMI_CMD_PLATFORM_EVENT_MSG] = { platform_event_msg, 10 },
>       [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
>       [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
>       [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
> 

Re: [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command

[Corey Minyard]

On 08/20/2017 05:24 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 08/20/2017 06:46 PM, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This lets an event be added to the SEL as if a sensor had generated
>> it.  The OpenIPMI driver uses it for storing panic event information.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index a0bbfd5..e84d710 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -38,6 +38,7 @@
>>     #define IPMI_NETFN_SENSOR_EVENT       0x04
>>   +#define IPMI_CMD_PLATFORM_EVENT_MSG       0x02
>>   #define IPMI_CMD_SET_SENSOR_EVT_ENABLE    0x28
>>   #define IPMI_CMD_GET_SENSOR_EVT_ENABLE    0x29
>>   #define IPMI_CMD_REARM_SENSOR_EVTS        0x2a
>> @@ -1581,6 +1582,28 @@ static void set_sel_time(IPMIBmcSim *ibs,
>>       ibs->sel.time_offset = now.tv_sec - ((long) val);
>>   }
>>   +static void platform_event_msg(IPMIBmcSim *ibs,
>> +                               uint8_t *cmd, unsigned int cmd_len,
>> +                               RspBuffer *rsp)
>> +{
>> +    uint8_t event[16];
>
> Watch out, here you are using uninitialized stack,
>
>> +
>> +    event[2] = 2; /* System event record */
>> +    event[7] = cmd[2]; /* Generator ID */
>> +    event[8] = 0;
>> +    event[9] = cmd[3]; /* EvMRev */
>> +    event[10] = cmd[4]; /* Sensor type */
>> +    event[11] = cmd[5]; /* Sensor number */
>> +    event[12] = cmd[6]; /* Event dir / Event type */
>> +    event[13] = cmd[7]; /* Event data 1 */
>> +    event[14] = cmd[8]; /* Event data 2 */
>> +    event[15] = cmd[9]; /* Event data 3 */
>> +
>> +    if (sel_add_event(ibs, event)) {
>
> So here you have event[0,1,3-6] as crap/random.
>

Actually, if you look at sel_add_event(), it fills in these values.

> Also it'd be more efficient to use
>
>     memcpy(&event[9], &cmd[3], sizeof(event) - 9);
>

Maybe, I'm not sure if the compiler would catch this and do something
more efficient.  I like being able to document the individual values.

-corey

>> +        rsp_buffer_set_error(rsp, IPMI_CC_OUT_OF_SPACE);
>> +    }
>> +}
>> +
>>   static void set_sensor_evt_enable(IPMIBmcSim *ibs,
>>                                     uint8_t *cmd, unsigned int cmd_len,
>>                                     RspBuffer *rsp)
>> @@ -1757,6 +1780,7 @@ static const IPMINetfn chassis_netfn = {
>>   };
>>     static const IPMICmdHandler sensor_event_cmds[] = {
>> +    [IPMI_CMD_PLATFORM_EVENT_MSG] = { platform_event_msg, 10 },
>>       [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
>>       [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
>>       [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
>>
>

Re: [Qemu-devel] [PATCH v2 1/3] ipmi: Fix SEL get/set time commands

[Cédric Le Goater]

On 08/20/2017 11:46 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The minimum message size was on the wrong commands, for getting
> the time it's zero and for setting the time it's 6.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ipmi/ipmi_bmc_sim.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 277c28c..cc068f2 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -1802,8 +1802,8 @@ static const IPMICmdHandler storage_cmds[] = {
>      [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
>      [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
>      [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
> -    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
> -    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
> +    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time },
> +    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time, 6 },
>  };
>  
>  static const IPMINetfn storage_netfn = {
> 

Re: [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command

[Cédric Le Goater]

On 08/20/2017 11:46 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This lets an event be added to the SEL as if a sensor had generated
> it.  The OpenIPMI driver uses it for storing panic event information.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

As Philippe pointed it out, we could initialize all the event
bytes to some initial value, but this is minor and this is done
later on in sel_add_event().

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ipmi/ipmi_bmc_sim.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index a0bbfd5..e84d710 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -38,6 +38,7 @@
>  
>  #define IPMI_NETFN_SENSOR_EVENT       0x04
>  
> +#define IPMI_CMD_PLATFORM_EVENT_MSG       0x02
>  #define IPMI_CMD_SET_SENSOR_EVT_ENABLE    0x28
>  #define IPMI_CMD_GET_SENSOR_EVT_ENABLE    0x29
>  #define IPMI_CMD_REARM_SENSOR_EVTS        0x2a
> @@ -1581,6 +1582,28 @@ static void set_sel_time(IPMIBmcSim *ibs,
>      ibs->sel.time_offset = now.tv_sec - ((long) val);
>  }
>  
> +static void platform_event_msg(IPMIBmcSim *ibs,
> +                               uint8_t *cmd, unsigned int cmd_len,
> +                               RspBuffer *rsp)
> +{
> +    uint8_t event[16];
> +
> +    event[2] = 2; /* System event record */
> +    event[7] = cmd[2]; /* Generator ID */
> +    event[8] = 0;
> +    event[9] = cmd[3]; /* EvMRev */
> +    event[10] = cmd[4]; /* Sensor type */
> +    event[11] = cmd[5]; /* Sensor number */
> +    event[12] = cmd[6]; /* Event dir / Event type */
> +    event[13] = cmd[7]; /* Event data 1 */
> +    event[14] = cmd[8]; /* Event data 2 */
> +    event[15] = cmd[9]; /* Event data 3 */
> +
> +    if (sel_add_event(ibs, event)) {
> +        rsp_buffer_set_error(rsp, IPMI_CC_OUT_OF_SPACE);
> +    }
> +}
> +
>  static void set_sensor_evt_enable(IPMIBmcSim *ibs,
>                                    uint8_t *cmd, unsigned int cmd_len,
>                                    RspBuffer *rsp)
> @@ -1757,6 +1780,7 @@ static const IPMINetfn chassis_netfn = {
>  };
>  
>  static const IPMICmdHandler sensor_event_cmds[] = {
> +    [IPMI_CMD_PLATFORM_EVENT_MSG] = { platform_event_msg, 10 },
>      [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
>      [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
>      [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
>