[U-Boot] 2018.09-rc1 breakage on orangepi pc2

[U-Boot] 2018.09-rc1 breakage on orangepi pc2

[Mark Kettenis]

I can no longer boot OpenBSD on the orangepi pc2.  The kernel faults
somewhere in the EFI detection code, almost certainly where it looks
through the configuration table entries looking for the ACPI table.
I've bisected this resulting in:


4182a129ef735bfd6c54788affe1b649ab85b851 is the first bad commit
commit 4182a129ef735bfd6c54788affe1b649ab85b851
Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
Date:   Thu Jun 28 12:45:32 2018 +0200

    efi_loader: allocate configuration table array
    
    The system table contains a link to the list of configurations tables.
    These include the device tree, SMBIOS table, and the ACPI table.
    
    This array is currently statically linked. With the patch it is allocated
    as EFI_RUNTIME_SERVICES_DATA. Due to the structure of the system table we
    cannot work with a linked list here.
    
    Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
    Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
    Signed-off-by: Alexander Graf <agraf@suse.de>

[U-Boot] 2018.09-rc1 breakage on orangepi pc2

[Mark Kettenis]

> Date: Thu, 2 Aug 2018 22:55:23 +0200 (CEST)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> 
> I can no longer boot OpenBSD on the orangepi pc2.  The kernel faults
> somewhere in the EFI detection code, almost certainly where it looks
> through the configuration table entries looking for the ACPI table.
> I've bisected this resulting in:
> 
> 
> 4182a129ef735bfd6c54788affe1b649ab85b851 is the first bad commit
> commit 4182a129ef735bfd6c54788affe1b649ab85b851
> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Date:   Thu Jun 28 12:45:32 2018 +0200
> 
>     efi_loader: allocate configuration table array
>     
>     The system table contains a link to the list of configurations tables.
>     These include the device tree, SMBIOS table, and the ACPI table.
>     
>     This array is currently statically linked. With the patch it is allocated
>     as EFI_RUNTIME_SERVICES_DATA. Due to the structure of the system table we
>     cannot work with a linked list here.
>     
>     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>     Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>     Signed-off-by: Alexander Graf <agraf@suse.de>

So after this commit, the ConfigurationTable field of the system table
is no longer translated from a physical address into a virtual address
when the SerVirtualAddressMap() runtime service gets called.

The way SetVirtualAddressMap() is implemented is actually quite nify.
It "relocates" the relevant bits by processing the ELF relocations for
everything that is marked as __efi_runtime_data.  This magically
modifies all the pointers in the system table that reference other
symbols, such as firmware_vendor, efi_runtime_services and
efi_conf_table.  But since the diff replaces the reference to
efi_conf_table with a dynamically updated pointer, there no longer is
a relocation for the tables member.

From the commit message it isn't abvious what is being fixed.  I think
the commit should be reverted.

[U-Boot] 2018.09-rc1 breakage on orangepi pc2

[Heinrich Schuchardt]

On 08/03/2018 12:07 AM, Mark Kettenis wrote:
>> Date: Thu, 2 Aug 2018 22:55:23 +0200 (CEST)
>> From: Mark Kettenis <mark.kettenis@xs4all.nl>
>>
>> I can no longer boot OpenBSD on the orangepi pc2.  The kernel faults
>> somewhere in the EFI detection code, almost certainly where it looks
>> through the configuration table entries looking for the ACPI table.
>> I've bisected this resulting in:
>>
>>
>> 4182a129ef735bfd6c54788affe1b649ab85b851 is the first bad commit
>> commit 4182a129ef735bfd6c54788affe1b649ab85b851
>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Date:   Thu Jun 28 12:45:32 2018 +0200
>>
>>     efi_loader: allocate configuration table array
>>     
>>     The system table contains a link to the list of configurations tables.
>>     These include the device tree, SMBIOS table, and the ACPI table.
>>     
>>     This array is currently statically linked. With the patch it is allocated
>>     as EFI_RUNTIME_SERVICES_DATA. Due to the structure of the system table we
>>     cannot work with a linked list here.
>>     
>>     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>     Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>>     Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> So after this commit, the ConfigurationTable field of the system table
> is no longer translated from a physical address into a virtual address
> when the SerVirtualAddressMap() runtime service gets called.
> 
> The way SetVirtualAddressMap() is implemented is actually quite nify.
> It "relocates" the relevant bits by processing the ELF relocations for
> everything that is marked as __efi_runtime_data.  This magically
> modifies all the pointers in the system table that reference other
> symbols, such as firmware_vendor, efi_runtime_services and
> efi_conf_table.  But since the diff replaces the reference to
> efi_conf_table with a dynamically updated pointer, there no longer is
> a relocation for the tables member.
> 
>>From the commit message it isn't abvious what is being fixed.  I think
> the commit should be reverted.
> 
Hello Mark,

thanks for pointing out the following requirement from the UEFI spec
that is not correctly implemented:

"Several fields of the EFI System Table must be converted from physical
pointers to virtual pointers using the ConvertPointer() service. These
fields include FirmwareVendor, RuntimeServices, and ConfigurationTable."

The EDK2 implementation is in
MdeModulePkg/Core/RuntimeDxe/Runtime.c:352:
RuntimeDriverConvertInternalPointer ((VOID **) &gST->ConfigurationTable);

I am currently testing a patch for this.

@Alex:

Relocation still has several todos:
- calculate crc32 after updating the system table
- provide ConvertPointer()
- do not assume U-Boot is the only runtime driver

Best regards

Heinrich

[U-Boot] 2018.09-rc1 breakage on orangepi pc2

[Mark Kettenis]

> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Date: Sat, 4 Aug 2018 14:21:17 +0200
> 
> On 08/03/2018 12:07 AM, Mark Kettenis wrote:
> >> Date: Thu, 2 Aug 2018 22:55:23 +0200 (CEST)
> >> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> >>
> >> I can no longer boot OpenBSD on the orangepi pc2.  The kernel faults
> >> somewhere in the EFI detection code, almost certainly where it looks
> >> through the configuration table entries looking for the ACPI table.
> >> I've bisected this resulting in:
> >>
> >>
> >> 4182a129ef735bfd6c54788affe1b649ab85b851 is the first bad commit
> >> commit 4182a129ef735bfd6c54788affe1b649ab85b851
> >> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> Date:   Thu Jun 28 12:45:32 2018 +0200
> >>
> >>     efi_loader: allocate configuration table array
> >>     
> >>     The system table contains a link to the list of configurations tables.
> >>     These include the device tree, SMBIOS table, and the ACPI table.
> >>     
> >>     This array is currently statically linked. With the patch it is allocated
> >>     as EFI_RUNTIME_SERVICES_DATA. Due to the structure of the system table we
> >>     cannot work with a linked list here.
> >>     
> >>     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>     Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> >>     Signed-off-by: Alexander Graf <agraf@suse.de>
> > 
> > So after this commit, the ConfigurationTable field of the system table
> > is no longer translated from a physical address into a virtual address
> > when the SerVirtualAddressMap() runtime service gets called.
> > 
> > The way SetVirtualAddressMap() is implemented is actually quite nify.
> > It "relocates" the relevant bits by processing the ELF relocations for
> > everything that is marked as __efi_runtime_data.  This magically
> > modifies all the pointers in the system table that reference other
> > symbols, such as firmware_vendor, efi_runtime_services and
> > efi_conf_table.  But since the diff replaces the reference to
> > efi_conf_table with a dynamically updated pointer, there no longer is
> > a relocation for the tables member.
> > 
> >>From the commit message it isn't abvious what is being fixed.  I think
> > the commit should be reverted.
> > 
> Hello Mark,
> 
> thanks for pointing out the following requirement from the UEFI spec
> that is not correctly implemented:
> 
> "Several fields of the EFI System Table must be converted from physical
> pointers to virtual pointers using the ConvertPointer() service. These
> fields include FirmwareVendor, RuntimeServices, and ConfigurationTable."

Sorry, but that functionality was implemented and it still works for
the FirmwareVendor and RuntimeServices fields.  Your commit introduced
a regression by breaking this functionality for the ConfigurationTable
field.

This regression should really be fixed before 2018.09 gets released.

> I am currently testing a patch for this.

Great.  Although I still don't understand what your commit is trying
to fix.

[U-Boot] [RFC 1/1] efi_loader: relocate pointer to tables

[Heinrich Schuchardt]

When applying a virtual memory map we have to update the pointer to the
list of configuration tables.

Fixes: 4182a129ef73 ("efi_loader: allocate configuration table array")
Reported-by: Mark Kettenis <mark.kettenis@xs4all.nl>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
Hello Mark,

could you, please, test if this solves the problem.

Best regards

Heinrich
---
 lib/efi_loader/efi_runtime.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 06958f23fa..45b7809dec 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -360,6 +360,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
 		efi_physical_addr_t map_start = map->physical_start;
 		efi_physical_addr_t map_len = map->num_pages << EFI_PAGE_SHIFT;
 		efi_physical_addr_t map_end = map_start + map_len;
+		u64 off = map->virtual_start - map_start;
 
 		/* Adjust all mmio pointers in this region */
 		list_for_each(lhandle, &efi_runtime_mmio) {
@@ -370,11 +371,17 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
 					   link);
 			if ((map_start <= lmmio->paddr) &&
 			    (map_end >= lmmio->paddr)) {
-				u64 off = map->virtual_start - map_start;
 				uintptr_t new_addr = lmmio->paddr + off;
 				*lmmio->ptr = (void *)new_addr;
 			}
 		}
+		if ((map_start <= (uintptr_t)systab.tables) &&
+		    (map_end >= (uintptr_t)systab.tables)) {
+			char *ptr = (char *)systab.tables;
+
+			ptr += off;
+			systab.tables = (struct efi_configuration_table *)ptr;
+		}
 	}
 
 	/* Move the actual runtime code over */
-- 
2.18.0

[U-Boot] [RFC 1/1] efi_loader: relocate pointer to tables

[Heinrich Schuchardt]

When applying a virtual memory map we have to update the pointer to the
list of configuration tables.

Fixes: 4182a129ef73 ("efi_loader: allocate configuration table array")
Reported-by: Mark Kettenis <mark.kettenis@xs4all.nl>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
Hello Mark,

could you, please, test if this solves the problem.

Best regards

Heinrich
---
 lib/efi_loader/efi_runtime.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 06958f23fa..45b7809dec 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -360,6 +360,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
 		efi_physical_addr_t map_start = map->physical_start;
 		efi_physical_addr_t map_len = map->num_pages << EFI_PAGE_SHIFT;
 		efi_physical_addr_t map_end = map_start + map_len;
+		u64 off = map->virtual_start - map_start;
 
 		/* Adjust all mmio pointers in this region */
 		list_for_each(lhandle, &efi_runtime_mmio) {
@@ -370,11 +371,17 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
 					   link);
 			if ((map_start <= lmmio->paddr) &&
 			    (map_end >= lmmio->paddr)) {
-				u64 off = map->virtual_start - map_start;
 				uintptr_t new_addr = lmmio->paddr + off;
 				*lmmio->ptr = (void *)new_addr;
 			}
 		}
+		if ((map_start <= (uintptr_t)systab.tables) &&
+		    (map_end >= (uintptr_t)systab.tables)) {
+			char *ptr = (char *)systab.tables;
+
+			ptr += off;
+			systab.tables = (struct efi_configuration_table *)ptr;
+		}
 	}
 
 	/* Move the actual runtime code over */
-- 
2.18.0

[U-Boot] [RFC 1/1] efi_loader: relocate pointer to tables

[Mark Kettenis]

> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Date: Sat,  4 Aug 2018 23:16:06 +0200
> 
> When applying a virtual memory map we have to update the pointer to the
> list of configuration tables.
> 
> Fixes: 4182a129ef73 ("efi_loader: allocate configuration table array")
> Reported-by: Mark Kettenis <mark.kettenis@xs4all.nl>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> Hello Mark,
> 
> could you, please, test if this solves the problem.

Works on my Orange Pi PC2.

So FWIW,

Tested-by: Mark Kettenis <kettenis@openbsd.org>

Thanks,

Mark

> Best regards
> 
> Heinrich
> ---
>  lib/efi_loader/efi_runtime.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 06958f23fa..45b7809dec 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -360,6 +360,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
>  		efi_physical_addr_t map_start = map->physical_start;
>  		efi_physical_addr_t map_len = map->num_pages << EFI_PAGE_SHIFT;
>  		efi_physical_addr_t map_end = map_start + map_len;
> +		u64 off = map->virtual_start - map_start;
>  
>  		/* Adjust all mmio pointers in this region */
>  		list_for_each(lhandle, &efi_runtime_mmio) {
> @@ -370,11 +371,17 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
>  					   link);
>  			if ((map_start <= lmmio->paddr) &&
>  			    (map_end >= lmmio->paddr)) {
> -				u64 off = map->virtual_start - map_start;
>  				uintptr_t new_addr = lmmio->paddr + off;
>  				*lmmio->ptr = (void *)new_addr;
>  			}
>  		}
> +		if ((map_start <= (uintptr_t)systab.tables) &&
> +		    (map_end >= (uintptr_t)systab.tables)) {
> +			char *ptr = (char *)systab.tables;
> +
> +			ptr += off;
> +			systab.tables = (struct efi_configuration_table *)ptr;
> +		}
>  	}
>  
>  	/* Move the actual runtime code over */
> -- 
> 2.18.0
> 
>