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

[Christophe Lombard]

Le 26/04/2023 à 15:10, Frederic Barrat a écrit :
>
>
> 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.
>

We want to avoid taking the address of packed member of struct 
pldm_set_event_receiver_req, which usually results in an unaligned 
pointer value.
For this reason we define a new structure based on what libpldm proposes.

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