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

[Frederic Barrat]

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.


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


> +
> +		/* 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"

   Fred