* [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
@ 2016-01-05 17:29 Cédric Le Goater
2016-01-08 19:46 ` Corey Minyard
2016-01-17 12:04 ` Marcel Apfelbaum
0 siblings, 2 replies; 10+ messages in thread
From: Cédric Le Goater @ 2016-01-05 17:29 UTC (permalink / raw)
To: Corey Minyard; +Cc: Cédric Le Goater, qemu-devel, Michael S. Tsirkin
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;
+}
+
+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);
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-05 17:29 [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands Cédric Le Goater
@ 2016-01-08 19:46 ` Corey Minyard
2016-01-12 7:36 ` Cédric Le Goater
2016-01-17 12:04 ` Marcel Apfelbaum
1 sibling, 1 reply; 10+ messages in thread
From: Corey Minyard @ 2016-01-08 19:46 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-devel, Michael S. Tsirkin
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);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-08 19:46 ` Corey Minyard
@ 2016-01-12 7:36 ` Cédric Le Goater
0 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2016-01-12 7:36 UTC (permalink / raw)
To: Corey Minyard; +Cc: qemu-devel, Michael S. Tsirkin
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);
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-05 17:29 [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands Cédric Le Goater
2016-01-08 19:46 ` Corey Minyard
@ 2016-01-17 12:04 ` Marcel Apfelbaum
2016-01-17 12:08 ` Marcel Apfelbaum
2016-01-17 14:16 ` Michael S. Tsirkin
1 sibling, 2 replies; 10+ messages in thread
From: Marcel Apfelbaum @ 2016-01-17 12:04 UTC (permalink / raw)
To: Cédric Le Goater, Corey Minyard; +Cc: qemu-devel, Michael S. Tsirkin
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);
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-17 12:04 ` Marcel Apfelbaum
@ 2016-01-17 12:08 ` Marcel Apfelbaum
2016-01-18 12:04 ` Cédric Le Goater
2016-01-17 14:16 ` Michael S. Tsirkin
1 sibling, 1 reply; 10+ messages in thread
From: Marcel Apfelbaum @ 2016-01-17 12:08 UTC (permalink / raw)
To: Cédric Le Goater, Corey Minyard; +Cc: qemu-devel, Michael S. Tsirkin
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);
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-17 12:04 ` Marcel Apfelbaum
2016-01-17 12:08 ` Marcel Apfelbaum
@ 2016-01-17 14:16 ` Michael S. Tsirkin
2016-01-21 16:37 ` Corey Minyard
1 sibling, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2016-01-17 14:16 UTC (permalink / raw)
To: marcel; +Cc: Corey Minyard, Cédric Le Goater, qemu-devel
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);
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-17 12:08 ` Marcel Apfelbaum
@ 2016-01-18 12:04 ` Cédric Le Goater
0 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2016-01-18 12:04 UTC (permalink / raw)
To: marcel, Corey Minyard; +Cc: qemu-devel, 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 <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);
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-17 14:16 ` Michael S. Tsirkin
@ 2016-01-21 16:37 ` Corey Minyard
2016-01-21 16:41 ` Cédric Le Goater
0 siblings, 1 reply; 10+ messages in thread
From: Corey Minyard @ 2016-01-21 16:37 UTC (permalink / raw)
To: Michael S. Tsirkin, marcel
Cc: Corey Minyard, Cédric Le Goater, qemu-devel
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);
>>>
>>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-21 16:37 ` Corey Minyard
@ 2016-01-21 16:41 ` Cédric Le Goater
2016-01-21 16:51 ` Marcel Apfelbaum
0 siblings, 1 reply; 10+ messages in thread
From: Cédric Le Goater @ 2016-01-21 16:41 UTC (permalink / raw)
To: minyard, Michael S. Tsirkin, marcel; +Cc: Corey Minyard, qemu-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands
2016-01-21 16:41 ` Cédric Le Goater
@ 2016-01-21 16:51 ` Marcel Apfelbaum
0 siblings, 0 replies; 10+ messages in thread
From: Marcel Apfelbaum @ 2016-01-21 16:51 UTC (permalink / raw)
To: Cédric Le Goater, minyard, Michael S. Tsirkin
Cc: Corey Minyard, qemu-devel
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.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-01-21 16:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-05 17:29 [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands Cédric Le Goater
2016-01-08 19:46 ` Corey Minyard
2016-01-12 7:36 ` Cédric Le Goater
2016-01-17 12:04 ` Marcel Apfelbaum
2016-01-17 12:08 ` Marcel Apfelbaum
2016-01-18 12:04 ` Cédric Le Goater
2016-01-17 14:16 ` Michael S. Tsirkin
2016-01-21 16:37 ` Corey Minyard
2016-01-21 16:41 ` Cédric Le Goater
2016-01-21 16:51 ` Marcel Apfelbaum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.