| Message ID | 20220429094744.72855-3-clombard@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Complete PLDM responder and enable PLDM support | expand |
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@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
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@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
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; +}; + +#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", + __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; + pldm_watchdog_reset_timer(); + } + + return rc; +} + +static struct pldm_cmd pldm_platform_set_event_receiver = { + .name = "PLDM_SET_EVENT_RECEIVER", + .pldm_cmd_id = PLDM_SET_EVENT_RECEIVER, + .handler = platform_set_event_receiver_handler, +}; + int pldm_rx_handle_request(struct pldm_rx_data *rx) { const struct pldm_type *t; @@ -409,5 +501,9 @@ int pldm_mctp_responder_init(void) pldm_add_cmd(&pldm_base_type, &pldm_base_get_commands); pldm_add_cmd(&pldm_base_type, &pldm_base_get_version); + /* Register platform commands we'll respond to - DSP0248 */ + pldm_add_type(&pldm_platform_type); + pldm_add_cmd(&pldm_platform_type, &pldm_platform_set_event_receiver); + return OPAL_SUCCESS; } diff --git a/core/pldm/pldm.h b/core/pldm/pldm.h index a6de9ae8..5470f64a 100644 --- a/core/pldm/pldm.h +++ b/core/pldm/pldm.h @@ -12,6 +12,8 @@ /* Common support */ void printbuf(const char *name, const char *msg, int len); +extern bool watchdog_armed; +extern int watchdog_period_sec; /* * EID is the MCTP endpoint ID, which aids in routing MCTP packets.
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@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(+)