From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55552) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aL8Xw-0000nK-I0 for qemu-devel@nongnu.org; Mon, 18 Jan 2016 07:04:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aL8Xr-0003WX-FI for qemu-devel@nongnu.org; Mon, 18 Jan 2016 07:04:32 -0500 Received: from e06smtp16.uk.ibm.com ([195.75.94.112]:54237) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aL8Xr-0003V7-65 for qemu-devel@nongnu.org; Mon, 18 Jan 2016 07:04:27 -0500 Received: from localhost by e06smtp16.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 18 Jan 2016 12:04:22 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 7B4732190046 for ; Mon, 18 Jan 2016 12:04:08 +0000 (GMT) Received: from d06av06.portsmouth.uk.ibm.com (d06av06.portsmouth.uk.ibm.com [9.149.37.217]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u0IC4Kam59703312 for ; Mon, 18 Jan 2016 12:04:20 GMT Received: from d06av06.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av06.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u0IC4JvY002736 for ; Mon, 18 Jan 2016 07:04:19 -0500 References: <1452015002-28493-6-git-send-email-clg@fr.ibm.com> <569B8350.2040305@gmail.com> <569B8449.3090501@gmail.com> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <569CD4BB.6090306@fr.ibm.com> Date: Mon, 18 Jan 2016 13:04:11 +0100 MIME-Version: 1.0 In-Reply-To: <569B8449.3090501@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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, Corey Minyard Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" 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 >>> --- >>> 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] = 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); >>> >>> >> >