[Skiboot] [PATCH V2 08/15] core/pldm: Update or create terminus locator in the given repo
Christophe Lombard
clombard at linux.ibm.com
Wed May 24 19:50:05 AEST 2023
Le 26/04/2023 à 16:41, Frederic Barrat a écrit :
>
>
> On 29/04/2022 11:47, Christophe Lombard wrote:
>> The Terminus Locator PDR forms the association between a TID and PLDM
>> Terminus Handle for a terminus.
>> This patch allows to add terminus locator record in the repository.
>> If a record matches with the Host TID, we activate the current terminus
>> locator record, otherwise a new record is created.
>
>
> It seems that we always create the terminus locator record below.
>
> A good chunk of this patch is to create the boot progress sensor,
> which the commit message doesn't mention. I guess it should be its own
> separate patch.
>
>
Thanks, I will update the commit message
>>
>> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
>> ---
>> core/pldm/pldm-platform-requests.c | 180 +++++++++++++++++++++++++++++
>> core/pldm/pldm.h | 15 +++
>> 2 files changed, 195 insertions(+)
>>
>> diff --git a/core/pldm/pldm-platform-requests.c
>> b/core/pldm/pldm-platform-requests.c
>> index 06cf407f..e3263b07 100644
>> --- a/core/pldm/pldm-platform-requests.c
>> +++ b/core/pldm/pldm-platform-requests.c
>> @@ -14,10 +14,16 @@
>> #include "pldm.h"
>> #define NO_MORE_PDR_HANDLES 0
>> +#define PDR_AUTO_CALCULATE_RECORD_HANDLE 0
>> +static bool PDR_IS_NOT_REMOTE;
>
>
> Those 2 are hard-coded values we pass in a call to libpldm. Why treat
> them differently and have a variable and a macro?
>
>
>> static pldm_pdr *repo;
>> static bool pdr_ready;
>> +static uint32_t records_handle[20];
>> +static uint8_t hosted_pdrs;
>> +static int state_sensor_id = 1;
>> +
>> static void pdr_init_complete(bool success)
>> {
>> /* Read not successful, error out and free the buffer */
>> @@ -235,6 +241,145 @@ int pldm_platform_restart(void)
>> return set_state_effecter_states_req(effecter_id, &field, true);
>> }
>> +static int add_states_sensor_pdr(pldm_pdr *repo,
>> + uint32_t *record_handle,
>> + uint16_t state_set_id,
>> + uint32_t states)
>> +{
>> +
>> + struct state_sensor_possible_states *possible_states;
>> + struct pldm_state_sensor_pdr *pdr;
>> + uint8_t DEFAULT_CONTAINER_ID = 0;
>> + size_t state_size, pdr_size, actual_pdr_size = 0;
>> + uint8_t *state_storage;
>> + uint32_t swapped;
>> + int rc;
>> +
>> + /* fill in possible states structure */
>> + state_size = sizeof(struct state_sensor_possible_states)
>> + + sizeof(states)
>> + - sizeof(bitfield8_t);
>> + state_storage = malloc(state_size);
>> +
>> + possible_states = (struct state_sensor_possible_states *)
>> state_storage;
>> + possible_states->state_set_id = state_set_id;
>> + possible_states->possible_states_size = sizeof(states);
>> +
>> + /* need to swap the byte order for little endian order */
>> + swapped = htole32(states);
>> + memcpy(possible_states->states, &swapped, sizeof(swapped));
>
>
> Last 2 lines equivalent to:
> possible_states->states = htole32(states);
>
possible_states->states is a bitfield8_t parameter
>
>> +
>> + pdr_size = sizeof(struct pldm_state_sensor_pdr) + state_size;
>> + pdr = malloc(pdr_size);
>> +
>> + /* header */
>> + pdr->hdr.record_handle = 0; /* ask libpldm to fill this out */
>> + pdr->hdr.version = 0; /* will be filled out by the encoder */
>> + pdr->hdr.type = 0; /* will be filled out by the encoder */
>> + pdr->hdr.record_change_num = 0;
>> + pdr->hdr.length = 0; /* will be filled out by the encoder */
>> +
>> + /* body */
>> + pdr->terminus_handle = HOST_TID;
>> + pdr->sensor_id = state_sensor_id++;
>> + pdr->entity_type = PLDM_ENTITY_SYS_BOARD;
>> + pdr->entity_instance = 1;
>> + pdr->container_id = DEFAULT_CONTAINER_ID;
>> + pdr->sensor_init = PLDM_NO_INIT;
>> + pdr->sensor_auxiliary_names_pdr = false;
>> + pdr->composite_sensor_count = 1;
>> +
>> + rc = encode_state_sensor_pdr(pdr, pdr_size,
>> + possible_states,
>> + state_size,
>> + &actual_pdr_size);
>> +
>> + if (rc != PLDM_SUCCESS) {
>> + prlog(PR_ERR, "%s - Failed to encode state sensor PDR, rc:
>> %d\n",
>> + __func__, rc);
>> + free(state_storage);
>> + free(pdr);
>> + return rc;
>> + }
>> +
>> + *record_handle = pldm_pdr_add(repo,
>> + (const uint8_t *) pdr, actual_pdr_size,
>> + PDR_AUTO_CALCULATE_RECORD_HANDLE,
>> + PDR_IS_NOT_REMOTE);
>> +
>> + free(state_storage);
>> + free(pdr);
>> +
>> + return OPAL_SUCCESS;
>> +}
>> +
>> +/*
>> + * Add boot progress record in the repository.
>> + */
>> +static uint32_t add_boot_progress_pdr(pldm_pdr *repo,
>> + uint32_t *record_handle)
>> +{
>> + int rc;
>> +
>> + rc = add_states_sensor_pdr(
>> + repo,
>> + record_handle,
>> + PLDM_STATE_SET_BOOT_PROGRESS,
>> + enum_bit(PLDM_STATE_SET_BOOT_PROG_STATE_COMPLETED) |
>> + enum_bit(PLDM_STATE_SET_BOOT_PROG_STATE_PCI_RESORUCE_CONFIG) |
>> + enum_bit(PLDM_STATE_SET_BOOT_PROG_STATE_STARTING_OP_SYS));
>
>
> Related to a later patch, but I didn't see how the set of possible
> values was being used. The enum_bit() function seems to be a bit of an
> overkill.
>
possible_states->states is a bitfield8_t parameter. For this reason I
used enum_bit()
>
>> + if (rc) {
>> + prlog(PR_ERR, "%s - Failed to add states sensor PDR, rc: %d\n",
>> + __func__, rc);
>> + return rc;
>> + }
>> +
>> + prlog(PR_DEBUG, "Add boot progress pdr (record handle: %d)\n",
>> + *record_handle);
>> +
>> + return OPAL_SUCCESS;
>> +}
>> +
>> +/*
>> + * Add terminus locator record in the repository.
>> + */
>> +static int add_terminus_locator_pdr(pldm_pdr *repo,
>> + uint32_t *record_handle)
>> +{
>> + struct pldm_terminus_locator_type_mctp_eid *locator_value;
>> + struct pldm_terminus_locator_pdr pdr;
>> + uint8_t DEFAULT_CONTAINER_ID = 0;
>> + uint32_t size;
>> +
>> + pdr.hdr.record_handle = 0; /* record_handle will be generated
>> for us */
>> + pdr.hdr.version = 1;
>> + pdr.hdr.type = PLDM_TERMINUS_LOCATOR_PDR;
>> + pdr.hdr.record_change_num = 0;
>> + pdr.hdr.length = htole16(sizeof(struct pldm_terminus_locator_pdr) -
>> + sizeof(struct pldm_pdr_hdr));
>> + pdr.terminus_handle = htole16(HOST_TID);
>> + pdr.validity = PLDM_TL_PDR_VALID;
>> + pdr.tid = HOST_TID;
>> + pdr.container_id = DEFAULT_CONTAINER_ID;
>> + pdr.terminus_locator_type = PLDM_TERMINUS_LOCATOR_TYPE_MCTP_EID;
>> + pdr.terminus_locator_value_size = sizeof(struct
>> pldm_terminus_locator_type_mctp_eid);
>> + locator_value = (struct pldm_terminus_locator_type_mctp_eid
>> *)pdr.terminus_locator_value;
>> + locator_value->eid = HOST_EID;
>> +
>> + size = sizeof(struct pldm_terminus_locator_pdr) +
>> + sizeof(struct pldm_terminus_locator_type_mctp_eid);
>> +
>> + *record_handle = pldm_pdr_add(repo,
>> + (const uint8_t *)(&pdr), size,
>> + PDR_AUTO_CALCULATE_RECORD_HANDLE,
>> + PDR_IS_NOT_REMOTE);
>> +
>> + prlog(PR_DEBUG, "Add terminus locator pdr (record handle: %d)\n",
>> + *record_handle);
>> +
>> + return OPAL_SUCCESS;
>> +}
>> +
>> static int send_repository_changed_event(uint32_t num_changed_pdrs,
>> uint32_t *record_handle)
>> {
>> @@ -353,6 +498,33 @@ static int
>> send_repository_changed_event(uint32_t num_changed_pdrs,
>> return OPAL_SUCCESS;
>> }
>> +static int add_hosted_pdrs(pldm_pdr *repo)
>> +{
>> + uint32_t record_handle;
>> + int rc = OPAL_SUCCESS;
>> +
>> + hosted_pdrs = 0;
>> +
>> + rc = add_boot_progress_pdr(repo, &record_handle);
>> + if (!rc) {
>> + records_handle[hosted_pdrs] = record_handle;
>> + hosted_pdrs++;
>
>
> The way we handle the records_handle[] is dangerous: we don't check
> for overflow. Why is it a global? It looks like everything is handled
> in add_hosted_pdr(), so we know we have 2 records, so it could be
> simplified.
>
> Fred
>
>
>> + }
>> +
>> + rc = add_terminus_locator_pdr(repo, &record_handle);
>> + if (!rc) {
>> + records_handle[hosted_pdrs] = record_handle;
>> + hosted_pdrs++;
>> + }
>> +
>> + /* tell BMC that these PDRs have changed */
>> + rc = send_repository_changed_event(hosted_pdrs, records_handle);
>> + if (rc)
>> + prlog(PR_ERR, "%s - Failed to update hosted PDRs\n", __func__);
>> +
>> + return rc;
>> +}
>> +
>> struct get_pdr_response {
>> uint8_t completion_code;
>> uint32_t next_record_hndl;
>> @@ -553,6 +725,14 @@ int pldm_platform_init(void)
>> return rc;
>> }
>> + /* add hosted pdrs */
>> + rc = add_hosted_pdrs(repo);
>> + if (rc) {
>> + pdr_init_complete(false);
>> + prlog(PR_ERR, "%s - done, rc: %d\n", __func__, rc);
>> + return rc;
>> + }
>> +
>> pdr_init_complete(true);
>> prlog(PR_DEBUG, "%s - done\n", __func__);
>> diff --git a/core/pldm/pldm.h b/core/pldm/pldm.h
>> index bc399271..8ec72b77 100644
>> --- a/core/pldm/pldm.h
>> +++ b/core/pldm/pldm.h
>> @@ -34,6 +34,21 @@ extern int watchdog_period_sec;
>> /* For all of the encode functions just pass in a default ID (0x00) */
>> #define DEFAULT_INSTANCE_ID 0
>> +/* Return an integer with a bit set in the position corresponding to
>> + * the given enumeration (starting from 0 = the least significant
>> + * bit) and zeroes in the other positions.
>> + * Used for libpldm enumeration constants.
>> + *
>> + *
>> + * @example enum_bit(0) = 0x00000001
>> + * @example enum_bit(1) = 0x00000002
>> + * @example enum_bit(4) = 0x00000010
>> + */
>> +inline uint32_t enum_bit(unsigned int enumeration)
>> +{
>> + return 1 << enumeration;
>> +}
>> +
>> struct pldm_rx_data {
>> struct pldm_header_info hdrinf; /* parsed message header */
More information about the Skiboot
mailing list