From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aKo7u-0001PQ-UC for qemu-devel@nongnu.org; Sun, 17 Jan 2016 09:16:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aKo7p-0006B6-Tw for qemu-devel@nongnu.org; Sun, 17 Jan 2016 09:16:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50230) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aKo7p-00069d-M5 for qemu-devel@nongnu.org; Sun, 17 Jan 2016 09:16:13 -0500 Date: Sun, 17 Jan 2016 16:16:09 +0200 From: "Michael S. Tsirkin" Message-ID: <20160117161425-mutt-send-email-mst@redhat.com> References: <1452015002-28493-6-git-send-email-clg@fr.ibm.com> <569B8350.2040305@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <569B8350.2040305@gmail.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcel@redhat.com Cc: Corey Minyard , =?iso-8859-1?Q?C=E9dric?= Le Goater , qemu-devel@nongnu.org On Sun, Jan 17, 2016 at 02:04:32PM +0200, Marcel Apfelbaum wrote: > On 01/05/2016 07:29 PM, C=E9dric Le Goater wrote: > >Signed-off-by: C=E9dric Le Goater > >--- > > 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 > > #include > > #include > >+#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] =3D cmd[2]; > >+ ibs->power_state[1] =3D cmd[3]; > >+ out: > >+ return; >=20 >=20 > Hi, >=20 > 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. >=20 > Thanks, > Marcel Yes - these macros with goto out are confusing. Please rewrite them to return bool, and put goto out in the caller. >=20 > >+} > >+ > >+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 =3D 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_le= n, > >@@ -1781,6 +1824,9 @@ static const IPMICmdHandler app_cmds[IPMI_NETFN_= APP_MAXCMD] =3D { > > [IPMI_CMD_GET_DEVICE_ID] =3D get_device_id, > > [IPMI_CMD_COLD_RESET] =3D cold_reset, > > [IPMI_CMD_WARM_RESET] =3D warm_reset, > >+ [IPMI_CMD_SET_POWER_STATE] =3D set_power_state, > >+ [IPMI_CMD_GET_POWER_STATE] =3D get_power_state, > >+ [IPMI_CMD_GET_DEVICE_GUID] =3D get_device_guid, > > [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] =3D set_bmc_global_enables, > > [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] =3D get_bmc_global_enables, > > [IPMI_CMD_CLR_MSG_FLAGS] =3D clr_msg_flags, > >@@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) > > i +=3D len; > > } > > > >+ ibs->power_state[0] =3D 0; > >+ ibs->power_state[1] =3D 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); > > > >