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 <cminy...@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(-) >>>> >