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

[David Gibson]

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 <address@hidden>
> > Signed-off-by: Cédric Le Goater <address@hidden>
> > ---
> >  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 <address@hidden>

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.

> 
> > +
> >  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

signature.asc
Description: PGP signature