[PATCH 13/16] efi_loader: memory buffer for variables

[Punit Agrawal]

Heinrich Schuchardt <xypron.glpk at gmx.de> writes:

> On 3/27/20 9:09 AM, Punit Agrawal wrote:
>> Heinrich Schuchardt <xypron.glpk at gmx.de> writes:
>>
>>> Saving UEFI variable as encoded U-Boot environment variables does not allow
>>> support at runtime.
>>>
>>> Provide functions to manage a memory buffer with UEFI variables.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>>   include/efi_variable.h             |  16 ++
>>>   lib/efi_loader/Makefile            |   1 +
>>>   lib/efi_loader/efi_variables_mem.c | 317 +++++++++++++++++++++++++++++
>>>   3 files changed, 334 insertions(+)
>>>   create mode 100644 lib/efi_loader/efi_variables_mem.c
>>
>> [...]
>>
>>> diff --git a/lib/efi_loader/efi_variables_mem.c b/lib/efi_loader/efi_variables_mem.c
>>> new file mode 100644
>>> index 0000000000..f70cc65f8b
>>> --- /dev/null
>>> +++ b/lib/efi_loader/efi_variables_mem.c
>>> @@ -0,0 +1,317 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * File interface for UEFI variables
>>> + *
>>> + * Copyright (c) 2020, Heinrich Schuchardt
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <efi_loader.h>
>>> +#include <efi_variable.h>
>>> +#include <u-boot/crc.h>
>>> +
>>> +static struct efi_var_file __efi_runtime_data *efi_var_buf;
>>> +static struct efi_var_entry __efi_runtime_data *efi_current_var;
>>> +
>>> +/**
>>> + * memcpy() - copy memory area
>>> + *
>>> + * At runtime memcpy() is not available.
>>> + *
>>> + * @dest:	destination buffer
>>> + * @src:	source buffer
>>> + * @n:		number of bytes to copy
>>> + * Return:	pointer to destination buffer
>>> + */
>>> +void __efi_runtime efi_var_mem_memcpy(void *dest, const void *src, size_t n)
>>> +{
>>> +	u8 *d = dest;
>>> +	const u8 *s = src;
>>> +
>>> +	for (; n; --n)
>>> +		*d++ = *s++;
>>> +}
>>> +
>>> +/**
>>> + * efi_var_mem_compare() - compare GUID and name with a variable
>>> + *
>>> + * @var:	variable to compare
>>> + * @guid:	GUID to compare
>>> + * @name:	variable name to compare
>>> + * @next:	pointer to next variable
>>> + * Return:	true if match
>>> + */
>>> +static bool __efi_runtime
>>> +efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
>>> +		    const u16 *name, struct efi_var_entry **next)
>>> +{
>>> +	int i;
>>> +	u8 *guid1, *guid2;
>>> +	const u16 *data, *var_name;
>>> +	bool match = true;
>>> +
>>> +	for (guid1 = (u8 *)&var->guid, guid2 = (u8 *)guid, i = 0;
>>> +	     i < sizeof(efi_guid_t) && match; ++i)
>>> +		match = (guid1[i] == guid2[i]);
>>> +
>>> +	for (data = var->name, var_name = name;; ++data, ++var_name) {
>>> +		if (match)
>>> +			match = (*data == *var_name);
>>> +		if (!*data)
>>> +			break;
>>> +	}
>>
>> Don't roll out two different versions of memcmp() - make it a generic
>> helper and use it here.
>
> memcmp() has many architecture specific implementations. Currently all
> of these are gone at UEFI runtime.
>
> The loop above does not only do comparison but also seeks the end of the
> u16 string.

... which is needed for iteration over the variables. Thanks for
pointing this out.

One way to avoid the complexity would be keep track of the variable
length in efi_var_entry. Same applies for the "value" as well. That ways
you can skip to the next variable without having to increment two bytes
at a time.

>>
>>> +
>>> +	++data;
>>> +
>>> +	if (next)
>>> +		*next = (struct efi_var_entry *)
>>> +			ALIGN((uintptr_t)data + var->length, 8);
>>
>> Also, instead of implementing iteration via carrying the iterator
>> around, the readability of patches would be improved if this was done as
>> a simple loop outside of this function.
>>
>> If for some reason, this is considered to be better, please add it as a
>> comment in your next version.
>
> I could extract a function for comparing two u16 strings and return the
> end of the first string if they match and NULL otherwise.
>
> Would this match your expectations?

Going by the function name, I was expecting something along the lines of
-

    return !memcmp(guid1, guid2, 16) && !memcmp(var->name, name, var->length);
    
depending on whether length is tracking bytes used.

Hope that makes sense.

Punit

[...]