diff mbox series

efi_loader: get_memory_map: return parameters whenever possible

Message ID 20200311061818.382-1-takahiro.akashi@linaro.org
State Accepted
Commit b484296f6fda23ab2c996892826ebcc12cbd2303
Headers show
Series efi_loader: get_memory_map: return parameters whenever possible | expand

Commit Message

AKASHI Takahiro March 11, 2020, 6:18 a.m. UTC
Currently, if GetMemoryMap API returns EFI_BUFFER_TOO_SMALL, it doesn't
set valid values to other parameters, descriptor_size and
descriptor_version, except memory_map_size.
Some efi applications, however, may use those value; in particular,
xen uses descriptor_size to calculate a size of buffer to be allocated.

While UEFI specification is ambiguous in this point, it would be better
to address this issue proactively to maximize the compatibility with
existing efi applications.

With this patch, for example, xen.efi (and hence linux kernel) can be
started via bootefi without modification.

Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
---
 lib/efi_loader/efi_memory.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Heinrich Schuchardt March 11, 2020, 7:16 a.m. UTC | #1
On 3/11/20 7:18 AM, AKASHI Takahiro wrote:
> Currently, if GetMemoryMap API returns EFI_BUFFER_TOO_SMALL, it doesn't
> set valid values to other parameters, descriptor_size and
> descriptor_version, except memory_map_size.
> Some efi applications, however, may use those value; in particular,
> xen uses descriptor_size to calculate a size of buffer to be allocated.
>
> While UEFI specification is ambiguous in this point, it would be better
> to address this issue proactively to maximize the compatibility with
> existing efi applications.
>
> With this patch, for example, xen.efi (and hence linux kernel) can be
> started via bootefi without modification.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>

Your change matches the logic in EDK2.

There is still a difference. This is a snippet from
MdeModulePkg/Core/Dxe/Mem/Page.c:

   Size = sizeof (EFI_MEMORY_DESCRIPTOR);

   //
   // Make sure Size != sizeof(EFI_MEMORY_DESCRIPTOR). This will
   // prevent people from having pointer math bugs in their code.
   // now you have to use *DescriptorSize to make things work.
   //
   Size += sizeof(UINT64) - (Size % sizeof (UINT64));

   if (DescriptorSize != NULL) {
     *DescriptorSize = Size;
   }

If Size is not a multiple of 8 it rounds up to a multiple of 8. But if
Size is a multiple of 8 is 8 is added. The size of EFI_MEMORY_DESCRIPTOR
is a multiple of 8. - I have no clue why EDK2 is doing this and would
not advice to copy this.

Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

> ---
>   lib/efi_loader/efi_memory.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 89adf2031024..97d90f069a63 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -627,18 +627,18 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>
>   	*memory_map_size = map_size;
>
> -	if (provided_map_size < map_size)
> -		return EFI_BUFFER_TOO_SMALL;
> -
> -	if (!memory_map)
> -		return EFI_INVALID_PARAMETER;
> -
>   	if (descriptor_size)
>   		*descriptor_size = sizeof(struct efi_mem_desc);
>
>   	if (descriptor_version)
>   		*descriptor_version = EFI_MEMORY_DESCRIPTOR_VERSION;
>
> +	if (provided_map_size < map_size)
> +		return EFI_BUFFER_TOO_SMALL;
> +
> +	if (!memory_map)
> +		return EFI_INVALID_PARAMETER;
> +
>   	/* Copy list into array */
>   	/* Return the list in ascending order */
>   	memory_map = &memory_map[map_entries - 1];
>
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 89adf2031024..97d90f069a63 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -627,18 +627,18 @@  efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 
 	*memory_map_size = map_size;
 
-	if (provided_map_size < map_size)
-		return EFI_BUFFER_TOO_SMALL;
-
-	if (!memory_map)
-		return EFI_INVALID_PARAMETER;
-
 	if (descriptor_size)
 		*descriptor_size = sizeof(struct efi_mem_desc);
 
 	if (descriptor_version)
 		*descriptor_version = EFI_MEMORY_DESCRIPTOR_VERSION;
 
+	if (provided_map_size < map_size)
+		return EFI_BUFFER_TOO_SMALL;
+
+	if (!memory_map)
+		return EFI_INVALID_PARAMETER;
+
 	/* Copy list into array */
 	/* Return the list in ascending order */
 	memory_map = &memory_map[map_entries - 1];