| Message ID | 1503265602-9349-4-git-send-email-minyard@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] ipmi: Fix SEL get/set time commands | expand |
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 }, >
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 }, >> >
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 }, >
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 },