[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/5] ipmi: Add support to customize OEM functions
From: |
Corey Minyard |
Subject: |
Re: [PATCH 2/5] ipmi: Add support to customize OEM functions |
Date: |
Sun, 27 Oct 2019 13:33:47 -0500 |
User-agent: |
Mutt/1.9.4 (2018-02-28) |
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 <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.
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