* [Qemu-devel] [PATCH v2 0/3] ipmi: Fix SEL get/set time commands
@ 2017-08-20 21:46 minyard
2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 1/3] " minyard
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: minyard @ 2017-08-20 21:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Cédric Le Goater, David Gibson
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH v2 1/3] ipmi: Fix SEL get/set time commands 2017-08-20 21:46 [Qemu-devel] [PATCH v2 0/3] ipmi: Fix SEL get/set time commands minyard @ 2017-08-20 21:46 ` minyard 2017-08-21 7:28 ` Cédric Le Goater 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 2/3] ipmi: Don't set the timestamp on add events that don't have it minyard 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command minyard 2 siblings, 1 reply; 8+ messages in thread From: minyard @ 2017-08-20 21:46 UTC (permalink / raw) To: qemu-devel; +Cc: Cédric Le Goater, David Gibson, Corey 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] ipmi: Fix SEL get/set time commands 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 1/3] " minyard @ 2017-08-21 7:28 ` Cédric Le Goater 0 siblings, 0 replies; 8+ messages in thread From: Cédric Le Goater @ 2017-08-21 7:28 UTC (permalink / raw) To: minyard, qemu-devel; +Cc: David Gibson, Corey Minyard 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 = { > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] ipmi: Don't set the timestamp on add events that don't have it 2017-08-20 21:46 [Qemu-devel] [PATCH v2 0/3] ipmi: Fix SEL get/set time commands minyard 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 1/3] " minyard @ 2017-08-20 21:46 ` minyard 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command minyard 2 siblings, 0 replies; 8+ messages in thread From: minyard @ 2017-08-20 21:46 UTC (permalink / raw) To: qemu-devel; +Cc: Cédric Le Goater, David Gibson, Corey 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command 2017-08-20 21:46 [Qemu-devel] [PATCH v2 0/3] ipmi: Fix SEL get/set time commands minyard 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 1/3] " minyard 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 2/3] ipmi: Don't set the timestamp on add events that don't have it minyard @ 2017-08-20 21:46 ` minyard 2017-08-20 22:24 ` Philippe Mathieu-Daudé 2017-08-21 7:37 ` Cédric Le Goater 2 siblings, 2 replies; 8+ messages in thread From: minyard @ 2017-08-20 21:46 UTC (permalink / raw) To: qemu-devel; +Cc: Cédric Le Goater, David Gibson, Corey 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command minyard @ 2017-08-20 22:24 ` Philippe Mathieu-Daudé 2017-08-20 22:41 ` Corey Minyard 2017-08-21 7:37 ` Cédric Le Goater 1 sibling, 1 reply; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2017-08-20 22:24 UTC (permalink / raw) To: minyard, qemu-devel; +Cc: Corey Minyard, Cédric Le Goater, David Gibson 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 }, > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command 2017-08-20 22:24 ` Philippe Mathieu-Daudé @ 2017-08-20 22:41 ` Corey Minyard 0 siblings, 0 replies; 8+ messages in thread From: Corey Minyard @ 2017-08-20 22:41 UTC (permalink / raw) To: Philippe Mathieu-Daudé, minyard, qemu-devel Cc: Corey Minyard, Cédric Le Goater, David Gibson 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 }, >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command minyard 2017-08-20 22:24 ` Philippe Mathieu-Daudé @ 2017-08-21 7:37 ` Cédric Le Goater 1 sibling, 0 replies; 8+ messages in thread From: Cédric Le Goater @ 2017-08-21 7:37 UTC (permalink / raw) To: minyard, qemu-devel; +Cc: David Gibson, Corey Minyard 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 }, > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-21 7:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-20 21:46 [Qemu-devel] [PATCH v2 0/3] ipmi: Fix SEL get/set time commands minyard 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 1/3] " minyard 2017-08-21 7:28 ` Cédric Le Goater 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 2/3] ipmi: Don't set the timestamp on add events that don't have it minyard 2017-08-20 21:46 ` [Qemu-devel] [PATCH v2 3/3] ipmi: Add the platform event message command minyard 2017-08-20 22:24 ` Philippe Mathieu-Daudé 2017-08-20 22:41 ` Corey Minyard 2017-08-21 7:37 ` Cédric Le Goater
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).