[Skiboot] [PATCH V2 12/15] core/pldm: Update boot progress state
Christophe Lombard
clombard at linux.ibm.com
Wed May 24 20:19:22 AEST 2023
Le 26/04/2023 à 17:17, Frederic Barrat a écrit :
>
>
> On 29/04/2022 11:47, Christophe Lombard wrote:
>> PLDM Event Messages are PLDM monitoring and control messages that are
>> used
>> by a PLDM terminus to synchronously or asynchronously report PLDM events
>> to a central party called the PLDM Event Receiver.
>>
>> This patch allows to send a:
>> - generic sensor events (events related to PLDM numeric and state
>> sensors).
>> - boot progress sensor event.
>>
>> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
>> ---
>> core/pldm/pldm-platform-requests.c | 162 +++++++++++++++++++++++++++++
>> include/pldm.h | 7 ++
>> 2 files changed, 169 insertions(+)
>>
>> diff --git a/core/pldm/pldm-platform-requests.c
>> b/core/pldm/pldm-platform-requests.c
>> index e3263b07..75d84d59 100644
>> --- a/core/pldm/pldm-platform-requests.c
>> +++ b/core/pldm/pldm-platform-requests.c
>> @@ -241,6 +241,168 @@ int pldm_platform_restart(void)
>> return set_state_effecter_states_req(effecter_id, &field, true);
>> }
>> +static int send_sensor_state_changed_event(uint16_t state_set_id,
>> + uint16_t sensor_id,
>> + uint8_t sensor_offset,
>> + uint8_t sensor_state)
>> +{
>> + size_t event_data_size = 0, actual_event_data_size;
>> + size_t response_len, payload_len;
>> + uint8_t *event_data = NULL;
>> + uint32_t request_length;
>> + void *response_msg;
>> + char *request_msg;
>> + int rc, i;
>> +
>> + struct pldm_platform_event_message_req event_message_req = {
>> + .format_version = PLDM_PLATFORM_EVENT_MESSAGE_FORMAT_VERSION,
>> + .tid = HOST_TID,
>> + .event_class = PLDM_SENSOR_EVENT,
>> + };
>> +
>> + struct pldm_platform_event_message_resp response;
>> +
>> + prlog(PR_DEBUG, "%s - state_set_id: %d, sensor_id: %d,
>> sensor_state: %d\n",
>> + __func__, state_set_id, sensor_id, sensor_state);
>> +
>> + /*
>> + * The first time around this loop, event_data is nullptr which
>> + * instructs the encoder to not actually do the encoding, but
>> + * rather fill out actual_change_records_size with the correct
>> + * size, stop and return PLDM_SUCCESS. Then we allocate the
>> + * proper amount of memory and call the encoder again, which
>> + * will cause it to actually encode the message.
>> + */
>> + for (i = 0; i < 2; i++) {
>> + rc = encode_sensor_event_data(
>> + (struct pldm_sensor_event_data *)event_data,
>> + event_data_size,
>> + sensor_id,
>> + PLDM_STATE_SENSOR_STATE,
>> + sensor_offset,
>> + sensor_state,
>> + sensor_state,
>
>
> That is supposed to be the previous sensor state. That we don't have,
> since we don't update our own copy of the PDR.
>
>
>> + &actual_event_data_size);
>> + if (rc) {
>> + prlog(PR_ERR, "Encode PldmSensorChgEventData Error, rc:
>> %d\n", rc);
>> + return OPAL_PARAMETER;
>> + }
>> +
>> + if (event_data == NULL) {
>> + event_data_size = actual_event_data_size;
>> + event_data = malloc(event_data_size);
>
>
> Same pattern as before: newer code has a zalloc() but we should test
> for failed allocation.
>
>
>> + }
>> + }
>> +
>> + /* Send the event request */
>> + payload_len = PLDM_PLATFORM_EVENT_MESSAGE_MIN_REQ_BYTES +
>> event_data_size;
>> +
>> + request_length = sizeof(struct pldm_msg_hdr) +
>> + sizeof(struct pldm_platform_event_message_req) +
>> + event_data_size;
>> + request_msg = malloc(request_length);
>> +
>> + /* Encode the platform event message request */
>> + rc = encode_platform_event_message_req(
>> + DEFAULT_INSTANCE_ID,
>> + event_message_req.format_version,
>> + event_message_req.tid,
>> + event_message_req.event_class,
>> + (const uint8_t *)event_data,
>> + event_data_size,
>> + (struct pldm_msg *)request_msg,
>> + payload_len);
>> + if (rc != PLDM_SUCCESS) {
>> + prlog(PR_ERR, "Encode PlatformEventMessage Error, rc: %d\n",
>> rc);
>> + free(event_data);
>> + free(request_msg);
>> + return OPAL_PARAMETER;
>> + }
>> + free(event_data);
>> +
>> + /* Send and get the response message bytes */
>> + rc = pldm_do_request(BMC_EID, request_msg, request_length - 1,
>> + &response_msg, &response_len);
>> + if (rc) {
>> + prlog(PR_ERR, "Communication Error, req:
>> PlatformEventMessage, rc: %d\n", rc);
>> + free(request_msg);
>> + return rc;
>> + }
>> + free(request_msg);
>> +
>> + /* Decode the message */
>> + payload_len = response_len - sizeof(struct pldm_msg_hdr);
>> + rc = decode_platform_event_message_resp(
>> + response_msg,
>> + payload_len,
>> + &response.completion_code,
>> + &response.platform_event_status);
>> + if (rc != PLDM_SUCCESS || response.completion_code !=
>> PLDM_SUCCESS) {
>
>
> So if we set a bogus value (compared to what we defined when we
> created the sensor), this is where we would know?
>
Normally, encode_platform_event_message_req() prevents any errors. Here,
we just want to indicate a potentiel issue from the BMC.
>
>> + prlog(PR_ERR, "Decode PlatformEventMessage Error, rc: %d,
>> cc: %d, pes: %d\n",
>> + rc, response.completion_code,
>> + response.platform_event_status);
>> + free(response_msg);
>> + return OPAL_PARAMETER;
>> + }
>> +
>> + free(response_msg);
>> +
>> + return OPAL_SUCCESS;
>> +}
>> +
>> +#define BOOT_STATE_SENSOR_INDEX 0
>> +
>> +int pldm_platform_send_progress_state_change(
>> + enum pldm_state_set_boot_progress_state_values state)
>> +{
>> + struct state_sensor_possible_states *possible_states;
>> + struct pldm_state_sensor_pdr *sensor_pdr = NULL;
>> + const pldm_pdr_record *record = NULL;
>> + uint16_t terminus_handle;
>> + uint8_t *outData = NULL;
>> + uint16_t sensor_id = 0;
>> + uint32_t size;
>> +
>> + prlog(PR_INFO, "Setting boot progress, state: %d\n", state);
>> +
>> + do {
>> + /* Find (first) PDR record by PLDM_STATE_SENSOR_PDR type
>> + * if record not NULL, then search will begin from this
>> + * record's next record
>> + */
>> + record = pldm_pdr_find_record_by_type(
>> + repo, /* PDR repo handle */
>> + PLDM_STATE_SENSOR_PDR,
>> + record, /* PDR record handle */
>> + &outData, &size);
>> +
>> + if (record) {
>> + sensor_pdr = (struct pldm_state_sensor_pdr *) outData;
>> + terminus_handle = le16_to_cpu(sensor_pdr->terminus_handle);
>> +
>> + if ((le16_to_cpu(sensor_pdr->entity_type) ==
>> PLDM_ENTITY_SYS_BOARD) &&
>> + (terminus_handle == HOST_TID)) {
>> + possible_states = (struct
>> state_sensor_possible_states *)
>> + sensor_pdr->possible_states;
>> +
>> + if (le16_to_cpu(possible_states->state_set_id) ==
>> + PLDM_STATE_SET_BOOT_PROGRESS)
>> + sensor_id = le16_to_cpu(sensor_pdr->sensor_id);
>
>
> We should break out of the loop here once we find the sensor ID.
>
>
>> + }
>> + }
>> +
>> + } while (record);
>
>
> So we create the sensor PDR at boot. But then we search for it in the
> full repo each time we want to update it. Couldn't we save it?
>
We create a boot progress record, with some specific values indicating
the Opal's progress during the boot:
PLDM_STATE_SET_BOOT_PROG_STATE_COMPLETED
PLDM_STATE_SET_BOOT_PROG_STATE_PCI_RESORUCE_CONFIG
PLDM_STATE_SET_BOOT_PROG_STATE_STARTING_OP_SYS
During the boot, we will use pldm_platform_send_progress_state_change()
with previous states, as IPMI done
> Side question: IIUC, at boot, we create a PDR repo that we fill up
> with all the records that the BMC gives us. My understanding is that's
> needed because we'll need to find a couple of effecter IDs in there,
> but mostly the info about the LIDs.
> Then we add a couple of local entries *in the same repo*. Do we have
> to? The search would be a lot faster if we separate local vs. BMC
> records. To be discussed...
>
> Fred
>
>
>> +
>> + if (sensor_id == 0)
>> + return OPAL_PARAMETER;
>> +
>> + return send_sensor_state_changed_event(
>> + PLDM_STATE_SET_BOOT_PROGRESS,
>> + sensor_id,
>> + BOOT_STATE_SENSOR_INDEX,
>> + state);
>> +}
>> +
>> static int add_states_sensor_pdr(pldm_pdr *repo,
>> uint32_t *record_handle,
>> uint16_t state_set_id,
>> diff --git a/include/pldm.h b/include/pldm.h
>> index 01af9a33..5acdcbbe 100644
>> --- a/include/pldm.h
>> +++ b/include/pldm.h
>> @@ -6,6 +6,7 @@
>> #define __PLDM_H__
>> #include <skiboot.h>
>> +#include <pldm/libpldm/state_set.h>
>> /**
>> * PLDM over MCTP initialization
>> @@ -47,4 +48,10 @@ bool pldm_lid_files_exit(struct blocklevel_device
>> *bl);
>> */
>> int pldm_watchdog_init(void);
>> +/**
>> + * Update boot progress state
>> + */
>> +int pldm_platform_send_progress_state_change(
>> + enum pldm_state_set_boot_progress_state_values state);
>> +
>> #endif /* __PLDM_H__ */
More information about the Skiboot
mailing list