Re: [Freeipmi-devel] [RFC][PATCH] freeipmi: Special calxeda commands ported from ipmitools

[Albert Chu]

Hi Thomas,

Comments inlined below:

On Fri, 2013-08-09 at 14:26 +0200, Thomas Renninger wrote:
> On Thursday, August 08, 2013 03:54:13 PM Albert Chu wrote:
> > Hi Thomas,
> > 
> > Thanks for the patch.  I saw this patch fly by on the ipmitool mailing
> > list.  I looked at it briefly but realized it'd be quite a bit of work
> > to get ported over to FreeIPMI.  Also, since I don't have docs for
> > Calxeda OEM extensions, it'd be harder for me todo.  Thanks for taking
> > the first steps.
> > 
> > > - What still has to be done to get this accepted mainline?
> > 
> > I think the main goal is to "FreeIPMI-ize" the code so it's more
> > consistent with the rest of FreeIPMI code.
> Disadvantage is that the code diverges.
> Maybe a way in the middle could be found.
>
> > We'll probably have to
> > iterate a number of times and I'll have to slowly learn some of this
> > code too.
> Sounds sane.
> 
> > I didn't look at this patch super-close.  But here are some
> > immediate/obvious things that I noticed:
> > 
> > 1) Appropriate manpage documentation should be written up for all the
> >    OEM extensions offered.
> The syntax is the same as in ipmitools, but I couldn't find any docs there
> for the cxoem command as well.
> This would more be something for someone who knows the bits in detail,
> I simply ported the code...

I suppose someone from Calxeda would have to add that information?
Calxeda people??

>  
> > 2) lprintf should be replaced w/ calls to pstdout_printf or
> >    pstdout_fprintf as appropriate (most notably pstdout_printf for
> >    good output, pstdout_fprintf to stderr for errors).
> I can do that.
>  
> > 3) I don't believe the "help" option is needed, as ipmi-oem already
> >    has something like this constructed into its architecture.  For
> >    example, what options are there for Dell.
> Yep.
>  
> > 4) Call ipmi_cmd_raw directly, no need to have ipmitool wrappers.  It
> >    seems this was already done in cx_fw_download(), perhaps as a
> >    test??  Then of course call appropriate error checks like
> > ipmi_oem_check_response_and_completion_code().
> Hm, it may be a good idea to keep the wrapper for easier diffing and
> lower maintenance overhead with ipmitools code
> 
> ipmi_oem_check_response_and_completion_code()
> would still need to be added to the wrapper.
> 
> Would that be acceptable?

I don't know if you guys are working w/ Calxeda directly or indirectly.
Do you have an idea on how much maintenance will have to be done?  In my
experience IPMI OEM extensions tend not to be changed b/c it's code
written against firmware that presumably isn't going to change.

Then again, I don't know how final this vendor's product is.  Will it be
going through a lot of iterations?

> > 5)
> > 
> > +struct cx_fw_info_rs {
> > +   unsigned char ver;      /* param version */
> > +   unsigned char count;    /* number of bytes */
> > +   img_info_t img_info;
> > +} __attribute__ ((packed));
> > 
> > I'd prefer the use of FreeIPMI "fiid" templates or just using buffers,
> > as "__attribute__ ((packed))" isn't portable across all many
> > compilers.  We can talk about this in more detail if you feel that the
> > structs are a deal breaker.
> I have to look deeper into this.
>  
> > 6)
> > 
> > A lot of the macros in freeipmi-1.2.9/ipmi-oem/_ipmi-oem-calxeda.h
> > should be placed in their appropriate header files in
> > libfreeipmi/include/freeipmi/spec.  For example OEM commands in
> > ipmi-cmd-oem-spec.h and net fn's in ipmi-netfn-oem-spec.h.  For many
> > random macros that are needed by OEM commands, they should go in
> > ipmi-oem-spec.h.
> Ok.
>  
> > 7)
> > 
> > If you look at most of the ipmi-oem code, you'll see comments like
> > this throughout:
> > 
> >   /* Supermicro OEM
> >    *
> >    * Request
> >    *
> >    * 0x30 - OEM network function
> >    * 0x70 - OEM cmd
> >    * 0xF0 - Sub-command
> >    * 0x?? - action
> >    *      - 0x00 - disable
> >    *      - 0x01 - enable
> >    *      - 0x02 - status
> >    *
> >    * Response
> >    *
> >    * 0x70 - OEM cmd
> >    * 0x?? - Completion Code
> >    * 0x?? - if action == status
> >    *      - 0x00 - disabled
> >    *      - 0x01 - enabled
> >    */
> > 
> > I think it's important to document each of the OEM extensions in as
> > much detail as possible to make things easier for myself and future
> > maintainers.  Unlike the IPMI spec, we don't want to reach the point
> > where the code is the only documentation, b/c alot of it can be
> > confusing.  For example:
> > 
> > +       req.msg.netfn = IPMI_NETFN_OEM_SS;
> > +       req.msg.cmd = IPMI_CMD_OEM_FW_DOWNLOAD;
> > +       msg_data[0] = type;
> > +       msg_data[1] = partition;
> > +       msg_data[2] = CXOEM_FW_UPLOAD;
> > +       msg_data[3] = 0;
> > +       msg_data[4] = 0;
> > +       msg_data[5] = 6;        // ipv4 addresses by default (for now)
> > 
> > Why are msg_data[3-4] set to 0?  Why is msg_data[5] set to 6??  I have
> > no idea why.
> Nothing for me. Hopefully Calxeda guys can add something.
> Sounds worth for both ipmitools and freeipmi and shouldn't be a big deal
> to push it for both (once the stuff is in a mainline branch?).
>  
> > There were some nits I found along the way, but they can be discussed
> > later.
> > 
> > Not sure how you all want to proceed.  Perhaps your side wants to try
> > 1-2 clean ups as a first attempt, then I can apply to a FreeIPMI branch
> > and we can iterate on that?
> 
> Sounds like a good idea.

Cool.

Al

> Be aware that I will not maintain this code.
> I can help a bit getting it cleaner, but either calxeda guys have to help
> or community picks up, once some bugs/issues are found.
> 
> Thanks a lot for the quick answer,
> 
>         Thomas
> 
>  
> > Al
> > 
> > On Thu, 2013-08-08 at 15:33 +0200, Thomas Renninger wrote:
> > > this is more or less a one to one copy from:
> > > http://sources.calxeda.com/gitweb/?p=ipmitool.git;a=summary
> > > file: lib/ipmi_cxoem.c
> > > implementing calxeda specific IPMI commands.
> > > 
> > > The entry points have been adjusted to freeipmi style and ipmitool
> > > specific
> > > coding style (struct ipmi_intf, intf->sendrecv(..)) have been put into
> > > small wrapper functions.
> > > Goal was to not have a perfectly correct coding style, but to show a prove
> > > of concept, that it's not that hard to get this functionality into
> > > freeipmi.
> > > 
> > > I wonder:
> > > - What still has to be done to get this accepted mainline?
> > > - Could Calxeda guys take over some testing? I have no idea about most
> > > 
> > >   of the commands.
> > > 
> > > - Could Calxeda provide some maintenance? If a bug in ipmitool is
> > > discovered,> 
> > >   the fix should probably also be pushed into freeipmi (or vice versa), as
> > >   the code is more or less the same.
> > > 
> > > Currently all commands are added:
> > > ipmi-oem calxeda
> > > Calxeda Command: help
> > > Calxeda Command: fw
> > > Calxeda Command: fabric
> > > Calxeda Command: mac
> > > Calxeda Command: log
> > > Calxeda Command: data
> > > Calxeda Command: info
> > > Calxeda Command: feature
> > > 
> > > but only some are implemented (the others would need some more adjusting
> > > of
> > > ipmitool specific commands used, but I doubt it would be that hard):
> > > 
> > > ./ipmi-oem -h 10.121.39.1 -u admin -p admin calxeda fw activate 10
> > > Partition         : 10
> > > activate: write flags <fffdfff8>
> > > 
> > > ./ipmi-oem -h 10.121.39.1 -u admin -p admin calxeda data mem read 4 100000
> > > xint Length   : 4
> > > Addr     : 00100000
> > > Value    : 0xdd0000ea
> > > 
> > > ./ipmi-oem -h 10.121.39.1 -u admin -p admin calxeda feature status mansen
> > > 
> > >    mansen is enabled
> > > 
> > > ./ipmi-oem -h 10.121.39.1 -u admin -p admin calxeda fabric get macaddr
> > > node 0 interface 1 fc:2f:40:0d:9f:11
> > > 
> > > ./ipmi-oem calxeda log
> > > ./ipmi-oem calxeda mac
> > > ./ipmi-oem calxeda info
> > > log command not implemented
> > > 
> > > Comments?
> > > 
> > > Thanks,
> > > 
> > >       Thomas
> > > 
> 
-- 
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory