phosphor-host-ipmid and phosphor-net-ipmid architecture

[Vernon Mauery]

On 12-Oct-2017 12:03 PM, Brad Bishop wrote:
> 
> > On Oct 11, 2017, at 6:05 PM, Vernon Mauery <vernon.mauery at linux.intel.com> wrote:
> > 
> > I am working on an ipmi provider library and had a few questions and 
> > observations.
> > 
> > 1) Why are there separate ipmi message queues for the host and network? 
> 
> iirc it was so that you could have a different set of providers
> registered for each channel or even different, channel specific
> implementations for the same command.

Okay, that makes sense. This is just a mindset change for me. :) In my 
head I was separating the providers by command group. I just need to 
make sure that we are also separating them by interface if that matters 
so they can only get registered in a single queue.

> >   It seems awkward that for the host, the ipmi request comes from a 
> >   different process (btbridge, or in our case kcsbridge), while for the 
> >   network (RMCP+), the messages are handled directly in the same 
> >   process.
> 
> Is it still awkward if you accept the two queue approach?  The 
> additional layer is needed for the bt/kcs -> host-ipmid
> abstraction.  No such abstraction is needed for network.

I guess it is weird to me that the network commands don't seem to get 
packaged up and sent across dbus, while they do for bt/kcs. Why don't 
the bt/kcs commands get processed in the same process instead of getting 
sent to ipmid. From the perspective of the command handlers in ipmid, 
they cannot tell if the channel is internal (on-bmc ipmitool) or the 
bt/kcs interface.

Having multiple queues only makes sense to me if there is one for every 
channel. Alternately, one queue that passes the channel information in 
with each command.

> >   It seems that the network handler could just as easily package the 
> >   command up and send it to ipmid the same way that btbridge does.
> > 
> > 2) Can we modify the signature of the handlers so that they can behave 
> >   in a more intelligent manner? It would be nice if they were handed a 
> >   gsl::span<uint8_t> instead of a void* and a length. This allows for  
> >   a no-copy, bounds-checked way of passing buffers.
> 
> sounds good to me!
> 
> > 
> >   It would be nice to know what channel something came in on. We might 
> >   want to be able to change behavior based on the incoming channel (as 
> >   some channels are more secure than others).
> 
> I think the separate message queues solves this need; however, doing
> this need not be mutually exclusive of having separate queues if that
> enables another use case we don’t support today.
> 
> I’m not totally stuck on two queues.  Especially if some alternative
> maintains the same level of flexibility.  The motivation for a single
> queue isn’t clear yet to me though - without more discussion it sounds
> like we’d end up with the same capability we already have…so why bother?

If any of the ipmi commands have any state involved, then that state 
needs to either be duplicated between two queues or somehow 
synchronized. Neither situation is ideal if the size of the shared data 
is large. A single queue would not have this problem.

But now that you mention the advantages of the multiple queues, it seems 
that neither situation is ideal. So maybe I will have to think some more 
on this one.

> >   It would be nice to know what IPMI privilege the command came in 
> >   with (ADMIN for session-less commands) so that the command handler 
> >   can behave appropriately based on the user.
> 
> Makes sense.
> 
> > 
> > 3) When registering commands, it would be nice of the list also 
> >   maintained a priority so that commands could be easily overridden. 
> >   Currently the only way to override a command is to make sure that 
> >   your library gets loaded first (and this is done via the library 
> >   name). If we had default ipmi commands loaded at DEFAULT_PRIO and 
> >   then had some higher priorities such as MFR_PRIO, and OEM_PRIO, or 
> >   something like that, we could have integrators further on down the 
> >   line able to easily add a new provider library and piecemeal override 
> >   individual command. An alternate (or addition) might be the addition 
> >   of a unregister command method to remove an existing command so it 
> >   could be replaced with a new one (or just straight up removed).
> 
> The intent is that everything in OpenBMC is overridable by an integrator.
> We provide a reasonable ‘reference’ implementation but can be replaced
> with something else via a bitbake layer.  At image construction time.
> 
> I think you are proposing a setup where an image has multiple providers
> for the same command, and the ‘reference’ has a low priority.  In their
> bitbake layer, an integrator could _add_ another provider with higher
> priority, which would be selected to handle the command since it has
> the higher priority.
> 
> That makes sense, but why wouldn’t an integrator just _replace_ the
> reference in an image with the custom integrator provider?  This type
> of thing is exactly why we are a bitbake distro.
> 
> I hope you don’t have a good answer to this :-) as one of my goals is
> to try move runtime complexity into the build process as much as we can.

The only thing I can think of is if an integrator likes 75% of the 
commands in one of the base provider libraries but still wants to 
override 25% of the commands (or even just one).

Pushing the configuration to the build is good. Maybe adding some way to 
enable/disable each command in the base providers at build time. 
Otherwise integrators would need to maintain a patch that disables the 
command the hard way.

That said, this may not really be a huge deal since it is more likely 
that any commands integrators will be adding would be OEM commands that 
don't collide anyway.

--Vernon