[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands |
Date: |
Sun, 17 Jan 2016 16:16:09 +0200 |
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 <address@hidden>
> >---
> > 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);
> >
> >