Re: [PATCH v5 00/12] smbios: Deal with tables beyond 4GB

[Heinrich Schuchardt]

Am 1. Januar 2024 01:37:39 MEZ schrieb Simon Glass :
>Hi Heinrich,
>
>On Sun, Dec 31, 2023 at 9:07 AM Heinrich Schuchardt  wrote:
>>
>> On 12/31/23 16:56, Heinrich Schuchardt wrote:
>> >
>> >
>> > Am 31. Dezember 2023 16:25:43 MEZ schrieb Simon Glass :
>> >> When the malloc() region extends beyond 4GB on ARM we may end up with
>> >> an SMBIOS table in that region.
>> >>
>> >> Add support for writing an SMBIOS3 table, which supports a 64-bit
>> >> address.
>> >>
>> >> Note that this problem does not happen on x86 since it requires the
>> >> tables to be placed just below 1MB in memory, unless
>> >> CONFIG_BLOBLIST_TABLES is enabled.
>> >
>> > The ACPI specification only requires that a low memory RSDP exists on 
>> > non-UEFI x86 systems.
>> >
>> > Nothing stops us from copying the RSDP only to low memory when not booting 
>> > via EFI. Maybe that way we could have more common code. But this is beyond 
>> > the scope of this series.
>> >
>>
>> Same is true for the SMBIOS anchor. You could have all tables in high
>> memory and only copy the anchor to low memory when booting non-UEFI x86.
>
>Yes, that's right...do you think it is worth mentioning that, though?

Reducing the x86 speific code is something we   should investigate in future. 
The current patch series looks good to me and hopefully will be merged soon.

Regards

Heinrich


>
>[..]
>
>Regards,
>Simon

Re: [PATCH v5 00/12] smbios: Deal with tables beyond 4GB

[Simon Glass]

Hi Heinrich,

On Sun, Dec 31, 2023 at 9:07 AM Heinrich Schuchardt  wrote:
>
> On 12/31/23 16:56, Heinrich Schuchardt wrote:
> >
> >
> > Am 31. Dezember 2023 16:25:43 MEZ schrieb Simon Glass :
> >> When the malloc() region extends beyond 4GB on ARM we may end up with
> >> an SMBIOS table in that region.
> >>
> >> Add support for writing an SMBIOS3 table, which supports a 64-bit
> >> address.
> >>
> >> Note that this problem does not happen on x86 since it requires the
> >> tables to be placed just below 1MB in memory, unless
> >> CONFIG_BLOBLIST_TABLES is enabled.
> >
> > The ACPI specification only requires that a low memory RSDP exists on 
> > non-UEFI x86 systems.
> >
> > Nothing stops us from copying the RSDP only to low memory when not booting 
> > via EFI. Maybe that way we could have more common code. But this is beyond 
> > the scope of this series.
> >
>
> Same is true for the SMBIOS anchor. You could have all tables in high
> memory and only copy the anchor to low memory when booting non-UEFI x86.

Yes, that's right...do you think it is worth mentioning that, though?

[..]

Regards,
Simon

Re: [PATCH v5 12/12] efi: Correct smbios-table installation

[Simon Glass]

Hi Heinrich,

On Sun, Dec 31, 2023 at 9:27 AM Heinrich Schuchardt  wrote:
>
> On 12/31/23 16:25, Simon Glass wrote:
> > At present this code allocates memory when writing the tables and
> > then unnecessarily adds another memory map when installing it.
> >
> > Adjust the code to allocate the tables using the normal U-Boot
> > mechanism. This avoids doing an EFI memory allocation early in
> > U-Boot, which may use memory that would be overwritten by a
> > 'load' command, for example.
> >
> > Signed-off-by: Simon Glass 
>
> In patch 11/12 you changed the address fields in ACPI tables from
> sandbox virtual addresses to pointers. Do you plan to do the same for
> SMBIOS?

I haven't looked at it, but could if it would help. Are you planning
to add an EFI test app for sandbox?

Regards,
Simon

Re: [PATCH v3 4/8] dts: Add alternative location for upstream DTB builds

[Mark Kettenis]

> Date: Sun, 31 Dec 2023 15:39:53 -0500
> From: Tom Rini 
> 
> On Sun, Dec 31, 2023 at 07:28:52AM -0700, Simon Glass wrote:
> > Hi Sumit,
> > 
> > On Fri, Dec 29, 2023 at 8:30 AM Sumit Garg  wrote:
> > >
> > > On Fri, 29 Dec 2023 at 01:18, Simon Glass  wrote:
> > > >
> > > > Hi Tom,
> > > >
> > > > On Thu, Dec 28, 2023 at 3:54 PM Tom Rini  wrote:
> > > > >
> > > > > On Thu, Dec 28, 2023 at 03:09:12PM +, Simon Glass wrote:
> > > > > > Hi Tom, Sumit,
> > > > > >
> > > > > > On Thu, Dec 28, 2023 at 2:03 PM Tom Rini  wrote:
> > > > > > >
> > > > > > > On Thu, Dec 28, 2023 at 01:37:26PM +, Simon Glass wrote:
> > > > > > > > Hi Sumit,
> > > > > > > >
> > > > > > > > On Thu, Dec 28, 2023 at 11:58 AM Sumit Garg 
> > > > > > > >  wrote:
> > > > > > > > >
> > > > > > > > > Allow platform owners to mirror devicetree files from 
> > > > > > > > > devitree-rebasing
> > > > > > > > > directory into dts/arch/$(ARCH) (special case for 
> > > > > > > > > dts/arch/arm64). Then
> > > > > > > > > build then along with any *-u-boot.dtsi file present in 
> > > > > > > > > arch/$(ARCH)/dts
> > > > > > > > > directory. Also add a new Makefile for arm64.
> > > > > > > > >
> > > > > > > > > This will help easy migration for platforms which currently 
> > > > > > > > > are compliant
> > > > > > > > > with upstream Linux kernel devicetree files.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sumit Garg 
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes in v3:
> > > > > > > > > --
> > > > > > > > > - Minor commit message update
> > > > > > > > >
> > > > > > > > > Changes in v2:
> > > > > > > > > --
> > > > > > > > > - s/DEVICE_TREE_LOC/dt_dir/ and s/U-boot/U-Boot/
> > > > > > > > >
> > > > > > > > >  dts/Kconfig | 11 +++
> > > > > > > > >  dts/Makefile| 17 ++---
> > > > > > > > >  dts/arch/arm64/Makefile | 14 ++
> > > > > > > > >  3 files changed, 39 insertions(+), 3 deletions(-)
> > > > > > > > >  create mode 100644 dts/arch/arm64/Makefile
> > > > > > > > >
> > > > > > > > > diff --git a/dts/Kconfig b/dts/Kconfig
> > > > > > > > > index 00c0aeff893..e58c1c6f2ab 100644
> > > > > > > > > --- a/dts/Kconfig
> > > > > > > > > +++ b/dts/Kconfig
> > > > > > > > > @@ -85,6 +85,17 @@ config OF_LIVE
> > > > > > > > >   enables a live tree which is available after 
> > > > > > > > > relocation,
> > > > > > > > >   and can be adjusted as needed.
> > > > > > > > >
> > > > > > > > > +config OF_UPSTREAM
> > > > > > > > > +   bool "Enable use of devicetree imported from Linux 
> > > > > > > > > kernel release"
> > > > > > > > > +   help
> > > > > > > > > + Traditionally, U-Boot platforms used to have their 
> > > > > > > > > custom devicetree
> > > > > > > > > + files or copy devicetree files from Linux kernel 
> > > > > > > > > which are hard to
> > > > > > > > > + maintain and can usually get out-of-sync from Linux 
> > > > > > > > > kernel. This
> > > > > > > > > + option enables platforms to migrate to 
> > > > > > > > > devicetree-rebasing repo where
> > > > > > > > > + a regular sync will be maintained every major Linux 
> > > > > > > > > kernel release
> > > > > > > > > + cycle. However, platforms can still have some 
> > > > > > > > > custom u-boot specific
> > > > > > > > > + bits maintained as part of *-u-boot.dtsi files.
> > > > > > > >
> > > > > > > > My only other suggestion here is to mention that this should be 
> > > > > > > > set in
> > > > > > > > Kconfig, for the SoC as a whole. So I believe that means that it
> > > > > > > > should be hidden, with no string for the 'bool':
> > > > > > > >
> > > > > > > >   bool  # Enable use of devicetree imported from Linux 
> > > > > > > > kernel release
> > > > > > >
> > > > > > > I think we can just keep prompting for it now, to make the 
> > > > > > > transition
> > > > > > > easier, before this option just goes away in time, hopefully.
> > > > > > >
> > > > > > > > Also, this doesn't seem to work for me. Before this series I 
> > > > > > > > get these
> > > > > > > > files when building firefly-rk3399:
> > > > > > > >
> > > > > > > > rk3399-eaidk-610.dtbrk3399-khadas-edge-v.dtb
> > > > > > > > rk3399-orangepi.dtbrk3399-rock-pi-4a.dtb
> > > > > > > > rk3399-evb.dtb  rk3399-leez-p710.dtb
> > > > > > > > rk3399-pinebook-pro.dtbrk3399-rock-pi-4c.dtb
> > > > > > > > rk3399-ficus.dtbrk3399-nanopc-t4.dtb
> > > > > > > > rk3399-pinephone-pro.dtb   rk3399-rockpro64.dtb
> > > > > > > > rk3399-firefly.dtb  rk3399-nanopi-m4-2gb.dtb
> > > > > > > > rk3399pro-rock-pi-n10.dtb  rk3399-roc-pc.dtb
> > > > > > > > rk3399-gru-bob.dtb  rk3399-nanopi-m4b.dtb
> > > > > > > > rk3399-puma-haikou.dtb rk3399-roc-pc-mezzanine.dtb
> > > > > > > > rk3399-gru-kevin.dtbrk3399-nanopi-m4.dtb
> > > > > > > > rk3399-rock-4c-plus.dtb
> > > > > 

[PATCH] usb: musb-new: sunxi: support usage with DM_USB_GADGET

[Aren Moynihan]

Add support for building the sunxi-musb driver with DM_USB_GADGET
including adding a separate IRQ handling function and registering the
driver with the musb system differently.

The implementation of usb_gadget_register_driver from
musb-new/musb_uboot.c only works when the gadget driver for the device
has already been probed and has called musb_register. On the pinephone
(using a allwinner a64 processor) this causes issues when trying to use
usb gadget mode (such as from the ums command) and CONFIG_USB_ETHER is
disabled.

The implementation of usb_gadget_register_driver provided when
DM_USB_GADGET is enabled will probe the necessary drivers when it's
called.

Without the patch, this is what the error condition looks like:
=> ums 0 mmc 1
UMS: LUN 0, dev mmc 1, hwpart 0, sector 0x0, count 0x3a3e000
Controller uninitialized
g_dnl_register: failed!, error: -6
g_dnl_register failed

based on:
commit 2e4865bc6486 ("musb-new: omap2430: fix compiling in DM_USB_GADGET 
config")

Signed-off-by: Aren Moynihan 
---
I don't fully understand what's going on here. Some gadget mode drivers,
such as usb ether (in usb_ether_init) will initialize the usb gadget
driver, but others (ums, fastboot) don't seem to do this, or don't do it
early enough, so they don't work.

 drivers/usb/musb-new/sunxi.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index 91f082fe05e..9b06f49ce47 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -429,6 +429,17 @@ static struct musb_hdrc_config musb_config_h3 = {
.ram_bits   = SUNXI_MUSB_RAM_BITS,
 };
 
+#ifdef CONFIG_DM_USB_GADGET
+int dm_usb_gadget_handle_interrupts(struct udevice *dev)
+{
+   int n = 0;
+   struct musb_host_data *host = dev_get_priv(dev);
+
+   host->host->isr(0, host->host);
+   return 0;
+}
+#endif
+
 static int musb_usb_probe(struct udevice *dev)
 {
struct sunxi_glue *glue = dev_get_priv(dev);
@@ -482,6 +493,15 @@ static int musb_usb_probe(struct udevice *dev)
ret = musb_lowlevel_init(host);
if (!ret)
printf("Allwinner mUSB OTG (Host)\n");
+#elifdef CONFIG_DM_USB_GADGET
+   pdata.mode = MUSB_PERIPHERAL;
+   host->host = musb_init_controller(&pdata, &glue->dev, base);
+   if (!host->host)
+   return -EIO;
+
+   ret = usb_add_gadget_udc(&glue->dev, &host->host->g);
+   if (!ret)
+   printf("Allwinner mUSB OTG (Peripheral)\n");
 #else
pdata.mode = MUSB_PERIPHERAL;
host->host = musb_register(&pdata, &glue->dev, base);
-- 
2.43.0

Re: [PATCH v3 4/8] dts: Add alternative location for upstream DTB builds

[Tom Rini]

On Sun, Dec 31, 2023 at 07:28:52AM -0700, Simon Glass wrote:
> Hi Sumit,
> 
> On Fri, Dec 29, 2023 at 8:30 AM Sumit Garg  wrote:
> >
> > On Fri, 29 Dec 2023 at 01:18, Simon Glass  wrote:
> > >
> > > Hi Tom,
> > >
> > > On Thu, Dec 28, 2023 at 3:54 PM Tom Rini  wrote:
> > > >
> > > > On Thu, Dec 28, 2023 at 03:09:12PM +, Simon Glass wrote:
> > > > > Hi Tom, Sumit,
> > > > >
> > > > > On Thu, Dec 28, 2023 at 2:03 PM Tom Rini  wrote:
> > > > > >
> > > > > > On Thu, Dec 28, 2023 at 01:37:26PM +, Simon Glass wrote:
> > > > > > > Hi Sumit,
> > > > > > >
> > > > > > > On Thu, Dec 28, 2023 at 11:58 AM Sumit Garg 
> > > > > > >  wrote:
> > > > > > > >
> > > > > > > > Allow platform owners to mirror devicetree files from 
> > > > > > > > devitree-rebasing
> > > > > > > > directory into dts/arch/$(ARCH) (special case for 
> > > > > > > > dts/arch/arm64). Then
> > > > > > > > build then along with any *-u-boot.dtsi file present in 
> > > > > > > > arch/$(ARCH)/dts
> > > > > > > > directory. Also add a new Makefile for arm64.
> > > > > > > >
> > > > > > > > This will help easy migration for platforms which currently are 
> > > > > > > > compliant
> > > > > > > > with upstream Linux kernel devicetree files.
> > > > > > > >
> > > > > > > > Signed-off-by: Sumit Garg 
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes in v3:
> > > > > > > > --
> > > > > > > > - Minor commit message update
> > > > > > > >
> > > > > > > > Changes in v2:
> > > > > > > > --
> > > > > > > > - s/DEVICE_TREE_LOC/dt_dir/ and s/U-boot/U-Boot/
> > > > > > > >
> > > > > > > >  dts/Kconfig | 11 +++
> > > > > > > >  dts/Makefile| 17 ++---
> > > > > > > >  dts/arch/arm64/Makefile | 14 ++
> > > > > > > >  3 files changed, 39 insertions(+), 3 deletions(-)
> > > > > > > >  create mode 100644 dts/arch/arm64/Makefile
> > > > > > > >
> > > > > > > > diff --git a/dts/Kconfig b/dts/Kconfig
> > > > > > > > index 00c0aeff893..e58c1c6f2ab 100644
> > > > > > > > --- a/dts/Kconfig
> > > > > > > > +++ b/dts/Kconfig
> > > > > > > > @@ -85,6 +85,17 @@ config OF_LIVE
> > > > > > > >   enables a live tree which is available after 
> > > > > > > > relocation,
> > > > > > > >   and can be adjusted as needed.
> > > > > > > >
> > > > > > > > +config OF_UPSTREAM
> > > > > > > > +   bool "Enable use of devicetree imported from Linux 
> > > > > > > > kernel release"
> > > > > > > > +   help
> > > > > > > > + Traditionally, U-Boot platforms used to have their 
> > > > > > > > custom devicetree
> > > > > > > > + files or copy devicetree files from Linux kernel 
> > > > > > > > which are hard to
> > > > > > > > + maintain and can usually get out-of-sync from Linux 
> > > > > > > > kernel. This
> > > > > > > > + option enables platforms to migrate to 
> > > > > > > > devicetree-rebasing repo where
> > > > > > > > + a regular sync will be maintained every major Linux 
> > > > > > > > kernel release
> > > > > > > > + cycle. However, platforms can still have some custom 
> > > > > > > > u-boot specific
> > > > > > > > + bits maintained as part of *-u-boot.dtsi files.
> > > > > > >
> > > > > > > My only other suggestion here is to mention that this should be 
> > > > > > > set in
> > > > > > > Kconfig, for the SoC as a whole. So I believe that means that it
> > > > > > > should be hidden, with no string for the 'bool':
> > > > > > >
> > > > > > >   bool  # Enable use of devicetree imported from Linux kernel 
> > > > > > > release
> > > > > >
> > > > > > I think we can just keep prompting for it now, to make the 
> > > > > > transition
> > > > > > easier, before this option just goes away in time, hopefully.
> > > > > >
> > > > > > > Also, this doesn't seem to work for me. Before this series I get 
> > > > > > > these
> > > > > > > files when building firefly-rk3399:
> > > > > > >
> > > > > > > rk3399-eaidk-610.dtbrk3399-khadas-edge-v.dtb
> > > > > > > rk3399-orangepi.dtbrk3399-rock-pi-4a.dtb
> > > > > > > rk3399-evb.dtb  rk3399-leez-p710.dtb
> > > > > > > rk3399-pinebook-pro.dtbrk3399-rock-pi-4c.dtb
> > > > > > > rk3399-ficus.dtbrk3399-nanopc-t4.dtb
> > > > > > > rk3399-pinephone-pro.dtb   rk3399-rockpro64.dtb
> > > > > > > rk3399-firefly.dtb  rk3399-nanopi-m4-2gb.dtb
> > > > > > > rk3399pro-rock-pi-n10.dtb  rk3399-roc-pc.dtb
> > > > > > > rk3399-gru-bob.dtb  rk3399-nanopi-m4b.dtb
> > > > > > > rk3399-puma-haikou.dtb rk3399-roc-pc-mezzanine.dtb
> > > > > > > rk3399-gru-kevin.dtbrk3399-nanopi-m4.dtb
> > > > > > > rk3399-rock-4c-plus.dtb
> > > > > > > rk3399-khadas-edge-captain.dtb  rk3399-nanopi-neo4.dtb
> > > > > > > rk3399-rock-4se.dtb
> > > > > > > rk3399-khadas-edge.dtb  rk3399-nanopi-r4s.dtb 
> > > > > > > rk3399-rock960.dtb
> > > > > > >
> > > > > > > Afterwards I get this:
> > > > > > >
> > >

RE: [PATCH v2 01/30] mtd: spi-nor: Add config to enable flash DTR

[Bhumkar, Tejas Arvind]

[AMD Official Use Only - General]

Hi Jagan,

> -Original Message-
> From: Jagan Teki 
> Sent: Wednesday, December 20, 2023 1:00 PM
> To: Bhumkar, Tejas Arvind 
> Cc: u-boot@lists.denx.de; joe.hershber...@ni.com; rfried@gmail.com;
> Simek, Michal ; vigne...@ti.com; g...@xilinx.com; T
> Karthik Reddy 
> Subject: Re: [PATCH v2 01/30] mtd: spi-nor: Add config to enable flash DTR
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, Dec 6, 2023 at 3:02 PM Tejas Bhumkar
>  wrote:
> >
> > From: T Karthik Reddy 
> >
> > The spi-nor framework will set up the flash parameters by reading the
> > flash id table flags, which include cmd opcodes, address width, dummy
> > bytes, and bus width. In case, flash supports octal DTR mode and the
> > controller does not support the DTR. There is no process to switch
> > back to SDR mode.
> > To avoid this issue, create a Kconfig option SPI_FLASH_DTR_ENABLE to
> > explicitly specify to enable/disable flash DTR support.
> > This config is disabled by default.
>
> We cannot control controller fixup in flash, DTR read based on the DTR flag I 
> don't
> think adding extra CONFIG to hack the controller with impact is.
[Tejas] : By default, this configuration is set to Disabled. It serves as a 
convenient option for operating the flash
between SDR and DDR without requiring any adjustments to the nor-id 
table flags.

Regards,
Tejas.
>
> Jagan,

Re: [PATCH 1/1] cmd: fdt: check fdt address

[Heinrich Schuchardt]

On 12/31/23 15:22, Simon Glass wrote:

Hi Heinrich,

On Wed, Dec 20, 2023 at 5:00 PM Heinrich Schuchardt
 wrote:


Trying to read a device-tree from an illegal address leads to a crash.

Check that the parameter passed to 'fdt addr' is within the RAM area and
non-zero.


What is the motivation for this patch? Is something crashing? We don't
do this with the 'md' command, for example.


QEMU with too little memory was crashing. But maybe we should write an 
explicit warning about too little memory instead.


Best regards

Heinrich



Regards,
Simon




Signed-off-by: Heinrich Schuchardt 
---
  cmd/fdt.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 331564c13b..dc954ea7d5 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -193,6 +193,11 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int 
argc, char *const argv[])
 }

 addr = hextoul(argv[0], NULL);
+   if (!addr || addr < gd->ram_base || addr >= gd->ram_top) {
+   printf("Invalid address\n");
+   return CMD_RET_FAILURE;
+   }
+
 blob = map_sysmem(addr, 0);
 if ((quiet && fdt_check_header(blob)) ||
 (!quiet && !fdt_valid(&blob)))
--
2.40.1

Re: [PATCH v5 12/12] efi: Correct smbios-table installation

[Heinrich Schuchardt]

On 12/31/23 16:25, Simon Glass wrote:

At present this code allocates memory when writing the tables and
then unnecessarily adds another memory map when installing it.

Adjust the code to allocate the tables using the normal U-Boot
mechanism. This avoids doing an EFI memory allocation early in
U-Boot, which may use memory that would be overwritten by a
'load' command, for example.

Signed-off-by: Simon Glass 


In patch 11/12 you changed the address fields in ACPI tables from
sandbox virtual addresses to pointers. Do you plan to do the same for
SMBIOS?

Best regards

Heinrich

Re: [PATCH v5 11/12] acpi: Write pointers to tables instead of addresses

[Heinrich Schuchardt]

On 12/31/23 16:25, Simon Glass wrote:

Sandbox uses an API to map between addresses and pointers. This allows
it to have (emulated) memory at zero and avoid arch-specific addressing
details. It also allows memory-mapped peripherals to work.

As an example, on many machines sandbox maps address 100 to pointer
value 1000.

However this is not correct for ACPI, if sandbox starts another program
(e.g EFI app) and passes it the tables. That app has no knowledge of
sandbox's address mapping. So to make this work we want to store
1000 as the value in the table.

Add two new 'nomap' functions which clearly make this exeption to how
sandbox works.

This should allow EFI apps to access ACPI tables with sandbox, e.g. for
testing purposes.

Signed-off-by: Simon Glass
Suggested-by: Heinrich Schuchardt


Thanks a lot for making this change.

Best regards

Heinrich

Re: [PATCH v5 09/12] efi: smbios: Drop support for SMBIOS2 tables

[Heinrich Schuchardt]

On 12/31/23 16:25, Simon Glass wrote:

Only the v3 table is supported now, so always use this when installing
the EFI table.

Signed-off-by: Simon Glass 


Reviewed-by: Heinrich Schuchardt 


---

Changes in v5:
- Add new patch to drop support for SMBIOS2 tables with EFI

  lib/efi_loader/efi_smbios.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 5cbce6dc4eb..5db342ee0d7 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -27,7 +27,6 @@ enum {
   */
  efi_status_t efi_smbios_register(void)
  {
-   const efi_guid_t *guid;
ulong addr;
efi_status_t ret;
void *buf;
@@ -47,8 +46,7 @@ efi_status_t efi_smbios_register(void)

/* Install SMBIOS information as configuration table */
buf = map_sysmem(addr, 0);
-   guid = !memcmp(buf, "_SM_", 4) ? &smbios_guid : &smbios3_guid;
-   ret = efi_install_configuration_table(guid, buf);
+   ret = efi_install_configuration_table(&smbios3_guid, buf);
unmap_sysmem(buf);

return ret;

Re: Proposal: U-Boot memory management

[Tom Rini]

On Sun, Dec 31, 2023 at 04:40:06PM +0100, Heinrich Schuchardt wrote:
> 
> 
> Am 31. Dezember 2023 16:11:44 MEZ schrieb Tom Rini :
> >On Sun, Dec 31, 2023 at 07:22:10AM -0700, Simon Glass wrote:
> >> Hi Tom,
> >> 
> >> On Sun, Dec 31, 2023 at 6:54 AM Tom Rini  wrote:
> >> >
> >> > On Sun, Dec 31, 2023 at 05:48:23AM -0700, Simon Glass wrote:
> >> > > Hi,
> >> > >
> >> > > On Fri, Dec 29, 2023 at 10:52 AM Tom Rini  wrote:
> >> > > >
> >> > > > On Fri, Dec 29, 2023 at 06:44:15PM +0100, Mark Kettenis wrote:
> >> > > > > > Date: Fri, 29 Dec 2023 11:17:44 -0500
> >> > > > > > From: Tom Rini 
> >> > > > > >
> >> > > > > > On Fri, Dec 29, 2023 at 05:05:17PM +0100, Heinrich Schuchardt 
> >> > > > > > wrote:
> >> > > > > > >
> >> > > > > > >
> >> > > > > > > Am 29. Dezember 2023 16:43:07 MEZ schrieb Tom Rini 
> >> > > > > > > :
> >> > > > > > > >On Fri, Dec 29, 2023 at 05:36:09AM +, Simon Glass wrote:
> >> > > > > > > >> Hi,
> >> > > > > > > >>
> >> > > > > > > >> On Sat, Dec 16, 2023 at 6:01 PM Simon Glass 
> >> > > > > > > >>  wrote:
> >> > > > > > > >> >
> >> > > > > > > >> > Hi,
> >> > > > > > > >> >
> >> > > > > > > >> > This records my thoughts after a discussion with Ilias & 
> >> > > > > > > >> > Heinrich re
> >> > > > > > > >> > memory allocation in U-Boot.
> >> > > > > > > >> >
> >> > > > > > > >> > 1. malloc()
> >> > > > > > > >> >
> >> > > > > > > >> > malloc() is used for programmatic memory allocation. It 
> >> > > > > > > >> > allows memory
> >> > > > > > > >> > to be freed. It is not designed for very large 
> >> > > > > > > >> > allocations (e.g. a
> >> > > > > > > >> > 10MB kernel or 100MB ramdisk).
> >> > > > > > > >> >
> >> > > > > > > >> > 2. lmb
> >> > > > > > > >> >
> >> > > > > > > >> > lmb is used for large blocks of memory, such as those 
> >> > > > > > > >> > needed for a
> >> > > > > > > >> > kernel or ramdisk. Allocation is only transitory, for the 
> >> > > > > > > >> > purposes of
> >> > > > > > > >> > loading some images and booting. If the boot fails, then 
> >> > > > > > > >> > all lmb
> >> > > > > > > >> > allocations go away.
> >> > > > > > > >> >
> >> > > > > > > >> > lmb is set up by getting all available memory and then 
> >> > > > > > > >> > removing what
> >> > > > > > > >> > is used by U-Boot (code, data, malloc() space, etc.)
> >> > > > > > > >> >
> >> > > > > > > >> > lmb reservations have a few flags so that areas of memory 
> >> > > > > > > >> > can be
> >> > > > > > > >> > provided with attributes
> >> > > > > > > >> >
> >> > > > > > > >> > There are some corner cases...e.g. loading a file does an 
> >> > > > > > > >> > lmb
> >> > > > > > > >> > allocation but only for the purpose of avoiding a file 
> >> > > > > > > >> > being loaded
> >> > > > > > > >> > over U-Boot code/data. The allocation is dropped 
> >> > > > > > > >> > immediately after the
> >> > > > > > > >> > file is loaded. Within the bootm command, or when using 
> >> > > > > > > >> > standard boot,
> >> > > > > > > >> > this would be fairly easy to solve.
> >> > > > > > > >> >
> >> > > > > > > >> > Linux has renamed lmb to memblock. We should consider 
> >> > > > > > > >> > doing the same.
> >> > > > > > > >> >
> >> > > > > > > >> > 3. EFI
> >> > > > > > > >> >
> >> > > > > > > >> > EFI has its own memory-allocation tables.
> >> > > > > > > >> >
> >> > > > > > > >> > Like lmb, EFI is able to deal with large allocations. But 
> >> > > > > > > >> > via a 'pool'
> >> > > > > > > >> > function it can also do smaller allocations similar to 
> >> > > > > > > >> > malloc(),
> >> > > > > > > >> > although each one uses at least 4KB at present.
> >> > > > > > > >> >
> >> > > > > > > >> > EFI allocations do not go away when a boot fails.
> >> > > > > > > >> >
> >> > > > > > > >> > With EFI it is possible to add allocations post facto, in 
> >> > > > > > > >> > which case
> >> > > > > > > >> > they are added to the allocation table just as if the 
> >> > > > > > > >> > memory was
> >> > > > > > > >> > allocated with EFI to begin with.
> >> > > > > > > >> >
> >> > > > > > > >> > The EFI allocations and the lmb allocations use the same 
> >> > > > > > > >> > memory, so in
> >> > > > > > > >> > principle could conflict.
> >> > > > > > > >> >
> >> > > > > > > >> > EFI allocations are sometimes used to allocate internal 
> >> > > > > > > >> > U-Boot data as
> >> > > > > > > >> > well, if needed by the EFI app. For example, while 
> >> > > > > > > >> > efi_image_parse()
> >> > > > > > > >> > uses malloc(), efi_var_mem.c uses EFI allocations since 
> >> > > > > > > >> > the code runs
> >> > > > > > > >> > in the app context and may need to access the memory 
> >> > > > > > > >> > after U-Boot has
> >> > > > > > > >> > exited. Also efi_smbios.c uses allocate_pages() and then 
> >> > > > > > > >> > adds a new
> >> > > > > > > >> > mapping as well.
> >> > > > > > > >> >
> >> > > > > > > >> > EFI memory has attributes, including what the memory is 
> >> > > > > > > >> > used for (to
> >> > > > > > > >> > some degree of granu

Re: [PATCH v5 08/12] smbios: Drop support for SMBIOS2 tables

[Heinrich Schuchardt]

On 12/31/23 16:25, Simon Glass wrote:

These tables are a pain since there is no way to handle memory above
4GB. Use SMBIOS3 always.

This should hopefully not create problems on x86 devices, since SMBIOS3
was released seven years ago (2015).

Signed-off-by: Simon Glass 


Reviewed-by: Heinrich Schuchardt 


---

Changes in v5:
- Add new patch to drop support for SMBIOS2 tables

  lib/smbios.c | 76 
  1 file changed, 17 insertions(+), 59 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c
index cfd451e4088..d9d52bd58d7 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -545,13 +545,12 @@ ulong write_smbios_table(ulong addr)
  {
ofnode parent_node = ofnode_null();
ulong table_addr, start_addr;
+   struct smbios3_entry *se;
struct smbios_ctx ctx;
ulong tables;
int len = 0;
int max_struct_size = 0;
int handle = 0;
-   char *istart;
-   int isize;
int i;

ctx.node = ofnode_null();
@@ -565,12 +564,8 @@ ulong write_smbios_table(ulong addr)

start_addr = addr;

-   /*
-* So far we don't know which struct will be used, but they both end
-* up using the same amount of 16-bit-aligned space
-*/
-   addr += max(sizeof(struct smbios_entry), sizeof(struct smbios3_entry));
-   addr = ALIGN(addr, 16);
+   /* move past the (so-far-unwritten) header to start writing structs */
+   addr = ALIGN(addr + sizeof(struct smbios3_entry), 16);
tables = addr;

/* populate minimum required tables */
@@ -592,59 +587,22 @@ ulong write_smbios_table(ulong addr)
 * We must use a pointer here so things work correctly on sandbox. The
 * user of this table is not aware of the mapping of addresses to
 * sandbox's DRAM buffer.
-*
-* Check the address of the end of the tables. If it is above 4GB then
-* it is sensible to use SMBIOS3 even if the start of the table is below
-* 4GB (this case is very unlikely to happen in practice)
 */
table_addr = (ulong)map_sysmem(tables, 0);
-   if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {
-   struct smbios3_entry *se;
-   /*
-* We need to put this >32-bit pointer into the table but the
-* field is only 32 bits wide.
-*/
-   log_debug("WARNING: Using SMBIOS3.0 due to table-address overflow 
%lx\n",
- table_addr);
-   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
-   memset(se, '\0', sizeof(struct smbios_entry));
-   memcpy(se->anchor, "_SM3_", 5);
-   se->length = sizeof(struct smbios3_entry);
-   se->major_ver = SMBIOS_MAJOR_VER;
-   se->minor_ver = SMBIOS_MINOR_VER;
-   se->doc_rev = 0;
-   se->entry_point_rev = 1;
-   se->max_struct_size = len;
-   se->struct_table_address = table_addr;
-   se->checksum = table_compute_checksum(se,
-   sizeof(struct smbios3_entry));
-   } else {
-   struct smbios_entry *se;
-
-   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
-   memset(se, '\0', sizeof(struct smbios_entry));
-   memcpy(se->anchor, "_SM_", 4);
-   se->length = sizeof(struct smbios_entry);
-   se->major_ver = SMBIOS_MAJOR_VER;
-   se->minor_ver = SMBIOS_MINOR_VER;
-   se->max_struct_size = max_struct_size;
-   memcpy(se->intermediate_anchor, "_DMI_", 5);
-   se->struct_table_length = len;
-
-   se->struct_table_address = table_addr;
-
-   se->struct_count = handle;
-
-   /* calculate checksums */
-   istart = (char *)se + SMBIOS_INTERMEDIATE_OFFSET;
-   isize = sizeof(struct smbios_entry) -
-   SMBIOS_INTERMEDIATE_OFFSET;
-   se->intermediate_checksum = table_compute_checksum(istart,
-  isize);
-   se->checksum = table_compute_checksum(se,
- sizeof(struct smbios_entry));
-   unmap_sysmem(se);
-   }
+
+   /* now go back and write the SMBIOS3 header */
+   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
+   memset(se, '\0', sizeof(struct smbios_entry));
+   memcpy(se->anchor, "_SM3_", 5);
+   se->length = sizeof(struct smbios3_entry);
+   se->major_ver = SMBIOS_MAJOR_VER;
+   se->minor_ver = SMBIOS_MINOR_VER;
+   se->doc_rev = 0;
+   se->entry_point_rev = 1;
+   se->max_struct_size = len;
+   se->struct_table_address = table_addr;
+   se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
+

Re: [PATCH v5 04/12] smbios: Use SMBIOS 3.0 to support an address above 4GB

[Heinrich Schuchardt]

On 12/31/23 17:11, Heinrich Schuchardt wrote:

On 12/31/23 16:25, Simon Glass wrote:

When the SMBIOS table is written to an address above 4GB a 32-bit table
address is not large enough.

Use an SMBIOS3 table in that case.

Note that we cannot use efi_allocate_pages() since this function has
nothing to do with EFI. There is no equivalent function to allocate
memory below 4GB in U-Boot. One solution would be to create a separate
malloc() pool, or just always put the malloc() pool below 4GB.

- Use log_debug() for warning
- Rebase on Heinrich's smbios.h patch
- Set the checksum for SMBIOS3

Signed-off-by: Simon Glass 
---

(no changes since v4)

Changes in v4:
- Check the start of the table rather than the end

Changes in v2:
- Check the end of the table rather than the start.

  include/smbios.h |  6 +-
  lib/smbios.c | 30 +-
  2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/include/smbios.h b/include/smbios.h
index e601283d293..77be58887a2 100644
--- a/include/smbios.h
+++ b/include/smbios.h
@@ -12,7 +12,7 @@

  /* SMBIOS spec version implemented */
  #define SMBIOS_MAJOR_VER    3
-#define SMBIOS_MINOR_VER    0
+#define SMBIOS_MINOR_VER    7

  enum {
  SMBIOS_STR_MAX    = 64,    /* Maximum length allowed for a
string */
@@ -80,6 +80,10 @@ struct __packed smbios3_entry {
  u64 struct_table_address;
  };

+/* These two structures should use the same amount of 16-byte-aligned
space */
+static_assert(ALIGN(16, sizeof(struct smbios_entry)) ==
+  ALIGN(16, sizeof(struct smbios3_entry)));
+
  /* BIOS characteristics */
  #define BIOS_CHARACTERISTICS_PCI_SUPPORTED    (1 << 7)
  #define BIOS_CHARACTERISTICS_UPGRADEABLE    (1 << 11)
diff --git a/lib/smbios.c b/lib/smbios.c
index eea72670bd9..7f79d969c92 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -567,7 +567,11 @@ ulong write_smbios_table(ulong addr)
  addr = ALIGN(addr, 16);
  start_addr = addr;

-    addr += sizeof(struct smbios_entry);
+    /*
+ * So far we don't know which struct will be used, but they both end
+ * up using the same amount of 16-bit-aligned space
+ */
+    addr += max(sizeof(struct smbios_entry), sizeof(struct
smbios3_entry));
  addr = ALIGN(addr, 16);
  tables = addr;

@@ -590,16 +594,32 @@ ulong write_smbios_table(ulong addr)
   * We must use a pointer here so things work correctly on
sandbox. The
   * user of this table is not aware of the mapping of addresses to
   * sandbox's DRAM buffer.
+ *
+ * Check the address of the end of the tables. If it is above 4GB
then
+ * it is sensible to use SMBIOS3 even if the start of the table
is below
+ * 4GB (this case is very unlikely to happen in practice)
   */
  table_addr = (ulong)map_sysmem(tables, 0);
-    if (sizeof(table_addr) > sizeof(u32) && table_addr >
(ulong)UINT_MAX) {
+    if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {



All SMBIOS specifications since version 3.0.0, published 2015, allow
using a SMBIOS3 anchor in all cases. SMBIOS_MAJOR_VER == 3.

Our target is to keep the code size small. Why do you increase the code
size here?


You drop SMBIOS2 in patch 8.

Reviewed-by: Heinrich Schuchardt 



Best regards

Heinrich


+    struct smbios3_entry *se;
  /*
   * We need to put this >32-bit pointer into the table but the
   * field is only 32 bits wide.
   */
-    printf("WARNING: SMBIOS table_address overflow %llx\n",
-   (unsigned long long)table_addr);
-    addr = 0;
+    log_debug("WARNING: Using SMBIOS3.0 due to table-address
overflow %lx\n",
+  table_addr);
+    se = map_sysmem(start_addr, sizeof(struct smbios_entry));
+    memset(se, '\0', sizeof(struct smbios_entry));
+    memcpy(se->anchor, "_SM3_", 5);
+    se->length = sizeof(struct smbios3_entry);
+    se->major_ver = SMBIOS_MAJOR_VER;
+    se->minor_ver = SMBIOS_MINOR_VER;
+    se->doc_rev = 0;
+    se->entry_point_rev = 1;
+    se->max_struct_size = len;
+    se->struct_table_address = table_addr;
+    se->checksum = table_compute_checksum(se,
+    sizeof(struct smbios3_entry));
  } else {
  struct smbios_entry *se;

Re: [PATCH v5 04/12] smbios: Use SMBIOS 3.0 to support an address above 4GB

[Heinrich Schuchardt]

On 12/31/23 16:25, Simon Glass wrote:

When the SMBIOS table is written to an address above 4GB a 32-bit table
address is not large enough.

Use an SMBIOS3 table in that case.

Note that we cannot use efi_allocate_pages() since this function has
nothing to do with EFI. There is no equivalent function to allocate
memory below 4GB in U-Boot. One solution would be to create a separate
malloc() pool, or just always put the malloc() pool below 4GB.

- Use log_debug() for warning
- Rebase on Heinrich's smbios.h patch
- Set the checksum for SMBIOS3

Signed-off-by: Simon Glass 
---

(no changes since v4)

Changes in v4:
- Check the start of the table rather than the end

Changes in v2:
- Check the end of the table rather than the start.

  include/smbios.h |  6 +-
  lib/smbios.c | 30 +-
  2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/include/smbios.h b/include/smbios.h
index e601283d293..77be58887a2 100644
--- a/include/smbios.h
+++ b/include/smbios.h
@@ -12,7 +12,7 @@

  /* SMBIOS spec version implemented */
  #define SMBIOS_MAJOR_VER  3
-#define SMBIOS_MINOR_VER   0
+#define SMBIOS_MINOR_VER   7

  enum {
SMBIOS_STR_MAX  = 64,   /* Maximum length allowed for a string */
@@ -80,6 +80,10 @@ struct __packed smbios3_entry {
u64 struct_table_address;
  };

+/* These two structures should use the same amount of 16-byte-aligned space */
+static_assert(ALIGN(16, sizeof(struct smbios_entry)) ==
+ ALIGN(16, sizeof(struct smbios3_entry)));
+
  /* BIOS characteristics */
  #define BIOS_CHARACTERISTICS_PCI_SUPPORTED(1 << 7)
  #define BIOS_CHARACTERISTICS_UPGRADEABLE  (1 << 11)
diff --git a/lib/smbios.c b/lib/smbios.c
index eea72670bd9..7f79d969c92 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -567,7 +567,11 @@ ulong write_smbios_table(ulong addr)
addr = ALIGN(addr, 16);
start_addr = addr;

-   addr += sizeof(struct smbios_entry);
+   /*
+* So far we don't know which struct will be used, but they both end
+* up using the same amount of 16-bit-aligned space
+*/
+   addr += max(sizeof(struct smbios_entry), sizeof(struct smbios3_entry));
addr = ALIGN(addr, 16);
tables = addr;

@@ -590,16 +594,32 @@ ulong write_smbios_table(ulong addr)
 * We must use a pointer here so things work correctly on sandbox. The
 * user of this table is not aware of the mapping of addresses to
 * sandbox's DRAM buffer.
+*
+* Check the address of the end of the tables. If it is above 4GB then
+* it is sensible to use SMBIOS3 even if the start of the table is below
+* 4GB (this case is very unlikely to happen in practice)
 */
table_addr = (ulong)map_sysmem(tables, 0);
-   if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
+   if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {



All SMBIOS specifications since version 3.0.0, published 2015, allow
using a SMBIOS3 anchor in all cases. SMBIOS_MAJOR_VER == 3.

Our target is to keep the code size small. Why do you increase the code
size here?

Best regards

Heinrich


+   struct smbios3_entry *se;
/*
 * We need to put this >32-bit pointer into the table but the
 * field is only 32 bits wide.
 */
-   printf("WARNING: SMBIOS table_address overflow %llx\n",
-  (unsigned long long)table_addr);
-   addr = 0;
+   log_debug("WARNING: Using SMBIOS3.0 due to table-address overflow 
%lx\n",
+ table_addr);
+   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
+   memset(se, '\0', sizeof(struct smbios_entry));
+   memcpy(se->anchor, "_SM3_", 5);
+   se->length = sizeof(struct smbios3_entry);
+   se->major_ver = SMBIOS_MAJOR_VER;
+   se->minor_ver = SMBIOS_MINOR_VER;
+   se->doc_rev = 0;
+   se->entry_point_rev = 1;
+   se->max_struct_size = len;
+   se->struct_table_address = table_addr;
+   se->checksum = table_compute_checksum(se,
+   sizeof(struct smbios3_entry));
} else {
struct smbios_entry *se;

Re: [PATCH v5 00/12] smbios: Deal with tables beyond 4GB

[Heinrich Schuchardt]

On 12/31/23 16:56, Heinrich Schuchardt wrote:



Am 31. Dezember 2023 16:25:43 MEZ schrieb Simon Glass :

When the malloc() region extends beyond 4GB on ARM we may end up with
an SMBIOS table in that region.

Add support for writing an SMBIOS3 table, which supports a 64-bit
address.

Note that this problem does not happen on x86 since it requires the
tables to be placed just below 1MB in memory, unless
CONFIG_BLOBLIST_TABLES is enabled.


The ACPI specification only requires that a low memory RSDP exists on non-UEFI 
x86 systems.

Nothing stops us from copying the RSDP only to low memory when not booting via 
EFI. Maybe that way we could have more common code. But this is beyond the 
scope of this series.



Same is true for the SMBIOS anchor. You could have all tables in high
memory and only copy the anchor to low memory when booting non-UEFI x86.

Best regards

Heinrich





v5 drops support for SMBIOS2 tables as well. It also adds some changes
to memory mapping which should allow an EFI app (launched from sandbox)
to read the ACPI tables correctly.

Changes in v5:
- Update comment to mention x86 alignment
- Add new patch to drop support for SMBIOS2 tables
- Add new patch to drop support for SMBIOS2 tables with EFI
- Add new patch to rename test dm_test_setup_ctx_and_base_tables()
- Add new patch to write pointers to tables instead of addresses
- Add new patch to correct smbios-table installation

Changes in v4:
- Bring in this patch from Heinrich's series
- Check the start of the table rather than the end

Changes in v2:
- Check the end of the table rather than the start.
- Add a new patch to correct gd_smbios_start()
- Add a note about why unmap_system() is called

Heinrich Schuchardt (1):
  smbios: SMBIOS 3.0 (64-bit) Entry Point structure

Simon Glass (11):
  smbios: Refactor 32-bit code into an else statement
  smbios: Move the rest of the SMBIOS2 code
  smbios: Use SMBIOS 3.0 to support an address above 4GB
  smbios: Correct gd_smbios_start()
  efi: Use the correct GUID for the SMBIOS table
  smbios: Require the caller to align the SMBIOS table
  smbios: Drop support for SMBIOS2 tables
  efi: smbios: Drop support for SMBIOS2 tables
  acpi: Rename test dm_test_setup_ctx_and_base_tables()
  acpi: Write pointers to tables instead of addresses
  efi: Correct smbios-table installation

arch/sandbox/include/asm/io.h | 16 +
arch/x86/lib/acpi_table.c |  6 ++--
cmd/acpi.c| 12 +++
include/asm-generic/global_data.h |  2 +-
include/efi_api.h |  4 +++
include/mapmem.h  | 18 ++
include/smbios.h  | 41 ---
lib/acpi/acpi.c   | 12 +++
lib/acpi/acpi_table.c |  4 +--
lib/acpi/base.c   |  4 +--
lib/efi_loader/efi_smbios.c   | 28 +---
lib/smbios.c  | 55 ++-
test/dm/acpi.c| 14 
13 files changed, 134 insertions(+), 82 deletions(-)

Re: [PATCH v5 00/12] smbios: Deal with tables beyond 4GB

[Heinrich Schuchardt]

Am 31. Dezember 2023 16:25:43 MEZ schrieb Simon Glass :
>When the malloc() region extends beyond 4GB on ARM we may end up with
>an SMBIOS table in that region.
>
>Add support for writing an SMBIOS3 table, which supports a 64-bit
>address.
>
>Note that this problem does not happen on x86 since it requires the
>tables to be placed just below 1MB in memory, unless
>CONFIG_BLOBLIST_TABLES is enabled.

The ACPI specification only requires that a low memory RSDP exists on non-UEFI 
x86 systems.

Nothing stops us from copying the RSDP only to low memory when not booting via 
EFI. Maybe that way we could have more common code. But this is beyond the 
scope of this series.

Best regards

Heinrich


>
>v5 drops support for SMBIOS2 tables as well. It also adds some changes
>to memory mapping which should allow an EFI app (launched from sandbox)
>to read the ACPI tables correctly.
>
>Changes in v5:
>- Update comment to mention x86 alignment
>- Add new patch to drop support for SMBIOS2 tables
>- Add new patch to drop support for SMBIOS2 tables with EFI
>- Add new patch to rename test dm_test_setup_ctx_and_base_tables()
>- Add new patch to write pointers to tables instead of addresses
>- Add new patch to correct smbios-table installation
>
>Changes in v4:
>- Bring in this patch from Heinrich's series
>- Check the start of the table rather than the end
>
>Changes in v2:
>- Check the end of the table rather than the start.
>- Add a new patch to correct gd_smbios_start()
>- Add a note about why unmap_system() is called
>
>Heinrich Schuchardt (1):
>  smbios: SMBIOS 3.0 (64-bit) Entry Point structure
>
>Simon Glass (11):
>  smbios: Refactor 32-bit code into an else statement
>  smbios: Move the rest of the SMBIOS2 code
>  smbios: Use SMBIOS 3.0 to support an address above 4GB
>  smbios: Correct gd_smbios_start()
>  efi: Use the correct GUID for the SMBIOS table
>  smbios: Require the caller to align the SMBIOS table
>  smbios: Drop support for SMBIOS2 tables
>  efi: smbios: Drop support for SMBIOS2 tables
>  acpi: Rename test dm_test_setup_ctx_and_base_tables()
>  acpi: Write pointers to tables instead of addresses
>  efi: Correct smbios-table installation
>
> arch/sandbox/include/asm/io.h | 16 +
> arch/x86/lib/acpi_table.c |  6 ++--
> cmd/acpi.c| 12 +++
> include/asm-generic/global_data.h |  2 +-
> include/efi_api.h |  4 +++
> include/mapmem.h  | 18 ++
> include/smbios.h  | 41 ---
> lib/acpi/acpi.c   | 12 +++
> lib/acpi/acpi_table.c |  4 +--
> lib/acpi/base.c   |  4 +--
> lib/efi_loader/efi_smbios.c   | 28 +---
> lib/smbios.c  | 55 ++-
> test/dm/acpi.c| 14 
> 13 files changed, 134 insertions(+), 82 deletions(-)
>

Re: [PATCH v5 10/12] acpi: Rename test dm_test_setup_ctx_and_base_tables()

[Heinrich Schuchardt]

Am 31. Dezember 2023 16:25:53 MEZ schrieb Simon Glass :
>Use the word 'acpi' in this test so that it runs along with all the
>other ACPI tests.
>
>Signed-off-by: Simon Glass 


Reviewed-by: Heinrich Schuchardt 

>---
>
>Changes in v5:
>- Add new patch to rename test dm_test_setup_ctx_and_base_tables()
>
> test/dm/acpi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/test/dm/acpi.c b/test/dm/acpi.c
>index fb071902b45..1211e2f0e7f 100644
>--- a/test/dm/acpi.c
>+++ b/test/dm/acpi.c
>@@ -330,7 +330,7 @@ static int dm_test_acpi_basic(struct unit_test_state *uts)
> DM_TEST(dm_test_acpi_basic, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> 
> /* Test setup_ctx_and_base_tables */
>-static int dm_test_setup_ctx_and_base_tables(struct unit_test_state *uts)
>+static int dm_test_acpi_ctx_and_base_tables(struct unit_test_state *uts)
> {
>   struct acpi_rsdp *rsdp;
>   struct acpi_rsdt *rsdt;
>@@ -376,7 +376,7 @@ static int dm_test_setup_ctx_and_base_tables(struct 
>unit_test_state *uts)
> 
>   return 0;
> }
>-DM_TEST(dm_test_setup_ctx_and_base_tables,
>+DM_TEST(dm_test_acpi_ctx_and_base_tables,
>   UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> 
> /* Test 'acpi list' command */

Re: Proposal: U-Boot memory management

[Heinrich Schuchardt]

Am 31. Dezember 2023 16:11:44 MEZ schrieb Tom Rini :
>On Sun, Dec 31, 2023 at 07:22:10AM -0700, Simon Glass wrote:
>> Hi Tom,
>> 
>> On Sun, Dec 31, 2023 at 6:54 AM Tom Rini  wrote:
>> >
>> > On Sun, Dec 31, 2023 at 05:48:23AM -0700, Simon Glass wrote:
>> > > Hi,
>> > >
>> > > On Fri, Dec 29, 2023 at 10:52 AM Tom Rini  wrote:
>> > > >
>> > > > On Fri, Dec 29, 2023 at 06:44:15PM +0100, Mark Kettenis wrote:
>> > > > > > Date: Fri, 29 Dec 2023 11:17:44 -0500
>> > > > > > From: Tom Rini 
>> > > > > >
>> > > > > > On Fri, Dec 29, 2023 at 05:05:17PM +0100, Heinrich Schuchardt 
>> > > > > > wrote:
>> > > > > > >
>> > > > > > >
>> > > > > > > Am 29. Dezember 2023 16:43:07 MEZ schrieb Tom Rini 
>> > > > > > > :
>> > > > > > > >On Fri, Dec 29, 2023 at 05:36:09AM +, Simon Glass wrote:
>> > > > > > > >> Hi,
>> > > > > > > >>
>> > > > > > > >> On Sat, Dec 16, 2023 at 6:01 PM Simon Glass 
>> > > > > > > >>  wrote:
>> > > > > > > >> >
>> > > > > > > >> > Hi,
>> > > > > > > >> >
>> > > > > > > >> > This records my thoughts after a discussion with Ilias & 
>> > > > > > > >> > Heinrich re
>> > > > > > > >> > memory allocation in U-Boot.
>> > > > > > > >> >
>> > > > > > > >> > 1. malloc()
>> > > > > > > >> >
>> > > > > > > >> > malloc() is used for programmatic memory allocation. It 
>> > > > > > > >> > allows memory
>> > > > > > > >> > to be freed. It is not designed for very large allocations 
>> > > > > > > >> > (e.g. a
>> > > > > > > >> > 10MB kernel or 100MB ramdisk).
>> > > > > > > >> >
>> > > > > > > >> > 2. lmb
>> > > > > > > >> >
>> > > > > > > >> > lmb is used for large blocks of memory, such as those 
>> > > > > > > >> > needed for a
>> > > > > > > >> > kernel or ramdisk. Allocation is only transitory, for the 
>> > > > > > > >> > purposes of
>> > > > > > > >> > loading some images and booting. If the boot fails, then 
>> > > > > > > >> > all lmb
>> > > > > > > >> > allocations go away.
>> > > > > > > >> >
>> > > > > > > >> > lmb is set up by getting all available memory and then 
>> > > > > > > >> > removing what
>> > > > > > > >> > is used by U-Boot (code, data, malloc() space, etc.)
>> > > > > > > >> >
>> > > > > > > >> > lmb reservations have a few flags so that areas of memory 
>> > > > > > > >> > can be
>> > > > > > > >> > provided with attributes
>> > > > > > > >> >
>> > > > > > > >> > There are some corner cases...e.g. loading a file does an 
>> > > > > > > >> > lmb
>> > > > > > > >> > allocation but only for the purpose of avoiding a file 
>> > > > > > > >> > being loaded
>> > > > > > > >> > over U-Boot code/data. The allocation is dropped 
>> > > > > > > >> > immediately after the
>> > > > > > > >> > file is loaded. Within the bootm command, or when using 
>> > > > > > > >> > standard boot,
>> > > > > > > >> > this would be fairly easy to solve.
>> > > > > > > >> >
>> > > > > > > >> > Linux has renamed lmb to memblock. We should consider doing 
>> > > > > > > >> > the same.
>> > > > > > > >> >
>> > > > > > > >> > 3. EFI
>> > > > > > > >> >
>> > > > > > > >> > EFI has its own memory-allocation tables.
>> > > > > > > >> >
>> > > > > > > >> > Like lmb, EFI is able to deal with large allocations. But 
>> > > > > > > >> > via a 'pool'
>> > > > > > > >> > function it can also do smaller allocations similar to 
>> > > > > > > >> > malloc(),
>> > > > > > > >> > although each one uses at least 4KB at present.
>> > > > > > > >> >
>> > > > > > > >> > EFI allocations do not go away when a boot fails.
>> > > > > > > >> >
>> > > > > > > >> > With EFI it is possible to add allocations post facto, in 
>> > > > > > > >> > which case
>> > > > > > > >> > they are added to the allocation table just as if the 
>> > > > > > > >> > memory was
>> > > > > > > >> > allocated with EFI to begin with.
>> > > > > > > >> >
>> > > > > > > >> > The EFI allocations and the lmb allocations use the same 
>> > > > > > > >> > memory, so in
>> > > > > > > >> > principle could conflict.
>> > > > > > > >> >
>> > > > > > > >> > EFI allocations are sometimes used to allocate internal 
>> > > > > > > >> > U-Boot data as
>> > > > > > > >> > well, if needed by the EFI app. For example, while 
>> > > > > > > >> > efi_image_parse()
>> > > > > > > >> > uses malloc(), efi_var_mem.c uses EFI allocations since the 
>> > > > > > > >> > code runs
>> > > > > > > >> > in the app context and may need to access the memory after 
>> > > > > > > >> > U-Boot has
>> > > > > > > >> > exited. Also efi_smbios.c uses allocate_pages() and then 
>> > > > > > > >> > adds a new
>> > > > > > > >> > mapping as well.
>> > > > > > > >> >
>> > > > > > > >> > EFI memory has attributes, including what the memory is 
>> > > > > > > >> > used for (to
>> > > > > > > >> > some degree of granularity). See enum efi_memory_type and 
>> > > > > > > >> > struct
>> > > > > > > >> > efi_mem_desc. In the latter there are also attribute flags 
>> > > > > > > >> > - whether
>> > > > > > > >> > memory is cacheable, etc.
>> > > > > > > >> >
>> > > > > > > >> > EFI also has the x86 idea of

[PATCH v5 08/12] smbios: Drop support for SMBIOS2 tables

[Simon Glass]

These tables are a pain since there is no way to handle memory above
4GB. Use SMBIOS3 always.

This should hopefully not create problems on x86 devices, since SMBIOS3
was released seven years ago (2015).

Signed-off-by: Simon Glass 
---

Changes in v5:
- Add new patch to drop support for SMBIOS2 tables

 lib/smbios.c | 76 
 1 file changed, 17 insertions(+), 59 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c
index cfd451e4088..d9d52bd58d7 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -545,13 +545,12 @@ ulong write_smbios_table(ulong addr)
 {
ofnode parent_node = ofnode_null();
ulong table_addr, start_addr;
+   struct smbios3_entry *se;
struct smbios_ctx ctx;
ulong tables;
int len = 0;
int max_struct_size = 0;
int handle = 0;
-   char *istart;
-   int isize;
int i;
 
ctx.node = ofnode_null();
@@ -565,12 +564,8 @@ ulong write_smbios_table(ulong addr)
 
start_addr = addr;
 
-   /*
-* So far we don't know which struct will be used, but they both end
-* up using the same amount of 16-bit-aligned space
-*/
-   addr += max(sizeof(struct smbios_entry), sizeof(struct smbios3_entry));
-   addr = ALIGN(addr, 16);
+   /* move past the (so-far-unwritten) header to start writing structs */
+   addr = ALIGN(addr + sizeof(struct smbios3_entry), 16);
tables = addr;
 
/* populate minimum required tables */
@@ -592,59 +587,22 @@ ulong write_smbios_table(ulong addr)
 * We must use a pointer here so things work correctly on sandbox. The
 * user of this table is not aware of the mapping of addresses to
 * sandbox's DRAM buffer.
-*
-* Check the address of the end of the tables. If it is above 4GB then
-* it is sensible to use SMBIOS3 even if the start of the table is below
-* 4GB (this case is very unlikely to happen in practice)
 */
table_addr = (ulong)map_sysmem(tables, 0);
-   if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {
-   struct smbios3_entry *se;
-   /*
-* We need to put this >32-bit pointer into the table but the
-* field is only 32 bits wide.
-*/
-   log_debug("WARNING: Using SMBIOS3.0 due to table-address 
overflow %lx\n",
- table_addr);
-   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
-   memset(se, '\0', sizeof(struct smbios_entry));
-   memcpy(se->anchor, "_SM3_", 5);
-   se->length = sizeof(struct smbios3_entry);
-   se->major_ver = SMBIOS_MAJOR_VER;
-   se->minor_ver = SMBIOS_MINOR_VER;
-   se->doc_rev = 0;
-   se->entry_point_rev = 1;
-   se->max_struct_size = len;
-   se->struct_table_address = table_addr;
-   se->checksum = table_compute_checksum(se,
-   sizeof(struct smbios3_entry));
-   } else {
-   struct smbios_entry *se;
-
-   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
-   memset(se, '\0', sizeof(struct smbios_entry));
-   memcpy(se->anchor, "_SM_", 4);
-   se->length = sizeof(struct smbios_entry);
-   se->major_ver = SMBIOS_MAJOR_VER;
-   se->minor_ver = SMBIOS_MINOR_VER;
-   se->max_struct_size = max_struct_size;
-   memcpy(se->intermediate_anchor, "_DMI_", 5);
-   se->struct_table_length = len;
-
-   se->struct_table_address = table_addr;
-
-   se->struct_count = handle;
-
-   /* calculate checksums */
-   istart = (char *)se + SMBIOS_INTERMEDIATE_OFFSET;
-   isize = sizeof(struct smbios_entry) -
-   SMBIOS_INTERMEDIATE_OFFSET;
-   se->intermediate_checksum = table_compute_checksum(istart,
-  isize);
-   se->checksum = table_compute_checksum(se,
- sizeof(struct smbios_entry));
-   unmap_sysmem(se);
-   }
+
+   /* now go back and write the SMBIOS3 header */
+   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
+   memset(se, '\0', sizeof(struct smbios_entry));
+   memcpy(se->anchor, "_SM3_", 5);
+   se->length = sizeof(struct smbios3_entry);
+   se->major_ver = SMBIOS_MAJOR_VER;
+   se->minor_ver = SMBIOS_MINOR_VER;
+   se->doc_rev = 0;
+   se->entry_point_rev = 1;
+   se->max_struct_size = len;
+   se->struct_table_address = table_addr;
+   se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
+   unmap_sysmem(se);
 
return addr;
 }
-- 
2.34.1

[PATCH v5 07/12] smbios: Require the caller to align the SMBIOS table

[Simon Glass]

All callers handle this alignment, so drop the unnecessary code. This
simplifies things a little.

Signed-off-by: Simon Glass 
Reviewed-by: Heinrich Schuchardt 
Reviewed-by: Ilias Apalodimas 
---

Changes in v5:
- Update comment to mention x86 alignment

 include/smbios.h | 9 +
 lib/smbios.c | 2 --
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/include/smbios.h b/include/smbios.h
index 77be58887a2..49de32fec2d 100644
--- a/include/smbios.h
+++ b/include/smbios.h
@@ -258,12 +258,13 @@ static inline void fill_smbios_header(void *table, int 
type,
  *
  * This writes SMBIOS table at a given address.
  *
- * @addr:  start address to write SMBIOS table. If this is not
- * 16-byte-aligned then it will be aligned before the table is
- * written.
+ * @addr:  start address to write SMBIOS table, 16-byte-alignment
+ * recommended. Note that while the SMBIOS tables themself have no alignment
+ * requirement, some systems may requires alignment. For example x86 systems
+ * which put tables at f require 16-byte alignment
+ *
  * Return: end address of SMBIOS table (and start address for next entry)
  * or NULL in case of an error
- *
  */
 ulong write_smbios_table(ulong addr);
 
diff --git a/lib/smbios.c b/lib/smbios.c
index 7f79d969c92..cfd451e4088 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -563,8 +563,6 @@ ulong write_smbios_table(ulong addr)
ctx.dev = NULL;
}
 
-   /* 16 byte align the table address */
-   addr = ALIGN(addr, 16);
start_addr = addr;
 
/*
-- 
2.34.1

[PATCH v5 12/12] efi: Correct smbios-table installation

[Simon Glass]

At present this code allocates memory when writing the tables and
then unnecessarily adds another memory map when installing it.

Adjust the code to allocate the tables using the normal U-Boot
mechanism. This avoids doing an EFI memory allocation early in
U-Boot, which may use memory that would be overwritten by a
'load' command, for example.

Signed-off-by: Simon Glass 
---
I believe this is the fix we agreed on for after the release, so I am
including it in this series.

Changes in v5:
- Add new patch to correct smbios-table installation

 lib/efi_loader/efi_smbios.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 5db342ee0d7..eb6d2ba43c9 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -54,27 +54,25 @@ efi_status_t efi_smbios_register(void)
 
 static int install_smbios_table(void)
 {
-   u64 addr;
-   efi_status_t ret;
+   ulong addr;
+   void *buf;
 
if (!IS_ENABLED(CONFIG_GENERATE_SMBIOS_TABLE) || IS_ENABLED(CONFIG_X86))
return 0;
 
-   addr = SZ_4G;
-   ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
-EFI_RUNTIME_SERVICES_DATA,
-efi_size_in_pages(TABLE_SIZE), &addr);
-   if (ret != EFI_SUCCESS)
+   /* Align the table to a 4KB boundary to keep EFI happy */
+   buf = memalign(SZ_4K, TABLE_SIZE);
+   if (!buf)
return log_msg_ret("mem", -ENOMEM);
 
-   addr = map_to_sysmem((void *)(uintptr_t)addr);
+   addr = map_to_sysmem(buf);
if (!write_smbios_table(addr)) {
log_err("Failed to write SMBIOS table\n");
return log_msg_ret("smbios", -EINVAL);
}
 
/* Make a note of where we put it */
-   log_debug("SMBIOS tables written to %llx\n", addr);
+   log_debug("SMBIOS tables written to %lx\n", addr);
gd->arch.smbios_start = addr;
 
return 0;
-- 
2.34.1

[PATCH v5 11/12] acpi: Write pointers to tables instead of addresses

[Simon Glass]

Sandbox uses an API to map between addresses and pointers. This allows
it to have (emulated) memory at zero and avoid arch-specific addressing
details. It also allows memory-mapped peripherals to work.

As an example, on many machines sandbox maps address 100 to pointer
value 1000.

However this is not correct for ACPI, if sandbox starts another program
(e.g EFI app) and passes it the tables. That app has no knowledge of
sandbox's address mapping. So to make this work we want to store
1000 as the value in the table.

Add two new 'nomap' functions which clearly make this exeption to how
sandbox works.

This should allow EFI apps to access ACPI tables with sandbox, e.g. for
testing purposes.

Signed-off-by: Simon Glass 
Suggested-by: Heinrich Schuchardt 
---

Changes in v5:
- Add new patch to write pointers to tables instead of addresses

 arch/sandbox/include/asm/io.h | 16 
 arch/x86/lib/acpi_table.c |  6 +++---
 cmd/acpi.c| 12 ++--
 include/mapmem.h  | 18 ++
 lib/acpi/acpi.c   | 12 ++--
 lib/acpi/acpi_table.c |  4 ++--
 lib/acpi/base.c   |  4 ++--
 test/dm/acpi.c| 10 +-
 8 files changed, 58 insertions(+), 24 deletions(-)

diff --git a/arch/sandbox/include/asm/io.h b/arch/sandbox/include/asm/io.h
index 3c0a102697e..a23bd64994a 100644
--- a/arch/sandbox/include/asm/io.h
+++ b/arch/sandbox/include/asm/io.h
@@ -231,5 +231,21 @@ static inline void unmap_sysmem(const void *vaddr)
unmap_physmem(vaddr, MAP_WRBACK);
 }
 
+/**
+ * nomap_sysmem() - pass through an address unchanged
+ *
+ * This is used to indicate an address which should NOT be mapped, e.g. in
+ * SMBIOS tables. Using this function instead of a case shows that the sandbox
+ * conversion has been done
+ */
+static inline void *nomap_sysmem(phys_addr_t paddr, unsigned long len)
+{
+   return (void *)(uintptr_t)paddr;
+}
+
+static inline phys_addr_t nomap_to_sysmem(const void *ptr)
+{
+   return (phys_addr_t)(uintptr_t)ptr;
+}
 
 #endif
diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index c5b33dc65de..b366e44e337 100644
--- a/arch/x86/lib/acpi_table.c
+++ b/arch/x86/lib/acpi_table.c
@@ -197,7 +197,7 @@ int acpi_write_tcpa(struct acpi_ctx *ctx, const struct 
acpi_writer *entry)
 
tcpa->platform_class = 0;
tcpa->laml = size;
-   tcpa->lasa = map_to_sysmem(log);
+   tcpa->lasa = nomap_to_sysmem(log);
 
/* (Re)calculate length and checksum */
current = (u32)tcpa + sizeof(struct acpi_tcpa);
@@ -268,7 +268,7 @@ static int acpi_write_tpm2(struct acpi_ctx *ctx,
 
/* Fill the log area size and start address fields. */
tpm2->laml = tpm2_log_len;
-   tpm2->lasa = map_to_sysmem(lasa);
+   tpm2->lasa = nomap_to_sysmem(lasa);
 
/* Calculate checksum. */
header->checksum = table_compute_checksum(tpm2, header->length);
@@ -430,7 +430,7 @@ int acpi_write_gnvs(struct acpi_ctx *ctx, const struct 
acpi_writer *entry)
u32 *gnvs = (u32 *)((u32)ctx->dsdt + i);
 
if (*gnvs == ACPI_GNVS_ADDR) {
-   *gnvs = map_to_sysmem(ctx->current);
+   *gnvs = nomap_to_sysmem(ctx->current);
log_debug("Fix up global NVS in DSDT to 
%#08x\n",
  *gnvs);
break;
diff --git a/cmd/acpi.c b/cmd/acpi.c
index 0c144092420..79e9335b5db 100644
--- a/cmd/acpi.c
+++ b/cmd/acpi.c
@@ -45,7 +45,7 @@ static int dump_table_name(const char *sig)
if (!hdr)
return -ENOENT;
printf("%.*s @ %16lx\n", ACPI_NAME_LEN, hdr->signature,
-  (ulong)map_to_sysmem(hdr));
+  (ulong)nomap_to_sysmem(hdr));
print_buffer(0, hdr, 1, hdr->length, 0);
 
return 0;
@@ -54,9 +54,9 @@ static int dump_table_name(const char *sig)
 static void list_fadt(struct acpi_fadt *fadt)
 {
if (fadt->dsdt)
-   dump_hdr(map_sysmem(fadt->dsdt, 0));
+   dump_hdr(nomap_sysmem(fadt->dsdt, 0));
if (fadt->firmware_ctrl)
-   dump_hdr(map_sysmem(fadt->firmware_ctrl, 0));
+   dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0));
 }
 
 static void list_rsdt(struct acpi_rsdp *rsdp)
@@ -66,11 +66,11 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
struct acpi_xsdt *xsdt;
 
if (rsdp->rsdt_address) {
-   rsdt = map_sysmem(rsdp->rsdt_address, 0);
+   rsdt = nomap_sysmem(rsdp->rsdt_address, 0);
dump_hdr(&rsdt->header);
}
if (rsdp->xsdt_address) {
-   xsdt = map_sysmem(rsdp->xsdt_address, 0);
+   xsdt = nomap_sysmem(rsdp->xsdt_address, 0);
dump_hdr(&xsdt->header);
len = xsdt->header.length - sizeof(xsdt->header);
count = len / sizeof(u64)

[PATCH v5 10/12] acpi: Rename test dm_test_setup_ctx_and_base_tables()

[Simon Glass]

Use the word 'acpi' in this test so that it runs along with all the
other ACPI tests.

Signed-off-by: Simon Glass 
---

Changes in v5:
- Add new patch to rename test dm_test_setup_ctx_and_base_tables()

 test/dm/acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/dm/acpi.c b/test/dm/acpi.c
index fb071902b45..1211e2f0e7f 100644
--- a/test/dm/acpi.c
+++ b/test/dm/acpi.c
@@ -330,7 +330,7 @@ static int dm_test_acpi_basic(struct unit_test_state *uts)
 DM_TEST(dm_test_acpi_basic, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
 
 /* Test setup_ctx_and_base_tables */
-static int dm_test_setup_ctx_and_base_tables(struct unit_test_state *uts)
+static int dm_test_acpi_ctx_and_base_tables(struct unit_test_state *uts)
 {
struct acpi_rsdp *rsdp;
struct acpi_rsdt *rsdt;
@@ -376,7 +376,7 @@ static int dm_test_setup_ctx_and_base_tables(struct 
unit_test_state *uts)
 
return 0;
 }
-DM_TEST(dm_test_setup_ctx_and_base_tables,
+DM_TEST(dm_test_acpi_ctx_and_base_tables,
UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
 
 /* Test 'acpi list' command */
-- 
2.34.1

[PATCH v5 09/12] efi: smbios: Drop support for SMBIOS2 tables

[Simon Glass]

Only the v3 table is supported now, so always use this when installing
the EFI table.

Signed-off-by: Simon Glass 
---

Changes in v5:
- Add new patch to drop support for SMBIOS2 tables with EFI

 lib/efi_loader/efi_smbios.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 5cbce6dc4eb..5db342ee0d7 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -27,7 +27,6 @@ enum {
  */
 efi_status_t efi_smbios_register(void)
 {
-   const efi_guid_t *guid;
ulong addr;
efi_status_t ret;
void *buf;
@@ -47,8 +46,7 @@ efi_status_t efi_smbios_register(void)
 
/* Install SMBIOS information as configuration table */
buf = map_sysmem(addr, 0);
-   guid = !memcmp(buf, "_SM_", 4) ? &smbios_guid : &smbios3_guid;
-   ret = efi_install_configuration_table(guid, buf);
+   ret = efi_install_configuration_table(&smbios3_guid, buf);
unmap_sysmem(buf);
 
return ret;
-- 
2.34.1

[PATCH v5 01/12] smbios: Refactor 32-bit code into an else statement

[Simon Glass]

In preparation for adding support for SMBIOS3 move this code into an
else statement. There is no functional change.

Signed-off-by: Simon Glass 
Reviewed-by: Heinrich Schuchardt 
Reviewed-by: Ilias Apalodimas 
---

(no changes since v1)

 lib/smbios.c | 38 +-
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c
index 45480b01af4..b417f8a9057 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -589,14 +589,6 @@ ulong write_smbios_table(ulong addr)
len += tmp;
}
 
-   memcpy(se->anchor, "_SM_", 4);
-   se->length = sizeof(struct smbios_entry);
-   se->major_ver = SMBIOS_MAJOR_VER;
-   se->minor_ver = SMBIOS_MINOR_VER;
-   se->max_struct_size = max_struct_size;
-   memcpy(se->intermediate_anchor, "_DMI_", 5);
-   se->struct_table_length = len;
-
/*
 * We must use a pointer here so things work correctly on sandbox. The
 * user of this table is not aware of the mapping of addresses to
@@ -612,16 +604,28 @@ ulong write_smbios_table(ulong addr)
   (unsigned long long)table_addr);
addr = 0;
goto out;
+   } else {
+   memcpy(se->anchor, "_SM_", 4);
+   se->length = sizeof(struct smbios_entry);
+   se->major_ver = SMBIOS_MAJOR_VER;
+   se->minor_ver = SMBIOS_MINOR_VER;
+   se->max_struct_size = max_struct_size;
+   memcpy(se->intermediate_anchor, "_DMI_", 5);
+   se->struct_table_length = len;
+
+   se->struct_table_address = table_addr;
+
+   se->struct_count = handle;
+
+   /* calculate checksums */
+   istart = (char *)se + SMBIOS_INTERMEDIATE_OFFSET;
+   isize = sizeof(struct smbios_entry) -
+   SMBIOS_INTERMEDIATE_OFFSET;
+   se->intermediate_checksum = table_compute_checksum(istart,
+  isize);
+   se->checksum = table_compute_checksum(se,
+ sizeof(struct smbios_entry));
}
-   se->struct_table_address = table_addr;
-
-   se->struct_count = handle;
-
-   /* calculate checksums */
-   istart = (char *)se + SMBIOS_INTERMEDIATE_OFFSET;
-   isize = sizeof(struct smbios_entry) - SMBIOS_INTERMEDIATE_OFFSET;
-   se->intermediate_checksum = table_compute_checksum(istart, isize);
-   se->checksum = table_compute_checksum(se, sizeof(struct smbios_entry));
 out:
unmap_sysmem(se);
 
-- 
2.34.1

[PATCH v5 06/12] efi: Use the correct GUID for the SMBIOS table

[Simon Glass]

EFI does not use the 'anchor string' to determine the SMBIOS table
version, instead preferring to have two separate GUIDs. Use the correct
one, depending on the table version.

Call unmap_system() to balance to the use of map_sysmem()

Signed-off-by: Simon Glass 
Reviewed-by: Heinrich Schuchardt 
Reviewed-by: Ilias Apalodimas 
---

(no changes since v2)

Changes in v2:
- Add a note about why unmap_system() is called

 include/efi_api.h   |  4 
 lib/efi_loader/efi_smbios.c | 12 ++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index 0e92cb8a7f6..ab40b1b5ddf 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -433,6 +433,10 @@ struct efi_runtime_services {
EFI_GUID(0xeb9d2d31, 0x2d88, 0x11d3,  \
 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
+#define SMBIOS3_TABLE_GUID \
+   EFI_GUID(0xf2fd1544, 0x9794, 0x4a2c, \
+0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94)
+
 #define EFI_LOAD_FILE_PROTOCOL_GUID \
EFI_GUID(0x56ec3091, 0x954c, 0x11d2, \
 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 49adc87e45a..5cbce6dc4eb 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -14,6 +14,8 @@
 #include 
 #include 
 
+const efi_guid_t smbios3_guid = SMBIOS3_TABLE_GUID;
+
 enum {
TABLE_SIZE  = SZ_4K,
 };
@@ -25,8 +27,10 @@ enum {
  */
 efi_status_t efi_smbios_register(void)
 {
+   const efi_guid_t *guid;
ulong addr;
efi_status_t ret;
+   void *buf;
 
addr = gd_smbios_start();
if (!addr) {
@@ -42,8 +46,12 @@ efi_status_t efi_smbios_register(void)
log_debug("EFI using SMBIOS tables at %lx\n", addr);
 
/* Install SMBIOS information as configuration table */
-   return efi_install_configuration_table(&smbios_guid,
-  map_sysmem(addr, 0));
+   buf = map_sysmem(addr, 0);
+   guid = !memcmp(buf, "_SM_", 4) ? &smbios_guid : &smbios3_guid;
+   ret = efi_install_configuration_table(guid, buf);
+   unmap_sysmem(buf);
+
+   return ret;
 }
 
 static int install_smbios_table(void)
-- 
2.34.1

[PATCH v5 05/12] smbios: Correct gd_smbios_start()

[Simon Glass]

This should access arch-specific properties. Fix it and update the
existing usage.

Signed-off-by: Simon Glass 
Reviewed-by: Heinrich Schuchardt 
Reviewed-by: Ilias Apalodimas 
---

(no changes since v2)

Changes in v2:
- Add a new patch to correct gd_smbios_start()

 include/asm-generic/global_data.h | 2 +-
 lib/efi_loader/efi_smbios.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/global_data.h 
b/include/asm-generic/global_data.h
index 99bde9ec7e4..fcc3c6e14ca 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -553,7 +553,7 @@ static_assert(sizeof(struct global_data) == GD_SIZE);
 #endif
 
 #ifdef CONFIG_SMBIOS
-#define gd_smbios_start()  gd->smbios_start
+#define gd_smbios_start()  gd->arch.smbios_start
 #define gd_set_smbios_start(addr)  gd->arch.smbios_start = addr
 #else
 #define gd_smbios_start()  0UL
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index bbb8421ce14..49adc87e45a 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -28,7 +28,7 @@ efi_status_t efi_smbios_register(void)
ulong addr;
efi_status_t ret;
 
-   addr = gd->arch.smbios_start;
+   addr = gd_smbios_start();
if (!addr) {
log_err("No SMBIOS tables to install\n");
return EFI_NOT_FOUND;
-- 
2.34.1

[PATCH v5 02/12] smbios: Move the rest of the SMBIOS2 code

[Simon Glass]

Move all of this logic into the else clause, since it will not be used
for SMBIOS3

Signed-off-by: Simon Glass 
Reviewed-by: Heinrich Schuchardt 
Reviewed-by: Ilias Apalodimas 
---

(no changes since v1)

 lib/smbios.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c
index b417f8a9057..eea72670bd9 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -544,9 +544,8 @@ static struct smbios_write_method smbios_write_funcs[] = {
 ulong write_smbios_table(ulong addr)
 {
ofnode parent_node = ofnode_null();
-   struct smbios_entry *se;
+   ulong table_addr, start_addr;
struct smbios_ctx ctx;
-   ulong table_addr;
ulong tables;
int len = 0;
int max_struct_size = 0;
@@ -566,9 +565,7 @@ ulong write_smbios_table(ulong addr)
 
/* 16 byte align the table address */
addr = ALIGN(addr, 16);
-
-   se = map_sysmem(addr, sizeof(struct smbios_entry));
-   memset(se, 0, sizeof(struct smbios_entry));
+   start_addr = addr;
 
addr += sizeof(struct smbios_entry);
addr = ALIGN(addr, 16);
@@ -603,8 +600,11 @@ ulong write_smbios_table(ulong addr)
printf("WARNING: SMBIOS table_address overflow %llx\n",
   (unsigned long long)table_addr);
addr = 0;
-   goto out;
} else {
+   struct smbios_entry *se;
+
+   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
+   memset(se, '\0', sizeof(struct smbios_entry));
memcpy(se->anchor, "_SM_", 4);
se->length = sizeof(struct smbios_entry);
se->major_ver = SMBIOS_MAJOR_VER;
@@ -625,9 +625,8 @@ ulong write_smbios_table(ulong addr)
   isize);
se->checksum = table_compute_checksum(se,
  sizeof(struct smbios_entry));
+   unmap_sysmem(se);
}
-out:
-   unmap_sysmem(se);
 
return addr;
 }
-- 
2.34.1

[PATCH v5 04/12] smbios: Use SMBIOS 3.0 to support an address above 4GB

[Simon Glass]

When the SMBIOS table is written to an address above 4GB a 32-bit table
address is not large enough.

Use an SMBIOS3 table in that case.

Note that we cannot use efi_allocate_pages() since this function has
nothing to do with EFI. There is no equivalent function to allocate
memory below 4GB in U-Boot. One solution would be to create a separate
malloc() pool, or just always put the malloc() pool below 4GB.

- Use log_debug() for warning
- Rebase on Heinrich's smbios.h patch
- Set the checksum for SMBIOS3

Signed-off-by: Simon Glass 
---

(no changes since v4)

Changes in v4:
- Check the start of the table rather than the end

Changes in v2:
- Check the end of the table rather than the start.

 include/smbios.h |  6 +-
 lib/smbios.c | 30 +-
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/include/smbios.h b/include/smbios.h
index e601283d293..77be58887a2 100644
--- a/include/smbios.h
+++ b/include/smbios.h
@@ -12,7 +12,7 @@
 
 /* SMBIOS spec version implemented */
 #define SMBIOS_MAJOR_VER   3
-#define SMBIOS_MINOR_VER   0
+#define SMBIOS_MINOR_VER   7
 
 enum {
SMBIOS_STR_MAX  = 64,   /* Maximum length allowed for a string */
@@ -80,6 +80,10 @@ struct __packed smbios3_entry {
u64 struct_table_address;
 };
 
+/* These two structures should use the same amount of 16-byte-aligned space */
+static_assert(ALIGN(16, sizeof(struct smbios_entry)) ==
+ ALIGN(16, sizeof(struct smbios3_entry)));
+
 /* BIOS characteristics */
 #define BIOS_CHARACTERISTICS_PCI_SUPPORTED (1 << 7)
 #define BIOS_CHARACTERISTICS_UPGRADEABLE   (1 << 11)
diff --git a/lib/smbios.c b/lib/smbios.c
index eea72670bd9..7f79d969c92 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -567,7 +567,11 @@ ulong write_smbios_table(ulong addr)
addr = ALIGN(addr, 16);
start_addr = addr;
 
-   addr += sizeof(struct smbios_entry);
+   /*
+* So far we don't know which struct will be used, but they both end
+* up using the same amount of 16-bit-aligned space
+*/
+   addr += max(sizeof(struct smbios_entry), sizeof(struct smbios3_entry));
addr = ALIGN(addr, 16);
tables = addr;
 
@@ -590,16 +594,32 @@ ulong write_smbios_table(ulong addr)
 * We must use a pointer here so things work correctly on sandbox. The
 * user of this table is not aware of the mapping of addresses to
 * sandbox's DRAM buffer.
+*
+* Check the address of the end of the tables. If it is above 4GB then
+* it is sensible to use SMBIOS3 even if the start of the table is below
+* 4GB (this case is very unlikely to happen in practice)
 */
table_addr = (ulong)map_sysmem(tables, 0);
-   if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
+   if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {
+   struct smbios3_entry *se;
/*
 * We need to put this >32-bit pointer into the table but the
 * field is only 32 bits wide.
 */
-   printf("WARNING: SMBIOS table_address overflow %llx\n",
-  (unsigned long long)table_addr);
-   addr = 0;
+   log_debug("WARNING: Using SMBIOS3.0 due to table-address 
overflow %lx\n",
+ table_addr);
+   se = map_sysmem(start_addr, sizeof(struct smbios_entry));
+   memset(se, '\0', sizeof(struct smbios_entry));
+   memcpy(se->anchor, "_SM3_", 5);
+   se->length = sizeof(struct smbios3_entry);
+   se->major_ver = SMBIOS_MAJOR_VER;
+   se->minor_ver = SMBIOS_MINOR_VER;
+   se->doc_rev = 0;
+   se->entry_point_rev = 1;
+   se->max_struct_size = len;
+   se->struct_table_address = table_addr;
+   se->checksum = table_compute_checksum(se,
+   sizeof(struct smbios3_entry));
} else {
struct smbios_entry *se;
 
-- 
2.34.1

[PATCH v5 03/12] smbios: SMBIOS 3.0 (64-bit) Entry Point structure

[Simon Glass]

From: Heinrich Schuchardt 

Add definition of the SMBIOS 3.0 (64-bit) Entry Point structure.

Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Simon Glass 
Reviewed-by: Ilias Apalodimas 

Signed-off-by: Simon Glass 
---

(no changes since v4)

Changes in v4:
- Bring in this patch from Heinrich's series

 include/smbios.h | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/include/smbios.h b/include/smbios.h
index c9df2706f5a..e601283d293 100644
--- a/include/smbios.h
+++ b/include/smbios.h
@@ -54,6 +54,32 @@ struct __packed smbios_entry {
u8 bcd_rev;
 };
 
+/**
+ * struct smbios3_entry - SMBIOS 3.0 (64-bit) Entry Point structure
+ */
+struct __packed smbios3_entry {
+   /** @anchor: anchor string */
+   u8 anchor[5];
+   /** @checksum: checksum of the entry point structure */
+   u8 checksum;
+   /** @length: length of the entry point structure */
+   u8 length;
+   /** @major_ver: major version of the SMBIOS specification */
+   u8 major_ver;
+   /** @minor_ver: minor version of the SMBIOS specification */
+   u8 minor_ver;
+   /** @docrev: revision of the SMBIOS specification */
+   u8 doc_rev;
+   /** @entry_point_rev: revision of the entry point structure */
+   u8 entry_point_rev;
+   /** @reserved: reserved */
+   u8 reserved;
+   /** maximum size of SMBIOS table */
+   u32 max_struct_size;
+   /** @struct_table_address: 64-bit physical starting address */
+   u64 struct_table_address;
+};
+
 /* BIOS characteristics */
 #define BIOS_CHARACTERISTICS_PCI_SUPPORTED (1 << 7)
 #define BIOS_CHARACTERISTICS_UPGRADEABLE   (1 << 11)
-- 
2.34.1

[PATCH v5 00/12] smbios: Deal with tables beyond 4GB

[Simon Glass]

When the malloc() region extends beyond 4GB on ARM we may end up with
an SMBIOS table in that region.

Add support for writing an SMBIOS3 table, which supports a 64-bit
address.

Note that this problem does not happen on x86 since it requires the
tables to be placed just below 1MB in memory, unless
CONFIG_BLOBLIST_TABLES is enabled.

v5 drops support for SMBIOS2 tables as well. It also adds some changes
to memory mapping which should allow an EFI app (launched from sandbox)
to read the ACPI tables correctly.

Changes in v5:
- Update comment to mention x86 alignment
- Add new patch to drop support for SMBIOS2 tables
- Add new patch to drop support for SMBIOS2 tables with EFI
- Add new patch to rename test dm_test_setup_ctx_and_base_tables()
- Add new patch to write pointers to tables instead of addresses
- Add new patch to correct smbios-table installation

Changes in v4:
- Bring in this patch from Heinrich's series
- Check the start of the table rather than the end

Changes in v2:
- Check the end of the table rather than the start.
- Add a new patch to correct gd_smbios_start()
- Add a note about why unmap_system() is called

Heinrich Schuchardt (1):
  smbios: SMBIOS 3.0 (64-bit) Entry Point structure

Simon Glass (11):
  smbios: Refactor 32-bit code into an else statement
  smbios: Move the rest of the SMBIOS2 code
  smbios: Use SMBIOS 3.0 to support an address above 4GB
  smbios: Correct gd_smbios_start()
  efi: Use the correct GUID for the SMBIOS table
  smbios: Require the caller to align the SMBIOS table
  smbios: Drop support for SMBIOS2 tables
  efi: smbios: Drop support for SMBIOS2 tables
  acpi: Rename test dm_test_setup_ctx_and_base_tables()
  acpi: Write pointers to tables instead of addresses
  efi: Correct smbios-table installation

 arch/sandbox/include/asm/io.h | 16 +
 arch/x86/lib/acpi_table.c |  6 ++--
 cmd/acpi.c| 12 +++
 include/asm-generic/global_data.h |  2 +-
 include/efi_api.h |  4 +++
 include/mapmem.h  | 18 ++
 include/smbios.h  | 41 ---
 lib/acpi/acpi.c   | 12 +++
 lib/acpi/acpi_table.c |  4 +--
 lib/acpi/base.c   |  4 +--
 lib/efi_loader/efi_smbios.c   | 28 +---
 lib/smbios.c  | 55 ++-
 test/dm/acpi.c| 14 
 13 files changed, 134 insertions(+), 82 deletions(-)

-- 
2.34.1

Re: Proposal: U-Boot memory management

[Tom Rini]

On Sun, Dec 31, 2023 at 07:22:10AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Sun, Dec 31, 2023 at 6:54 AM Tom Rini  wrote:
> >
> > On Sun, Dec 31, 2023 at 05:48:23AM -0700, Simon Glass wrote:
> > > Hi,
> > >
> > > On Fri, Dec 29, 2023 at 10:52 AM Tom Rini  wrote:
> > > >
> > > > On Fri, Dec 29, 2023 at 06:44:15PM +0100, Mark Kettenis wrote:
> > > > > > Date: Fri, 29 Dec 2023 11:17:44 -0500
> > > > > > From: Tom Rini 
> > > > > >
> > > > > > On Fri, Dec 29, 2023 at 05:05:17PM +0100, Heinrich Schuchardt wrote:
> > > > > > >
> > > > > > >
> > > > > > > Am 29. Dezember 2023 16:43:07 MEZ schrieb Tom Rini 
> > > > > > > :
> > > > > > > >On Fri, Dec 29, 2023 at 05:36:09AM +, Simon Glass wrote:
> > > > > > > >> Hi,
> > > > > > > >>
> > > > > > > >> On Sat, Dec 16, 2023 at 6:01 PM Simon Glass 
> > > > > > > >>  wrote:
> > > > > > > >> >
> > > > > > > >> > Hi,
> > > > > > > >> >
> > > > > > > >> > This records my thoughts after a discussion with Ilias & 
> > > > > > > >> > Heinrich re
> > > > > > > >> > memory allocation in U-Boot.
> > > > > > > >> >
> > > > > > > >> > 1. malloc()
> > > > > > > >> >
> > > > > > > >> > malloc() is used for programmatic memory allocation. It 
> > > > > > > >> > allows memory
> > > > > > > >> > to be freed. It is not designed for very large allocations 
> > > > > > > >> > (e.g. a
> > > > > > > >> > 10MB kernel or 100MB ramdisk).
> > > > > > > >> >
> > > > > > > >> > 2. lmb
> > > > > > > >> >
> > > > > > > >> > lmb is used for large blocks of memory, such as those needed 
> > > > > > > >> > for a
> > > > > > > >> > kernel or ramdisk. Allocation is only transitory, for the 
> > > > > > > >> > purposes of
> > > > > > > >> > loading some images and booting. If the boot fails, then all 
> > > > > > > >> > lmb
> > > > > > > >> > allocations go away.
> > > > > > > >> >
> > > > > > > >> > lmb is set up by getting all available memory and then 
> > > > > > > >> > removing what
> > > > > > > >> > is used by U-Boot (code, data, malloc() space, etc.)
> > > > > > > >> >
> > > > > > > >> > lmb reservations have a few flags so that areas of memory 
> > > > > > > >> > can be
> > > > > > > >> > provided with attributes
> > > > > > > >> >
> > > > > > > >> > There are some corner cases...e.g. loading a file does an lmb
> > > > > > > >> > allocation but only for the purpose of avoiding a file being 
> > > > > > > >> > loaded
> > > > > > > >> > over U-Boot code/data. The allocation is dropped immediately 
> > > > > > > >> > after the
> > > > > > > >> > file is loaded. Within the bootm command, or when using 
> > > > > > > >> > standard boot,
> > > > > > > >> > this would be fairly easy to solve.
> > > > > > > >> >
> > > > > > > >> > Linux has renamed lmb to memblock. We should consider doing 
> > > > > > > >> > the same.
> > > > > > > >> >
> > > > > > > >> > 3. EFI
> > > > > > > >> >
> > > > > > > >> > EFI has its own memory-allocation tables.
> > > > > > > >> >
> > > > > > > >> > Like lmb, EFI is able to deal with large allocations. But 
> > > > > > > >> > via a 'pool'
> > > > > > > >> > function it can also do smaller allocations similar to 
> > > > > > > >> > malloc(),
> > > > > > > >> > although each one uses at least 4KB at present.
> > > > > > > >> >
> > > > > > > >> > EFI allocations do not go away when a boot fails.
> > > > > > > >> >
> > > > > > > >> > With EFI it is possible to add allocations post facto, in 
> > > > > > > >> > which case
> > > > > > > >> > they are added to the allocation table just as if the memory 
> > > > > > > >> > was
> > > > > > > >> > allocated with EFI to begin with.
> > > > > > > >> >
> > > > > > > >> > The EFI allocations and the lmb allocations use the same 
> > > > > > > >> > memory, so in
> > > > > > > >> > principle could conflict.
> > > > > > > >> >
> > > > > > > >> > EFI allocations are sometimes used to allocate internal 
> > > > > > > >> > U-Boot data as
> > > > > > > >> > well, if needed by the EFI app. For example, while 
> > > > > > > >> > efi_image_parse()
> > > > > > > >> > uses malloc(), efi_var_mem.c uses EFI allocations since the 
> > > > > > > >> > code runs
> > > > > > > >> > in the app context and may need to access the memory after 
> > > > > > > >> > U-Boot has
> > > > > > > >> > exited. Also efi_smbios.c uses allocate_pages() and then 
> > > > > > > >> > adds a new
> > > > > > > >> > mapping as well.
> > > > > > > >> >
> > > > > > > >> > EFI memory has attributes, including what the memory is used 
> > > > > > > >> > for (to
> > > > > > > >> > some degree of granularity). See enum efi_memory_type and 
> > > > > > > >> > struct
> > > > > > > >> > efi_mem_desc. In the latter there are also attribute flags - 
> > > > > > > >> > whether
> > > > > > > >> > memory is cacheable, etc.
> > > > > > > >> >
> > > > > > > >> > EFI also has the x86 idea of 'conventional' memory, meaning 
> > > > > > > >> > (I
> > > > > > > >> > believe) that below 4GB that isn't reserved for the 
> > > > > > > >> > hardware/system.
> > > > > > > >> > This is meaningless, 

Re: [PATCH v6] fdt: Allow the devicetree to come from a bloblist

[Tom Rini]

On Sun, Dec 31, 2023 at 05:47:30AM -0700, Simon Glass wrote:
> Hi,
> 
> On Thu, Dec 28, 2023 at 2:24 PM Tom Rini  wrote:
> >
> > On Thu, Dec 28, 2023 at 07:47:25PM +, Simon Glass wrote:
> >
> > > Standard passage provides for a bloblist to be passed from one firmware
> > > phase to the next. That can be used to pass the devicetree along as well.
> > > Add an option to support this.
> > >
> > > Tests for this will be added as part of the Universal Payload work.
> > >
> > > Signed-off-by: Simon Glass 
> > > ---
> > [snip]
> > > diff --git a/dts/Kconfig b/dts/Kconfig
> > > index 00c0aeff893..ae451b9caf7 100644
> > > --- a/dts/Kconfig
> > > +++ b/dts/Kconfig
> > > @@ -105,6 +105,19 @@ config OF_EMBED
> > >
> > >  endchoice
> > >
> > > +config OF_BLOBLIST
> > > + bool "Provided by a bloblist at runtime"
> > > + depends on BLOBLIST && OF_SEPARATE
> >
> > This is now even more confusing, frankly. The help for OF_SEPARATE says:
> > If this option is enabled, the device tree will be built and placed as a
> > separate u-boot.dtb file alongside the U-Boot image.
> >
> > So why would you enable that to then have a device tree passed via
> > bloblist instead?
> 
> Historical

Sorry, I mean the help text doesn't make any sense, with your patch. So
that needs to be fixed. That's part of how everyone else is going to try
and understand this mess of options afterwards.

> > We should probably start by fixing all of this confusing naming / logic
> > and then correct things such that:
> > - OF_EMBED wins if set. This is the override-has-been-set we-must-use-it
> >   switch. First choice, not last choice. If binman needs tweaks so that
> >   it will still generate images for platforms in this case, that needs
> >   to happen.
> 
> Perhaps we should rename this to OF_EMBED_DEBUG ? Really it should not
> be used by any board.

It may or may not be appropriate for use by boards, in production. Maybe
future designs can avoid this, but I think current ones cannot. But
that's also a separate thread an cleanup task to bring up. Again, I
suspect one of the use cases here is "embed the DTB so that we secure
boot check and validate a single memory range".

> > - If we have a bloblist, we scan the bloblist for DT and if found, use
> >   it.
> 
> unless we don't want to, e.g. because that DT has a bug or we want to
> use a different one during development.

Nope. If the bloblist has a bug we either have to perform a fixup (the
bloblist can't be changed but it's a situation where we're less strict)
or for development go back up to OF_EMBED. That's the switch for
development where you changed the device tree.

> > - If it looks like we've been booted as a fake Linux kernel, and we can
> >   start with just aarch64 and let riscv come in as a follow up, so
> >   what's documented within
> >   
> > https://www.kernel.org/doc/html/latest/arch/arm64/booting.html#call-the-kernel-image
> >   then we use that device tree.
> 
> Eek, really? Is this the rpi case and you are trying to make it
> generic? I would rather that be a board-specific thing, calling into a
> generic implementation. We should encourage bloblist.

No, it's the generic case. It's not board specific. It's what we do,
iirc, on Apple M1/M2/etc, some subsets of Tegra I believe (I know I've
done it for xavier-based systems, but then abandoned the effort),
certainly some of the older qualcomm platforms, and I believe that's one
of the entry points for the newer qualcomm platforms.

I spelled out the case above the way that I did because it's a valid
case that's not going away. In addition to the places where we can't
change the firmware chain, it's also for the places where people don't
want to change the firmware chain, or because it's an important bringup
path.

And part of the problem here is that we've just done it ad-hoc so many
times that it's scattered about the tree.

> >   - This _may_ just end up having to be "Does x0 (or similar) point to a
> > valid DT?" as I don't know how correct everything using this method
> > today is too what the spec above lists.
> 
> OK, well a generic impl would be good I suppose, but dereferencing
> pointers to look for a magic number might not end well.

It's a long standing ad-hoc standard.

> > - If we have a dtb appended to use by what we call today OF_SEPARATE but
> >   should really stop calling it that we use that.
> 
> Yes it is a complete misnomer now. I will try to think of a better
> name. It really just controls generation of u-boot.dtb and what goes
> in u-boot.bin so perhaps we can just drop it and have OF_EMBED be
> false in the normal case. Or rename to OF_STANDARD ?

I'm not sure about the name either.

> > At that point, we can probably have zero "totally board specific kludge
> > for device tree location", and kill off OF_HAS_PRIOR_STAGE too (since
> > that's really just bloblist or fake-linux-kernel). We'll also be able to
> > support migration from fake-linux-kernel to bloblist
> 
> OF_HAS_PRIOR_STAGE 

Re: [PATCH v3 4/8] dts: Add alternative location for upstream DTB builds

[Simon Glass]

Hi Sumit,

On Fri, Dec 29, 2023 at 8:30 AM Sumit Garg  wrote:
>
> On Fri, 29 Dec 2023 at 01:18, Simon Glass  wrote:
> >
> > Hi Tom,
> >
> > On Thu, Dec 28, 2023 at 3:54 PM Tom Rini  wrote:
> > >
> > > On Thu, Dec 28, 2023 at 03:09:12PM +, Simon Glass wrote:
> > > > Hi Tom, Sumit,
> > > >
> > > > On Thu, Dec 28, 2023 at 2:03 PM Tom Rini  wrote:
> > > > >
> > > > > On Thu, Dec 28, 2023 at 01:37:26PM +, Simon Glass wrote:
> > > > > > Hi Sumit,
> > > > > >
> > > > > > On Thu, Dec 28, 2023 at 11:58 AM Sumit Garg  
> > > > > > wrote:
> > > > > > >
> > > > > > > Allow platform owners to mirror devicetree files from 
> > > > > > > devitree-rebasing
> > > > > > > directory into dts/arch/$(ARCH) (special case for 
> > > > > > > dts/arch/arm64). Then
> > > > > > > build then along with any *-u-boot.dtsi file present in 
> > > > > > > arch/$(ARCH)/dts
> > > > > > > directory. Also add a new Makefile for arm64.
> > > > > > >
> > > > > > > This will help easy migration for platforms which currently are 
> > > > > > > compliant
> > > > > > > with upstream Linux kernel devicetree files.
> > > > > > >
> > > > > > > Signed-off-by: Sumit Garg 
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v3:
> > > > > > > --
> > > > > > > - Minor commit message update
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > > --
> > > > > > > - s/DEVICE_TREE_LOC/dt_dir/ and s/U-boot/U-Boot/
> > > > > > >
> > > > > > >  dts/Kconfig | 11 +++
> > > > > > >  dts/Makefile| 17 ++---
> > > > > > >  dts/arch/arm64/Makefile | 14 ++
> > > > > > >  3 files changed, 39 insertions(+), 3 deletions(-)
> > > > > > >  create mode 100644 dts/arch/arm64/Makefile
> > > > > > >
> > > > > > > diff --git a/dts/Kconfig b/dts/Kconfig
> > > > > > > index 00c0aeff893..e58c1c6f2ab 100644
> > > > > > > --- a/dts/Kconfig
> > > > > > > +++ b/dts/Kconfig
> > > > > > > @@ -85,6 +85,17 @@ config OF_LIVE
> > > > > > >   enables a live tree which is available after relocation,
> > > > > > >   and can be adjusted as needed.
> > > > > > >
> > > > > > > +config OF_UPSTREAM
> > > > > > > +   bool "Enable use of devicetree imported from Linux kernel 
> > > > > > > release"
> > > > > > > +   help
> > > > > > > + Traditionally, U-Boot platforms used to have their 
> > > > > > > custom devicetree
> > > > > > > + files or copy devicetree files from Linux kernel which 
> > > > > > > are hard to
> > > > > > > + maintain and can usually get out-of-sync from Linux 
> > > > > > > kernel. This
> > > > > > > + option enables platforms to migrate to 
> > > > > > > devicetree-rebasing repo where
> > > > > > > + a regular sync will be maintained every major Linux 
> > > > > > > kernel release
> > > > > > > + cycle. However, platforms can still have some custom 
> > > > > > > u-boot specific
> > > > > > > + bits maintained as part of *-u-boot.dtsi files.
> > > > > >
> > > > > > My only other suggestion here is to mention that this should be set 
> > > > > > in
> > > > > > Kconfig, for the SoC as a whole. So I believe that means that it
> > > > > > should be hidden, with no string for the 'bool':
> > > > > >
> > > > > >   bool  # Enable use of devicetree imported from Linux kernel 
> > > > > > release
> > > > >
> > > > > I think we can just keep prompting for it now, to make the transition
> > > > > easier, before this option just goes away in time, hopefully.
> > > > >
> > > > > > Also, this doesn't seem to work for me. Before this series I get 
> > > > > > these
> > > > > > files when building firefly-rk3399:
> > > > > >
> > > > > > rk3399-eaidk-610.dtbrk3399-khadas-edge-v.dtb
> > > > > > rk3399-orangepi.dtbrk3399-rock-pi-4a.dtb
> > > > > > rk3399-evb.dtb  rk3399-leez-p710.dtb
> > > > > > rk3399-pinebook-pro.dtbrk3399-rock-pi-4c.dtb
> > > > > > rk3399-ficus.dtbrk3399-nanopc-t4.dtb
> > > > > > rk3399-pinephone-pro.dtb   rk3399-rockpro64.dtb
> > > > > > rk3399-firefly.dtb  rk3399-nanopi-m4-2gb.dtb
> > > > > > rk3399pro-rock-pi-n10.dtb  rk3399-roc-pc.dtb
> > > > > > rk3399-gru-bob.dtb  rk3399-nanopi-m4b.dtb
> > > > > > rk3399-puma-haikou.dtb rk3399-roc-pc-mezzanine.dtb
> > > > > > rk3399-gru-kevin.dtbrk3399-nanopi-m4.dtb
> > > > > > rk3399-rock-4c-plus.dtb
> > > > > > rk3399-khadas-edge-captain.dtb  rk3399-nanopi-neo4.dtb
> > > > > > rk3399-rock-4se.dtb
> > > > > > rk3399-khadas-edge.dtb  rk3399-nanopi-r4s.dtb 
> > > > > > rk3399-rock960.dtb
> > > > > >
> > > > > > Afterwards I get this:
> > > > > >
> > > > > > make[3]: *** No rule to make target
> > > > > > 'dts/arch/arm64/rk3399-firefly.dtb', needed by 'dtbs'.  Stop.
> > > > > >
> > > > > > So I set this manually for that one board:
> > > > > >
> > > > > > CONFIG_DEFAULT_DEVICE_TREE="rockchip/rk3399-firefly"
> > > > > >
> > > > > > and get:
> > > > > >
> > >

Re: [PATCH v2 1/1] efi_loader: expose the device-tree file name

[Peter Robinson]

On Mon, Oct 23, 2023 at 4:55 PM Mark Kettenis  wrote:
>
> > From: Simon Glass 
> > Date: Mon, 23 Oct 2023 00:08:40 -0700
> >
> > > > fdt_node_check_compatible() does most of the work...then you need to
> > > > check which FDT has the most specific match (i.e. latest in the string
> > > > list). That handles things like board revisions, variants, etc.
> > > >
> > > > My concern is about adding a feature when there is already a defined
> > > > spec and mechanism for this to work. What happens when we load the
> > > > file and the compatible is wrong?
> > > >
> > > > At best, I see the filename as a hint.
> > > >
> > > > [Perhaps this is the wrong time to ask, but why are kernels +DT not
> > > > shipped in FIT on ARM?]
> > >
> > > FIT is U-Boot specific. For Linux distributions it is easier to use a
> > > firmware agnostic method of booting.
> >
> > I'd like to suggest that distros use both. Then U-Boot can work as it
> > was designed and we can avoid these work-arounds.
> >
> > FIT is actually implemented in various other bootloaders. In fact
> > perhaps grub is the only one that doesn't? I can't think of any
> > others.
>
> Simon, please stop pushing this.  OpenBSD's bootloader does not
> support FIT and we have no interest in supporting it.  Our users
> expect to be able to just copy a new kernel in place and use it and
> our OS upgrade procedure depends on this as well.  And this is
> incompatble with FIT.  I've explained this about a dozen times to you
> now.

For reference Fedora and related ecosystems have no interest in
supporting FIT either, we have enough moving targets, we're not
supporting more, it's very much why we're focused on UEFI, it provides
one way, one kernel, for booting all various different devices we
actively support in main Fedora, it's very much the way the x86
ecosystem has gone as well as the aarch64 server ecosystem. Fedora has
no interest in using FIT in this context either.

Peter

Re: [PATCH 1/1] cmd: fdt: check fdt address

[Simon Glass]

Hi Heinrich,

On Wed, Dec 20, 2023 at 5:00 PM Heinrich Schuchardt
 wrote:
>
> Trying to read a device-tree from an illegal address leads to a crash.
>
> Check that the parameter passed to 'fdt addr' is within the RAM area and
> non-zero.

What is the motivation for this patch? Is something crashing? We don't
do this with the 'md' command, for example.

Regards,
Simon


>
> Signed-off-by: Heinrich Schuchardt 
> ---
>  cmd/fdt.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index 331564c13b..dc954ea7d5 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -193,6 +193,11 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int 
> argc, char *const argv[])
> }
>
> addr = hextoul(argv[0], NULL);
> +   if (!addr || addr < gd->ram_base || addr >= gd->ram_top) {
> +   printf("Invalid address\n");
> +   return CMD_RET_FAILURE;
> +   }
> +
> blob = map_sysmem(addr, 0);
> if ((quiet && fdt_check_header(blob)) ||
> (!quiet && !fdt_valid(&blob)))
> --
> 2.40.1
>

Re: Proposal: U-Boot memory management

[Simon Glass]

Hi Tom,

On Sun, Dec 31, 2023 at 6:54 AM Tom Rini  wrote:
>
> On Sun, Dec 31, 2023 at 05:48:23AM -0700, Simon Glass wrote:
> > Hi,
> >
> > On Fri, Dec 29, 2023 at 10:52 AM Tom Rini  wrote:
> > >
> > > On Fri, Dec 29, 2023 at 06:44:15PM +0100, Mark Kettenis wrote:
> > > > > Date: Fri, 29 Dec 2023 11:17:44 -0500
> > > > > From: Tom Rini 
> > > > >
> > > > > On Fri, Dec 29, 2023 at 05:05:17PM +0100, Heinrich Schuchardt wrote:
> > > > > >
> > > > > >
> > > > > > Am 29. Dezember 2023 16:43:07 MEZ schrieb Tom Rini 
> > > > > > :
> > > > > > >On Fri, Dec 29, 2023 at 05:36:09AM +, Simon Glass wrote:
> > > > > > >> Hi,
> > > > > > >>
> > > > > > >> On Sat, Dec 16, 2023 at 6:01 PM Simon Glass  
> > > > > > >> wrote:
> > > > > > >> >
> > > > > > >> > Hi,
> > > > > > >> >
> > > > > > >> > This records my thoughts after a discussion with Ilias & 
> > > > > > >> > Heinrich re
> > > > > > >> > memory allocation in U-Boot.
> > > > > > >> >
> > > > > > >> > 1. malloc()
> > > > > > >> >
> > > > > > >> > malloc() is used for programmatic memory allocation. It allows 
> > > > > > >> > memory
> > > > > > >> > to be freed. It is not designed for very large allocations 
> > > > > > >> > (e.g. a
> > > > > > >> > 10MB kernel or 100MB ramdisk).
> > > > > > >> >
> > > > > > >> > 2. lmb
> > > > > > >> >
> > > > > > >> > lmb is used for large blocks of memory, such as those needed 
> > > > > > >> > for a
> > > > > > >> > kernel or ramdisk. Allocation is only transitory, for the 
> > > > > > >> > purposes of
> > > > > > >> > loading some images and booting. If the boot fails, then all 
> > > > > > >> > lmb
> > > > > > >> > allocations go away.
> > > > > > >> >
> > > > > > >> > lmb is set up by getting all available memory and then 
> > > > > > >> > removing what
> > > > > > >> > is used by U-Boot (code, data, malloc() space, etc.)
> > > > > > >> >
> > > > > > >> > lmb reservations have a few flags so that areas of memory can 
> > > > > > >> > be
> > > > > > >> > provided with attributes
> > > > > > >> >
> > > > > > >> > There are some corner cases...e.g. loading a file does an lmb
> > > > > > >> > allocation but only for the purpose of avoiding a file being 
> > > > > > >> > loaded
> > > > > > >> > over U-Boot code/data. The allocation is dropped immediately 
> > > > > > >> > after the
> > > > > > >> > file is loaded. Within the bootm command, or when using 
> > > > > > >> > standard boot,
> > > > > > >> > this would be fairly easy to solve.
> > > > > > >> >
> > > > > > >> > Linux has renamed lmb to memblock. We should consider doing 
> > > > > > >> > the same.
> > > > > > >> >
> > > > > > >> > 3. EFI
> > > > > > >> >
> > > > > > >> > EFI has its own memory-allocation tables.
> > > > > > >> >
> > > > > > >> > Like lmb, EFI is able to deal with large allocations. But via 
> > > > > > >> > a 'pool'
> > > > > > >> > function it can also do smaller allocations similar to 
> > > > > > >> > malloc(),
> > > > > > >> > although each one uses at least 4KB at present.
> > > > > > >> >
> > > > > > >> > EFI allocations do not go away when a boot fails.
> > > > > > >> >
> > > > > > >> > With EFI it is possible to add allocations post facto, in 
> > > > > > >> > which case
> > > > > > >> > they are added to the allocation table just as if the memory 
> > > > > > >> > was
> > > > > > >> > allocated with EFI to begin with.
> > > > > > >> >
> > > > > > >> > The EFI allocations and the lmb allocations use the same 
> > > > > > >> > memory, so in
> > > > > > >> > principle could conflict.
> > > > > > >> >
> > > > > > >> > EFI allocations are sometimes used to allocate internal U-Boot 
> > > > > > >> > data as
> > > > > > >> > well, if needed by the EFI app. For example, while 
> > > > > > >> > efi_image_parse()
> > > > > > >> > uses malloc(), efi_var_mem.c uses EFI allocations since the 
> > > > > > >> > code runs
> > > > > > >> > in the app context and may need to access the memory after 
> > > > > > >> > U-Boot has
> > > > > > >> > exited. Also efi_smbios.c uses allocate_pages() and then adds 
> > > > > > >> > a new
> > > > > > >> > mapping as well.
> > > > > > >> >
> > > > > > >> > EFI memory has attributes, including what the memory is used 
> > > > > > >> > for (to
> > > > > > >> > some degree of granularity). See enum efi_memory_type and 
> > > > > > >> > struct
> > > > > > >> > efi_mem_desc. In the latter there are also attribute flags - 
> > > > > > >> > whether
> > > > > > >> > memory is cacheable, etc.
> > > > > > >> >
> > > > > > >> > EFI also has the x86 idea of 'conventional' memory, meaning (I
> > > > > > >> > believe) that below 4GB that isn't reserved for the 
> > > > > > >> > hardware/system.
> > > > > > >> > This is meaningless, or at least confusing, on ARM systems.
> > > > > > >> >
> > > > > > >> > 4. reservations
> > > > > > >> >
> > > > > > >> > It is perhaps worth mentioning a fourth method of memory 
> > > > > > >> > management,
> > > > > > >> > where U-Boot reserves chunks of memory before relocation (in
> > > > > > >> > boar

Re: [PATCH 3/5] cmd: provide command to display SMBIOS information

[Peter Robinson]

On Sat, Dec 23, 2023 at 12:53 AM Heinrich Schuchardt  wrote:
>
> U-Boot can either generated an SMBIOS table or copy it from a prior boot
> stage, e.g. QEMU.
>
> Provide a command to display the SMBIOS information.
>
> Currently only type 1 and 2 are translated to human readable text.
> Other types may be added later. Currently only a hexdump and the list of
> strings is provided for these.
>
> Signed-off-by: Heinrich Schuchardt 
> ---
>  cmd/Kconfig  |   7 ++
>  cmd/Makefile |   1 +
>  cmd/smbios.c | 191 +++
>  3 files changed, 199 insertions(+)
>  create mode 100644 cmd/smbios.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5a7678f0ac..580aeec8ee 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -206,6 +206,13 @@ config CMD_SBI
> help
>   Display information about the SBI implementation.
>
> +config CMD_SMBIOS
> +   bool "smbios"
> +   depends on SMBIOS
> +   default y

Should this be default? Maybe default if SMBIOS or similar?

> +   help
> + Display the SMBIOS information.
> +
>  endmenu
>
>  menu "Boot commands"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 5ed0e4011d..8a5dfd8ca9 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -168,6 +168,7 @@ obj-$(CONFIG_CMD_SETEXPR) += setexpr.o
>  obj-$(CONFIG_CMD_SETEXPR_FMT) += printf.o
>  obj-$(CONFIG_CMD_SPI) += spi.o
>  obj-$(CONFIG_CMD_STRINGS) += strings.o
> +obj-$(CONFIG_CMD_SMBIOS) += smbios.o
>  obj-$(CONFIG_CMD_SMC) += smccc.o
>  obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o
>  obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o
> diff --git a/cmd/smbios.c b/cmd/smbios.c
> new file mode 100644
> index 00..63f908e92c
> --- /dev/null
> +++ b/cmd/smbios.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * The 'smbios' command displays information from the SMBIOS table.
> + *
> + * Copyright (c) 2023, Heinrich Schuchardt 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/**
> + * smbios_get_string() - get SMBIOS string from table
> + *
> + * @table: SMBIOS table
> + * @index: index of the string
> + * Return: address of string, may point to empty string
> + */
> +static const char *smbios_get_string(void *table, int index)
> +{
> +   const char *str = (char *)table +
> + ((struct smbios_header *)table)->length;
> +
> +   if (!*str)
> +   ++str;
> +   for (--index; *str && index; --index)
> +   str += strlen(str) + 1;
> +
> +   return str;
> +}
> +
> +static struct smbios_header *next_table(struct smbios_header *table)
> +{
> +   const char *str;
> +
> +   if (table->type == SMBIOS_END_OF_TABLE)
> +   return NULL;
> +
> +   str = smbios_get_string(table, 0);
> +   return (struct smbios_header *)(++str);
> +}
> +
> +static void smbios_print_generic(struct smbios_header *table)
> +{
> +   char *str = (char *)table + table->length;
> +
> +   if (CONFIG_IS_ENABLED(HEXDUMP)) {
> +   printf("Header and Data:\n");
> +   print_hex_dump("\t", DUMP_PREFIX_OFFSET, 16, 1,
> +  table, table->length, false);
> +   }
> +   if (*str) {
> +   printf("Strings:\n");
> +   for (int index = 1; *str; ++index) {
> +   printf("\tString %u: %s\n", index, str);
> +   str += strlen(str) + 1;
> +   }
> +   }
> +}
> +
> +void smbios_print_str(const char *label, void *table, u8 index)
> +{
> +   printf("\t%s: %s\n", label, smbios_get_string(table, index));
> +}
> +
> +static void smbios_print_type1(struct smbios_type1 *table)
> +{
> +   printf("System Information\n");
> +   smbios_print_str("Manufacturer", table, table->manufacturer);
> +   smbios_print_str("Product Name", table, table->product_name);
> +   smbios_print_str("Version", table, table->version);
> +   smbios_print_str("Serial Number", table, table->serial_number);
> +   if (table->length >= 0x19) {
> +   printf("\tUUID %pUl\n", table->uuid);
> +   smbios_print_str("Wake Up Type", table, table->serial_number);
> +   }
> +   if (table->length >= 0x1b) {
> +   smbios_print_str("Serial Number", table, 
> table->serial_number);
> +   smbios_print_str("SKU Number", table, table->sku_number);
> +   }
> +}
> +
> +static void smbios_print_type2(struct smbios_type2 *table)
> +{
> +   u16 *handle;
> +
> +   printf("Base Board Information\n");
> +   smbios_print_str("Manufacturer", table, table->manufacturer);
> +   smbios_print_str("Product Name", table, table->product_name);
> +   smbios_print_str("Version", table, table->version);
> +   smbios_print_str("Serial Number", table, table->serial_number);
> +   smbios_print_str("Asset Tag", table, table->asset_

Re: [PATCH 0/9] dts: Move to SoC-specific build rules

[Tom Rini]

On Sun, Dec 31, 2023 at 05:45:00AM -0700, Simon Glass wrote:
> -Scott as it is bouncing
> 
> Hi,
> 
> On Fri, Dec 29, 2023 at 9:46 AM Peter Robinson  wrote:
> >
> > On Fri, Dec 29, 2023 at 12:23 AM Tom Rini  wrote:
> > >
> > > On Thu, Dec 28, 2023 at 07:48:08PM +, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, Dec 28, 2023 at 3:40 PM Tom Rini  wrote:
> > > > >
> > > > > On Thu, Dec 28, 2023 at 03:09:40PM +, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, Dec 28, 2023 at 2:23 PM Tom Rini  wrote:
> > > > > > >
> > > > > > > On Thu, Dec 28, 2023 at 01:37:07PM +, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Wed, Dec 27, 2023 at 1:21 PM Tom Rini  
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Dec 27, 2023 at 08:23:56AM +, Simon Glass wrote:
> > > > > > > > >
> > > > > > > > > > U-Boot builds devicetree binaries from its source tree. As 
> > > > > > > > > > part of the
> > > > > > > > > > Kconfig conversion, the Makefiles were updated to align 
> > > > > > > > > > with how this
> > > > > > > > > > is done in Linux: a single target for each SoC is used to 
> > > > > > > > > > build all the
> > > > > > > > > > .dtb files for that SoC.
> > > > > > > > > >
> > > > > > > > > > Since then, the Makefiles have devolved in some cases, 
> > > > > > > > > > resulting in
> > > > > > > > > > lots of target-specific build rules. Also Linux has moved 
> > > > > > > > > > to using
> > > > > > > > > > subdirectories for each vendor.
> > > > > > > > > >
> > > > > > > > > > Recent work aims to allow U-Boot to directly use devicetree 
> > > > > > > > > > files from
> > > > > > > > > > Linux. This would be easier if the directory structure were 
> > > > > > > > > > the same.
> > > > > > > > > > Another recent discussion involved dropping the build rules 
> > > > > > > > > > altogether.
> > > > > > > > > >
> > > > > > > > > > This series makes a start at cleaning up some of the build 
> > > > > > > > > > rules, to
> > > > > > > > > > reduce the amount of code and make it easier to add new 
> > > > > > > > > > boards for the
> > > > > > > > > > same SoC.
> > > > > > > > > >
> > > > > > > > > > One issue is that the ARCH_xxx Kconfig options between 
> > > > > > > > > > U-Boot and Linux
> > > > > > > > > > are not always the same. Given the large number of SoCs and 
> > > > > > > > > > boards
> > > > > > > > > > supported by U-Boot, it would be useful to align these 
> > > > > > > > > > where possible.
> > > > > > > > >
> > > > > > > > > I don't know why we should start with this now, and further 
> > > > > > > > > not being on
> > > > > > > > > top of Sumit's series to remove our duplicate dts files. And 
> > > > > > > > > that's
> > > > > > > > > where we can have the conversation about for which cases it 
> > > > > > > > > even makes
> > > > > > > > > sense to build all of the dts files for a given SoC.
> > > > > > > >
> > > > > > > > This is a completely different series - there are no conflicts 
> > > > > > > > with
> > > > > > > > Sumit's series so it can be applied before or after it.
> > > > > > > >
> > > > > > > > My goal here is to clean up our build rules, rather than just 
> > > > > > > > throwing
> > > > > > > > them all away, for reasons we have discussed previously. I 
> > > > > > > > filed [1]
> > > > > > > > to track that.
> > > > > > >
> > > > > > > Yes, I'm saying we shouldn't be doing anything this series does 
> > > > > > > until
> > > > > > > after Sumit's series has landed. Along with the fact that I don't 
> > > > > > > like
> > > > > > > what's going on in this series.
> > > > > >
> > > > > > This seems to again be the disagreement over whether a single DT
> > > > > > should be build for each board, or all the DTs for an SoC?
> > > > >
> > > > > It's about the disagreement on what we should be building, and what 
> > > > > that
> > > > > infrastructure looks like. I do not like adding extra rules for
> > > > > "clarity" because they don't make things clearer, they lead to the
> > > > > unclear mess we have today getting worse. Most instructions are _not_
> > > > > "now take this dtb and this binary and .." but just "take this 
> > > > > binary",
> > > > > because we already have the rules and logic to ensure we build the
> > > > > required dtbs. I also don't like the parts of this series that amount
> > > > > to deck shuffling when we should just be taking the chairs away. 
> > > > > There's
> > > > > just not nor will there be a case for omap3/4/5 platforms of "change 
> > > > > the
> > > > > dtb", so building more is pointless. For other SoCs, there may be some
> > > > > cases of "this could have been just a DT change", like
> > > > > rock5b-rk3588_defconfig / rock5a-rk3588s_defconfig could share a board
> > > > > dir, but the configs are different and the dts are different, so I 
> > > > > don't
> > > > > know that you could really just swap the dtbs.
> > > >
> > > > It is true that we have a different def

Re: Proposal: U-Boot memory management

[Tom Rini]

On Sun, Dec 31, 2023 at 05:48:23AM -0700, Simon Glass wrote:
> Hi,
> 
> On Fri, Dec 29, 2023 at 10:52 AM Tom Rini  wrote:
> >
> > On Fri, Dec 29, 2023 at 06:44:15PM +0100, Mark Kettenis wrote:
> > > > Date: Fri, 29 Dec 2023 11:17:44 -0500
> > > > From: Tom Rini 
> > > >
> > > > On Fri, Dec 29, 2023 at 05:05:17PM +0100, Heinrich Schuchardt wrote:
> > > > >
> > > > >
> > > > > Am 29. Dezember 2023 16:43:07 MEZ schrieb Tom Rini 
> > > > > :
> > > > > >On Fri, Dec 29, 2023 at 05:36:09AM +, Simon Glass wrote:
> > > > > >> Hi,
> > > > > >>
> > > > > >> On Sat, Dec 16, 2023 at 6:01 PM Simon Glass  
> > > > > >> wrote:
> > > > > >> >
> > > > > >> > Hi,
> > > > > >> >
> > > > > >> > This records my thoughts after a discussion with Ilias & 
> > > > > >> > Heinrich re
> > > > > >> > memory allocation in U-Boot.
> > > > > >> >
> > > > > >> > 1. malloc()
> > > > > >> >
> > > > > >> > malloc() is used for programmatic memory allocation. It allows 
> > > > > >> > memory
> > > > > >> > to be freed. It is not designed for very large allocations (e.g. 
> > > > > >> > a
> > > > > >> > 10MB kernel or 100MB ramdisk).
> > > > > >> >
> > > > > >> > 2. lmb
> > > > > >> >
> > > > > >> > lmb is used for large blocks of memory, such as those needed for 
> > > > > >> > a
> > > > > >> > kernel or ramdisk. Allocation is only transitory, for the 
> > > > > >> > purposes of
> > > > > >> > loading some images and booting. If the boot fails, then all lmb
> > > > > >> > allocations go away.
> > > > > >> >
> > > > > >> > lmb is set up by getting all available memory and then removing 
> > > > > >> > what
> > > > > >> > is used by U-Boot (code, data, malloc() space, etc.)
> > > > > >> >
> > > > > >> > lmb reservations have a few flags so that areas of memory can be
> > > > > >> > provided with attributes
> > > > > >> >
> > > > > >> > There are some corner cases...e.g. loading a file does an lmb
> > > > > >> > allocation but only for the purpose of avoiding a file being 
> > > > > >> > loaded
> > > > > >> > over U-Boot code/data. The allocation is dropped immediately 
> > > > > >> > after the
> > > > > >> > file is loaded. Within the bootm command, or when using standard 
> > > > > >> > boot,
> > > > > >> > this would be fairly easy to solve.
> > > > > >> >
> > > > > >> > Linux has renamed lmb to memblock. We should consider doing the 
> > > > > >> > same.
> > > > > >> >
> > > > > >> > 3. EFI
> > > > > >> >
> > > > > >> > EFI has its own memory-allocation tables.
> > > > > >> >
> > > > > >> > Like lmb, EFI is able to deal with large allocations. But via a 
> > > > > >> > 'pool'
> > > > > >> > function it can also do smaller allocations similar to malloc(),
> > > > > >> > although each one uses at least 4KB at present.
> > > > > >> >
> > > > > >> > EFI allocations do not go away when a boot fails.
> > > > > >> >
> > > > > >> > With EFI it is possible to add allocations post facto, in which 
> > > > > >> > case
> > > > > >> > they are added to the allocation table just as if the memory was
> > > > > >> > allocated with EFI to begin with.
> > > > > >> >
> > > > > >> > The EFI allocations and the lmb allocations use the same memory, 
> > > > > >> > so in
> > > > > >> > principle could conflict.
> > > > > >> >
> > > > > >> > EFI allocations are sometimes used to allocate internal U-Boot 
> > > > > >> > data as
> > > > > >> > well, if needed by the EFI app. For example, while 
> > > > > >> > efi_image_parse()
> > > > > >> > uses malloc(), efi_var_mem.c uses EFI allocations since the code 
> > > > > >> > runs
> > > > > >> > in the app context and may need to access the memory after 
> > > > > >> > U-Boot has
> > > > > >> > exited. Also efi_smbios.c uses allocate_pages() and then adds a 
> > > > > >> > new
> > > > > >> > mapping as well.
> > > > > >> >
> > > > > >> > EFI memory has attributes, including what the memory is used for 
> > > > > >> > (to
> > > > > >> > some degree of granularity). See enum efi_memory_type and struct
> > > > > >> > efi_mem_desc. In the latter there are also attribute flags - 
> > > > > >> > whether
> > > > > >> > memory is cacheable, etc.
> > > > > >> >
> > > > > >> > EFI also has the x86 idea of 'conventional' memory, meaning (I
> > > > > >> > believe) that below 4GB that isn't reserved for the 
> > > > > >> > hardware/system.
> > > > > >> > This is meaningless, or at least confusing, on ARM systems.
> > > > > >> >
> > > > > >> > 4. reservations
> > > > > >> >
> > > > > >> > It is perhaps worth mentioning a fourth method of memory 
> > > > > >> > management,
> > > > > >> > where U-Boot reserves chunks of memory before relocation (in
> > > > > >> > board_init_f.c), e.g. for the framebuffer, U-Boot code, the 
> > > > > >> > malloc()
> > > > > >> > region, etc.
> > > > > >> >
> > > > > >> >
> > > > > >> > Problems
> > > > > >> > —---
> > > > > >> >
> > > > > >> > There are no urgent problems, but here are some things that 
> > > > > >> > could be improved:
> > > > > >> >
> > > > > >> > 1. EFI should attach most of it

Re: [PATCH v4 4/7] smbios: Use SMBIOS 3.0 to support an address above 4GB

[Simon Glass]

Hi Tom,

On Thu, Dec 28, 2023 at 10:57 AM Tom Rini  wrote:
>
> On Thu, Dec 28, 2023 at 03:46:09PM +0100, Mark Kettenis wrote:
> > > From: Simon Glass 
> > > Date: Thu, 28 Dec 2023 13:37:03 +
> > >
> > > Hi Ilias,
> > >
> > > On Wed, Dec 27, 2023 at 11:05 AM Ilias Apalodimas
> > >  wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Wed, 27 Dec 2023 at 09:40, Simon Glass  wrote:
> > > > >
> > > > > When the SMBIOS table is written to an address above 4GB a 32-bit 
> > > > > table
> > > > > address is not large enough.
> > > > >
> > > > > Use an SMBIOS3 table in that case.
> > > >
> > > > Maybe I missed this on the previous revisions, but is there a reason
> > > > we don't always use SMBIOS3 now?
> > >
> > > I am not sure...there was some comment about it not being supported in
> > > some cases, so I have tried to accommodate that.
> > >
> > > >  And perhaps try to install SMBIOS2 if
> > > > 1. we fail
> > >
> > > due to?
> > >
> > > > 2. and the address is < 4GB
> > >
> > > We could, I suppose. Effectively we would drop generation of SMBIOS2.
> > >
> > > I really don't mind. This whole SMBIOS thing is a bit ridiculous, if
> > > you ask me.
> >
> > Linux added support for the SMBIOS 3.0 64-bit entry point in 2014.  I
> > doubt anyone who cares about SMBIOS cares about kernels that old.
> >
> > So if it simplifies things, I'd drop support for the 32-bit SMBIOS
> > entry point.
>
> I agree, lets just provide SMBIOS3 tables.

OK...I would like to do this as a followup patch, so we still have the
SMBIOS2 in the git history. I will take a look.

Regards,
Simon

Re: [PATCH 1/2] image-host: add a check of the return value of snprintf.

[Simon Glass]

Hi Hugo,

On Fri, Dec 29, 2023 at 2:07 PM Hugo Cornelis
 wrote:
>
> This patch allows to generate a sensible error message when generating
> binary images using very long filenames.
>
> This can happen with Buildroot and Yocto builds.
>
> Signed-off-by: Hugo Cornelis 
> ---
>
>  tools/image-host.c | 29 +
>  1 file changed, 25 insertions(+), 4 deletions(-)
>

Good to see this, but I would like a refactoring, please see below.

> diff --git a/tools/image-host.c b/tools/image-host.c
> index ca4950312f..3719f36117 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -378,6 +378,7 @@ static int fit_image_setup_cipher(struct 
> image_cipher_info *info,
> char *algo_name;
> char filename[128];
> int ret = -1;
> +   int snprintf_return;

sret ? We try to use 'ret' for return values. But really I would
prefer if you could split this function in two, since it is too long.
Then you probably can use 'ret' in both functions.

>
> if (fit_image_cipher_get_algo(fit, noffset, &algo_name)) {
> fprintf(stderr, "Can't get algo name for cipher in image 
> '%s'\n",
> @@ -414,8 +415,18 @@ static int fit_image_setup_cipher(struct 
> image_cipher_info *info,
> }
>
> /* Read the key in the file */
> -   snprintf(filename, sizeof(filename), "%s/%s%s",
> -info->keydir, info->keyname, ".bin");
> +   snprintf_return = snprintf(filename, sizeof(filename), "%s/%s%s",
> +  info->keydir, info->keyname, ".bin");
> +   if (snprintf_return >= sizeof(filename)) {
> +   printf("Can't format the key filename when setting up the 
> cipher: insufficient buffer space\n");
> +   ret = -1;
> +   goto out;
> +   }
> +   if (snprintf_return < 0) {
> +   printf("Can't format the key filename when setting up the 
> cipher: snprintf error\n");
> +   ret = -1;
> +   goto out;
> +   }
> info->key = malloc(info->cipher->key_len);
> if (!info->key) {
> fprintf(stderr, "Can't allocate memory for key\n");
> @@ -436,8 +447,18 @@ static int fit_image_setup_cipher(struct 
> image_cipher_info *info,
>
> if (info->ivname) {
> /* Read the IV in the file */
> -   snprintf(filename, sizeof(filename), "%s/%s%s",
> -info->keydir, info->ivname, ".bin");
> +   snprintf_return = snprintf(filename, sizeof(filename), 
> "%s/%s%s",
> +  info->keydir, info->ivname, 
> ".bin");
> +   if (snprintf_return >= sizeof(filename)) {
> +   printf("Can't format the IV filename when setting up 
> the cipher: insufficient buffer space\n");
> +   ret = -1;
> +   goto out;
> +   }
> +   if (snprintf_return < 0) {
> +   printf("Can't format the IV filename when setting up 
> the cipher: snprintf error\n");
> +   ret = -1;
> +   goto out;
> +   }
> ret = fit_image_read_data(filename, (unsigned char *)info->iv,
>   info->cipher->iv_len);
> } else {
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH 2/2] image-host: increase path length when setting up the cipher.

[Simon Glass]

On Fri, Dec 29, 2023 at 2:07 PM Hugo Cornelis
 wrote:
>
> This patch increases the maximum path length of the filename
> containing the cipher key for the kernel from 128 to 256 characters.
>
> Signed-off-by: Hugo Cornelis 
> ---
>
>  tools/image-host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass 

This is fine, but do you not want to use PATH_MAX ? We should not need
to worry about stack space on the host.

>
> diff --git a/tools/image-host.c b/tools/image-host.c
> index 3719f36117..b0012a714d 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -376,7 +376,7 @@ static int fit_image_setup_cipher(struct 
> image_cipher_info *info,
>   int noffset)
>  {
> char *algo_name;
> -   char filename[128];
> +   char filename[256];
> int ret = -1;
> int snprintf_return;
>
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH 1/2] board: ti: am62x: am62x.env: Fix boot_targets

[Simon Glass]

Hi,

On Wed, Nov 29, 2023 at 7:48 PM Simon Glass  wrote:
>
> Hi Andrew,
>
> On Mon, 6 Nov 2023 at 11:05, Andrew Davis  wrote:
> >
> > On 11/6/23 11:47 AM, Simon Glass wrote:
> > > Hi Andrew,
> > >
> > > On Mon, 6 Nov 2023 at 10:27, Andrew Davis  wrote:
> > >>
> > >> On 11/6/23 9:31 AM, Tom Rini wrote:
> > >>> On Mon, Nov 06, 2023 at 11:23:51AM +0530, Manorit Chawdhry wrote:
> >  Hi Simon,
> > 
> >  On 11:22-20231005, Simon Glass wrote:
> > > Hi Nishanth,
> > >
> > > On Thu, 5 Oct 2023 at 11:16, Nishanth Menon  wrote:
> > >>
> > >> On 12:10-20231005, Nishanth Menon wrote:
> > >>> On 12:36-20231005, Tom Rini wrote:
> >  On Thu, Oct 05, 2023 at 09:19:48AM -0500, Andrew Davis wrote:
> > > On 10/4/23 8:54 AM, Nishanth Menon wrote:
> > >> On 08:48-20231004, Andrew Davis wrote:
> > >>> On 10/4/23 8:23 AM, Roger Quadros wrote:
> >  ti_mmc is not a valid boot_target for standard boot flow so
> > >>>
> > >>> Is there some way to make it into a valid boot_target? Otherwise
> > >>> how do we use uEnv.txt files, or boot from FIT images with 
> > >>> overlays?
> > >>
> > >> envboot takes care of uEnv.txt file (see
> > >> https://lore.kernel.org/all/20231004132324.44198-3-rog...@kernel.org/)
> > >>
> > >> Early remote proc loading and FIT image is a question for 
> > >> stdboot itself.
> > >>
> > >
> > > If stdboot is missing these features then we shouldn't switch 
> > > until it
> > > has them. I'm all for switching to this, but only if it is 
> > > complete.
> > 
> >  Depends on what you mean?  Did you mean an option to run scripts
> >  (exists) or an option to do what TI needs done, via
> >  boot/bootmeth_something.c ?  If the latter, someone from TI needs 
> >  to
> >  figure out what that should be and do (but plumbing-wise 
> >  everything it
> >  needs should exist).
> > >>>
> > >>> Andrew is generalizing here (on the wrong patch though).
> > >>>
> > >>> On am62x platforms, there is nothing regressing with this series. 
> > >>> The
> > >>> challenge is early remote_proc loading which is done for J7* 
> > >>> platforms.
> > >>>
> > >>> How that is initiated as part of bootmethods is something of a gap.
> > >>>
> > >>> The other gap has been support for uEnv.txt -> which we can 
> > >>> workaround
> > >>> at the moment by using CONFIG_BOOTCOMMAND="run envboot; bootflow 
> > >>> scan
> > >>> -lb" in defconfig (This series from Roger already does that - hence 
> > >>> I am
> > >>> saying that Andrew is complaining on the wrong series).
> > >>>
> > >>> Ideally, we should just have CONFIG_BOOTCOMMAND="bootflow scan -lb" 
> > >>> and
> > >>> uEnv.txt remoteproc loads and the various standard bootmethods 
> > >>> should
> > >>> "just work".
> > >>
> > >>
> > >> I forgot to add: FIT image authenticated boot flow. That is really 
> > >> what
> > >> ti_mmc distroboot method was trying to solve.
> > >>
> > >> Maybe Simon or someone know how the stdboot flow handles 
> > >> authenticated
> > >> kernel image and dtb boot flow with FIT image?
> > >
> > > Yes you can use FIT configuration verification and things should work 
> > > as normal.
> > >
> > 
> >  Could you give any reference documentation for this?
> > >>>
> > >>> I suspect you should start with doc/usage/fit/beaglebone_vboot.rst
> > >>>
> > >>
> > >>   From that doc:
> > >>
> > >> ```
> > >> Boot the board using the commands below::
> > >>
> > >>   setenv bootargs console=ttyO0,115200n8 quiet root=/dev/mmcblk0p2 
> > >> ro rootfstype=ext4 rootwait
> > >>   ext2load mmc 0:2 8200 /boot/image.fit
> > >>   bootm 8200
> > >> ```
> > >>
> > >> This is very much not stdboot :( This doc has some good and current info 
> > >> on building
> > >> a secure FIT image, but much of it is showing its age. Stdboot is still 
> > >> missing
> > >>
> > >>* ability to limit boot mode search to only one image (FIT)
> > >
> > > What does this mean? Can you please be more specific or give an example?
> > >
> >
> > Sure, for instance with secure boot we only want to search for FIT images
> > and if for each media this fails we do not want to fall back to other
> > image types or boot modes (like UART boot or net boot which would bypass
> > the signature checks).
>
> We could have something like:
>
> bootstd {
>image-types = "fit", "uimage";
> }
>
> to limit the supported types.
>
> >
> > Something similar is needed for searching for EFI compatible boot across
> > each enabled boot media first, before trying other methods on each media.
> > Basically breadth-first search on each media type not depth-first.
>
> How about:
>
> bootstd {
>scan-orde

Re: Proposal: U-Boot memory management

[Simon Glass]

Hi,

On Fri, Dec 29, 2023 at 10:52 AM Tom Rini  wrote:
>
> On Fri, Dec 29, 2023 at 06:44:15PM +0100, Mark Kettenis wrote:
> > > Date: Fri, 29 Dec 2023 11:17:44 -0500
> > > From: Tom Rini 
> > >
> > > On Fri, Dec 29, 2023 at 05:05:17PM +0100, Heinrich Schuchardt wrote:
> > > >
> > > >
> > > > Am 29. Dezember 2023 16:43:07 MEZ schrieb Tom Rini :
> > > > >On Fri, Dec 29, 2023 at 05:36:09AM +, Simon Glass wrote:
> > > > >> Hi,
> > > > >>
> > > > >> On Sat, Dec 16, 2023 at 6:01 PM Simon Glass  
> > > > >> wrote:
> > > > >> >
> > > > >> > Hi,
> > > > >> >
> > > > >> > This records my thoughts after a discussion with Ilias & Heinrich 
> > > > >> > re
> > > > >> > memory allocation in U-Boot.
> > > > >> >
> > > > >> > 1. malloc()
> > > > >> >
> > > > >> > malloc() is used for programmatic memory allocation. It allows 
> > > > >> > memory
> > > > >> > to be freed. It is not designed for very large allocations (e.g. a
> > > > >> > 10MB kernel or 100MB ramdisk).
> > > > >> >
> > > > >> > 2. lmb
> > > > >> >
> > > > >> > lmb is used for large blocks of memory, such as those needed for a
> > > > >> > kernel or ramdisk. Allocation is only transitory, for the purposes 
> > > > >> > of
> > > > >> > loading some images and booting. If the boot fails, then all lmb
> > > > >> > allocations go away.
> > > > >> >
> > > > >> > lmb is set up by getting all available memory and then removing 
> > > > >> > what
> > > > >> > is used by U-Boot (code, data, malloc() space, etc.)
> > > > >> >
> > > > >> > lmb reservations have a few flags so that areas of memory can be
> > > > >> > provided with attributes
> > > > >> >
> > > > >> > There are some corner cases...e.g. loading a file does an lmb
> > > > >> > allocation but only for the purpose of avoiding a file being loaded
> > > > >> > over U-Boot code/data. The allocation is dropped immediately after 
> > > > >> > the
> > > > >> > file is loaded. Within the bootm command, or when using standard 
> > > > >> > boot,
> > > > >> > this would be fairly easy to solve.
> > > > >> >
> > > > >> > Linux has renamed lmb to memblock. We should consider doing the 
> > > > >> > same.
> > > > >> >
> > > > >> > 3. EFI
> > > > >> >
> > > > >> > EFI has its own memory-allocation tables.
> > > > >> >
> > > > >> > Like lmb, EFI is able to deal with large allocations. But via a 
> > > > >> > 'pool'
> > > > >> > function it can also do smaller allocations similar to malloc(),
> > > > >> > although each one uses at least 4KB at present.
> > > > >> >
> > > > >> > EFI allocations do not go away when a boot fails.
> > > > >> >
> > > > >> > With EFI it is possible to add allocations post facto, in which 
> > > > >> > case
> > > > >> > they are added to the allocation table just as if the memory was
> > > > >> > allocated with EFI to begin with.
> > > > >> >
> > > > >> > The EFI allocations and the lmb allocations use the same memory, 
> > > > >> > so in
> > > > >> > principle could conflict.
> > > > >> >
> > > > >> > EFI allocations are sometimes used to allocate internal U-Boot 
> > > > >> > data as
> > > > >> > well, if needed by the EFI app. For example, while 
> > > > >> > efi_image_parse()
> > > > >> > uses malloc(), efi_var_mem.c uses EFI allocations since the code 
> > > > >> > runs
> > > > >> > in the app context and may need to access the memory after U-Boot 
> > > > >> > has
> > > > >> > exited. Also efi_smbios.c uses allocate_pages() and then adds a new
> > > > >> > mapping as well.
> > > > >> >
> > > > >> > EFI memory has attributes, including what the memory is used for 
> > > > >> > (to
> > > > >> > some degree of granularity). See enum efi_memory_type and struct
> > > > >> > efi_mem_desc. In the latter there are also attribute flags - 
> > > > >> > whether
> > > > >> > memory is cacheable, etc.
> > > > >> >
> > > > >> > EFI also has the x86 idea of 'conventional' memory, meaning (I
> > > > >> > believe) that below 4GB that isn't reserved for the 
> > > > >> > hardware/system.
> > > > >> > This is meaningless, or at least confusing, on ARM systems.
> > > > >> >
> > > > >> > 4. reservations
> > > > >> >
> > > > >> > It is perhaps worth mentioning a fourth method of memory 
> > > > >> > management,
> > > > >> > where U-Boot reserves chunks of memory before relocation (in
> > > > >> > board_init_f.c), e.g. for the framebuffer, U-Boot code, the 
> > > > >> > malloc()
> > > > >> > region, etc.
> > > > >> >
> > > > >> >
> > > > >> > Problems
> > > > >> > —---
> > > > >> >
> > > > >> > There are no urgent problems, but here are some things that could 
> > > > >> > be improved:
> > > > >> >
> > > > >> > 1. EFI should attach most of its data structures to driver model. 
> > > > >> > This
> > > > >> > work has started, with the partition support, but more effort would
> > > > >> > help. This would make it easier to see which memory is related to
> > > > >> > devices and which is separate.
> > > > >> >
> > > > >> > 2. Some drivers do EFI reservations today, whether EFI is used for
> > > > >> > bootin

Re: [PATCH v3 7/8] dts: meson-gxbb: Switch to using upstream DT

[Simon Glass]

Hi Tom,

On Thu, Dec 28, 2023 at 1:32 PM Tom Rini  wrote:
>
> On Thu, Dec 28, 2023 at 07:48:13PM +, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, Dec 28, 2023 at 3:25 PM Tom Rini  wrote:
> > >
> > > On Thu, Dec 28, 2023 at 03:09:41PM +, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, Dec 28, 2023 at 1:56 PM Tom Rini  wrote:
> > > > >
> > > > > On Thu, Dec 28, 2023 at 01:37:11PM +, Simon Glass wrote:
> > > > > > Hi Sumit,
> > > > > >
> > > > > > On Thu, Dec 28, 2023 at 11:59 AM Sumit Garg  
> > > > > > wrote:
> > > > > > >
> > > > > > > Although there were still some variations in board DTS files 
> > > > > > > based on
> > > > > > > meson-gxbb SoC but I think those were minor differences from 
> > > > > > > upstream
> > > > > > > and shouldn't impact boot on these devices.
> > > > > > >
> > > > > > > So switch to upstream DT via mirroring amlogic/ directory from
> > > > > > > devicetree-rebasing/src/arm64/amlogic/ directory. And thereby 
> > > > > > > directly
> > > > > > > building DTB from there including *-u-boot.dtsi files from
> > > > > > > arch/$(ARCH)/dts/ directory.
> > > > > > >
> > > > > > > Reviewed-by: Neil Armstrong 
> > > > > > > Signed-off-by: Sumit Garg 
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v3:
> > > > > > > --
> > > > > > > - Dropped Makefile portion and enabled OF_UPSTREAM for SoC 
> > > > > > > instead.
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > > --
> > > > > > > - Picked up review tag
> > > > > > >
> > > > > > >  arch/arm/mach-meson/Kconfig   | 1 +
> > > > > > >  configs/nanopi-k2_defconfig   | 2 +-
> > > > > > >  configs/odroid-c2_defconfig   | 2 +-
> > > > > > >  configs/p200_defconfig| 2 +-
> > > > > > >  configs/p201_defconfig| 2 +-
> > > > > > >  configs/videostrong-kii-pro_defconfig | 2 +-
> > > > > > >  configs/wetek-hub_defconfig   | 2 +-
> > > > > > >  configs/wetek-play2_defconfig | 2 +-
> > > > > > >  dts/arch/arm64/amlogic| 1 +
> > > > > > >  9 files changed, 9 insertions(+), 7 deletions(-)
> > > > > > >  create mode 12 dts/arch/arm64/amlogic
> > > > > > >
> > > > > >
> > > > > > Reviewed-by: Simon Glass 
> > > > > >
> > > > > > > diff --git a/arch/arm/mach-meson/Kconfig 
> > > > > > > b/arch/arm/mach-meson/Kconfig
> > > > > > > index d6c89058061..8ddb59161a0 100644
> > > > > > > --- a/arch/arm/mach-meson/Kconfig
> > > > > > > +++ b/arch/arm/mach-meson/Kconfig
> > > > > > > @@ -25,6 +25,7 @@ choice
> > > > > > >  config MESON_GXBB
> > > > > > > bool "GXBB"
> > > > > > > select MESON_GX
> > > > > > > +   imply OF_UPSTREAM
> > > > > > > help
> > > > > > >   Select this if your SoC is an S905
> > > > > > >
> > > > > > > diff --git a/configs/nanopi-k2_defconfig 
> > > > > > > b/configs/nanopi-k2_defconfig
> > > > > > > index 41dbf7981f8..2e1c756bf7a 100644
> > > > > > > --- a/configs/nanopi-k2_defconfig
> > > > > > > +++ b/configs/nanopi-k2_defconfig
> > > > > > > @@ -6,7 +6,7 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> > > > > > >  CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x2000
> > > > > > >  CONFIG_ENV_SIZE=0x2000
> > > > > > >  CONFIG_DM_GPIO=y
> > > > > > > -CONFIG_DEFAULT_DEVICE_TREE="meson-gxbb-nanopi-k2"
> > > > > > > +CONFIG_DEFAULT_DEVICE_TREE="amlogic/meson-gxbb-nanopi-k2"
> > > > > > >  CONFIG_OF_LIBFDT_OVERLAY=y
> > > > > > >  CONFIG_DM_RESET=y
> > > > > > >  CONFIG_DEBUG_UART_BASE=0xc81004c0
> > > > > >
> > > > > > So now I am wondering if we can (later) have the SoC be implied when
> > > > > > using OF_UPSTREAM so we can do:
> > > > > >
> > > > > > CONFIG_DEFAULT_DEVICE_TREE="meson-gxbb-nanopi-k2"
> > > > > >
> > > > > > and it will add the 'amlogic/' automatically?
> > > > > >
> > > > > > Perhaps that might be a way to bridge the gap in terms of file 
> > > > > > layout?
> > > > >
> > > > > That's going to start to look real ugly real quick I suspect and I'm 
> > > > > not
> > > > > sure what it really gets us? It seems to me that the convention of
> > > > > "vendor/board" has already been well enough adapted outside of the
> > > > > kernel, with the odd sticking point being getting everyone used to
> > > > > whatever will be happening for 32bit boards long term. So I think 
> > > > > hiding
> > > > > the vendor part of the string here will be more, not less, confusing
> > > > > long term.
> > > >
> > > > Now that I dig in a bit, I see that the devicetree-rebasing tree does
> > > > not have any Makefiles in it. How does Linux build with it? Does
> > > > anyone have instructions?
> > >
> > > Linux doesn't build with it, it's the relevant parts of the kernel tree
> > > extracted for other projects to more easily use (more or less).
> >
> > Hmmm, in that case can we add Makefiles into the U-Boot version of the
> > tree, for U-Boot use?
>
> No? Why would we do that? I think you're missing how the amlogic
> conversion works.

I will reply on the other thread about this.

Re: [PATCH v6] fdt: Allow the devicetree to come from a bloblist

[Simon Glass]

Hi,

On Thu, Dec 28, 2023 at 2:24 PM Tom Rini  wrote:
>
> On Thu, Dec 28, 2023 at 07:47:25PM +, Simon Glass wrote:
>
> > Standard passage provides for a bloblist to be passed from one firmware
> > phase to the next. That can be used to pass the devicetree along as well.
> > Add an option to support this.
> >
> > Tests for this will be added as part of the Universal Payload work.
> >
> > Signed-off-by: Simon Glass 
> > ---
> [snip]
> > diff --git a/dts/Kconfig b/dts/Kconfig
> > index 00c0aeff893..ae451b9caf7 100644
> > --- a/dts/Kconfig
> > +++ b/dts/Kconfig
> > @@ -105,6 +105,19 @@ config OF_EMBED
> >
> >  endchoice
> >
> > +config OF_BLOBLIST
> > + bool "Provided by a bloblist at runtime"
> > + depends on BLOBLIST && OF_SEPARATE
>
> This is now even more confusing, frankly. The help for OF_SEPARATE says:
> If this option is enabled, the device tree will be built and placed as a
> separate u-boot.dtb file alongside the U-Boot image.
>
> So why would you enable that to then have a device tree passed via
> bloblist instead?

Historical

>
> We should probably start by fixing all of this confusing naming / logic
> and then correct things such that:
> - OF_EMBED wins if set. This is the override-has-been-set we-must-use-it
>   switch. First choice, not last choice. If binman needs tweaks so that
>   it will still generate images for platforms in this case, that needs
>   to happen.

Perhaps we should rename this to OF_EMBED_DEBUG ? Really it should not
be used by any board.

> - If we have a bloblist, we scan the bloblist for DT and if found, use
>   it.

unless we don't want to, e.g. because that DT has a bug or we want to
use a different one during development.

> - If it looks like we've been booted as a fake Linux kernel, and we can
>   start with just aarch64 and let riscv come in as a follow up, so
>   what's documented within
>   
> https://www.kernel.org/doc/html/latest/arch/arm64/booting.html#call-the-kernel-image
>   then we use that device tree.

Eek, really? Is this the rpi case and you are trying to make it
generic? I would rather that be a board-specific thing, calling into a
generic implementation. We should encourage bloblist.

>   - This _may_ just end up having to be "Does x0 (or similar) point to a
> valid DT?" as I don't know how correct everything using this method
> today is too what the spec above lists.

OK, well a generic impl would be good I suppose, but dereferencing
pointers to look for a magic number might not end well.

> - If we have a dtb appended to use by what we call today OF_SEPARATE but
>   should really stop calling it that we use that.

Yes it is a complete misnomer now. I will try to think of a better
name. It really just controls generation of u-boot.dtb and what goes
in u-boot.bin so perhaps we can just drop it and have OF_EMBED be
false in the normal case. Or rename to OF_STANDARD ?

>
> At that point, we can probably have zero "totally board specific kludge
> for device tree location", and kill off OF_HAS_PRIOR_STAGE too (since
> that's really just bloblist or fake-linux-kernel). We'll also be able to
> support migration from fake-linux-kernel to bloblist

OF_HAS_PRIOR_STAGE controls things like building the DT and OF_BOARD
... which from what you say above can perhaps go away. But there are
quite a lot of board_fdt_blob_setup() functions...it doesn't look like
they are all the same.

Automatic is OK I suppose. I just want to make sure that these things
can be disabled easily so it is possible to use the DT built by U-Boot
without hacking the code. That is my goal with having a Kconfig to
enable this mechanisms.

Regards,
Simon

Re: ACPI: usage of sandbox virtual addresses

[Simon Glass]

Hi Heinrich,

On Fri, Dec 29, 2023 at 8:58 AM Heinrich Schuchardt  wrote:
>
>
>
> Am 29. Dezember 2023 15:31:24 MEZ schrieb Simon Glass :
> >Hi Heinrich,
> >
> >On Fri, Dec 29, 2023 at 11:14 AM Heinrich Schuchardt  
> >wrote:
> >>
> >> On 12/29/23 09:26, Simon Glass wrote:
> >> > Hi Heinrich,
> >> >
> >> > On Tue, Dec 26, 2023 at 10:03 AM Heinrich Schuchardt 
> >> >  wrote:
> >> >>
> >> >> On 12/26/23 10:50, Simon Glass wrote:
> >> >>> Hi Heinrich,
> >> >>>
> >> >>> On Tue, Dec 26, 2023 at 8:56 AM Heinrich Schuchardt 
> >> >>>  wrote:
> >> 
> >>  Hello Simon,
> >> 
> >>  currently we use sandbox virtual addresses in all ACPI tables. This
> >>  means that an application started by the U-Boot sandbox consuming 
> >>  these
> >>  tables will crash due to accessing invalid addresses.
> >> 
> >>  Shouldn't the ACPI tables be migrated to use valid pointers?
> >> >>>
> >> >>> The confusion arises due to the difference between addresses and
> >> >>> pointers. If we store addresses in the ACPI tables, then things should
> >> >>> work.
> >> >>>
> >> >>> My approach has been to avoid using casts, but instead use
> >> >>> map_sysmem() and mem_to_sysmem().
> >> >>>
> >> >>> Which particular piece of code is wrong in this case?
> >> >>
> >> >> Tables DSDT, XSDT, RSDP, FADT in the sandbox contain sandbox virtual
> >> >> addresses instead of pointers to real host memory.
> >> >>
> >> >> All code referring to these tables should be changed.433:   
> >> >>  *gnvs = map_to_sysmem(ctx->current);
> >> >
> >> > I'm still not quite clear on this...can you point to functions or
> >> > lines of code? When I look at acpi_add_table() it does use memmap, but
> >> > perhaps some parts are wrong?
> >> >
> >> > Regards,
> >> > Simon
> >>
> >> Here are some examples where the wrong values are set. We must get rid
> >> of all map_to_sysmem() calls where writing ACPI tables.
> >>
> >> lib/acpi/acpi_table.c:
> >> 170 rsdt->entry[i] = map_to_sysmem(table);
> >> 188 xsdt->entry[i] = map_to_sysmem(table);
> >>
> >> lib/acpi/base.c:
> >> 27: rsdp->rsdt_address = map_to_sysmem(rsdt);
> >> 30: rsdp->xsdt_address = map_to_sysmem(xsdt);
> >>
> >> arch/x86/cpu/baytrail/acpi.c:
> >> 81: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs);
> >> 82: fadt->x_dsdt = map_to_sysmem(ctx->dsdt);
> >>
> >> arch/x86/cpu/quark/acpi.c:
> >> 76: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs);
> >> 77: fadt->x_dsdt = map_to_sysmem(ctx->dsdt);
> >>
> >> arch/x86/cpu/tangier/acpi.c:
> >> 47: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs);
> >> 48: fadt->x_dsdt = map_to_sysmem(ctx->dsdt);
> >>
> >> arch/x86/lib/acpi_table.c:
> >> 200:tcpa->lasa = map_to_sysmem(log);
> >> 271:tpm2->lasa = map_to_sysmem(lasa);
> >> 433:*gnvs = map_to_sysmem(ctx->current);
> >> 575:fadt->x_firmware_ctrl = map_to_sysmem(facs);
> >> 576:fadt->x_dsdt = map_to_sysmem(dsdt);
> >
> >OK, I see. But at least within sandbox, the address is what we want to
> >store, not the pointer. Are you worried about what an EFI app will do
> >in that case, when we run an app from sandbox? If so, then yes it is
> >definitely a problem.
> >
> >Regards,
> >Simon
>
> The sandbox is an environment to run EFI binaries inside an OS. So the ACPI 
> tables must contain pointers.
>
> The acpi command should display sandbox virtual addresses. All conversions 
> should be moved to the command.

OK, it makes sense to me. For sandbox we want addresses, but in this
case (with an external program using the addresses) we are going to
have to use pointers cast to addresses.

Regards,
Simon

Re: [PATCH v7 1/9] dtoc: Change dst to self in debug message

[Simon Glass]

On Fri, Dec 29, 2023 at 10:46 AM Manorit Chawdhry  wrote:
>
> Fix the error message to not use dst and use self as it is copying the
> properties to self.
>
> While using templating if there are no subnodes defined, we end up in
> this situation where "dst" isn't defined and it tries to print the error
> message and fails.
>
> 'UnboundLocalError: local variable 'dst' referenced before assignment'
>
> Fixes: 55e1278d5eca ("dtoc: Allow inserting a list of nodes into another")
>
> Signed-off-by: Manorit Chawdhry 
> ---
>  tools/dtoc/fdt.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Simon Glass 

Re: [PATCH 0/9] dts: Move to SoC-specific build rules

[Simon Glass]

-Scott as it is bouncing

Hi,

On Fri, Dec 29, 2023 at 9:46 AM Peter Robinson  wrote:
>
> On Fri, Dec 29, 2023 at 12:23 AM Tom Rini  wrote:
> >
> > On Thu, Dec 28, 2023 at 07:48:08PM +, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, Dec 28, 2023 at 3:40 PM Tom Rini  wrote:
> > > >
> > > > On Thu, Dec 28, 2023 at 03:09:40PM +, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, Dec 28, 2023 at 2:23 PM Tom Rini  wrote:
> > > > > >
> > > > > > On Thu, Dec 28, 2023 at 01:37:07PM +, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Wed, Dec 27, 2023 at 1:21 PM Tom Rini  
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Wed, Dec 27, 2023 at 08:23:56AM +, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > U-Boot builds devicetree binaries from its source tree. As 
> > > > > > > > > part of the
> > > > > > > > > Kconfig conversion, the Makefiles were updated to align with 
> > > > > > > > > how this
> > > > > > > > > is done in Linux: a single target for each SoC is used to 
> > > > > > > > > build all the
> > > > > > > > > .dtb files for that SoC.
> > > > > > > > >
> > > > > > > > > Since then, the Makefiles have devolved in some cases, 
> > > > > > > > > resulting in
> > > > > > > > > lots of target-specific build rules. Also Linux has moved to 
> > > > > > > > > using
> > > > > > > > > subdirectories for each vendor.
> > > > > > > > >
> > > > > > > > > Recent work aims to allow U-Boot to directly use devicetree 
> > > > > > > > > files from
> > > > > > > > > Linux. This would be easier if the directory structure were 
> > > > > > > > > the same.
> > > > > > > > > Another recent discussion involved dropping the build rules 
> > > > > > > > > altogether.
> > > > > > > > >
> > > > > > > > > This series makes a start at cleaning up some of the build 
> > > > > > > > > rules, to
> > > > > > > > > reduce the amount of code and make it easier to add new 
> > > > > > > > > boards for the
> > > > > > > > > same SoC.
> > > > > > > > >
> > > > > > > > > One issue is that the ARCH_xxx Kconfig options between U-Boot 
> > > > > > > > > and Linux
> > > > > > > > > are not always the same. Given the large number of SoCs and 
> > > > > > > > > boards
> > > > > > > > > supported by U-Boot, it would be useful to align these where 
> > > > > > > > > possible.
> > > > > > > >
> > > > > > > > I don't know why we should start with this now, and further not 
> > > > > > > > being on
> > > > > > > > top of Sumit's series to remove our duplicate dts files. And 
> > > > > > > > that's
> > > > > > > > where we can have the conversation about for which cases it 
> > > > > > > > even makes
> > > > > > > > sense to build all of the dts files for a given SoC.
> > > > > > >
> > > > > > > This is a completely different series - there are no conflicts 
> > > > > > > with
> > > > > > > Sumit's series so it can be applied before or after it.
> > > > > > >
> > > > > > > My goal here is to clean up our build rules, rather than just 
> > > > > > > throwing
> > > > > > > them all away, for reasons we have discussed previously. I filed 
> > > > > > > [1]
> > > > > > > to track that.
> > > > > >
> > > > > > Yes, I'm saying we shouldn't be doing anything this series does 
> > > > > > until
> > > > > > after Sumit's series has landed. Along with the fact that I don't 
> > > > > > like
> > > > > > what's going on in this series.
> > > > >
> > > > > This seems to again be the disagreement over whether a single DT
> > > > > should be build for each board, or all the DTs for an SoC?
> > > >
> > > > It's about the disagreement on what we should be building, and what that
> > > > infrastructure looks like. I do not like adding extra rules for
> > > > "clarity" because they don't make things clearer, they lead to the
> > > > unclear mess we have today getting worse. Most instructions are _not_
> > > > "now take this dtb and this binary and .." but just "take this binary",
> > > > because we already have the rules and logic to ensure we build the
> > > > required dtbs. I also don't like the parts of this series that amount
> > > > to deck shuffling when we should just be taking the chairs away. There's
> > > > just not nor will there be a case for omap3/4/5 platforms of "change the
> > > > dtb", so building more is pointless. For other SoCs, there may be some
> > > > cases of "this could have been just a DT change", like
> > > > rock5b-rk3588_defconfig / rock5a-rk3588s_defconfig could share a board
> > > > dir, but the configs are different and the dts are different, so I don't
> > > > know that you could really just swap the dtbs.
> > >
> > > It is true that we have a different defconfig for each board, but IMO
> > > that is not good. It is an artifact of the way the build system works.
> > > IMO Kconfig should be used to define sensible defaults so that the
> > > defconfigs are nearly empty. Perhaps config fragments can be part of
> > > the mix, if we can agree on [1].
> > >
> > > But if we let t

Re: [PATCH 00/34] x86: expo: Add support for editing coreboot CMOS RAM settings

[Simon Glass]

Hi Bin,

On Sun, Dec 31, 2023 at 2:59 AM Bin Meng  wrote:
>
> Hi Simon,
>
> On Wed, Nov 15, 2023 at 9:24 PM Simon Glass  wrote:
> >
> > Hi Bin,
> >
> > On Sun, 1 Oct 2023 at 19:16, Simon Glass  wrote:
> > >
> > > U-Boot provides support for editing settings with an 'expo', as well as
> > > reading and writing settings to CMOS RAM.
> > >
> > > This series integrates expo functionality with coreboot, using the
> > > sysinfo table to get a list of settings, creating an expo with them and
> > > allowing them to be edited.
> > >
> > > A new CI test provides coverage for some coreboot-related commands. For
> > > this to work, a number of minor tweaks are made to existing tests, so
> > > that they pass on coreboot (x86) or at least are skipped when they
> > > cannot pass. Likely most of these fixes will apply to other boards too.
> > >
> > > It includes several other fixes and improvements:
> > > - new -m flag for 'bootflow scan' so it can display a menu automatically
> > > - Fix for video-scrolling crash with Truetype
> > > - menu items can now have individual integer values
> > > - menu items are lined up according to the width of all menu labels
> > >
> > >
> > > Simon Glass (34):
> > >   test: Run tests that don't need devices
> > >   test: Add a new suite for commands
> > >   test: Add helper to skip to partial console line
> > >   test: Make UT_LIB_ASN1 depend on sandbox
> > >   test: Run bootstd tests only on sandbox
> > >   test: Handle use of stack pointer in bdinfo
> > >   test: bdinfo: Add missing asserts
> > >   test: fdt: Add a special case for real boards
> > >   test: font: Add dependencies on fonts
> > >   test: event: Only run test_event_probe() on sandbox
> > >   test: lmb: Move tests into the lib suite
> > >   test: print: Skip test on x86
> > >   video: Add a function to clear the display
> > >   sandbox: Add a dummy booti command
> > >   bootstd: Add a menu option to bootflow scan
> > >   video: Add a dark-grey console colour
> > >   video: Avoid starting a new line to close to the bottom
> > >   expo: Place menu items to the right of all labels
> > >   expo: Set the initial next_id to 1
> > >   expo: Use standard numbering for save and discard
> > >   expo: Allow menu items to have values
> > >   expo: Add a little more cedit CMOS logging
> > >   expo: Support menu-item values in cedit
> > >   expo: Drop unneceesary calls to expo_str()
> > >   expo: Drop scene_title_set()
> > >   expo: Add forward declaration for udevice to cedit
> > >   x86: coreboot: Enable unit tests
> > >   x86: CI: Update coreboot
> > >   x86: coreboot: Add a test for cbsysinfo command
> > >   x86: coreboot: Show the option table
> > >   x86: coreboot: Enable support for the configuration editor
> > >   x86: coreboot: Add a command to check and update CMOS RAM
> > >   x86: coreboot: Allow building an expo for editing CMOS config
> > >   x86: Enable RTC command by default
> > >
> > >  .azure-pipelines.yml  |   2 +-
> > >  .gitlab-ci.yml|   2 +-
> > >  arch/sandbox/dts/cedit.dtsi   |   3 +
> > >  arch/sandbox/lib/bootm.c  |   7 +
> > >  arch/x86/dts/coreboot.dts |   7 +
> > >  boot/Makefile |   4 +
> > >  boot/cedit.c  | 191 +++
> > >  boot/expo.c   |   3 +
> > >  boot/expo_build.c |  36 ++---
> > >  boot/expo_build_cb.c  | 244 ++
> > >  boot/scene.c  |  61 ++--
> > >  boot/scene_internal.h |  30 +++-
> > >  boot/scene_menu.c |  26 +++-
> > >  boot/scene_textline.c |   3 +-
> > >  cmd/Kconfig   |  14 +-
> > >  cmd/bootflow.c|  27 +++-
> > >  cmd/booti.c   |   2 +-
> > >  cmd/cedit.c   |  28 
> > >  cmd/cls.c |  25 +--
> > >  cmd/x86/Makefile  |   1 +
> > >  cmd/x86/cbcmos.c  | 141 +
> > >  cmd/x86/cbsysinfo.c   |  73 -
> > >  common/console.c  |  31 
> > >  configs/coreboot64_defconfig  |   2 +
> > >  configs/coreboot_defconfig|   6 +
> > >  configs/tools-only_defconfig  |   3 +-
> > >  doc/board/coreboot/coreboot.rst   |   6 +
> > >  doc/develop/cedit.rst |   9 +-
> > >  doc/develop/expo.rst  |  26 +++-
> > >  doc/usage/cmd/bootflow.rst|   5 +
> > >  doc/usage/cmd/cbcmos.rst  |  45 ++
> > >  doc/usage/cmd/cbsysinfo.rst   |  99 
> > >  doc/usage/cmd/cedit.rst   |  91 ++-
> > >  doc/usage/index.rst   |   1 +
> > >  drivers/video/vidconsole-uclass.c |   4 +-
> > >  drivers/video/video-uclass.c  |   3 +
> > >  include/cedit.h   |   1 +
> > >  include/console.h |  10 ++
> > >  include/expo.h|  51 +--
> > >  include/test/cedit

Re: [PATCH 00/34] x86: expo: Add support for editing coreboot CMOS RAM settings

[Bin Meng]

Hi Simon,

On Wed, Nov 15, 2023 at 9:24 PM Simon Glass  wrote:
>
> Hi Bin,
>
> On Sun, 1 Oct 2023 at 19:16, Simon Glass  wrote:
> >
> > U-Boot provides support for editing settings with an 'expo', as well as
> > reading and writing settings to CMOS RAM.
> >
> > This series integrates expo functionality with coreboot, using the
> > sysinfo table to get a list of settings, creating an expo with them and
> > allowing them to be edited.
> >
> > A new CI test provides coverage for some coreboot-related commands. For
> > this to work, a number of minor tweaks are made to existing tests, so
> > that they pass on coreboot (x86) or at least are skipped when they
> > cannot pass. Likely most of these fixes will apply to other boards too.
> >
> > It includes several other fixes and improvements:
> > - new -m flag for 'bootflow scan' so it can display a menu automatically
> > - Fix for video-scrolling crash with Truetype
> > - menu items can now have individual integer values
> > - menu items are lined up according to the width of all menu labels
> >
> >
> > Simon Glass (34):
> >   test: Run tests that don't need devices
> >   test: Add a new suite for commands
> >   test: Add helper to skip to partial console line
> >   test: Make UT_LIB_ASN1 depend on sandbox
> >   test: Run bootstd tests only on sandbox
> >   test: Handle use of stack pointer in bdinfo
> >   test: bdinfo: Add missing asserts
> >   test: fdt: Add a special case for real boards
> >   test: font: Add dependencies on fonts
> >   test: event: Only run test_event_probe() on sandbox
> >   test: lmb: Move tests into the lib suite
> >   test: print: Skip test on x86
> >   video: Add a function to clear the display
> >   sandbox: Add a dummy booti command
> >   bootstd: Add a menu option to bootflow scan
> >   video: Add a dark-grey console colour
> >   video: Avoid starting a new line to close to the bottom
> >   expo: Place menu items to the right of all labels
> >   expo: Set the initial next_id to 1
> >   expo: Use standard numbering for save and discard
> >   expo: Allow menu items to have values
> >   expo: Add a little more cedit CMOS logging
> >   expo: Support menu-item values in cedit
> >   expo: Drop unneceesary calls to expo_str()
> >   expo: Drop scene_title_set()
> >   expo: Add forward declaration for udevice to cedit
> >   x86: coreboot: Enable unit tests
> >   x86: CI: Update coreboot
> >   x86: coreboot: Add a test for cbsysinfo command
> >   x86: coreboot: Show the option table
> >   x86: coreboot: Enable support for the configuration editor
> >   x86: coreboot: Add a command to check and update CMOS RAM
> >   x86: coreboot: Allow building an expo for editing CMOS config
> >   x86: Enable RTC command by default
> >
> >  .azure-pipelines.yml  |   2 +-
> >  .gitlab-ci.yml|   2 +-
> >  arch/sandbox/dts/cedit.dtsi   |   3 +
> >  arch/sandbox/lib/bootm.c  |   7 +
> >  arch/x86/dts/coreboot.dts |   7 +
> >  boot/Makefile |   4 +
> >  boot/cedit.c  | 191 +++
> >  boot/expo.c   |   3 +
> >  boot/expo_build.c |  36 ++---
> >  boot/expo_build_cb.c  | 244 ++
> >  boot/scene.c  |  61 ++--
> >  boot/scene_internal.h |  30 +++-
> >  boot/scene_menu.c |  26 +++-
> >  boot/scene_textline.c |   3 +-
> >  cmd/Kconfig   |  14 +-
> >  cmd/bootflow.c|  27 +++-
> >  cmd/booti.c   |   2 +-
> >  cmd/cedit.c   |  28 
> >  cmd/cls.c |  25 +--
> >  cmd/x86/Makefile  |   1 +
> >  cmd/x86/cbcmos.c  | 141 +
> >  cmd/x86/cbsysinfo.c   |  73 -
> >  common/console.c  |  31 
> >  configs/coreboot64_defconfig  |   2 +
> >  configs/coreboot_defconfig|   6 +
> >  configs/tools-only_defconfig  |   3 +-
> >  doc/board/coreboot/coreboot.rst   |   6 +
> >  doc/develop/cedit.rst |   9 +-
> >  doc/develop/expo.rst  |  26 +++-
> >  doc/usage/cmd/bootflow.rst|   5 +
> >  doc/usage/cmd/cbcmos.rst  |  45 ++
> >  doc/usage/cmd/cbsysinfo.rst   |  99 
> >  doc/usage/cmd/cedit.rst   |  91 ++-
> >  doc/usage/index.rst   |   1 +
> >  drivers/video/vidconsole-uclass.c |   4 +-
> >  drivers/video/video-uclass.c  |   3 +
> >  include/cedit.h   |   1 +
> >  include/console.h |  10 ++
> >  include/expo.h|  51 +--
> >  include/test/cedit-test.h |  30 ++--
> >  include/test/cmd.h|  15 ++
> >  include/test/suites.h |   1 +
> >  include/test/ut.h |  30 
> >  include/video.h   |   4 +-
> >  include/video_console.h   |   8