[Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC simulator (round 2)

[Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC simulator (round 2)

[Cédric Le Goater]

The first patches are cleanups and prepare ground for an extension of
the BMC simulator providing a SDR loader using a file. A simple FRU
support comes next.

The last patches introduce APIs to populate a guest device tree with
the available sensors and to generate events, which is needed by
platforms in some occasions.

Based on e4a096b1cd43 and also available here  :

  https://github.com/legoater/qemu/commits/ipmi

Thanks,

C.

Cédric Le Goater (8):
  ipmi: add a realize function to the device class
  ipmi: use a function to initialize the SDR table
  ipmi: remove the need of an ending record in the SDR table
  ipmi: add some local variables in ipmi_sdr_init
  ipmi: use a file to load SDRs
  ipmi: provide support for FRUs
  ipmi: introduce an ipmi_bmc_sdr_find() API
  ipmi: introduce an ipmi_bmc_gen_event() API

 hw/ipmi/ipmi_bmc_sim.c | 256 +++++++++++++++++++++++++++++++++++++++++++------
 include/hw/ipmi/ipmi.h |   4 +
 2 files changed, 233 insertions(+), 27 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/8] ipmi: add a realize function to the device class

[Cédric Le Goater]

This will be useful to define and use properties when the object is
instanciated.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index e1ad19b8db6e..13171336f7f1 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1731,9 +1731,9 @@ static const VMStateDescription vmstate_ipmi_sim = {
     }
 };
 
-static void ipmi_sim_init(Object *obj)
+static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 {
-    IPMIBmc *b = IPMI_BMC(obj);
+    IPMIBmc *b = IPMI_BMC(dev);
     unsigned int i;
     unsigned int recid;
     IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
@@ -1790,10 +1790,17 @@ static void ipmi_sim_init(Object *obj)
     vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
 }
 
+static Property ipmi_sim_properties[] = {
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void ipmi_sim_class_init(ObjectClass *oc, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(oc);
     IPMIBmcClass *bk = IPMI_BMC_CLASS(oc);
 
+    dc->realize = ipmi_sim_realize;
+    dc->props = ipmi_sim_properties;
     bk->handle_command = ipmi_sim_handle_command;
 }
 
@@ -1801,7 +1808,6 @@ static const TypeInfo ipmi_sim_type = {
     .name          = TYPE_IPMI_BMC_SIMULATOR,
     .parent        = TYPE_IPMI_BMC,
     .instance_size = sizeof(IPMIBmcSim),
-    .instance_init = ipmi_sim_init,
     .class_init    = ipmi_sim_class_init,
 };
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/8] ipmi: add a realize function to the device class

[Marcel Apfelbaum]

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
> This will be useful to define and use properties when the object is
> instanciated.

Hi,

/s/instanciated/instantiated

>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index e1ad19b8db6e..13171336f7f1 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -1731,9 +1731,9 @@ static const VMStateDescription vmstate_ipmi_sim = {
>       }
>   };
>
> -static void ipmi_sim_init(Object *obj)
> +static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>   {
> -    IPMIBmc *b = IPMI_BMC(obj);
> +    IPMIBmc *b = IPMI_BMC(dev);
>       unsigned int i;
>       unsigned int recid;
>       IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
> @@ -1790,10 +1790,17 @@ static void ipmi_sim_init(Object *obj)
>       vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
>   }
>
> +static Property ipmi_sim_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),
> +};

There is no need to add an empty property list.
You should add it in patch 5 together with the first property.

Besides that,

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel


> +
>   static void ipmi_sim_class_init(ObjectClass *oc, void *data)
>   {
> +    DeviceClass *dc = DEVICE_CLASS(oc);
>       IPMIBmcClass *bk = IPMI_BMC_CLASS(oc);
>
> +    dc->realize = ipmi_sim_realize;
> +    dc->props = ipmi_sim_properties;
>       bk->handle_command = ipmi_sim_handle_command;
>   }
>
> @@ -1801,7 +1808,6 @@ static const TypeInfo ipmi_sim_type = {
>       .name          = TYPE_IPMI_BMC_SIMULATOR,
>       .parent        = TYPE_IPMI_BMC,
>       .instance_size = sizeof(IPMIBmcSim),
> -    .instance_init = ipmi_sim_init,
>       .class_init    = ipmi_sim_class_init,
>   };
>
>

[Qemu-devel] [PATCH 2/8] ipmi: use a function to initialize the SDR table

[Cédric Le Goater]

This patch does not change anything. It only moves the code section
initializing the sdrs in its own routine and prepares ground for the
subsequent patches.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 13171336f7f1..0abc9cb5de94 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1703,6 +1703,33 @@ static const uint8_t init_sdrs[] = {
     0xff, 0xff, 0x00, 0x00, 0x00
 };
 
+static void ipmi_sdr_init(IPMIBmcSim *ibs)
+{
+    unsigned int i;
+    unsigned int recid;
+
+    for (i = 0;;) {
+        struct ipmi_sdr_header *sdrh;
+        int len;
+        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
+            error_report("Problem with recid 0x%4.4x", i);
+            return;
+        }
+        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
+        len = ipmi_sdr_length(sdrh);
+        recid = ipmi_sdr_recid(sdrh);
+        if (recid == 0xffff) {
+            break;
+        }
+        if ((i + len) > sizeof(init_sdrs)) {
+            error_report("Problem with recid 0x%4.4x", i);
+            return;
+        }
+        sdr_add_entry(ibs, sdrh, len, NULL);
+        i += len;
+    }
+}
+
 static const VMStateDescription vmstate_ipmi_sim = {
     .name = TYPE_IPMI_BMC_SIMULATOR,
     .version_id = 1,
@@ -1735,7 +1762,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 {
     IPMIBmc *b = IPMI_BMC(dev);
     unsigned int i;
-    unsigned int recid;
     IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
 
     qemu_mutex_init(&ibs->lock);
@@ -1752,26 +1778,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
         ibs->sdr.last_clear[i] = 0xff;
     }
 
-    for (i = 0;;) {
-        struct ipmi_sdr_header *sdrh;
-        int len;
-        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
-            error_report("Problem with recid 0x%4.4x", i);
-            return;
-        }
-        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
-        len = ipmi_sdr_length(sdrh);
-        recid = ipmi_sdr_recid(sdrh);
-        if (recid == 0xffff) {
-            break;
-        }
-        if ((i + len) > sizeof(init_sdrs)) {
-            error_report("Problem with recid 0x%4.4x", i);
-            return;
-        }
-        sdr_add_entry(ibs, sdrh, len, NULL);
-        i += len;
-    }
+    ipmi_sdr_init(ibs);
 
     ibs->acpi_power_state[0] = 0;
     ibs->acpi_power_state[1] = 0;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/8] ipmi: use a function to initialize the SDR table

[Marcel Apfelbaum]

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
> This patch does not change anything.

Hi,

Well, it changes *something*, otherwise ... :)

Maybe "This is only a re-factoring."



  It only moves the code section
> initializing the sdrs in its own routine and prepares ground for the
> subsequent patches.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 49 ++++++++++++++++++++++++++++---------------------
>   1 file changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 13171336f7f1..0abc9cb5de94 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -1703,6 +1703,33 @@ static const uint8_t init_sdrs[] = {
>       0xff, 0xff, 0x00, 0x00, 0x00
>   };
>
> +static void ipmi_sdr_init(IPMIBmcSim *ibs)
> +{
> +    unsigned int i;
> +    unsigned int recid;
> +
> +    for (i = 0;;) {


I am all for extracting the functionality in a function,
what bothers me a little is the above construct.

Maybe you can find a more "standard: approach like:

  while (recid != 0xffff) {
      ...
  }

Or maybe is just me.

Thanks,
Marcel

> +        struct ipmi_sdr_header *sdrh;
> +        int len;
> +        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
> +            error_report("Problem with recid 0x%4.4x", i);
> +            return;
> +        }
> +        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
> +        len = ipmi_sdr_length(sdrh);
> +        recid = ipmi_sdr_recid(sdrh);
> +        if (recid == 0xffff) {
> +            break;
> +        }
> +        if ((i + len) > sizeof(init_sdrs)) {
> +            error_report("Problem with recid 0x%4.4x", i);
> +            return;
> +        }
> +        sdr_add_entry(ibs, sdrh, len, NULL);
> +        i += len;
> +    }
> +}
> +
>   static const VMStateDescription vmstate_ipmi_sim = {
>       .name = TYPE_IPMI_BMC_SIMULATOR,
>       .version_id = 1,
> @@ -1735,7 +1762,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>   {
>       IPMIBmc *b = IPMI_BMC(dev);
>       unsigned int i;
> -    unsigned int recid;
>       IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
>
>       qemu_mutex_init(&ibs->lock);
> @@ -1752,26 +1778,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>           ibs->sdr.last_clear[i] = 0xff;
>       }
>
> -    for (i = 0;;) {
> -        struct ipmi_sdr_header *sdrh;
> -        int len;
> -        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
> -            error_report("Problem with recid 0x%4.4x", i);
> -            return;
> -        }
> -        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
> -        len = ipmi_sdr_length(sdrh);
> -        recid = ipmi_sdr_recid(sdrh);
> -        if (recid == 0xffff) {
> -            break;
> -        }
> -        if ((i + len) > sizeof(init_sdrs)) {
> -            error_report("Problem with recid 0x%4.4x", i);
> -            return;
> -        }
> -        sdr_add_entry(ibs, sdrh, len, NULL);
> -        i += len;
> -    }
> +    ipmi_sdr_init(ibs);
>
>       ibs->acpi_power_state[0] = 0;
>       ibs->acpi_power_state[1] = 0;
>

Re: [Qemu-devel] [PATCH 2/8] ipmi: use a function to initialize the SDR table

[Cédric Le Goater]

On 02/14/2016 09:31 AM, Marcel Apfelbaum wrote:
> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>> This patch does not change anything.
> 
> Hi,
> 
> Well, it changes *something*, otherwise ... :)
> 
> Maybe "This is only a re-factoring."

Yes. This is much better. "does not change anything" doe not make sense ...

C.

> 
> 
> 
>  It only moves the code section
>> initializing the sdrs in its own routine and prepares ground for the
>> subsequent patches.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 49 ++++++++++++++++++++++++++++---------------------
>>   1 file changed, 28 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index 13171336f7f1..0abc9cb5de94 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -1703,6 +1703,33 @@ static const uint8_t init_sdrs[] = {
>>       0xff, 0xff, 0x00, 0x00, 0x00
>>   };
>>
>> +static void ipmi_sdr_init(IPMIBmcSim *ibs)
>> +{
>> +    unsigned int i;
>> +    unsigned int recid;
>> +
>> +    for (i = 0;;) {
> 
> 
> I am all for extracting the functionality in a function,
> what bothers me a little is the above construct.
> 
> Maybe you can find a more "standard: approach like:
> 
>  while (recid != 0xffff) {
>      ...
>  }
> 
> Or maybe is just me.
> 
> Thanks,
> Marcel
> 
>> +        struct ipmi_sdr_header *sdrh;
>> +        int len;
>> +        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
>> +            error_report("Problem with recid 0x%4.4x", i);
>> +            return;
>> +        }
>> +        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
>> +        len = ipmi_sdr_length(sdrh);
>> +        recid = ipmi_sdr_recid(sdrh);
>> +        if (recid == 0xffff) {
>> +            break;
>> +        }
>> +        if ((i + len) > sizeof(init_sdrs)) {
>> +            error_report("Problem with recid 0x%4.4x", i);
>> +            return;
>> +        }
>> +        sdr_add_entry(ibs, sdrh, len, NULL);
>> +        i += len;
>> +    }
>> +}
>> +
>>   static const VMStateDescription vmstate_ipmi_sim = {
>>       .name = TYPE_IPMI_BMC_SIMULATOR,
>>       .version_id = 1,
>> @@ -1735,7 +1762,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>   {
>>       IPMIBmc *b = IPMI_BMC(dev);
>>       unsigned int i;
>> -    unsigned int recid;
>>       IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
>>
>>       qemu_mutex_init(&ibs->lock);
>> @@ -1752,26 +1778,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>           ibs->sdr.last_clear[i] = 0xff;
>>       }
>>
>> -    for (i = 0;;) {
>> -        struct ipmi_sdr_header *sdrh;
>> -        int len;
>> -        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
>> -            error_report("Problem with recid 0x%4.4x", i);
>> -            return;
>> -        }
>> -        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
>> -        len = ipmi_sdr_length(sdrh);
>> -        recid = ipmi_sdr_recid(sdrh);
>> -        if (recid == 0xffff) {
>> -            break;
>> -        }
>> -        if ((i + len) > sizeof(init_sdrs)) {
>> -            error_report("Problem with recid 0x%4.4x", i);
>> -            return;
>> -        }
>> -        sdr_add_entry(ibs, sdrh, len, NULL);
>> -        i += len;
>> -    }
>> +    ipmi_sdr_init(ibs);
>>
>>       ibs->acpi_power_state[0] = 0;
>>       ibs->acpi_power_state[1] = 0;
>>
> 

[Qemu-devel] [PATCH 3/8] ipmi: remove the need of an ending record in the SDR table

[Cédric Le Goater]

Currently, the code initializing the sdr table relies on an ending
record with a recid of 0xffff. This patch changes the loop to use the
sdr size as a breaking condition.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 0abc9cb5de94..f219bfc7a2f3 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1699,34 +1699,27 @@ static const uint8_t init_sdrs[] = {
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc8,
     'W',  'a',  't',  'c',  'h',  'd',  'o',  'g',
-    /* End */
-    0xff, 0xff, 0x00, 0x00, 0x00
 };
 
 static void ipmi_sdr_init(IPMIBmcSim *ibs)
 {
     unsigned int i;
-    unsigned int recid;
+    int len;
 
-    for (i = 0;;) {
+    for (i = 0; i < sizeof(init_sdrs); i += len) {
         struct ipmi_sdr_header *sdrh;
-        int len;
+
         if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
             error_report("Problem with recid 0x%4.4x", i);
             return;
         }
         sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
         len = ipmi_sdr_length(sdrh);
-        recid = ipmi_sdr_recid(sdrh);
-        if (recid == 0xffff) {
-            break;
-        }
         if ((i + len) > sizeof(init_sdrs)) {
             error_report("Problem with recid 0x%4.4x", i);
             return;
         }
         sdr_add_entry(ibs, sdrh, len, NULL);
-        i += len;
     }
 }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 3/8] ipmi: remove the need of an ending record in the SDR table

[Marcel Apfelbaum]

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
> Currently, the code initializing the sdr table relies on an ending
> record with a recid of 0xffff. This patch changes the loop to use the
> sdr size as a breaking condition.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 0abc9cb5de94..f219bfc7a2f3 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -1699,34 +1699,27 @@ static const uint8_t init_sdrs[] = {
>       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc8,
>       'W',  'a',  't',  'c',  'h',  'd',  'o',  'g',
> -    /* End */
> -    0xff, 0xff, 0x00, 0x00, 0x00
>   };
>
>   static void ipmi_sdr_init(IPMIBmcSim *ibs)
>   {
>       unsigned int i;
> -    unsigned int recid;
> +    int len;
>
> -    for (i = 0;;) {
> +    for (i = 0; i < sizeof(init_sdrs); i += len) {


Hi,

This is much better, maybe you can squash it into the previous
patch, but I understand if you want to separate re-factoring and new code.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>



>           struct ipmi_sdr_header *sdrh;
> -        int len;
> +
>           if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
>               error_report("Problem with recid 0x%4.4x", i);
>               return;
>           }
>           sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
>           len = ipmi_sdr_length(sdrh);
> -        recid = ipmi_sdr_recid(sdrh);
> -        if (recid == 0xffff) {
> -            break;
> -        }
>           if ((i + len) > sizeof(init_sdrs)) {
>               error_report("Problem with recid 0x%4.4x", i);
>               return;
>           }
>           sdr_add_entry(ibs, sdrh, len, NULL);
> -        i += len;
>       }
>   }
>
>

[Qemu-devel] [PATCH 4/8] ipmi: add some local variables in ipmi_sdr_init

[Cédric Le Goater]

This patch adds a couple of variables to manipulate the raw sdr
entries. The const attribute is also removed on init_sdrs. This will
ease the introduction of a sdr loader using a file.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index f219bfc7a2f3..aff818cf22ab 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1692,7 +1692,7 @@ static void register_cmds(IPMIBmcSim *s)
     ipmi_register_netfn(s, IPMI_NETFN_STORAGE, &storage_netfn);
 }
 
-static const uint8_t init_sdrs[] = {
+static uint8_t init_sdrs[] = {
     /* Watchdog device */
     0x00, 0x00, 0x51, 0x02,   35, 0x20, 0x00, 0x00,
     0x23, 0x01, 0x63, 0x00, 0x23, 0x6f, 0x0f, 0x01,
@@ -1705,17 +1705,22 @@ static void ipmi_sdr_init(IPMIBmcSim *ibs)
 {
     unsigned int i;
     int len;
+    size_t sdrs_size;
+    uint8_t *sdrs;
 
-    for (i = 0; i < sizeof(init_sdrs); i += len) {
+    sdrs_size = sizeof(init_sdrs);
+    sdrs = init_sdrs;
+
+    for (i = 0; i < sdrs_size; i += len) {
         struct ipmi_sdr_header *sdrh;
 
-        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
+        if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
             error_report("Problem with recid 0x%4.4x", i);
             return;
         }
-        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
+        sdrh = (struct ipmi_sdr_header *) &sdrs[i];
         len = ipmi_sdr_length(sdrh);
-        if ((i + len) > sizeof(init_sdrs)) {
+        if (i + len > sdrs_size) {
             error_report("Problem with recid 0x%4.4x", i);
             return;
         }
-- 
2.1.4

Re: [Qemu-devel] [PATCH 4/8] ipmi: add some local variables in ipmi_sdr_init

[Marcel Apfelbaum]

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
> This patch adds a couple of variables to manipulate the raw sdr
> entries. The const attribute is also removed on init_sdrs. This will
> ease the introduction of a sdr loader using a file.

Hi,

You could remove the const attribute when you have to, anyway

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel


>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index f219bfc7a2f3..aff818cf22ab 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -1692,7 +1692,7 @@ static void register_cmds(IPMIBmcSim *s)
>       ipmi_register_netfn(s, IPMI_NETFN_STORAGE, &storage_netfn);
>   }
>
> -static const uint8_t init_sdrs[] = {
> +static uint8_t init_sdrs[] = {
>       /* Watchdog device */
>       0x00, 0x00, 0x51, 0x02,   35, 0x20, 0x00, 0x00,
>       0x23, 0x01, 0x63, 0x00, 0x23, 0x6f, 0x0f, 0x01,
> @@ -1705,17 +1705,22 @@ static void ipmi_sdr_init(IPMIBmcSim *ibs)
>   {
>       unsigned int i;
>       int len;
> +    size_t sdrs_size;
> +    uint8_t *sdrs;
>
> -    for (i = 0; i < sizeof(init_sdrs); i += len) {
> +    sdrs_size = sizeof(init_sdrs);
> +    sdrs = init_sdrs;
> +
> +    for (i = 0; i < sdrs_size; i += len) {
>           struct ipmi_sdr_header *sdrh;
>
> -        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
> +        if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
>               error_report("Problem with recid 0x%4.4x", i);
>               return;
>           }
> -        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
> +        sdrh = (struct ipmi_sdr_header *) &sdrs[i];
>           len = ipmi_sdr_length(sdrh);
> -        if ((i + len) > sizeof(init_sdrs)) {
> +        if (i + len > sdrs_size) {
>               error_report("Problem with recid 0x%4.4x", i);
>               return;
>           }
>

Re: [Qemu-devel] [PATCH 4/8] ipmi: add some local variables in ipmi_sdr_init

[Cédric Le Goater]

On 02/14/2016 09:55 AM, Marcel Apfelbaum wrote:
> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>> This patch adds a couple of variables to manipulate the raw sdr
>> entries. The const attribute is also removed on init_sdrs. This will
>> ease the introduction of a sdr loader using a file.
> 
> Hi,
> 
> You could remove the const attribute when you have to, anyway

OK. I can do that in the next patch.

Thanks,

C. 

> 
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Thanks,
> Marcel
> 
> 
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index f219bfc7a2f3..aff818cf22ab 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -1692,7 +1692,7 @@ static void register_cmds(IPMIBmcSim *s)
>>       ipmi_register_netfn(s, IPMI_NETFN_STORAGE, &storage_netfn);
>>   }
>>
>> -static const uint8_t init_sdrs[] = {
>> +static uint8_t init_sdrs[] = {
>>       /* Watchdog device */
>>       0x00, 0x00, 0x51, 0x02,   35, 0x20, 0x00, 0x00,
>>       0x23, 0x01, 0x63, 0x00, 0x23, 0x6f, 0x0f, 0x01,
>> @@ -1705,17 +1705,22 @@ static void ipmi_sdr_init(IPMIBmcSim *ibs)
>>   {
>>       unsigned int i;
>>       int len;
>> +    size_t sdrs_size;
>> +    uint8_t *sdrs;
>>
>> -    for (i = 0; i < sizeof(init_sdrs); i += len) {
>> +    sdrs_size = sizeof(init_sdrs);
>> +    sdrs = init_sdrs;
>> +
>> +    for (i = 0; i < sdrs_size; i += len) {
>>           struct ipmi_sdr_header *sdrh;
>>
>> -        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
>> +        if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
>>               error_report("Problem with recid 0x%4.4x", i);
>>               return;
>>           }
>> -        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
>> +        sdrh = (struct ipmi_sdr_header *) &sdrs[i];
>>           len = ipmi_sdr_length(sdrh);
>> -        if ((i + len) > sizeof(init_sdrs)) {
>> +        if (i + len > sdrs_size) {
>>               error_report("Problem with recid 0x%4.4x", i);
>>               return;
>>           }
>>
> 
> 

[Qemu-devel] [PATCH 5/8] ipmi: use a file to load SDRs

[Cédric Le Goater]

The IPMI BMC simulator populates the sdr/sensor tables with a minimal
set of entries (Watchdog). But some qemu platforms might want to use
extra entries for their custom needs.

This patch modifies slighty the initializing routine to take into
account a larger set read from a file. The name of the file to use is
defined through a new 'sdr' property of the simulator device.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index aff818cf22ab..69318eb6b556 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -27,6 +27,7 @@
 #include "qemu/timer.h"
 #include "hw/ipmi/ipmi.h"
 #include "qemu/error-report.h"
+#include "hw/loader.h"
 
 #define IPMI_NETFN_CHASSIS            0x00
 
@@ -208,6 +209,7 @@ struct IPMIBmcSim {
     IPMISel sel;
     IPMISdr sdr;
     IPMISensor sensors[MAX_SENSORS];
+    char *sdr_filename;
 
     /* Odd netfns are for responses, so we only need the even ones. */
     const IPMINetfn *netfns[MAX_NETFNS / 2];
@@ -1708,24 +1710,32 @@ static void ipmi_sdr_init(IPMIBmcSim *ibs)
     size_t sdrs_size;
     uint8_t *sdrs;
 
-    sdrs_size = sizeof(init_sdrs);
-    sdrs = init_sdrs;
+    if (!ibs->sdr_filename ||
+        !g_file_get_contents(ibs->sdr_filename, (gchar **) &sdrs, &sdrs_size,
+                            NULL)) {
+        sdrs_size = sizeof(init_sdrs);
+        sdrs = init_sdrs;
+    }
 
     for (i = 0; i < sdrs_size; i += len) {
         struct ipmi_sdr_header *sdrh;
 
         if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
             error_report("Problem with recid 0x%4.4x", i);
-            return;
+            break;
         }
         sdrh = (struct ipmi_sdr_header *) &sdrs[i];
         len = ipmi_sdr_length(sdrh);
         if (i + len > sdrs_size) {
             error_report("Problem with recid 0x%4.4x", i);
-            return;
+            break;
         }
         sdr_add_entry(ibs, sdrh, len, NULL);
     }
+
+    if (sdrs != init_sdrs) {
+        g_free(sdrs);
+    }
 }
 
 static const VMStateDescription vmstate_ipmi_sim = {
@@ -1796,6 +1806,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 }
 
 static Property ipmi_sim_properties[] = {
+    DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 5/8] ipmi: use a file to load SDRs

[Marcel Apfelbaum]

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
> The IPMI BMC simulator populates the sdr/sensor tables with a minimal
> set of entries (Watchdog). But some qemu platforms might want to use
> extra entries for their custom needs.
>
> This patch modifies slighty the initializing routine to take into
> account a larger set read from a file. The name of the file to use is
> defined through a new 'sdr' property of the simulator device.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index aff818cf22ab..69318eb6b556 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -27,6 +27,7 @@
>   #include "qemu/timer.h"
>   #include "hw/ipmi/ipmi.h"
>   #include "qemu/error-report.h"
> +#include "hw/loader.h"
>
>   #define IPMI_NETFN_CHASSIS            0x00
>
> @@ -208,6 +209,7 @@ struct IPMIBmcSim {
>       IPMISel sel;
>       IPMISdr sdr;
>       IPMISensor sensors[MAX_SENSORS];
> +    char *sdr_filename;
>
>       /* Odd netfns are for responses, so we only need the even ones. */
>       const IPMINetfn *netfns[MAX_NETFNS / 2];
> @@ -1708,24 +1710,32 @@ static void ipmi_sdr_init(IPMIBmcSim *ibs)
>       size_t sdrs_size;
>       uint8_t *sdrs;
>
> -    sdrs_size = sizeof(init_sdrs);
> -    sdrs = init_sdrs;
> +    if (!ibs->sdr_filename ||
> +        !g_file_get_contents(ibs->sdr_filename, (gchar **) &sdrs, &sdrs_size,
> +                            NULL)) {

Hi,

If the file exists but you cannot read it you may want at least to
warn the user. He may think the contents are read from the file.

Other than that and the change of the property name (as suggested in this mail thread

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel


> +        sdrs_size = sizeof(init_sdrs);
> +        sdrs = init_sdrs;
> +    }
>
>       for (i = 0; i < sdrs_size; i += len) {
>           struct ipmi_sdr_header *sdrh;
>
>           if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
>               error_report("Problem with recid 0x%4.4x", i);
> -            return;
> +            break;
>           }
>           sdrh = (struct ipmi_sdr_header *) &sdrs[i];
>           len = ipmi_sdr_length(sdrh);
>           if (i + len > sdrs_size) {
>               error_report("Problem with recid 0x%4.4x", i);
> -            return;
> +            break;
>           }
>           sdr_add_entry(ibs, sdrh, len, NULL);
>       }
> +
> +    if (sdrs != init_sdrs) {
> +        g_free(sdrs);
> +    }
>   }
>
>   static const VMStateDescription vmstate_ipmi_sim = {
> @@ -1796,6 +1806,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>   }
>
>   static Property ipmi_sim_properties[] = {
> +    DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>
>

Re: [Qemu-devel] [PATCH 5/8] ipmi: use a file to load SDRs

[Cédric Le Goater]

On 02/14/2016 10:08 AM, Marcel Apfelbaum wrote:
> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>> The IPMI BMC simulator populates the sdr/sensor tables with a minimal
>> set of entries (Watchdog). But some qemu platforms might want to use
>> extra entries for their custom needs.
>>
>> This patch modifies slighty the initializing routine to take into
>> account a larger set read from a file. The name of the file to use is
>> defined through a new 'sdr' property of the simulator device.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index aff818cf22ab..69318eb6b556 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -27,6 +27,7 @@
>>   #include "qemu/timer.h"
>>   #include "hw/ipmi/ipmi.h"
>>   #include "qemu/error-report.h"
>> +#include "hw/loader.h"
>>
>>   #define IPMI_NETFN_CHASSIS            0x00
>>
>> @@ -208,6 +209,7 @@ struct IPMIBmcSim {
>>       IPMISel sel;
>>       IPMISdr sdr;
>>       IPMISensor sensors[MAX_SENSORS];
>> +    char *sdr_filename;
>>
>>       /* Odd netfns are for responses, so we only need the even ones. */
>>       const IPMINetfn *netfns[MAX_NETFNS / 2];
>> @@ -1708,24 +1710,32 @@ static void ipmi_sdr_init(IPMIBmcSim *ibs)
>>       size_t sdrs_size;
>>       uint8_t *sdrs;
>>
>> -    sdrs_size = sizeof(init_sdrs);
>> -    sdrs = init_sdrs;
>> +    if (!ibs->sdr_filename ||
>> +        !g_file_get_contents(ibs->sdr_filename, (gchar **) &sdrs, &sdrs_size,
>> +                            NULL)) {
> 
> Hi,
> 
> If the file exists but you cannot read it you may want at least to
> warn the user. He may think the contents are read from the file.

In fact, I did in an early version of the patch and then I removed it 
because I thought qemu did not have to complain for not fatal errors. 

I will add a error_report().

Thanks,

C. 



> Other than that and the change of the property name (as suggested in this mail thread
> 
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Thanks,
> Marcel
> 
> 
>> +        sdrs_size = sizeof(init_sdrs);
>> +        sdrs = init_sdrs;
>> +    }
>>
>>       for (i = 0; i < sdrs_size; i += len) {
>>           struct ipmi_sdr_header *sdrh;
>>
>>           if (i + IPMI_SDR_HEADER_SIZE > sdrs_size) {
>>               error_report("Problem with recid 0x%4.4x", i);
>> -            return;
>> +            break;
>>           }
>>           sdrh = (struct ipmi_sdr_header *) &sdrs[i];
>>           len = ipmi_sdr_length(sdrh);
>>           if (i + len > sdrs_size) {
>>               error_report("Problem with recid 0x%4.4x", i);
>> -            return;
>> +            break;
>>           }
>>           sdr_add_entry(ibs, sdrh, len, NULL);
>>       }
>> +
>> +    if (sdrs != init_sdrs) {
>> +        g_free(sdrs);
>> +    }
>>   }
>>
>>   static const VMStateDescription vmstate_ipmi_sim = {
>> @@ -1796,6 +1806,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>   }
>>
>>   static Property ipmi_sim_properties[] = {
>> +    DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>
>>
> 

[Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs

[Cédric Le Goater]

This patch provides a simple FRU support for the BMC simulator. FRUs
are loaded from a file which name is specified in the object
properties, each entry having a fixed size, also specified in the
properties. If the file is unknown or not accessible for some reason,
a unique entry of 1024 bytes is created as a default. Just enough to
start some simulation.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 69318eb6b556..b0754893fc08 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -80,6 +80,9 @@
 #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
 #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
 #define IPMI_CMD_RUN_INIT_AGENT           0x2C
+#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
+#define IPMI_CMD_READ_FRU_DATA            0x11
+#define IPMI_CMD_WRITE_FRU_DATA           0x12
 #define IPMI_CMD_GET_SEL_INFO             0x40
 #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
 #define IPMI_CMD_RESERVE_SEL              0x42
@@ -122,6 +125,13 @@ typedef struct IPMISdr {
     uint8_t overflow;
 } IPMISdr;
 
+typedef struct IPMIFru {
+    char *filename;
+    unsigned int nentries;
+    uint16_t size;
+    uint8_t *data;
+} IPMIFru;
+
 typedef struct IPMISensor {
     uint8_t status;
     uint8_t reading;
@@ -208,6 +218,7 @@ struct IPMIBmcSim {
 
     IPMISel sel;
     IPMISdr sdr;
+    IPMIFru fru;
     IPMISensor sensors[MAX_SENSORS];
     char *sdr_filename;
 
@@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs,
     IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
 }
 
+static void get_fru_area_info(IPMIBmcSim *ibs,
+                         uint8_t *cmd, unsigned int cmd_len,
+                         uint8_t *rsp, unsigned int *rsp_len,
+                         unsigned int max_rsp_len)
+{
+    uint8_t fruid;
+    uint16_t fru_entry_size;
+
+    IPMI_CHECK_CMD_LEN(3);
+
+    fruid = cmd[2];
+
+    if (fruid >= ibs->fru.nentries) {
+        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        return;
+    }
+
+    fru_entry_size = ibs->fru.size;
+
+    IPMI_ADD_RSP_DATA(fru_entry_size & 0xff);
+    IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff);
+    IPMI_ADD_RSP_DATA(0x0);
+}
+
+#define min(x, y) ((x) < (y) ? (x) : (y))
+#define max(x, y) ((x) > (y) ? (x) : (y))
+
+static void read_fru_data(IPMIBmcSim *ibs,
+                         uint8_t *cmd, unsigned int cmd_len,
+                         uint8_t *rsp, unsigned int *rsp_len,
+                         unsigned int max_rsp_len)
+{
+    uint8_t fruid;
+    uint16_t offset;
+    int i;
+    uint8_t *fru_entry;
+    unsigned int count;
+
+    IPMI_CHECK_CMD_LEN(5);
+
+    fruid = cmd[2];
+    offset = (cmd[3] | cmd[4] << 8);
+
+    if (fruid >= ibs->fru.nentries) {
+        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        return;
+    }
+
+    if (offset >= ibs->fru.size - 1) {
+        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        return;
+    }
+
+    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
+
+    count = min(cmd[5], ibs->fru.size - offset);
+
+    IPMI_ADD_RSP_DATA(count & 0xff);
+    for (i = 0; i < count; i++) {
+        IPMI_ADD_RSP_DATA(fru_entry[offset + i]);
+    }
+}
+
+static void write_fru_data(IPMIBmcSim *ibs,
+                         uint8_t *cmd, unsigned int cmd_len,
+                         uint8_t *rsp, unsigned int *rsp_len,
+                         unsigned int max_rsp_len)
+{
+    uint8_t fruid;
+    uint16_t offset;
+    uint8_t *fru_entry;
+    unsigned int count;
+
+    IPMI_CHECK_CMD_LEN(5);
+
+    fruid = cmd[2];
+    offset = (cmd[3] | cmd[4] << 8);
+
+    if (fruid >= ibs->fru.nentries) {
+        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        return;
+    }
+
+    if (offset >= ibs->fru.size - 1) {
+        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+        return;
+    }
+
+    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
+
+    count = min(cmd_len - 5, ibs->fru.size - offset);
+
+    memcpy(fru_entry + offset, cmd + 5, count);
+
+    IPMI_ADD_RSP_DATA(count & 0xff);
+}
+
 static void reserve_sel(IPMIBmcSim *ibs,
                         uint8_t *cmd, unsigned int cmd_len,
                         uint8_t *rsp, unsigned int *rsp_len,
@@ -1667,6 +1775,9 @@ static const IPMINetfn app_netfn = {
 };
 
 static const IPMICmdHandler storage_cmds[] = {
+    [IPMI_CMD_GET_FRU_AREA_INFO] = get_fru_area_info,
+    [IPMI_CMD_READ_FRU_DATA] = read_fru_data,
+    [IPMI_CMD_WRITE_FRU_DATA] = write_fru_data,
     [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
     [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
     [IPMI_CMD_GET_SDR] = get_sdr,
@@ -1766,6 +1877,31 @@ static const VMStateDescription vmstate_ipmi_sim = {
     }
 };
 
+static void ipmi_fru_init(IPMIFru *fru)
+{
+    int fsize;
+    int size = 0;
+
+    fsize = get_image_size(fru->filename);
+    if (fsize > 0) {
+        size = QEMU_ALIGN_UP(fsize, fru->size);
+        fru->data = g_malloc0(size);
+        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
+            error_report("Could not load file '%s'", fru->filename);
+            g_free(fru->data);
+            fru->data = NULL;
+        }
+    }
+
+    if (!fru->data) {
+        /* give one default FRU */
+        size = fru->size;
+        fru->data = g_malloc0(size);
+    }
+
+    fru->nentries = size / fru->size;
+}
+
 static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 {
     IPMIBmc *b = IPMI_BMC(dev);
@@ -1788,6 +1924,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 
     ipmi_sdr_init(ibs);
 
+    ipmi_fru_init(&ibs->fru);
+
     ibs->acpi_power_state[0] = 0;
     ibs->acpi_power_state[1] = 0;
 
@@ -1806,6 +1944,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 }
 
 static Property ipmi_sim_properties[] = {
+    DEFINE_PROP_UINT16("frusize", IPMIBmcSim, fru.size, 1024),
+    DEFINE_PROP_STRING("frufile", IPMIBmcSim, fru.filename),
     DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
2.1.4

Re: [Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs

[Marcel Apfelbaum]

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
> This patch provides a simple FRU support for the BMC simulator. FRUs
> are loaded from a file which name is specified in the object
> properties, each entry having a fixed size, also specified in the
> properties. If the file is unknown or not accessible for some reason,
> a unique entry of 1024 bytes is created as a default. Just enough to
> start some simulation.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 140 insertions(+)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 69318eb6b556..b0754893fc08 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -80,6 +80,9 @@
>   #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>   #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>   #define IPMI_CMD_RUN_INIT_AGENT           0x2C
> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
> +#define IPMI_CMD_READ_FRU_DATA            0x11
> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
>   #define IPMI_CMD_GET_SEL_INFO             0x40
>   #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>   #define IPMI_CMD_RESERVE_SEL              0x42
> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
>       uint8_t overflow;
>   } IPMISdr;
>
> +typedef struct IPMIFru {
> +    char *filename;
> +    unsigned int nentries;
> +    uint16_t size;
> +    uint8_t *data;
> +} IPMIFru;
> +
>   typedef struct IPMISensor {
>       uint8_t status;
>       uint8_t reading;
> @@ -208,6 +218,7 @@ struct IPMIBmcSim {
>
>       IPMISel sel;
>       IPMISdr sdr;
> +    IPMIFru fru;
>       IPMISensor sensors[MAX_SENSORS];
>       char *sdr_filename;
>
> @@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs,
>       IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
>   }
>
> +static void get_fru_area_info(IPMIBmcSim *ibs,
> +                         uint8_t *cmd, unsigned int cmd_len,
> +                         uint8_t *rsp, unsigned int *rsp_len,
> +                         unsigned int max_rsp_len)
> +{
> +    uint8_t fruid;
> +    uint16_t fru_entry_size;
> +
> +    IPMI_CHECK_CMD_LEN(3);

Hi,

This is little awkward for me. The cmd_len and rsp
parameters of the macro are implied.

Am I the only one this bothers?

> +
> +    fruid = cmd[2];
> +
> +    if (fruid >= ibs->fru.nentries) {
> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
> +        return;
> +    }
> +
> +    fru_entry_size = ibs->fru.size;
> +
> +    IPMI_ADD_RSP_DATA(fru_entry_size & 0xff);
> +    IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff);
> +    IPMI_ADD_RSP_DATA(0x0);

Same here. By the way, do you have some spec for the above or
is an ad-hoc encoding of the fields? If yes, you could
add a reference for the spec.(This is also for the other functions in this patch)

Thanks,
Marcel

> +}
> +
> +#define min(x, y) ((x) < (y) ? (x) : (y))
> +#define max(x, y) ((x) > (y) ? (x) : (y))
> +
> +static void read_fru_data(IPMIBmcSim *ibs,
> +                         uint8_t *cmd, unsigned int cmd_len,
> +                         uint8_t *rsp, unsigned int *rsp_len,
> +                         unsigned int max_rsp_len)
> +{
> +    uint8_t fruid;
> +    uint16_t offset;
> +    int i;
> +    uint8_t *fru_entry;
> +    unsigned int count;
> +
> +    IPMI_CHECK_CMD_LEN(5);
> +
> +    fruid = cmd[2];
> +    offset = (cmd[3] | cmd[4] << 8);
> +
> +    if (fruid >= ibs->fru.nentries) {
> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
> +        return;
> +    }
> +
> +    if (offset >= ibs->fru.size - 1) {
> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
> +        return;
> +    }
> +
> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
> +
> +    count = min(cmd[5], ibs->fru.size - offset);
> +
> +    IPMI_ADD_RSP_DATA(count & 0xff);
> +    for (i = 0; i < count; i++) {
> +        IPMI_ADD_RSP_DATA(fru_entry[offset + i]);
> +    }
> +}
> +
> +static void write_fru_data(IPMIBmcSim *ibs,
> +                         uint8_t *cmd, unsigned int cmd_len,
> +                         uint8_t *rsp, unsigned int *rsp_len,
> +                         unsigned int max_rsp_len)
> +{
> +    uint8_t fruid;
> +    uint16_t offset;
> +    uint8_t *fru_entry;
> +    unsigned int count;
> +
> +    IPMI_CHECK_CMD_LEN(5);
> +
> +    fruid = cmd[2];
> +    offset = (cmd[3] | cmd[4] << 8);
> +
> +    if (fruid >= ibs->fru.nentries) {
> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
> +        return;
> +    }
> +
> +    if (offset >= ibs->fru.size - 1) {
> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
> +        return;
> +    }
> +
> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
> +
> +    count = min(cmd_len - 5, ibs->fru.size - offset);
> +
> +    memcpy(fru_entry + offset, cmd + 5, count);
> +
> +    IPMI_ADD_RSP_DATA(count & 0xff);
> +}
> +
>   static void reserve_sel(IPMIBmcSim *ibs,
>                           uint8_t *cmd, unsigned int cmd_len,
>                           uint8_t *rsp, unsigned int *rsp_len,
> @@ -1667,6 +1775,9 @@ static const IPMINetfn app_netfn = {
>   };
>
>   static const IPMICmdHandler storage_cmds[] = {
> +    [IPMI_CMD_GET_FRU_AREA_INFO] = get_fru_area_info,
> +    [IPMI_CMD_READ_FRU_DATA] = read_fru_data,
> +    [IPMI_CMD_WRITE_FRU_DATA] = write_fru_data,
>       [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>       [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>       [IPMI_CMD_GET_SDR] = get_sdr,
> @@ -1766,6 +1877,31 @@ static const VMStateDescription vmstate_ipmi_sim = {
>       }
>   };
>
> +static void ipmi_fru_init(IPMIFru *fru)
> +{
> +    int fsize;
> +    int size = 0;
> +
> +    fsize = get_image_size(fru->filename);
> +    if (fsize > 0) {
> +        size = QEMU_ALIGN_UP(fsize, fru->size);
> +        fru->data = g_malloc0(size);
> +        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
> +            error_report("Could not load file '%s'", fru->filename);
> +            g_free(fru->data);
> +            fru->data = NULL;
> +        }
> +    }
> +
> +    if (!fru->data) {
> +        /* give one default FRU */
> +        size = fru->size;
> +        fru->data = g_malloc0(size);
> +    }
> +
> +    fru->nentries = size / fru->size;
> +}
> +
>   static void ipmi_sim_realize(DeviceState *dev, Error **errp)
652776

>   {
>       IPMIBmc *b = IPMI_BMC(dev);
> @@ -1788,6 +1924,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>
>       ipmi_sdr_init(ibs);
>
> +    ipmi_fru_init(&ibs->fru);
> +
>       ibs->acpi_power_state[0] = 0;
>       ibs->acpi_power_state[1] = 0;
>
> @@ -1806,6 +1944,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>   }
>
>   static Property ipmi_sim_properties[] = {
> +    DEFINE_PROP_UINT16("frusize", IPMIBmcSim, fru.size, 1024),
> +    DEFINE_PROP_STRING("frufile", IPMIBmcSim, fru.filename),
>       DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>

Re: [Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs

[Cédric Le Goater]

On 02/14/2016 10:25 AM, Marcel Apfelbaum wrote:
> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>> This patch provides a simple FRU support for the BMC simulator. FRUs
>> are loaded from a file which name is specified in the object
>> properties, each entry having a fixed size, also specified in the
>> properties. If the file is unknown or not accessible for some reason,
>> a unique entry of 1024 bytes is created as a default. Just enough to
>> start some simulation.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 140 insertions(+)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index 69318eb6b556..b0754893fc08 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -80,6 +80,9 @@
>>   #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>>   #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>>   #define IPMI_CMD_RUN_INIT_AGENT           0x2C
>> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
>> +#define IPMI_CMD_READ_FRU_DATA            0x11
>> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
>>   #define IPMI_CMD_GET_SEL_INFO             0x40
>>   #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>>   #define IPMI_CMD_RESERVE_SEL              0x42
>> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
>>       uint8_t overflow;
>>   } IPMISdr;
>>
>> +typedef struct IPMIFru {
>> +    char *filename;
>> +    unsigned int nentries;
>> +    uint16_t size;
>> +    uint8_t *data;
>> +} IPMIFru;
>> +
>>   typedef struct IPMISensor {
>>       uint8_t status;
>>       uint8_t reading;
>> @@ -208,6 +218,7 @@ struct IPMIBmcSim {
>>
>>       IPMISel sel;
>>       IPMISdr sdr;
>> +    IPMIFru fru;
>>       IPMISensor sensors[MAX_SENSORS];
>>       char *sdr_filename;
>>
>> @@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs,
>>       IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
>>   }
>>
>> +static void get_fru_area_info(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         uint8_t *rsp, unsigned int *rsp_len,
>> +                         unsigned int max_rsp_len)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t fru_entry_size;
>> +
>> +    IPMI_CHECK_CMD_LEN(3);
> 
> Hi,
> 
> This is little awkward for me. The cmd_len and rsp
> parameters of the macro are implied.

hmm, I am not sure what you mean. Are you concerned by that fact 
we could overflow rsp and cmd ? 

I guess we could probably move the IPMI_CHECK_CMD_LEN in the caller
of these commands and use an array of expected argument lengths.
For rsp, IPMI_ADD_RSP_DATA needs to be used to check that we are not 
exceeding max_rsp_len

> Am I the only one this bothers?
> 
>> +
>> +    fruid = cmd[2];
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>> +        return;
>> +    }
>> +
>> +    fru_entry_size = ibs->fru.size;
>> +
>> +    IPMI_ADD_RSP_DATA(fru_entry_size & 0xff);
>> +    IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff);
>> +    IPMI_ADD_RSP_DATA(0x0);
> 
> Same here. By the way, do you have some spec for the above or
> is an ad-hoc encoding of the fields? If yes, you could
> add a reference for the spec.(This is also for the other functions in this patch)

Yes I will add the reference.

Thanks,

C.

 
> Thanks,
> Marcel
> 
>> +}
>> +
>> +#define min(x, y) ((x) < (y) ? (x) : (y))
>> +#define max(x, y) ((x) > (y) ? (x) : (y))
>> +
>> +static void read_fru_data(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         uint8_t *rsp, unsigned int *rsp_len,
>> +                         unsigned int max_rsp_len)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t offset;
>> +    int i;
>> +    uint8_t *fru_entry;
>> +    unsigned int count;
>> +
>> +    IPMI_CHECK_CMD_LEN(5);
>> +
>> +    fruid = cmd[2];
>> +    offset = (cmd[3] | cmd[4] << 8);
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>> +        return;
>> +    }
>> +
>> +    if (offset >= ibs->fru.size - 1) {
>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>> +        return;
>> +    }
>> +
>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
>> +
>> +    count = min(cmd[5], ibs->fru.size - offset);
>> +
>> +    IPMI_ADD_RSP_DATA(count & 0xff);
>> +    for (i = 0; i < count; i++) {
>> +        IPMI_ADD_RSP_DATA(fru_entry[offset + i]);
>> +    }
>> +}
>> +
>> +static void write_fru_data(IPMIBmcSim *ibs,
>> +                         uint8_t *cmd, unsigned int cmd_len,
>> +                         uint8_t *rsp, unsigned int *rsp_len,
>> +                         unsigned int max_rsp_len)
>> +{
>> +    uint8_t fruid;
>> +    uint16_t offset;
>> +    uint8_t *fru_entry;
>> +    unsigned int count;
>> +
>> +    IPMI_CHECK_CMD_LEN(5);
>> +
>> +    fruid = cmd[2];
>> +    offset = (cmd[3] | cmd[4] << 8);
>> +
>> +    if (fruid >= ibs->fru.nentries) {
>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>> +        return;
>> +    }
>> +
>> +    if (offset >= ibs->fru.size - 1) {
>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>> +        return;
>> +    }
>> +
>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
>> +
>> +    count = min(cmd_len - 5, ibs->fru.size - offset);
>> +
>> +    memcpy(fru_entry + offset, cmd + 5, count);
>> +
>> +    IPMI_ADD_RSP_DATA(count & 0xff);
>> +}
>> +
>>   static void reserve_sel(IPMIBmcSim *ibs,
>>                           uint8_t *cmd, unsigned int cmd_len,
>>                           uint8_t *rsp, unsigned int *rsp_len,
>> @@ -1667,6 +1775,9 @@ static const IPMINetfn app_netfn = {
>>   };
>>
>>   static const IPMICmdHandler storage_cmds[] = {
>> +    [IPMI_CMD_GET_FRU_AREA_INFO] = get_fru_area_info,
>> +    [IPMI_CMD_READ_FRU_DATA] = read_fru_data,
>> +    [IPMI_CMD_WRITE_FRU_DATA] = write_fru_data,
>>       [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>>       [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>>       [IPMI_CMD_GET_SDR] = get_sdr,
>> @@ -1766,6 +1877,31 @@ static const VMStateDescription vmstate_ipmi_sim = {
>>       }
>>   };
>>
>> +static void ipmi_fru_init(IPMIFru *fru)
>> +{
>> +    int fsize;
>> +    int size = 0;
>> +
>> +    fsize = get_image_size(fru->filename);
>> +    if (fsize > 0) {
>> +        size = QEMU_ALIGN_UP(fsize, fru->size);
>> +        fru->data = g_malloc0(size);
>> +        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
>> +            error_report("Could not load file '%s'", fru->filename);
>> +            g_free(fru->data);
>> +            fru->data = NULL;
>> +        }
>> +    }
>> +
>> +    if (!fru->data) {
>> +        /* give one default FRU */
>> +        size = fru->size;
>> +        fru->data = g_malloc0(size);
>> +    }
>> +
>> +    fru->nentries = size / fru->size;
>> +}
>> +
>>   static void ipmi_sim_realize(DeviceState *dev, Error **errp)
> 652776
> 
>>   {
>>       IPMIBmc *b = IPMI_BMC(dev);
>> @@ -1788,6 +1924,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>
>>       ipmi_sdr_init(ibs);
>>
>> +    ipmi_fru_init(&ibs->fru);
>> +
>>       ibs->acpi_power_state[0] = 0;
>>       ibs->acpi_power_state[1] = 0;
>>
>> @@ -1806,6 +1944,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>   }
>>
>>   static Property ipmi_sim_properties[] = {
>> +    DEFINE_PROP_UINT16("frusize", IPMIBmcSim, fru.size, 1024),
>> +    DEFINE_PROP_STRING("frufile", IPMIBmcSim, fru.filename),
>>       DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>
> 

Re: [Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs

[Marcel Apfelbaum]

On 02/15/2016 07:17 PM, Cédric Le Goater wrote:
> On 02/14/2016 10:25 AM, Marcel Apfelbaum wrote:
>> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>>> This patch provides a simple FRU support for the BMC simulator. FRUs
>>> are loaded from a file which name is specified in the object
>>> properties, each entry having a fixed size, also specified in the
>>> properties. If the file is unknown or not accessible for some reason,
>>> a unique entry of 1024 bytes is created as a default. Just enough to
>>> start some simulation.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>> ---
>>>    hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 140 insertions(+)
>>>
>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>>> index 69318eb6b556..b0754893fc08 100644
>>> --- a/hw/ipmi/ipmi_bmc_sim.c
>>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>>> @@ -80,6 +80,9 @@
>>>    #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>>>    #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>>>    #define IPMI_CMD_RUN_INIT_AGENT           0x2C
>>> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
>>> +#define IPMI_CMD_READ_FRU_DATA            0x11
>>> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
>>>    #define IPMI_CMD_GET_SEL_INFO             0x40
>>>    #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>>>    #define IPMI_CMD_RESERVE_SEL              0x42
>>> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
>>>        uint8_t overflow;
>>>    } IPMISdr;
>>>
>>> +typedef struct IPMIFru {
>>> +    char *filename;
>>> +    unsigned int nentries;
>>> +    uint16_t size;
>>> +    uint8_t *data;
>>> +} IPMIFru;
>>> +
>>>    typedef struct IPMISensor {
>>>        uint8_t status;
>>>        uint8_t reading;
>>> @@ -208,6 +218,7 @@ struct IPMIBmcSim {
>>>
>>>        IPMISel sel;
>>>        IPMISdr sdr;
>>> +    IPMIFru fru;
>>>        IPMISensor sensors[MAX_SENSORS];
>>>        char *sdr_filename;
>>>
>>> @@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs,
>>>        IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
>>>    }
>>>
>>> +static void get_fru_area_info(IPMIBmcSim *ibs,
>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>> +                         unsigned int max_rsp_len)
>>> +{
>>> +    uint8_t fruid;
>>> +    uint16_t fru_entry_size;
>>> +
>>> +    IPMI_CHECK_CMD_LEN(3);
>>
>> Hi,
>>
>> This is little awkward for me. The cmd_len and rsp
>> parameters of the macro are implied.
>
> hmm, I am not sure what you mean. Are you concerned by that fact
> we could overflow rsp and cmd ?

Not really, no. Something more simple:

IPMI_CHECK_CMD_LEN(3) should be actually IPMI_CHECK_CMD_LEN(3, cmd_len, rsp).
What bothers me is that both cmd_len and rsp are implied by the macro.

In other words, we don't know what parameters IPMI_CHECK_CMD_LEN really has.
"3" is for sure not enough, so we need to guess or look a the macro definition
to see what it uses.

But again, maybe is only me.

Thanks,
Marcel


>
> I guess we could probably move the IPMI_CHECK_CMD_LEN in the caller
> of these commands and use an array of expected argument lengths.
> For rsp, IPMI_ADD_RSP_DATA needs to be used to check that we are not
> exceeding max_rsp_len
>
>> Am I the only one this bothers?
>>
>>> +
>>> +    fruid = cmd[2];
>>> +
>>> +    if (fruid >= ibs->fru.nentries) {
>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>> +        return;
>>> +    }
>>> +
>>> +    fru_entry_size = ibs->fru.size;
>>> +
>>> +    IPMI_ADD_RSP_DATA(fru_entry_size & 0xff);
>>> +    IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff);
>>> +    IPMI_ADD_RSP_DATA(0x0);
>>
>> Same here. By the way, do you have some spec for the above or
>> is an ad-hoc encoding of the fields? If yes, you could
>> add a reference for the spec.(This is also for the other functions in this patch)
>
> Yes I will add the reference.
>
> Thanks,
>
> C.
>
>
>> Thanks,
>> Marcel
>>
>>> +}
>>> +
>>> +#define min(x, y) ((x) < (y) ? (x) : (y))
>>> +#define max(x, y) ((x) > (y) ? (x) : (y))
>>> +
>>> +static void read_fru_data(IPMIBmcSim *ibs,
>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>> +                         unsigned int max_rsp_len)
>>> +{
>>> +    uint8_t fruid;
>>> +    uint16_t offset;
>>> +    int i;
>>> +    uint8_t *fru_entry;
>>> +    unsigned int count;
>>> +
>>> +    IPMI_CHECK_CMD_LEN(5);
>>> +
>>> +    fruid = cmd[2];
>>> +    offset = (cmd[3] | cmd[4] << 8);
>>> +
>>> +    if (fruid >= ibs->fru.nentries) {
>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>> +        return;
>>> +    }
>>> +
>>> +    if (offset >= ibs->fru.size - 1) {
>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>> +        return;
>>> +    }
>>> +
>>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
>>> +
>>> +    count = min(cmd[5], ibs->fru.size - offset);
>>> +
>>> +    IPMI_ADD_RSP_DATA(count & 0xff);
>>> +    for (i = 0; i < count; i++) {
>>> +        IPMI_ADD_RSP_DATA(fru_entry[offset + i]);
>>> +    }
>>> +}
>>> +
>>> +static void write_fru_data(IPMIBmcSim *ibs,
>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>> +                         unsigned int max_rsp_len)
>>> +{
>>> +    uint8_t fruid;
>>> +    uint16_t offset;
>>> +    uint8_t *fru_entry;
>>> +    unsigned int count;
>>> +
>>> +    IPMI_CHECK_CMD_LEN(5);
>>> +
>>> +    fruid = cmd[2];
>>> +    offset = (cmd[3] | cmd[4] << 8);
>>> +
>>> +    if (fruid >= ibs->fru.nentries) {
>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>> +        return;
>>> +    }
>>> +
>>> +    if (offset >= ibs->fru.size - 1) {
>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>> +        return;
>>> +    }
>>> +
>>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
>>> +
>>> +    count = min(cmd_len - 5, ibs->fru.size - offset);
>>> +
>>> +    memcpy(fru_entry + offset, cmd + 5, count);
>>> +
>>> +    IPMI_ADD_RSP_DATA(count & 0xff);
>>> +}
>>> +
>>>    static void reserve_sel(IPMIBmcSim *ibs,
>>>                            uint8_t *cmd, unsigned int cmd_len,
>>>                            uint8_t *rsp, unsigned int *rsp_len,
>>> @@ -1667,6 +1775,9 @@ static const IPMINetfn app_netfn = {
>>>    };
>>>
>>>    static const IPMICmdHandler storage_cmds[] = {
>>> +    [IPMI_CMD_GET_FRU_AREA_INFO] = get_fru_area_info,
>>> +    [IPMI_CMD_READ_FRU_DATA] = read_fru_data,
>>> +    [IPMI_CMD_WRITE_FRU_DATA] = write_fru_data,
>>>        [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>>>        [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>>>        [IPMI_CMD_GET_SDR] = get_sdr,
>>> @@ -1766,6 +1877,31 @@ static const VMStateDescription vmstate_ipmi_sim = {
>>>        }
>>>    };
>>>
>>> +static void ipmi_fru_init(IPMIFru *fru)
>>> +{
>>> +    int fsize;
>>> +    int size = 0;
>>> +
>>> +    fsize = get_image_size(fru->filename);
>>> +    if (fsize > 0) {
>>> +        size = QEMU_ALIGN_UP(fsize, fru->size);
>>> +        fru->data = g_malloc0(size);
>>> +        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
>>> +            error_report("Could not load file '%s'", fru->filename);
>>> +            g_free(fru->data);
>>> +            fru->data = NULL;
>>> +        }
>>> +    }
>>> +
>>> +    if (!fru->data) {
>>> +        /* give one default FRU */
>>> +        size = fru->size;
>>> +        fru->data = g_malloc0(size);
>>> +    }
>>> +
>>> +    fru->nentries = size / fru->size;
>>> +}
>>> +
>>>    static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>
>>
>>>    {
>>>        IPMIBmc *b = IPMI_BMC(dev);
>>> @@ -1788,6 +1924,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>>
>>>        ipmi_sdr_init(ibs);
>>>
>>> +    ipmi_fru_init(&ibs->fru);
>>> +
>>>        ibs->acpi_power_state[0] = 0;
>>>        ibs->acpi_power_state[1] = 0;
>>>
>>> @@ -1806,6 +1944,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>>    }
>>>
>>>    static Property ipmi_sim_properties[] = {
>>> +    DEFINE_PROP_UINT16("frusize", IPMIBmcSim, fru.size, 1024),
>>> +    DEFINE_PROP_STRING("frufile", IPMIBmcSim, fru.filename),
>>>        DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
>>>        DEFINE_PROP_END_OF_LIST(),
>>>    };
>>>
>>
>

Re: [Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs

[Corey Minyard]

On 02/15/2016 12:40 PM, Marcel Apfelbaum wrote:
> On 02/15/2016 07:17 PM, Cédric Le Goater wrote:
>> On 02/14/2016 10:25 AM, Marcel Apfelbaum wrote:
>>> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>>>> This patch provides a simple FRU support for the BMC simulator. FRUs
>>>> are loaded from a file which name is specified in the object
>>>> properties, each entry having a fixed size, also specified in the
>>>> properties. If the file is unknown or not accessible for some reason,
>>>> a unique entry of 1024 bytes is created as a default. Just enough to
>>>> start some simulation.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>>> ---
>>>>    hw/ipmi/ipmi_bmc_sim.c | 140 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 140 insertions(+)
>>>>
>>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>>>> index 69318eb6b556..b0754893fc08 100644
>>>> --- a/hw/ipmi/ipmi_bmc_sim.c
>>>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>>>> @@ -80,6 +80,9 @@
>>>>    #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>>>>    #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>>>>    #define IPMI_CMD_RUN_INIT_AGENT           0x2C
>>>> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
>>>> +#define IPMI_CMD_READ_FRU_DATA            0x11
>>>> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
>>>>    #define IPMI_CMD_GET_SEL_INFO             0x40
>>>>    #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>>>>    #define IPMI_CMD_RESERVE_SEL              0x42
>>>> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
>>>>        uint8_t overflow;
>>>>    } IPMISdr;
>>>>
>>>> +typedef struct IPMIFru {
>>>> +    char *filename;
>>>> +    unsigned int nentries;
>>>> +    uint16_t size;
>>>> +    uint8_t *data;
>>>> +} IPMIFru;
>>>> +
>>>>    typedef struct IPMISensor {
>>>>        uint8_t status;
>>>>        uint8_t reading;
>>>> @@ -208,6 +218,7 @@ struct IPMIBmcSim {
>>>>
>>>>        IPMISel sel;
>>>>        IPMISdr sdr;
>>>> +    IPMIFru fru;
>>>>        IPMISensor sensors[MAX_SENSORS];
>>>>        char *sdr_filename;
>>>>
>>>> @@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs,
>>>>        IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
>>>>    }
>>>>
>>>> +static void get_fru_area_info(IPMIBmcSim *ibs,
>>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>>> +                         unsigned int max_rsp_len)
>>>> +{
>>>> +    uint8_t fruid;
>>>> +    uint16_t fru_entry_size;
>>>> +
>>>> +    IPMI_CHECK_CMD_LEN(3);
>>>
>>> Hi,
>>>
>>> This is little awkward for me. The cmd_len and rsp
>>> parameters of the macro are implied.
>>
>> hmm, I am not sure what you mean. Are you concerned by that fact
>> we could overflow rsp and cmd ?
>
> Not really, no. Something more simple:
>
> IPMI_CHECK_CMD_LEN(3) should be actually IPMI_CHECK_CMD_LEN(3, 
> cmd_len, rsp).
> What bothers me is that both cmd_len and rsp are implied by the macro.

I would tend to agree.  The hidden stuff in these macros was really a 
bad idea in
hindsight.

-corey

>
> In other words, we don't know what parameters IPMI_CHECK_CMD_LEN 
> really has.
> "3" is for sure not enough, so we need to guess or look a the macro 
> definition
> to see what it uses.
>
> But again, maybe is only me.
>
> Thanks,
> Marcel
>
>
>>
>> I guess we could probably move the IPMI_CHECK_CMD_LEN in the caller
>> of these commands and use an array of expected argument lengths.
>> For rsp, IPMI_ADD_RSP_DATA needs to be used to check that we are not
>> exceeding max_rsp_len
>>
>>> Am I the only one this bothers?
>>>
>>>> +
>>>> +    fruid = cmd[2];
>>>> +
>>>> +    if (fruid >= ibs->fru.nentries) {
>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    fru_entry_size = ibs->fru.size;
>>>> +
>>>> +    IPMI_ADD_RSP_DATA(fru_entry_size & 0xff);
>>>> +    IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff);
>>>> +    IPMI_ADD_RSP_DATA(0x0);
>>>
>>> Same here. By the way, do you have some spec for the above or
>>> is an ad-hoc encoding of the fields? If yes, you could
>>> add a reference for the spec.(This is also for the other functions 
>>> in this patch)
>>
>> Yes I will add the reference.
>>
>> Thanks,
>>
>> C.
>>
>>
>>> Thanks,
>>> Marcel
>>>
>>>> +}
>>>> +
>>>> +#define min(x, y) ((x) < (y) ? (x) : (y))
>>>> +#define max(x, y) ((x) > (y) ? (x) : (y))
>>>> +
>>>> +static void read_fru_data(IPMIBmcSim *ibs,
>>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>>> +                         unsigned int max_rsp_len)
>>>> +{
>>>> +    uint8_t fruid;
>>>> +    uint16_t offset;
>>>> +    int i;
>>>> +    uint8_t *fru_entry;
>>>> +    unsigned int count;
>>>> +
>>>> +    IPMI_CHECK_CMD_LEN(5);
>>>> +
>>>> +    fruid = cmd[2];
>>>> +    offset = (cmd[3] | cmd[4] << 8);
>>>> +
>>>> +    if (fruid >= ibs->fru.nentries) {
>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (offset >= ibs->fru.size - 1) {
>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
>>>> +
>>>> +    count = min(cmd[5], ibs->fru.size - offset);
>>>> +
>>>> +    IPMI_ADD_RSP_DATA(count & 0xff);
>>>> +    for (i = 0; i < count; i++) {
>>>> +        IPMI_ADD_RSP_DATA(fru_entry[offset + i]);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void write_fru_data(IPMIBmcSim *ibs,
>>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>>> +                         unsigned int max_rsp_len)
>>>> +{
>>>> +    uint8_t fruid;
>>>> +    uint16_t offset;
>>>> +    uint8_t *fru_entry;
>>>> +    unsigned int count;
>>>> +
>>>> +    IPMI_CHECK_CMD_LEN(5);
>>>> +
>>>> +    fruid = cmd[2];
>>>> +    offset = (cmd[3] | cmd[4] << 8);
>>>> +
>>>> +    if (fruid >= ibs->fru.nentries) {
>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (offset >= ibs->fru.size - 1) {
>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
>>>> +
>>>> +    count = min(cmd_len - 5, ibs->fru.size - offset);
>>>> +
>>>> +    memcpy(fru_entry + offset, cmd + 5, count);
>>>> +
>>>> +    IPMI_ADD_RSP_DATA(count & 0xff);
>>>> +}
>>>> +
>>>>    static void reserve_sel(IPMIBmcSim *ibs,
>>>>                            uint8_t *cmd, unsigned int cmd_len,
>>>>                            uint8_t *rsp, unsigned int *rsp_len,
>>>> @@ -1667,6 +1775,9 @@ static const IPMINetfn app_netfn = {
>>>>    };
>>>>
>>>>    static const IPMICmdHandler storage_cmds[] = {
>>>> +    [IPMI_CMD_GET_FRU_AREA_INFO] = get_fru_area_info,
>>>> +    [IPMI_CMD_READ_FRU_DATA] = read_fru_data,
>>>> +    [IPMI_CMD_WRITE_FRU_DATA] = write_fru_data,
>>>>        [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>>>>        [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>>>>        [IPMI_CMD_GET_SDR] = get_sdr,
>>>> @@ -1766,6 +1877,31 @@ static const VMStateDescription 
>>>> vmstate_ipmi_sim = {
>>>>        }
>>>>    };
>>>>
>>>> +static void ipmi_fru_init(IPMIFru *fru)
>>>> +{
>>>> +    int fsize;
>>>> +    int size = 0;
>>>> +
>>>> +    fsize = get_image_size(fru->filename);
>>>> +    if (fsize > 0) {
>>>> +        size = QEMU_ALIGN_UP(fsize, fru->size);
>>>> +        fru->data = g_malloc0(size);
>>>> +        if (load_image_size(fru->filename, fru->data, fsize) != 
>>>> fsize) {
>>>> +            error_report("Could not load file '%s'", fru->filename);
>>>> +            g_free(fru->data);
>>>> +            fru->data = NULL;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (!fru->data) {
>>>> +        /* give one default FRU */
>>>> +        size = fru->size;
>>>> +        fru->data = g_malloc0(size);
>>>> +    }
>>>> +
>>>> +    fru->nentries = size / fru->size;
>>>> +}
>>>> +
>>>>    static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>>
>>>
>>>>    {
>>>>        IPMIBmc *b = IPMI_BMC(dev);
>>>> @@ -1788,6 +1924,8 @@ static void ipmi_sim_realize(DeviceState 
>>>> *dev, Error **errp)
>>>>
>>>>        ipmi_sdr_init(ibs);
>>>>
>>>> +    ipmi_fru_init(&ibs->fru);
>>>> +
>>>>        ibs->acpi_power_state[0] = 0;
>>>>        ibs->acpi_power_state[1] = 0;
>>>>
>>>> @@ -1806,6 +1944,8 @@ static void ipmi_sim_realize(DeviceState 
>>>> *dev, Error **errp)
>>>>    }
>>>>
>>>>    static Property ipmi_sim_properties[] = {
>>>> +    DEFINE_PROP_UINT16("frusize", IPMIBmcSim, fru.size, 1024),
>>>> +    DEFINE_PROP_STRING("frufile", IPMIBmcSim, fru.filename),
>>>>        DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
>>>>        DEFINE_PROP_END_OF_LIST(),
>>>>    };
>>>>
>>>
>>
>

Re: [Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs

[Cédric Le Goater]

On 02/16/2016 04:38 AM, Corey Minyard wrote:
> On 02/15/2016 12:40 PM, Marcel Apfelbaum wrote:
>> On 02/15/2016 07:17 PM, Cédric Le Goater wrote:
>>> On 02/14/2016 10:25 AM, Marcel Apfelbaum wrote:
>>>> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>>>>> This patch provides a simple FRU support for the BMC simulator. FRUs
>>>>> are loaded from a file which name is specified in the object
>>>>> properties, each entry having a fixed size, also specified in the
>>>>> properties. If the file is unknown or not accessible for some reason,
>>>>> a unique entry of 1024 bytes is created as a default. Just enough to
>>>>> start some simulation.
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>>>> ---
>>>>>    hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 140 insertions(+)
>>>>>
>>>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>>>>> index 69318eb6b556..b0754893fc08 100644
>>>>> --- a/hw/ipmi/ipmi_bmc_sim.c
>>>>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>>>>> @@ -80,6 +80,9 @@
>>>>>    #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
>>>>>    #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE    0x2B
>>>>>    #define IPMI_CMD_RUN_INIT_AGENT           0x2C
>>>>> +#define IPMI_CMD_GET_FRU_AREA_INFO        0x10
>>>>> +#define IPMI_CMD_READ_FRU_DATA            0x11
>>>>> +#define IPMI_CMD_WRITE_FRU_DATA           0x12
>>>>>    #define IPMI_CMD_GET_SEL_INFO             0x40
>>>>>    #define IPMI_CMD_GET_SEL_ALLOC_INFO       0x41
>>>>>    #define IPMI_CMD_RESERVE_SEL              0x42
>>>>> @@ -122,6 +125,13 @@ typedef struct IPMISdr {
>>>>>        uint8_t overflow;
>>>>>    } IPMISdr;
>>>>>
>>>>> +typedef struct IPMIFru {
>>>>> +    char *filename;
>>>>> +    unsigned int nentries;
>>>>> +    uint16_t size;
>>>>> +    uint8_t *data;
>>>>> +} IPMIFru;
>>>>> +
>>>>>    typedef struct IPMISensor {
>>>>>        uint8_t status;
>>>>>        uint8_t reading;
>>>>> @@ -208,6 +218,7 @@ struct IPMIBmcSim {
>>>>>
>>>>>        IPMISel sel;
>>>>>        IPMISdr sdr;
>>>>> +    IPMIFru fru;
>>>>>        IPMISensor sensors[MAX_SENSORS];
>>>>>        char *sdr_filename;
>>>>>
>>>>> @@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs,
>>>>>        IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
>>>>>    }
>>>>>
>>>>> +static void get_fru_area_info(IPMIBmcSim *ibs,
>>>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>>>> +                         unsigned int max_rsp_len)
>>>>> +{
>>>>> +    uint8_t fruid;
>>>>> +    uint16_t fru_entry_size;
>>>>> +
>>>>> +    IPMI_CHECK_CMD_LEN(3);
>>>>
>>>> Hi,
>>>>
>>>> This is little awkward for me. The cmd_len and rsp
>>>> parameters of the macro are implied.
>>>
>>> hmm, I am not sure what you mean. Are you concerned by that fact
>>> we could overflow rsp and cmd ?
>>
>> Not really, no. Something more simple:
>>
>> IPMI_CHECK_CMD_LEN(3) should be actually IPMI_CHECK_CMD_LEN(3, cmd_len, rsp).
>> What bothers me is that both cmd_len and rsp are implied by the macro.
> 
> I would tend to agree.  The hidden stuff in these macros was really a bad idea in
> hindsight.

IPMI_CHECK_CMD_LEN() could be replaced by an array of constants. 

I like the way IPMI_ADD_RSP_DATA() pushes bytes in the response 
buffer, it makes the code quite readable from the spec perspective. 
Maybe we could replace rsp and rsp_len with a simple stack-like 
struct and use one helper to push response bytes. 

It should not be too complex to cleanup. I will add a proposal in
the next round.

Thanks,

C.

> -corey
> 
>>
>> In other words, we don't know what parameters IPMI_CHECK_CMD_LEN really has.
>> "3" is for sure not enough, so we need to guess or look a the macro definition
>> to see what it uses.
>>
>> But again, maybe is only me.
>>
>> Thanks,
>> Marcel
>>
>>
>>>
>>> I guess we could probably move the IPMI_CHECK_CMD_LEN in the caller
>>> of these commands and use an array of expected argument lengths.
>>> For rsp, IPMI_ADD_RSP_DATA needs to be used to check that we are not
>>> exceeding max_rsp_len
>>>
>>>> Am I the only one this bothers?
>>>>
>>>>> +
>>>>> +    fruid = cmd[2];
>>>>> +
>>>>> +    if (fruid >= ibs->fru.nentries) {
>>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    fru_entry_size = ibs->fru.size;
>>>>> +
>>>>> +    IPMI_ADD_RSP_DATA(fru_entry_size & 0xff);
>>>>> +    IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff);
>>>>> +    IPMI_ADD_RSP_DATA(0x0);
>>>>
>>>> Same here. By the way, do you have some spec for the above or
>>>> is an ad-hoc encoding of the fields? If yes, you could
>>>> add a reference for the spec.(This is also for the other functions in this patch)
>>>
>>> Yes I will add the reference.
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>>> Thanks,
>>>> Marcel
>>>>
>>>>> +}
>>>>> +
>>>>> +#define min(x, y) ((x) < (y) ? (x) : (y))
>>>>> +#define max(x, y) ((x) > (y) ? (x) : (y))
>>>>> +
>>>>> +static void read_fru_data(IPMIBmcSim *ibs,
>>>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>>>> +                         unsigned int max_rsp_len)
>>>>> +{
>>>>> +    uint8_t fruid;
>>>>> +    uint16_t offset;
>>>>> +    int i;
>>>>> +    uint8_t *fru_entry;
>>>>> +    unsigned int count;
>>>>> +
>>>>> +    IPMI_CHECK_CMD_LEN(5);
>>>>> +
>>>>> +    fruid = cmd[2];
>>>>> +    offset = (cmd[3] | cmd[4] << 8);
>>>>> +
>>>>> +    if (fruid >= ibs->fru.nentries) {
>>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    if (offset >= ibs->fru.size - 1) {
>>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
>>>>> +
>>>>> +    count = min(cmd[5], ibs->fru.size - offset);
>>>>> +
>>>>> +    IPMI_ADD_RSP_DATA(count & 0xff);
>>>>> +    for (i = 0; i < count; i++) {
>>>>> +        IPMI_ADD_RSP_DATA(fru_entry[offset + i]);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static void write_fru_data(IPMIBmcSim *ibs,
>>>>> +                         uint8_t *cmd, unsigned int cmd_len,
>>>>> +                         uint8_t *rsp, unsigned int *rsp_len,
>>>>> +                         unsigned int max_rsp_len)
>>>>> +{
>>>>> +    uint8_t fruid;
>>>>> +    uint16_t offset;
>>>>> +    uint8_t *fru_entry;
>>>>> +    unsigned int count;
>>>>> +
>>>>> +    IPMI_CHECK_CMD_LEN(5);
>>>>> +
>>>>> +    fruid = cmd[2];
>>>>> +    offset = (cmd[3] | cmd[4] << 8);
>>>>> +
>>>>> +    if (fruid >= ibs->fru.nentries) {
>>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    if (offset >= ibs->fru.size - 1) {
>>>>> +        rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    fru_entry = &ibs->fru.data[fruid * ibs->fru.size];
>>>>> +
>>>>> +    count = min(cmd_len - 5, ibs->fru.size - offset);
>>>>> +
>>>>> +    memcpy(fru_entry + offset, cmd + 5, count);
>>>>> +
>>>>> +    IPMI_ADD_RSP_DATA(count & 0xff);
>>>>> +}
>>>>> +
>>>>>    static void reserve_sel(IPMIBmcSim *ibs,
>>>>>                            uint8_t *cmd, unsigned int cmd_len,
>>>>>                            uint8_t *rsp, unsigned int *rsp_len,
>>>>> @@ -1667,6 +1775,9 @@ static const IPMINetfn app_netfn = {
>>>>>    };
>>>>>
>>>>>    static const IPMICmdHandler storage_cmds[] = {
>>>>> +    [IPMI_CMD_GET_FRU_AREA_INFO] = get_fru_area_info,
>>>>> +    [IPMI_CMD_READ_FRU_DATA] = read_fru_data,
>>>>> +    [IPMI_CMD_WRITE_FRU_DATA] = write_fru_data,
>>>>>        [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>>>>>        [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>>>>>        [IPMI_CMD_GET_SDR] = get_sdr,
>>>>> @@ -1766,6 +1877,31 @@ static const VMStateDescription vmstate_ipmi_sim = {
>>>>>        }
>>>>>    };
>>>>>
>>>>> +static void ipmi_fru_init(IPMIFru *fru)
>>>>> +{
>>>>> +    int fsize;
>>>>> +    int size = 0;
>>>>> +
>>>>> +    fsize = get_image_size(fru->filename);
>>>>> +    if (fsize > 0) {
>>>>> +        size = QEMU_ALIGN_UP(fsize, fru->size);
>>>>> +        fru->data = g_malloc0(size);
>>>>> +        if (load_image_size(fru->filename, fru->data, fsize) != fsize) {
>>>>> +            error_report("Could not load file '%s'", fru->filename);
>>>>> +            g_free(fru->data);
>>>>> +            fru->data = NULL;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    if (!fru->data) {
>>>>> +        /* give one default FRU */
>>>>> +        size = fru->size;
>>>>> +        fru->data = g_malloc0(size);
>>>>> +    }
>>>>> +
>>>>> +    fru->nentries = size / fru->size;
>>>>> +}
>>>>> +
>>>>>    static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>>>
>>>>
>>>>>    {
>>>>>        IPMIBmc *b = IPMI_BMC(dev);
>>>>> @@ -1788,6 +1924,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>>>>
>>>>>        ipmi_sdr_init(ibs);
>>>>>
>>>>> +    ipmi_fru_init(&ibs->fru);
>>>>> +
>>>>>        ibs->acpi_power_state[0] = 0;
>>>>>        ibs->acpi_power_state[1] = 0;
>>>>>
>>>>> @@ -1806,6 +1944,8 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>>>>>    }
>>>>>
>>>>>    static Property ipmi_sim_properties[] = {
>>>>> +    DEFINE_PROP_UINT16("frusize", IPMIBmcSim, fru.size, 1024),
>>>>> +    DEFINE_PROP_STRING("frufile", IPMIBmcSim, fru.filename),
>>>>>        DEFINE_PROP_STRING("sdr", IPMIBmcSim, sdr_filename),
>>>>>        DEFINE_PROP_END_OF_LIST(),
>>>>>    };
>>>>>
>>>>
>>>
>>
> 

[Qemu-devel] [PATCH 7/8] ipmi: introduce an ipmi_bmc_sdr_find() API

[Cédric Le Goater]

This patch exposes a new IPMI routine to query a sdr entry from the
sdr table maintained by the IPMI BMC simulator. The API is very
similar to the internal sdr_find_entry() routine and should be used
the same way to query one or all sdrs.

A typical use would be to loop on the sdrs to build nodes of a device
tree.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 16 ++++++++++++++++
 include/hw/ipmi/ipmi.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index b0754893fc08..c952219429f4 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -406,6 +406,22 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid,
     return 1;
 }
 
+int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
+                      const struct ipmi_sdr_compact **sdr, uint16_t *nextrec)
+
+{
+    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
+    unsigned int pos;
+
+    pos = 0;
+    if (sdr_find_entry(&ibs->sdr, recid, &pos, nextrec)) {
+        return -1;
+    }
+
+    *sdr = (const struct ipmi_sdr_compact *) &ibs->sdr.sdr[pos];
+    return 0;
+}
+
 static void sel_inc_reservation(IPMISel *sel)
 {
     sel->reservation++;
diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index 74a2b5af9613..e41321db174d 100644
--- a/include/hw/ipmi/ipmi.h
+++ b/include/hw/ipmi/ipmi.h
@@ -255,4 +255,6 @@ struct ipmi_sdr_compact {
 
 typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)];
 
+int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
+                      const struct ipmi_sdr_compact **sdr, uint16_t *nextrec);
 #endif
-- 
2.1.4

Re: [Qemu-devel] [PATCH 7/8] ipmi: introduce an ipmi_bmc_sdr_find() API

[Marcel Apfelbaum]

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
> This patch exposes a new IPMI routine to query a sdr entry from the
> sdr table maintained by the IPMI BMC simulator. The API is very
> similar to the internal sdr_find_entry() routine and should be used
> the same way to query one or all sdrs.
>
> A typical use would be to loop on the sdrs to build nodes of a device
> tree.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 16 ++++++++++++++++
>   include/hw/ipmi/ipmi.h |  2 ++
>   2 files changed, 18 insertions(+)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index b0754893fc08..c952219429f4 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -406,6 +406,22 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid,
>       return 1;
>   }
>
> +int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
> +                      const struct ipmi_sdr_compact **sdr, uint16_t *nextrec)
> +
> +{
> +    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
> +    unsigned int pos;
> +
> +    pos = 0;
> +    if (sdr_find_entry(&ibs->sdr, recid, &pos, nextrec)) {
> +        return -1;
> +    }
> +
> +    *sdr = (const struct ipmi_sdr_compact *) &ibs->sdr.sdr[pos];
> +    return 0;
> +}
> +
>   static void sel_inc_reservation(IPMISel *sel)
>   {
>       sel->reservation++;
> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
> index 74a2b5af9613..e41321db174d 100644
> --- a/include/hw/ipmi/ipmi.h
> +++ b/include/hw/ipmi/ipmi.h
> @@ -255,4 +255,6 @@ struct ipmi_sdr_compact {
>
>   typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)];
>
> +int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
> +                      const struct ipmi_sdr_compact **sdr, uint16_t *nextrec);

This method is not used yet. I suppose you have a work in progress
using it, I suggest to add this to your next series when it will be used.

Thanks,
Marcel

>   #endif
>

Re: [Qemu-devel] [PATCH 7/8] ipmi: introduce an ipmi_bmc_sdr_find() API

[Cédric Le Goater]

On 02/14/2016 10:30 AM, Marcel Apfelbaum wrote:
> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>> This patch exposes a new IPMI routine to query a sdr entry from the
>> sdr table maintained by the IPMI BMC simulator. The API is very
>> similar to the internal sdr_find_entry() routine and should be used
>> the same way to query one or all sdrs.
>>
>> A typical use would be to loop on the sdrs to build nodes of a device
>> tree.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 16 ++++++++++++++++
>>   include/hw/ipmi/ipmi.h |  2 ++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index b0754893fc08..c952219429f4 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -406,6 +406,22 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid,
>>       return 1;
>>   }
>>
>> +int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
>> +                      const struct ipmi_sdr_compact **sdr, uint16_t *nextrec)
>> +
>> +{
>> +    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
>> +    unsigned int pos;
>> +
>> +    pos = 0;
>> +    if (sdr_find_entry(&ibs->sdr, recid, &pos, nextrec)) {
>> +        return -1;
>> +    }
>> +
>> +    *sdr = (const struct ipmi_sdr_compact *) &ibs->sdr.sdr[pos];
>> +    return 0;
>> +}
>> +
>>   static void sel_inc_reservation(IPMISel *sel)
>>   {
>>       sel->reservation++;
>> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
>> index 74a2b5af9613..e41321db174d 100644
>> --- a/include/hw/ipmi/ipmi.h
>> +++ b/include/hw/ipmi/ipmi.h
>> @@ -255,4 +255,6 @@ struct ipmi_sdr_compact {
>>
>>   typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)];
>>
>> +int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
>> +                      const struct ipmi_sdr_compact **sdr, uint16_t *nextrec);
> 
> This method is not used yet. I suppose you have a work in progress
> using it, I suggest to add this to your next series when it will be used.

OK. I will do. 

Thanks,

C.



> Thanks,
> Marcel
> 
>>   #endif
>>
> 

[Qemu-devel] [PATCH 8/8] ipmi: introduce an ipmi_bmc_gen_event() API

[Cédric Le Goater]

It will be used to fill the message buffer with custom events expected
by some systems. Typically, an Open PowerNV platform guest is notified
with an OEM SEL message before a shutdown or a reboot.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 24 ++++++++++++++++++++++++
 include/hw/ipmi/ipmi.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index c952219429f4..cd2db03a4188 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -463,6 +463,30 @@ static int attn_irq_enabled(IPMIBmcSim *ibs)
             IPMI_BMC_MSG_FLAG_EVT_BUF_FULL_SET(ibs));
 }
 
+void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log)
+{
+    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
+    IPMIInterface *s = ibs->parent.intf;
+    IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
+
+    if (!IPMI_BMC_EVENT_MSG_BUF_ENABLED(ibs)) {
+        return;
+    }
+
+    if (log && IPMI_BMC_EVENT_LOG_ENABLED(ibs)) {
+        sel_add_event(ibs, evt);
+    }
+
+    if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) {
+        goto out;
+    }
+
+    memcpy(ibs->evtbuf, evt, 16);
+    ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL;
+    k->set_atn(s, 1, attn_irq_enabled(ibs));
+ out:
+    return;
+}
 static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert,
                       uint8_t evd1, uint8_t evd2, uint8_t evd3)
 {
diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index e41321db174d..178e72d72b40 100644
--- a/include/hw/ipmi/ipmi.h
+++ b/include/hw/ipmi/ipmi.h
@@ -257,4 +257,6 @@ typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)];
 
 int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
                       const struct ipmi_sdr_compact **sdr, uint16_t *nextrec);
+void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log);
+
 #endif
-- 
2.1.4

Re: [Qemu-devel] [PATCH 8/8] ipmi: introduce an ipmi_bmc_gen_event() API

[Marcel Apfelbaum]

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
> It will be used to fill the message buffer with custom events expected
> by some systems. Typically, an Open PowerNV platform guest is notified
> with an OEM SEL message before a shutdown or a reboot.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 24 ++++++++++++++++++++++++
>   include/hw/ipmi/ipmi.h |  2 ++
>   2 files changed, 26 insertions(+)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index c952219429f4..cd2db03a4188 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -463,6 +463,30 @@ static int attn_irq_enabled(IPMIBmcSim *ibs)
>               IPMI_BMC_MSG_FLAG_EVT_BUF_FULL_SET(ibs));
>   }
>
> +void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log)
> +{
> +    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
> +    IPMIInterface *s = ibs->parent.intf;
> +    IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
> +
> +    if (!IPMI_BMC_EVENT_MSG_BUF_ENABLED(ibs)) {
> +        return;
> +    }
> +
> +    if (log && IPMI_BMC_EVENT_LOG_ENABLED(ibs)) {
> +        sel_add_event(ibs, evt);
> +    }
> +
> +    if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) {
> +        goto out;
> +    }
> +
> +    memcpy(ibs->evtbuf, evt, 16);
> +    ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL;
> +    k->set_atn(s, 1, attn_irq_enabled(ibs));
> + out:
> +    return;
> +}
>   static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert,
>                         uint8_t evd1, uint8_t evd2, uint8_t evd3)
>   {
> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
> index e41321db174d..178e72d72b40 100644
> --- a/include/hw/ipmi/ipmi.h
> +++ b/include/hw/ipmi/ipmi.h
> @@ -257,4 +257,6 @@ typedef uint8_t ipmi_sdr_compact_buffer[sizeof(struct ipmi_sdr_compact)];
>
>   int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
>                         const struct ipmi_sdr_compact **sdr, uint16_t *nextrec);
> +void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log);

Hi,

The same here, maybe you can add this function when it will be used.

Thanks,
Marcel

> +
>   #endif
>

Re: [Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC simulator (round 2)

[Corey Minyard]

On 02/09/2016 06:13 AM, Cédric Le Goater wrote:
> The first patches are cleanups and prepare ground for an extension of
> the BMC simulator providing a SDR loader using a file. A simple FRU
> support comes next.
>
> The last patches introduce APIs to populate a guest device tree with
> the available sensors and to generate events, which is needed by
> platforms in some occasions.
>

I have reviewed all of these, and they look good.  I only have one
comment: The naming of the properties probably needs to be
fixed.

"sdr" should be "sdrfile" to be consistent with everything else.

Technically, a "FRU" is a piece of hardware that can be replaced
in the field, "FRU data" is the data describing that FRU, and a "FRU
area" is the memory used to store FRU data.  I know this is mixed
up a lot (and I have done so) but some people are picky about this.

With the renaming of sdr (fru is your option):

Acked-by: Corey Minyard <cminyard@mvista.com>

for all patches.

Oh, and I assume you need to add documentation for the
properties to qemu-options.hx.

-corey

> Based on e4a096b1cd43 and also available here  :
>
>    https://github.com/legoater/qemu/commits/ipmi
>
> Thanks,
>
> C.
>
> Cédric Le Goater (8):
>    ipmi: add a realize function to the device class
>    ipmi: use a function to initialize the SDR table
>    ipmi: remove the need of an ending record in the SDR table
>    ipmi: add some local variables in ipmi_sdr_init
>    ipmi: use a file to load SDRs
>    ipmi: provide support for FRUs
>    ipmi: introduce an ipmi_bmc_sdr_find() API
>    ipmi: introduce an ipmi_bmc_gen_event() API
>
>   hw/ipmi/ipmi_bmc_sim.c | 256 +++++++++++++++++++++++++++++++++++++++++++------
>   include/hw/ipmi/ipmi.h |   4 +
>   2 files changed, 233 insertions(+), 27 deletions(-)
>

Re: [Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC simulator (round 2)

[Cédric Le Goater]

Hello Corey,

On 02/09/2016 07:25 PM, Corey Minyard wrote:
> On 02/09/2016 06:13 AM, Cédric Le Goater wrote:
>> The first patches are cleanups and prepare ground for an extension of
>> the BMC simulator providing a SDR loader using a file. A simple FRU
>> support comes next.
>>
>> The last patches introduce APIs to populate a guest device tree with
>> the available sensors and to generate events, which is needed by
>> platforms in some occasions.
>>
> 
> I have reviewed all of these, and they look good.  I only have one
> comment: The naming of the properties probably needs to be
> fixed.
> 
> "sdr" should be "sdrfile" to be consistent with everything else.

yes. I agree. I am glad you took a look. 

> Technically, a "FRU" is a piece of hardware that can be replaced
> in the field, "FRU data" is the data describing that FRU, and a "FRU
> area" is the memory used to store FRU data.  I know this is mixed
> up a lot (and I have done so) but some people are picky about this.
> 
> With the renaming of sdr (fru is your option):

I will rename the "sdr" property to "sdrfile".

As for FRU, you would rather have the code use FruData than Fru ? 
Something like: 

	typedef struct IPMIFruData {
	    char *filename;
	    unsigned int nentries;
	    uint16_t size;
	    uint8_t *area;
	} IPMIFruData;

The code using the IPMIFruData structure would look like :

    uint8_t *fru_area;

    ...
    fru_area = &ibs->frudata.area[fruid * ibs->frudata.size];
    ...

Changing all the names is not a problem. Let's get them right.

And, so, the properties would become :

    DEFINE_PROP_UINT16("frudatasize", IPMIBmcSim, frudata.size, 1024),
    DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, frudata.filename),
    DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),

> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
> for all patches.
> 
> Oh, and I assume you need to add documentation for the
> properties to qemu-options.hx.

Yes. Forgot that.

Thanks,

C. 

> -corey
> 
>> Based on e4a096b1cd43 and also available here  :
>>
>>    https://github.com/legoater/qemu/commits/ipmi
>>
>> Thanks,
>>
>> C.
>>
>> Cédric Le Goater (8):
>>    ipmi: add a realize function to the device class
>>    ipmi: use a function to initialize the SDR table
>>    ipmi: remove the need of an ending record in the SDR table
>>    ipmi: add some local variables in ipmi_sdr_init
>>    ipmi: use a file to load SDRs
>>    ipmi: provide support for FRUs
>>    ipmi: introduce an ipmi_bmc_sdr_find() API
>>    ipmi: introduce an ipmi_bmc_gen_event() API
>>
>>   hw/ipmi/ipmi_bmc_sim.c | 256 +++++++++++++++++++++++++++++++++++++++++++------
>>   include/hw/ipmi/ipmi.h |   4 +
>>   2 files changed, 233 insertions(+), 27 deletions(-)
>>
> 

Re: [Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC simulator (round 2)

[Corey Minyard]

On 02/10/2016 08:05 AM, Cédric Le Goater wrote:
> Hello Corey,
>
> On 02/09/2016 07:25 PM, Corey Minyard wrote:
>> On 02/09/2016 06:13 AM, Cédric Le Goater wrote:
>>> The first patches are cleanups and prepare ground for an extension of
>>> the BMC simulator providing a SDR loader using a file. A simple FRU
>>> support comes next.
>>>
>>> The last patches introduce APIs to populate a guest device tree with
>>> the available sensors and to generate events, which is needed by
>>> platforms in some occasions.
>>>
>> I have reviewed all of these, and they look good.  I only have one
>> comment: The naming of the properties probably needs to be
>> fixed.
>>
>> "sdr" should be "sdrfile" to be consistent with everything else.
> yes. I agree. I am glad you took a look.
>
>> Technically, a "FRU" is a piece of hardware that can be replaced
>> in the field, "FRU data" is the data describing that FRU, and a "FRU
>> area" is the memory used to store FRU data.  I know this is mixed
>> up a lot (and I have done so) but some people are picky about this.
>>
>> With the renaming of sdr (fru is your option):
> I will rename the "sdr" property to "sdrfile".
>
> As for FRU, you would rather have the code use FruData than Fru ?
> Something like:
>
> 	typedef struct IPMIFruData {
> 	    char *filename;
> 	    unsigned int nentries;
> 	    uint16_t size;
> 	    uint8_t *area;
> 	} IPMIFruData;
>
> The code using the IPMIFruData structure would look like :
>
>      uint8_t *fru_area;
>
>      ...
>      fru_area = &ibs->frudata.area[fruid * ibs->frudata.size];
>      ...
>
> Changing all the names is not a problem. Let's get them right.
>
> And, so, the properties would become :
>
>      DEFINE_PROP_UINT16("frudatasize", IPMIBmcSim, frudata.size, 1024),

I would name this "fruareasize" to be clear that it is not the size of 
the "frudatafile".

Other than that, I'm happy with those names.

-corey

>      DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, frudata.filename),
>      DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
>
>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>
>> for all patches.
>>
>> Oh, and I assume you need to add documentation for the
>> properties to qemu-options.hx.
> Yes. Forgot that.
>
> Thanks,
>
> C.
>
>> -corey
>>
>>> Based on e4a096b1cd43 and also available here  :
>>>
>>>     https://github.com/legoater/qemu/commits/ipmi
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>> Cédric Le Goater (8):
>>>     ipmi: add a realize function to the device class
>>>     ipmi: use a function to initialize the SDR table
>>>     ipmi: remove the need of an ending record in the SDR table
>>>     ipmi: add some local variables in ipmi_sdr_init
>>>     ipmi: use a file to load SDRs
>>>     ipmi: provide support for FRUs
>>>     ipmi: introduce an ipmi_bmc_sdr_find() API
>>>     ipmi: introduce an ipmi_bmc_gen_event() API
>>>
>>>    hw/ipmi/ipmi_bmc_sim.c | 256 +++++++++++++++++++++++++++++++++++++++++++------
>>>    include/hw/ipmi/ipmi.h |   4 +
>>>    2 files changed, 233 insertions(+), 27 deletions(-)
>>>

Re: [Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC simulator (round 2)

[Cédric Le Goater]

On 02/10/2016 05:06 PM, Corey Minyard wrote:
> On 02/10/2016 08:05 AM, Cédric Le Goater wrote:
>> Hello Corey,
>>
>> On 02/09/2016 07:25 PM, Corey Minyard wrote:
>>> On 02/09/2016 06:13 AM, Cédric Le Goater wrote:
>>>> The first patches are cleanups and prepare ground for an extension of
>>>> the BMC simulator providing a SDR loader using a file. A simple FRU
>>>> support comes next.
>>>>
>>>> The last patches introduce APIs to populate a guest device tree with
>>>> the available sensors and to generate events, which is needed by
>>>> platforms in some occasions.
>>>>
>>> I have reviewed all of these, and they look good.  I only have one
>>> comment: The naming of the properties probably needs to be
>>> fixed.
>>>
>>> "sdr" should be "sdrfile" to be consistent with everything else.
>> yes. I agree. I am glad you took a look.
>>
>>> Technically, a "FRU" is a piece of hardware that can be replaced
>>> in the field, "FRU data" is the data describing that FRU, and a "FRU
>>> area" is the memory used to store FRU data.  I know this is mixed
>>> up a lot (and I have done so) but some people are picky about this.
>>>
>>> With the renaming of sdr (fru is your option):
>> I will rename the "sdr" property to "sdrfile".
>>
>> As for FRU, you would rather have the code use FruData than Fru ?
>> Something like:
>>
>>     typedef struct IPMIFruData {
>>         char *filename;
>>         unsigned int nentries;
>>         uint16_t size;
>>         uint8_t *area;
>>     } IPMIFruData;
>>
>> The code using the IPMIFruData structure would look like :
>>
>>      uint8_t *fru_area;
>>
>>      ...
>>      fru_area = &ibs->frudata.area[fruid * ibs->frudata.size];
>>      ...
>>
>> Changing all the names is not a problem. Let's get them right.
>>
>> And, so, the properties would become :
>>
>>      DEFINE_PROP_UINT16("frudatasize", IPMIBmcSim, frudata.size, 1024),
> 
> I would name this "fruareasize" to be clear that it is not the size of the "frudatafile".
> 
> Other than that, I'm happy with those names.
>
> -corey


ok. I will only change the names of the properties and add Documentation
for them in v2

Thanks,

C.  

 
>>      DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, frudata.filename),
>>      DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
>>
>>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>>
>>> for all patches.
>>>
>>> Oh, and I assume you need to add documentation for the
>>> properties to qemu-options.hx.
>> Yes. Forgot that.
>>
>> Thanks,
>>
>> C.
>>
>>> -corey
>>>
>>>> Based on e4a096b1cd43 and also available here  :
>>>>
>>>>     https://github.com/legoater/qemu/commits/ipmi
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>> Cédric Le Goater (8):
>>>>     ipmi: add a realize function to the device class
>>>>     ipmi: use a function to initialize the SDR table
>>>>     ipmi: remove the need of an ending record in the SDR table
>>>>     ipmi: add some local variables in ipmi_sdr_init
>>>>     ipmi: use a file to load SDRs
>>>>     ipmi: provide support for FRUs
>>>>     ipmi: introduce an ipmi_bmc_sdr_find() API
>>>>     ipmi: introduce an ipmi_bmc_gen_event() API
>>>>
>>>>    hw/ipmi/ipmi_bmc_sim.c | 256 +++++++++++++++++++++++++++++++++++++++++++------
>>>>    include/hw/ipmi/ipmi.h |   4 +
>>>>    2 files changed, 233 insertions(+), 27 deletions(-)
>>>>
>