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 <address@hidden>
>>>
>>> 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(-)
>>>>
>