[2/5] ipmi: Add support to customize OEM functions
diff mbox series

Message ID 20191021131215.3693-3-clg@kaod.org
State New
Headers show
Series
  • ppc/pnv: Add PNOR support
Related show

Commit Message

Cédric Le Goater Oct. 21, 2019, 1:12 p.m. UTC
The routine ipmi_register_oem_netfn() lets external modules register
command handlers for OEM functions. Required for the PowerNV machine.

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(-)

Comments

Corey Minyard Oct. 21, 2019, 2:30 p.m. UTC | #1
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>

> +
>  static const IPMICmdHandler *ipmi_get_handler(IPMIBmcSim *ibs,
>                                                unsigned int netfn,
>                                                unsigned int cmd)
> -- 
> 2.21.0
>
David Gibson Oct. 27, 2019, 5:47 p.m. UTC | #2
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.

> 
> > +
> >  static const IPMICmdHandler *ipmi_get_handler(IPMIBmcSim *ibs,
> >                                                unsigned int netfn,
> >                                                unsigned int cmd)
>
Corey Minyard Oct. 27, 2019, 6:33 p.m. UTC | #3
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.

-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
David Gibson Nov. 7, 2019, 5 p.m. UTC | #4
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)
> > > 
> > 
> 
>
Cédric Le Goater Nov. 7, 2019, 5:14 p.m. UTC | #5
>>> 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.  

Here is the patch :

	http://patchwork.ozlabs.org/patch/1185187/


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

OK. I was wondering where it had gone.

Thanks,

C.
Corey Minyard Nov. 7, 2019, 5:25 p.m. UTC | #6
On Thu, Nov 07, 2019 at 06:14:58PM +0100, Cédric Le Goater wrote:
> >>> 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.  
> 
> Here is the patch :
> 
> 	http://patchwork.ozlabs.org/patch/1185187/

Ok, I have this in my tree.

I assume there is nothing like the linux-next tree for qemu, right?

-corey

> 
> 
> > 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.
> 
> OK. I was wondering where it had gone.
> 
> Thanks,
> 
> C.
Cédric Le Goater Nov. 7, 2019, 5:29 p.m. UTC | #7
On 07/11/2019 18:25, Corey Minyard wrote:
> On Thu, Nov 07, 2019 at 06:14:58PM +0100, Cédric Le Goater wrote:
>>>>> 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.  
>>
>> Here is the patch :
>>
>> 	http://patchwork.ozlabs.org/patch/1185187/
> 
> Ok, I have this in my tree.
> 
> I assume there is nothing like the linux-next tree for qemu, right?

no. These IPMI OEM commands are handled by the OPAL firmware only.

C.

Patch
diff mbox series

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;
+
+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);
+}
+
 static const IPMICmdHandler *ipmi_get_handler(IPMIBmcSim *ibs,
                                               unsigned int netfn,
                                               unsigned int cmd)