[PATCH openpower-host-ipmi-oem 1/2] Support OpenPOWERs OEM commands on openbmc

Joel Stanley joel at jms.id.au
Thu Oct 15 15:48:23 AEDT 2015


Hi Chris,

I had a bunch of questions from reading your code. Please see below.

On Thu, Oct 15, 2015 at 12:53 AM, OpenBMC Patches <patches at stwcx.xyz> wrote:
> From: Chris Austen <austenc at us.ibm.com>
>
> ---
>  Makefile     | 21 ++++++++++++++
>  ipmid-api.h  | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  oemhandler.C | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  oemhandler.h | 32 +++++++++++++++++++++
>  4 files changed, 228 insertions(+)
>  create mode 100644 Makefile
>  create mode 100644 ipmid-api.h
>  create mode 100644 oemhandler.C
>  create mode 100644 oemhandler.h
>
> diff --git a/Makefile b/Makefile
> new file mode 100644
> index 0000000..f495d94
> --- /dev/null
> +++ b/Makefile
> @@ -0,0 +1,21 @@
> +CXX ?= $(CROSS_COMPILE)g++
> +
> +LIB_OEM_OBJ = oemhandler.o
> +LIB_OEM     = liboemhandler.so
> +
> +INC_FLAG += -I. -O2 --std=gnu++11

What does INC_FLAG mean? I suggest using CXXFLAGS for your C++ flags.
You can include the -fpic as well.

We should be building with -Wall enabled too.

> +LIB_FLAG += -rdynamic

I'd call this LDFLAGS, as that is what it is. You can add the -shared as well.

> +
> +
> +all: $(LIB_OEM)
> +
> +%.o: %.C
> +       $(CXX) -fpic -c $< $(CXXFLAGS) $(INC_FLAG) $(IPMID_PATH) -o $@

IPMID_PATH does not appear to be defined.

This rule is implict, if you use the names I suggested above. You can
omit it and the makefile will still do the same thing.

> +
> +
> +$(LIB_OEM): $(LIB_OEM_OBJ)
> +       $(CXX) $^ -shared $(LDFLAGS) $(LIB_FLAG) -o $@
> +
> +
> +clean:
> +       rm -f *.o *.so

perhaps this:

 clean:
           $(RM) $(LIB_OEM_OBJ) $(LIB_OEM)

> diff --git a/ipmid-api.h b/ipmid-api.h
> new file mode 100644
> index 0000000..ef2be38
> --- /dev/null
> +++ b/ipmid-api.h
> @@ -0,0 +1,83 @@
> +#ifndef __HOST_IPMID_IPMI_COMMON_H__
> +#define __HOST_IPMID_IPMI_COMMON_H__

I'd expect the guard to be #define IPMID_API_H?

> +#include <stdlib.h>
> +
> +// length of Completion Code and its ALWAYS _1_
> +#define IPMI_CC_LEN 1
> +
> +// IPMI Net Function number as specified by IPMI V2.0 spec.
> +// Example :
> +// NETFUN_APP      =   (0x06 << 2),
> +typedef unsigned char   ipmi_netfn_t;
> +
> +// IPMI Command for a Net Function number as specified by IPMI V2.0 spec.
> +typedef unsigned char   ipmi_cmd_t;
> +
> +// Buffer containing data from sender of netfn and command as part of request
> +typedef void*           ipmi_request_t;

Is there much value in these typedefs? I think the obscure what's
going on in the functions below.

> +// This is the response buffer that the provider of [netfn,cmd] will send back
> +// to the caller. Provider will allocate the memory inside the handler and then
> +// will do a memcpy to this response buffer and also will set the data size
> +// parameter to the size of the buffer.
> +// EXAMPLE :
> +// unsigned char str[] = {0x00, 0x01, 0xFE, 0xFF, 0x0A, 0x01};
> +// *data_len = 6;
> +// memcpy(response, &str, *data_len);
> +typedef void*           ipmi_response_t;
> +
> +// This buffer contains any *user specific* data that is of interest only to the
> +// plugin. For a ipmi function router, this data is opaque. At the time of
> +// registering the plugin handlers, plugin may optionally allocate a memory and
> +// fill in whatever needed that will be of help during the actual handling of
> +// command. IPMID will just pass the netfn, cmd and also this data to plugins
> +// during the command handler invocation.
> +typedef void*           ipmi_context_t;
> +
> +// Length of request / response buffer depending on whether the data is a
> +// request or a response from a plugin handler.
> +typedef size_t*   ipmi_data_len_t;
> +
> +// Plugin function return the status code
> +typedef unsigned char ipmi_ret_t;

I do not think this is necessary. We can just return a uint8_t.

> +
> +// This is the callback handler that the plugin registers with IPMID. IPMI
> +// function router will then make a call to this callback handler with the
> +// necessary arguments of netfn, cmd, request, response, size and context.
> +typedef ipmi_ret_t (*ipmid_callback_t)(ipmi_netfn_t, ipmi_cmd_t, ipmi_request_t,
> +                                       ipmi_response_t, ipmi_data_len_t, ipmi_context_t);
> +
> +// This is the constructor function that is called into by each plugin handlers.
> +// When ipmi sets up the callback handlers, a call is made to this with
> +// information of netfn, cmd, callback handler pointer and context data.
> +// Making this a extern "C" so that plugin libraries written in C can also use
> +// it.
> +extern "C" void ipmi_register_callback(ipmi_netfn_t, ipmi_cmd_t,
> +                                       ipmi_context_t, ipmid_callback_t);
> +
> +// These are the command network functions, the response
> +// network functions are the function + 1. So to determine
> +// the proper network function which issued the command
> +// associated with a response, subtract 1.
> +// Note: these are also shifted left to make room for the LUN.
> +enum ipmi_net_fns
> +{
> +    NETFUN_OEM_IBM  =   (0x32 << 2)
> +};
> +
> +// IPMI commands for net functions. Since this is to be used both by the ipmi
> +// function router and also the callback handler registration function, its put
> +// in this .H file.
> +enum ipmi_netfn_wild_card_cmd
> +{
> +    IPMI_CMD_WILDCARD       = 0xFF,
> +};
> +
> +// Return codes from a IPMI operation as needed by IPMI V2.0 spec.
> +enum ipmi_return_codes
> +{
> +    IPMI_CC_OK = 0x00,
> +    IPMI_CC_INVALID = 0xC1
> +};
> +
> +#endif
> diff --git a/oemhandler.C b/oemhandler.C
> new file mode 100644
> index 0000000..c4eae21
> --- /dev/null
> +++ b/oemhandler.C
> @@ -0,0 +1,92 @@
> +#include "oemhandler.h"
> +#include "ipmid-api.h"
> +#include <stdio.h>
> +#include <string.h>
> +
> +void register_netfn_oem_partial_esel() __attribute__((constructor));
> +
> +
> +

Why the extra whitespace?

> +const char *g_esel_path = "/tmp/";

Can we come up with something better than sticking files in /tmp?

> +uint16_t g_record_id = 0x0100;
> +
> +

Whitespace.

> +#define LSMSSWAP(x,y) (y<<8|x)
> +
> +

Whitespace.

> +///////////////////////////////////////////////////////////////////////////////

I don't think this is necessary.

> +// For the First partial add eSEL the SEL Record ID and offset
> +// value should be 0x0000. The extended data needs to be in
> +// the form of an IPMI SEL Event Record, with Event sensor type
> +// of 0xDF and Event Message format of 0x04. The returned
> +// Record ID should be used for all partial eSEL adds.
> +//
> +// This function creates a /tmp/esel# file to store the
> +// incoming partial esel.  It is the role of some other
> +// function to commit the error log in to long term
> +// storage.  Likely via the ipmi add_sel command.

Do we really need to write this out as a file? Should it not be passed
to the program that is going to commit it to long term storage?

> +///////////////////////////////////////////////////////////////////////////////
> +ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> +                              ipmi_request_t request, ipmi_response_t response,
> +                              ipmi_data_len_t data_len, ipmi_context_t context)
> +{
> +    esel_request_t *reqptr = (esel_request_t*) request;
> +    FILE *fp;
> +    char string[32];
> +    short offset = 0, record=0;
> +    unsigned short rlen;
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +    char iocmd[4];
> +
> +    strcpy(string, g_esel_path);

Why the strcpy? You could just use g_esel_path as the first string below.

> +
> +    offset = LSMSSWAP(reqptr->offsetls, reqptr->offsetms);
> +
> +    sprintf(string, "%s%s%02x%02x", string, "esel", reqptr->selrecordms, reqptr->selrecordls);

snprintf.

Are selrecordms and selrecordls unique for every SEL? How will this
code behave if we get two SELs at the same time?

> +
> +
> +    // Length is the number of request bytes minus the header itself.
> +    // The header contains an extra byte to indicate the start of
> +    // the data (so didn't need to worry about word/byte boundaries)
> +    // hence the -1...
> +    rlen = ((unsigned short)*data_len) - (sizeof(esel_request_t));

Why make data_len a size_t if we're just going to cast it away?

> +
> +
> +    printf("IPMI PARTIAL ESEL for %s  Offset = %d Length = %d\n",
> +        string, offset, rlen);
> +
> +    if (offset == 0) {
> +        strcpy(iocmd, "wb");
> +    } else {
> +        // I was thinking "ab+" but it appears it doesn't
> +        // do what fseek asks.  Modify to rb+ and fseek
> +        // works great...
> +        strcpy(iocmd, "rb+");

Could you just have a pointer, instead of copying memory around?

> +    }
> +
> +    if ((fp = fopen(string, iocmd)) != NULL) {
> +        fseek(fp, offset, SEEK_SET);
> +        fwrite(reqptr+1,rlen,1,fp);

Do we care about how much data is written?

> +        fclose(fp);
> +
> +

Whitespace.

> +        *data_len = sizeof(g_record_id);
> +        memcpy(response, &g_record_id, *data_len);
> +
> +

Whitespace

> +    } else {
> +        fprintf(stderr, "Error trying to perform %s for esel%s\n",iocmd, string);
> +        ipmi_ret_t rc = IPMI_CC_INVALID;
> +        *data_len     = 0;
> +    }
> +
> +    return rc;
> +}
> +
> +

Whitespace.

> +void register_netfn_oem_partial_esel()
> +{
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_OEM_IBM, IPMI_CMD_PESEL);
> +    ipmi_register_callback(NETFUN_OEM_IBM, IPMI_CMD_PESEL, NULL, ipmi_ibm_oem_partial_esel);
> +    return;
> +}
> diff --git a/oemhandler.h b/oemhandler.h
> new file mode 100644
> index 0000000..c176fd6
> --- /dev/null
> +++ b/oemhandler.h
> @@ -0,0 +1,32 @@
> +#ifndef __HOST_IPMI_OPENPOWEROEM_HANDLER_H__
> +#define __HOST_IPMI_OPENPOWEROEM_HANDLER_H__
> +
> +#include <stdint.h>
> +#include "ipmid-api.h"
> +
> +// IPMI commands for net functions.
> +enum ipmi_netfn_oem_cmds
> +{
> +    IPMI_CMD_PESEL = 0xF0,
> +};
> +
> +
> +

Whitespace.

> +ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> +                              ipmi_request_t request, ipmi_response_t response,
> +                              ipmi_data_len_t data_len, ipmi_context_t context);
> +
> +
> +struct esel_request_t {
> +    uint8_t  residls;
> +    uint8_t  residms;
> +    uint8_t  selrecordls;
> +    uint8_t  selrecordms;
> +    uint8_t  offsetls;
> +    uint8_t  offsetms;
> +    uint8_t  progress;
> +}  __attribute__ ((packed)) ;

Why the attribute?

> +
> +
> +
> +#endif
> --
> 2.6.0
>
>
> _______________________________________________
> openbmc mailing list
> openbmc at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc


More information about the openbmc mailing list