[U-Boot] [PATCH 3/3] efi: update virtual address in efi_mem_carve_out

[Heinrich Schuchardt]

On 4/10/19 11:02 AM, Patrick Delaunay wrote:
> Handle virtual address in efi_mem_carve_out function
> when a new region is created to avoid issue in EFI memory map.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> ---
>
> Issue detected during test of test of UEFI on ev1/dk2 boards.
>
> For debug purpose, I add the dump of the map in efi_mem_sort:
>
> static void efi_mem_sort(void)
> {
> .....
> 	int i = 0;
> 	printf("-- %s -------------------\n", __func__ );
> 	list_for_each(lhandle, &efi_mem) {
> 		struct efi_mem_list *lmem;
> 		struct efi_mem_desc *cur;
>
> 		lmem = list_entry(lhandle, struct efi_mem_list, link);
> 		cur = &lmem->desc;
>
> 		printf("%02d: va %08x pa %08x size %08x (%04x pages) => va end %08x attribute = %llx type=%d\n",
> 			i++,
> 			(u32)cur->virtual_start,
> 			(u32)cur->physical_start,
> 			(u32)(cur->num_pages << EFI_PAGE_SHIFT),
> 			(u32)cur->num_pages,
> 			(u32) (cur->virtual_start + (cur->num_pages << EFI_PAGE_SHIFT)),
> 			cur->attribute,
> 			cur->type);
> 	}
> }
>
> I get the memory layout (for EV1):
>
> 00: va fcf87000 pa fff92000 size 0006e000 (006e pages) => va end fcff5000 attribute = 8 type=2
> 01: va fff91000 pa fff91000 size 00001000 (0001 pages) => va end fff92000 attribute = 8000000000000008 type=5
> 02: va fcf87000 pa fcf87000 size 0300a000 (300a pages) => va end fff91000 attribute = 8 type=2
> 03: va fcf86000 pa fcf86000 size 00001000 (0001 pages) => va end fcf87000 attribute = 8000000000000008 type=6
> 04: va fcf46000 pa fcf7d000 size 00009000 (0009 pages) => va end fcf4f000 attribute = 8 type=0
> 05: va fcf7c000 pa fcf7c000 size 00001000 (0001 pages) => va end fcf7d000 attribute = 8000000000000008 type=6
> 06: va fcf46000 pa fcf77000 size 00005000 (0005 pages) => va end fcf4b000 attribute = 8 type=0
> 07: va c0000000 pa c0000000 size 3cf77000 (3cf77 pages) => va end fcf77000 attribute = 8 type=7
>
> But for some region virtual address (va) != physical address (pa)
>
> After check of the code, the virtual address is not correctly managed
> when new region is created.
>
> I don't sure that could be a issue, but I solve the issue
> with the proposed patch:
>
> 00: va fff92000 pa fff92000 size 0006e000 (006e pages) => va end 00000000 attribute = 8 type=2
> 01: va fff91000 pa fff91000 size 00001000 (0001 pages) => va end fff92000 attribute = 8000000000000008 type=5
> 02: va fcf87000 pa fcf87000 size 0300a000 (300a pages) => va end fff91000 attribute = 8 type=2
> 03: va fcf86000 pa fcf86000 size 00001000 (0001 pages) => va end fcf87000 attribute = 8000000000000008 type=6
> 04: va fcf7d000 pa fcf7d000 size 00009000 (0009 pages) => va end fcf86000 attribute = 8 type=0
> 05: va fcf7c000 pa fcf7c000 size 00001000 (0001 pages) => va end fcf7d000 attribute = 8000000000000008 type=6
> 06: va fcf77000 pa fcf77000 size 00005000 (0005 pages) => va end fcf7c000 attribute = 8 type=0
> 07: va c0000000 pa c0000000 size 3cf77000 (3cf77 pages) => va end fcf77000 attribute = 8 type=7
>
> The virtual address are now correct.
>
> PS: I don't found more elegant code to update the virtual address
>      but I avoid to force virtual_address = physical address
>      in this function.
>
>   lib/efi_loader/efi_memory.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 81dc5fc..a47adef 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -168,6 +168,16 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
>   			free(map);
>   		} else {
>   			map->desc.physical_start = carve_end;

Thanks for reporting the issue. I think you correctly identified the two
places where the update of the virtual address is missing.

The efi_mem_carve_out() function is called efi_add_memory_map() which is
only called at boottime. By definition the physical and virtual
addresses must be the same at boottime. So a single line

map->desc.virtual_start = carve_end;

should be enough here.

> +			if (carve_end == map_end)
> +				map->desc.virtual_start =
> +					map_desc->virtual_start +
> +					(map_desc->num_pages << EFI_PAGE_SHIFT);
> +			else
> +				map->desc.virtual_start =
> +					carve_desc->virtual_start +
> +					(carve_desc->num_pages <<
> +					 EFI_PAGE_SHIFT);
> +
>   			map->desc.num_pages = (map_end - carve_end)
>   					      >> EFI_PAGE_SHIFT;
>   		}
> @@ -186,6 +196,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
>   	newmap = calloc(1, sizeof(*newmap));
>   	newmap->desc = map->desc;
>   	newmap->desc.physical_start = carve_start;

newmap->desc.virtual_start = carve_start;

should be ok here.

Best regards

Heinrich

> +	if (carve_start == map_start)
> +		newmap->desc.virtual_start = map_desc->virtual_start;
> +	else
> +		newmap->desc.virtual_start = carve_desc->virtual_start;
> +
>   	newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT;
>   	/* Insert before current entry (descending address order) */
>   	list_add_tail(&newmap->link, &map->link);
>