qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC s


From: Corey Minyard
Subject: Re: [Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC simulator (round 2)
Date: Wed, 10 Feb 2016 10:06:39 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]