[Skiboot] [PATCH V2 12/15] core/pldm: Update boot progress state

[Frederic Barrat]

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?


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

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__ */