Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-21 Thread Marcel Apfelbaum 

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 
---
   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_ID0x01
   #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.

Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-21 Thread Cédric Le Goater 
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 
 ---
   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_ID0x01
   #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.

Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-21 Thread Corey Minyard 

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 
---
  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_ID0x01
  #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(>uuid, qemu_uuid, 16);
+} else {
+memset(>uuid, 0, 16);
+}
+
  ipmi_init_sensors_from_sdrs(ibs);
  register_cmds(ibs);

Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-18 Thread Cédric Le Goater 
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_ID0x01
>>>   #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(>uuid, qemu_uuid, 16);
>>> +} else {
>>> +memset(>uuid, 0, 16);
>>> +}
>>> +
>>>   ipmi_init_sensors_from_sdrs(ibs);
>>>   register_cmds(ibs);
>>>
>>>
>>
>

Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-17 Thread Marcel Apfelbaum 

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_ID0x01
  #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(>uuid, qemu_uuid, 16);
+} else {
+memset(>uuid, 0, 16);
+}
+
  ipmi_init_sensors_from_sdrs(ibs);
  register_cmds(ibs);

Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-17 Thread Marcel Apfelbaum 

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_ID0x01
  #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(>uuid, qemu_uuid, 16);
+} else {
+memset(>uuid, 0, 16);
+}
+
  ipmi_init_sensors_from_sdrs(ibs);
  register_cmds(ibs);

Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-17 Thread Michael S. Tsirkin 
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 
> >---
> >  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_ID0x01
> >  #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(>uuid, qemu_uuid, 16);
> >+} else {
> >+memset(>uuid, 0, 16);
> >+}
> >+
> >  ipmi_init_sensors_from_sdrs(ibs);
> >  register_cmds(ibs);
> >
> >

Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-11 Thread Cédric Le Goater 
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 
>> ---
>>   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_ID0x01
>>   #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(>uuid, qemu_uuid, 16);
>> +} else {
>> +memset(>uuid, 0, 16);
>> +}
>> +
>>   ipmi_init_sensors_from_sdrs(ibs);
>>   register_cmds(ibs);
>>   
>

Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-08 Thread Corey Minyard 

On 01/05/2016 11:29 AM, 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_ID0x01
  #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(>uuid, qemu_uuid, 16);
+} else {
+memset(>uuid, 0, 16);
+}
+
  ipmi_init_sensors_from_sdrs(ibs);
  register_cmds(ibs);

[Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

2016-01-05 Thread Cédric Le Goater 
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_ID0x01
 #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(>uuid, qemu_uuid, 16);
+} else {
+memset(>uuid, 0, 16);
+}
+
 ipmi_init_sensors_from_sdrs(ibs);
 register_cmds(ibs);
 
-- 
2.1.4