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

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

On Tue, Sep 13, 2022 at 12:26:48PM +0200, 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@linux.vnet.ibm.com>
Reviewed-by: Abhishek Singh Tomar <abhishek@linux.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;
> +	}
> +
>  	return OPAL_UNSUPPORTED;
>  }
> 
> @@ -105,9 +113,17 @@ int pldm_mctp_init(uint8_t mode)
> 
>  	/* MCTP Binding */
>  	rc = ast_mctp_init(mode, message_rx);
> -	if (rc)
> +	if (rc) {
>  		prlog(PR_ERR, "Failed to bind MCTP\n");
> +		goto out;
> +	}
> +
> +	/* Register mandatory commands we'll respond to */
> +	rc = pldm_responder_init();
> +	if (rc)
> +		prlog(PR_ERR, "Failed to register mandatory commands\n");
> 
> +out:
>  	prlog(PR_NOTICE, "%s - done, rc: %d\n", __func__, rc);
>  	return rc;
>  }
> diff --git a/core/pldm/pldm-responder.c b/core/pldm/pldm-responder.c
> new file mode 100644
> index 00000000..94289cac
> --- /dev/null
> +++ b/core/pldm/pldm-responder.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +// Copyright 2022 IBM Corp.
> +
> +#define pr_fmt(fmt) "PLDM: " fmt
> +
> +#include <bitmap.h>
> +#include <cpu.h>
> +#include <opal.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <pldm/libpldm/utils.h>
> +#include "pldm.h"
> +
> +struct pldm_type {
> +	const char *name;
> +	int pldm_type_id;
> +	ver32_t version;
> +
> +	struct list_head commands;
> +	struct list_node link;
> +};
> +
> +struct pldm_cmd {
> +	const char *name;
> +	int pldm_cmd_id;
> +
> +	int (*handler)(const struct pldm_rx_data *req);
> +
> +	struct list_node link; /* link in the msg type's command list */
> +};
> +
> +/*
> + * Send a response with just a completion code and no payload
> + */
> +static int cc_resp(const struct pldm_rx_data *req, uint8_t type,
> +			uint8_t command, uint8_t cc)
> +{
> +	char response_msg[PKT_SIZE(uint8_t)];
> +	int rc;
> +
> +	memset(response_msg, 0, sizeof(response_msg));
> +
> +	encode_cc_only_resp(req->hdrinf.instance,
> +			    type,
> +			    command,
> +			    cc,
> +			    (struct pldm_msg *)response_msg);
> +
> +	rc = pldm_mctp_message_tx(req->source_eid, response_msg, sizeof(response_msg));
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to send response message containing only cc, "
> +			      "rc = %d, cc = %d\n", rc, cc);
> +		return OPAL_HARDWARE;
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +/*
> + * PLDM Type / Command wrangling.
> + */
> +LIST_HEAD(pldm_type_list);
> +
> +static const struct pldm_type *find_type(int type_id)
> +{
> +	struct pldm_type *iter;
> +
> +	list_for_each(&pldm_type_list, iter, link) {
> +		if (iter->pldm_type_id == type_id)
> +			return iter;
> +	}
> +
> +	return NULL;
> +}
> +
> +static const struct pldm_cmd *find_cmd(const struct pldm_type *type, int cmd)
> +{
> +	struct pldm_cmd *iter;
> +
> +	list_for_each(&type->commands, iter, link)
> +		if (iter->pldm_cmd_id == cmd)
> +			return iter;
> +
> +	return NULL;
> +}
> +
> +static void add_type(struct pldm_type *new_type)
> +{
> +	assert(new_type->pldm_type_id < 32); /* limited by GetPLDMTypes */
> +	assert(!find_type(new_type->pldm_type_id));
> +
> +	list_head_init(&new_type->commands);
> +	list_add_tail(&pldm_type_list, &new_type->link);
> +
> +	prlog(PR_DEBUG, "Registered type %s (%d)\n",
> +	      new_type->name, new_type->pldm_type_id);
> +}
> +
> +/*
> + * PLDM Base commands support
> + */
> +static struct pldm_type pldm_base_type = {
> +	.name = "base",
> +	.pldm_type_id = PLDM_BASE,
> +	.version = { 0xF1, 0xF0, 0xF0, 0x00 },
> +};
> +
> +int pldm_responder_handle_request(struct pldm_rx_data *rx)
> +{
> +	const struct pldm_type *type;
> +	const struct pldm_cmd *cmd;
> +
> +	prlog(PR_INFO, "Receive PLDM request from BMC, type: 0x%x, command: 0x%x\n",
> +			rx->hdrinf.pldm_type, rx->hdrinf.command);
> +
> +	type = find_type(rx->hdrinf.pldm_type);
> +	if (!type) {
> +		prlog(PR_ERR, "Type not supported, type: 0x%x\n",
> +			      rx->hdrinf.pldm_type);
> +		cc_resp(rx, rx->hdrinf.pldm_type,
> +			rx->hdrinf.command,
> +			PLDM_ERROR_INVALID_PLDM_TYPE);
> +		return OPAL_UNSUPPORTED;
> +	}
> +
> +	cmd = find_cmd(type, rx->hdrinf.command);
> +	if (!cmd) {
> +		prlog(PR_ERR, "Command not supported, type: 0x%x, command: 0x%x\n",
> +			      rx->hdrinf.pldm_type, rx->hdrinf.command);
> +		cc_resp(rx, rx->hdrinf.pldm_type,
> +			rx->hdrinf.command,
> +			PLDM_ERROR_UNSUPPORTED_PLDM_CMD);
> +		return OPAL_UNSUPPORTED;
> +	}
> +
> +	return cmd->handler(rx);
> +}
> +
> +int pldm_responder_init(void)
> +{
> +	/* Register mandatory commands we'll respond to - DSP0240 */
> +	add_type(&pldm_base_type);
> +
> +	return OPAL_SUCCESS;
> +}
> 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))
> +
>  struct pldm_rx_data {
>  	struct pldm_header_info hdrinf; /* parsed message header */
> 
> @@ -30,4 +32,8 @@ struct pldm_rx_data {
> 
>  int pldm_mctp_message_tx(uint8_t dest_id, uint8_t *buf, int len);
> 
> +/* Responder support */
> +int pldm_responder_handle_request(struct pldm_rx_data *rx);
> +int pldm_responder_init(void);
> +
>  #endif /* __COREPLDM_H__ */
> -- 
> 2.37.3
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

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

Le 12/04/2023 à 16:45, Frederic Barrat a écrit :
>
>
> 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@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
>

sounds good.

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


That could be an idea, but don't forget that some callers are not using 
PKT_SIZE.
They need to allocate a local buffer.