[PATCH 0/4] efi: Minor improvements for the EFI app

[PATCH 0/4] efi: Minor improvements for the EFI app

[Simon Glass]

This series collects a few fixes and improvements useful when booting
U-Boot as an EFI app.


Simon Glass (4):
  acpi: Use __packed with struct acpi_xsdt
  efi: Collect the ACPI tables in the app
  efi: Correct display of table GUIDs
  efi: Avoid using dm_scan_other()

 cmd/efi_common.c                |  4 +---
 configs/efi-x86_app64_defconfig |  1 +
 include/acpi/acpi_table.h       |  2 +-
 lib/efi/efi_app.c               | 29 +++++++++++++++++++++++------
 4 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.42.0.869.gea05f2083d-goog

[PATCH 1/4] acpi: Use __packed with struct acpi_xsdt

[Simon Glass]

Since struct acpi_table_header is not a multiple of 64 bits, use the
__packed option for struct acpi_xsdt

This ensures that the entry[] array starts on the correct boundary.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/acpi/acpi_table.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
index 1f85de091d39..59ab79ed17c2 100644
--- a/include/acpi/acpi_table.h
+++ b/include/acpi/acpi_table.h
@@ -80,7 +80,7 @@ struct acpi_rsdt {
 };
 
 /* XSDT (Extended System Description Table) */
-struct acpi_xsdt {
+struct __packed acpi_xsdt {
 	struct acpi_table_header header;
 	u64 entry[MAX_ACPI_TABLES];
 };
-- 
2.42.0.869.gea05f2083d-goog

[PATCH 2/4] efi: Collect the ACPI tables in the app

[Simon Glass]

Locate these so that they can be displayed using the 'acpi' command.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/efi/efi_app.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index 2209410f35b5..c5eb816655ea 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -12,18 +12,21 @@
 #include <cpu_func.h>
 #include <debug_uart.h>
 #include <dm.h>
+#include <efi.h>
+#include <efi_api.h>
 #include <errno.h>
 #include <init.h>
 #include <malloc.h>
+#include <sysreset.h>
+#include <uuid.h>
 #include <asm/global_data.h>
 #include <linux/err.h>
 #include <linux/types.h>
-#include <efi.h>
-#include <efi_api.h>
-#include <sysreset.h>
+#include <asm/global_data.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
 #include <dm/root.h>
+#include <mapmem.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -320,6 +323,19 @@ int dm_scan_other(bool pre_reloc_only)
 	return 0;
 }
 
+static void scan_tables(struct efi_system_table *sys_table)
+{
+	efi_guid_t acpi = EFI_ACPI_TABLE_GUID;
+	uint i;
+
+	for (i = 0; i < sys_table->nr_tables; i++) {
+		struct efi_configuration_table *tab = &sys_table->tables[i];
+
+		if (!memcmp(&tab->guid, &acpi, sizeof(efi_guid_t)))
+			gd_set_acpi_start(map_to_sysmem(tab->table));
+	}
+}
+
 /**
  * efi_main() - Start an EFI image
  *
@@ -354,6 +370,8 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 		return ret;
 	}
 
+	scan_tables(priv->sys_table);
+
 	/*
 	 * We could store the EFI memory map here, but it changes all the time,
 	 * so this is only useful for debugging.
-- 
2.42.0.869.gea05f2083d-goog

[PATCH 3/4] efi: Correct display of table GUIDs

[Simon Glass]

The printf() %pU option decodes GUIDs so it is not necessary to do this
first. Drop the incorrect code.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 cmd/efi_common.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/cmd/efi_common.c b/cmd/efi_common.c
index f4056096cd3f..1aa2351fcdfd 100644
--- a/cmd/efi_common.c
+++ b/cmd/efi_common.c
@@ -17,10 +17,8 @@ void efi_show_tables(struct efi_system_table *systab)
 
 	for (i = 0; i < systab->nr_tables; i++) {
 		struct efi_configuration_table *tab = &systab->tables[i];
-		char guid_str[37];
 
-		uuid_bin_to_str(tab->guid.b, guid_str, 1);
-		printf("%p  %pUl  %s\n", tab->table, guid_str,
+		printf("%p  %pUl  %s\n", tab->table, tab->guid.b,
 		       uuid_guid_get_str(tab->guid.b) ?: "(unknown)");
 	}
 }
-- 
2.42.0.869.gea05f2083d-goog

[PATCH 4/4] efi: Avoid using dm_scan_other()

[Simon Glass]

This function is defined by bootstd so using it precludes using that
feature. Use the board_early_init_r() feature instead.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 configs/efi-x86_app64_defconfig | 1 +
 lib/efi/efi_app.c               | 5 ++---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/configs/efi-x86_app64_defconfig b/configs/efi-x86_app64_defconfig
index d6b6c3d82995..e6a62b30dd09 100644
--- a/configs/efi-x86_app64_defconfig
+++ b/configs/efi-x86_app64_defconfig
@@ -17,6 +17,7 @@ CONFIG_USE_BOOTCOMMAND=y
 CONFIG_BOOTCOMMAND="ext2load scsi 0:3 01000000 /boot/vmlinuz; zboot 01000000"
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
 CONFIG_DISPLAY_BOARDINFO_LATE=y
+CONFIG_BOARD_EARLY_INIT_R=y
 CONFIG_HUSH_PARSER=y
 CONFIG_SYS_PBSIZE=532
 CONFIG_CMD_BOOTZ=y
diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index c5eb816655ea..1bced775a4d0 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -302,15 +302,14 @@ static int setup_block(void)
 }
 
 /**
- * dm_scan_other() - Scan for UEFI devices that should be available to U-Boot
+ * board_early_init_r() - Scan for UEFI devices that should be available
  *
  * This sets up block devices within U-Boot for those found in UEFI. With this,
  * U-Boot can access those devices
  *
- * @pre_reloc_only: true to only bind pre-relocation devices (ignored)
  * Returns: 0 on success, -ve on error
  */
-int dm_scan_other(bool pre_reloc_only)
+int board_early_init_r(void)
 {
 	if (gd->flags & GD_FLG_RELOC) {
 		int ret;
-- 
2.42.0.869.gea05f2083d-goog

Re: [PATCH 2/4] efi: Collect the ACPI tables in the app

[Heinrich Schuchardt]

On 11/12/23 16:58, Simon Glass wrote:
> Locate these so that they can be displayed using the 'acpi' command.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   lib/efi/efi_app.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> index 2209410f35b5..c5eb816655ea 100644
> --- a/lib/efi/efi_app.c
> +++ b/lib/efi/efi_app.c
> @@ -12,18 +12,21 @@
>   #include <cpu_func.h>
>   #include <debug_uart.h>
>   #include <dm.h>
> +#include <efi.h>
> +#include <efi_api.h>
>   #include <errno.h>
>   #include <init.h>
>   #include <malloc.h>
> +#include <sysreset.h>
> +#include <uuid.h>
>   #include <asm/global_data.h>
>   #include <linux/err.h>
>   #include <linux/types.h>
> -#include <efi.h>
> -#include <efi_api.h>
> -#include <sysreset.h>
> +#include <asm/global_data.h>
>   #include <dm/device-internal.h>
>   #include <dm/lists.h>
>   #include <dm/root.h>
> +#include <mapmem.h>
>
>   DECLARE_GLOBAL_DATA_PTR;
>
> @@ -320,6 +323,19 @@ int dm_scan_other(bool pre_reloc_only)
>   	return 0;
>   }
>
> +static void scan_tables(struct efi_system_table *sys_table)
> +{
> +	efi_guid_t acpi = EFI_ACPI_TABLE_GUID;
> +	uint i;
> +
> +	for (i = 0; i < sys_table->nr_tables; i++) {
> +		struct efi_configuration_table *tab = &sys_table->tables[i];
> +
> +		if (!memcmp(&tab->guid, &acpi, sizeof(efi_guid_t)))
> +			gd_set_acpi_start(map_to_sysmem(tab->table));
> +	}

Should we have a function efi_get_configuration_table(efi_guid_t *guid)
that we can use to retrieve different configuration tables?

This would allow us to read the device-tree on non-x86 tables.

We may at some point also be interested in reading the
EFI_RT_PROPERTIES_TABLE to find out which runtime services we are
allowed to call.

Or would you prefer to all of this into scan_tables()?

Best regards

Heinrich

> +}
> +
>   /**
>    * efi_main() - Start an EFI image
>    *
> @@ -354,6 +370,8 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>   		return ret;
>   	}
>
> +	scan_tables(priv->sys_table);
> +
>   	/*
>   	 * We could store the EFI memory map here, but it changes all the time,
>   	 * so this is only useful for debugging.

Re: [PATCH 1/4] acpi: Use __packed with struct acpi_xsdt

[Heinrich Schuchardt]

On 11/12/23 16:58, Simon Glass wrote:
> Since struct acpi_table_header is not a multiple of 64 bits, use the
> __packed option for struct acpi_xsdt
> 
> This ensures that the entry[] array starts on the correct boundary.
> 

Typically we keep the original signed-off-by when reposting patches.

Thanks for picking this change up.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

As in your review you suggested to add a unit test I have done so in my v2:

[PATCH v2 1/2] acpi: fix struct acpi_xsdt
https://patchwork.ozlabs.org/project/uboot/patch/20231112070316.17982-2-heinrich.schuchardt@canonical.com/

> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>   include/acpi/acpi_table.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
> index 1f85de091d39..59ab79ed17c2 100644
> --- a/include/acpi/acpi_table.h
> +++ b/include/acpi/acpi_table.h
> @@ -80,7 +80,7 @@ struct acpi_rsdt {
>   };
>   
>   /* XSDT (Extended System Description Table) */
> -struct acpi_xsdt {
> +struct __packed acpi_xsdt {
>   	struct acpi_table_header header;
>   	u64 entry[MAX_ACPI_TABLES];
>   };

Re: [PATCH 3/4] efi: Correct display of table GUIDs

[Heinrich Schuchardt]

On 11/12/23 16:58, Simon Glass wrote:
> The printf() %pU option decodes GUIDs so it is not necessary to do this
> first. Drop the incorrect code.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   cmd/efi_common.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/cmd/efi_common.c b/cmd/efi_common.c
> index f4056096cd3f..1aa2351fcdfd 100644
> --- a/cmd/efi_common.c
> +++ b/cmd/efi_common.c
> @@ -17,10 +17,8 @@ void efi_show_tables(struct efi_system_table *systab)
>
>   	for (i = 0; i < systab->nr_tables; i++) {
>   		struct efi_configuration_table *tab = &systab->tables[i];
> -		char guid_str[37];
>
> -		uuid_bin_to_str(tab->guid.b, guid_str, 1);
> -		printf("%p  %pUl  %s\n", tab->table, guid_str,
> +		printf("%p  %pUl  %s\n", tab->table, tab->guid.b,
>   		       uuid_guid_get_str(tab->guid.b) ?: "(unknown)");

Please, observe, that we have printf("%pUs", &guid) for printing the
text representation of a GUID. If the text representation is unknown, it
will print the numeric representation.

Best regards

Heinrich

>   	}
>   }

Re: [PATCH 4/4] efi: Avoid using dm_scan_other()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]

On Sun, Nov 12, 2023 at 08:58:05AM -0700, Simon Glass wrote:
> This function is defined by bootstd so using it precludes using that
> feature. Use the board_early_init_r() feature instead.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  configs/efi-x86_app64_defconfig | 1 +
>  lib/efi/efi_app.c               | 5 ++---
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/configs/efi-x86_app64_defconfig b/configs/efi-x86_app64_defconfig
> index d6b6c3d82995..e6a62b30dd09 100644
> --- a/configs/efi-x86_app64_defconfig
> +++ b/configs/efi-x86_app64_defconfig
> @@ -17,6 +17,7 @@ CONFIG_USE_BOOTCOMMAND=y
>  CONFIG_BOOTCOMMAND="ext2load scsi 0:3 01000000 /boot/vmlinuz; zboot 01000000"
>  CONFIG_SYS_CONSOLE_INFO_QUIET=y
>  CONFIG_DISPLAY_BOARDINFO_LATE=y
> +CONFIG_BOARD_EARLY_INIT_R=y
>  CONFIG_HUSH_PARSER=y
>  CONFIG_SYS_PBSIZE=532
>  CONFIG_CMD_BOOTZ=y
> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> index c5eb816655ea..1bced775a4d0 100644
> --- a/lib/efi/efi_app.c
> +++ b/lib/efi/efi_app.c
> @@ -302,15 +302,14 @@ static int setup_block(void)
>  }
>  
>  /**
> - * dm_scan_other() - Scan for UEFI devices that should be available to U-Boot
> + * board_early_init_r() - Scan for UEFI devices that should be available
>   *
>   * This sets up block devices within U-Boot for those found in UEFI. With this,
>   * U-Boot can access those devices
>   *
> - * @pre_reloc_only: true to only bind pre-relocation devices (ignored)
>   * Returns: 0 on success, -ve on error
>   */
> -int dm_scan_other(bool pre_reloc_only)
> +int board_early_init_r(void)
>  {
>  	if (gd->flags & GD_FLG_RELOC) {
>  		int ret;

Should this file really be board/efi/efi-x86_app/something.c ? We don't
define this function outside of the board directory today. Or should we
just move the function as the rest of the code is used in other EFI
applications we build (or no, the hello world one is too simple to need
this) ? If all of that sounds too wrong-direction, can we use event
lists or something?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 4/4] efi: Avoid using dm_scan_other()

[Heinrich Schuchardt]

Am 12. November 2023 19:03:57 MEZ schrieb Tom Rini <trini@konsulko.com>:
>On Sun, Nov 12, 2023 at 08:58:05AM -0700, Simon Glass wrote:
>> This function is defined by bootstd so using it precludes using that
>> feature. Use the board_early_init_r() feature instead.
>> 
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> 
>>  configs/efi-x86_app64_defconfig | 1 +
>>  lib/efi/efi_app.c               | 5 ++---
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/configs/efi-x86_app64_defconfig b/configs/efi-x86_app64_defconfig
>> index d6b6c3d82995..e6a62b30dd09 100644
>> --- a/configs/efi-x86_app64_defconfig
>> +++ b/configs/efi-x86_app64_defconfig
>> @@ -17,6 +17,7 @@ CONFIG_USE_BOOTCOMMAND=y
>>  CONFIG_BOOTCOMMAND="ext2load scsi 0:3 01000000 /boot/vmlinuz; zboot 01000000"
>>  CONFIG_SYS_CONSOLE_INFO_QUIET=y
>>  CONFIG_DISPLAY_BOARDINFO_LATE=y
>> +CONFIG_BOARD_EARLY_INIT_R=y
>>  CONFIG_HUSH_PARSER=y
>>  CONFIG_SYS_PBSIZE=532
>>  CONFIG_CMD_BOOTZ=y
>> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
>> index c5eb816655ea..1bced775a4d0 100644
>> --- a/lib/efi/efi_app.c
>> +++ b/lib/efi/efi_app.c
>> @@ -302,15 +302,14 @@ static int setup_block(void)
>>  }
>>  
>>  /**
>> - * dm_scan_other() - Scan for UEFI devices that should be available to U-Boot
>> + * board_early_init_r() - Scan for UEFI devices that should be available
>>   *
>>   * This sets up block devices within U-Boot for those found in UEFI. With this,
>>   * U-Boot can access those devices
>>   *
>> - * @pre_reloc_only: true to only bind pre-relocation devices (ignored)
>>   * Returns: 0 on success, -ve on error
>>   */
>> -int dm_scan_other(bool pre_reloc_only)
>> +int board_early_init_r(void)
>>  {
>>  	if (gd->flags & GD_FLG_RELOC) {
>>  		int ret;
>
>Should this file really be board/efi/efi-x86_app/something.c ? We don't

The EFI app should not be x86 specific.

Best regards

Heinrich


>define this function outside of the board directory today. Or should we
>just move the function as the rest of the code is used in other EFI
>applications we build (or no, the hello world one is too simple to need
>this) ? If all of that sounds too wrong-direction, can we use event
>lists or something?
>

Re: [PATCH 4/4] efi: Avoid using dm_scan_other()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]

On Sun, Nov 12, 2023 at 08:19:57PM +0100, Heinrich Schuchardt wrote:
> 
> 
> Am 12. November 2023 19:03:57 MEZ schrieb Tom Rini <trini@konsulko.com>:
> >On Sun, Nov 12, 2023 at 08:58:05AM -0700, Simon Glass wrote:
> >> This function is defined by bootstd so using it precludes using that
> >> feature. Use the board_early_init_r() feature instead.
> >> 
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >> 
> >>  configs/efi-x86_app64_defconfig | 1 +
> >>  lib/efi/efi_app.c               | 5 ++---
> >>  2 files changed, 3 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/configs/efi-x86_app64_defconfig b/configs/efi-x86_app64_defconfig
> >> index d6b6c3d82995..e6a62b30dd09 100644
> >> --- a/configs/efi-x86_app64_defconfig
> >> +++ b/configs/efi-x86_app64_defconfig
> >> @@ -17,6 +17,7 @@ CONFIG_USE_BOOTCOMMAND=y
> >>  CONFIG_BOOTCOMMAND="ext2load scsi 0:3 01000000 /boot/vmlinuz; zboot 01000000"
> >>  CONFIG_SYS_CONSOLE_INFO_QUIET=y
> >>  CONFIG_DISPLAY_BOARDINFO_LATE=y
> >> +CONFIG_BOARD_EARLY_INIT_R=y
> >>  CONFIG_HUSH_PARSER=y
> >>  CONFIG_SYS_PBSIZE=532
> >>  CONFIG_CMD_BOOTZ=y
> >> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> >> index c5eb816655ea..1bced775a4d0 100644
> >> --- a/lib/efi/efi_app.c
> >> +++ b/lib/efi/efi_app.c
> >> @@ -302,15 +302,14 @@ static int setup_block(void)
> >>  }
> >>  
> >>  /**
> >> - * dm_scan_other() - Scan for UEFI devices that should be available to U-Boot
> >> + * board_early_init_r() - Scan for UEFI devices that should be available
> >>   *
> >>   * This sets up block devices within U-Boot for those found in UEFI. With this,
> >>   * U-Boot can access those devices
> >>   *
> >> - * @pre_reloc_only: true to only bind pre-relocation devices (ignored)
> >>   * Returns: 0 on success, -ve on error
> >>   */
> >> -int dm_scan_other(bool pre_reloc_only)
> >> +int board_early_init_r(void)
> >>  {
> >>  	if (gd->flags & GD_FLG_RELOC) {
> >>  		int ret;
> >
> >Should this file really be board/efi/efi-x86_app/something.c ? We don't
> 
> The EFI app should not be x86 specific.

Yes, I suspect a follow-up would be to do whatever is needed to target a
given architecture with the U-Boot as EFI application board.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/4] efi: Collect the ACPI tables in the app

[Simon Glass]

Hi Heinrich,

On Sun, 12 Nov 2023 at 09:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/12/23 16:58, Simon Glass wrote:
> > Locate these so that they can be displayed using the 'acpi' command.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   lib/efi/efi_app.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> > index 2209410f35b5..c5eb816655ea 100644
> > --- a/lib/efi/efi_app.c
> > +++ b/lib/efi/efi_app.c
> > @@ -12,18 +12,21 @@
> >   #include <cpu_func.h>
> >   #include <debug_uart.h>
> >   #include <dm.h>
> > +#include <efi.h>
> > +#include <efi_api.h>
> >   #include <errno.h>
> >   #include <init.h>
> >   #include <malloc.h>
> > +#include <sysreset.h>
> > +#include <uuid.h>
> >   #include <asm/global_data.h>
> >   #include <linux/err.h>
> >   #include <linux/types.h>
> > -#include <efi.h>
> > -#include <efi_api.h>
> > -#include <sysreset.h>
> > +#include <asm/global_data.h>
> >   #include <dm/device-internal.h>
> >   #include <dm/lists.h>
> >   #include <dm/root.h>
> > +#include <mapmem.h>
> >
> >   DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -320,6 +323,19 @@ int dm_scan_other(bool pre_reloc_only)
> >       return 0;
> >   }
> >
> > +static void scan_tables(struct efi_system_table *sys_table)
> > +{
> > +     efi_guid_t acpi = EFI_ACPI_TABLE_GUID;
> > +     uint i;
> > +
> > +     for (i = 0; i < sys_table->nr_tables; i++) {
> > +             struct efi_configuration_table *tab = &sys_table->tables[i];
> > +
> > +             if (!memcmp(&tab->guid, &acpi, sizeof(efi_guid_t)))
> > +                     gd_set_acpi_start(map_to_sysmem(tab->table));
> > +     }
>
> Should we have a function efi_get_configuration_table(efi_guid_t *guid)
> that we can use to retrieve different configuration tables?
>
> This would allow us to read the device-tree on non-x86 tables.
>
> We may at some point also be interested in reading the
> EFI_RT_PROPERTIES_TABLE to find out which runtime services we are
> allowed to call.
>
> Or would you prefer to all of this into scan_tables()?

I'm not sure...but perhaps the best approach is to use this function
to find all the tables we are interested in?

BTW I started this series with a view to figuring out what is needed
to make the app boot Linux, etc. It is a bit strange since the boot
services need to be 'passed through' U-Boot to the underlying UEFI.

Regards,
Simon

Re: [PATCH 1/4] acpi: Use __packed with struct acpi_xsdt

[Simon Glass]

Hi Heinrich,

On Sun, 12 Nov 2023 at 09:46, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 11/12/23 16:58, Simon Glass wrote:
> > Since struct acpi_table_header is not a multiple of 64 bits, use the
> > __packed option for struct acpi_xsdt
> >
> > This ensures that the entry[] array starts on the correct boundary.
> >
>
> Typically we keep the original signed-off-by when reposting patches.
>
> Thanks for picking this change up.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>
> As in your review you suggested to add a unit test I have done so in my v2:
>
> [PATCH v2 1/2] acpi: fix struct acpi_xsdt
> https://patchwork.ozlabs.org/project/uboot/patch/20231112070316.17982-2-heinrich.schuchardt@canonical.com/

Oh yes, this is a duplicate patch to yours...I had it hanging around
for a while. So we can drop it.

>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   include/acpi/acpi_table.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
> > index 1f85de091d39..59ab79ed17c2 100644
> > --- a/include/acpi/acpi_table.h
> > +++ b/include/acpi/acpi_table.h
> > @@ -80,7 +80,7 @@ struct acpi_rsdt {
> >   };
> >
> >   /* XSDT (Extended System Description Table) */
> > -struct acpi_xsdt {
> > +struct __packed acpi_xsdt {
> >       struct acpi_table_header header;
> >       u64 entry[MAX_ACPI_TABLES];
> >   };
>

Regards,
Simon

Re: [PATCH 4/4] efi: Avoid using dm_scan_other()

[Simon Glass]

Hi Tom, Heinrich,

On Sun, 12 Nov 2023 at 12:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 12. November 2023 19:03:57 MEZ schrieb Tom Rini <trini@konsulko.com>:
> >On Sun, Nov 12, 2023 at 08:58:05AM -0700, Simon Glass wrote:
> >> This function is defined by bootstd so using it precludes using that
> >> feature. Use the board_early_init_r() feature instead.
> >>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>
> >>  configs/efi-x86_app64_defconfig | 1 +
> >>  lib/efi/efi_app.c               | 5 ++---
> >>  2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/configs/efi-x86_app64_defconfig b/configs/efi-x86_app64_defconfig
> >> index d6b6c3d82995..e6a62b30dd09 100644
> >> --- a/configs/efi-x86_app64_defconfig
> >> +++ b/configs/efi-x86_app64_defconfig
> >> @@ -17,6 +17,7 @@ CONFIG_USE_BOOTCOMMAND=y
> >>  CONFIG_BOOTCOMMAND="ext2load scsi 0:3 01000000 /boot/vmlinuz; zboot 01000000"
> >>  CONFIG_SYS_CONSOLE_INFO_QUIET=y
> >>  CONFIG_DISPLAY_BOARDINFO_LATE=y
> >> +CONFIG_BOARD_EARLY_INIT_R=y
> >>  CONFIG_HUSH_PARSER=y
> >>  CONFIG_SYS_PBSIZE=532
> >>  CONFIG_CMD_BOOTZ=y
> >> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> >> index c5eb816655ea..1bced775a4d0 100644
> >> --- a/lib/efi/efi_app.c
> >> +++ b/lib/efi/efi_app.c
> >> @@ -302,15 +302,14 @@ static int setup_block(void)
> >>  }
> >>
> >>  /**
> >> - * dm_scan_other() - Scan for UEFI devices that should be available to U-Boot
> >> + * board_early_init_r() - Scan for UEFI devices that should be available
> >>   *
> >>   * This sets up block devices within U-Boot for those found in UEFI. With this,
> >>   * U-Boot can access those devices
> >>   *
> >> - * @pre_reloc_only: true to only bind pre-relocation devices (ignored)
> >>   * Returns: 0 on success, -ve on error
> >>   */
> >> -int dm_scan_other(bool pre_reloc_only)
> >> +int board_early_init_r(void)
> >>  {
> >>      if (gd->flags & GD_FLG_RELOC) {
> >>              int ret;
> >
> >Should this file really be board/efi/efi-x86_app/something.c ? We don't

OK will fix.

>
> The EFI app should not be x86 specific.

Sure, but so far we don't have an ARM version.

>
> Best regards
>
> Heinrich
>
>
> >define this function outside of the board directory today. Or should we
> >just move the function as the rest of the code is used in other EFI
> >applications we build (or no, the hello world one is too simple to need
> >this) ? If all of that sounds too wrong-direction, can we use event
> >lists or something?
> >

Regards,
Simon

Re: [PATCH 3/4] efi: Correct display of table GUIDs

[Simon Glass]

Hi Heinrich,

On Sun, 12 Nov 2023 at 09:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/12/23 16:58, Simon Glass wrote:
> > The printf() %pU option decodes GUIDs so it is not necessary to do this
> > first. Drop the incorrect code.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   cmd/efi_common.c | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/cmd/efi_common.c b/cmd/efi_common.c
> > index f4056096cd3f..1aa2351fcdfd 100644
> > --- a/cmd/efi_common.c
> > +++ b/cmd/efi_common.c
> > @@ -17,10 +17,8 @@ void efi_show_tables(struct efi_system_table *systab)
> >
> >       for (i = 0; i < systab->nr_tables; i++) {
> >               struct efi_configuration_table *tab = &systab->tables[i];
> > -             char guid_str[37];
> >
> > -             uuid_bin_to_str(tab->guid.b, guid_str, 1);
> > -             printf("%p  %pUl  %s\n", tab->table, guid_str,
> > +             printf("%p  %pUl  %s\n", tab->table, tab->guid.b,
> >                      uuid_guid_get_str(tab->guid.b) ?: "(unknown)");
>
> Please, observe, that we have printf("%pUs", &guid) for printing the
> text representation of a GUID. If the text representation is unknown, it
> will print the numeric representation.

Yes, I understand but this is the output I want:

=> efi tables
bfbd7690  ee4e5898-3914-4259-9d6e-dc7bd79403cf  EFI_LZMA_COMPRESSED
bff27c40  05ad34ba-6f02-4214-952e-4da0398e2bb9  EFI_DXE_SERVICES
bfbd4010  7739f24c-93d7-11d4-9a3a-0090273fc14d  EFI_HOB_LIST
bff283c0  4c19049f-4137-4dd3-9c10-8b97a83ffdfa  EFI_MEMORY_TYPE
bff2892c  49152e77-1ada-4764-b7a2-7afefed95e8b  (unknown)
bfcb6210  060cc026-4c0d-4dda-8f41-595fef00a502  EFI_MEM_STATUS_CODE_REC
bfc80000  eb9d2d31-2d88-11d3-9a16-0090273fc14d  SMBIOS table
bfe67000  eb9d2d30-2d88-11d3-9a16-0090273fc14d  EFI_GUID_EFI_ACPI1
bfe67014  8868e871-e4f1-11d3-bc22-0080c73c8881  ACPI table
be9b9010  dcfa911d-26eb-469f-a220-38b7dc461220  (unknown)

(so I want to show the GUID in all cases, but also indicate which ones
are unknown)

Regards,
Simon