[Qemu-devel] [PATCH 0/3] ipmi: Fix some minor issues

[Qemu-devel] [PATCH 0/3] ipmi: Fix some minor issues

[minyard]

I ran into these testing some stuff in the IPMI driver.  Nothing big,
but they need to be fixed.

-corey

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

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The minimum message size was wrong for both commands, for getting
the time it's zero and for setting the time it's 4.  And the data
was being pulled from the wrong place in the set time message, it
should be the first four bytes.

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

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 277c28c..1c732aa 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1571,7 +1571,7 @@ static void set_sel_time(IPMIBmcSim *ibs,
     uint32_t val;
     struct ipmi_time now;
 
-    val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
+    val = cmd[0] | (cmd[1] << 8) | (cmd[2] << 16) | (cmd[3] << 24);
     ipmi_gettime(&now);
     ibs->sel.time_offset = now.tv_sec - ((long) val);
 }
@@ -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, 4 },
 };
 
 static const IPMINetfn storage_netfn = {
-- 
2.7.4

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

[Cédric Le Goater]

On 08/19/2017 11:40 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The minimum message size was wrong for both commands, for getting
> the time it's zero and for setting the time it's 4.  And the data
> was being pulled from the wrong place in the set time message, it
> should be the first four bytes.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/ipmi/ipmi_bmc_sim.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 277c28c..1c732aa 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -1571,7 +1571,7 @@ static void set_sel_time(IPMIBmcSim *ibs,
>      uint32_t val;
>      struct ipmi_time now;
>  
> -    val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
> +    val = cmd[0] | (cmd[1] << 8) | (cmd[2] << 16) | (cmd[3] << 24);

I am confused. I thought cmd[0] and cmd[1] were the 'netfun' and 
'cmd' bytes, data beginning at byte 2. Isn't it the case for this
command also ? 

C.


>      ipmi_gettime(&now);
>      ibs->sel.time_offset = now.tv_sec - ((long) val);
>  }
> @@ -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, 4 },
>  };
>  
>  static const IPMINetfn storage_netfn = {
> 

[Qemu-devel] [PATCH 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, 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>
---
 hw/ipmi/ipmi_bmc_sim.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 1c732aa..0f191e8 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -443,16 +443,20 @@ 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 these, per the spec. */
+        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

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

[Cédric Le Goater]

On 08/19/2017 11:40 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> According to the spec, event types from 0x0e to 0xff do not have a
> timestamp.  

>From section "32.3 OEM SEL Record - Type E0h-FFh"

> 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 | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 1c732aa..0f191e8 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -443,16 +443,20 @@ 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 these, per the spec. */
> +        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);
> 

[Qemu-devel] [PATCH 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 | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 0f191e8..09b15a8 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
@@ -1580,6 +1581,27 @@ 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[0]; /* Generator ID */
+    event[8] = 0;
+    event[9] = cmd[1]; /* EvMRev */
+    event[10] = cmd[2]; /* Sensor type */
+    event[11] = cmd[3]; /* Sensor number */
+    event[12] = cmd[4]; /* Event dir / Event type */
+    event[13] = cmd[5]; /* Event data 1 */
+    event[14] = cmd[6]; /* Event data 2 */
+    event[15] = cmd[7]; /* 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)
@@ -1756,6 +1778,7 @@ static const IPMINetfn chassis_netfn = {
 };
 
 static const IPMICmdHandler sensor_event_cmds[] = {
+    [IPMI_CMD_PLATFORM_EVENT_MSG] = { platform_event_msg, 8 },
     [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 3/3] ipmi: Add the platform event message command

[Cédric Le Goater]

On 08/19/2017 11:40 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 | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 0f191e8..09b15a8 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
> @@ -1580,6 +1581,27 @@ 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[0]; /* Generator ID */

Like for patch 1, aren't cmd[0] and cmd[1] the 'netfun' and 'cmd' 
bytes ? Otherwise, I checked the specs.

C.


> +    event[8] = 0;
> +    event[9] = cmd[1]; /* EvMRev */
> +    event[10] = cmd[2]; /* Sensor type */
> +    event[11] = cmd[3]; /* Sensor number */
> +    event[12] = cmd[4]; /* Event dir / Event type */
> +    event[13] = cmd[5]; /* Event data 1 */
> +    event[14] = cmd[6]; /* Event data 2 */
> +    event[15] = cmd[7]; /* 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)
> @@ -1756,6 +1778,7 @@ static const IPMINetfn chassis_netfn = {
>  };
>  
>  static const IPMICmdHandler sensor_event_cmds[] = {
> +    [IPMI_CMD_PLATFORM_EVENT_MSG] = { platform_event_msg, 8 },
>      [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 0/3] ipmi: Fix some minor issues

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1503178840-21512-1-git-send-email-minyard@acm.org
Subject: [Qemu-devel] [PATCH 0/3] ipmi: Fix some minor issues

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3d36816e3d ipmi: Add the platform event message command
f35f019721 ipmi: Don't set the timestamp on add events that don't have it
1e6c3d5b54 ipmi: Fix SEL get/set time commands

=== OUTPUT BEGIN ===
Checking PATCH 1/3: ipmi: Fix SEL get/set time commands...
Checking PATCH 2/3: ipmi: Don't set the timestamp on add events that don't have it...
ERROR: braces {} are necessary for all arms of this statement
#29: FILE: hw/ipmi/ipmi_bmc_sim.c:451:
+    if (event[2] < 0xe0) /* Don't set timestamps for these, per the spec. */
[...]

total: 1 errors, 0 warnings, 22 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/3: ipmi: Add the platform event message command...
ERROR: braces {} are necessary for all arms of this statement
#45: FILE: hw/ipmi/ipmi_bmc_sim.c:1601:
+    if (sel_add_event(ibs, event))
[...]

total: 1 errors, 0 warnings, 41 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org