Re: [PATCH 3/5] efi_loader: use correct type for AllocatePages, AllocatePool
Heinrich,
On Wed, Aug 18, 2021 at 02:23:40PM +0900, AKASHI Takahiro wrote:
> On Wed, Aug 18, 2021 at 06:50:40AM +0200, Heinrich Schuchardt wrote:
> > Am 18. August 2021 03:45:28 MESZ schrieb AKASHI Takahiro
> > :
> > >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
> > >> ---
> > >> 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.
>
> So what?
>
> (for efi_allocate_pages())
> !/*
> ! * Allocate memory pages.
> ! *
> ! * @typetype of allocation to be performed
> ! * @memory_type usage type of the allocated memory
>
> !/**
> ! * efi_allocate_pool - allocate memory from pool
> ! *
> ! * @pool_type: type of the pool from which memory is to be allocated
>
> You give the same type of arguments different names and explanation.
> I think that could be confusing.
> It is worth noting that efi_allocate_pool() directly passes
> the "pool_type" variable to efi_allocate_pages().
Did you ignore my comment?
-Takahiro Akashi
> -Takahiro Akashi
>
> > Best regards
> >
> > Heinrich
> >
> > >
> > >Otherwise, it looks good.
> > >
> > >-Takahiro Akashi
> > >
> > >
> > >> {
> > >> efi_status_t r;
> > >> u64 addr;
> > >> --
> > >> 2.30.2
> > >>
> >
Re: [PATCH 3/5] efi_loader: use correct type for AllocatePages, AllocatePool
On Wed, Aug 18, 2021 at 06:50:40AM +0200, Heinrich Schuchardt wrote:
> Am 18. August 2021 03:45:28 MESZ schrieb AKASHI Takahiro
> :
> >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
> >> ---
> >> 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)
> >> * @memoryallocated memory
> >> * @returnstatus 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.
So what?
(for efi_allocate_pages())
!/*
! * Allocate memory pages.
! *
! * @typetype of allocation to be performed
! * @memory_type usage type of the allocated memory
!/**
! * efi_allocate_pool - allocate memory from pool
! *
! * @pool_type: type of the pool from which memory is to be allocated
You give the same type of arguments different names and explanation.
I think that could be confusing.
It is worth noting that efi_allocate_pool() directly passes
the "pool_type" variable to efi_allocate_pages().
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> >
> >Otherwise, it looks good.
> >
> >-Takahiro Akashi
> >
> >
> >> {
> >>efi_status_t r;
> >>u64 addr;
> >> --
> >> 2.30.2
> >>
>
Re: [PATCH 3/5] efi_loader: use correct type for AllocatePages, AllocatePool
Am 18. August 2021 03:45:28 MESZ schrieb AKASHI Takahiro
:
>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
>> ---
>> 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
>>
Re: [PATCH 3/5] efi_loader: use correct type for AllocatePages, AllocatePool
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
> ---
> 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)
Otherwise, it looks good.
-Takahiro Akashi
> {
> efi_status_t r;
> u64 addr;
> --
> 2.30.2
>
[PATCH 3/5] efi_loader: use correct type for AllocatePages, AllocatePool
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
---
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)
{
efi_status_t r;
u64 addr;
--
2.30.2
