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

2016-02-16 Thread 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 
> ---
>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_MODE0x2B
>#define IPMI_CMD_RUN_INIT_AGENT   0x2C
> +#define IPMI_CMD_GET_FRU_AREA_INFO0x10
> +#define IPMI_CMD_READ_FRU_DATA0x11
> +#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) :

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

2016-02-15 Thread 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 
---
   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_MODE0x2B
   #define IPMI_CMD_RUN_INIT_AGENT   0x2C
+#define IPMI_CMD_GET_FRU_AREA_INFO0x10
+#define IPMI_CMD_READ_FRU_DATA0x11
+#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 = >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

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

2016-02-15 Thread 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 
---
   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_MODE0x2B
   #define IPMI_CMD_RUN_INIT_AGENT   0x2C
+#define IPMI_CMD_GET_FRU_AREA_INFO0x10
+#define IPMI_CMD_READ_FRU_DATA0x11
+#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 = >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];
+

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

2016-02-15 Thread 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 
>> ---
>>   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_MODE0x2B
>>   #define IPMI_CMD_RUN_INIT_AGENT   0x2C
>> +#define IPMI_CMD_GET_FRU_AREA_INFO0x10
>> +#define IPMI_CMD_READ_FRU_DATA0x11
>> +#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 = >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)

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

2016-02-14 Thread 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 
---
  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_MODE0x2B
  #define IPMI_CMD_RUN_INIT_AGENT   0x2C
+#define IPMI_CMD_GET_FRU_AREA_INFO0x10
+#define IPMI_CMD_READ_FRU_DATA0x11
+#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 = >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 = >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,

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

2016-02-09 Thread 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 
---
 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_MODE0x2B
 #define IPMI_CMD_RUN_INIT_AGENT   0x2C
+#define IPMI_CMD_GET_FRU_AREA_INFO0x10
+#define IPMI_CMD_READ_FRU_DATA0x11
+#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 = >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 = >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);
+