[Skiboot] [PATCH V2 02/15] core/pldm: Decode the SetEventReceiver request

[Frederic Barrat]

On 29/04/2022 11:47, Christophe Lombard wrote:
> The SetEventReceiver command is used to set the address of the Event
> Receiver into a terminus that generates event messages. It is also used to
> globally enable or disable whether event messages are generated from the
> terminus.
> 
> For the time being, only the following global event message is supported:
> PLDM_EVENT_MESSAGE_GLOBAL_ENABLE_ASYNC_KEEP_ALIVE.
> 
> The Event Receiver acknowledges receiving the PLDM Event Message in the
> response to this command.
> 
> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
> ---
>   core/pldm/Makefile.inc     |  1 +
>   core/pldm/pldm-responder.c | 96 ++++++++++++++++++++++++++++++++++++++
>   core/pldm/pldm.h           |  2 +
>   3 files changed, 99 insertions(+)
> 
> diff --git a/core/pldm/Makefile.inc b/core/pldm/Makefile.inc
> index c6f78822..01117680 100644
> --- a/core/pldm/Makefile.inc
> +++ b/core/pldm/Makefile.inc
> @@ -10,6 +10,7 @@ CPPFLAGS += -I$(SRC)/pldm/ibm/libpldm/
>   CFLAGS_$(PLDM_DIR)/pldm-platform-requests.o = -Wno-strict-prototypes
>   CFLAGS_$(PLDM_DIR)/pldm-bios-requests.o = -Wno-strict-prototypes
>   CFLAGS_$(PLDM_DIR)/pldm-watchdog.o = -Wno-strict-prototypes
> +CFLAGS_$(PLDM_DIR)/pldm-responder.o = -Wno-strict-prototypes
>   
>   PLDM_OBJS = pldm-common.o pldm-responder.o pldm-requester.o
>   PLDM_OBJS += pldm-base-requests.o pldm-platform-requests.o
> diff --git a/core/pldm/pldm-responder.c b/core/pldm/pldm-responder.c
> index 16ce6b1f..232447e2 100644
> --- a/core/pldm/pldm-responder.c
> +++ b/core/pldm/pldm-responder.c
> @@ -8,6 +8,7 @@
>   #include <opal.h>
>   #include <stdio.h>
>   #include <string.h>
> +#include <pldm/libpldm/platform.h>
>   #include <pldm/libpldm/utils.h>
>   #include "pldm.h"
>   
> @@ -369,6 +370,97 @@ static struct pldm_cmd pldm_base_get_version = {
>   	.handler = base_get_version_handler,
>   };
>   
> +/*
> + * PLDM Platform commands support
> + */
> +static struct pldm_type pldm_platform_type = {
> +	.name = "platform",
> +	.pldm_type_id = PLDM_PLATFORM,
> +};
> +
> +struct event_receiver_req {
> +	uint8_t event_message_global_enable;
> +	uint8_t transport_protocol_type;
> +	uint8_t event_receiver_address_info;
> +	uint16_t heartbeat_timer;
> +};


why not reuse struct event_receiver_req from libpldm, which is 
identical? That would also be true for the following patches where we 
introduce several structures replicated in libpldm.


> +
> +#define MIN_WATCHDOG_TIMEOUT_SEC 15
> +
> +/*
> + * SetEventReceiver (0x04)
> + * The SetEventReceiver command is used to set the address of the Event
> + * Receiver into a terminus that generates event messages. It is also
> + * used to globally enable or disable whether event messages are
> + * generated from the terminus.
> + */
> +static int platform_set_event_receiver_handler(const struct pldm_rx_data *req)
> +{
> +	struct event_receiver_req receiver_req;
> +	uint8_t cc = PLDM_SUCCESS;
> +	int rc = OPAL_SUCCESS;
> +
> +	/* decode SetEventReceiver request data */
> +	rc = decode_set_event_receiver_req(
> +				req->msg,
> +				PLDM_SET_EVENT_RECEIVER_REQ_BYTES,
> +				&receiver_req.event_message_global_enable,
> +				&receiver_req.transport_protocol_type,
> +				&receiver_req.event_receiver_address_info,
> +				&receiver_req.heartbeat_timer);
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to decode SetEventReceiver request, rc = %d\n", rc);
> +		pldm_cc_resp(req, req->hdrinf.pldm_type,
> +			     req->hdrinf.command, PLDM_ERROR);
> +		return OPAL_INTERNAL_ERROR;
> +	}
> +
> +	/* invoke the appropriate callback handler */
> +	prlog(PR_DEBUG, "%s - event_message_global_enable: %d, "
> +			"transport_protocol_type: %d "
> +			"event_receiver_address_info: %d "
> +			"heartbeat_timer: 0x%x\n",


nitpick: could we print heartbeat_timer is decimal, since it's a number 
of seconds so it's more easily interpreted?


> +			__func__,
> +			receiver_req.event_message_global_enable,
> +			receiver_req.transport_protocol_type,
> +			receiver_req.event_receiver_address_info,
> +			receiver_req.heartbeat_timer);
> +
> +	if (receiver_req.event_message_global_enable !=
> +		PLDM_EVENT_MESSAGE_GLOBAL_ENABLE_ASYNC_KEEP_ALIVE) {
> +
> +		prlog(PR_ERR, "%s - invalid value for message global enable received: %d\n",
> +			      __func__, receiver_req.event_message_global_enable);
> +		cc = PLDM_PLATFORM_ENABLE_METHOD_NOT_SUPPORTED;
> +	}
> +
> +	if (receiver_req.heartbeat_timer < MIN_WATCHDOG_TIMEOUT_SEC) {
> +		prlog(PR_ERR, "%s - BMC requested watchdog timeout that's too small: %d\n",
> +			      __func__, receiver_req.heartbeat_timer);
> +		cc = PLDM_PLATFORM_HEARTBEAT_FREQUENCY_TOO_HIGH;
> +	} else {
> +		/* set the internal watchdog period to what BMC indicated */
> +		watchdog_period_sec = receiver_req.heartbeat_timer;
> +	}
> +
> +	/* send the response to BMC */
> +	pldm_cc_resp(req, PLDM_PLATFORM, PLDM_SET_EVENT_RECEIVER, cc);
> +
> +	/* no error happened above, so arm the watchdog and set the default timeout */
> +	if (cc == PLDM_SUCCESS) {
> +		watchdog_armed = true;


We should define a single call to trigger the heartbeat and define its 
rate instead of having 2 global variables.

   Fred