[PATCH 1/2] efi_loader: move dp_alloc() to efi_alloc()

[Heinrich Schuchardt]

On 3/20/23 08:38, Ilias Apalodimas wrote:
> Hi Heinrich,
> 
> On Sun, Mar 19, 2023 at 09:20:22AM +0100, Heinrich Schuchardt wrote:
>> The incumbent function efi_alloc() is unused.
>>
>> Replace dp_alloc() by a new function efi_alloc() that we can use more
>> widely.
> 
> [...]
> 
>>   #include <efi_loader.h>
>>   #include <init.h>
>> +#include <log.h>
>>   #include <malloc.h>
>>   #include <mapmem.h>
>>   #include <watchdog.h>
>> @@ -533,27 +536,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>>   	return EFI_SUCCESS;
>>   }
>>
>> -/**
>> - * efi_alloc() - allocate memory pages
>> - *
>> - * @len:		size of the memory to be allocated
>> - * @memory_type:	usage type of the allocated memory
>> - * Return:		pointer to the allocated memory area or NULL
>> - */
>> -void *efi_alloc(uint64_t len, int memory_type)
>> -{
>> -	uint64_t ret = 0;
>> -	uint64_t pages = efi_size_in_pages(len);
>> -	efi_status_t r;
>> -
>> -	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages,
>> -			       &ret);
>> -	if (r == EFI_SUCCESS)
>> -		return (void*)(uintptr_t)ret;
>> -
>> -	return NULL;
>> -}
>> -
>>   /**
>>    * efi_free_pages() - free memory pages
>>    *
>> @@ -672,6 +654,28 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
>>   	return r;
>>   }
>>
>> +/**
>> + * efi_alloc() - allocate boot services data pool memory
>> + *
>> + * Allocate memory from pool and zero it out.
>> + *
>> + * @size:	number of bytes to allocate
>> + * Return:	pointer to allocated memory or NULL
>> + */
>> +void *efi_alloc(size_t size)
> 
> All our allocation related functions require the memory type to be passed.
> If we want to default this to 'EFI_BOOT_SERVICES_DATA' I think we need to
> change the name a bit to indicate that.

We have 13 instances of allocating EFI_BOOT_SERVICES_DATA from the pool 
and only 2 instances (see below) of other memory types. So this is the 
only case worth factoring out. I would prefer to keep identifiers short.

> 
>> +{
>> +	void *buf;
>> +
>> +	if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, size, &buf) !=
> 
> Is there a reason we are using efi_allocate_pool instead of
> efi_allocate_pages?

The UEFI specification explicitly requires pool memory in most places 
where the caller has to free memory. Freeing pages requires to know the 
size of the memory area while freeing from pool does not. Furthermore 
when freeing from pool we can check if the memory was ever allocated.

In future we may want to make small allocations from pool more efficient 
by not consuming a minimum of one page.

We could rethink why we are allocating configuration tables from pool in 
the following instances:

lib/efi_loader/efi_tcg2.c:1703
create_final_event()

lib/efi_loader/efi_runtime.c:116
efi_init_runtime_supported()

Best regards

Heinrich

> 
>> +	    EFI_SUCCESS) {
>> +		log_err("out of memory");
>> +		return NULL;
>> +	}
>> +	memset(buf, 0, size);
>> +
>> +	return buf;
>> +}
>> +
>>   /**
>>    * efi_free_pool() - free memory from pool
>>    *
>> --
>> 2.39.2
>>
> 
> Thanks
> /Ilias