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

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@linux.vnet.ibm.com>
---
 core/pldm/pldm-platform-requests.c | 162 +++++++++++++++++++++++++++++
 include/pldm.h                     |   7 ++
 2 files changed, 169 insertions(+)

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

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