From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45379) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTXI5-0005Mc-Jn for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:06:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTXI4-0005KI-9G for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:06:53 -0500 Received: from mail-ob0-x234.google.com ([2607:f8b0:4003:c01::234]:34405) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTXI4-0005KB-1d for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:06:52 -0500 Received: by mail-ob0-x234.google.com with SMTP id wb13so33674413obb.1 for ; Wed, 10 Feb 2016 08:06:51 -0800 (PST) References: <1455020010-17532-1-git-send-email-clg@fr.ibm.com> <56BA2F29.8010109@mvista.com> <56BB4390.5040107@fr.ibm.com> From: Corey Minyard Message-ID: <56BB600F.4030209@mvista.com> Date: Wed, 10 Feb 2016 10:06:39 -0600 MIME-Version: 1.0 In-Reply-To: <56BB4390.5040107@fr.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 0/8] ipmi: a couple of enhancements to the BMC simulator (round 2) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Cc: Greg Kurz , qemu-devel@nongnu.org, "Michael S. Tsirkin" 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 >> >> 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(-) >>>