All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G
@ 2019-04-10  9:02 Patrick Delaunay
  2019-04-10  9:02 ` [U-Boot] [PATCH 2/3] efi: add protection for block_dev Patrick Delaunay
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Patrick Delaunay @ 2019-04-10  9:02 UTC (permalink / raw)
  To: u-boot

Avoid ram_end = 0 on 32bit targets with ram_end at 4G.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---
example of issue in stm32mp1 board ev1:
  ram_start = c0000000
  size = 40000000
  ram_end = 100000000
  ram_end &= ~EFI_PAGE_MASK => result is 0

 lib/efi_loader/efi_memory.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 55622d2..81dc5fc 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -574,6 +574,10 @@ __weak void efi_add_known_memory(void)
 
 		/* Remove partial pages */
 		ram_end &= ~EFI_PAGE_MASK;
+		/* Fix for 32bit targets with ram_top at 4G */
+		if (!ram_end)
+			ram_end = 0x100000000ULL;
+
 		ram_start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
 
 		if (ram_end <= ram_start) {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH 2/3] efi: add protection for block_dev
  2019-04-10  9:02 [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G Patrick Delaunay
@ 2019-04-10  9:02 ` Patrick Delaunay
  2019-04-10 17:42   ` Heinrich Schuchardt
  2019-04-11 17:26   ` Heinrich Schuchardt
  2019-04-10  9:02 ` [U-Boot] [PATCH 3/3] efi: update virtual address in efi_mem_carve_out Patrick Delaunay
  2019-04-10 16:40 ` [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G Patrick Wildt
  2 siblings, 2 replies; 9+ messages in thread
From: Patrick Delaunay @ 2019-04-10  9:02 UTC (permalink / raw)
  To: u-boot

Check the value of block_dev before to use this pointer.

This patch solves problem for the command "load" when ubifs
is previously mounted: in this case the function
blk_get_device_part_str("ubi 0") don't return error but return
block_dev = NULL and then data abort.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

To reproduce the issue, I have a boot script 'boot.scr.uimg'
with a load command executed during ubi boot:

load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}

I have a data abort for call stack:
- do_load_wrapper for "ubi 0"
-- efi_set_bootdev
--- efi_dp_from_name

=> desc = 0 and data abort for access to 'desc->*'

I also proposed a protection for the same issue in ums command
http://patchwork.ozlabs.org/project/uboot/list/?series=68096


 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 53b40c8..fd57be8 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 	if (!is_net) {
 		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
 					       1);
-		if (part < 0)
+		if (part < 0 || !desc)
 			return EFI_INVALID_PARAMETER;
 
 		if (device)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH 3/3] efi: update virtual address in efi_mem_carve_out
  2019-04-10  9:02 [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G Patrick Delaunay
  2019-04-10  9:02 ` [U-Boot] [PATCH 2/3] efi: add protection for block_dev Patrick Delaunay
@ 2019-04-10  9:02 ` Patrick Delaunay
  2019-04-11 18:12   ` Heinrich Schuchardt
  2019-04-10 16:40 ` [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G Patrick Wildt
  2 siblings, 1 reply; 9+ messages in thread
From: Patrick Delaunay @ 2019-04-10  9:02 UTC (permalink / raw)
  To: u-boot

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@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;
+			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;
+	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);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G
  2019-04-10  9:02 [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G Patrick Delaunay
  2019-04-10  9:02 ` [U-Boot] [PATCH 2/3] efi: add protection for block_dev Patrick Delaunay
  2019-04-10  9:02 ` [U-Boot] [PATCH 3/3] efi: update virtual address in efi_mem_carve_out Patrick Delaunay
@ 2019-04-10 16:40 ` Patrick Wildt
  2019-04-11  8:03   ` Patrick DELAUNAY
  2 siblings, 1 reply; 9+ messages in thread
From: Patrick Wildt @ 2019-04-10 16:40 UTC (permalink / raw)
  To: u-boot

On Wed, Apr 10, 2019 at 11:02:57AM +0200, Patrick Delaunay wrote:
> Avoid ram_end = 0 on 32bit targets with ram_end at 4G.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> example of issue in stm32mp1 board ev1:
>   ram_start = c0000000
>   size = 40000000
>   ram_end = 100000000
>   ram_end &= ~EFI_PAGE_MASK => result is 0
> 
>  lib/efi_loader/efi_memory.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 55622d2..81dc5fc 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -574,6 +574,10 @@ __weak void efi_add_known_memory(void)
>  
>  		/* Remove partial pages */
>  		ram_end &= ~EFI_PAGE_MASK;
> +		/* Fix for 32bit targets with ram_top at 4G */
> +		if (!ram_end)
> +			ram_end = 0x100000000ULL;
> +
>  		ram_start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
>  
>  		if (ram_end <= ram_start) {
> -- 
> 2.7.4
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Hi,

I had the exact same issue yesterday and posted a diff which I think
is a better approach:

https://patchwork.ozlabs.org/patch/1082739/

Best regards,
another Patrick

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH 2/3] efi: add protection for block_dev
  2019-04-10  9:02 ` [U-Boot] [PATCH 2/3] efi: add protection for block_dev Patrick Delaunay
@ 2019-04-10 17:42   ` Heinrich Schuchardt
  2019-04-11  9:30     ` Patrick DELAUNAY
  2019-04-11 17:26   ` Heinrich Schuchardt
  1 sibling, 1 reply; 9+ messages in thread
From: Heinrich Schuchardt @ 2019-04-10 17:42 UTC (permalink / raw)
  To: u-boot

On 4/10/19 11:02 AM, Patrick Delaunay wrote:
> Check the value of block_dev before to use this pointer.
>
> This patch solves problem for the command "load" when ubifs
> is previously mounted: in this case the function
> blk_get_device_part_str("ubi 0") don't return error but return
> block_dev = NULL and then data abort.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
> To reproduce the issue, I have a boot script 'boot.scr.uimg'
> with a load command executed during ubi boot:
>
> load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}
>
> I have a data abort for call stack:
> - do_load_wrapper for "ubi 0"
> -- efi_set_bootdev
> --- efi_dp_from_name
>
> => desc = 0 and data abort for access to 'desc->*'

Thanks for reporting and analyzing the problem

Where exactly is the NULL dereference occurring?

Igor reported a similar bug for a USB device in
cmd: fs: fix data abort in load cmd
https://lists.denx.de/pipermail/u-boot/2019-April/364484.htmll

>
> I also proposed a protection for the same issue in ums command
> http://patchwork.ozlabs.org/project/uboot/list/?series=68096
>
>
>   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 53b40c8..fd57be8 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>   	if (!is_net) {
>   		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
>   					       1);
> -		if (part < 0)
> +		if (part < 0 || !desc)

part = 0, desc = NULL occurs for UBI if the UBI file system is mounted.

Returning an error here means in the end that we will not be able to
install run GRUB from the UBI device because we cannot describe the boot
device.

I think that UBI volumes should be handled like any other block device.
This will avoid having separate program paths for UBI and not UBI.

Heiko and Kyungmin could you, please, explain why UBI currently is not
providing a struct blk_desc * block descriptor and how this can be fixed.

Best regards

Heinrich

 >   			return EFI_INVALID_PARAMETER;
>
>   		if (device)
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G
  2019-04-10 16:40 ` [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G Patrick Wildt
@ 2019-04-11  8:03   ` Patrick DELAUNAY
  0 siblings, 0 replies; 9+ messages in thread
From: Patrick DELAUNAY @ 2019-04-11  8:03 UTC (permalink / raw)
  To: u-boot

Hi Patrick,

> From: Patrick Wildt <patrick@blueri.se>
> 
> On Wed, Apr 10, 2019 at 11:02:57AM +0200, Patrick Delaunay wrote:
> > Avoid ram_end = 0 on 32bit targets with ram_end at 4G.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> > example of issue in stm32mp1 board ev1:
> >   ram_start = c0000000
> >   size = 40000000
> >   ram_end = 100000000
> >   ram_end &= ~EFI_PAGE_MASK => result is 0
> >
> >  lib/efi_loader/efi_memory.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index 55622d2..81dc5fc 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -574,6 +574,10 @@ __weak void efi_add_known_memory(void)
> >
> >  		/* Remove partial pages */
> >  		ram_end &= ~EFI_PAGE_MASK;
> > +		/* Fix for 32bit targets with ram_top at 4G */
> > +		if (!ram_end)
> > +			ram_end = 0x100000000ULL;
> > +
> >  		ram_start = (ram_start + EFI_PAGE_MASK) &
> ~EFI_PAGE_MASK;
> >
> >  		if (ram_end <= ram_start) {
> > --
> > 2.7.4
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
> 
> Hi,
> 
> I had the exact same issue yesterday and posted a diff which I think is a better
> approach:

I tested your patch and yes that also correct my issue.

And I am agree that it is a better approach: So I will hack your patch and I abandon this patch (1/3) of this serie.

Regards

Patrick
 
> https://patchwork.ozlabs.org/patch/1082739/
> 
> Best regards,
> another Patrick

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH 2/3] efi: add protection for block_dev
  2019-04-10 17:42   ` Heinrich Schuchardt
@ 2019-04-11  9:30     ` Patrick DELAUNAY
  0 siblings, 0 replies; 9+ messages in thread
From: Patrick DELAUNAY @ 2019-04-11  9:30 UTC (permalink / raw)
  To: u-boot

Hi Heinrich

> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> On 4/10/19 11:02 AM, Patrick Delaunay wrote:
> > Check the value of block_dev before to use this pointer.
> >
> > This patch solves problem for the command "load" when ubifs is
> > previously mounted: in this case the function
> > blk_get_device_part_str("ubi 0") don't return error but return
> > block_dev = NULL and then data abort.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> >
> > To reproduce the issue, I have a boot script 'boot.scr.uimg'
> > with a load command executed during ubi boot:
> >
> > load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}
> >
> > I have a data abort for call stack:
> > - do_load_wrapper for "ubi 0"
> > -- efi_set_bootdev
> > --- efi_dp_from_name
> >
> > => desc = 0 and data abort for access to 'desc->*'
> 
> Thanks for reporting and analyzing the problem
> 
> Where exactly is the NULL dereference occurring?

I see the issue on v2018.11 and I adapt the patch for v2019.04 (as efi_dp_from_name as be created in v2019.01)

On v2018.11, the stack frame was

do_load_wrapper
=> efi_set_bootdev
==> efi_dp_from_name
===> efi_dp_from_part
====> dp_part_size

With result for blk_get_device_part_str
  dev="ubi"
  devnr="0:"
  path="/boot.scr.uimg"

desc =00000000 part=0

And the crash occurs in the function dp_part_size / called from efi_dp_from_part

In trace :
	pc : [<ffc91d28>]	   lr : [<ffc92383>]
	reloc pc : [<c0166d28>]	   lr : [<c0167383>]

with symbol 

	System.map:c0167358 T efi_dp_is_multi_instance
	System.map:c0167378 T efi_dp_from_part
	System.map:c01673a4 T efi_dp_part_node
	System.map:c01673c8 T efi_dp_from_file

	System.map:c0166d22 t dp_part_size
	System.map:c0166d50 t dp_part_node


> Igor reported a similar bug for a USB device in
> cmd: fs: fix data abort in load cmd
> https://lists.denx.de/pipermail/u-boot/2019-April/364484.htmll
> 
> >
> > I also proposed a protection for the same issue in ums command
> > http://patchwork.ozlabs.org/project/uboot/list/?series=68096
> >
> >
> >   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 53b40c8..fd57be8 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const
> char *devnr,
> >   	if (!is_net) {
> >   		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
> >   					       1);
> > -		if (part < 0)
> > +		if (part < 0 || !desc)
> 
> part = 0, desc = NULL occurs for UBI if the UBI file system is mounted.
> 
> Returning an error here means in the end that we will not be able to install run
> GRUB from the UBI device because we cannot describe the boot device.
> 
> I think that UBI volumes should be handled like any other block device.
> This will avoid having separate program paths for UBI and not UBI.
> 
> Heiko and Kyungmin could you, please, explain why UBI currently is not providing
> a struct blk_desc * block descriptor and how this can be fixed.
> 
> Best regards
> 
> Heinrich
> 
>  >   			return EFI_INVALID_PARAMETER;
> >
> >   		if (device)
> >

Best regards

Patrick

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH 2/3] efi: add protection for block_dev
  2019-04-10  9:02 ` [U-Boot] [PATCH 2/3] efi: add protection for block_dev Patrick Delaunay
  2019-04-10 17:42   ` Heinrich Schuchardt
@ 2019-04-11 17:26   ` Heinrich Schuchardt
  1 sibling, 0 replies; 9+ messages in thread
From: Heinrich Schuchardt @ 2019-04-11 17:26 UTC (permalink / raw)
  To: u-boot

On 4/10/19 11:02 AM, Patrick Delaunay wrote:
> Check the value of block_dev before to use this pointer.
>
> This patch solves problem for the command "load" when ubifs
> is previously mounted: in this case the function
> blk_get_device_part_str("ubi 0") don't return error but return
> block_dev = NULL and then data abort.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Added to efi-2019-07 branch.

> ---
>
> To reproduce the issue, I have a boot script 'boot.scr.uimg'
> with a load command executed during ubi boot:
>
> load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}
>
> I have a data abort for call stack:
> - do_load_wrapper for "ubi 0"
> -- efi_set_bootdev
> --- efi_dp_from_name
>
> => desc = 0 and data abort for access to 'desc->*'
>
> I also proposed a protection for the same issue in ums command
> http://patchwork.ozlabs.org/project/uboot/list/?series=68096
>
>
>   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 53b40c8..fd57be8 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>   	if (!is_net) {
>   		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
>   					       1);
> -		if (part < 0)
> +		if (part < 0 || !desc)
>   			return EFI_INVALID_PARAMETER;
>
>   		if (device)
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH 3/3] efi: update virtual address in efi_mem_carve_out
  2019-04-10  9:02 ` [U-Boot] [PATCH 3/3] efi: update virtual address in efi_mem_carve_out Patrick Delaunay
@ 2019-04-11 18:12   ` Heinrich Schuchardt
  0 siblings, 0 replies; 9+ messages in thread
From: Heinrich Schuchardt @ 2019-04-11 18:12 UTC (permalink / raw)
  To: u-boot

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@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);
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-04-11 18:12 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-10  9:02 [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G Patrick Delaunay
2019-04-10  9:02 ` [U-Boot] [PATCH 2/3] efi: add protection for block_dev Patrick Delaunay
2019-04-10 17:42   ` Heinrich Schuchardt
2019-04-11  9:30     ` Patrick DELAUNAY
2019-04-11 17:26   ` Heinrich Schuchardt
2019-04-10  9:02 ` [U-Boot] [PATCH 3/3] efi: update virtual address in efi_mem_carve_out Patrick Delaunay
2019-04-11 18:12   ` Heinrich Schuchardt
2019-04-10 16:40 ` [U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G Patrick Wildt
2019-04-11  8:03   ` Patrick DELAUNAY

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.