Re: [PATCH 2/5] ipmi: Add support to customize OEM functions

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6888 bytes --]

On Sun, Oct 27, 2019 at 01:33:47PM -0500, Corey Minyard wrote:
> On Sun, Oct 27, 2019 at 06:47:39PM +0100, David Gibson wrote:
> > On Mon, Oct 21, 2019 at 09:30:17AM -0500, Corey Minyard wrote:
> > > On Mon, Oct 21, 2019 at 03:12:12PM +0200, Cédric Le Goater wrote:
> > > > The routine ipmi_register_oem_netfn() lets external modules register
> > > > command handlers for OEM functions. Required for the PowerNV machine.
> > > 
> > > Comments inline.
> > > 
> > > > 
> > > > Cc: Corey Minyard <cminyard@mvista.com>
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > >  include/hw/ipmi/ipmi.h | 36 ++++++++++++++++++++++++++++++++++++
> > > >  hw/ipmi/ipmi_bmc_sim.c | 41 ++++++-----------------------------------
> > > >  2 files changed, 42 insertions(+), 35 deletions(-)
> > > > 
> > > > diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
> > > > index 6f2413b39b4a..cb7203b06767 100644
> > > > --- a/include/hw/ipmi/ipmi.h
> > > > +++ b/include/hw/ipmi/ipmi.h
> > > > @@ -265,4 +265,40 @@ int ipmi_bmc_sdr_find(IPMIBmc *b, uint16_t recid,
> > > >                        const struct ipmi_sdr_compact **sdr, uint16_t *nextrec);
> > > >  void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log);
> > > >  
> > > > +typedef struct IPMIBmcSim IPMIBmcSim;
> > > 
> > > This type isn't very useful outside of the simulator, but changes for
> > > that can come as they are needed.  I don't see an easy way to avoid
> > > putting it here.
> > > 
> > > > +
> > > > +typedef struct RspBuffer {
> > > > +    uint8_t buffer[MAX_IPMI_MSG_SIZE];
> > > > +    unsigned int len;
> > > > +} RspBuffer;
> > > > +
> > > > +static inline void rsp_buffer_set_error(RspBuffer *rsp, uint8_t byte)
> > > > +{
> > > > +    rsp->buffer[2] = byte;
> > > > +}
> > > > +
> > > > +/* Add a byte to the response. */
> > > > +static inline void rsp_buffer_push(RspBuffer *rsp, uint8_t byte)
> > > > +{
> > > > +    if (rsp->len >= sizeof(rsp->buffer)) {
> > > > +        rsp_buffer_set_error(rsp, IPMI_CC_REQUEST_DATA_TRUNCATED);
> > > > +        return;
> > > > +    }
> > > > +    rsp->buffer[rsp->len++] = byte;
> > > > +}
> > > > +
> > > > +typedef struct IPMICmdHandler {
> > > > +    void (*cmd_handler)(IPMIBmcSim *s,
> > > > +                        uint8_t *cmd, unsigned int cmd_len,
> > > > +                        RspBuffer *rsp);
> > > > +    unsigned int cmd_len_min;
> > > > +} IPMICmdHandler;
> > > > +
> > > > +typedef struct IPMINetfn {
> > > > +    unsigned int cmd_nums;
> > > > +    const IPMICmdHandler *cmd_handlers;
> > > > +} IPMINetfn;
> > > > +
> > > > +int ipmi_register_oem_netfn(IPMIBmc *b, const IPMINetfn *netfnd);
> > > > +
> > > >  #endif
> > > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> > > > index 71e56f3b13d1..770aace55b08 100644
> > > > --- a/hw/ipmi/ipmi_bmc_sim.c
> > > > +++ b/hw/ipmi/ipmi_bmc_sim.c
> > > > @@ -98,6 +98,7 @@
> > > >  #define IPMI_CMD_GET_SEL_TIME             0x48
> > > >  #define IPMI_CMD_SET_SEL_TIME             0x49
> > > >  
> > > > +#define IPMI_NETFN_OEM                    0x3a
> > > >  
> > > >  /* Same as a timespec struct. */
> > > >  struct ipmi_time {
> > > > @@ -167,23 +168,8 @@ typedef struct IPMISensor {
> > > >  #define MAX_SENSORS 20
> > > >  #define IPMI_WATCHDOG_SENSOR 0
> > > >  
> > > > -typedef struct IPMIBmcSim IPMIBmcSim;
> > > > -typedef struct RspBuffer RspBuffer;
> > > > -
> > > >  #define MAX_NETFNS 64
> > > >  
> > > > -typedef struct IPMICmdHandler {
> > > > -    void (*cmd_handler)(IPMIBmcSim *s,
> > > > -                        uint8_t *cmd, unsigned int cmd_len,
> > > > -                        RspBuffer *rsp);
> > > > -    unsigned int cmd_len_min;
> > > > -} IPMICmdHandler;
> > > > -
> > > > -typedef struct IPMINetfn {
> > > > -    unsigned int cmd_nums;
> > > > -    const IPMICmdHandler *cmd_handlers;
> > > > -} IPMINetfn;
> > > > -
> > > >  typedef struct IPMIRcvBufEntry {
> > > >      QTAILQ_ENTRY(IPMIRcvBufEntry) entry;
> > > >      uint8_t len;
> > > > @@ -279,28 +265,8 @@ struct IPMIBmcSim {
> > > >  #define IPMI_BMC_WATCHDOG_ACTION_POWER_DOWN      2
> > > >  #define IPMI_BMC_WATCHDOG_ACTION_POWER_CYCLE     3
> > > >  
> > > > -struct RspBuffer {
> > > > -    uint8_t buffer[MAX_IPMI_MSG_SIZE];
> > > > -    unsigned int len;
> > > > -};
> > > > -
> > > >  #define RSP_BUFFER_INITIALIZER { }
> > > >  
> > > > -static inline void rsp_buffer_set_error(RspBuffer *rsp, uint8_t byte)
> > > > -{
> > > > -    rsp->buffer[2] = byte;
> > > > -}
> > > > -
> > > > -/* Add a byte to the response. */
> > > > -static inline void rsp_buffer_push(RspBuffer *rsp, uint8_t byte)
> > > > -{
> > > > -    if (rsp->len >= sizeof(rsp->buffer)) {
> > > > -        rsp_buffer_set_error(rsp, IPMI_CC_REQUEST_DATA_TRUNCATED);
> > > > -        return;
> > > > -    }
> > > > -    rsp->buffer[rsp->len++] = byte;
> > > > -}
> > > > -
> > > >  static inline void rsp_buffer_pushmore(RspBuffer *rsp, uint8_t *bytes,
> > > >                                         unsigned int n)
> > > >  {
> > > > @@ -640,6 +606,11 @@ static int ipmi_register_netfn(IPMIBmcSim *s, unsigned int netfn,
> > > >      return 0;
> > > >  }
> > > >  
> > > > +int ipmi_register_oem_netfn(IPMIBmc *b, const IPMINetfn *netfnd)
> > > > +{
> > > > +    return ipmi_register_netfn(IPMI_BMC_SIMULATOR(b), IPMI_NETFN_OEM, netfnd);
> > > > +}
> > > 
> > > I think I would prefer just exposing ipmi_register_netfn() and maybe
> > > rename it ipmi_sim_register_netfn() or something like that.  There may
> > > be other netfns needed in the future.
> > > 
> > > But with that change, this looks good to me:
> > > 
> > > Reviewed-by: Corey Minyard <cminyard@mvista.com>
> > 
> > What's the plan for merging this, once it's ready?  Is there an IPMI
> > tree for it to be staged in?  If not I could take it through the ppc
> > tree, but I'd need some Acked-bys in that case.
> 
> I have an IPMI tree for this.  I was assuming it was going in to the PPC
> tree, but it's not big deal.

I'd be more comfortable if the generic ipmi changes went through the
ipmi tree.  Note that I've moved the initial ppc specific patch from
my ppc-for-4.2 tree to my ppc-for-4.3 tree, since it missed my
previous pull request and it's not really post-freeze material.

> 
> -corey
> 
> > 
> > > 
> > > > +
> > > >  static const IPMICmdHandler *ipmi_get_handler(IPMIBmcSim *ibs,
> > > >                                                unsigned int netfn,
> > > >                                                unsigned int cmd)
> > > 
> > 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]