[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: |
Mon, 21 Oct 2019 09:30:17 -0500 |
User-agent: |
Mutt/1.9.4 (2018-02-28) |
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>
> +
> static const IPMICmdHandler *ipmi_get_handler(IPMIBmcSim *ibs,
> unsigned int netfn,
> unsigned int cmd)
> --
> 2.21.0
>