| Message ID | 1452015002-28493-6-git-send-email-clg@fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 01/05/2016 11:29 AM, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 60586a67104e..c3a06d0ac7e4 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -25,6 +25,7 @@ > #include <stdio.h> > #include <string.h> > #include <stdint.h> > +#include "sysemu/sysemu.h" > #include "qemu/timer.h" > #include "hw/ipmi/ipmi.h" > #include "qemu/error-report.h" > @@ -54,6 +55,9 @@ > #define IPMI_CMD_GET_DEVICE_ID 0x01 > #define IPMI_CMD_COLD_RESET 0x02 > #define IPMI_CMD_WARM_RESET 0x03 > +#define IPMI_CMD_SET_POWER_STATE 0x06 > +#define IPMI_CMD_GET_POWER_STATE 0x07 These are ACPI power state commands per the spec, can we add ACPI to the name? -corey > +#define IPMI_CMD_GET_DEVICE_GUID 0x08 > #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 > #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 > #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 > @@ -215,6 +219,9 @@ struct IPMIBmcSim { > > uint8_t restart_cause; > > + uint8_t power_state[2]; > + uint8_t uuid[16]; > + > IPMISel sel; > IPMISdr sdr; > IPMIFru fru; > @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, > k->reset(s, false); > } > } > +static void set_power_state(IPMIBmcSim *ibs, > + uint8_t *cmd, unsigned int cmd_len, > + uint8_t *rsp, unsigned int *rsp_len, > + unsigned int max_rsp_len) > +{ > + IPMI_CHECK_CMD_LEN(4); > + ibs->power_state[0] = cmd[2]; > + ibs->power_state[1] = cmd[3]; > + out: > + return; > +} > + > +static void get_power_state(IPMIBmcSim *ibs, > + uint8_t *cmd, unsigned int cmd_len, > + uint8_t *rsp, unsigned int *rsp_len, > + unsigned int max_rsp_len) > +{ > + IPMI_ADD_RSP_DATA(ibs->power_state[0]); > + IPMI_ADD_RSP_DATA(ibs->power_state[1]); > + out: > + return; > +} > + > +static void get_device_guid(IPMIBmcSim *ibs, > + uint8_t *cmd, unsigned int cmd_len, > + uint8_t *rsp, unsigned int *rsp_len, > + unsigned int max_rsp_len) > +{ > + unsigned int i; > + > + for (i = 0; i < 16; i++) { > + IPMI_ADD_RSP_DATA(ibs->uuid[i]); > + } > + out: > + return; > +} > > static void set_bmc_global_enables(IPMIBmcSim *ibs, > uint8_t *cmd, unsigned int cmd_len, > @@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { > [IPMI_CMD_GET_DEVICE_ID] = get_device_id, > [IPMI_CMD_COLD_RESET] = cold_reset, > [IPMI_CMD_WARM_RESET] = warm_reset, > + [IPMI_CMD_SET_POWER_STATE] = set_power_state, > + [IPMI_CMD_GET_POWER_STATE] = get_power_state, > + [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid, > [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables, > [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables, > [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags, > @@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) > i += len; > } > > + ibs->power_state[0] = 0; > + ibs->power_state[1] = 0; > + > + if (qemu_uuid_set) { > + memcpy(&ibs->uuid, qemu_uuid, 16); > + } else { > + memset(&ibs->uuid, 0, 16); > + } > + > ipmi_init_sensors_from_sdrs(ibs); > register_cmds(ibs); >
On 01/08/2016 08:46 PM, Corey Minyard wrote: > On 01/05/2016 11:29 AM, Cédric Le Goater wrote: >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >> --- >> hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >> index 60586a67104e..c3a06d0ac7e4 100644 >> --- a/hw/ipmi/ipmi_bmc_sim.c >> +++ b/hw/ipmi/ipmi_bmc_sim.c >> @@ -25,6 +25,7 @@ >> #include <stdio.h> >> #include <string.h> >> #include <stdint.h> >> +#include "sysemu/sysemu.h" >> #include "qemu/timer.h" >> #include "hw/ipmi/ipmi.h" >> #include "qemu/error-report.h" >> @@ -54,6 +55,9 @@ >> #define IPMI_CMD_GET_DEVICE_ID 0x01 >> #define IPMI_CMD_COLD_RESET 0x02 >> #define IPMI_CMD_WARM_RESET 0x03 >> +#define IPMI_CMD_SET_POWER_STATE 0x06sensors/temperature/outnorth 9.06 sensors/temperature/outsouth 8.38 >> +#define IPMI_CMD_GET_POWER_STATE 0x07 > > These are ACPI power state commands per the spec, can we add ACPI to the name? sure. Will do. Thanks, C. > > -corey >> +#define IPMI_CMD_GET_DEVICE_GUID 0x08 >> #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 >> #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 >> #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 >> @@ -215,6 +219,9 @@ struct IPMIBmcSim { >> uint8_t restart_cause; >> + uint8_t power_state[2]; >> + uint8_t uuid[16]; >> + >> IPMISel sel; >> IPMISdr sdr; >> IPMIFru fru; >> @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, >> k->reset(s, false); >> } >> } >> +static void set_power_state(IPMIBmcSim *ibs, >> + uint8_t *cmd, unsigned int cmd_len, >> + uint8_t *rsp, unsigned int *rsp_len, >> + unsigned int max_rsp_len) >> +{ >> + IPMI_CHECK_CMD_LEN(4); >> + ibs->power_state[0] = cmd[2]; >> + ibs->power_state[1] = cmd[3]; >> + out: >> + return; >> +} >> + >> +static void get_power_state(IPMIBmcSim *ibs, >> + uint8_t *cmd, unsigned int cmd_len, >> + uint8_t *rsp, unsigned int *rsp_len, >> + unsigned int max_rsp_len) >> +{ >> + IPMI_ADD_RSP_DATA(ibs->power_state[0]); >> + IPMI_ADD_RSP_DATA(ibs->power_state[1]); >> + out: >> + return; >> +} >> + >> +static void get_device_guid(IPMIBmcSim *ibs, >> + uint8_t *cmd, unsigned int cmd_len, >> + uint8_t *rsp, unsigned int *rsp_len, >> + unsigned int max_rsp_len) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < 16; i++) { >> + IPMI_ADD_RSP_DATA(ibs->uuid[i]); >> + } >> + out: >> + return; >> +} >> static void set_bmc_global_enables(IPMIBmcSim *ibs, >> uint8_t *cmd, unsigned int cmd_len, >> @@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { >> [IPMI_CMD_GET_DEVICE_ID] = get_device_id, >> [IPMI_CMD_COLD_RESET] = cold_reset, >> [IPMI_CMD_WARM_RESET] = warm_reset, >> + [IPMI_CMD_SET_POWER_STATE] = set_power_state, >> + [IPMI_CMD_GET_POWER_STATE] = get_power_state, >> + [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid, >> [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables, >> [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables, >> [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags, >> @@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) >> i += len; >> } >> + ibs->power_state[0] = 0; >> + ibs->power_state[1] = 0; >> + >> + if (qemu_uuid_set) { >> + memcpy(&ibs->uuid, qemu_uuid, 16); >> + } else { >> + memset(&ibs->uuid, 0, 16); >> + } >> + >> ipmi_init_sensors_from_sdrs(ibs); >> register_cmds(ibs); >> >
On 01/05/2016 07:29 PM, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index 60586a67104e..c3a06d0ac7e4 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -25,6 +25,7 @@ > #include <stdio.h> > #include <string.h> > #include <stdint.h> > +#include "sysemu/sysemu.h" > #include "qemu/timer.h" > #include "hw/ipmi/ipmi.h" > #include "qemu/error-report.h" > @@ -54,6 +55,9 @@ > #define IPMI_CMD_GET_DEVICE_ID 0x01 > #define IPMI_CMD_COLD_RESET 0x02 > #define IPMI_CMD_WARM_RESET 0x03 > +#define IPMI_CMD_SET_POWER_STATE 0x06 > +#define IPMI_CMD_GET_POWER_STATE 0x07 > +#define IPMI_CMD_GET_DEVICE_GUID 0x08 > #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 > #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 > #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 > @@ -215,6 +219,9 @@ struct IPMIBmcSim { > > uint8_t restart_cause; > > + uint8_t power_state[2]; > + uint8_t uuid[16]; > + > IPMISel sel; > IPMISdr sdr; > IPMIFru fru; > @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, > k->reset(s, false); > } > } > +static void set_power_state(IPMIBmcSim *ibs, > + uint8_t *cmd, unsigned int cmd_len, > + uint8_t *rsp, unsigned int *rsp_len, > + unsigned int max_rsp_len) > +{ > + IPMI_CHECK_CMD_LEN(4); > + ibs->power_state[0] = cmd[2]; > + ibs->power_state[1] = cmd[3]; > + out: > + return; Hi, I am sorry for my late comment, but I find a little strange the use of the "out" label here. I understand this is because of its usage in IPMI_* macros, but I looked into every usage(I hope I didn't miss anything) and the code simply returns. Also the correlation between those macros is a little odd. Thanks, Marcel > +} > + > +static void get_power_state(IPMIBmcSim *ibs, > + uint8_t *cmd, unsigned int cmd_len, > + uint8_t *rsp, unsigned int *rsp_len, > + unsigned int max_rsp_len) > +{ > + IPMI_ADD_RSP_DATA(ibs->power_state[0]); > + IPMI_ADD_RSP_DATA(ibs->power_state[1]); > + out: > + return; > +} > + > +static void get_device_guid(IPMIBmcSim *ibs, > + uint8_t *cmd, unsigned int cmd_len, > + uint8_t *rsp, unsigned int *rsp_len, > + unsigned int max_rsp_len) > +{ > + unsigned int i; > + > + for (i = 0; i < 16; i++) { > + IPMI_ADD_RSP_DATA(ibs->uuid[i]); > + } > + out: > + return; > +} > > static void set_bmc_global_enables(IPMIBmcSim *ibs, > uint8_t *cmd, unsigned int cmd_len, > @@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { > [IPMI_CMD_GET_DEVICE_ID] = get_device_id, > [IPMI_CMD_COLD_RESET] = cold_reset, > [IPMI_CMD_WARM_RESET] = warm_reset, > + [IPMI_CMD_SET_POWER_STATE] = set_power_state, > + [IPMI_CMD_GET_POWER_STATE] = get_power_state, > + [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid, > [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables, > [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables, > [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags, > @@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) > i += len; > } > > + ibs->power_state[0] = 0; > + ibs->power_state[1] = 0; > + > + if (qemu_uuid_set) { > + memcpy(&ibs->uuid, qemu_uuid, 16); > + } else { > + memset(&ibs->uuid, 0, 16); > + } > + > ipmi_init_sensors_from_sdrs(ibs); > register_cmds(ibs); > >
On 01/17/2016 02:04 PM, Marcel Apfelbaum wrote: > On 01/05/2016 07:29 PM, Cédric Le Goater wrote: >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >> --- >> hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >> index 60586a67104e..c3a06d0ac7e4 100644 >> --- a/hw/ipmi/ipmi_bmc_sim.c >> +++ b/hw/ipmi/ipmi_bmc_sim.c >> @@ -25,6 +25,7 @@ >> #include <stdio.h> >> #include <string.h> >> #include <stdint.h> >> +#include "sysemu/sysemu.h" >> #include "qemu/timer.h" >> #include "hw/ipmi/ipmi.h" >> #include "qemu/error-report.h" >> @@ -54,6 +55,9 @@ >> #define IPMI_CMD_GET_DEVICE_ID 0x01 >> #define IPMI_CMD_COLD_RESET 0x02 >> #define IPMI_CMD_WARM_RESET 0x03 >> +#define IPMI_CMD_SET_POWER_STATE 0x06 >> +#define IPMI_CMD_GET_POWER_STATE 0x07 >> +#define IPMI_CMD_GET_DEVICE_GUID 0x08 >> #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 >> #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 >> #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 >> @@ -215,6 +219,9 @@ struct IPMIBmcSim { >> >> uint8_t restart_cause; >> >> + uint8_t power_state[2]; >> + uint8_t uuid[16]; >> + >> IPMISel sel; >> IPMISdr sdr; >> IPMIFru fru; >> @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, >> k->reset(s, false); >> } >> } >> +static void set_power_state(IPMIBmcSim *ibs, >> + uint8_t *cmd, unsigned int cmd_len, >> + uint8_t *rsp, unsigned int *rsp_len, >> + unsigned int max_rsp_len) >> +{ >> + IPMI_CHECK_CMD_LEN(4); >> + ibs->power_state[0] = cmd[2]; >> + ibs->power_state[1] = cmd[3]; >> + out: >> + return; > > > Hi, > > I am sorry for my late comment, but I find a little strange the use of > the "out" label here. > I understand this is because of its usage in IPMI_* macros, but > I looked into every usage(I hope I didn't miss anything) and the code > simply returns. > Also the correlation between those macros is a little odd. I meant the correlation between the macros and the "out" label. Thanks, Marcel > > Thanks, > Marcel > > >> +} >> + >> +static void get_power_state(IPMIBmcSim *ibs, >> + uint8_t *cmd, unsigned int cmd_len, >> + uint8_t *rsp, unsigned int *rsp_len, >> + unsigned int max_rsp_len) >> +{ >> + IPMI_ADD_RSP_DATA(ibs->power_state[0]); >> + IPMI_ADD_RSP_DATA(ibs->power_state[1]); >> + out: >> + return; >> +} >> + >> +static void get_device_guid(IPMIBmcSim *ibs, >> + uint8_t *cmd, unsigned int cmd_len, >> + uint8_t *rsp, unsigned int *rsp_len, >> + unsigned int max_rsp_len) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < 16; i++) { >> + IPMI_ADD_RSP_DATA(ibs->uuid[i]); >> + } >> + out: >> + return; >> +} >> >> static void set_bmc_global_enables(IPMIBmcSim *ibs, >> uint8_t *cmd, unsigned int cmd_len, >> @@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { >> [IPMI_CMD_GET_DEVICE_ID] = get_device_id, >> [IPMI_CMD_COLD_RESET] = cold_reset, >> [IPMI_CMD_WARM_RESET] = warm_reset, >> + [IPMI_CMD_SET_POWER_STATE] = set_power_state, >> + [IPMI_CMD_GET_POWER_STATE] = get_power_state, >> + [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid, >> [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables, >> [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables, >> [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags, >> @@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) >> i += len; >> } >> >> + ibs->power_state[0] = 0; >> + ibs->power_state[1] = 0; >> + >> + if (qemu_uuid_set) { >> + memcpy(&ibs->uuid, qemu_uuid, 16); >> + } else { >> + memset(&ibs->uuid, 0, 16); >> + } >> + >> ipmi_init_sensors_from_sdrs(ibs); >> register_cmds(ibs); >> >> >
On Sun, Jan 17, 2016 at 02:04:32PM +0200, Marcel Apfelbaum wrote: > On 01/05/2016 07:29 PM, Cédric Le Goater wrote: > >Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > >--- > > hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > >diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > >index 60586a67104e..c3a06d0ac7e4 100644 > >--- a/hw/ipmi/ipmi_bmc_sim.c > >+++ b/hw/ipmi/ipmi_bmc_sim.c > >@@ -25,6 +25,7 @@ > > #include <stdio.h> > > #include <string.h> > > #include <stdint.h> > >+#include "sysemu/sysemu.h" > > #include "qemu/timer.h" > > #include "hw/ipmi/ipmi.h" > > #include "qemu/error-report.h" > >@@ -54,6 +55,9 @@ > > #define IPMI_CMD_GET_DEVICE_ID 0x01 > > #define IPMI_CMD_COLD_RESET 0x02 > > #define IPMI_CMD_WARM_RESET 0x03 > >+#define IPMI_CMD_SET_POWER_STATE 0x06 > >+#define IPMI_CMD_GET_POWER_STATE 0x07 > >+#define IPMI_CMD_GET_DEVICE_GUID 0x08 > > #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 > > #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 > > #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 > >@@ -215,6 +219,9 @@ struct IPMIBmcSim { > > > > uint8_t restart_cause; > > > >+ uint8_t power_state[2]; > >+ uint8_t uuid[16]; > >+ > > IPMISel sel; > > IPMISdr sdr; > > IPMIFru fru; > >@@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, > > k->reset(s, false); > > } > > } > >+static void set_power_state(IPMIBmcSim *ibs, > >+ uint8_t *cmd, unsigned int cmd_len, > >+ uint8_t *rsp, unsigned int *rsp_len, > >+ unsigned int max_rsp_len) > >+{ > >+ IPMI_CHECK_CMD_LEN(4); > >+ ibs->power_state[0] = cmd[2]; > >+ ibs->power_state[1] = cmd[3]; > >+ out: > >+ return; > > > Hi, > > I am sorry for my late comment, but I find a little strange the use of > the "out" label here. > I understand this is because of its usage in IPMI_* macros, but > I looked into every usage(I hope I didn't miss anything) and the code > simply returns. > Also the correlation between those macros is a little odd. > > Thanks, > Marcel Yes - these macros with goto out are confusing. Please rewrite them to return bool, and put goto out in the caller. > > >+} > >+ > >+static void get_power_state(IPMIBmcSim *ibs, > >+ uint8_t *cmd, unsigned int cmd_len, > >+ uint8_t *rsp, unsigned int *rsp_len, > >+ unsigned int max_rsp_len) > >+{ > >+ IPMI_ADD_RSP_DATA(ibs->power_state[0]); > >+ IPMI_ADD_RSP_DATA(ibs->power_state[1]); > >+ out: > >+ return; > >+} > >+ > >+static void get_device_guid(IPMIBmcSim *ibs, > >+ uint8_t *cmd, unsigned int cmd_len, > >+ uint8_t *rsp, unsigned int *rsp_len, > >+ unsigned int max_rsp_len) > >+{ > >+ unsigned int i; > >+ > >+ for (i = 0; i < 16; i++) { > >+ IPMI_ADD_RSP_DATA(ibs->uuid[i]); > >+ } > >+ out: > >+ return; > >+} > > > > static void set_bmc_global_enables(IPMIBmcSim *ibs, > > uint8_t *cmd, unsigned int cmd_len, > >@@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { > > [IPMI_CMD_GET_DEVICE_ID] = get_device_id, > > [IPMI_CMD_COLD_RESET] = cold_reset, > > [IPMI_CMD_WARM_RESET] = warm_reset, > >+ [IPMI_CMD_SET_POWER_STATE] = set_power_state, > >+ [IPMI_CMD_GET_POWER_STATE] = get_power_state, > >+ [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid, > > [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables, > > [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables, > > [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags, > >@@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) > > i += len; > > } > > > >+ ibs->power_state[0] = 0; > >+ ibs->power_state[1] = 0; > >+ > >+ if (qemu_uuid_set) { > >+ memcpy(&ibs->uuid, qemu_uuid, 16); > >+ } else { > >+ memset(&ibs->uuid, 0, 16); > >+ } > >+ > > ipmi_init_sensors_from_sdrs(ibs); > > register_cmds(ibs); > > > >
On 01/17/2016 01:08 PM, Marcel Apfelbaum wrote: > On 01/17/2016 02:04 PM, Marcel Apfelbaum wrote: >> On 01/05/2016 07:29 PM, Cédric Le Goater wrote: >>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >>> --- >>> hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 55 insertions(+) >>> >>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >>> index 60586a67104e..c3a06d0ac7e4 100644 >>> --- a/hw/ipmi/ipmi_bmc_sim.c >>> +++ b/hw/ipmi/ipmi_bmc_sim.c >>> @@ -25,6 +25,7 @@ >>> #include <stdio.h> >>> #include <string.h> >>> #include <stdint.h> >>> +#include "sysemu/sysemu.h" >>> #include "qemu/timer.h" >>> #include "hw/ipmi/ipmi.h" >>> #include "qemu/error-report.h" >>> @@ -54,6 +55,9 @@ >>> #define IPMI_CMD_GET_DEVICE_ID 0x01 >>> #define IPMI_CMD_COLD_RESET 0x02 >>> #define IPMI_CMD_WARM_RESET 0x03 >>> +#define IPMI_CMD_SET_POWER_STATE 0x06 >>> +#define IPMI_CMD_GET_POWER_STATE 0x07 >>> +#define IPMI_CMD_GET_DEVICE_GUID 0x08 >>> #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 >>> #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 >>> #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 >>> @@ -215,6 +219,9 @@ struct IPMIBmcSim { >>> >>> uint8_t restart_cause; >>> >>> + uint8_t power_state[2]; >>> + uint8_t uuid[16]; >>> + >>> IPMISel sel; >>> IPMISdr sdr; >>> IPMIFru fru; >>> @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, >>> k->reset(s, false); >>> } >>> } >>> +static void set_power_state(IPMIBmcSim *ibs, >>> + uint8_t *cmd, unsigned int cmd_len, >>> + uint8_t *rsp, unsigned int *rsp_len, >>> + unsigned int max_rsp_len) >>> +{ >>> + IPMI_CHECK_CMD_LEN(4); >>> + ibs->power_state[0] = cmd[2]; >>> + ibs->power_state[1] = cmd[3]; >>> + out: >>> + return; >> >> >> Hi, >> >> I am sorry for my late comment, but I find a little strange the use of >> the "out" label here. >> I understand this is because of its usage in IPMI_* macros, but >> I looked into every usage(I hope I didn't miss anything) and the code >> simply returns. >> Also the correlation between those macros is a little odd. > > I meant the correlation between the macros and the "out" label. Yes. I agree these gotos are a little odd. There is a slight difference with the routine ipmi_sim_handle_command() and the use of the macro IPMI_ADD_RSP_DATA(). I will see what I can do. Thanks, C. > Thanks, > Marcel > >> >> Thanks, >> Marcel >> >> >>> +} >>> + >>> +static void get_power_state(IPMIBmcSim *ibs, >>> + uint8_t *cmd, unsigned int cmd_len, >>> + uint8_t *rsp, unsigned int *rsp_len, >>> + unsigned int max_rsp_len) >>> +{ >>> + IPMI_ADD_RSP_DATA(ibs->power_state[0]); >>> + IPMI_ADD_RSP_DATA(ibs->power_state[1]); >>> + out: >>> + return; >>> +} >>> + >>> +static void get_device_guid(IPMIBmcSim *ibs, >>> + uint8_t *cmd, unsigned int cmd_len, >>> + uint8_t *rsp, unsigned int *rsp_len, >>> + unsigned int max_rsp_len) >>> +{ >>> + unsigned int i; >>> + >>> + for (i = 0; i < 16; i++) { >>> + IPMI_ADD_RSP_DATA(ibs->uuid[i]); >>> + } >>> + out: >>> + return; >>> +} >>> >>> static void set_bmc_global_enables(IPMIBmcSim *ibs, >>> uint8_t *cmd, unsigned int cmd_len, >>> @@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { >>> [IPMI_CMD_GET_DEVICE_ID] = get_device_id, >>> [IPMI_CMD_COLD_RESET] = cold_reset, >>> [IPMI_CMD_WARM_RESET] = warm_reset, >>> + [IPMI_CMD_SET_POWER_STATE] = set_power_state, >>> + [IPMI_CMD_GET_POWER_STATE] = get_power_state, >>> + [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid, >>> [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables, >>> [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables, >>> [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags, >>> @@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) >>> i += len; >>> } >>> >>> + ibs->power_state[0] = 0; >>> + ibs->power_state[1] = 0; >>> + >>> + if (qemu_uuid_set) { >>> + memcpy(&ibs->uuid, qemu_uuid, 16); >>> + } else { >>> + memset(&ibs->uuid, 0, 16); >>> + } >>> + >>> ipmi_init_sensors_from_sdrs(ibs); >>> register_cmds(ibs); >>> >>> >> >
On 01/17/2016 08:16 AM, Michael S. Tsirkin wrote: > On Sun, Jan 17, 2016 at 02:04:32PM +0200, Marcel Apfelbaum wrote: >> On 01/05/2016 07:29 PM, Cédric Le Goater wrote: >>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >>> --- >>> hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 55 insertions(+) >>> >>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >>> index 60586a67104e..c3a06d0ac7e4 100644 >>> --- a/hw/ipmi/ipmi_bmc_sim.c >>> +++ b/hw/ipmi/ipmi_bmc_sim.c >>> @@ -25,6 +25,7 @@ >>> #include <stdio.h> >>> #include <string.h> >>> #include <stdint.h> >>> +#include "sysemu/sysemu.h" >>> #include "qemu/timer.h" >>> #include "hw/ipmi/ipmi.h" >>> #include "qemu/error-report.h" >>> @@ -54,6 +55,9 @@ >>> #define IPMI_CMD_GET_DEVICE_ID 0x01 >>> #define IPMI_CMD_COLD_RESET 0x02 >>> #define IPMI_CMD_WARM_RESET 0x03 >>> +#define IPMI_CMD_SET_POWER_STATE 0x06 >>> +#define IPMI_CMD_GET_POWER_STATE 0x07 >>> +#define IPMI_CMD_GET_DEVICE_GUID 0x08 >>> #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 >>> #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 >>> #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 >>> @@ -215,6 +219,9 @@ struct IPMIBmcSim { >>> >>> uint8_t restart_cause; >>> >>> + uint8_t power_state[2]; >>> + uint8_t uuid[16]; >>> + >>> IPMISel sel; >>> IPMISdr sdr; >>> IPMIFru fru; >>> @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, >>> k->reset(s, false); >>> } >>> } >>> +static void set_power_state(IPMIBmcSim *ibs, >>> + uint8_t *cmd, unsigned int cmd_len, >>> + uint8_t *rsp, unsigned int *rsp_len, >>> + unsigned int max_rsp_len) >>> +{ >>> + IPMI_CHECK_CMD_LEN(4); >>> + ibs->power_state[0] = cmd[2]; >>> + ibs->power_state[1] = cmd[3]; >>> + out: >>> + return; >> >> Hi, >> >> I am sorry for my late comment, but I find a little strange the use of >> the "out" label here. >> I understand this is because of its usage in IPMI_* macros, but >> I looked into every usage(I hope I didn't miss anything) and the code >> simply returns. >> Also the correlation between those macros is a little odd. >> >> Thanks, >> Marcel > > Yes - these macros with goto out are confusing. > > Please rewrite them to return bool, and put > goto out in the caller. > Marcel, do you want me to do this? -corey > >>> +} >>> + >>> +static void get_power_state(IPMIBmcSim *ibs, >>> + uint8_t *cmd, unsigned int cmd_len, >>> + uint8_t *rsp, unsigned int *rsp_len, >>> + unsigned int max_rsp_len) >>> +{ >>> + IPMI_ADD_RSP_DATA(ibs->power_state[0]); >>> + IPMI_ADD_RSP_DATA(ibs->power_state[1]); >>> + out: >>> + return; >>> +} >>> + >>> +static void get_device_guid(IPMIBmcSim *ibs, >>> + uint8_t *cmd, unsigned int cmd_len, >>> + uint8_t *rsp, unsigned int *rsp_len, >>> + unsigned int max_rsp_len) >>> +{ >>> + unsigned int i; >>> + >>> + for (i = 0; i < 16; i++) { >>> + IPMI_ADD_RSP_DATA(ibs->uuid[i]); >>> + } >>> + out: >>> + return; >>> +} >>> >>> static void set_bmc_global_enables(IPMIBmcSim *ibs, >>> uint8_t *cmd, unsigned int cmd_len, >>> @@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { >>> [IPMI_CMD_GET_DEVICE_ID] = get_device_id, >>> [IPMI_CMD_COLD_RESET] = cold_reset, >>> [IPMI_CMD_WARM_RESET] = warm_reset, >>> + [IPMI_CMD_SET_POWER_STATE] = set_power_state, >>> + [IPMI_CMD_GET_POWER_STATE] = get_power_state, >>> + [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid, >>> [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables, >>> [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables, >>> [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags, >>> @@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) >>> i += len; >>> } >>> >>> + ibs->power_state[0] = 0; >>> + ibs->power_state[1] = 0; >>> + >>> + if (qemu_uuid_set) { >>> + memcpy(&ibs->uuid, qemu_uuid, 16); >>> + } else { >>> + memset(&ibs->uuid, 0, 16); >>> + } >>> + >>> ipmi_init_sensors_from_sdrs(ibs); >>> register_cmds(ibs); >>> >>>
On 01/21/2016 05:37 PM, Corey Minyard wrote: > On 01/17/2016 08:16 AM, Michael S. Tsirkin wrote: >> On Sun, Jan 17, 2016 at 02:04:32PM +0200, Marcel Apfelbaum wrote: >>> On 01/05/2016 07:29 PM, Cédric Le Goater wrote: >>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >>>> --- >>>> hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 55 insertions(+) >>>> >>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >>>> index 60586a67104e..c3a06d0ac7e4 100644 >>>> --- a/hw/ipmi/ipmi_bmc_sim.c >>>> +++ b/hw/ipmi/ipmi_bmc_sim.c >>>> @@ -25,6 +25,7 @@ >>>> #include <stdio.h> >>>> #include <string.h> >>>> #include <stdint.h> >>>> +#include "sysemu/sysemu.h" >>>> #include "qemu/timer.h" >>>> #include "hw/ipmi/ipmi.h" >>>> #include "qemu/error-report.h" >>>> @@ -54,6 +55,9 @@ >>>> #define IPMI_CMD_GET_DEVICE_ID 0x01 >>>> #define IPMI_CMD_COLD_RESET 0x02 >>>> #define IPMI_CMD_WARM_RESET 0x03 >>>> +#define IPMI_CMD_SET_POWER_STATE 0x06 >>>> +#define IPMI_CMD_GET_POWER_STATE 0x07 >>>> +#define IPMI_CMD_GET_DEVICE_GUID 0x08 >>>> #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 >>>> #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 >>>> #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 >>>> @@ -215,6 +219,9 @@ struct IPMIBmcSim { >>>> >>>> uint8_t restart_cause; >>>> >>>> + uint8_t power_state[2]; >>>> + uint8_t uuid[16]; >>>> + >>>> IPMISel sel; >>>> IPMISdr sdr; >>>> IPMIFru fru; >>>> @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, >>>> k->reset(s, false); >>>> } >>>> } >>>> +static void set_power_state(IPMIBmcSim *ibs, >>>> + uint8_t *cmd, unsigned int cmd_len, >>>> + uint8_t *rsp, unsigned int *rsp_len, >>>> + unsigned int max_rsp_len) >>>> +{ >>>> + IPMI_CHECK_CMD_LEN(4); >>>> + ibs->power_state[0] = cmd[2]; >>>> + ibs->power_state[1] = cmd[3]; >>>> + out: >>>> + return; >>> >>> Hi, >>> >>> I am sorry for my late comment, but I find a little strange the use of >>> the "out" label here. >>> I understand this is because of its usage in IPMI_* macros, but >>> I looked into every usage(I hope I didn't miss anything) and the code >>> simply returns. >>> Also the correlation between those macros is a little odd. >>> >>> Thanks, >>> Marcel >> >> Yes - these macros with goto out are confusing. >> >> Please rewrite them to return bool, and put >> goto out in the caller. >> > > Marcel, do you want me to do this? I have the patch ready (and more). I will send this one to start with. C.
On 01/21/2016 06:41 PM, Cédric Le Goater wrote: > On 01/21/2016 05:37 PM, Corey Minyard wrote: >> On 01/17/2016 08:16 AM, Michael S. Tsirkin wrote: >>> On Sun, Jan 17, 2016 at 02:04:32PM +0200, Marcel Apfelbaum wrote: >>>> On 01/05/2016 07:29 PM, Cédric Le Goater wrote: >>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >>>>> --- >>>>> hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 55 insertions(+) >>>>> >>>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >>>>> index 60586a67104e..c3a06d0ac7e4 100644 >>>>> --- a/hw/ipmi/ipmi_bmc_sim.c >>>>> +++ b/hw/ipmi/ipmi_bmc_sim.c >>>>> @@ -25,6 +25,7 @@ >>>>> #include <stdio.h> >>>>> #include <string.h> >>>>> #include <stdint.h> >>>>> +#include "sysemu/sysemu.h" >>>>> #include "qemu/timer.h" >>>>> #include "hw/ipmi/ipmi.h" >>>>> #include "qemu/error-report.h" >>>>> @@ -54,6 +55,9 @@ >>>>> #define IPMI_CMD_GET_DEVICE_ID 0x01 >>>>> #define IPMI_CMD_COLD_RESET 0x02 >>>>> #define IPMI_CMD_WARM_RESET 0x03 >>>>> +#define IPMI_CMD_SET_POWER_STATE 0x06 >>>>> +#define IPMI_CMD_GET_POWER_STATE 0x07 >>>>> +#define IPMI_CMD_GET_DEVICE_GUID 0x08 >>>>> #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 >>>>> #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 >>>>> #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 >>>>> @@ -215,6 +219,9 @@ struct IPMIBmcSim { >>>>> >>>>> uint8_t restart_cause; >>>>> >>>>> + uint8_t power_state[2]; >>>>> + uint8_t uuid[16]; >>>>> + >>>>> IPMISel sel; >>>>> IPMISdr sdr; >>>>> IPMIFru fru; >>>>> @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, >>>>> k->reset(s, false); >>>>> } >>>>> } >>>>> +static void set_power_state(IPMIBmcSim *ibs, >>>>> + uint8_t *cmd, unsigned int cmd_len, >>>>> + uint8_t *rsp, unsigned int *rsp_len, >>>>> + unsigned int max_rsp_len) >>>>> +{ >>>>> + IPMI_CHECK_CMD_LEN(4); >>>>> + ibs->power_state[0] = cmd[2]; >>>>> + ibs->power_state[1] = cmd[3]; >>>>> + out: >>>>> + return; >>>> >>>> Hi, >>>> >>>> I am sorry for my late comment, but I find a little strange the use of >>>> the "out" label here. >>>> I understand this is because of its usage in IPMI_* macros, but >>>> I looked into every usage(I hope I didn't miss anything) and the code >>>> simply returns. >>>> Also the correlation between those macros is a little odd. >>>> >>>> Thanks, >>>> Marcel >>> >>> Yes - these macros with goto out are confusing. >>> >>> Please rewrite them to return bool, and put >>> goto out in the caller. >>> >> >> Marcel, do you want me to do this? > > I have the patch ready (and more). I will send this one to start with. Thanks! Marcel > > C. >
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index 60586a67104e..c3a06d0ac7e4 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -25,6 +25,7 @@ #include <stdio.h> #include <string.h> #include <stdint.h> +#include "sysemu/sysemu.h" #include "qemu/timer.h" #include "hw/ipmi/ipmi.h" #include "qemu/error-report.h" @@ -54,6 +55,9 @@ #define IPMI_CMD_GET_DEVICE_ID 0x01 #define IPMI_CMD_COLD_RESET 0x02 #define IPMI_CMD_WARM_RESET 0x03 +#define IPMI_CMD_SET_POWER_STATE 0x06 +#define IPMI_CMD_GET_POWER_STATE 0x07 +#define IPMI_CMD_GET_DEVICE_GUID 0x08 #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 @@ -215,6 +219,9 @@ struct IPMIBmcSim { uint8_t restart_cause; + uint8_t power_state[2]; + uint8_t uuid[16]; + IPMISel sel; IPMISdr sdr; IPMIFru fru; @@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, k->reset(s, false); } } +static void set_power_state(IPMIBmcSim *ibs, + uint8_t *cmd, unsigned int cmd_len, + uint8_t *rsp, unsigned int *rsp_len, + unsigned int max_rsp_len) +{ + IPMI_CHECK_CMD_LEN(4); + ibs->power_state[0] = cmd[2]; + ibs->power_state[1] = cmd[3]; + out: + return; +} + +static void get_power_state(IPMIBmcSim *ibs, + uint8_t *cmd, unsigned int cmd_len, + uint8_t *rsp, unsigned int *rsp_len, + unsigned int max_rsp_len) +{ + IPMI_ADD_RSP_DATA(ibs->power_state[0]); + IPMI_ADD_RSP_DATA(ibs->power_state[1]); + out: + return; +} + +static void get_device_guid(IPMIBmcSim *ibs, + uint8_t *cmd, unsigned int cmd_len, + uint8_t *rsp, unsigned int *rsp_len, + unsigned int max_rsp_len) +{ + unsigned int i; + + for (i = 0; i < 16; i++) { + IPMI_ADD_RSP_DATA(ibs->uuid[i]); + } + out: + return; +} static void set_bmc_global_enables(IPMIBmcSim *ibs, uint8_t *cmd, unsigned int cmd_len, @@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_APP_MAXCMD] = { [IPMI_CMD_GET_DEVICE_ID] = get_device_id, [IPMI_CMD_COLD_RESET] = cold_reset, [IPMI_CMD_WARM_RESET] = warm_reset, + [IPMI_CMD_SET_POWER_STATE] = set_power_state, + [IPMI_CMD_GET_POWER_STATE] = get_power_state, + [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid, [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables, [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables, [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags, @@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) i += len; } + ibs->power_state[0] = 0; + ibs->power_state[1] = 0; + + if (qemu_uuid_set) { + memcpy(&ibs->uuid, qemu_uuid, 16); + } else { + memset(&ibs->uuid, 0, 16); + } + ipmi_init_sensors_from_sdrs(ibs); register_cmds(ibs);
Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- hw/ipmi/ipmi_bmc_sim.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)