[Skiboot] [PATCH V6 04/21] core/pldm: Add PLDM responder support

[Frederic Barrat]

On 13/09/2022 12:26, Christophe Lombard wrote:
> PLDM defines data representations and commands that abstract the platform
> management hardware.
> 
> A PLDM Terminus (or responder) is defined as the point of communication
> termination for PLDM messages and the PLDM functions associated with those
> messages.
> A PLDM terminus is not required to process more than one request at a time
> (that is, it can be "single threaded" and does not have to accept and act
> on new requests until it has finished responding to any previous request).
> 
> Some PLDM control and discovery requests (PLDM_TYPE = PLDM_BASE) are
> mandatory a PLDM terminus has to answer.
> 
> These following mandatory PLDM command codes for PLDM messaging control
> and discovery will be defined in next patches.
>      GetTID          0x02
>      GetPLDMVersion  0x03
>      GetPLDMTypes    0x04
>      GetPLDMCommands 0x05
> 
> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
> ---
>   core/pldm/Makefile.inc     |   2 +-
>   core/pldm/pldm-mctp.c      |  18 ++++-
>   core/pldm/pldm-responder.c | 145 +++++++++++++++++++++++++++++++++++++
>   core/pldm/pldm.h           |   6 ++
>   4 files changed, 169 insertions(+), 2 deletions(-)
>   create mode 100644 core/pldm/pldm-responder.c
> 
> diff --git a/core/pldm/Makefile.inc b/core/pldm/Makefile.inc
> index c878ef1f..dbca102a 100644
> --- a/core/pldm/Makefile.inc
> +++ b/core/pldm/Makefile.inc
> @@ -7,7 +7,7 @@ SUBDIRS += $(PLDM_DIR)
>   CPPFLAGS += -I$(SRC)/pldm/libpldm/
>   CPPFLAGS += -I$(SRC)/pldm/ibm/libpldm/
>   
> -PLDM_OBJS = pldm-mctp.o
> +PLDM_OBJS = pldm-mctp.o pldm-responder.o
>   
>   PLDM = $(PLDM_DIR)/built-in.a
>   $(PLDM): $(PLDM_OBJS:%=$(PLDM_DIR)/%)
> diff --git a/core/pldm/pldm-mctp.c b/core/pldm/pldm-mctp.c
> index 832b767a..51dbad0a 100644
> --- a/core/pldm/pldm-mctp.c
> +++ b/core/pldm/pldm-mctp.c
> @@ -75,6 +75,14 @@ static int handle_message_rx(uint8_t eid, const uint8_t *buf, int len)
>   		return OPAL_EMPTY;
>   	}
>   
> +	switch (rx.hdrinf.msg_type) {
> +	case PLDM_REQUEST:
> +		return pldm_responder_handle_request(&rx);
> +	break;
> +	default:
> +	break;


The indentation of "break" should be further.
Print an error in the default case, just in case? With prlog_once() to 
avoid flooding the log



> diff --git a/core/pldm/pldm.h b/core/pldm/pldm.h
> index a006d1f4..3add1bac 100644
> --- a/core/pldm/pldm.h
> +++ b/core/pldm/pldm.h
> @@ -20,6 +20,8 @@ void printbuf(const char *name, const char *msg, int len);
>   #define BMC_EID  8
>   #define HOST_EID 9
>   
> +#define PKT_SIZE(x) (sizeof(struct pldm_msg_hdr) + sizeof(x))


This is where I'm thinking we could add the +1 byte mentioned in the 
previous patch. And maybe provide macros to easily access the header, 
payload and MCTP type byte.

Regarding the name, it is my understanding that MCTP can break a single 
PLDM message into several packets. Here, we're allocating the full PLDM 
message. Maybe rename to PLDM_MSG_SIZE(x) or someting similar, avoiding 
'PKT' to avoid confusion?

   Fred