[PATCH 0/5] efi_loader: user EfiBootServicesData for device path
When creating device paths or converting the device paths to text we must memory of type EfiBootServiceData.
Change function definitions to use enums instead of int to avoid future misuse of constants.
Heinrich Schuchardt (5): efi_loader: use an enum for the memory allocation types efi_loader rename enum efi_mem_type to efi_memory_type efi_loader: use correct type for AllocatePages, AllocatePool efi_loader: use EfiBootServicesData for device path efi_loader: use EfiBootServicesData for DP to text
arch/x86/include/asm/hob.h | 2 +- include/efi.h | 36 +++++++++++++++++++----- include/efi_api.h | 2 +- include/efi_loader.h | 9 +++--- lib/efi_loader/efi_device_path.c | 2 +- lib/efi_loader/efi_device_path_to_text.c | 2 +- lib/efi_loader/efi_memory.c | 5 ++-- 7 files changed, 41 insertions(+), 17 deletions(-)
For type checking we need an enum.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- include/efi.h | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/include/efi.h b/include/efi.h index 6417a9b8c5..4eb573a280 100644 --- a/include/efi.h +++ b/include/efi.h @@ -125,6 +125,34 @@ struct efi_table_hdr { u32 reserved; };
+/* Allocation types for calls to boottime->allocate_pages*/ +/** + * enum efi_allocate_type - address restriction for memory allocation + */ +enum efi_allocate_type { + /** + * @EFI_ALLOCATE_ANY_PAGES: + * Allocate any block of sufficient size. Ignore memory address. + */ + EFI_ALLOCATE_ANY_PAGES, + /** + * @EFI_ALLOCATE_MAX_ADDRESS: + * Allocate a memory block with an uppermost address less or equal + * to the indicated address. + */ + EFI_ALLOCATE_MAX_ADDRESS, + /** + * @EFI_ALLOCATE_ADDRESS: + * Allocate a memory block starting at the indicated address. + */ + EFI_ALLOCATE_ADDRESS, + /** + * @EFI_MAX_ALLOCATE_TYPE: + * Value use for range checking. + */ + EFI_MAX_ALLOCATE_TYPE, +}; + /* Enumeration of memory types introduced in UEFI */ enum efi_mem_type { EFI_RESERVED_MEMORY_TYPE, @@ -224,12 +252,6 @@ struct efi_mem_desc {
#define EFI_MEMORY_DESCRIPTOR_VERSION 1
-/* Allocation types for calls to boottime->allocate_pages*/ -#define EFI_ALLOCATE_ANY_PAGES 0 -#define EFI_ALLOCATE_MAX_ADDRESS 1 -#define EFI_ALLOCATE_ADDRESS 2 -#define EFI_MAX_ALLOCATE_TYPE 3 - /* Types and defines for Time Services */ #define EFI_TIME_ADJUST_DAYLIGHT 0x1 #define EFI_TIME_IN_DAYLIGHT 0x2
Use the same name as in the UEFI specification to avoid confusion.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- arch/x86/include/asm/hob.h | 2 +- include/efi.h | 2 +- include/efi_api.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/hob.h b/arch/x86/include/asm/hob.h index 56e11dbb28..2f5b6e24c2 100644 --- a/arch/x86/include/asm/hob.h +++ b/arch/x86/include/asm/hob.h @@ -91,7 +91,7 @@ struct hob_mem_alloc { * Type EFI_MEMORY_TYPE is defined in AllocatePages() in the UEFI 2.0 * specification. */ - enum efi_mem_type mem_type; + enum efi_memory_type mem_type; /* padding */ u8 reserved[4]; }; diff --git a/include/efi.h b/include/efi.h index 4eb573a280..18c13e0370 100644 --- a/include/efi.h +++ b/include/efi.h @@ -154,7 +154,7 @@ enum efi_allocate_type { };
/* Enumeration of memory types introduced in UEFI */ -enum efi_mem_type { +enum efi_memory_type { EFI_RESERVED_MEMORY_TYPE, /* * The code portions of a loaded application. diff --git a/include/efi_api.h b/include/efi_api.h index 38ac47f164..c8f959bb72 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -249,7 +249,7 @@ struct efi_memory_range { struct efi_memory_range_capsule { struct efi_capsule_header *header; /* EFI_MEMORY_TYPE: 0x80000000-0xFFFFFFFF */ - enum efi_mem_type os_requested_memory_type; + enum efi_memory_type os_requested_memory_type; u64 number_of_memory_ranges; struct efi_memory_range memory_ranges[]; } __packed;
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@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) { efi_status_t r; u64 addr;
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@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)
Otherwise, it looks good.
-Takahiro Akashi
{ efi_status_t r; u64 addr; -- 2.30.2
Am 18. August 2021 03:45:28 MESZ schrieb AKASHI Takahiro takahiro.akashi@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@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
On Wed, Aug 18, 2021 at 06:50:40AM +0200, Heinrich Schuchardt wrote:
Am 18. August 2021 03:45:28 MESZ schrieb AKASHI Takahiro takahiro.akashi@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@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.
So what?
(for efi_allocate_pages()) !/* ! * Allocate memory pages. ! * ! * @type type 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
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 takahiro.akashi@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@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.
So what?
(for efi_allocate_pages()) !/* ! * Allocate memory pages. ! * ! * @type type 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
dp_alloc() was using a constant from the wrong enum resulting in creating device paths in EfiReservedMemory.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- lib/efi_loader/efi_device_path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 9c3ac712fe..cbdb466da4 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -68,7 +68,7 @@ static void *dp_alloc(size_t sz) { void *buf;
- if (efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES, sz, &buf) != + if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, sz, &buf) != EFI_SUCCESS) { debug("EFI: ERROR: out of memory in %s\n", __func__); return NULL;
Memory allocated in the implementation of the EFI_DEVICE_PATH_TO_TEXT_PROTOCOL must be of type EfiBootServicesData.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- lib/efi_loader/efi_device_path_to_text.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c index d46327a1c9..57fa9d97f7 100644 --- a/lib/efi_loader/efi_device_path_to_text.c +++ b/lib/efi_loader/efi_device_path_to_text.c @@ -34,7 +34,7 @@ static u16 *efi_str_to_u16(char *str) efi_status_t ret;
len = sizeof(u16) * (utf8_utf16_strlen(str) + 1); - ret = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES, len, (void **)&out); + ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, len, (void **)&out); if (ret != EFI_SUCCESS) return NULL; dst = out;
participants (3)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Heinrich Schuchardt