[Skiboot] [PATCH V3 1/2] core/pldm: Lid ids string parsing

[Christophe Lombard]

Le 14/04/2023 à 16:04, Frederic Barrat a écrit :
>
>
> On 29/04/2022 11:46, Christophe Lombard wrote:
>> This patch parses the "hb_lid_ids" string from bios tables and complete
>> the global list of lid files. Each entry in the list contains the name,
>> the id, the length of the lid file and the virtual address start access.
>> This virtual address is used for for PNOR Resource Provider operations.
>> 16 MB of VMM address are reserved  space per section.
>>
>> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
>> ---
>>   core/pldm/Makefile.inc     |   2 +-
>>   core/pldm/pldm-lid-files.c | 170 +++++++++++++++++++++++++++++++++++++
>>   include/pldm.h             |   7 ++
>>   3 files changed, 178 insertions(+), 1 deletion(-)
>>   create mode 100644 core/pldm/pldm-lid-files.c
>>
>> diff --git a/core/pldm/Makefile.inc b/core/pldm/Makefile.inc
>> index d3affaa0..506bf5d7 100644
>> --- a/core/pldm/Makefile.inc
>> +++ b/core/pldm/Makefile.inc
>> @@ -13,7 +13,7 @@ CFLAGS_$(PLDM_DIR)/pldm-bios-requests.o = 
>> -Wno-strict-prototypes
>>   PLDM_OBJS = pldm-common.o pldm-responder.o pldm-requester.o
>>   PLDM_OBJS += pldm-base-requests.o pldm-platform-requests.o
>>   PLDM_OBJS += pldm-bios-requests.o pldm-fru-requests.o
>> -PLDM_OBJS += pldm-file-io-requests.o
>> +PLDM_OBJS += pldm-file-io-requests.o pldm-lid-files.o
>>     PLDM = $(PLDM_DIR)/built-in.a
>>   $(PLDM): $(PLDM_OBJS:%=$(PLDM_DIR)/%)
>> diff --git a/core/pldm/pldm-lid-files.c b/core/pldm/pldm-lid-files.c
>> new file mode 100644
>> index 00000000..11e2f9e2
>> --- /dev/null
>> +++ b/core/pldm/pldm-lid-files.c
>> @@ -0,0 +1,170 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>> +// Copyright 2022 IBM Corp.
>> +
>> +#define pr_fmt(fmt) "PLDM: " fmt
>> +
>> +#include <endian.h>
>> +#include <lock.h>
>> +#include <opal-api.h>
>> +#include <libflash/errors.h>
>> +#include <libflash/ffs.h>
>> +#include "pldm.h"
>> +
>> +/*
>> + * This struct is used to map a PNOR sections.
>> + * The content is deriving from the hb_lid_ids PLDM BIOS Attribute.
>> + */
>> +struct pldm_lid {
>> +    struct list_node    list;
>> +    uint32_t        start;
>> +    uint32_t        handle;
>> +    uint32_t        length;
>> +    char            name[PART_NAME_MAX + 1];
>> +    char            id[PART_NAME_MAX + 1];
>> +};
>> +
>> +static LIST_HEAD(lid_files);
>> +
>> +#define MEGABYTE (1024*1024)
>> +
>> +/*
>> + * When using PLDM for PNOR Resource Provider operations,
>> + * reserve 16 MB of VMM address space per section.
>> + * Note that all of this space may not actually be used by each 
>> section.
>> + */
>> +#define VMM_SIZE_RESERVED_PER_SECTION (16 * MEGABYTE)
>> +
>> +/*
>> + * Print the attributes of lid files.
>> + */
>> +static void print_lid_files_attr(void)
>> +{
>> +    struct pldm_lid *lid = NULL;
>> +
>> +    list_for_each(&lid_files, lid, list)
>> +        prlog(PR_NOTICE, "name: %s, id: %s, handle: %d, length: 
>> 0x%x, start: 0x%x\n",
>> +                 lid->name, lid->id, lid->handle, lid->length, 
>> lid->start);
>> +
>> +}
>> +
>> +/*
>> + * Return the number of lid files.
>> + */
>> +static uint32_t get_lids_count(void)
>> +{
>> +    struct pldm_lid *lid = NULL;
>> +    uint32_t count = 0;
>> +
>> +    list_for_each(&lid_files, lid, list)
>> +        count++;
>> +
>> +    return count;
>> +}
>> +
>> +/*
>> + * parse the "hb_lid_ids" string
>> + * <ATTR_a>=<lid_id_1>,<ATTR_b>=<lid_id_2>
>> + */
>> +static int parse_hb_lid_ids_string(char *str)
>> +{
>> +    struct pldm_lid *lid;
>> +    const char *pp = "=";
>> +    char *attr, *attr_end;
>> +    int rc, count = 1;
>> +    char *lid_id;
>> +
>> +    for (char *p = strtok(str, ","); p != NULL; p = strtok(NULL, 
>> ",")) {
>> +        lid = zalloc(sizeof(struct pldm_lid));
>> +        if (!lid) {
>> +            prlog(PR_ERR, "Error allocating pldm_lid structure\n");
>> +            rc = OPAL_RESOURCE;
>> +            goto err;
>> +        }
>> +
>> +        /* parse the string <attr>=<lid_id> */
>> +        attr = p;
>> +        while (*pp != *p)
>> +            p++;
>
>
> Same comment as in a previous patch: we should protect ourselves 
> against malformed expressions in case the string doesn't have a "=" in 
> it.

Normally it should never, but you never know. Thanks.

>
>
>> +
>> +        attr_end = p;
>> +        lid_id = ++p;
>> +        *attr_end = '\0';
>> +
>> +        strcpy(lid->name, attr);
>> +        strcpy(lid->id, lid_id);
>> +
>> +        /* reserve 16 MB of VMM address space per section */
>> +        lid->start = VMM_SIZE_RESERVED_PER_SECTION * count;
>
>
> I was wondering by we start with count = 1, i.e the very first section 
> address it not 0. I found the answer in the next patch (I think :) ), 
> as we want to emulate a flash rom and we reserve address 0 for the 
> header. A comment could be useful.
>
>
>> +
>> +        /* handle and length */
>> +        rc = pldm_find_file_handle_by_lid_id(lid->id,
>> +                             &lid->handle,
>> +                             &lid->length);
>> +        if ((rc) && (rc != OPAL_PARAMETER))
>> +            goto err;
>
>
> This is where I think it would make sense to check and err out if the 
> lid length is bigger than the VMM_SIZE_RESERVED_PER_SECTION space we 
> reserve.

Correct.

>
>
>> +
>> +        /* add new member in the global list */
>> +        list_add_tail(&lid_files, &lid->list);
>> +
>> +        count++;
>> +    }
>> +
>> +    return OPAL_SUCCESS;
>> +
>> +err:
>> +    /* free all lid entries */
>> +    list_for_each(&lid_files, lid, list)
>> +        free(lid);
>> +
>> +    return rc;
>> +}
>> +
>> +/*
>> + * Parse the "hb_lid_ids" string from bios tables and complete
>> + * the global list of lid files.
>> + */
>> +static int lid_ids_to_vaddr_mapping(void)
>> +{
>> +    char *lid_ids_string;
>
>
> lid_ids_string must be initialized to NULL because of "goto out"

Correct. Thanks.

>
>   Fred
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20230418/89135428/attachment-0001.htm>