[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