Re: [PATCH 3/5] efi_loader: use correct type for AllocatePages, AllocatePool

[Heinrich Schuchardt]

Am 18. August 2021 03:45:28 MESZ schrieb AKASHI Takahiro <takahiro.akashi at linaro.org>:
>Heinrich,
>
>On Tue, Aug 17, 2021 at 06:02:23PM +0200, Heinrich Schuchardt wrote:
>> Use enum efi_memory_type and enum_allocate_type in the definitions of the
>> efi_allocate_pages(), efi_allocate_pool().
>> 
>> In the external UEFI API leave the type as int as the UEFI specification
>> explicitly requires that enums use a 32bit type.
>> 
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>>  include/efi_loader.h        | 9 +++++----
>>  lib/efi_loader/efi_memory.c | 5 +++--
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 32cb8d0f1e..c440962fe5 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -676,13 +676,14 @@ struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid);
>>  /* Generic EFI memory allocator, call this to get memory */
>>  void *efi_alloc(uint64_t len, int memory_type);
>>  /* More specific EFI memory allocator, called by EFI payloads */
>> -efi_status_t efi_allocate_pages(int type, int memory_type, efi_uintn_t pages,
>> -				uint64_t *memory);
>> +efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>> +				enum efi_memory_type memory_type,
>> +				efi_uintn_t pages, uint64_t *memory);
>>  /* EFI memory free function. */
>>  efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages);
>>  /* EFI memory allocator for small allocations */
>> -efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size,
>> -			       void **buffer);
>> +efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
>> +			       efi_uintn_t size, void **buffer);
>>  /* EFI pool memory free function. */
>>  efi_status_t efi_free_pool(void *buffer);
>>  /* Returns the EFI memory map */
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index be2f655dff..f4acbee4f9 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -454,7 +454,8 @@ static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr)
>>   * @memory		allocated memory
>>   * @return		status code
>>   */
>> -efi_status_t efi_allocate_pages(int type, int memory_type,
>> +efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>> +				enum efi_memory_type memory_type,
>>  				efi_uintn_t pages, uint64_t *memory)
>>  {
>>  	u64 len = pages << EFI_PAGE_SHIFT;
>> @@ -556,7 +557,7 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>>   * @buffer:	allocated memory
>>   * Return:	status code
>>   */
>> -efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>> +efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer)
>
>Given the purpose of this patch series, I think that the second argument
>of this function should be renamed from "pool_type" to "memory_type"
>which is also used in efi_allocate_pages() to avoid any confusion.
>(and the description for @pool_type as well)

pool_type is the name used by the UEFI specification.

Best regards

Heinrich

>
>Otherwise, it looks good.
>
>-Takahiro Akashi
>
>
>>  {
>>  	efi_status_t r;
>>  	u64 addr;
>> -- 
>> 2.30.2
>>