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

[Corey Minyard <cminyard@mvista.com>]

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(-)
>>>