[PATCH v5 00/46] x86: Improve operation under QEMU

[PATCH v5 00/46] x86: Improve operation under QEMU

[Simon Glass]

U-Boot can start and boot an OS in both qemu-x86 and qemu-x86_64 but it
is not perfect.

With both builds, executing the VESA ROM causes an intermittent hang, at
least on some AMD CPUs.

With qemu-x86_64 kvm cannot be used since the move to long mode (64-bit)
is done in a way that works on real hardware but not with QEMU. This
means that performance is 4-5x slower than it could be, at least on my
CPU.

We can work around the first problem by using Bochs, which is anyway a
better choice than VESA for QEMU. The second can be addressed by using
the same descriptor across the jump to long mode.

With an MTRR fix this allows booting into Ubuntu on qemu-x86_64

In v3 some e820 patches are included to make booting reliable and avoid
ACPI tables being dropped. Also, several MTTR problems are addressed, to
support memory sizes above 4GB reliably.

Changes in v5:
- Drop timeouts patch
- Use Ctrl-N instead of down-arrow to make the test more reliable
- Add more logging into the test

Changes in v4:
- Rewrite the commit message
- Enable the flag by default
- Split out into separate patch
- Split out the command change into a separate patch

Changes in v3:
- Always return true from flag_is_changeable_p() on amd64
- Add new patch with functions to convert between mtrr size and mask
- Add new patch to update mtrr command to support 64-bit values
- Add new patch to update cpuid_eax et al to work on amd64
- Add new patch to correct msr operation on amd64
- Add new patch to allow adding non-aligned size for MTRR
- Support memory sizes above 3GB properly
- Add new patch to correct sizing of created disks
- Add new patch with a function to dump the e820 table
- Add new patch with an API for e820
- Add new patch to use the new e820 API
- Add new patch to support BLOBLIST_TABLES properly in QEMU
- Add new patch to support a 64-bit ramdisk address
- Add new patch to mark struct acpi_rsdp as packed
- Add new patch to consider non-bootable partitions
- Add new patch to handle running out of labels
- Add new patch to support IO UARTs for earlycon and console
- Add new patch to correct mapping in FADT
- Add new patch to add a checksum to the DMAR table
- Add new patch to correct memory leaks in the ACPI test
- Add new patch to support checking checksums with ACPI command
- Significantly expanded to correct e820 and other issues

Changes in v2:
- Redo commit message
- Add new patch to rename the _D dirty flag
- Add new patch to support CPU functions in long mode
- Add new patch to set an MTRR for the RAM in QEMU
- Add new patch with a helper to send characters
- Add new patch to allow tests to be filtered by role
- Add more patches to support booting with kvm
- Add new patch with a test for booting Ubuntu 24.04

Simon Glass (46):
  boot: Correct ramdisk address
  sandbox: Correct a typo in mapmem
  x86: Expand x86_64 early memory
  x86: qemu: Switch to bochs display
  x86: qemu: Enable dhrystone
  x86: qemu: Avoid accessing BSS too early
  x86: Drop mpspec from the SPL build
  x86: Add some log categories
  x86: Drop use of CONFIG_REALMODE_DEBUG
  x86: Avoid clearing the VESA display
  x86: Add 64-bit entries to the GDT
  x86: Use defines for the cache flags
  x86: spl: Drop duplicate CPU init
  x86: Drop the message about features missing in 64-bit
  x86: Include stdbool.h in interrupt header
  x86: Tidy up the GDT size in start/16.S
  x86: Disable paging before changing to long mode
  x86: Use the same GDT when jumping to long mode
  x86: Use a simple jump into long mode
  x86: Rename the _D dirty flag
  x86: Support CPU functions in long mode
  x86: Add functions to convert between mtrr size and mask
  x86: Update mtrr command to support 64-bit values
  x86: Update cpuid_eax et al to work on amd64
  x86: Correct msr operation on amd64
  x86: Allow adding non-aligned size for MTRR
  x86: emulation: Set an MTRR for the RAM
  x86: qemu: Support environment and cat command
  test/py: Correct sizing of created disks
  test/py: Add a helper to send characters
  test/py: Allow tests to be filtered by role
  x86: e820: Add a function to dump the e820 table
  x86: Add a new API for e820
  x86: qemu: Use the new e820 API
  x86: emulation: Support BLOBLIST_TABLES properly
  x86: Support a 64-bit ramdisk address
  acpi: Mark struct acpi_rsdp as packed
  boot: Consider non-bootable partitions
  boot: Handle running out of labels
  boot: Support IO UARTs for earlycon and console
  sandbox: acpi: Avoid a warning about FADT
  sandbox: acpi: Correct mapping in FADT
  acpi: Add a checksum to the DMAR table
  test: acpi: Correct memory leaks
  acpi: Support checking checksums
  test: Add a test for booting Ubuntu 24.04

 .gitlab-ci.yml                   |   8 ++-
 arch/x86/cpu/cpu.c               |  24 +++++++
 arch/x86/cpu/i386/call64.S       |  35 ++++------
 arch/x86/cpu/i386/cpu.c          |  41 ++++-------
 arch/x86/cpu/mtrr.c              | 115 ++++++++++++++++++++++++-------
 arch/x86/cpu/qemu/dram.c         |  18 +++++
 arch/x86/cpu/qemu/e820.c         |  62 +++++++----------
 arch/x86/cpu/qemu/qemu.c         |  20 ++++--
 arch/x86/cpu/start.S             |   4 +-
 arch/x86/cpu/start16.S           |   3 +-
 arch/x86/cpu/x86_64/cpu.c        |   5 --
 arch/x86/include/asm/bootparam.h |  15 +++-
 arch/x86/include/asm/cpu.h       |  91 ++++++++++--------------
 arch/x86/include/asm/e820.h      |  95 ++++++++++++++++++++++++-
 arch/x86/include/asm/interrupt.h |   1 +
 arch/x86/include/asm/msr.h       |   9 ++-
 arch/x86/include/asm/mtrr.h      |  16 +++++
 arch/x86/include/asm/processor.h |   5 +-
 arch/x86/lib/Makefile            |   2 +
 arch/x86/lib/bios.c              |  27 +++++---
 arch/x86/lib/bios_interrupts.c   |   8 +--
 arch/x86/lib/e820.c              |  70 +++++++++++++++++++
 arch/x86/lib/i8259.c             |   2 +
 arch/x86/lib/spl.c               |   4 +-
 arch/x86/lib/tables.c            |   9 ++-
 arch/x86/lib/zimage.c            |  34 +++------
 boot/bootdev-uclass.c            |   7 +-
 boot/bootflow.c                  |   7 +-
 boot/pxe_utils.c                 |   3 +-
 cmd/acpi.c                       |  59 ++++++++++------
 cmd/bootflow.c                   |   2 +-
 cmd/x86/mtrr.c                   |  11 ++-
 configs/qemu-x86_64_defconfig    |  18 ++---
 configs/qemu-x86_defconfig       |  12 ++--
 doc/develop/bootstd/overview.rst |   5 +-
 doc/usage/cmd/acpi.rst           |  20 +++++-
 drivers/misc/qfw_acpi.c          |  32 +++++++--
 include/acpi/acpi_table.h        |   2 +-
 include/bootflow.h               |   2 +
 include/bootstd.h                |   3 +
 include/mapmem.h                 |   2 +-
 lib/acpi/acpi_table.c            |   5 +-
 test/boot/bootdev.c              |   1 +
 test/boot/bootflow.c             |   5 +-
 test/dm/acpi.c                   |  59 ++++++++++++++++
 test/py/conftest.py              |  22 ++++++
 test/py/console_base.py          |  25 ++++---
 test/py/pytest.ini               |   1 +
 test/py/tests/test_distro.py     |  61 ++++++++++++++++
 test/py/tests/test_ut.py         |  17 ++++-
 50 files changed, 805 insertions(+), 299 deletions(-)
 create mode 100644 test/py/tests/test_distro.py

-- 
2.43.0

[PATCH v5 01/46] boot: Correct ramdisk address

[Simon Glass]

We must use the ramdisk address for the initrd_addr field, not the kernel
address. Fix this.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: e05cda3004f ("boot: pxe: Refactor label_run_boot() to avoid")
Reviewed-by: Tom Rini <trini@konsulko.com>
---

(no changes since v1)

 boot/pxe_utils.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index 16608c6ebc8..d827b1135bb 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -601,7 +601,8 @@ static int label_run_boot(struct pxe_context *ctx, struct pxe_label *label,
 
 	if (initrd_addr_str) {
 		bmi.conf_ramdisk = initrd_str;
-		bootm_x86_set(&bmi, initrd_addr, hextoul(kernel_addr, NULL));
+		bootm_x86_set(&bmi, initrd_addr,
+			      hextoul(initrd_addr_str, NULL));
 		bootm_x86_set(&bmi, initrd_size,
 			      hextoul(initrd_filesize, NULL));
 	}
-- 
2.43.0

[PATCH v5 02/46] sandbox: Correct a typo in mapmem

[Simon Glass]

This should say 'cast', not 'case', so fix it.

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

(no changes since v1)

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

diff --git a/include/mapmem.h b/include/mapmem.h
index f496c96d16c..8ec03d59f0b 100644
--- a/include/mapmem.h
+++ b/include/mapmem.h
@@ -33,7 +33,7 @@ static inline phys_addr_t map_to_sysmem(const void *ptr)
  * 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
+ * SMBIOS tables. Using this function instead of a cast shows that the sandbox
  * conversion has been done
  */
 static inline void *nomap_sysmem(phys_addr_t paddr, unsigned long len)
-- 
2.43.0

[PATCH v5 03/46] x86: Expand x86_64 early memory

[Simon Glass]

The SPL and pre-reloc malloc()-space is not large enough to start up
with a display. Expand it.

Switch the order of SPL_SYS_MALLOC_F_LEN and SPL_TEXT_BASE since this
matches what 'savedefconfig' gives us.

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

(no changes since v1)

 configs/qemu-x86_64_defconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
index 792ba06c2b8..af275e8ce8f 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -1,13 +1,13 @@
 CONFIG_X86=y
 CONFIG_TEXT_BASE=0x1110000
-CONFIG_SYS_MALLOC_F_LEN=0x1000
+CONFIG_SYS_MALLOC_F_LEN=0x1800
 CONFIG_BLOBLIST_SIZE_RELOC=0x20000
 CONFIG_NR_DRAM_BANKS=8
 CONFIG_ENV_SIZE=0x40000
 CONFIG_MAX_CPUS=2
 CONFIG_SPL_DM_SPI=y
 CONFIG_DEFAULT_DEVICE_TREE="qemu-x86_i440fx"
-CONFIG_SPL_SYS_MALLOC_F_LEN=0x3000
+CONFIG_SPL_SYS_MALLOC_F_LEN=0x4800
 CONFIG_SPL_TEXT_BASE=0xfffd0000
 CONFIG_DEBUG_UART_BASE=0x3f8
 CONFIG_DEBUG_UART_CLOCK=1843200
-- 
2.43.0

[PATCH v5 04/46] x86: qemu: Switch to bochs display

[Simon Glass]

The vesa display is widely used on hardware, but it is a bit of a pain
with QEMU. It requires executing option ROMs, which either doesn't work
with kvm, or is difficult to do in a kvm/QEMU-friendly way.

THe bochs display is probably better anyway, so switch to that. It works
fine with kvm as it doesn't need an option ROM.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/31
---

(no changes since v2)

Changes in v2:
- Redo commit message

 configs/qemu-x86_64_defconfig | 5 ++---
 configs/qemu-x86_defconfig    | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
index af275e8ce8f..ff2a192ee7d 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -83,9 +83,8 @@ CONFIG_SYS_NS16550_PORT_MAPPED=y
 CONFIG_SPI=y
 CONFIG_USB_KEYBOARD=y
 CONFIG_CONSOLE_TRUETYPE=y
-CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
-CONFIG_FRAMEBUFFER_VESA_MODE_USER=y
-CONFIG_FRAMEBUFFER_VESA_MODE=0x144
+CONFIG_VIDEO_BOCHS=y
+# CONFIG_VIDEO_VESA is not set
 CONFIG_CONSOLE_SCROLL_LINES=5
 CONFIG_SPL_VIDEO=y
 # CONFIG_SPL_USE_TINY_PRINTF is not set
diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
index 0b0e10c795f..ee5b150e062 100644
--- a/configs/qemu-x86_defconfig
+++ b/configs/qemu-x86_defconfig
@@ -62,9 +62,8 @@ CONFIG_SYS_NS16550_PORT_MAPPED=y
 CONFIG_SPI=y
 CONFIG_USB_KEYBOARD=y
 CONFIG_CONSOLE_TRUETYPE=y
-CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
-CONFIG_FRAMEBUFFER_VESA_MODE_USER=y
-CONFIG_FRAMEBUFFER_VESA_MODE=0x144
+CONFIG_VIDEO_BOCHS=y
+# CONFIG_VIDEO_VESA is not set
 CONFIG_CONSOLE_SCROLL_LINES=5
 CONFIG_GENERATE_ACPI_TABLE=y
 # CONFIG_GZIP is not set
-- 
2.43.0

[PATCH v5 05/46] x86: qemu: Enable dhrystone

[Simon Glass]

Provide the 'dhry' command, which helps to check that kvm is being used
properly with QEMU.

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

(no changes since v1)

 configs/qemu-x86_64_defconfig | 1 +
 configs/qemu-x86_defconfig    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
index ff2a192ee7d..84b7dc8e99f 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -89,4 +89,5 @@ CONFIG_CONSOLE_SCROLL_LINES=5
 CONFIG_SPL_VIDEO=y
 # CONFIG_SPL_USE_TINY_PRINTF is not set
 CONFIG_GENERATE_ACPI_TABLE=y
+CONFIG_CMD_DHRYSTONE=y
 # CONFIG_GZIP is not set
diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
index ee5b150e062..d52afa42955 100644
--- a/configs/qemu-x86_defconfig
+++ b/configs/qemu-x86_defconfig
@@ -66,4 +66,5 @@ CONFIG_VIDEO_BOCHS=y
 # CONFIG_VIDEO_VESA is not set
 CONFIG_CONSOLE_SCROLL_LINES=5
 CONFIG_GENERATE_ACPI_TABLE=y
+CONFIG_CMD_DHRYSTONE=y
 # CONFIG_GZIP is not set
-- 
2.43.0

[PATCH v5 06/46] x86: qemu: Avoid accessing BSS too early

[Simon Glass]

BSS is placed in DRAM which is actually available early with QEMU. But
it is cleared by the init sequence, so values stored there are lost.

Move the system-type flag into a function, instead.

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

(no changes since v1)

 arch/x86/cpu/qemu/qemu.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
index 563f63e2bc8..e846ccd44aa 100644
--- a/arch/x86/cpu/qemu/qemu.c
+++ b/arch/x86/cpu/qemu/qemu.c
@@ -15,14 +15,21 @@
 #include <asm/arch/qemu.h>
 #include <asm/u-boot-x86.h>
 
-static bool i440fx;
-
 #if CONFIG_IS_ENABLED(QFW_PIO)
 U_BOOT_DRVINFO(x86_qfw_pio) = {
 	.name = "qfw_pio",
 };
 #endif
 
+static bool is_i440fx(void)
+{
+	u16 device;
+
+	pci_read_config16(PCI_BDF(0, 0, 0), PCI_DEVICE_ID, &device);
+
+	return device == PCI_DEVICE_ID_INTEL_82441;
+}
+
 static void enable_pm_piix(void)
 {
 	u8 en;
@@ -50,16 +57,17 @@ static void enable_pm_ich9(void)
 
 void qemu_chipset_init(void)
 {
-	u16 device, xbcs;
+	bool i440fx;
+	u16 xbcs;
 	int pam, i;
 
+	i440fx = is_i440fx();
+
 	/*
 	 * i440FX and Q35 chipset have different PAM register offset, but with
 	 * the same bitfield layout. Here we determine the offset based on its
 	 * PCI device ID.
 	 */
-	pci_read_config16(PCI_BDF(0, 0, 0), PCI_DEVICE_ID, &device);
-	i440fx = (device == PCI_DEVICE_ID_INTEL_82441);
 	pam = i440fx ? I440FX_PAM : Q35_PAM;
 
 	/*
@@ -123,7 +131,7 @@ int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq)
 {
 	u8 irq;
 
-	if (i440fx) {
+	if (is_i440fx()) {
 		/*
 		 * Not like most x86 platforms, the PIRQ[A-D] on PIIX3 are not
 		 * connected to I/O APIC INTPIN#16-19. Instead they are routed
-- 
2.43.0

[PATCH v5 07/46] x86: Drop mpspec from the SPL build

[Simon Glass]

This is not needed in SPL, so drop it.

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

(no changes since v1)

 arch/x86/lib/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index 43e6a1de77d..a908356e8a6 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -26,7 +26,9 @@ obj-y	+= e820.o
 obj-y	+= init_helpers.o
 obj-y	+= interrupts.o
 obj-y	+= lpc-uclass.o
+ifndef CONFIG_XPL_BUILD
 obj-y	+= mpspec.o
+endif
 obj-$(CONFIG_$(PHASE_)ACPIGEN) += acpi_nhlt.o
 obj-y	+= northbridge-uclass.o
 obj-$(CONFIG_I8259_PIC) += i8259.o
-- 
2.43.0

[PATCH v5 08/46] x86: Add some log categories

[Simon Glass]

Add categories for i8259 and bios files, so that log statements have the
right category.

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

(no changes since v1)

 arch/x86/lib/bios.c            | 3 +++
 arch/x86/lib/bios_interrupts.c | 2 ++
 arch/x86/lib/i8259.c           | 2 ++
 3 files changed, 7 insertions(+)

diff --git a/arch/x86/lib/bios.c b/arch/x86/lib/bios.c
index 03f7360032c..acae635cf19 100644
--- a/arch/x86/lib/bios.c
+++ b/arch/x86/lib/bios.c
@@ -5,6 +5,9 @@
  * Copyright (C) 2007 Advanced Micro Devices, Inc.
  * Copyright (C) 2009-2010 coresystems GmbH
  */
+
+#define LOG_CATEGRORY	LOGC_ARCH
+
 #include <compiler.h>
 #include <bios_emul.h>
 #include <irq_func.h>
diff --git a/arch/x86/lib/bios_interrupts.c b/arch/x86/lib/bios_interrupts.c
index b2cf1527b1c..3ae6c193ec7 100644
--- a/arch/x86/lib/bios_interrupts.c
+++ b/arch/x86/lib/bios_interrupts.c
@@ -7,6 +7,8 @@
  * Copyright (C) 2007-2009 coresystems GmbH
  */
 
+#define LOG_CATEGRORY	LOGC_ARCH
+
 #include <log.h>
 #include <asm/pci.h>
 #include "bios_emul.h"
diff --git a/arch/x86/lib/i8259.c b/arch/x86/lib/i8259.c
index 465ff70146f..088f78f4661 100644
--- a/arch/x86/lib/i8259.c
+++ b/arch/x86/lib/i8259.c
@@ -13,6 +13,8 @@
  * Programmable Interrupt Controllers.
  */
 
+#define LOG_CATEGORY	UCLASS_IRQ
+
 #include <log.h>
 #include <asm/io.h>
 #include <asm/i8259.h>
-- 
2.43.0

[PATCH v5 09/46] x86: Drop use of CONFIG_REALMODE_DEBUG

[Simon Glass]

This option is not actually defined in Kconfig anymore. Use a normal
debug print instead, which has a similar effect.

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

(no changes since v1)

 arch/x86/lib/bios.c            | 18 ++++++++----------
 arch/x86/lib/bios_interrupts.c |  6 ++----
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/arch/x86/lib/bios.c b/arch/x86/lib/bios.c
index acae635cf19..5dfe5a647eb 100644
--- a/arch/x86/lib/bios.c
+++ b/arch/x86/lib/bios.c
@@ -301,16 +301,14 @@ asmlinkage int interrupt_handler(u32 intnumber, u32 gsfs, u32 dses,
 	cs = cs_ip >> 16;
 	flags = stackflags;
 
-#ifdef CONFIG_REALMODE_DEBUG
-	debug("oprom: INT# 0x%x\n", intnumber);
-	debug("oprom: eax: %08x ebx: %08x ecx: %08x edx: %08x\n",
-	      eax, ebx, ecx, edx);
-	debug("oprom: ebp: %08x esp: %08x edi: %08x esi: %08x\n",
-	      ebp, esp, edi, esi);
-	debug("oprom:  ip: %04x      cs: %04x   flags: %08x\n",
-	      ip, cs, flags);
-	debug("oprom: stackflags = %04x\n", stackflags);
-#endif
+	log_debug("oprom: INT# 0x%x\n", intnumber);
+	log_debug("oprom: eax: %08x ebx: %08x ecx: %08x edx: %08x\n",
+		  eax, ebx, ecx, edx);
+	log_debug("oprom: ebp: %08x esp: %08x edi: %08x esi: %08x\n",
+		  ebp, esp, edi, esi);
+	log_debug("oprom:  ip: %04x      cs: %04x   flags: %08x\n",
+		  ip, cs, flags);
+	log_debug("oprom: stackflags = %04x\n", stackflags);
 
 	/*
 	 * Fetch arguments from the stack and put them to a place
diff --git a/arch/x86/lib/bios_interrupts.c b/arch/x86/lib/bios_interrupts.c
index 3ae6c193ec7..e0c2284a901 100644
--- a/arch/x86/lib/bios_interrupts.c
+++ b/arch/x86/lib/bios_interrupts.c
@@ -200,10 +200,8 @@ int int1a_handler(void)
 			dm_pci_write_config32(dev, reg, dword);
 			break;
 		}
-#ifdef CONFIG_REALMODE_DEBUG
-		debug("0x%x: bus %d devfn 0x%x reg 0x%x val 0x%x\n", func,
-		      bus, devfn, reg, M.x86.R_ECX);
-#endif
+		log_debug("0x%x: bus %d devfn 0x%x reg 0x%x val 0x%x\n", func,
+			  bus, devfn, reg, M.x86.R_ECX);
 		M.x86.R_EAX &= 0xffff00ff; /* Clear AH */
 		M.x86.R_EAX |= PCIBIOS_SUCCESSFUL;
 		retval = 1;
-- 
2.43.0

[PATCH v5 10/46] x86: Avoid clearing the VESA display

[Simon Glass]

U-Boot clears the display when it starts up, so there is no need to ask
the VESA driver to do this. Fix this and add a comment explaining the
flags.

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

(no changes since v1)

 arch/x86/lib/bios.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/lib/bios.c b/arch/x86/lib/bios.c
index 5dfe5a647eb..de4578666fb 100644
--- a/arch/x86/lib/bios.c
+++ b/arch/x86/lib/bios.c
@@ -231,7 +231,11 @@ static void vbe_set_graphics(int vesa_mode, struct vesa_state *mode_info)
 {
 	unsigned char *framebuffer;
 
-	mode_info->video_mode = (1 << 14) | vesa_mode;
+	/*
+	 * bit 14 is linear-framebuffer mode
+	 * bit 15 means don't clear the display
+	 */
+	mode_info->video_mode = (1 << 14) | (1 << 15) | vesa_mode;
 	vbe_get_mode_info(mode_info);
 
 	framebuffer = (unsigned char *)(ulong)mode_info->vesa.phys_base_ptr;
-- 
2.43.0

[PATCH v5 11/46] x86: Add 64-bit entries to the GDT

[Simon Glass]

At present it is not possible to execution 64-bit code without
installing an entire new Global Descriptor Table. This is inconvenient
since kvm does not seem to like switching into long mode with a new
table.

It isn't actually necessary, since we can just extend the existing
table. Add some new entries to this effect.

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

(no changes since v1)

 arch/x86/cpu/i386/cpu.c          | 3 +++
 arch/x86/include/asm/processor.h | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
index a51a24498a7..3bbad3b2eca 100644
--- a/arch/x86/cpu/i386/cpu.c
+++ b/arch/x86/cpu/i386/cpu.c
@@ -160,6 +160,9 @@ void arch_setup_gd(gd_t *new_gd)
 
 	gdt_addr[X86_GDT_ENTRY_16BIT_FLAT_CS] = GDT_ENTRY(0x809b, 0, 0xfffff);
 	gdt_addr[X86_GDT_ENTRY_16BIT_FLAT_DS] = GDT_ENTRY(0x8093, 0, 0xfffff);
+	gdt_addr[X86_GDT_ENTRY_64BIT_CS] = GDT_ENTRY(0xaf9b, 0, 0xfffff);
+	gdt_addr[X86_GDT_ENTRY_64BIT_TS1] = GDT_ENTRY(0x8980, 0, 0xfffff);
+	gdt_addr[X86_GDT_ENTRY_64BIT_TS2] = 0;
 
 	load_gdt(gdt_addr, X86_GDT_NUM_ENTRIES);
 	load_ds(X86_GDT_ENTRY_32BIT_DS);
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index d7b68367861..ad8240be387 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -18,7 +18,10 @@
 #define X86_GDT_ENTRY_16BIT_DS		6
 #define X86_GDT_ENTRY_16BIT_FLAT_CS	7
 #define X86_GDT_ENTRY_16BIT_FLAT_DS	8
-#define X86_GDT_NUM_ENTRIES		9
+#define X86_GDT_ENTRY_64BIT_CS		9
+#define X86_GDT_ENTRY_64BIT_TS1		10
+#define X86_GDT_ENTRY_64BIT_TS2		11
+#define X86_GDT_NUM_ENTRIES		12
 
 #define X86_GDT_SIZE		(X86_GDT_NUM_ENTRIES * X86_GDT_ENTRY_SIZE)
 
-- 
2.43.0

[PATCH v5 12/46] x86: Use defines for the cache flags

[Simon Glass]

Use some named flags when setting up the cache, so it is easier to see
what is going on.

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

(no changes since v1)

 arch/x86/cpu/i386/cpu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
index 3bbad3b2eca..845e00ca439 100644
--- a/arch/x86/cpu/i386/cpu.c
+++ b/arch/x86/cpu/i386/cpu.c
@@ -592,6 +592,13 @@ int cpu_has_64bit(void)
 #define PAGETABLE_BASE		0x80000
 #define PAGETABLE_SIZE		(6 * 4096)
 
+#define _PRES BIT(0)	/* present */
+#define _RW   BIT(1)	/* write allowed */
+#define _US   BIT(2)	/* user-access allowed */
+#define _A    BIT(5)	/* has been accessed */
+#define _D    BIT(6)	/* has been written to */
+#define _PS   BIT(7)	/* indicates 2MB page size here */
+
 /**
  * build_pagetable() - build a flat 4GiB page table structure for 64-bti mode
  *
@@ -604,15 +611,17 @@ static void build_pagetable(uint32_t *pgtable)
 	memset(pgtable, '\0', PAGETABLE_SIZE);
 
 	/* Level 4 needs a single entry */
-	pgtable[0] = (ulong)&pgtable[1024] + 7;
+	pgtable[0] = (ulong)&pgtable[1024] + _PRES + _RW + _US + _A;
 
 	/* Level 3 has one 64-bit entry for each GiB of memory */
 	for (i = 0; i < 4; i++)
-		pgtable[1024 + i * 2] = (ulong)&pgtable[2048] + 0x1000 * i + 7;
+		pgtable[1024 + i * 2] = (ulong)&pgtable[2048] + 0x1000 * i +
+			_PRES + _RW + _US + _A;
 
 	/* Level 2 has 2048 64-bit entries, each repesenting 2MiB */
 	for (i = 0; i < 2048; i++)
-		pgtable[2048 + i * 2] = 0x183 + (i << 21UL);
+		pgtable[2048 + i * 2] = _PRES + _RW + _US + _PS + _A +  _D +
+					 (i << 21UL);
 }
 
 int cpu_jump_to_64bit(ulong setup_base, ulong target)
-- 
2.43.0

[PATCH v5 13/46] x86: spl: Drop duplicate CPU init

[Simon Glass]

x86_cpu_init_f() is called by arch_cpu_init() a few lines below this
code. Drop the duplicate call.

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

(no changes since v1)

 arch/x86/lib/spl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
index 7a033505101..47cf9c862b5 100644
--- a/arch/x86/lib/spl.c
+++ b/arch/x86/lib/spl.c
@@ -84,8 +84,6 @@ static int x86_spl_init(void)
 	log_debug("x86 spl starting\n");
 	if (IS_ENABLED(TPL))
 		ret = x86_cpu_reinit_f();
-	else
-		ret = x86_cpu_init_f();
 	ret = spl_init();
 	if (ret) {
 		log_debug("spl_init() failed (err=%d)\n", ret);
-- 
2.43.0

[PATCH v5 14/46] x86: Drop the message about features missing in 64-bit

[Simon Glass]

This functions normally and has done for a while, so drop this scary
message.

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

(no changes since v1)

 arch/x86/lib/spl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
index 47cf9c862b5..0a6a761987e 100644
--- a/arch/x86/lib/spl.c
+++ b/arch/x86/lib/spl.c
@@ -281,7 +281,7 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 {
 	int ret;
 
-	printf("Jumping to 64-bit U-Boot: Note many features are missing\n");
+	log_debug("Jumping to 64-bit U-Boot\n");
 	ret = cpu_jump_to_64bit_uboot(spl_image->entry_point);
 	debug("ret=%d\n", ret);
 	hang();
-- 
2.43.0

[PATCH v5 15/46] x86: Include stdbool.h in interrupt header

[Simon Glass]

This makes use of a 'bool' type, so include the required header.

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

(no changes since v1)

 arch/x86/include/asm/interrupt.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/interrupt.h b/arch/x86/include/asm/interrupt.h
index e23fb2c8e72..c689fc23d08 100644
--- a/arch/x86/include/asm/interrupt.h
+++ b/arch/x86/include/asm/interrupt.h
@@ -10,6 +10,7 @@
 #ifndef __ASM_INTERRUPT_H_
 #define __ASM_INTERRUPT_H_ 1
 
+#include <stdbool.h>
 #include <asm/types.h>
 
 #define SYS_NUM_IRQS	16
-- 
2.43.0

[PATCH v5 16/46] x86: Tidy up the GDT size in start/16.S

[Simon Glass]

Use a symbol to select the size of the GDT, rather than hard-coding a
value. This matches how it is done in start64

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

(no changes since v1)

 arch/x86/cpu/start.S   | 4 +++-
 arch/x86/cpu/start16.S | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S
index 0ef27cc5a00..385a691265e 100644
--- a/arch/x86/cpu/start.S
+++ b/arch/x86/cpu/start.S
@@ -254,7 +254,7 @@ multiboot_header:
 	 * GDT is setup in a safe location in RAM
 	 */
 gdt_ptr2:
-	.word	0x1f		/* limit (31 bytes = 4 GDT entries - 1) */
+	.word	gdt2_end - gdt_ptr2 - 1
 	.long	gdt_rom2	/* base */
 
 	/* Some CPUs are picky about GDT alignment... */
@@ -313,4 +313,6 @@ gdt_rom2:
 	.byte	0x93		/* access */
 	.byte	0xcf		/* flags + limit_high */
 	.byte	0x00		/* base_high */
+gdt2_end:
+
 #endif
diff --git a/arch/x86/cpu/start16.S b/arch/x86/cpu/start16.S
index 865a49731e5..8d9acb193e0 100644
--- a/arch/x86/cpu/start16.S
+++ b/arch/x86/cpu/start16.S
@@ -61,7 +61,7 @@ idt_ptr:
 	 * GDT is setup in a safe location in RAM
 	 */
 gdt_ptr:
-	.word	0x1f		/* limit (31 bytes = 4 GDT entries - 1) */
+	.word	gdt_end - gdt_rom - 1
 	.long	BOOT_SEG + gdt_rom	/* base */
 
 	/* Some CPUs are picky about GDT alignment... */
@@ -120,3 +120,4 @@ gdt_rom:
 	.byte	0x93		/* access */
 	.byte	0xcf		/* flags + limit_high */
 	.byte	0x00		/* base_high */
+gdt_end:
-- 
2.43.0

[PATCH v5 17/46] x86: Disable paging before changing to long mode

[Simon Glass]

This is required as part of the procedure. The existing code works
because it changes the GDT at the same time, but this makes kvm
unhappy.

Update the algorithm to disable and then re-enable paging.

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

(no changes since v1)

 arch/x86/cpu/i386/call64.S | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/cpu/i386/call64.S b/arch/x86/cpu/i386/call64.S
index d81bcc6f8f4..e5a0420d1ba 100644
--- a/arch/x86/cpu/i386/call64.S
+++ b/arch/x86/cpu/i386/call64.S
@@ -25,6 +25,11 @@ cpu_call64:
 	push	%edx		/* arg1 = setup_base */
 	mov	%eax, %ebx
 
+	# disable paging
+	movl	%cr0, %eax
+	andl	$~X86_CR0_PG, %eax
+	movl	%eax, %cr0
+
 	/* Load new GDT with the 64bit segments using 32bit descriptor */
 	leal	gdt, %eax
 	movl	%eax, gdt+2
@@ -67,7 +72,8 @@ cpu_call64:
 	pushl	%eax
 
 	/* Enter paged protected Mode, activating Long Mode */
-	movl	$(X86_CR0_PG | X86_CR0_PE), %eax
+	movl	%cr0, %eax
+	orl	$X86_CR0_PG, %eax
 	movl	%eax, %cr0
 
 	/* Jump from 32bit compatibility mode into 64bit mode. */
-- 
2.43.0

[PATCH v5 18/46] x86: Use the same GDT when jumping to long mode

[Simon Glass]

Make use the existing GDT which now includes entries for 64-bit code.
Leave the interrupt descriptors alone. They can be tidied up once U-Boot
starts up.

With this, kvm mode works with QEMU.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/31
---

(no changes since v1)

 arch/x86/cpu/i386/call64.S | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/x86/cpu/i386/call64.S b/arch/x86/cpu/i386/call64.S
index e5a0420d1ba..3137ec17d31 100644
--- a/arch/x86/cpu/i386/call64.S
+++ b/arch/x86/cpu/i386/call64.S
@@ -7,6 +7,7 @@
  */
 
 #include <asm/msr-index.h>
+#include <asm/processor.h>
 #include <asm/processor-flags.h>
 
 .code32
@@ -30,11 +31,6 @@ cpu_call64:
 	andl	$~X86_CR0_PG, %eax
 	movl	%eax, %cr0
 
-	/* Load new GDT with the 64bit segments using 32bit descriptor */
-	leal	gdt, %eax
-	movl	%eax, gdt+2
-	lgdt	gdt
-
 	/* Enable PAE mode */
 	movl	$(X86_CR4_PAE), %eax
 	movl	%eax, %cr4
@@ -49,12 +45,6 @@ cpu_call64:
 	btsl	$_EFER_LME, %eax
 	wrmsr
 
-	/* After gdt is loaded */
-	xorl	%eax, %eax
-	lldt	%ax
-	movl    $0x20, %eax
-	ltr	%ax
-
 	/*
 	 * Setup for the jump to 64bit mode
 	 *
@@ -67,7 +57,7 @@ cpu_call64:
 	 */
 	pop	%esi			/* setup_base */
 
-	pushl	$0x10
+	pushl	$(X86_GDT_ENTRY_64BIT_CS * X86_GDT_ENTRY_SIZE)
 	leal	lret_target, %eax
 	pushl	%eax
 
-- 
2.43.0

[PATCH v5 19/46] x86: Use a simple jump into long mode

[Simon Glass]

With the 64-bit descriptor we can use a jump instruction, rather than
pushing things on the stack.

Since the processor is in 64-bit mode by this point, pop a 64-bit value
from the stack, containing the target address.

This simplifies the code slightly, in particular its use of the stack.

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

(no changes since v1)

 arch/x86/cpu/i386/call64.S | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/x86/cpu/i386/call64.S b/arch/x86/cpu/i386/call64.S
index 3137ec17d31..c6308b92e25 100644
--- a/arch/x86/cpu/i386/call64.S
+++ b/arch/x86/cpu/i386/call64.S
@@ -22,6 +22,7 @@ cpu_call64:
 	 * ecx - target
 	 */
 	cli
+	pushl	$0		/* top 64-bits of target */
 	push	%ecx		/* arg2 = target */
 	push	%edx		/* arg1 = setup_base */
 	mov	%eax, %ebx
@@ -32,7 +33,8 @@ cpu_call64:
 	movl	%eax, %cr0
 
 	/* Enable PAE mode */
-	movl	$(X86_CR4_PAE), %eax
+	movl	%cr4, %eax
+	orl	$X86_CR4_PAE, %eax
 	movl	%eax, %cr4
 
 	/* Enable the boot page tables */
@@ -57,23 +59,18 @@ cpu_call64:
 	 */
 	pop	%esi			/* setup_base */
 
-	pushl	$(X86_GDT_ENTRY_64BIT_CS * X86_GDT_ENTRY_SIZE)
-	leal	lret_target, %eax
-	pushl	%eax
-
 	/* Enter paged protected Mode, activating Long Mode */
 	movl	%cr0, %eax
 	orl	$X86_CR0_PG, %eax
 	movl	%eax, %cr0
 
 	/* Jump from 32bit compatibility mode into 64bit mode. */
-	lret
+	ljmp	$(X86_GDT_ENTRY_64BIT_CS * X86_GDT_ENTRY_SIZE), $lret_target
 
-code64:
+.code64
 lret_target:
-	pop	%eax			/* target */
-	mov	%eax, %eax		/* Clear bits 63:32 */
-	jmp	*%eax			/* Jump to the 64-bit target */
+	pop	%rax			/* target */
+	jmp	*%rax			/* Jump to the 64-bit target */
 
 .globl call64_stub_size
 call64_stub_size:
-- 
2.43.0

[PATCH v5 20/46] x86: Rename the _D dirty flag

[Simon Glass]

This value happens to be used by ctype.h so chose a different name.

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

(no changes since v2)

Changes in v2:
- Add new patch to rename the _D dirty flag

 arch/x86/cpu/i386/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
index 845e00ca439..b108f724c51 100644
--- a/arch/x86/cpu/i386/cpu.c
+++ b/arch/x86/cpu/i386/cpu.c
@@ -596,7 +596,7 @@ int cpu_has_64bit(void)
 #define _RW   BIT(1)	/* write allowed */
 #define _US   BIT(2)	/* user-access allowed */
 #define _A    BIT(5)	/* has been accessed */
-#define _D    BIT(6)	/* has been written to */
+#define _DT   BIT(6)	/* has been written to */
 #define _PS   BIT(7)	/* indicates 2MB page size here */
 
 /**
@@ -620,7 +620,7 @@ static void build_pagetable(uint32_t *pgtable)
 
 	/* Level 2 has 2048 64-bit entries, each repesenting 2MiB */
 	for (i = 0; i < 2048; i++)
-		pgtable[2048 + i * 2] = _PRES + _RW + _US + _PS + _A +  _D +
+		pgtable[2048 + i * 2] = _PRES + _RW + _US + _PS + _A +  _DT +
 					 (i << 21UL);
 }
 
-- 
2.43.0

[PATCH v5 21/46] x86: Support CPU functions in long mode

[Simon Glass]

At present it is not possible to find out the physical-address size in
long mode, so a predefined value is used.

Update the macros to support this properly, since it is important when
programming MTRRs.

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

(no changes since v3)

Changes in v3:
- Always return true from flag_is_changeable_p() on amd64

Changes in v2:
- Add new patch to support CPU functions in long mode

 arch/x86/cpu/cpu.c         | 24 ++++++++++++++++++++++++
 arch/x86/cpu/i386/cpu.c    | 23 -----------------------
 arch/x86/cpu/x86_64/cpu.c  |  5 -----
 arch/x86/include/asm/cpu.h | 20 ++++++++++++++------
 4 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
index a8b21406ac0..c373b14df30 100644
--- a/arch/x86/cpu/cpu.c
+++ b/arch/x86/cpu/cpu.c
@@ -364,3 +364,27 @@ long locate_coreboot_table(void)
 
 	return addr;
 }
+
+static bool has_cpuid(void)
+{
+	return flag_is_changeable_p(X86_EFLAGS_ID);
+}
+
+static uint cpu_cpuid_extended_level(void)
+{
+	return cpuid_eax(0x80000000);
+}
+
+int cpu_phys_address_size(void)
+{
+	if (!has_cpuid())
+		return 32;
+
+	if (cpu_cpuid_extended_level() >= 0x80000008)
+		return cpuid_eax(0x80000008) & 0xff;
+
+	if (cpuid_edx(1) & (CPUID_FEATURE_PAE | CPUID_FEATURE_PSE36))
+		return 36;
+
+	return 32;
+}
diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
index b108f724c51..ee6dbeb5c48 100644
--- a/arch/x86/cpu/i386/cpu.c
+++ b/arch/x86/cpu/i386/cpu.c
@@ -35,10 +35,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#define CPUID_FEATURE_PAE	BIT(6)
-#define CPUID_FEATURE_PSE36	BIT(17)
-#define CPUID_FEAURE_HTT	BIT(28)
-
 /*
  * Constructor for a conventional segment GDT (or LDT) entry
  * This is a macro so it can be used in initialisers
@@ -412,25 +408,6 @@ static void setup_identity(void)
 	}
 }
 
-static uint cpu_cpuid_extended_level(void)
-{
-	return cpuid_eax(0x80000000);
-}
-
-int cpu_phys_address_size(void)
-{
-	if (!has_cpuid())
-		return 32;
-
-	if (cpu_cpuid_extended_level() >= 0x80000008)
-		return cpuid_eax(0x80000008) & 0xff;
-
-	if (cpuid_edx(1) & (CPUID_FEATURE_PAE | CPUID_FEATURE_PSE36))
-		return 36;
-
-	return 32;
-}
-
 static void setup_mtrr(void)
 {
 	u64 mtrr_cap;
diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
index 71bc07f872a..25ae92c702f 100644
--- a/arch/x86/cpu/x86_64/cpu.c
+++ b/arch/x86/cpu/x86_64/cpu.c
@@ -59,11 +59,6 @@ int x86_cpu_reinit_f(void)
 	return 0;
 }
 
-int cpu_phys_address_size(void)
-{
-	return CONFIG_CPU_ADDR_BITS;
-}
-
 int x86_cpu_init_f(void)
 {
 	return 0;
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index fd389d4024c..1f1b545ea50 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -58,6 +58,10 @@ enum {
 	X86_SYSCON_PUNIT,	/* Power unit */
 };
 
+#define CPUID_FEATURE_PAE	BIT(6)
+#define CPUID_FEATURE_PSE36	BIT(17)
+#define CPUID_FEAURE_HTT	BIT(28)
+
 struct cpuid_result {
 	uint32_t eax;
 	uint32_t ebx;
@@ -161,12 +165,16 @@ static inline unsigned int cpuid_edx(unsigned int op)
 	return edx;
 }
 
-#if !CONFIG_IS_ENABLED(X86_64)
-
+#if CONFIG_IS_ENABLED(X86_64)
+static inline int flag_is_changeable_p(u32 flag)
+{
+	return 1;
+}
+#else
 /* Standard macro to see if a specific flag is changeable */
-static inline int flag_is_changeable_p(uint32_t flag)
+static inline int flag_is_changeable_p(u32 flag)
 {
-	uint32_t f1, f2;
+	u32 f1, f2;
 
 	asm(
 		"pushfl\n\t"
@@ -181,9 +189,9 @@ static inline int flag_is_changeable_p(uint32_t flag)
 		"popfl\n\t"
 		: "=&r" (f1), "=&r" (f2)
 		: "ir" (flag));
-	return ((f1^f2) & flag) != 0;
+	return ((f1 ^ f2) & flag) != 0;
 }
-#endif
+#endif /* X86_64 */
 
 /**
  * cpu_enable_paging_pae() - Enable PAE-paging
-- 
2.43.0

[PATCH v5 22/46] x86: Add functions to convert between mtrr size and mask

[Simon Glass]

Rather than repeating the same code in several places, add some
functions which can do the conversion.

Use the cpu_phys_address_size() function to obtain the physical-address
size, since it is more reliable with kvm, where the host CPU may have a
different value from the emulation CPU.

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

(no changes since v3)

Changes in v3:
- Add new patch with functions to convert between mtrr size and mask

 arch/x86/cpu/mtrr.c         | 33 +++++++++++++++++++++++++--------
 arch/x86/include/asm/mtrr.h | 16 ++++++++++++++++
 cmd/x86/mtrr.c              |  5 ++---
 3 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c
index 07ea89162de..fca7b28f815 100644
--- a/arch/x86/cpu/mtrr.c
+++ b/arch/x86/cpu/mtrr.c
@@ -16,6 +16,7 @@
  * since the MTRR registers are sometimes in flux.
  */
 
+#include <cpu.h>
 #include <cpu_func.h>
 #include <log.h>
 #include <sort.h>
@@ -39,6 +40,27 @@ static const char *const mtrr_type_name[MTRR_TYPE_COUNT] = {
 	"Back",
 };
 
+u64 mtrr_to_size(u64 mask)
+{
+	u64 size;
+
+	size = ~mask & ((1ULL << cpu_phys_address_size()) - 1);
+	size |= (1 << 12) - 1;
+	size += 1;
+
+	return size;
+}
+
+u64 mtrr_to_mask(u64 size)
+{
+	u64 mask;
+
+	mask = ~(size - 1);
+	mask &= (1ull << cpu_phys_address_size()) - 1;
+
+	return mask;
+}
+
 /* Prepare to adjust MTRRs */
 void mtrr_open(struct mtrr_state *state, bool do_caches)
 {
@@ -68,11 +90,9 @@ void mtrr_close(struct mtrr_state *state, bool do_caches)
 
 static void set_var_mtrr(uint reg, uint type, uint64_t start, uint64_t size)
 {
-	u64 mask;
+	u64 mask = mtrr_to_mask(size);
 
 	wrmsrl(MTRR_PHYS_BASE_MSR(reg), start | type);
-	mask = ~(size - 1);
-	mask &= (1ULL << CONFIG_CPU_ADDR_BITS) - 1;
 	wrmsrl(MTRR_PHYS_MASK_MSR(reg), mask | MTRR_PHYS_MASK_VALID);
 }
 
@@ -204,8 +224,7 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size)
 	req->size = size;
 	debug("%d: type=%d, %08llx  %08llx\n", gd->arch.mtrr_req_count - 1,
 	      req->type, req->start, req->size);
-	mask = ~(req->size - 1);
-	mask &= (1ULL << CONFIG_CPU_ADDR_BITS) - 1;
+	mask = mtrr_to_mask(req->size);
 	mask |= MTRR_PHYS_MASK_VALID;
 	debug("   %016llx %016llx\n", req->start | req->type, mask);
 
@@ -360,9 +379,7 @@ int mtrr_list(int reg_count, int cpu_select)
 
 		base = info.mtrr[i].base;
 		mask = info.mtrr[i].mask;
-		size = ~mask & ((1ULL << CONFIG_CPU_ADDR_BITS) - 1);
-		size |= (1 << 12) - 1;
-		size += 1;
+		size = mtrr_to_size(mask);
 		valid = mask & MTRR_PHYS_MASK_VALID;
 		type = mtrr_type_name[base & MTRR_BASE_TYPE_MASK];
 		printf("%d   %-5s %-12s %016llx %016llx %016llx\n", i,
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 2e995f54061..67e897daa25 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -90,6 +90,22 @@ struct mtrr_info {
 	struct mtrr mtrr[MTRR_MAX_COUNT];
 };
 
+/**
+ * mtrr_to_size() - Convert a mask to a size value
+ *
+ * @mask: Value of the mask register
+ * Return: associated size
+ */
+u64 mtrr_to_size(u64 mask);
+
+/**
+ * mtrr_to_mask() - Convert a size to a mask value
+ *
+ * @size: Value of the size register
+ * Return: associated mask, without MTRR_PHYS_MASK_VALID
+ */
+u64 mtrr_to_mask(u64 size);
+
 /**
  * mtrr_open() - Prepare to adjust MTRRs
  *
diff --git a/cmd/x86/mtrr.c b/cmd/x86/mtrr.c
index b2afb598c73..93be84f74b8 100644
--- a/cmd/x86/mtrr.c
+++ b/cmd/x86/mtrr.c
@@ -14,7 +14,7 @@ static int do_mtrr_set(int cpu_select, uint reg, int argc, char *const argv[])
 {
 	const char *typename = argv[0];
 	uint32_t start, size;
-	uint64_t base, mask;
+	u64 base, mask;
 	int type = -1;
 	bool valid;
 	int ret;
@@ -31,8 +31,7 @@ static int do_mtrr_set(int cpu_select, uint reg, int argc, char *const argv[])
 
 	base = start | type;
 	valid = native_read_msr(MTRR_PHYS_MASK_MSR(reg)) & MTRR_PHYS_MASK_VALID;
-	mask = ~((uint64_t)size - 1);
-	mask &= (1ULL << CONFIG_CPU_ADDR_BITS) - 1;
+	mask = mtrr_to_mask(size);
 	if (valid)
 		mask |= MTRR_PHYS_MASK_VALID;
 
-- 
2.43.0

[PATCH v5 23/46] x86: Update mtrr command to support 64-bit values

[Simon Glass]

The MTRR registers have 64-bit values. Update the command to use 64-bit
values so that memory larger than 4GB can be handled.

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

(no changes since v3)

Changes in v3:
- Add new patch to update mtrr command to support 64-bit values

 cmd/x86/mtrr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/x86/mtrr.c b/cmd/x86/mtrr.c
index 93be84f74b8..289865515ef 100644
--- a/cmd/x86/mtrr.c
+++ b/cmd/x86/mtrr.c
@@ -13,7 +13,7 @@
 static int do_mtrr_set(int cpu_select, uint reg, int argc, char *const argv[])
 {
 	const char *typename = argv[0];
-	uint32_t start, size;
+	u64 start, size;
 	u64 base, mask;
 	int type = -1;
 	bool valid;
@@ -26,8 +26,8 @@ static int do_mtrr_set(int cpu_select, uint reg, int argc, char *const argv[])
 		printf("Invalid type name %s\n", typename);
 		return CMD_RET_USAGE;
 	}
-	start = hextoul(argv[1], NULL);
-	size = hextoul(argv[2], NULL);
+	start = hextoull(argv[1], NULL);
+	size = hextoull(argv[2], NULL);
 
 	base = start | type;
 	valid = native_read_msr(MTRR_PHYS_MASK_MSR(reg)) & MTRR_PHYS_MASK_VALID;
-- 
2.43.0

[PATCH v5 24/46] x86: Update cpuid_eax et al to work on amd64

[Simon Glass]

The existing functions work but the register clobbers are wrong, so
strange bugs results.

The original functions were taken from a very old version of Linux.
Update them from Linux 6.13

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

(no changes since v3)

Changes in v3:
- Add new patch to update cpuid_eax et al to work on amd64

 arch/x86/include/asm/cpu.h | 75 +++++++++++++-------------------------
 1 file changed, 25 insertions(+), 50 deletions(-)

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 1f1b545ea50..5d24c17f8a3 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -109,61 +109,36 @@ static inline struct cpuid_result cpuid_ext(int op, unsigned ecx)
 	return result;
 }
 
-/*
- * CPUID functions returning a single datum
- */
-static inline unsigned int cpuid_eax(unsigned int op)
+static inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
+				unsigned int *ecx, unsigned int *edx)
 {
-	unsigned int eax;
-
-	__asm__("mov %%ebx, %%edi;"
-		"cpuid;"
-		"mov %%edi, %%ebx;"
-		: "=a" (eax)
-		: "0" (op)
-		: "ecx", "edx", "edi");
-	return eax;
+	/* ecx is often an input as well as an output. */
+	asm volatile("cpuid"
+	    : "=a" (*eax),
+	      "=b" (*ebx),
+	      "=c" (*ecx),
+	      "=d" (*edx)
+	    : "0" (*eax), "2" (*ecx)
+	    : "memory");
 }
 
-static inline unsigned int cpuid_ebx(unsigned int op)
-{
-	unsigned int eax, ebx;
-
-	__asm__("mov %%ebx, %%edi;"
-		"cpuid;"
-		"mov %%ebx, %%esi;"
-		"mov %%edi, %%ebx;"
-		: "=a" (eax), "=S" (ebx)
-		: "0" (op)
-		: "ecx", "edx", "edi");
-	return ebx;
+#define native_cpuid_reg(reg)					\
+static inline unsigned int cpuid_##reg(unsigned int op)	\
+{								\
+	unsigned int eax = op, ebx, ecx = 0, edx;		\
+								\
+	native_cpuid(&eax, &ebx, &ecx, &edx);			\
+								\
+	return reg;						\
 }
 
-static inline unsigned int cpuid_ecx(unsigned int op)
-{
-	unsigned int eax, ecx;
-
-	__asm__("mov %%ebx, %%edi;"
-		"cpuid;"
-		"mov %%edi, %%ebx;"
-		: "=a" (eax), "=c" (ecx)
-		: "0" (op)
-		: "edx", "edi");
-	return ecx;
-}
-
-static inline unsigned int cpuid_edx(unsigned int op)
-{
-	unsigned int eax, edx;
-
-	__asm__("mov %%ebx, %%edi;"
-		"cpuid;"
-		"mov %%edi, %%ebx;"
-		: "=a" (eax), "=d" (edx)
-		: "0" (op)
-		: "ecx", "edi");
-	return edx;
-}
+/*
+ * Native CPUID functions returning a single datum.
+ */
+native_cpuid_reg(eax)
+native_cpuid_reg(ebx)
+native_cpuid_reg(ecx)
+native_cpuid_reg(edx)
 
 #if CONFIG_IS_ENABLED(X86_64)
 static inline int flag_is_changeable_p(u32 flag)
-- 
2.43.0

[PATCH v5 25/46] x86: Correct msr operation on amd64

[Simon Glass]

The CONFIG option is no-longer correct since we can have SPL and PPL
with different bitness.

Fix this and sync up with Linux 6.13 in this area, since this is where
the code came from many years ago.

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

(no changes since v3)

Changes in v3:
- Add new patch to correct msr operation on amd64

 arch/x86/include/asm/msr.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index af5f9a11980..39dc7b33aa0 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -59,15 +59,14 @@ static inline unsigned long long native_read_tscp(unsigned int *aux)
  * edx:eax, while for x86_64 it doesn't mean rdx:rax or edx:eax. Instead,
  * it means rax *or* rdx.
  */
-#ifdef CONFIG_X86_64
-#define DECLARE_ARGS(val, low, high)	unsigned low, high
-#define EAX_EDX_VAL(val, low, high)	((low) | ((u64)(high) << 32))
-#define EAX_EDX_ARGS(val, low, high)	"a" (low), "d" (high)
+#if CONFIG_IS_ENABLED(X86_64)
+/* Using 64-bit values saves one instruction clearing the high half of low */
+#define DECLARE_ARGS(val, low, high)	unsigned long low, high
+#define EAX_EDX_VAL(val, low, high)	((low) | (high) << 32)
 #define EAX_EDX_RET(val, low, high)	"=a" (low), "=d" (high)
 #else
 #define DECLARE_ARGS(val, low, high)	unsigned long long val
 #define EAX_EDX_VAL(val, low, high)	(val)
-#define EAX_EDX_ARGS(val, low, high)	"A" (val)
 #define EAX_EDX_RET(val, low, high)	"=A" (val)
 #endif
 
-- 
2.43.0

[PATCH v5 26/46] x86: Allow adding non-aligned size for MTRR

[Simon Glass]

At present mtrr_add_request() requires that the size is a power of two.
This is too limiting for machines with 4GB (or more) of RAM, since they
often must take account of a memory hole at 3GB.

Update the function to automatically deal with an unaligned size, using
more MTRRs as required.

The algorithm is taken from coreboot commit 60bce10750

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

(no changes since v3)

Changes in v3:
- Add new patch to allow adding non-aligned size for MTRR

 arch/x86/cpu/mtrr.c | 84 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 68 insertions(+), 16 deletions(-)

diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c
index fca7b28f815..7a0f00b9b8f 100644
--- a/arch/x86/cpu/mtrr.c
+++ b/arch/x86/cpu/mtrr.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
  * (C) Copyright 2014 Google, Inc
+ * Portions added from coreboot
  *
  * Memory Type Range Regsters - these are used to tell the CPU whether
  * memory is cacheable and if so the cache write mode to use.
@@ -204,29 +205,80 @@ int mtrr_commit(bool do_caches)
 	return 0;
 }
 
-int mtrr_add_request(int type, uint64_t start, uint64_t size)
+/* fms: find most significant bit set (from Linux)  */
+static inline uint fms(uint val)
+{
+	uint ret;
+
+	__asm__("bsrl %1,%0\n\t"
+		"jnz 1f\n\t"
+		"movl $0,%0\n"
+		"1:" : "=r" (ret) : "mr" (val));
+
+	return ret;
+}
+
+/*
+ * fms64: find most significant bit set in a 64-bit word
+ * As samples, fms64(0x0) = 0; fms64(0x4400) = 14;
+ * fms64(0x40400000000) = 42.
+ */
+static uint fms64(uint64_t val)
+{
+	u32 hi = (u32)(val >> 32);
+
+	if (!hi)
+		return fms((u32)val);
+
+	return fms(hi) + 32;
+}
+
+int mtrr_add_request(int type, u64 base, uint64_t size)
 {
 	struct mtrr_request *req;
-	uint64_t mask;
+	u64 mask;
 
 	debug("%s: count=%d\n", __func__, gd->arch.mtrr_req_count);
 	if (!gd->arch.has_mtrr)
 		return -ENOSYS;
 
-	if (!is_power_of_2(size))
-		return -EINVAL;
-
-	if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS)
-		return -ENOSPC;
-	req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++];
-	req->type = type;
-	req->start = start;
-	req->size = size;
-	debug("%d: type=%d, %08llx  %08llx\n", gd->arch.mtrr_req_count - 1,
-	      req->type, req->start, req->size);
-	mask = mtrr_to_mask(req->size);
-	mask |= MTRR_PHYS_MASK_VALID;
-	debug("   %016llx %016llx\n", req->start | req->type, mask);
+	while (size) {
+		uint addr_lsb;
+		uint size_msb;
+		u64 mtrr_size;
+
+		addr_lsb = fls64(base);
+		size_msb = fms64(size);
+
+		/*
+		 * All MTRR entries need to have their base aligned to the
+		 * mask size. The maximum size is calculated by a function of
+		 * the min base bit set and maximum size bit set.
+		 * Algorithm is from coreboot
+		 */
+		if (!addr_lsb || addr_lsb > size_msb)
+			mtrr_size = 1ull << size_msb;
+		else
+			mtrr_size = 1ull << addr_lsb;
+		log_debug("addr_lsb %x size_msb %x mtrr_size %llx\n",
+			  addr_lsb, size_msb, mtrr_size);
+
+		if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS)
+			return -ENOSPC;
+		req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++];
+		req->type = type;
+		req->start = base;
+		req->size = mtrr_size;
+		log_debug("%d: type=%d, %08llx  %08llx ",
+			  gd->arch.mtrr_req_count - 1, req->type, req->start,
+			  req->size);
+		mask = mtrr_to_mask(req->size);
+		mask |= MTRR_PHYS_MASK_VALID;
+		log_debug("   %016llx %016llx\n", req->start | req->type, mask);
+
+		size -= mtrr_size;
+		base += mtrr_size;
+	}
 
 	return 0;
 }
-- 
2.43.0

[PATCH v5 27/46] x86: emulation: Set an MTRR for the RAM

[Simon Glass]

QEMU likes to have an MTRR set up, just like real machines. Add an MTRR
which covers the total RAM size.

This does nothing on machines without MTRRs.

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

(no changes since v3)

Changes in v3:
- Support memory sizes above 3GB properly

Changes in v2:
- Add new patch to set an MTRR for the RAM in QEMU

 arch/x86/cpu/qemu/dram.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/cpu/qemu/dram.c b/arch/x86/cpu/qemu/dram.c
index 62a301c0fd3..ba3638e6acc 100644
--- a/arch/x86/cpu/qemu/dram.c
+++ b/arch/x86/cpu/qemu/dram.c
@@ -4,7 +4,9 @@
  */
 
 #include <init.h>
+#include <spl.h>
 #include <asm/global_data.h>
+#include <asm/mtrr.h>
 #include <asm/post.h>
 #include <asm/arch/qemu.h>
 #include <linux/sizes.h>
@@ -44,6 +46,22 @@ int dram_init(void)
 	gd->ram_size += qemu_get_high_memory_size();
 	post_code(POST_DRAM);
 
+	if (xpl_phase() == PHASE_BOARD_F) {
+		u64 total = gd->ram_size;
+		int ret;
+
+		if (total > SZ_2G + SZ_1G)
+			total += SZ_1G;
+		ret = mtrr_add_request(MTRR_TYPE_WRBACK, 0, total);
+		if (ret != -ENOSYS) {
+			if (ret)
+				return log_msg_ret("mta", ret);
+			ret = mtrr_commit(false);
+			if (ret)
+				return log_msg_ret("mtc", ret);
+		}
+	}
+
 	return 0;
 }
 
-- 
2.43.0

[PATCH v5 28/46] x86: qemu: Support environment and cat command

[Simon Glass]

Add support for an environment stored in the first partition of the
disk, which is assumed to hold a FAT filesystem.

Support the 'cat' command as it is useful for looking at extlinux.conf
files.

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

(no changes since v1)

 configs/qemu-x86_64_defconfig | 6 ++++--
 configs/qemu-x86_defconfig    | 4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
index 84b7dc8e99f..ae34cc2a6f8 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -41,9 +41,7 @@ CONFIG_SPL_NO_BSS_LIMIT=y
 CONFIG_SPL_BOARD_INIT=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_CPU=y
-CONFIG_SPL_ENV_SUPPORT=y
 CONFIG_SPL_DM_SPI_FLASH=y
-CONFIG_SPL_NET=y
 CONFIG_SPL_PCI=y
 CONFIG_SPL_PCH=y
 CONFIG_SPL_RTC=y
@@ -54,6 +52,7 @@ CONFIG_CMD_MEM_SEARCH=y
 CONFIG_CMD_IDE=y
 CONFIG_CMD_SPI=y
 CONFIG_CMD_USB=y
+CONFIG_CMD_CAT=y
 # CONFIG_CMD_SETEXPR is not set
 CONFIG_BOOTP_BOOTFILESIZE=y
 CONFIG_CMD_EFIDEBUG=y
@@ -61,6 +60,9 @@ CONFIG_CMD_TIME=y
 CONFIG_CMD_BOOTSTAGE=y
 CONFIG_CMD_EXT4_WRITE=y
 CONFIG_ENV_OVERWRITE=y
+CONFIG_ENV_IS_IN_FAT=y
+CONFIG_ENV_FAT_INTERFACE="virtio"
+CONFIG_ENV_FAT_DEVICE_AND_PART="0:1"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_USE_BOOTFILE=y
 CONFIG_BOOTFILE="bzImage"
diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
index d52afa42955..7afa3827e61 100644
--- a/configs/qemu-x86_defconfig
+++ b/configs/qemu-x86_defconfig
@@ -34,6 +34,7 @@ CONFIG_CMD_MEM_SEARCH=y
 CONFIG_CMD_IDE=y
 CONFIG_CMD_SPI=y
 CONFIG_CMD_USB=y
+CONFIG_CMD_CAT=y
 # CONFIG_CMD_SETEXPR is not set
 CONFIG_BOOTP_BOOTFILESIZE=y
 CONFIG_CMD_EFIDEBUG=y
@@ -42,6 +43,9 @@ CONFIG_CMD_BOOTSTAGE=y
 CONFIG_CMD_EXT4_WRITE=y
 CONFIG_MAC_PARTITION=y
 CONFIG_ENV_OVERWRITE=y
+CONFIG_ENV_IS_IN_FAT=y
+CONFIG_ENV_FAT_INTERFACE="virtio"
+CONFIG_ENV_FAT_DEVICE_AND_PART="0:1"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_USE_BOOTFILE=y
 CONFIG_BOOTFILE="bzImage"
-- 
2.43.0

[PATCH v5 29/46] test/py: Correct sizing of created disks

[Simon Glass]

At present the disks end up being 1MB shorter than they should be,
since dd truncates by default.

Move the code into a function and update it to avoid truncation.

This resolves various warnings when running sandbox tests, of the form:

   mmc_bread() MMC: block number 0x9801 exceeds max(0x9800)

caused by the FAT partition being scanning.

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

(no changes since v3)

Changes in v3:
- Add new patch to correct sizing of created disks

 test/py/tests/test_ut.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py
index ea0c43cd4fc..b8adb597e11 100644
--- a/test/py/tests/test_ut.py
+++ b/test/py/tests/test_ut.py
@@ -57,6 +57,17 @@ def setup_image(ubman, devnum, part_type, img_size=20, second_part=False,
                              stdin=spec.encode('utf-8'))
     return fname, mnt
 
+def copy_partition(ubman, fsfile, outname):
+    """Copy a partition into a disk iamge
+
+    Args:
+        ubman (ConsoleBase): U-Boot fixture
+        fsfile (str): Name of partition file
+        outname (str): Name of full-disk file to update
+    """
+    utils.run_and_log(ubman,
+                      f'dd if={fsfile} of={outname} bs=1M seek=1 conv=notrunc')
+
 def setup_bootmenu_image(ubman):
     """Create a 20MB disk image with a single ext4 partition
 
@@ -172,7 +183,7 @@ booti ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}
     fsfile = 'ext18M.img'
     utils.run_and_log(ubman, f'fallocate -l 18M {fsfile}')
     utils.run_and_log(ubman, f'mkfs.ext4 {fsfile} -d {mnt}')
-    utils.run_and_log(ubman, f'dd if={fsfile} of={fname} bs=1M seek=1')
+    copy_partition(ubman, fsfile, fname)
     utils.run_and_log(ubman, f'rm -rf {mnt}')
     utils.run_and_log(ubman, f'rm -f {fsfile}')
 
@@ -224,7 +235,7 @@ label Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
     utils.run_and_log(ubman, f'fallocate -l 18M {fsfile}')
     utils.run_and_log(ubman, f'mkfs.vfat {fsfile}')
     utils.run_and_log(ubman, ['sh', '-c', f'mcopy -i {fsfile} {mnt}/* ::/'])
-    utils.run_and_log(ubman, f'dd if={fsfile} of={fname} bs=1M seek=1')
+    copy_partition(ubman, fsfile, fname)
     utils.run_and_log(ubman, f'rm -rf {mnt}')
     utils.run_and_log(ubman, f'rm -f {fsfile}')
 
@@ -562,7 +573,7 @@ def setup_efi_image(ubman):
     utils.run_and_log(ubman, f'fallocate -l 18M {fsfile}')
     utils.run_and_log(ubman, f'mkfs.vfat {fsfile}')
     utils.run_and_log(ubman, ['sh', '-c', f'mcopy -vs -i {fsfile} {mnt}/* ::/'])
-    utils.run_and_log(ubman, f'dd if={fsfile} of={fname} bs=1M seek=1')
+    copy_partition(ubman, fsfile, fname)
     utils.run_and_log(ubman, f'rm -rf {mnt}')
     utils.run_and_log(ubman, f'rm -f {fsfile}')
 
-- 
2.43.0

[PATCH v5 30/46] test/py: Add a helper to send characters

[Simon Glass]

The existing run_command() method is not great for sending things other
than U-Boot commands. Add a helper for sending arbitrary strings as well
as control characters.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

(no changes since v2)

Changes in v2:
- Add new patch with a helper to send characters

 test/py/console_base.py | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/test/py/console_base.py b/test/py/console_base.py
index b080c389547..073ab0462dc 100644
--- a/test/py/console_base.py
+++ b/test/py/console_base.py
@@ -369,21 +369,30 @@ class ConsoleBase(object):
             output.append(self.run_command(cmd))
         return output
 
-    def ctrlc(self):
-        """Send a CTRL-C character to U-Boot.
+    def send(self, msg):
+        """Send characters without waiting for echo, etc."""
+        self.run_command(msg, wait_for_prompt=False, wait_for_echo=False,
+                         send_nl=False)
+
+    def ctrl(self, char):
+        """Send a CTRL- character to U-Boot.
 
         This is useful in order to stop execution of long-running synchronous
         commands such as "ums".
 
         Args:
-            None.
-
-        Returns:
-            Nothing.
+            char (str): Character to send, e.g. 'C' to send Ctrl-C
         """
+        self.log.action(f'Sending Ctrl-{char}')
+        self.send(chr(ord(char) - ord('@')))
 
-        self.log.action('Sending Ctrl-C')
-        self.run_command(chr(3), wait_for_echo=False, send_nl=False)
+    def ctrlc(self):
+        """Send a CTRL-C character to U-Boot.
+
+        This is useful in order to stop execution of long-running synchronous
+        commands such as "ums".
+        """
+        self.ctrl('C')
 
     def wait_for(self, text):
         """Wait for a pattern to be emitted by U-Boot.
-- 
2.43.0

[PATCH v5 31/46] test/py: Allow tests to be filtered by role

[Simon Glass]

Some test can only be run by a particular board in a lab, e.g. because
they are loaded with an OS image used by the test. Add a way to specify
this in tests.

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

(no changes since v2)

Changes in v2:
- Add new patch to allow tests to be filtered by role

 test/py/conftest.py | 22 ++++++++++++++++++++++
 test/py/pytest.ini  |  1 +
 2 files changed, 23 insertions(+)

diff --git a/test/py/conftest.py b/test/py/conftest.py
index e59897c1f78..5aea85647af 100644
--- a/test/py/conftest.py
+++ b/test/py/conftest.py
@@ -334,6 +334,7 @@ def pytest_configure(config):
     ubconfig.dtb = build_dir + '/arch/sandbox/dts/test.dtb'
     ubconfig.connection_ok = True
     ubconfig.timing = config.getoption('timing')
+    ubconfig.role = config.getoption('role')
 
     env_vars = (
         'board_type',
@@ -760,6 +761,26 @@ def setup_singlethread(item):
         if worker_id and worker_id != 'master':
             pytest.skip('must run single-threaded')
 
+def setup_role(item):
+    """Process any 'role' marker for a test.
+
+    Skip this test if the role does not match.
+
+    Args:
+        item (pytest.Item): The pytest test item
+    """
+    required_roles = []
+    for roles in item.iter_markers('role'):
+        role = roles.args[0]
+        if role.startswith('!'):
+            if ubconfig.role == role[1:]:
+                pytest.skip(f'role "{ubconfig.role}" not supported')
+                return
+        else:
+            required_roles.append(role)
+    if required_roles and ubconfig.role not in required_roles:
+        pytest.skip(f'board "{ubconfig.role}" not supported')
+
 def start_test_section(item):
     anchors[item.name] = log.start_section(item.name)
 
@@ -781,6 +802,7 @@ def pytest_runtest_setup(item):
     setup_buildconfigspec(item)
     setup_requiredtool(item)
     setup_singlethread(item)
+    setup_role(item)
 
 def pytest_runtest_protocol(item, nextitem):
     """pytest hook: Called to execute a test.
diff --git a/test/py/pytest.ini b/test/py/pytest.ini
index 26d83f83e00..361be0178ee 100644
--- a/test/py/pytest.ini
+++ b/test/py/pytest.ini
@@ -12,3 +12,4 @@ markers =
     requiredtool: U-Boot: Required host tools for a test.
     slow: U-Boot: Specific test will run slowly.
     singlethread: Cannot run in parallel
+    role: U-Boot: Indicates the lab 'role' which can execute this test
-- 
2.43.0

[PATCH v5 32/46] x86: e820: Add a function to dump the e820 table

[Simon Glass]

There is already code for this in zimage. Move it to the e820 file so
it can be used elsewhere.

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

(no changes since v3)

Changes in v3:
- Add new patch with a function to dump the e820 table

 arch/x86/include/asm/e820.h |  8 ++++++++
 arch/x86/lib/e820.c         | 23 +++++++++++++++++++++++
 arch/x86/lib/zimage.c       | 22 ++--------------------
 3 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/e820.h b/arch/x86/include/asm/e820.h
index 1ab709abfc8..cc3758b59fd 100644
--- a/arch/x86/include/asm/e820.h
+++ b/arch/x86/include/asm/e820.h
@@ -39,6 +39,14 @@ unsigned int install_e820_map(unsigned int max_entries,
 unsigned int cb_install_e820_map(unsigned int max_entries,
 				 struct e820_entry *entries);
 
+/**
+ * e820_dump() - Dump an e820 table
+ *
+ * @entries: Pointer to first entry
+ * @count: Number of entries in the table
+ */
+void e820_dump(struct e820_entry *entries, uint count);
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_X86_E820_H */
diff --git a/arch/x86/lib/e820.c b/arch/x86/lib/e820.c
index d478b7486e3..00ca94776d2 100644
--- a/arch/x86/lib/e820.c
+++ b/arch/x86/lib/e820.c
@@ -10,6 +10,29 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static const char *const e820_type_name[E820_COUNT] = {
+	[E820_RAM] = "RAM",
+	[E820_RESERVED] = "Reserved",
+	[E820_ACPI] = "ACPI",
+	[E820_NVS] = "ACPI NVS",
+	[E820_UNUSABLE] = "Unusable",
+};
+
+void e820_dump(struct e820_entry *entries, uint count)
+{
+	int i;
+
+	printf("%12s  %10s  %s\n", "Addr", "Size", "Type");
+	for (i = 0; i < count; i++) {
+		struct e820_entry *entry = &entries[i];
+
+		printf("%12llx  %10llx  %s\n", entry->addr, entry->size,
+		       entry->type < E820_COUNT ?
+		       e820_type_name[entry->type] :
+		       simple_itoa(entry->type));
+	}
+}
+
 /*
  * Install a default e820 table with 4 entries as follows:
  *
diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
index 26d623711bc..3ffc7593f87 100644
--- a/arch/x86/lib/zimage.c
+++ b/arch/x86/lib/zimage.c
@@ -475,14 +475,6 @@ static void print_num64(const char *name, u64 value)
 	printf("%-20s: %llx\n", name, value);
 }
 
-static const char *const e820_type_name[E820_COUNT] = {
-	[E820_RAM] = "RAM",
-	[E820_RESERVED] = "Reserved",
-	[E820_ACPI] = "ACPI",
-	[E820_NVS] = "ACPI NVS",
-	[E820_UNUSABLE] = "Unusable",
-};
-
 static const char *const bootloader_id[] = {
 	"LILO",
 	"Loadlin",
@@ -570,24 +562,14 @@ void zimage_dump(struct bootm_info *bmi, bool show_cmdline)
 {
 	struct boot_params *base_ptr;
 	struct setup_header *hdr;
-	int i;
 
 	base_ptr = bmi->base_ptr;
 	printf("Setup located at %p:\n\n", base_ptr);
 	print_num64("ACPI RSDP addr", base_ptr->acpi_rsdp_addr);
 
 	printf("E820: %d entries\n", base_ptr->e820_entries);
-	if (base_ptr->e820_entries) {
-		printf("%12s  %10s  %s\n", "Addr", "Size", "Type");
-		for (i = 0; i < base_ptr->e820_entries; i++) {
-			struct e820_entry *entry = &base_ptr->e820_map[i];
-
-			printf("%12llx  %10llx  %s\n", entry->addr, entry->size,
-			       entry->type < E820_COUNT ?
-			       e820_type_name[entry->type] :
-			       simple_itoa(entry->type));
-		}
-	}
+	if (base_ptr->e820_entries)
+		e820_dump(base_ptr->e820_map, base_ptr->e820_entries);
 
 	hdr = &base_ptr->hdr;
 	print_num("Setup sectors", hdr->setup_sects);
-- 
2.43.0

[PATCH v5 33/46] x86: Add a new API for e820

[Simon Glass]

The existing mechanism is pretty painful as it requires manual
calculations for anything but a trivial setup.

Add a new API for adding e820 entries.

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

(no changes since v3)

Changes in v3:
- Add new patch with an API for e820

 arch/x86/include/asm/e820.h | 87 ++++++++++++++++++++++++++++++++++++-
 arch/x86/lib/e820.c         | 47 ++++++++++++++++++++
 2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/e820.h b/arch/x86/include/asm/e820.h
index cc3758b59fd..a535818b2d5 100644
--- a/arch/x86/include/asm/e820.h
+++ b/arch/x86/include/asm/e820.h
@@ -3,6 +3,8 @@
 
 #define E820MAX		128	/* number of entries in E820MAP */
 
+#ifdef __ASSEMBLY__
+
 #define E820_RAM	1
 #define E820_RESERVED	2
 #define E820_ACPI	3
@@ -10,9 +12,21 @@
 #define E820_UNUSABLE	5
 #define E820_COUNT	6	/* Number of types */
 
-#ifndef __ASSEMBLY__
+#else
+
 #include <linux/types.h>
 
+/* Available e820 memory-region types */
+enum e820_type {
+	E820_RAM	= 1,
+	E820_RESERVED,
+	E820_ACPI,
+	E820_NVS,
+	E820_UNUSABLE,
+
+	E820_COUNT,
+};
+
 struct e820_entry {
 	__u64 addr;	/* start of memory segment */
 	__u64 size;	/* size of memory segment */
@@ -22,10 +36,81 @@ struct e820_entry {
 #define ISA_START_ADDRESS	0xa0000
 #define ISA_END_ADDRESS		0x100000
 
+/**
+ * Context to use for e820_add()
+ *
+ * @entries: Table being filled in
+ * @addr: Current address we are up to
+ * @count: Number of entries added to @entries so far
+ * @max_entries: Maximum number of entries allowed
+ */
+struct e820_ctx {
+	struct e820_entry *entries;
+	u64 addr;
+	int count;
+	int max_entries;
+};
+
+/**
+ * e820_init() - Start setting up an e820 table
+ *
+ * @ctx: Context to set up
+ * @entries: Place to put entries
+ * @max_entries: Maximum size of @entries
+ */
+void e820_init(struct e820_ctx *ctx, struct e820_entry *entries,
+	       int max_entries);
+
+/**
+ * e820_add() - Add an entry to the table
+ *
+ * @ctx: Context
+ * @type: Type of entry
+ * @addr: Start address of entry
+ * @size Size of entry
+ */
+void e820_add(struct e820_ctx *ctx, enum e820_type type, u64 addr, u64 size);
+
+/**
+ * e820_to_addr() - Add an entry that covers the space up to a given address
+ *
+ * @ctx: Context
+ * @type: Type of entry
+ * @end_addr: Address where the entry should finish
+ */
+void e820_to_addr(struct e820_ctx *ctx, enum e820_type type, u64 end_addr);
+
+/**
+ * e820_next() - Add an entry that carries on from the last one
+ *
+ * @ctx: Context
+ * @type: Type of entry
+ * @size Size of entry
+ */
+void e820_next(struct e820_ctx *ctx, enum e820_type type, u64 size);
+
+/**
+ * e820_finish() - Finish the table
+ *
+ * Checks the table is not too large, panics if so
+ *
+ * @ctx: Context
+ * Returns: Number of entries
+ */
+int e820_finish(struct e820_ctx *ctx);
+
 /* Implementation-defined function to install an e820 map */
 unsigned int install_e820_map(unsigned int max_entries,
 			      struct e820_entry *);
 
+/**
+ * e820_dump() - Dump the e820 table
+ *
+ * @entries: Pointer to start of table
+ * @count: Number of entries in the table
+ */
+void e820_dump(struct e820_entry *entries, uint count);
+
 /**
  * cb_install_e820_map() - Install e820 map provided by coreboot sysinfo
  *
diff --git a/arch/x86/lib/e820.c b/arch/x86/lib/e820.c
index 00ca94776d2..bcc5f6f3044 100644
--- a/arch/x86/lib/e820.c
+++ b/arch/x86/lib/e820.c
@@ -3,8 +3,11 @@
  * Copyright (C) 2015, Bin Meng <bmeng.cn@gmail.com>
  */
 
+#define LOG_CATEGORY	LOGC_ARCH
+
 #include <efi_loader.h>
 #include <lmb.h>
+#include <log.h>
 #include <asm/e820.h>
 #include <asm/global_data.h>
 
@@ -60,6 +63,50 @@ __weak unsigned int install_e820_map(unsigned int max_entries,
 	return 4;
 }
 
+void e820_init(struct e820_ctx *ctx, struct e820_entry *entries,
+	       int max_entries)
+{
+	memset(ctx, '\0', sizeof(*ctx));
+	ctx->entries = entries;
+	ctx->max_entries = max_entries;
+}
+
+void e820_add(struct e820_ctx *ctx, enum e820_type type, u64 addr, u64 size)
+{
+	struct e820_entry *entry = &ctx->entries[ctx->count++];
+
+	if (ctx->count <= ctx->max_entries) {
+		entry->addr = addr;
+		entry->size = size;
+		entry->type = type;
+	}
+	ctx->addr = addr + size;
+}
+
+void e820_next(struct e820_ctx *ctx, enum e820_type type, u64 size)
+{
+	e820_add(ctx, type, ctx->addr, size);
+}
+
+void e820_to_addr(struct e820_ctx *ctx, enum e820_type type, u64 addr)
+{
+	e820_next(ctx, type, addr - ctx->addr);
+}
+
+int e820_finish(struct e820_ctx *ctx)
+{
+	if (ctx->count > ctx->max_entries) {
+		printf("e820 has %d entries but room for only %d\n", ctx->count,
+		       ctx->max_entries);
+		panic("e820 table too large");
+	}
+	log_debug("e820 map installed, n=%d\n", ctx->count);
+	if (_DEBUG)
+		e820_dump(ctx->entries, ctx->count);
+
+	return ctx->count;
+}
+
 #if CONFIG_IS_ENABLED(EFI_LOADER)
 void efi_add_known_memory(void)
 {
-- 
2.43.0

[PATCH v5 34/46] x86: qemu: Use the new e820 API

[Simon Glass]

Move over to use this API before making the code even more complicated.

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

(no changes since v3)

Changes in v3:
- Add new patch to use the new e820 API

 arch/x86/cpu/qemu/e820.c | 48 ++++++++++------------------------------
 1 file changed, 12 insertions(+), 36 deletions(-)

diff --git a/arch/x86/cpu/qemu/e820.c b/arch/x86/cpu/qemu/e820.c
index 17a04f86479..2b0ad179489 100644
--- a/arch/x86/cpu/qemu/e820.c
+++ b/arch/x86/cpu/qemu/e820.c
@@ -19,51 +19,27 @@ unsigned int install_e820_map(unsigned int max_entries,
 			      struct e820_entry *entries)
 {
 	u64 high_mem_size;
-	int n = 0;
+	struct e820_ctx ctx;
 
-	entries[n].addr = 0;
-	entries[n].size = ISA_START_ADDRESS;
-	entries[n].type = E820_RAM;
-	n++;
+	e820_init(&ctx, entries, max_entries);
 
-	entries[n].addr = ISA_START_ADDRESS;
-	entries[n].size = ISA_END_ADDRESS - ISA_START_ADDRESS;
-	entries[n].type = E820_RESERVED;
-	n++;
+	e820_next(&ctx, E820_RAM, ISA_START_ADDRESS);
+	e820_next(&ctx, E820_RESERVED, ISA_END_ADDRESS);
 
 	/*
 	 * since we use memalign(malloc) to allocate high memory for
 	 * storing ACPI tables, we need to reserve them in e820 tables,
 	 * otherwise kernel will reclaim them and data will be corrupted
 	 */
-	entries[n].addr = ISA_END_ADDRESS;
-	entries[n].size = gd->relocaddr - TOTAL_MALLOC_LEN - ISA_END_ADDRESS;
-	entries[n].type = E820_RAM;
-	n++;
-
-	/* for simplicity, reserve entire malloc space */
-	entries[n].addr = gd->relocaddr - TOTAL_MALLOC_LEN;
-	entries[n].size = TOTAL_MALLOC_LEN;
-	entries[n].type = E820_RESERVED;
-	n++;
-
-	entries[n].addr = gd->relocaddr;
-	entries[n].size = qemu_get_low_memory_size() - gd->relocaddr;
-	entries[n].type = E820_RESERVED;
-	n++;
-
-	entries[n].addr = CONFIG_PCIE_ECAM_BASE;
-	entries[n].size = CONFIG_PCIE_ECAM_SIZE;
-	entries[n].type = E820_RESERVED;
-	n++;
+	e820_to_addr(&ctx, E820_RAM, gd->relocaddr - TOTAL_MALLOC_LEN);
+	e820_next(&ctx, E820_RESERVED, TOTAL_MALLOC_LEN);
+	e820_to_addr(&ctx, E820_RAM, qemu_get_low_memory_size());
+	e820_add(&ctx, E820_RESERVED, CONFIG_PCIE_ECAM_BASE,
+		 CONFIG_PCIE_ECAM_SIZE);
 
 	high_mem_size = qemu_get_high_memory_size();
-	if (high_mem_size) {
-		entries[n].addr = SZ_4G;
-		entries[n].size = high_mem_size;
-		entries[n].type = E820_RAM;
-		n++;
-	}
+	if (high_mem_size)
+		e820_add(&ctx, E820_RAM, SZ_4G, high_mem_size);
 
-	return n;
+	return e820_finish(&ctx);
 }
-- 
2.43.0

[PATCH v5 35/46] x86: emulation: Support BLOBLIST_TABLES properly

[Simon Glass]

The existing QEMU implementation mostly ignored BLOBLIST_TABLES and
allocates the bulk of the tables with malloc(). Update it to place all
tables in the bloblist. Since QEMU declares a size of 128KB regardless
of the size of its tables, this requires a larger bloblist.

Fix up the e820 table to handle this, keeping the old code as an option
for now, to assist with any future bug-fixing.

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

(no changes since v3)

Changes in v3:
- Add new patch to support BLOBLIST_TABLES properly in QEMU

 arch/x86/cpu/qemu/e820.c      | 18 +++++++++++++-----
 arch/x86/lib/tables.c         |  9 ++++++++-
 configs/qemu-x86_64_defconfig |  2 +-
 configs/qemu-x86_defconfig    |  2 +-
 drivers/misc/qfw_acpi.c       | 32 +++++++++++++++++++++++++++-----
 5 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/arch/x86/cpu/qemu/e820.c b/arch/x86/cpu/qemu/e820.c
index 2b0ad179489..078d1d86b02 100644
--- a/arch/x86/cpu/qemu/e820.c
+++ b/arch/x86/cpu/qemu/e820.c
@@ -6,6 +6,7 @@
  * (C) Copyright 2019 Bin Meng <bmeng.cn@gmail.com>
  */
 
+#include <bloblist.h>
 #include <env_internal.h>
 #include <malloc.h>
 #include <asm/e820.h>
@@ -27,12 +28,19 @@ unsigned int install_e820_map(unsigned int max_entries,
 	e820_next(&ctx, E820_RESERVED, ISA_END_ADDRESS);
 
 	/*
-	 * since we use memalign(malloc) to allocate high memory for
-	 * storing ACPI tables, we need to reserve them in e820 tables,
-	 * otherwise kernel will reclaim them and data will be corrupted
+	 * if we use bloblist to allocate high memory for storing ACPI tables,
+	 * we need to reserve that region in e820 tables, otherwise the kernel
+	 * will reclaim them and data will be corrupted. The ACPI tables may not
+	 * have been written yet, so use the whole bloblist size
 	 */
-	e820_to_addr(&ctx, E820_RAM, gd->relocaddr - TOTAL_MALLOC_LEN);
-	e820_next(&ctx, E820_RESERVED, TOTAL_MALLOC_LEN);
+	if (IS_ENABLED(CONFIG_BLOBLIST_TABLES)) {
+		e820_to_addr(&ctx, E820_RAM, (ulong)gd->bloblist);
+		e820_next(&ctx, E820_ACPI, bloblist_get_total_size());
+	} else {
+		/* If using memalign() reserve that whole region instead */
+		e820_to_addr(&ctx, E820_RAM, gd->relocaddr - TOTAL_MALLOC_LEN);
+		e820_next(&ctx, E820_ACPI, TOTAL_MALLOC_LEN);
+	}
 	e820_to_addr(&ctx, E820_RAM, qemu_get_low_memory_size());
 	e820_add(&ctx, E820_RESERVED, CONFIG_PCIE_ECAM_BASE,
 		 CONFIG_PCIE_ECAM_SIZE);
diff --git a/arch/x86/lib/tables.c b/arch/x86/lib/tables.c
index 44fe80c5224..ec52992209f 100644
--- a/arch/x86/lib/tables.c
+++ b/arch/x86/lib/tables.c
@@ -45,6 +45,13 @@ struct table_info {
 	int align;
 };
 
+/* QEMU's tables include quite a bit of empty space */
+#ifdef CONFIG_QEMU
+#define ACPI_SIZE	(192 << 10)
+#else
+#define ACPI_SIZE	SZ_64K
+#endif
+
 static struct table_info table_list[] = {
 #ifdef CONFIG_GENERATE_PIRQ_TABLE
 	{ "pirq", write_pirq_routing_table },
@@ -60,7 +67,7 @@ static struct table_info table_list[] = {
 	 * that the calculation of gd->table_end works properly
 	 */
 #ifdef CONFIG_GENERATE_ACPI_TABLE
-	{ "acpi", write_acpi_tables, BLOBLISTT_ACPI_TABLES, SZ_64K, SZ_4K},
+	{ "acpi", write_acpi_tables, BLOBLISTT_ACPI_TABLES, ACPI_SIZE, SZ_4K},
 #endif
 #ifdef CONFIG_GENERATE_SMBIOS_TABLE
 	/*
diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
index ae34cc2a6f8..58b1fbf132d 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -1,7 +1,7 @@
 CONFIG_X86=y
 CONFIG_TEXT_BASE=0x1110000
 CONFIG_SYS_MALLOC_F_LEN=0x1800
-CONFIG_BLOBLIST_SIZE_RELOC=0x20000
+CONFIG_BLOBLIST_SIZE_RELOC=0x40000
 CONFIG_NR_DRAM_BANKS=8
 CONFIG_ENV_SIZE=0x40000
 CONFIG_MAX_CPUS=2
diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
index 7afa3827e61..099d3f32d1b 100644
--- a/configs/qemu-x86_defconfig
+++ b/configs/qemu-x86_defconfig
@@ -1,7 +1,7 @@
 CONFIG_X86=y
 CONFIG_TEXT_BASE=0xFFF00000
 CONFIG_SYS_MALLOC_F_LEN=0x1000
-CONFIG_BLOBLIST_SIZE_RELOC=0x20000
+CONFIG_BLOBLIST_SIZE_RELOC=0x40000
 CONFIG_NR_DRAM_BANKS=8
 CONFIG_ENV_SIZE=0x40000
 CONFIG_MAX_CPUS=2
diff --git a/drivers/misc/qfw_acpi.c b/drivers/misc/qfw_acpi.c
index 0d0cf764689..ec00ba65ade 100644
--- a/drivers/misc/qfw_acpi.c
+++ b/drivers/misc/qfw_acpi.c
@@ -25,17 +25,18 @@ DECLARE_GLOBAL_DATA_PTR;
  *
  * @entry : BIOS linker command entry which tells where to allocate memory
  *          (either high memory or low memory)
- * @addr  : The address that should be used for low memory allcation. If the
+ * @addr  : The address that should be used for low memory allocation. If the
  *          memory allocation request is 'ZONE_HIGH' then this parameter will
  *          be ignored.
  * @return: 0 on success, or negative value on failure
  */
-static int bios_linker_allocate(struct udevice *dev,
+static int bios_linker_allocate(struct acpi_ctx *ctx, struct udevice *dev,
 				struct bios_linker_entry *entry, ulong *addr)
 {
 	uint32_t size, align;
 	struct fw_file *file;
 	unsigned long aligned_addr;
+	struct acpi_rsdp *rsdp;
 
 	align = le32_to_cpu(entry->alloc.align);
 	/* align must be power of 2 */
@@ -58,7 +59,9 @@ static int bios_linker_allocate(struct udevice *dev,
 	 * If allocation zone is ZONE_FSEG, then we use the 'addr' passed
 	 * in which is low memory
 	 */
-	if (entry->alloc.zone == BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH) {
+	if (IS_ENABLED(CONFIG_BLOBLIST_TABLES)) {
+		aligned_addr = ALIGN(*addr, align);
+	} else if (entry->alloc.zone == BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH) {
 		aligned_addr = (unsigned long)memalign(align, size);
 		if (!aligned_addr) {
 			printf("error: allocating resource\n");
@@ -83,8 +86,13 @@ static int bios_linker_allocate(struct udevice *dev,
 		       (void *)aligned_addr);
 	file->addr = aligned_addr;
 
+	rsdp = (void *)aligned_addr;
+	if (!strncmp(rsdp->signature, RSDP_SIG, sizeof(rsdp->signature)))
+		ctx->rsdp = rsdp;
+
 	/* adjust address for low memory allocation */
-	if (entry->alloc.zone == BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG)
+	if (IS_ENABLED(CONFIG_BLOBLIST_TABLES) ||
+	    entry->alloc.zone == BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG)
 		*addr = (aligned_addr + size);
 
 	return 0;
@@ -209,19 +217,23 @@ ulong write_acpi_tables(ulong addr)
 	qfw_read_entry(dev, be16_to_cpu(file->cfg.select), size, table_loader);
 
 	for (i = 0; i < (size / sizeof(*entry)); i++) {
+		log_content("entry %d: addr %lx\n", i, addr);
 		entry = table_loader + i;
 		switch (le32_to_cpu(entry->command)) {
 		case BIOS_LINKER_LOADER_COMMAND_ALLOCATE:
-			ret = bios_linker_allocate(dev, entry, &addr);
+			log_content("   - %s\n", entry->alloc.file);
+			ret = bios_linker_allocate(ctx, dev, entry, &addr);
 			if (ret)
 				goto out;
 			break;
 		case BIOS_LINKER_LOADER_COMMAND_ADD_POINTER:
+			log_content("   - %s\n", entry->pointer.src_file);
 			ret = bios_linker_add_pointer(dev, entry);
 			if (ret)
 				goto out;
 			break;
 		case BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM:
+			log_content("   - %s\n", entry->cksum.file);
 			ret = bios_linker_add_checksum(dev, entry);
 			if (ret)
 				goto out;
@@ -246,6 +258,16 @@ out:
 
 	free(table_loader);
 
+	if (!ctx->rsdp) {
+		printf("error: no RSDP found\n");
+		return addr;
+	}
+	struct acpi_rsdp *rsdp = ctx->rsdp;
+
+	rsdp->length = sizeof(*rsdp);
+	rsdp->xsdt_address = 0;
+	rsdp->ext_checksum = table_compute_checksum((u8 *)rsdp, sizeof(*rsdp));
+
 	gd_set_acpi_start(acpi_get_rsdp_addr());
 
 	return addr;
-- 
2.43.0

[PATCH v5 36/46] x86: Support a 64-bit ramdisk address

[Simon Glass]

Add some missing pieces to bootparams so that a 64-bit ramdisk address
can be used. Tidy up the logging while we are here.

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

(no changes since v3)

Changes in v3:
- Add new patch to support a 64-bit ramdisk address

 arch/x86/include/asm/bootparam.h | 15 ++++++++++++++-
 arch/x86/lib/zimage.c            | 12 +++++++-----
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/bootparam.h b/arch/x86/include/asm/bootparam.h
index ac4865300f1..657d920b14f 100644
--- a/arch/x86/include/asm/bootparam.h
+++ b/arch/x86/include/asm/bootparam.h
@@ -122,6 +122,14 @@ struct efi_info {
 	__u32 efi_memmap_hi;
 };
 
+/* Gleaned from OFW's set-parameters in cpu/x86/pc/linux.fth */
+struct olpc_ofw_header {
+	__u32 ofw_magic;	/* OFW signature */
+	__u32 ofw_version;
+	__u32 cif_handler;	/* callback into OFW */
+	__u32 irq_desc_table;
+} __attribute__((packed));
+
 /* The so-called "zeropage" */
 struct boot_params {
 	struct screen_info screen_info;			/* 0x000 */
@@ -134,7 +142,12 @@ struct boot_params {
 	__u8  hd0_info[16];	/* obsolete! */		/* 0x080 */
 	__u8  hd1_info[16];	/* obsolete! */		/* 0x090 */
 	struct sys_desc_table sys_desc_table;		/* 0x0a0 */
-	__u8  _pad4[144];				/* 0x0b0 */
+	struct olpc_ofw_header olpc_ofw_header;		/* 0x0b0 */
+	__u32 ext_ramdisk_image;			/* 0x0c0 */
+	__u32 ext_ramdisk_size;				/* 0x0c4 */
+	__u32 ext_cmd_line_ptr;				/* 0x0c8 */
+	__u8  _pad4[112];				/* 0x0cc */
+	__u32 cc_blob_address;				/* 0x13c */
 	struct edid_info edid_info;			/* 0x140 */
 	struct efi_info efi_info;			/* 0x1c0 */
 	__u32 alt_mem_k;				/* 0x1e0 */
diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
index 3ffc7593f87..bd1ee273dec 100644
--- a/arch/x86/lib/zimage.c
+++ b/arch/x86/lib/zimage.c
@@ -222,7 +222,7 @@ struct boot_params *load_zimage(char *image, unsigned long kernel_size,
 	else
 		*load_addressp = ZIMAGE_LOAD_ADDR;
 
-	printf("Building boot_params at 0x%8.8lx\n", (ulong)setup_base);
+	printf("Building boot_params at %lx\n", (ulong)setup_base);
 	memset(setup_base, 0, sizeof(*setup_base));
 	setup_base->hdr = params->hdr;
 
@@ -298,10 +298,13 @@ int setup_zimage(struct boot_params *setup_base, char *cmd_line, int auto_boot,
 		hdr->type_of_loader = 0x80;	/* U-Boot version 0 */
 		if (initrd_addr) {
 			printf("Initial RAM disk at linear address "
-			       "0x%08lx, size %ld bytes\n",
-			       initrd_addr, initrd_size);
+			       "%lx, size %lx (%ld bytes)\n",
+			       initrd_addr, initrd_size, initrd_size);
 
 			hdr->ramdisk_image = initrd_addr;
+			setup_base->ext_ramdisk_image = 0;
+			setup_base->ext_ramdisk_size = 0;
+			setup_base->ext_cmd_line_ptr = 0;
 			hdr->ramdisk_size = initrd_size;
 		}
 	}
@@ -372,8 +375,7 @@ int zboot_load(struct bootm_info *bmi)
 		struct boot_params *from = (struct boot_params *)bmi->base_ptr;
 
 		base_ptr = (struct boot_params *)DEFAULT_SETUP_BASE;
-		log_debug("Building boot_params at 0x%8.8lx\n",
-			  (ulong)base_ptr);
+		log_debug("Building boot_params at %lx\n", (ulong)base_ptr);
 		memset(base_ptr, '\0', sizeof(*base_ptr));
 		base_ptr->hdr = from->hdr;
 	} else {
-- 
2.43.0

[PATCH v5 37/46] acpi: Mark struct acpi_rsdp as packed

[Simon Glass]

At present the size of this struct is too large on 64-bit machines.
Annotate it with __packed to fix this.

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

(no changes since v3)

Changes in v3:
- Add new patch to mark struct acpi_rsdp as packed

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

diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
index b8b1f1338c6..8ef19adc990 100644
--- a/include/acpi/acpi_table.h
+++ b/include/acpi/acpi_table.h
@@ -36,7 +36,7 @@ struct acpi_ctx;
  * RSDP (Root System Description Pointer)
  * Note: ACPI 1.0 didn't have length, xsdt_address, and ext_checksum
  */
-struct acpi_rsdp {
+struct __packed acpi_rsdp {
 	char signature[8];	/* RSDP signature */
 	u8 checksum;		/* Checksum of the first 20 bytes */
 	char oem_id[6];		/* OEM ID */
-- 
2.43.0

[PATCH v5 38/46] boot: Consider non-bootable partitions

[Simon Glass]

Any 'bootable' flag in a DOS partition causes boostd to only scan
bootable partitions for that media. This can mean that extlinux.conf
files on the root disk are missed.

Put this logic behind a flag and update the documentation.

For now, the flag is enabled, to preserve existing behaviour. Future
work may provide a command (or some other mechanism) to control this.

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

(no changes since v4)

Changes in v4:
- Rewrite the commit message
- Enable the flag by default

Changes in v3:
- Add new patch to consider non-bootable partitions

 boot/bootdev-uclass.c            | 4 +++-
 cmd/bootflow.c                   | 2 +-
 doc/develop/bootstd/overview.rst | 5 +++--
 include/bootflow.h               | 2 ++
 test/boot/bootdev.c              | 1 +
 test/boot/bootflow.c             | 5 +++--
 6 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index c39147940b6..9e4c3db2dc1 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -168,8 +168,10 @@ int bootdev_find_in_blk(struct udevice *dev, struct udevice *blk,
 		 */
 
 	/* if there are bootable partitions, scan only those */
-	} else if (iter->first_bootable >= 0 &&
+	} else if ((iter->flags & BOOTFLOWIF_ONLY_BOOTABLE) &&
+		   iter->first_bootable >= 0 &&
 		   (iter->first_bootable ? !info.bootable : iter->part != 1)) {
+		log_debug("Skipping non-bootable partition %d\n", iter->part);
 		return log_msg_ret("boot", -EINVAL);
 	} else {
 		ret = fs_set_blk_dev_with_part(desc, bflow->part);
diff --git a/cmd/bootflow.c b/cmd/bootflow.c
index a89c3bfd9c5..e9ac9746104 100644
--- a/cmd/bootflow.c
+++ b/cmd/bootflow.c
@@ -174,7 +174,7 @@ static int do_bootflow_scan(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	std->cur_bootflow = NULL;
 
-	flags = 0;
+	flags = BOOTFLOWIF_ONLY_BOOTABLE;
 	if (list)
 		flags |= BOOTFLOWIF_SHOW;
 	if (all)
diff --git a/doc/develop/bootstd/overview.rst b/doc/develop/bootstd/overview.rst
index c46bd7a5a90..7bca043791b 100644
--- a/doc/develop/bootstd/overview.rst
+++ b/doc/develop/bootstd/overview.rst
@@ -235,8 +235,9 @@ means that `default_get_bootflow()` is used. This simply obtains the
 block device and calls a bootdev helper function to do the rest. The
 implementation of `bootdev_find_in_blk()` checks the partition table, and
 attempts to read a file from a filesystem on the partition number given by the
-`@iter->part` parameter. If there are any bootable partitions in the table,
-then only bootable partitions are considered.
+`@iter->part` parameter. If there are any bootable partitions in the table and
+the BOOTFLOWIF_ONLY_BOOTABLE flag is set in `@iter->flags`, then only bootable
+partitions are considered.
 
 Each bootdev has a priority, which indicates the order in which it is used,
 if `boot_targets` is not used. Faster bootdevs are used first, since they are
diff --git a/include/bootflow.h b/include/bootflow.h
index d9045bc3dae..2caeb80b878 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -161,6 +161,7 @@ struct bootflow_img {
  * before using it
  * @BOOTFLOWIF_ALL: Return bootflows with errors as well
  * @BOOTFLOWIF_HUNT: Hunt for new bootdevs using the bootdrv hunters
+ * @BOOTFLOWIF_ONLY_BOOTABLE: Only consider partitions marked 'bootable'
  *
  * Internal flags:
  * @BOOTFLOWIF_SINGLE_DEV: (internal) Just scan one bootdev
@@ -177,6 +178,7 @@ enum bootflow_iter_flags_t {
 	BOOTFLOWIF_SHOW			= 1 << 1,
 	BOOTFLOWIF_ALL			= 1 << 2,
 	BOOTFLOWIF_HUNT			= 1 << 3,
+	BOOTFLOWIF_ONLY_BOOTABLE	= BIT(4),
 
 	/*
 	 * flags used internally by standard boot - do not set these when
diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index 5f07430714e..d5499918249 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -509,6 +509,7 @@ static int bootdev_test_bootable(struct unit_test_state *uts)
 	iter.part = 0;
 	ut_assertok(uclass_get_device_by_name(UCLASS_BLK, "mmc1.blk", &blk));
 	iter.dev = blk;
+	iter.flags = BOOTFLOWIF_ONLY_BOOTABLE;
 	ut_assertok(device_find_next_child(&iter.dev));
 	uclass_first_device(UCLASS_BOOTMETH, &bflow.method);
 
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 49ef4d0c3ab..1447af2eb14 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -301,8 +301,9 @@ static int bootflow_iter(struct unit_test_state *uts)
 
 	/* The first device is mmc2.bootdev which has no media */
 	ut_asserteq(-EPROTONOSUPPORT,
-		    bootflow_scan_first(NULL, NULL, &iter,
-					BOOTFLOWIF_ALL | BOOTFLOWIF_SKIP_GLOBAL, &bflow));
+		    bootflow_scan_first(NULL, NULL, &iter, BOOTFLOWIF_ALL |
+					BOOTFLOWIF_SKIP_GLOBAL |
+					BOOTFLOWIF_ONLY_BOOTABLE, &bflow));
 	ut_asserteq(2, iter.num_methods);
 	ut_asserteq(0, iter.cur_method);
 	ut_asserteq(0, iter.part);
-- 
2.43.0

[PATCH v5 39/46] boot: Handle running out of labels

[Simon Glass]

If only a single label is provided in the list, bootdev_next_label()
does not operate correctly and reads beyond the end of the pointer list.

Fix this by adding a new check. Also add a note to convert this array
to an alist

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

(no changes since v3)

Changes in v3:
- Add new patch to handle running out of labels

 boot/bootdev-uclass.c | 3 +++
 include/bootstd.h     | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index 9e4c3db2dc1..3791ebfcb42 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -579,6 +579,9 @@ int bootdev_next_label(struct bootflow_iter *iter, struct udevice **devp,
 	struct udevice *dev;
 
 	log_debug("next\n");
+	if (iter->cur_label >= 0 && !iter->labels[iter->cur_label])
+		return log_msg_ret("fil", -ENODEV);
+
 	for (dev = NULL; !dev && iter->labels[++iter->cur_label];) {
 		const char *label = iter->labels[iter->cur_label];
 		int ret;
diff --git a/include/bootstd.h b/include/bootstd.h
index c39058c0787..ac3c1922fcc 100644
--- a/include/bootstd.h
+++ b/include/bootstd.h
@@ -24,6 +24,9 @@ struct udevice;
  * This is attached to the (only) bootstd device, so there is only one instance
  * of this struct. It provides overall information about bootdevs and bootflows.
  *
+ * TODO(sjg@chromium.org): Convert prefixes, bootdev_order and env_order to use
+ *	alist
+ *
  * @prefixes: NULL-terminated list of prefixes to use for bootflow filenames,
  *	e.g. "/", "/boot/"; NULL if none
  * @bootdev_order: Order to use for bootdevs (or NULL if none), with each item
-- 
2.43.0

[PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Simon Glass]

Update the string to take account of UARTs which are connected on I/O
ports, as on x86.

Fix a typo in an error message in the same command, while we are here.

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

(no changes since v3)

Changes in v3:
- Add new patch to support IO UARTs for earlycon and console

 boot/bootflow.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 58a1afa7a75..4054a966af8 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
 	*buf = '\0';
 	if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
 		snprintf(buf, sizeof(buf),
-			 "uart8250,mmio32,%#lx,%dn8", info.addr,
-			 info.baudrate);
+			 "uart8250,%s,%#lx,%dn8",
+			 info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
+			 "mmio", info.addr, info.baudrate);
 	} else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
 		snprintf(buf, sizeof(buf),
 			 "pl011,mmio32,%#lx,%dn8", info.addr,
@@ -954,7 +955,7 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
 	}
 
 	if (!*buf) {
-		printf("Unknown param '%s\n", arg);
+		printf("Unknown param '%s'\n", arg);
 		return -ENOENT;
 	}
 
-- 
2.43.0

[PATCH v5 41/46] sandbox: acpi: Avoid a warning about FADT

[Simon Glass]

Add a condition for sandbox, to match that of x86, to avoid the warning
"FADT not ACPI-hardware-reduced-compliant".

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

---

(no changes since v4)

Changes in v4:
- Split out into separate patch

 cmd/acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/acpi.c b/cmd/acpi.c
index 094d9d4e858..2273176f106 100644
--- a/cmd/acpi.c
+++ b/cmd/acpi.c
@@ -58,7 +58,7 @@ static void list_fadt(struct acpi_fadt *fadt)
 		dump_hdr(nomap_sysmem(fadt->x_dsdt, 0));
 	else if (fadt->dsdt)
 		dump_hdr(nomap_sysmem(fadt->dsdt, 0));
-	if (!IS_ENABLED(CONFIG_X86) &&
+	if (!IS_ENABLED(CONFIG_X86) && !IS_ENABLED(CONFIG_SANDBOX) &&
 	    !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
 		log_err("FADT not ACPI-hardware-reduced-compliant\n");
 	if (fadt->header.revision >= 3 && fadt->x_firmware_ctrl)
-- 
2.43.0

[PATCH v5 42/46] sandbox: acpi: Correct mapping in FADT

[Simon Glass]

The values in the FADT are pointers so should not go through sandbox's
normal addr<->pointer mapping. Fix this.

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

(no changes since v4)

Changes in v4:
- Split out the command change into a separate patch

Changes in v3:
- Add new patch to correct mapping in FADT

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

diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
index 150f75027a5..3805b61159e 100644
--- a/lib/acpi/acpi_table.c
+++ b/lib/acpi/acpi_table.c
@@ -255,8 +255,8 @@ int acpi_write_fadt(struct acpi_ctx *ctx, const struct acpi_writer *entry)
 	header->creator_revision = 1;
 	fadt->minor_revision = 2;
 
-	fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs);
-	fadt->x_dsdt = map_to_sysmem(ctx->dsdt);
+	fadt->x_firmware_ctrl = nomap_to_sysmem(ctx->facs);
+	fadt->x_dsdt = nomap_to_sysmem(ctx->dsdt);
 
 	if (fadt->x_firmware_ctrl < 0x100000000ULL)
 		fadt->firmware_ctrl = fadt->x_firmware_ctrl;
-- 
2.43.0

[PATCH v5 43/46] acpi: Add a checksum to the DMAR table

[Simon Glass]

This table lacks a correct checksum at present, so fix it.

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

(no changes since v3)

Changes in v3:
- Add new patch to add a checksum to the DMAR table

 lib/acpi/acpi_table.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
index 3805b61159e..4044a64d688 100644
--- a/lib/acpi/acpi_table.c
+++ b/lib/acpi/acpi_table.c
@@ -66,6 +66,7 @@ int acpi_create_dmar(struct acpi_dmar *dmar, enum dmar_flags flags)
 
 	dmar->host_address_width = info.address_width - 1;
 	dmar->flags = flags;
+	header->checksum = table_compute_checksum(dmar, header->length);
 
 	return 0;
 }
-- 
2.43.0

[PATCH v5 44/46] test: acpi: Correct memory leaks

[Simon Glass]

Free the memory used in tests to avoid a leak. Also unmap the addresses
for sandbox.

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

(no changes since v3)

Changes in v3:
- Add new patch to correct memory leaks in the ACPI test

 test/dm/acpi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/test/dm/acpi.c b/test/dm/acpi.c
index 39a26bbb492..f98f9d1e74b 100644
--- a/test/dm/acpi.c
+++ b/test/dm/acpi.c
@@ -309,6 +309,8 @@ static int dm_test_acpi_write_tables(struct unit_test_state *uts)
 	}
 	ut_asserteq(0, ctx.rsdt->entry[3]);
 	ut_asserteq(0, ctx.xsdt->entry[3]);
+	unmap_sysmem(buf);
+	free(buf);
 
 	return 0;
 }
@@ -386,6 +388,8 @@ static int dm_test_acpi_ctx_and_base_tables(struct unit_test_state *uts)
 
 	ut_asserteq(nomap_to_sysmem(rsdt), rsdp->rsdt_address);
 	ut_asserteq(nomap_to_sysmem(xsdt), rsdp->xsdt_address);
+	unmap_sysmem(buf);
+	free(buf);
 
 	return 0;
 }
@@ -428,6 +432,8 @@ static int dm_test_acpi_cmd_list(struct unit_test_state *uts)
 	ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0",
 			   addr, sizeof(struct acpi_dmar), OEM_REVISION);
 	ut_assert_console_end();
+	unmap_sysmem(buf);
+	free(buf);
 
 	return 0;
 }
@@ -458,6 +464,8 @@ static int dm_test_acpi_cmd_dump(struct unit_test_state *uts)
 	ut_assert_nextline("DMAR @ %16lx", addr);
 	ut_assert_nextlines_are_dump(0x30);
 	ut_assert_console_end();
+	unmap_sysmem(buf);
+	free(buf);
 
 	return 0;
 }
@@ -642,6 +650,8 @@ static int dm_test_acpi_cmd_items(struct unit_test_state *uts)
 	ut_assert_nextlines_are_dump(2);
 	ut_assert_nextline("%s", "");
 	ut_assert_console_end();
+	unmap_sysmem(buf);
+	free(buf);
 
 	return 0;
 }
@@ -679,6 +689,8 @@ static int dm_test_acpi_cmd_set(struct unit_test_state *uts)
 	ut_asserteq(addr, gd_acpi_start());
 
 	ut_assert_console_end();
+	unmap_sysmem(buf);
+	free(buf);
 
 	return 0;
 }
@@ -774,6 +786,7 @@ static int dm_test_acpi_find_table(struct unit_test_state *uts)
 
 	/* Restore previous ACPI tables */
 	gd_set_acpi_start(acpi_start);
+	unmap_sysmem(buf);
 	free(buf);
 
 	return 0;
-- 
2.43.0

[PATCH v5 45/46] acpi: Support checking checksums

[Simon Glass]

When the ACPI tables come from an earlier bootloader it is helpful to
see whether the checksums are correct or not. Add a -c flag to the
'acpi list' command to support that.

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

(no changes since v3)

Changes in v3:
- Add new patch to support checking checksums with ACPI command

 cmd/acpi.c             | 57 +++++++++++++++++++++++++++---------------
 doc/usage/cmd/acpi.rst | 20 ++++++++++++++-
 test/dm/acpi.c         | 46 ++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 21 deletions(-)

diff --git a/cmd/acpi.c b/cmd/acpi.c
index 2273176f106..bb243202009 100644
--- a/cmd/acpi.c
+++ b/cmd/acpi.c
@@ -7,6 +7,7 @@
 #include <display_options.h>
 #include <log.h>
 #include <mapmem.h>
+#include <tables_csum.h>
 #include <acpi/acpi_table.h>
 #include <asm/acpi_table.h>
 #include <asm/global_data.h>
@@ -15,6 +16,17 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static const char *show_checksum(void *ptr, uint size, bool chksums)
+{
+	uint checksum;
+
+	if (!chksums)
+		return "";
+	checksum = table_compute_checksum(ptr, size);
+
+	return checksum ? "  bad" : "  OK";
+}
+
 /**
  * dump_hdr() - Dump an ACPI header
  *
@@ -23,16 +35,17 @@ DECLARE_GLOBAL_DATA_PTR;
  *
  * @hdr: ACPI header to dump
  */
-static void dump_hdr(struct acpi_table_header *hdr)
+static void dump_hdr(struct acpi_table_header *hdr, bool chksums)
 {
 	bool has_hdr = memcmp(hdr->signature, "FACS", ACPI_NAME_LEN);
 
 	printf("%.*s  %16lx  %5x", ACPI_NAME_LEN, hdr->signature,
 	       (ulong)map_to_sysmem(hdr), hdr->length);
 	if (has_hdr) {
-		printf("  v%02d %.6s %.8s %x %.4s %x\n", hdr->revision,
+		printf("  v%02d %.6s %.8s %x %.4s %x%s\n", hdr->revision,
 		       hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
-		       hdr->creator_id, hdr->creator_revision);
+		       hdr->creator_id, hdr->creator_revision,
+		       show_checksum(hdr, hdr->length, chksums));
 	} else {
 		printf("\n");
 	}
@@ -52,22 +65,22 @@ static int dump_table_name(const char *sig)
 	return 0;
 }
 
-static void list_fadt(struct acpi_fadt *fadt)
+static void list_fadt(struct acpi_fadt *fadt, bool chksums)
 {
 	if (fadt->header.revision >= 3 && fadt->x_dsdt)
-		dump_hdr(nomap_sysmem(fadt->x_dsdt, 0));
+		dump_hdr(nomap_sysmem(fadt->x_dsdt, 0), chksums);
 	else if (fadt->dsdt)
-		dump_hdr(nomap_sysmem(fadt->dsdt, 0));
+		dump_hdr(nomap_sysmem(fadt->dsdt, 0), chksums);
 	if (!IS_ENABLED(CONFIG_X86) && !IS_ENABLED(CONFIG_SANDBOX) &&
 	    !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
 		log_err("FADT not ACPI-hardware-reduced-compliant\n");
 	if (fadt->header.revision >= 3 && fadt->x_firmware_ctrl)
-		dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0));
+		dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0), chksums);
 	else if (fadt->firmware_ctrl)
-		dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0));
+		dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0), chksums);
 }
 
-static void list_rsdt(struct acpi_rsdp *rsdp)
+static void list_rsdt(struct acpi_rsdp *rsdp, bool chksums)
 {
 	int len, i, count;
 	struct acpi_rsdt *rsdt;
@@ -75,11 +88,11 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
 
 	if (rsdp->rsdt_address) {
 		rsdt = nomap_sysmem(rsdp->rsdt_address, 0);
-		dump_hdr(&rsdt->header);
+		dump_hdr(&rsdt->header, chksums);
 	}
 	if (rsdp->xsdt_address) {
 		xsdt = nomap_sysmem(rsdp->xsdt_address, 0);
-		dump_hdr(&xsdt->header);
+		dump_hdr(&xsdt->header, chksums);
 		len = xsdt->header.length - sizeof(xsdt->header);
 		count = len / sizeof(u64);
 	} else if (rsdp->rsdt_address) {
@@ -100,24 +113,28 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
 		if (!entry)
 			break;
 		hdr = nomap_sysmem(entry, 0);
-		dump_hdr(hdr);
+		dump_hdr(hdr, chksums);
 		if (!memcmp(hdr->signature, "FACP", ACPI_NAME_LEN))
-			list_fadt((struct acpi_fadt *)hdr);
+			list_fadt((struct acpi_fadt *)hdr, chksums);
 	}
 }
 
-static void list_rsdp(struct acpi_rsdp *rsdp)
+static void list_rsdp(struct acpi_rsdp *rsdp, bool chksums)
 {
-	printf("RSDP  %16lx  %5x  v%02d %.6s\n", (ulong)map_to_sysmem(rsdp),
-	       rsdp->length, rsdp->revision, rsdp->oem_id);
-	list_rsdt(rsdp);
+	printf("RSDP  %16lx  %5x  v%02d %.6s%s%s\n",
+	       (ulong)map_to_sysmem(rsdp), rsdp->length, rsdp->revision,
+	       rsdp->oem_id, show_checksum(rsdp, 0x14, chksums),
+	       show_checksum(rsdp, rsdp->length, chksums));
+	list_rsdt(rsdp, chksums);
 }
 
 static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc,
 			char *const argv[])
 {
 	struct acpi_rsdp *rsdp;
+	bool chksums;
 
+	chksums = argc >= 2 && !strcmp("-c", argv[1]);
 	rsdp = map_sysmem(gd_acpi_start(), 0);
 	if (!rsdp) {
 		printf("No ACPI tables present\n");
@@ -125,7 +142,7 @@ static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc,
 	}
 	printf("Name              Base   Size  Detail\n"
 	       "----  ----------------  -----  ----------------------------\n");
-	list_rsdp(rsdp);
+	list_rsdp(rsdp, chksums);
 
 	return 0;
 }
@@ -187,13 +204,13 @@ static int do_acpi_dump(struct cmd_tbl *cmdtp, int flag, int argc,
 }
 
 U_BOOT_LONGHELP(acpi,
-	"list  - list ACPI tables\n"
+	"list [-c] - list ACPI tables [check checksums]\n"
 	"acpi items [-d]   - List/dump each piece of ACPI data from devices\n"
 	"acpi set [<addr>] - Set or show address of ACPI tables\n"
 	"acpi dump <name>  - Dump ACPI table");
 
 U_BOOT_CMD_WITH_SUBCMDS(acpi, "ACPI tables", acpi_help_text,
-	U_BOOT_SUBCMD_MKENT(list, 1, 1, do_acpi_list),
+	U_BOOT_SUBCMD_MKENT(list, 2, 1, do_acpi_list),
 	U_BOOT_SUBCMD_MKENT(items, 2, 1, do_acpi_items),
 	U_BOOT_SUBCMD_MKENT(set, 2, 1, do_acpi_set),
 	U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_acpi_dump));
diff --git a/doc/usage/cmd/acpi.rst b/doc/usage/cmd/acpi.rst
index 9f30972fe53..e652968d584 100644
--- a/doc/usage/cmd/acpi.rst
+++ b/doc/usage/cmd/acpi.rst
@@ -11,7 +11,7 @@ Synopsis
 
 ::
 
-    acpi list
+    acpi list [-c]
     acpi items [-d]
     acpi dump <name>
     acpi set <address>
@@ -38,6 +38,9 @@ List the ACPI tables that have been generated. Each table has a 4-character
 table name (e.g. SSDT, FACS) and has a format defined by the
 `ACPI specification`_.
 
+The `-c` flag tells U-Boot to verify the checksums and print 'OK' or 'BAD' next
+to each table.
+
 U-Boot does not currently support decoding the tables. Unlike devicetree, ACPI
 tables have no regular schema and also some include bytecode, so decoding the
 tables requires a lot of code.
@@ -259,5 +262,20 @@ pointer::
     WAET  bff76a3b     28  v01 BOCHS  BXPC     1 BXPC 1
     SSDT  bff95040     c5  v02 COREv4 COREBOOT 2a CORE 20221020
 
+This shows checking that the checksums are correct for each table::
+
+    => acpi list -c
+    Name              Base   Size  Detail
+    ----  ----------------  -----  ----------------------------
+    RSDP          bec9a000     24  v00 BOCHS   OK OK
+    RSDT          bec9bd4a     38  v01 BOCHS  BXPC     1 BXPC 1  OK
+    FACP          bec9bb46     74  v01 BOCHS  BXPC     1 BXPC 1  OK
+    DSDT          bec9a080   1ac6  v01 BOCHS  BXPC     1 BXPC 1  OK
+    FACS          bec9a040     40
+    APIC          bec9bbba     78  v03 BOCHS  BXPC     1 BXPC 1  OK
+    HPET          bec9bc32     38  v01 BOCHS  BXPC     1 BXPC 1  OK
+    SRAT          bec9bc6a     b8  v01 BOCHS  BXPC     1 BXPC 1  OK
+    WAET          bec9bd22     28  v01 BOCHS  BXPC     1 BXPC 1  OK
+
 
 .. _`ACPI specification`: https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
diff --git a/test/dm/acpi.c b/test/dm/acpi.c
index f98f9d1e74b..db012b6d2f1 100644
--- a/test/dm/acpi.c
+++ b/test/dm/acpi.c
@@ -439,6 +439,52 @@ static int dm_test_acpi_cmd_list(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_acpi_cmd_list, UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
 
+/* Test 'acpi list -c' command */
+static int dm_test_acpi_cmd_list_chksum(struct unit_test_state *uts)
+{
+	struct acpi_ctx ctx;
+	ulong addr;
+	void *buf;
+
+	buf = memalign(16, BUF_SIZE);
+	ut_assertnonnull(buf);
+	addr = map_to_sysmem(buf);
+	ut_assertok(setup_ctx_and_base_tables(uts, &ctx, addr));
+
+	ut_assertok(acpi_write_dev_tables(&ctx));
+
+	run_command("acpi list -c", 0);
+	ut_assert_nextline("Name              Base   Size  Detail");
+	ut_assert_nextline("----  ----------------  -----  ----------------------------");
+	ut_assert_nextline("RSDP  %16lx  %5zx  v02 U-BOOT  OK  OK", addr,
+			   sizeof(struct acpi_rsdp));
+	addr = ALIGN(addr + sizeof(struct acpi_rsdp), 16);
+	ut_assert_nextline("RSDT  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
+			   addr, sizeof(struct acpi_table_header) +
+			   3 * sizeof(u32), OEM_REVISION);
+	addr = ALIGN(addr + sizeof(struct acpi_rsdt), 16);
+	ut_assert_nextline("XSDT  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
+			   addr, sizeof(struct acpi_table_header) +
+			   3 * sizeof(u64), OEM_REVISION);
+	addr = ALIGN(addr + sizeof(struct acpi_xsdt), 64);
+	ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
+			   addr, sizeof(struct acpi_dmar), OEM_REVISION);
+	addr = ALIGN(addr + sizeof(struct acpi_dmar), 16);
+	ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
+			   addr, sizeof(struct acpi_dmar), OEM_REVISION);
+	addr = ALIGN(addr + sizeof(struct acpi_dmar), 16);
+	ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
+			   addr, sizeof(struct acpi_dmar), OEM_REVISION);
+	ut_assert_console_end();
+	ut_assert_console_end();
+	unmap_sysmem(buf);
+	free(buf);
+
+	return 0;
+}
+DM_TEST(dm_test_acpi_cmd_list_chksum,
+	UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
+
 /* Test 'acpi dump' command */
 static int dm_test_acpi_cmd_dump(struct unit_test_state *uts)
 {
-- 
2.43.0

[PATCH v5 46/46] test: Add a test for booting Ubuntu 24.04

[Simon Glass]

Now that U-Boot can boot this quickly, using kvm, add a test that the
installer starts up correctly.

Use the qemu-x86_64 board in the SJG lab.

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

Changes in v5:
- Drop timeouts patch
- Use Ctrl-N instead of down-arrow to make the test more reliable
- Add more logging into the test

Changes in v3:
- Significantly expanded to correct e820 and other issues

Changes in v2:
- Add more patches to support booting with kvm
- Add new patch with a test for booting Ubuntu 24.04

 .gitlab-ci.yml               |  8 ++++-
 test/py/tests/test_distro.py | 61 ++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 test/py/tests/test_distro.py

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c6461f33982..0a7eef0a1d7 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -572,7 +572,7 @@ coreboot test.py:
     - export USE_LABGRID_SJG=1
     # export verbose="-v"
     - ${SRC}/test/py/test.py --role ${ROLE} --build-dir "${OUT}"
-        --capture=tee-sys -k "not bootstd" || ret=$?
+        --capture=tee-sys -k "not bootstd ${TEST_PY_TEST_SPEC}" || ret=$?
     - U_BOOT_BOARD_IDENTITY="${ROLE}" u-boot-test-release || true
     - if [[ $ret -ne 0 ]]; then
         exit $ret;
@@ -747,3 +747,9 @@ zybo:
   variables:
     ROLE: zybo
   <<: *lab_dfn
+
+qemu-x86_64:
+  variables:
+    ROLE: qemu-x86_64
+    TEST_PY_TEST_SPEC: "and not sleep"
+  <<: *lab_dfn
diff --git a/test/py/tests/test_distro.py b/test/py/tests/test_distro.py
new file mode 100644
index 00000000000..bdf4ab657d1
--- /dev/null
+++ b/test/py/tests/test_distro.py
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright 2025 Canonical Ltd.
+# Written by Simon Glass <simon.glass@canonical.com>
+
+import pytest
+
+# Enable early console so that the test can see if something goes wrong
+CONSOLE = 'earlycon=uart8250,io,0x3f8 console=uart8250,io,0x3f8'
+
+@pytest.mark.boardspec('qemu-x86_64')
+@pytest.mark.role('qemu-x86_64')
+def test_distro(ubman):
+    """Test that of-platdata can be generated and used in sandbox"""
+    with ubman.log.section('boot'):
+        ubman.run_command('boot', wait_for_prompt=False)
+
+    with ubman.log.section('Grub'):
+        # Wait for grub to come up and offset a menu
+        ubman.p.expect(['Try or Install Ubuntu'])
+
+        # Press 'e' to edit the command line
+        ubman.log.info("Pressing 'e'")
+        ubman.run_command('e', wait_for_prompt=False, send_nl=False)
+
+        # Wait until we see the editor appear
+        ubman.p.expect(['/casper/initrd'])
+
+        # Go down to the 'linux' line. Avoid using down-arrow as that includes
+        # an Escape character, which may be parsed by Grub as such, causing it
+        # to return to the top menu
+        ubman.log.info("Going DOWN")
+        ubman.ctrl('N')
+        ubman.ctrl('N')
+        ubman.ctrl('N')
+
+        # Go to end of line
+        ubman.log.info("Going to EOL")
+        ubman.ctrl('E')
+
+        # Backspace to remove 'quiet splash'
+        ubman.log.info("Erasing quiet and splash")
+        ubman.send('\b' * len('quiet splash'))
+
+        # Send our noisy console
+        ubman.log.info("Noisy console")
+        ubman.send(CONSOLE)
+
+        # Tell grub to boot
+        ubman.log.info("boot")
+        ubman.ctrl('X')
+        ubman.p.expect(['Booting a command list'])
+
+    with ubman.log.section('Linux'):
+        # Linux should start immediately
+        ubman.p.expect(['Linux version'])
+
+    with ubman.log.section('Ubuntu'):
+        # Shortly later, we should see this banner
+        ubman.p.expect(['Welcome to .*Ubuntu 24.04.1 LTS.*!'])
+
+    ubman.restart_uboot()
-- 
2.43.0

Re: [PATCH v5 07/46] x86: Drop mpspec from the SPL build

[Tom Rini]

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

On Sat, Mar 15, 2025 at 02:25:27PM +0000, Simon Glass wrote:

> This is not needed in SPL, so drop it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  arch/x86/lib/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> index 43e6a1de77d..a908356e8a6 100644
> --- a/arch/x86/lib/Makefile
> +++ b/arch/x86/lib/Makefile
> @@ -26,7 +26,9 @@ obj-y	+= e820.o
>  obj-y	+= init_helpers.o
>  obj-y	+= interrupts.o
>  obj-y	+= lpc-uclass.o
> +ifndef CONFIG_XPL_BUILD
>  obj-y	+= mpspec.o
> +endif
>  obj-$(CONFIG_$(PHASE_)ACPIGEN) += acpi_nhlt.o
>  obj-y	+= northbridge-uclass.o
>  obj-$(CONFIG_I8259_PIC) += i8259.o

What's going on? This should be discarded if unused in SPL
automatically.

-- 
Tom

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

Re: [PATCH v5 09/46] x86: Drop use of CONFIG_REALMODE_DEBUG

[Tom Rini]

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

On Sat, Mar 15, 2025 at 02:25:29PM +0000, Simon Glass wrote:

> This option is not actually defined in Kconfig anymore. Use a normal
> debug print instead, which has a similar effect.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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

Re: [PATCH v5 07/46] x86: Drop mpspec from the SPL build

[Simon Glass]

Hi Tom,

On Mon, 17 Mar 2025 at 15:32, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Mar 15, 2025 at 02:25:27PM +0000, Simon Glass wrote:
>
> > This is not needed in SPL, so drop it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  arch/x86/lib/Makefile | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> > index 43e6a1de77d..a908356e8a6 100644
> > --- a/arch/x86/lib/Makefile
> > +++ b/arch/x86/lib/Makefile
> > @@ -26,7 +26,9 @@ obj-y       += e820.o
> >  obj-y        += init_helpers.o
> >  obj-y        += interrupts.o
> >  obj-y        += lpc-uclass.o
> > +ifndef CONFIG_XPL_BUILD
> >  obj-y        += mpspec.o
> > +endif
> >  obj-$(CONFIG_$(PHASE_)ACPIGEN) += acpi_nhlt.o
> >  obj-y        += northbridge-uclass.o
> >  obj-$(CONFIG_I8259_PIC) += i8259.o
>
> What's going on? This should be discarded if unused in SPL
> automatically.

Yes and I'm actually not sure about this. It probably relates to the
trouble I had getting BIOS ROMs going in SPL, but since I gave up on
that, this patch doesn't seem to be needed.

Regards,
Simon

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Tom Rini]

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

On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:

> Any 'bootable' flag in a DOS partition causes boostd to only scan
> bootable partitions for that media. This can mean that extlinux.conf
> files on the root disk are missed.
> 
> Put this logic behind a flag and update the documentation.
> 
> For now, the flag is enabled, to preserve existing behaviour. Future
> work may provide a command (or some other mechanism) to control this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Isn't this backwards as the existing behavior (prior to bootstd) was to
scan and use these files, and so that's the behavior we need to preserve
by default.

-- 
Tom

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

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Simon Glass]

Hi Tom,

On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
>
> > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > bootable partitions for that media. This can mean that extlinux.conf
> > files on the root disk are missed.
> >
> > Put this logic behind a flag and update the documentation.
> >
> > For now, the flag is enabled, to preserve existing behaviour. Future
> > work may provide a command (or some other mechanism) to control this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Isn't this backwards as the existing behavior (prior to bootstd) was to
> scan and use these files, and so that's the behavior we need to preserve
> by default.

Not so far as I know, but I've already spent too much time trying to
decode those scripts.

If you look at scan_dev_for_boot_part, that is what I was trying to
replicate with bootstd.

Regards,
SImon

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Tom Rini]

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

On Tue, Mar 18, 2025 at 03:24:02PM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> >
> > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > bootable partitions for that media. This can mean that extlinux.conf
> > > files on the root disk are missed.
> > >
> > > Put this logic behind a flag and update the documentation.
> > >
> > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > work may provide a command (or some other mechanism) to control this.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Isn't this backwards as the existing behavior (prior to bootstd) was to
> > scan and use these files, and so that's the behavior we need to preserve
> > by default.
> 
> Not so far as I know, but I've already spent too much time trying to
> decode those scripts.
> 
> If you look at scan_dev_for_boot_part, that is what I was trying to
> replicate with bootstd.

The feedback from the community call where this was brought up was that
it used to work and now didn't, I thought. Heinrich, I think you had one
of the cases here (something about RISC-V and multiple distros?) or am I
misremembering?

-- 
Tom

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

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Simon Glass]

Hi Tom,

On Tue, 18 Mar 2025 at 16:53, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 18, 2025 at 03:24:02PM +0000, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> > >
> > > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > > bootable partitions for that media. This can mean that extlinux.conf
> > > > files on the root disk are missed.
> > > >
> > > > Put this logic behind a flag and update the documentation.
> > > >
> > > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > > work may provide a command (or some other mechanism) to control this.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > >
> > > Isn't this backwards as the existing behavior (prior to bootstd) was to
> > > scan and use these files, and so that's the behavior we need to preserve
> > > by default.
> >
> > Not so far as I know, but I've already spent too much time trying to
> > decode those scripts.
> >
> > If you look at scan_dev_for_boot_part, that is what I was trying to
> > replicate with bootstd.
>
> The feedback from the community call where this was brought up was that
> it used to work and now didn't, I thought. Heinrich, I think you had one
> of the cases here (something about RISC-V and multiple distros?) or am I
> misremembering?

OK let's see if Heinrich chimes in.

Are there notes for the call? It would help me if they could all be in
a shared document, like we used to use[1].

Regards,
Simon

[1] https://docs.google.com/document/d/1YBOMsbM19uSFyoJWnt7-PsOLBaevzQUgV-hiR88a5-o/edit?tab=t.0#heading=h.byz50ta07iok

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Tom Rini]

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

On Wed, Mar 19, 2025 at 03:03:49PM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 18 Mar 2025 at 16:53, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Mar 18, 2025 at 03:24:02PM +0000, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> > > >
> > > > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > > > bootable partitions for that media. This can mean that extlinux.conf
> > > > > files on the root disk are missed.
> > > > >
> > > > > Put this logic behind a flag and update the documentation.
> > > > >
> > > > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > > > work may provide a command (or some other mechanism) to control this.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > Isn't this backwards as the existing behavior (prior to bootstd) was to
> > > > scan and use these files, and so that's the behavior we need to preserve
> > > > by default.
> > >
> > > Not so far as I know, but I've already spent too much time trying to
> > > decode those scripts.
> > >
> > > If you look at scan_dev_for_boot_part, that is what I was trying to
> > > replicate with bootstd.
> >
> > The feedback from the community call where this was brought up was that
> > it used to work and now didn't, I thought. Heinrich, I think you had one
> > of the cases here (something about RISC-V and multiple distros?) or am I
> > misremembering?
> 
> OK let's see if Heinrich chimes in.
> 
> Are there notes for the call? It would help me if they could all be in
> a shared document, like we used to use[1].

Yes, they're in email.

-- 
Tom

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

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Simon Glass]

Hi Tom,

On Wed, 19 Mar 2025 at 16:40, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 19, 2025 at 03:03:49PM +0000, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 18 Mar 2025 at 16:53, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Mar 18, 2025 at 03:24:02PM +0000, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> > > > >
> > > > > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > > > > bootable partitions for that media. This can mean that extlinux.conf
> > > > > > files on the root disk are missed.
> > > > > >
> > > > > > Put this logic behind a flag and update the documentation.
> > > > > >
> > > > > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > > > > work may provide a command (or some other mechanism) to control this.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > Isn't this backwards as the existing behavior (prior to bootstd) was to
> > > > > scan and use these files, and so that's the behavior we need to preserve
> > > > > by default.
> > > >
> > > > Not so far as I know, but I've already spent too much time trying to
> > > > decode those scripts.
> > > >
> > > > If you look at scan_dev_for_boot_part, that is what I was trying to
> > > > replicate with bootstd.
> > >
> > > The feedback from the community call where this was brought up was that
> > > it used to work and now didn't, I thought. Heinrich, I think you had one
> > > of the cases here (something about RISC-V and multiple distros?) or am I
> > > misremembering?
> >
> > OK let's see if Heinrich chimes in.
> >
> > Are there notes for the call? It would help me if they could all be in
> > a shared document, like we used to use[1].
>
> Yes, they're in email.

What subject should I search for to find them? I looked for 'community
call' and a few other things, but cannot find them.

Regards,
Simon

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Tom Rini]

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

On Thu, Mar 20, 2025 at 03:43:36AM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 19 Mar 2025 at 16:40, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 03:03:49PM +0000, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 18 Mar 2025 at 16:53, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Mar 18, 2025 at 03:24:02PM +0000, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> > > > > >
> > > > > > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > > > > > bootable partitions for that media. This can mean that extlinux.conf
> > > > > > > files on the root disk are missed.
> > > > > > >
> > > > > > > Put this logic behind a flag and update the documentation.
> > > > > > >
> > > > > > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > > > > > work may provide a command (or some other mechanism) to control this.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > >
> > > > > > Isn't this backwards as the existing behavior (prior to bootstd) was to
> > > > > > scan and use these files, and so that's the behavior we need to preserve
> > > > > > by default.
> > > > >
> > > > > Not so far as I know, but I've already spent too much time trying to
> > > > > decode those scripts.
> > > > >
> > > > > If you look at scan_dev_for_boot_part, that is what I was trying to
> > > > > replicate with bootstd.
> > > >
> > > > The feedback from the community call where this was brought up was that
> > > > it used to work and now didn't, I thought. Heinrich, I think you had one
> > > > of the cases here (something about RISC-V and multiple distros?) or am I
> > > > misremembering?
> > >
> > > OK let's see if Heinrich chimes in.
> > >
> > > Are there notes for the call? It would help me if they could all be in
> > > a shared document, like we used to use[1].
> >
> > Yes, they're in email.
> 
> What subject should I search for to find them? I looked for 'community
> call' and a few other things, but cannot find them.

They're in reply to every announcement email and sent to all three
lists.

-- 
Tom

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

Re: [PATCH v5 45/46] acpi: Support checking checksums

[Heinrich Schuchardt]

On 15.03.25 15:26, Simon Glass wrote:
> When the ACPI tables come from an earlier bootloader it is helpful to
> see whether the checksums are correct or not. Add a -c flag to the
> 'acpi list' command to support that.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Add new patch to support checking checksums with ACPI command
>
>   cmd/acpi.c             | 57 +++++++++++++++++++++++++++---------------
>   doc/usage/cmd/acpi.rst | 20 ++++++++++++++-
>   test/dm/acpi.c         | 46 ++++++++++++++++++++++++++++++++++
>   3 files changed, 102 insertions(+), 21 deletions(-)
>
> diff --git a/cmd/acpi.c b/cmd/acpi.c
> index 2273176f106..bb243202009 100644
> --- a/cmd/acpi.c
> +++ b/cmd/acpi.c
> @@ -7,6 +7,7 @@
>   #include <display_options.h>
>   #include <log.h>
>   #include <mapmem.h>
> +#include <tables_csum.h>
>   #include <acpi/acpi_table.h>
>   #include <asm/acpi_table.h>
>   #include <asm/global_data.h>
> @@ -15,6 +16,17 @@
>
>   DECLARE_GLOBAL_DATA_PTR;
>
> +static const char *show_checksum(void *ptr, uint size, bool chksums)
> +{
> +	uint checksum;
> +
> +	if (!chksums)
> +		return "";
> +	checksum = table_compute_checksum(ptr, size);
> +
> +	return checksum ? "  bad" : "  OK";

Please, move the leading spaces to the printf statements below.

> +}
> +
>   /**
>    * dump_hdr() - Dump an ACPI header
>    *
> @@ -23,16 +35,17 @@ DECLARE_GLOBAL_DATA_PTR;
>    *
>    * @hdr: ACPI header to dump
>    */
> -static void dump_hdr(struct acpi_table_header *hdr)
> +static void dump_hdr(struct acpi_table_header *hdr, bool chksums)
>   {
>   	bool has_hdr = memcmp(hdr->signature, "FACS", ACPI_NAME_LEN);
>
>   	printf("%.*s  %16lx  %5x", ACPI_NAME_LEN, hdr->signature,
>   	       (ulong)map_to_sysmem(hdr), hdr->length);
>   	if (has_hdr) {
> -		printf("  v%02d %.6s %.8s %x %.4s %x\n", hdr->revision,
> +		printf("  v%02d %.6s %.8s %x %.4s %x%s\n", hdr->revision,

%x results in variable length output. To avoid ragged layout, please,
define the output length.

Best regards

Heinrich

>   		       hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
> -		       hdr->creator_id, hdr->creator_revision);
> +		       hdr->creator_id, hdr->creator_revision,
> +		       show_checksum(hdr, hdr->length, chksums));
>   	} else {
>   		printf("\n");
>   	}
> @@ -52,22 +65,22 @@ static int dump_table_name(const char *sig)
>   	return 0;
>   }
>
> -static void list_fadt(struct acpi_fadt *fadt)
> +static void list_fadt(struct acpi_fadt *fadt, bool chksums)
>   {
>   	if (fadt->header.revision >= 3 && fadt->x_dsdt)
> -		dump_hdr(nomap_sysmem(fadt->x_dsdt, 0));
> +		dump_hdr(nomap_sysmem(fadt->x_dsdt, 0), chksums);
>   	else if (fadt->dsdt)
> -		dump_hdr(nomap_sysmem(fadt->dsdt, 0));
> +		dump_hdr(nomap_sysmem(fadt->dsdt, 0), chksums);
>   	if (!IS_ENABLED(CONFIG_X86) && !IS_ENABLED(CONFIG_SANDBOX) &&
>   	    !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
>   		log_err("FADT not ACPI-hardware-reduced-compliant\n");
>   	if (fadt->header.revision >= 3 && fadt->x_firmware_ctrl)
> -		dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0));
> +		dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0), chksums);
>   	else if (fadt->firmware_ctrl)
> -		dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0));
> +		dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0), chksums);
>   }
>
> -static void list_rsdt(struct acpi_rsdp *rsdp)
> +static void list_rsdt(struct acpi_rsdp *rsdp, bool chksums)
>   {
>   	int len, i, count;
>   	struct acpi_rsdt *rsdt;
> @@ -75,11 +88,11 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
>
>   	if (rsdp->rsdt_address) {
>   		rsdt = nomap_sysmem(rsdp->rsdt_address, 0);
> -		dump_hdr(&rsdt->header);
> +		dump_hdr(&rsdt->header, chksums);
>   	}
>   	if (rsdp->xsdt_address) {
>   		xsdt = nomap_sysmem(rsdp->xsdt_address, 0);
> -		dump_hdr(&xsdt->header);
> +		dump_hdr(&xsdt->header, chksums);
>   		len = xsdt->header.length - sizeof(xsdt->header);
>   		count = len / sizeof(u64);
>   	} else if (rsdp->rsdt_address) {
> @@ -100,24 +113,28 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
>   		if (!entry)
>   			break;
>   		hdr = nomap_sysmem(entry, 0);
> -		dump_hdr(hdr);
> +		dump_hdr(hdr, chksums);
>   		if (!memcmp(hdr->signature, "FACP", ACPI_NAME_LEN))
> -			list_fadt((struct acpi_fadt *)hdr);
> +			list_fadt((struct acpi_fadt *)hdr, chksums);
>   	}
>   }
>
> -static void list_rsdp(struct acpi_rsdp *rsdp)
> +static void list_rsdp(struct acpi_rsdp *rsdp, bool chksums)
>   {
> -	printf("RSDP  %16lx  %5x  v%02d %.6s\n", (ulong)map_to_sysmem(rsdp),
> -	       rsdp->length, rsdp->revision, rsdp->oem_id);
> -	list_rsdt(rsdp);
> +	printf("RSDP  %16lx  %5x  v%02d %.6s%s%s\n",
> +	       (ulong)map_to_sysmem(rsdp), rsdp->length, rsdp->revision,
> +	       rsdp->oem_id, show_checksum(rsdp, 0x14, chksums),
> +	       show_checksum(rsdp, rsdp->length, chksums));
> +	list_rsdt(rsdp, chksums);
>   }
>
>   static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc,
>   			char *const argv[])
>   {
>   	struct acpi_rsdp *rsdp;
> +	bool chksums;
>
> +	chksums = argc >= 2 && !strcmp("-c", argv[1]);
>   	rsdp = map_sysmem(gd_acpi_start(), 0);
>   	if (!rsdp) {
>   		printf("No ACPI tables present\n");
> @@ -125,7 +142,7 @@ static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc,
>   	}
>   	printf("Name              Base   Size  Detail\n"
>   	       "----  ----------------  -----  ----------------------------\n");
> -	list_rsdp(rsdp);
> +	list_rsdp(rsdp, chksums);
>
>   	return 0;
>   }
> @@ -187,13 +204,13 @@ static int do_acpi_dump(struct cmd_tbl *cmdtp, int flag, int argc,
>   }
>
>   U_BOOT_LONGHELP(acpi,
> -	"list  - list ACPI tables\n"
> +	"list [-c] - list ACPI tables [check checksums]\n"
>   	"acpi items [-d]   - List/dump each piece of ACPI data from devices\n"
>   	"acpi set [<addr>] - Set or show address of ACPI tables\n"
>   	"acpi dump <name>  - Dump ACPI table");
>
>   U_BOOT_CMD_WITH_SUBCMDS(acpi, "ACPI tables", acpi_help_text,
> -	U_BOOT_SUBCMD_MKENT(list, 1, 1, do_acpi_list),
> +	U_BOOT_SUBCMD_MKENT(list, 2, 1, do_acpi_list),
>   	U_BOOT_SUBCMD_MKENT(items, 2, 1, do_acpi_items),
>   	U_BOOT_SUBCMD_MKENT(set, 2, 1, do_acpi_set),
>   	U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_acpi_dump));
> diff --git a/doc/usage/cmd/acpi.rst b/doc/usage/cmd/acpi.rst
> index 9f30972fe53..e652968d584 100644
> --- a/doc/usage/cmd/acpi.rst
> +++ b/doc/usage/cmd/acpi.rst
> @@ -11,7 +11,7 @@ Synopsis
>
>   ::
>
> -    acpi list
> +    acpi list [-c]
>       acpi items [-d]
>       acpi dump <name>
>       acpi set <address>
> @@ -38,6 +38,9 @@ List the ACPI tables that have been generated. Each table has a 4-character
>   table name (e.g. SSDT, FACS) and has a format defined by the
>   `ACPI specification`_.
>
> +The `-c` flag tells U-Boot to verify the checksums and print 'OK' or 'BAD' next
> +to each table.
> +
>   U-Boot does not currently support decoding the tables. Unlike devicetree, ACPI
>   tables have no regular schema and also some include bytecode, so decoding the
>   tables requires a lot of code.
> @@ -259,5 +262,20 @@ pointer::
>       WAET  bff76a3b     28  v01 BOCHS  BXPC     1 BXPC 1
>       SSDT  bff95040     c5  v02 COREv4 COREBOOT 2a CORE 20221020
>
> +This shows checking that the checksums are correct for each table::
> +
> +    => acpi list -c
> +    Name              Base   Size  Detail
> +    ----  ----------------  -----  ----------------------------
> +    RSDP          bec9a000     24  v00 BOCHS   OK OK
> +    RSDT          bec9bd4a     38  v01 BOCHS  BXPC     1 BXPC 1  OK
> +    FACP          bec9bb46     74  v01 BOCHS  BXPC     1 BXPC 1  OK
> +    DSDT          bec9a080   1ac6  v01 BOCHS  BXPC     1 BXPC 1  OK
> +    FACS          bec9a040     40
> +    APIC          bec9bbba     78  v03 BOCHS  BXPC     1 BXPC 1  OK
> +    HPET          bec9bc32     38  v01 BOCHS  BXPC     1 BXPC 1  OK
> +    SRAT          bec9bc6a     b8  v01 BOCHS  BXPC     1 BXPC 1  OK
> +    WAET          bec9bd22     28  v01 BOCHS  BXPC     1 BXPC 1  OK
> +
>
>   .. _`ACPI specification`: https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
> diff --git a/test/dm/acpi.c b/test/dm/acpi.c
> index f98f9d1e74b..db012b6d2f1 100644
> --- a/test/dm/acpi.c
> +++ b/test/dm/acpi.c
> @@ -439,6 +439,52 @@ static int dm_test_acpi_cmd_list(struct unit_test_state *uts)
>   }
>   DM_TEST(dm_test_acpi_cmd_list, UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
>
> +/* Test 'acpi list -c' command */
> +static int dm_test_acpi_cmd_list_chksum(struct unit_test_state *uts)
> +{
> +	struct acpi_ctx ctx;
> +	ulong addr;
> +	void *buf;
> +
> +	buf = memalign(16, BUF_SIZE);
> +	ut_assertnonnull(buf);
> +	addr = map_to_sysmem(buf);
> +	ut_assertok(setup_ctx_and_base_tables(uts, &ctx, addr));
> +
> +	ut_assertok(acpi_write_dev_tables(&ctx));
> +
> +	run_command("acpi list -c", 0);
> +	ut_assert_nextline("Name              Base   Size  Detail");
> +	ut_assert_nextline("----  ----------------  -----  ----------------------------");
> +	ut_assert_nextline("RSDP  %16lx  %5zx  v02 U-BOOT  OK  OK", addr,
> +			   sizeof(struct acpi_rsdp));
> +	addr = ALIGN(addr + sizeof(struct acpi_rsdp), 16);
> +	ut_assert_nextline("RSDT  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
> +			   addr, sizeof(struct acpi_table_header) +
> +			   3 * sizeof(u32), OEM_REVISION);
> +	addr = ALIGN(addr + sizeof(struct acpi_rsdt), 16);
> +	ut_assert_nextline("XSDT  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
> +			   addr, sizeof(struct acpi_table_header) +
> +			   3 * sizeof(u64), OEM_REVISION);
> +	addr = ALIGN(addr + sizeof(struct acpi_xsdt), 64);
> +	ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
> +			   addr, sizeof(struct acpi_dmar), OEM_REVISION);
> +	addr = ALIGN(addr + sizeof(struct acpi_dmar), 16);
> +	ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
> +			   addr, sizeof(struct acpi_dmar), OEM_REVISION);
> +	addr = ALIGN(addr + sizeof(struct acpi_dmar), 16);
> +	ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0  OK",
> +			   addr, sizeof(struct acpi_dmar), OEM_REVISION);
> +	ut_assert_console_end();
> +	ut_assert_console_end();
> +	unmap_sysmem(buf);
> +	free(buf);
> +
> +	return 0;
> +}
> +DM_TEST(dm_test_acpi_cmd_list_chksum,
> +	UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
> +
>   /* Test 'acpi dump' command */
>   static int dm_test_acpi_cmd_dump(struct unit_test_state *uts)
>   {

Re: [PATCH v5 44/46] test: acpi: Correct memory leaks

[Heinrich Schuchardt]

On 15.03.25 15:26, Simon Glass wrote:
> Free the memory used in tests to avoid a leak. Also unmap the addresses
> for sandbox.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Add new patch to correct memory leaks in the ACPI test
>
>   test/dm/acpi.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/test/dm/acpi.c b/test/dm/acpi.c
> index 39a26bbb492..f98f9d1e74b 100644
> --- a/test/dm/acpi.c
> +++ b/test/dm/acpi.c
> @@ -309,6 +309,8 @@ static int dm_test_acpi_write_tables(struct unit_test_state *uts)
>   	}
>   	ut_asserteq(0, ctx.rsdt->entry[3]);
>   	ut_asserteq(0, ctx.xsdt->entry[3]);
> +	unmap_sysmem(buf);
> +	free(buf);
>
>   	return 0;
>   }
> @@ -386,6 +388,8 @@ static int dm_test_acpi_ctx_and_base_tables(struct unit_test_state *uts)
>
>   	ut_asserteq(nomap_to_sysmem(rsdt), rsdp->rsdt_address);
>   	ut_asserteq(nomap_to_sysmem(xsdt), rsdp->xsdt_address);
> +	unmap_sysmem(buf);
> +	free(buf);
>
>   	return 0;
>   }
> @@ -428,6 +432,8 @@ static int dm_test_acpi_cmd_list(struct unit_test_state *uts)
>   	ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x INTL 0",
>   			   addr, sizeof(struct acpi_dmar), OEM_REVISION);
>   	ut_assert_console_end();
> +	unmap_sysmem(buf);
> +	free(buf);
>
>   	return 0;
>   }
> @@ -458,6 +464,8 @@ static int dm_test_acpi_cmd_dump(struct unit_test_state *uts)
>   	ut_assert_nextline("DMAR @ %16lx", addr);
>   	ut_assert_nextlines_are_dump(0x30);
>   	ut_assert_console_end();
> +	unmap_sysmem(buf);
> +	free(buf);
>
>   	return 0;
>   }
> @@ -642,6 +650,8 @@ static int dm_test_acpi_cmd_items(struct unit_test_state *uts)
>   	ut_assert_nextlines_are_dump(2);
>   	ut_assert_nextline("%s", "");
>   	ut_assert_console_end();
> +	unmap_sysmem(buf);
> +	free(buf);
>
>   	return 0;
>   }
> @@ -679,6 +689,8 @@ static int dm_test_acpi_cmd_set(struct unit_test_state *uts)
>   	ut_asserteq(addr, gd_acpi_start());
>
>   	ut_assert_console_end();
> +	unmap_sysmem(buf);
> +	free(buf);
>
>   	return 0;
>   }
> @@ -774,6 +786,7 @@ static int dm_test_acpi_find_table(struct unit_test_state *uts)
>
>   	/* Restore previous ACPI tables */
>   	gd_set_acpi_start(acpi_start);
> +	unmap_sysmem(buf);
>   	free(buf);
>
>   	return 0;

Re: [PATCH v5 43/46] acpi: Add a checksum to the DMAR table

[Heinrich Schuchardt]

On 15.03.25 15:26, Simon Glass wrote:
> This table lacks a correct checksum at present, so fix it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Add new patch to add a checksum to the DMAR table
>
>   lib/acpi/acpi_table.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> index 3805b61159e..4044a64d688 100644
> --- a/lib/acpi/acpi_table.c
> +++ b/lib/acpi/acpi_table.c
> @@ -66,6 +66,7 @@ int acpi_create_dmar(struct acpi_dmar *dmar, enum dmar_flags flags)
>
>   	dmar->host_address_width = info.address_width - 1;
>   	dmar->flags = flags;
> +	header->checksum = table_compute_checksum(dmar, header->length);

table_compute_checksum is used for ACPI and SMBIOS.

In the ACPI case header->length and header->checksum are always at the
same offset to the table start. Shouldn't we introduce a new function to
avoid repeating ourselves:

/**
  * acpi_update_checksum() - update ACPI table checksum
  *
  * @header - header of an ACPI table
  */
void acpi_update_checksum(struct acpi_table_header *header)
{
	header->checksum = table_compute_checksum(header, header->length);
}

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>
>   	return 0;
>   }

Re: [PATCH v5 45/46] acpi: Support checking checksums

[Heinrich Schuchardt]

On 21.03.25 11:49, Heinrich Schuchardt wrote:
> On 15.03.25 15:26, Simon Glass wrote:
>> When the ACPI tables come from an earlier bootloader it is helpful to
>> see whether the checksums are correct or not. Add a -c flag to the
>> 'acpi list' command to support that.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> (no changes since v3)
>>
>> Changes in v3:
>> - Add new patch to support checking checksums with ACPI command
>>
>>   cmd/acpi.c             | 57 +++++++++++++++++++++++++++---------------
>>   doc/usage/cmd/acpi.rst | 20 ++++++++++++++-
>>   test/dm/acpi.c         | 46 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 102 insertions(+), 21 deletions(-)
>>
>> diff --git a/cmd/acpi.c b/cmd/acpi.c
>> index 2273176f106..bb243202009 100644
>> --- a/cmd/acpi.c
>> +++ b/cmd/acpi.c
>> @@ -7,6 +7,7 @@
>>   #include <display_options.h>
>>   #include <log.h>
>>   #include <mapmem.h>
>> +#include <tables_csum.h>
>>   #include <acpi/acpi_table.h>
>>   #include <asm/acpi_table.h>
>>   #include <asm/global_data.h>
>> @@ -15,6 +16,17 @@
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>> +static const char *show_checksum(void *ptr, uint size, bool chksums)
>> +{
>> +    uint checksum;
>> +
>> +    if (!chksums)
>> +        return "";
>> +    checksum = table_compute_checksum(ptr, size);
>> +
>> +    return checksum ? "  bad" : "  OK";
> 
> Please, move the leading spaces to the printf statements below.
> 
>> +}
>> +
>>   /**
>>    * dump_hdr() - Dump an ACPI header
>>    *
>> @@ -23,16 +35,17 @@ DECLARE_GLOBAL_DATA_PTR;
>>    *
>>    * @hdr: ACPI header to dump
>>    */
>> -static void dump_hdr(struct acpi_table_header *hdr)
>> +static void dump_hdr(struct acpi_table_header *hdr, bool chksums)
>>   {
>>       bool has_hdr = memcmp(hdr->signature, "FACS", ACPI_NAME_LEN);
>>
>>       printf("%.*s  %16lx  %5x", ACPI_NAME_LEN, hdr->signature,
>>              (ulong)map_to_sysmem(hdr), hdr->length);
>>       if (has_hdr) {
>> -        printf("  v%02d %.6s %.8s %x %.4s %x\n", hdr->revision,
>> +        printf("  v%02d %.6s %.8s %x %.4s %x%s\n", hdr->revision,
> 
> %x results in variable length output. To avoid ragged layout, please,
> define the output length.
> 
> Best regards
> 
> Heinrich
> 
>>                  hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
>> -               hdr->creator_id, hdr->creator_revision);
>> +               hdr->creator_id, hdr->creator_revision,
>> +               show_checksum(hdr, hdr->length, chksums));
>>       } else {
>>           printf("\n");
>>       }
>> @@ -52,22 +65,22 @@ static int dump_table_name(const char *sig)
>>       return 0;
>>   }
>>
>> -static void list_fadt(struct acpi_fadt *fadt)
>> +static void list_fadt(struct acpi_fadt *fadt, bool chksums)
>>   {
>>       if (fadt->header.revision >= 3 && fadt->x_dsdt)
>> -        dump_hdr(nomap_sysmem(fadt->x_dsdt, 0));
>> +        dump_hdr(nomap_sysmem(fadt->x_dsdt, 0), chksums);
>>       else if (fadt->dsdt)
>> -        dump_hdr(nomap_sysmem(fadt->dsdt, 0));
>> +        dump_hdr(nomap_sysmem(fadt->dsdt, 0), chksums);
>>       if (!IS_ENABLED(CONFIG_X86) && !IS_ENABLED(CONFIG_SANDBOX) &&
>>           !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
>>           log_err("FADT not ACPI-hardware-reduced-compliant\n");
>>       if (fadt->header.revision >= 3 && fadt->x_firmware_ctrl)
>> -        dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0));
>> +        dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0), chksums);
>>       else if (fadt->firmware_ctrl)
>> -        dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0));
>> +        dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0), chksums);
>>   }
>>
>> -static void list_rsdt(struct acpi_rsdp *rsdp)
>> +static void list_rsdt(struct acpi_rsdp *rsdp, bool chksums)
>>   {
>>       int len, i, count;
>>       struct acpi_rsdt *rsdt;
>> @@ -75,11 +88,11 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
>>
>>       if (rsdp->rsdt_address) {
>>           rsdt = nomap_sysmem(rsdp->rsdt_address, 0);
>> -        dump_hdr(&rsdt->header);
>> +        dump_hdr(&rsdt->header, chksums);
>>       }
>>       if (rsdp->xsdt_address) {
>>           xsdt = nomap_sysmem(rsdp->xsdt_address, 0);
>> -        dump_hdr(&xsdt->header);
>> +        dump_hdr(&xsdt->header, chksums);
>>           len = xsdt->header.length - sizeof(xsdt->header);
>>           count = len / sizeof(u64);
>>       } else if (rsdp->rsdt_address) {
>> @@ -100,24 +113,28 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
>>           if (!entry)
>>               break;
>>           hdr = nomap_sysmem(entry, 0);
>> -        dump_hdr(hdr);
>> +        dump_hdr(hdr, chksums);
>>           if (!memcmp(hdr->signature, "FACP", ACPI_NAME_LEN))
>> -            list_fadt((struct acpi_fadt *)hdr);
>> +            list_fadt((struct acpi_fadt *)hdr, chksums);
>>       }
>>   }
>>
>> -static void list_rsdp(struct acpi_rsdp *rsdp)
>> +static void list_rsdp(struct acpi_rsdp *rsdp, bool chksums)
>>   {
>> -    printf("RSDP  %16lx  %5x  v%02d %.6s\n", (ulong)map_to_sysmem(rsdp),
>> -           rsdp->length, rsdp->revision, rsdp->oem_id);
>> -    list_rsdt(rsdp);
>> +    printf("RSDP  %16lx  %5x  v%02d %.6s%s%s\n",
>> +           (ulong)map_to_sysmem(rsdp), rsdp->length, rsdp->revision,
>> +           rsdp->oem_id, show_checksum(rsdp, 0x14, chksums),
>> +           show_checksum(rsdp, rsdp->length, chksums));
>> +    list_rsdt(rsdp, chksums);
>>   }
>>
>>   static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc,
>>               char *const argv[])
>>   {
>>       struct acpi_rsdp *rsdp;
>> +    bool chksums;
>>
>> +    chksums = argc >= 2 && !strcmp("-c", argv[1]);
>>       rsdp = map_sysmem(gd_acpi_start(), 0);
>>       if (!rsdp) {
>>           printf("No ACPI tables present\n");
>> @@ -125,7 +142,7 @@ static int do_acpi_list(struct cmd_tbl *cmdtp, int 
>> flag, int argc,
>>       }
>>       printf("Name              Base   Size  Detail\n"
>>              "----  ----------------  -----  
>> ----------------------------\n");
>> -    list_rsdp(rsdp);
>> +    list_rsdp(rsdp, chksums);
>>
>>       return 0;
>>   }
>> @@ -187,13 +204,13 @@ static int do_acpi_dump(struct cmd_tbl *cmdtp, 
>> int flag, int argc,
>>   }
>>
>>   U_BOOT_LONGHELP(acpi,
>> -    "list  - list ACPI tables\n"
>> +    "list [-c] - list ACPI tables [check checksums]\n"

Let's keep things simple for the user:

Always check the checksums.
Only print a notice if a checksum is wrong.

Best regards

Heinrich

>>       "acpi items [-d]   - List/dump each piece of ACPI data from 
>> devices\n"
>>       "acpi set [<addr>] - Set or show address of ACPI tables\n"
>>       "acpi dump <name>  - Dump ACPI table");
>>
>>   U_BOOT_CMD_WITH_SUBCMDS(acpi, "ACPI tables", acpi_help_text,
>> -    U_BOOT_SUBCMD_MKENT(list, 1, 1, do_acpi_list),
>> +    U_BOOT_SUBCMD_MKENT(list, 2, 1, do_acpi_list),
>>       U_BOOT_SUBCMD_MKENT(items, 2, 1, do_acpi_items),
>>       U_BOOT_SUBCMD_MKENT(set, 2, 1, do_acpi_set),
>>       U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_acpi_dump));
>> diff --git a/doc/usage/cmd/acpi.rst b/doc/usage/cmd/acpi.rst
>> index 9f30972fe53..e652968d584 100644
>> --- a/doc/usage/cmd/acpi.rst
>> +++ b/doc/usage/cmd/acpi.rst
>> @@ -11,7 +11,7 @@ Synopsis
>>
>>   ::
>>
>> -    acpi list
>> +    acpi list [-c]
>>       acpi items [-d]
>>       acpi dump <name>
>>       acpi set <address>
>> @@ -38,6 +38,9 @@ List the ACPI tables that have been generated. Each 
>> table has a 4-character
>>   table name (e.g. SSDT, FACS) and has a format defined by the
>>   `ACPI specification`_.
>>
>> +The `-c` flag tells U-Boot to verify the checksums and print 'OK' or 
>> 'BAD' next
>> +to each table.
>> +
>>   U-Boot does not currently support decoding the tables. Unlike 
>> devicetree, ACPI
>>   tables have no regular schema and also some include bytecode, so 
>> decoding the
>>   tables requires a lot of code.
>> @@ -259,5 +262,20 @@ pointer::
>>       WAET  bff76a3b     28  v01 BOCHS  BXPC     1 BXPC 1
>>       SSDT  bff95040     c5  v02 COREv4 COREBOOT 2a CORE 20221020
>>
>> +This shows checking that the checksums are correct for each table::
>> +
>> +    => acpi list -c
>> +    Name              Base   Size  Detail
>> +    ----  ----------------  -----  ----------------------------
>> +    RSDP          bec9a000     24  v00 BOCHS   OK OK
>> +    RSDT          bec9bd4a     38  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    FACP          bec9bb46     74  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    DSDT          bec9a080   1ac6  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    FACS          bec9a040     40
>> +    APIC          bec9bbba     78  v03 BOCHS  BXPC     1 BXPC 1  OK
>> +    HPET          bec9bc32     38  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    SRAT          bec9bc6a     b8  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +    WAET          bec9bd22     28  v01 BOCHS  BXPC     1 BXPC 1  OK
>> +
>>
>>   .. _`ACPI specification`: https://uefi.org/sites/default/files/ 
>> resources/ACPI_6_3_final_Jan30.pdf
>> diff --git a/test/dm/acpi.c b/test/dm/acpi.c
>> index f98f9d1e74b..db012b6d2f1 100644
>> --- a/test/dm/acpi.c
>> +++ b/test/dm/acpi.c
>> @@ -439,6 +439,52 @@ static int dm_test_acpi_cmd_list(struct 
>> unit_test_state *uts)
>>   }
>>   DM_TEST(dm_test_acpi_cmd_list, UTF_SCAN_PDATA | UTF_SCAN_FDT | 
>> UTF_CONSOLE);
>>
>> +/* Test 'acpi list -c' command */
>> +static int dm_test_acpi_cmd_list_chksum(struct unit_test_state *uts)
>> +{
>> +    struct acpi_ctx ctx;
>> +    ulong addr;
>> +    void *buf;
>> +
>> +    buf = memalign(16, BUF_SIZE);
>> +    ut_assertnonnull(buf);
>> +    addr = map_to_sysmem(buf);
>> +    ut_assertok(setup_ctx_and_base_tables(uts, &ctx, addr));
>> +
>> +    ut_assertok(acpi_write_dev_tables(&ctx));
>> +
>> +    run_command("acpi list -c", 0);
>> +    ut_assert_nextline("Name              Base   Size  Detail");
>> +    ut_assert_nextline("----  ----------------  -----  
>> ----------------------------");
>> +    ut_assert_nextline("RSDP  %16lx  %5zx  v02 U-BOOT  OK  OK", addr,
>> +               sizeof(struct acpi_rsdp));
>> +    addr = ALIGN(addr + sizeof(struct acpi_rsdp), 16);
>> +    ut_assert_nextline("RSDT  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_table_header) +
>> +               3 * sizeof(u32), OEM_REVISION);
>> +    addr = ALIGN(addr + sizeof(struct acpi_rsdt), 16);
>> +    ut_assert_nextline("XSDT  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_table_header) +
>> +               3 * sizeof(u64), OEM_REVISION);
>> +    addr = ALIGN(addr + sizeof(struct acpi_xsdt), 64);
>> +    ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_dmar), OEM_REVISION);
>> +    addr = ALIGN(addr + sizeof(struct acpi_dmar), 16);
>> +    ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_dmar), OEM_REVISION);
>> +    addr = ALIGN(addr + sizeof(struct acpi_dmar), 16);
>> +    ut_assert_nextline("DMAR  %16lx  %5zx  v01 U-BOOT U-BOOTBL %x 
>> INTL 0  OK",
>> +               addr, sizeof(struct acpi_dmar), OEM_REVISION);
>> +    ut_assert_console_end();
>> +    ut_assert_console_end();
>> +    unmap_sysmem(buf);
>> +    free(buf);
>> +
>> +    return 0;
>> +}
>> +DM_TEST(dm_test_acpi_cmd_list_chksum,
>> +    UTF_SCAN_PDATA | UTF_SCAN_FDT | UTF_CONSOLE);
>> +
>>   /* Test 'acpi dump' command */
>>   static int dm_test_acpi_cmd_dump(struct unit_test_state *uts)
>>   {
> 

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Simon Glass]

Hi Tom,

On Thu, 20 Mar 2025 at 15:22, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Mar 20, 2025 at 03:43:36AM +0000, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 19 Mar 2025 at 16:40, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Mar 19, 2025 at 03:03:49PM +0000, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 18 Mar 2025 at 16:53, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Mar 18, 2025 at 03:24:02PM +0000, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> > > > > > >
> > > > > > > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > > > > > > bootable partitions for that media. This can mean that extlinux.conf
> > > > > > > > files on the root disk are missed.
> > > > > > > >
> > > > > > > > Put this logic behind a flag and update the documentation.
> > > > > > > >
> > > > > > > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > > > > > > work may provide a command (or some other mechanism) to control this.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > >
> > > > > > > Isn't this backwards as the existing behavior (prior to bootstd) was to
> > > > > > > scan and use these files, and so that's the behavior we need to preserve
> > > > > > > by default.
> > > > > >
> > > > > > Not so far as I know, but I've already spent too much time trying to
> > > > > > decode those scripts.
> > > > > >
> > > > > > If you look at scan_dev_for_boot_part, that is what I was trying to
> > > > > > replicate with bootstd.
> > > > >
> > > > > The feedback from the community call where this was brought up was that
> > > > > it used to work and now didn't, I thought. Heinrich, I think you had one
> > > > > of the cases here (something about RISC-V and multiple distros?) or am I
> > > > > misremembering?
> > > >
> > > > OK let's see if Heinrich chimes in.
> > > >
> > > > Are there notes for the call? It would help me if they could all be in
> > > > a shared document, like we used to use[1].
> > >
> > > Yes, they're in email.
> >
> > What subject should I search for to find them? I looked for 'community
> > call' and a few other things, but cannot find them.
>
> They're in reply to every announcement email and sent to all three
> lists.

OK, well I can't find them, sorry. I did see the first one you sent
but after that I have no record.

It sounds like you don't want them in a doc for easy reference?

Regards,
Simon

Re: [PATCH v5 45/46] acpi: Support checking checksums

[Simon Glass]

Hi Heinrich,

On Fri, 21 Mar 2025 at 06:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 21.03.25 11:49, Heinrich Schuchardt wrote:
> > On 15.03.25 15:26, Simon Glass wrote:
> >> When the ACPI tables come from an earlier bootloader it is helpful to
> >> see whether the checksums are correct or not. Add a -c flag to the
> >> 'acpi list' command to support that.
> >>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>
> >> (no changes since v3)
> >>
> >> Changes in v3:
> >> - Add new patch to support checking checksums with ACPI command
> >>
> >>   cmd/acpi.c             | 57 +++++++++++++++++++++++++++---------------
> >>   doc/usage/cmd/acpi.rst | 20 ++++++++++++++-
> >>   test/dm/acpi.c         | 46 ++++++++++++++++++++++++++++++++++
> >>   3 files changed, 102 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/cmd/acpi.c b/cmd/acpi.c
> >> index 2273176f106..bb243202009 100644
> >> --- a/cmd/acpi.c
> >> +++ b/cmd/acpi.c
> >> @@ -7,6 +7,7 @@
> >>   #include <display_options.h>
> >>   #include <log.h>
> >>   #include <mapmem.h>
> >> +#include <tables_csum.h>
> >>   #include <acpi/acpi_table.h>
> >>   #include <asm/acpi_table.h>
> >>   #include <asm/global_data.h>
> >> @@ -15,6 +16,17 @@
> >>
> >>   DECLARE_GLOBAL_DATA_PTR;
> >>
> >> +static const char *show_checksum(void *ptr, uint size, bool chksums)
> >> +{
> >> +    uint checksum;
> >> +
> >> +    if (!chksums)
> >> +        return "";
> >> +    checksum = table_compute_checksum(ptr, size);
> >> +
> >> +    return checksum ? "  bad" : "  OK";
> >
> > Please, move the leading spaces to the printf statements below.
> >
> >> +}
> >> +
> >>   /**
> >>    * dump_hdr() - Dump an ACPI header
> >>    *
> >> @@ -23,16 +35,17 @@ DECLARE_GLOBAL_DATA_PTR;
> >>    *
> >>    * @hdr: ACPI header to dump
> >>    */
> >> -static void dump_hdr(struct acpi_table_header *hdr)
> >> +static void dump_hdr(struct acpi_table_header *hdr, bool chksums)
> >>   {
> >>       bool has_hdr = memcmp(hdr->signature, "FACS", ACPI_NAME_LEN);
> >>
> >>       printf("%.*s  %16lx  %5x", ACPI_NAME_LEN, hdr->signature,
> >>              (ulong)map_to_sysmem(hdr), hdr->length);
> >>       if (has_hdr) {
> >> -        printf("  v%02d %.6s %.8s %x %.4s %x\n", hdr->revision,
> >> +        printf("  v%02d %.6s %.8s %x %.4s %x%s\n", hdr->revision,
> >
> > %x results in variable length output. To avoid ragged layout, please,
> > define the output length.
> >
> > Best regards
> >
> > Heinrich
> >
> >>                  hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
> >> -               hdr->creator_id, hdr->creator_revision);
> >> +               hdr->creator_id, hdr->creator_revision,
> >> +               show_checksum(hdr, hdr->length, chksums));
> >>       } else {
> >>           printf("\n");
> >>       }
> >> @@ -52,22 +65,22 @@ static int dump_table_name(const char *sig)
> >>       return 0;
> >>   }
> >>
> >> -static void list_fadt(struct acpi_fadt *fadt)
> >> +static void list_fadt(struct acpi_fadt *fadt, bool chksums)
> >>   {
> >>       if (fadt->header.revision >= 3 && fadt->x_dsdt)
> >> -        dump_hdr(nomap_sysmem(fadt->x_dsdt, 0));
> >> +        dump_hdr(nomap_sysmem(fadt->x_dsdt, 0), chksums);
> >>       else if (fadt->dsdt)
> >> -        dump_hdr(nomap_sysmem(fadt->dsdt, 0));
> >> +        dump_hdr(nomap_sysmem(fadt->dsdt, 0), chksums);
> >>       if (!IS_ENABLED(CONFIG_X86) && !IS_ENABLED(CONFIG_SANDBOX) &&
> >>           !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
> >>           log_err("FADT not ACPI-hardware-reduced-compliant\n");
> >>       if (fadt->header.revision >= 3 && fadt->x_firmware_ctrl)
> >> -        dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0));
> >> +        dump_hdr(nomap_sysmem(fadt->x_firmware_ctrl, 0), chksums);
> >>       else if (fadt->firmware_ctrl)
> >> -        dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0));
> >> +        dump_hdr(nomap_sysmem(fadt->firmware_ctrl, 0), chksums);
> >>   }
> >>
> >> -static void list_rsdt(struct acpi_rsdp *rsdp)
> >> +static void list_rsdt(struct acpi_rsdp *rsdp, bool chksums)
> >>   {
> >>       int len, i, count;
> >>       struct acpi_rsdt *rsdt;
> >> @@ -75,11 +88,11 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
> >>
> >>       if (rsdp->rsdt_address) {
> >>           rsdt = nomap_sysmem(rsdp->rsdt_address, 0);
> >> -        dump_hdr(&rsdt->header);
> >> +        dump_hdr(&rsdt->header, chksums);
> >>       }
> >>       if (rsdp->xsdt_address) {
> >>           xsdt = nomap_sysmem(rsdp->xsdt_address, 0);
> >> -        dump_hdr(&xsdt->header);
> >> +        dump_hdr(&xsdt->header, chksums);
> >>           len = xsdt->header.length - sizeof(xsdt->header);
> >>           count = len / sizeof(u64);
> >>       } else if (rsdp->rsdt_address) {
> >> @@ -100,24 +113,28 @@ static void list_rsdt(struct acpi_rsdp *rsdp)
> >>           if (!entry)
> >>               break;
> >>           hdr = nomap_sysmem(entry, 0);
> >> -        dump_hdr(hdr);
> >> +        dump_hdr(hdr, chksums);
> >>           if (!memcmp(hdr->signature, "FACP", ACPI_NAME_LEN))
> >> -            list_fadt((struct acpi_fadt *)hdr);
> >> +            list_fadt((struct acpi_fadt *)hdr, chksums);
> >>       }
> >>   }
> >>
> >> -static void list_rsdp(struct acpi_rsdp *rsdp)
> >> +static void list_rsdp(struct acpi_rsdp *rsdp, bool chksums)
> >>   {
> >> -    printf("RSDP  %16lx  %5x  v%02d %.6s\n", (ulong)map_to_sysmem(rsdp),
> >> -           rsdp->length, rsdp->revision, rsdp->oem_id);
> >> -    list_rsdt(rsdp);
> >> +    printf("RSDP  %16lx  %5x  v%02d %.6s%s%s\n",
> >> +           (ulong)map_to_sysmem(rsdp), rsdp->length, rsdp->revision,
> >> +           rsdp->oem_id, show_checksum(rsdp, 0x14, chksums),
> >> +           show_checksum(rsdp, rsdp->length, chksums));
> >> +    list_rsdt(rsdp, chksums);
> >>   }
> >>
> >>   static int do_acpi_list(struct cmd_tbl *cmdtp, int flag, int argc,
> >>               char *const argv[])
> >>   {
> >>       struct acpi_rsdp *rsdp;
> >> +    bool chksums;
> >>
> >> +    chksums = argc >= 2 && !strcmp("-c", argv[1]);
> >>       rsdp = map_sysmem(gd_acpi_start(), 0);
> >>       if (!rsdp) {
> >>           printf("No ACPI tables present\n");
> >> @@ -125,7 +142,7 @@ static int do_acpi_list(struct cmd_tbl *cmdtp, int
> >> flag, int argc,
> >>       }
> >>       printf("Name              Base   Size  Detail\n"
> >>              "----  ----------------  -----
> >> ----------------------------\n");
> >> -    list_rsdp(rsdp);
> >> +    list_rsdp(rsdp, chksums);
> >>
> >>       return 0;
> >>   }
> >> @@ -187,13 +204,13 @@ static int do_acpi_dump(struct cmd_tbl *cmdtp,
> >> int flag, int argc,
> >>   }
> >>
> >>   U_BOOT_LONGHELP(acpi,
> >> -    "list  - list ACPI tables\n"
> >> +    "list [-c] - list ACPI tables [check checksums]\n"
>
> Let's keep things simple for the user:
>
> Always check the checksums.
> Only print a notice if a checksum is wrong.

I found that one table was wrong so I wanted a positive ack that each
table is either OK or BAD. I made the check optional though.

If you like, we could always check checksums but only print the OK/BAD
if -c is provided?

BTW I see you send a cleanup series for ACPI checksums. Is that based
on top of this series?

Regards,
Simon

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Tom Rini]

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

On Fri, Mar 21, 2025 at 07:38:27PM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 20 Mar 2025 at 15:22, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Mar 20, 2025 at 03:43:36AM +0000, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 19 Mar 2025 at 16:40, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Mar 19, 2025 at 03:03:49PM +0000, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 18 Mar 2025 at 16:53, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Tue, Mar 18, 2025 at 03:24:02PM +0000, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > > > > > > > bootable partitions for that media. This can mean that extlinux.conf
> > > > > > > > > files on the root disk are missed.
> > > > > > > > >
> > > > > > > > > Put this logic behind a flag and update the documentation.
> > > > > > > > >
> > > > > > > > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > > > > > > > work may provide a command (or some other mechanism) to control this.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > >
> > > > > > > > Isn't this backwards as the existing behavior (prior to bootstd) was to
> > > > > > > > scan and use these files, and so that's the behavior we need to preserve
> > > > > > > > by default.
> > > > > > >
> > > > > > > Not so far as I know, but I've already spent too much time trying to
> > > > > > > decode those scripts.
> > > > > > >
> > > > > > > If you look at scan_dev_for_boot_part, that is what I was trying to
> > > > > > > replicate with bootstd.
> > > > > >
> > > > > > The feedback from the community call where this was brought up was that
> > > > > > it used to work and now didn't, I thought. Heinrich, I think you had one
> > > > > > of the cases here (something about RISC-V and multiple distros?) or am I
> > > > > > misremembering?
> > > > >
> > > > > OK let's see if Heinrich chimes in.
> > > > >
> > > > > Are there notes for the call? It would help me if they could all be in
> > > > > a shared document, like we used to use[1].
> > > >
> > > > Yes, they're in email.
> > >
> > > What subject should I search for to find them? I looked for 'community
> > > call' and a few other things, but cannot find them.
> >
> > They're in reply to every announcement email and sent to all three
> > lists.
> 
> OK, well I can't find them, sorry. I did see the first one you sent
> but after that I have no record.
> 
> It sounds like you don't want them in a doc for easy reference?

I don't want them hidden away in some document, I want them easily
accessible.
https://lore.kernel.org/u-boot/20250128171923.GQ1233568@bill-the-cat/
https://lore.kernel.org/u-boot/20250211204927.GF1233568@bill-the-cat/
https://lore.kernel.org/u-boot/20250225191031.GS1233568@bill-the-cat/
https://lore.kernel.org/u-boot/20250311203117.GV2640854@bill-the-cat/

All of which are in reply to the announcement email for the release.

-- 
Tom

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

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Simon Glass]

Hi Tom,

On Fri, 28 Mar 2025 at 10:30, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Mar 21, 2025 at 07:38:27PM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 20 Mar 2025 at 15:22, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Mar 20, 2025 at 03:43:36AM +0000, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 19 Mar 2025 at 16:40, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Mar 19, 2025 at 03:03:49PM +0000, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Tue, 18 Mar 2025 at 16:53, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Tue, Mar 18, 2025 at 03:24:02PM +0000, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Tue, 18 Mar 2025 at 15:04, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> > > > > > > > >
> > > > > > > > > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > > > > > > > > bootable partitions for that media. This can mean that extlinux.conf
> > > > > > > > > > files on the root disk are missed.
> > > > > > > > > >
> > > > > > > > > > Put this logic behind a flag and update the documentation.
> > > > > > > > > >
> > > > > > > > > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > > > > > > > > work may provide a command (or some other mechanism) to control this.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > >
> > > > > > > > > Isn't this backwards as the existing behavior (prior to bootstd) was to
> > > > > > > > > scan and use these files, and so that's the behavior we need to preserve
> > > > > > > > > by default.
> > > > > > > >
> > > > > > > > Not so far as I know, but I've already spent too much time trying to
> > > > > > > > decode those scripts.
> > > > > > > >
> > > > > > > > If you look at scan_dev_for_boot_part, that is what I was trying to
> > > > > > > > replicate with bootstd.
> > > > > > >
> > > > > > > The feedback from the community call where this was brought up was that
> > > > > > > it used to work and now didn't, I thought. Heinrich, I think you had one
> > > > > > > of the cases here (something about RISC-V and multiple distros?) or am I
> > > > > > > misremembering?
> > > > > >
> > > > > > OK let's see if Heinrich chimes in.
> > > > > >
> > > > > > Are there notes for the call? It would help me if they could all be in
> > > > > > a shared document, like we used to use[1].
> > > > >
> > > > > Yes, they're in email.
> > > >
> > > > What subject should I search for to find them? I looked for 'community
> > > > call' and a few other things, but cannot find them.
> > >
> > > They're in reply to every announcement email and sent to all three
> > > lists.
> >
> > OK, well I can't find them, sorry. I did see the first one you sent
> > but after that I have no record.
> >
> > It sounds like you don't want them in a doc for easy reference?
>
> I don't want them hidden away in some document, I want them easily
> accessible.
> https://lore.kernel.org/u-boot/20250128171923.GQ1233568@bill-the-cat/
> https://lore.kernel.org/u-boot/20250211204927.GF1233568@bill-the-cat/
> https://lore.kernel.org/u-boot/20250225191031.GS1233568@bill-the-cat/
> https://lore.kernel.org/u-boot/20250311203117.GV2640854@bill-the-cat/
>
> All of which are in reply to the announcement email for the release.

OK thanks, I see it now. I need to search for '[ann]' not 'community meeting'.

I've copied them into the doc as I find it a bit easier to keep track
of. So much email.

Regards,
Simon

>
> --
> Tom

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Tom Rini]

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

On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:

> Any 'bootable' flag in a DOS partition causes boostd to only scan
> bootable partitions for that media. This can mean that extlinux.conf
> files on the root disk are missed.
> 
> Put this logic behind a flag and update the documentation.
> 
> For now, the flag is enabled, to preserve existing behaviour. Future
> work may provide a command (or some other mechanism) to control this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v4)
> 
> Changes in v4:
> - Rewrite the commit message
> - Enable the flag by default
> 
> Changes in v3:
> - Add new patch to consider non-bootable partitions
> 
>  boot/bootdev-uclass.c            | 4 +++-
>  cmd/bootflow.c                   | 2 +-
>  doc/develop/bootstd/overview.rst | 5 +++--
>  include/bootflow.h               | 2 ++
>  test/boot/bootdev.c              | 1 +
>  test/boot/bootflow.c             | 5 +++--
>  6 files changed, 13 insertions(+), 6 deletions(-)

Having spent another 10 minutes just now trying to understand things,
again:
- The commit message, and the implementation are either in disagreement
  or just too vague. Saying "to preserve existing behaviour" is unclear
  about which behavior is being preserved, since we're setting the flag
  to continue to only check bootable flagged partitions
  (BOOTFLOWIF_ONLY_BOOTABLE). But then we would continue to miss
  extlinux.conf files on root filesystems, which would seem to be the
  bug that needed fixing?
- It's still unclear if this makes bootstd match the exiting distro
  script behavior or not, and from your other emails it sounded like you
  were expecting someone else to dig around.

-- 
Tom

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

Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Tom Rini]

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

On Sat, Mar 15, 2025 at 02:26:00PM +0000, Simon Glass wrote:
> Update the string to take account of UARTs which are connected on I/O
> ports, as on x86.
> 
> Fix a typo in an error message in the same command, while we are here.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v3)
> 
> Changes in v3:
> - Add new patch to support IO UARTs for earlycon and console
> 
>  boot/bootflow.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 58a1afa7a75..4054a966af8 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
>  	*buf = '\0';
>  	if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
>  		snprintf(buf, sizeof(buf),
> -			 "uart8250,mmio32,%#lx,%dn8", info.addr,
> -			 info.baudrate);
> +			 "uart8250,%s,%#lx,%dn8",
> +			 info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> +			 "mmio", info.addr, info.baudrate);
>  	} else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
>  		snprintf(buf, sizeof(buf),
>  			 "pl011,mmio32,%#lx,%dn8", info.addr,

I suppose we're well past the point where we can delete
bootflow_cmdline_auto() itself because that's just going to lead us to
trouble down the line (5 years from now when the kernel adopts a new
preferred way to pass this info) and grows every platform by some amount
of space every time we add something new here.

-- 
Tom

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

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Simon Glass]

Hi Tom,

On Thu, 3 Apr 2025 at 03:28, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
>
> > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > bootable partitions for that media. This can mean that extlinux.conf
> > files on the root disk are missed.
> >
> > Put this logic behind a flag and update the documentation.
> >
> > For now, the flag is enabled, to preserve existing behaviour. Future
> > work may provide a command (or some other mechanism) to control this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v4)
> >
> > Changes in v4:
> > - Rewrite the commit message
> > - Enable the flag by default
> >
> > Changes in v3:
> > - Add new patch to consider non-bootable partitions
> >
> >  boot/bootdev-uclass.c            | 4 +++-
> >  cmd/bootflow.c                   | 2 +-
> >  doc/develop/bootstd/overview.rst | 5 +++--
> >  include/bootflow.h               | 2 ++
> >  test/boot/bootdev.c              | 1 +
> >  test/boot/bootflow.c             | 5 +++--
> >  6 files changed, 13 insertions(+), 6 deletions(-)
>
> Having spent another 10 minutes just now trying to understand things,
> again:
> - The commit message, and the implementation are either in disagreement
>   or just too vague. Saying "to preserve existing behaviour" is unclear
>   about which behavior is being preserved, since we're setting the flag
>   to continue to only check bootable flagged partitions
>   (BOOTFLOWIF_ONLY_BOOTABLE). But then we would continue to miss
>   extlinux.conf files on root filesystems, which would seem to be the
>   bug that needed fixing?

How about 'to preserve the existing behaviour of bootstd which is to
ignore non-bootable partitions so long as there is at least one
bootable partition on the disk"  ?

> - It's still unclear if this makes bootstd match the exiting distro
>   script behavior or not, and from your other emails it sounded like you
>   were expecting someone else to dig around.

The distro scripts include the code below. My reading of it is that it
only considers bootable partitions, except that if there is none, then
it always scans partition 1.

"scan_dev_for_boot_part="                                         \
"part list ${devtype} ${devnum} -bootable devplist; "     \
"env exists devplist || setenv devplist 1; "              \
"for distro_bootpart in ${devplist}; do "                 \
"if fstype ${devtype} "                           \
"${devnum}:${distro_bootpart} "   \
"bootfstype; then "               \
"part uuid ${devtype} "                   \
"${devnum}:${distro_bootpart} "   \
"distro_bootpart_uuid ; "         \
"run scan_dev_for_boot; "                 \
"fi; "                                            \
"done; "                                                  \
"setenv devplist\0"   \
\

Regards,
Simon

Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Simon Glass]

Hi Tom,

On Thu, 3 Apr 2025 at 03:29, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Mar 15, 2025 at 02:26:00PM +0000, Simon Glass wrote:
> > Update the string to take account of UARTs which are connected on I/O
> > ports, as on x86.
> >
> > Fix a typo in an error message in the same command, while we are here.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v3)
> >
> > Changes in v3:
> > - Add new patch to support IO UARTs for earlycon and console
> >
> >  boot/bootflow.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > index 58a1afa7a75..4054a966af8 100644
> > --- a/boot/bootflow.c
> > +++ b/boot/bootflow.c
> > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
> >       *buf = '\0';
> >       if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
> >               snprintf(buf, sizeof(buf),
> > -                      "uart8250,mmio32,%#lx,%dn8", info.addr,
> > -                      info.baudrate);
> > +                      "uart8250,%s,%#lx,%dn8",
> > +                      info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> > +                      "mmio", info.addr, info.baudrate);
> >       } else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
> >               snprintf(buf, sizeof(buf),
> >                        "pl011,mmio32,%#lx,%dn8", info.addr,
>
> I suppose we're well past the point where we can delete
> bootflow_cmdline_auto() itself because that's just going to lead us to
> trouble down the line (5 years from now when the kernel adopts a new
> preferred way to pass this info) and grows every platform by some amount
> of space every time we add something new here.

Well firstly, why would you want to delete this command? It is very
useful to be able to change the cmdline.

This command is only available with BOOTSTD_FULL, which is less than
10% of boards.

Regards,
Simon

Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Tom Rini]

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

On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 3 Apr 2025 at 03:29, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Mar 15, 2025 at 02:26:00PM +0000, Simon Glass wrote:
> > > Update the string to take account of UARTs which are connected on I/O
> > > ports, as on x86.
> > >
> > > Fix a typo in an error message in the same command, while we are here.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v3)
> > >
> > > Changes in v3:
> > > - Add new patch to support IO UARTs for earlycon and console
> > >
> > >  boot/bootflow.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > index 58a1afa7a75..4054a966af8 100644
> > > --- a/boot/bootflow.c
> > > +++ b/boot/bootflow.c
> > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
> > >       *buf = '\0';
> > >       if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
> > >               snprintf(buf, sizeof(buf),
> > > -                      "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > -                      info.baudrate);
> > > +                      "uart8250,%s,%#lx,%dn8",
> > > +                      info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> > > +                      "mmio", info.addr, info.baudrate);
> > >       } else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
> > >               snprintf(buf, sizeof(buf),
> > >                        "pl011,mmio32,%#lx,%dn8", info.addr,
> >
> > I suppose we're well past the point where we can delete
> > bootflow_cmdline_auto() itself because that's just going to lead us to
> > trouble down the line (5 years from now when the kernel adopts a new
> > preferred way to pass this info) and grows every platform by some amount
> > of space every time we add something new here.
> 
> Well firstly, why would you want to delete this command? It is very
> useful to be able to change the cmdline.
> 
> This command is only available with BOOTSTD_FULL, which is less than
> 10% of boards.

Because it's automatic non-obvious stuff. We should not be modifying the
command line at all. Is it even documented that we're doing this?

-- 
Tom

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

Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Tom Rini]

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

On Wed, Apr 02, 2025 at 01:50:33PM -0600, Tom Rini wrote:
> On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> > Hi Tom,
> > 
> > On Thu, 3 Apr 2025 at 03:29, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sat, Mar 15, 2025 at 02:26:00PM +0000, Simon Glass wrote:
> > > > Update the string to take account of UARTs which are connected on I/O
> > > > ports, as on x86.
> > > >
> > > > Fix a typo in an error message in the same command, while we are here.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > (no changes since v3)
> > > >
> > > > Changes in v3:
> > > > - Add new patch to support IO UARTs for earlycon and console
> > > >
> > > >  boot/bootflow.c | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > index 58a1afa7a75..4054a966af8 100644
> > > > --- a/boot/bootflow.c
> > > > +++ b/boot/bootflow.c
> > > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
> > > >       *buf = '\0';
> > > >       if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
> > > >               snprintf(buf, sizeof(buf),
> > > > -                      "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > > -                      info.baudrate);
> > > > +                      "uart8250,%s,%#lx,%dn8",
> > > > +                      info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> > > > +                      "mmio", info.addr, info.baudrate);
> > > >       } else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
> > > >               snprintf(buf, sizeof(buf),
> > > >                        "pl011,mmio32,%#lx,%dn8", info.addr,
> > >
> > > I suppose we're well past the point where we can delete
> > > bootflow_cmdline_auto() itself because that's just going to lead us to
> > > trouble down the line (5 years from now when the kernel adopts a new
> > > preferred way to pass this info) and grows every platform by some amount
> > > of space every time we add something new here.
> > 
> > Well firstly, why would you want to delete this command? It is very
> > useful to be able to change the cmdline.
> > 
> > This command is only available with BOOTSTD_FULL, which is less than
> > 10% of boards.
> 
> Because it's automatic non-obvious stuff. We should not be modifying the
> command line at all. Is it even documented that we're doing this?

To be clearer, the more I review your changes, the more I see a blurred
line that I do not this is good between "I found this handy while
doing something" and "This is a good generic design / feature".

That it can be annoying at times to add the debug uart information is
not a new unique problem. It's something that's generally normally
solved. I assume you hit this on x86 where it's more annoying than most.
But a generic feature it should not have been.

-- 
Tom

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

Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Simon Glass]

Hi Tom,

On Thu, 3 Apr 2025 at 08:57, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Apr 02, 2025 at 01:50:33PM -0600, Tom Rini wrote:
> > On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 3 Apr 2025 at 03:29, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sat, Mar 15, 2025 at 02:26:00PM +0000, Simon Glass wrote:
> > > > > Update the string to take account of UARTs which are connected on I/O
> > > > > ports, as on x86.
> > > > >
> > > > > Fix a typo in an error message in the same command, while we are here.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v3)
> > > > >
> > > > > Changes in v3:
> > > > > - Add new patch to support IO UARTs for earlycon and console
> > > > >
> > > > >  boot/bootflow.c | 7 ++++---
> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > index 58a1afa7a75..4054a966af8 100644
> > > > > --- a/boot/bootflow.c
> > > > > +++ b/boot/bootflow.c
> > > > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
> > > > >       *buf = '\0';
> > > > >       if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
> > > > >               snprintf(buf, sizeof(buf),
> > > > > -                      "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > > > -                      info.baudrate);
> > > > > +                      "uart8250,%s,%#lx,%dn8",
> > > > > +                      info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> > > > > +                      "mmio", info.addr, info.baudrate);
> > > > >       } else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
> > > > >               snprintf(buf, sizeof(buf),
> > > > >                        "pl011,mmio32,%#lx,%dn8", info.addr,
> > > >
> > > > I suppose we're well past the point where we can delete
> > > > bootflow_cmdline_auto() itself because that's just going to lead us to
> > > > trouble down the line (5 years from now when the kernel adopts a new
> > > > preferred way to pass this info) and grows every platform by some amount
> > > > of space every time we add something new here.
> > >
> > > Well firstly, why would you want to delete this command? It is very
> > > useful to be able to change the cmdline.
> > >
> > > This command is only available with BOOTSTD_FULL, which is less than
> > > 10% of boards.
> >
> > Because it's automatic non-obvious stuff. We should not be modifying the
> > command line at all. Is it even documented that we're doing this?

Yes, the command is documented. It only happens when you run the
command. Otherwise the command line is not modified.

As you know U-Boot has bootargs but that doesn't work with extlinux
and the like, so we have this command, which does.

>
> To be clearer, the more I review your changes, the more I see a blurred
> line that I do not this is good between "I found this handy while
> doing something" and "This is a good generic design / feature".
>
> That it can be annoying at times to add the debug uart information is
> not a new unique problem. It's something that's generally normally
> solved. I assume you hit this on x86 where it's more annoying than most.
> But a generic feature it should not have been.

I'm not even sure why you wrote this email, then.

U-Boot has all the information needed to set up the UART correctly and
it is extremely tedious to do otherwise. This is a bootloader, so it
needs to support kernel development. I actually think this is a really
cool feature.

At this point, I really would encourage you to look at what you are
trying to achieve with all these 'no' emails. Collected together they
create a substantial corpus of negativity. Why not just start saying
'yes' instead of 'no' for a while and see how things go?

Regards,
Simon

Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Tom Rini]

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

On Thu, Apr 03, 2025 at 12:54:42PM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 3 Apr 2025 at 08:57, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Apr 02, 2025 at 01:50:33PM -0600, Tom Rini wrote:
> > > On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 3 Apr 2025 at 03:29, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sat, Mar 15, 2025 at 02:26:00PM +0000, Simon Glass wrote:
> > > > > > Update the string to take account of UARTs which are connected on I/O
> > > > > > ports, as on x86.
> > > > > >
> > > > > > Fix a typo in an error message in the same command, while we are here.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > (no changes since v3)
> > > > > >
> > > > > > Changes in v3:
> > > > > > - Add new patch to support IO UARTs for earlycon and console
> > > > > >
> > > > > >  boot/bootflow.c | 7 ++++---
> > > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > > index 58a1afa7a75..4054a966af8 100644
> > > > > > --- a/boot/bootflow.c
> > > > > > +++ b/boot/bootflow.c
> > > > > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
> > > > > >       *buf = '\0';
> > > > > >       if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
> > > > > >               snprintf(buf, sizeof(buf),
> > > > > > -                      "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > > > > -                      info.baudrate);
> > > > > > +                      "uart8250,%s,%#lx,%dn8",
> > > > > > +                      info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> > > > > > +                      "mmio", info.addr, info.baudrate);
> > > > > >       } else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
> > > > > >               snprintf(buf, sizeof(buf),
> > > > > >                        "pl011,mmio32,%#lx,%dn8", info.addr,
> > > > >
> > > > > I suppose we're well past the point where we can delete
> > > > > bootflow_cmdline_auto() itself because that's just going to lead us to
> > > > > trouble down the line (5 years from now when the kernel adopts a new
> > > > > preferred way to pass this info) and grows every platform by some amount
> > > > > of space every time we add something new here.
> > > >
> > > > Well firstly, why would you want to delete this command? It is very
> > > > useful to be able to change the cmdline.
> > > >
> > > > This command is only available with BOOTSTD_FULL, which is less than
> > > > 10% of boards.
> > >
> > > Because it's automatic non-obvious stuff. We should not be modifying the
> > > command line at all. Is it even documented that we're doing this?
> 
> Yes, the command is documented. It only happens when you run the
> command. Otherwise the command line is not modified.
> 
> As you know U-Boot has bootargs but that doesn't work with extlinux
> and the like, so we have this command, which does.
> 
> >
> > To be clearer, the more I review your changes, the more I see a blurred
> > line that I do not this is good between "I found this handy while
> > doing something" and "This is a good generic design / feature".
> >
> > That it can be annoying at times to add the debug uart information is
> > not a new unique problem. It's something that's generally normally
> > solved. I assume you hit this on x86 where it's more annoying than most.
> > But a generic feature it should not have been.
> 
> I'm not even sure why you wrote this email, then.

Because this is another little thing I'm being flexible on and not
naking, I just think it's wrong-headed. And you seem to think I'm never
being flexible. So I started this by saying I'm not naking it, I just
think it's wrong.

> U-Boot has all the information needed to set up the UART correctly and
> it is extremely tedious to do otherwise. This is a bootloader, so it
> needs to support kernel development. I actually think this is a really
> cool feature.

It's "cool" until it breaks. And grows platforms for features they
can't/won't support.

-- 
Tom

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

Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Tom Rini]

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

On Thu, Apr 03, 2025 at 08:22:30AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 3 Apr 2025 at 03:28, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> >
> > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > bootable partitions for that media. This can mean that extlinux.conf
> > > files on the root disk are missed.
> > >
> > > Put this logic behind a flag and update the documentation.
> > >
> > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > work may provide a command (or some other mechanism) to control this.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v4)
> > >
> > > Changes in v4:
> > > - Rewrite the commit message
> > > - Enable the flag by default
> > >
> > > Changes in v3:
> > > - Add new patch to consider non-bootable partitions
> > >
> > >  boot/bootdev-uclass.c            | 4 +++-
> > >  cmd/bootflow.c                   | 2 +-
> > >  doc/develop/bootstd/overview.rst | 5 +++--
> > >  include/bootflow.h               | 2 ++
> > >  test/boot/bootdev.c              | 1 +
> > >  test/boot/bootflow.c             | 5 +++--
> > >  6 files changed, 13 insertions(+), 6 deletions(-)
> >
> > Having spent another 10 minutes just now trying to understand things,
> > again:
> > - The commit message, and the implementation are either in disagreement
> >   or just too vague. Saying "to preserve existing behaviour" is unclear
> >   about which behavior is being preserved, since we're setting the flag
> >   to continue to only check bootable flagged partitions
> >   (BOOTFLOWIF_ONLY_BOOTABLE). But then we would continue to miss
> >   extlinux.conf files on root filesystems, which would seem to be the
> >   bug that needed fixing?
> 
> How about 'to preserve the existing behaviour of bootstd which is to
> ignore non-bootable partitions so long as there is at least one
> bootable partition on the disk"  ?

Yes, that's better. I've reworded the commit message with that.

> > - It's still unclear if this makes bootstd match the exiting distro
> >   script behavior or not, and from your other emails it sounded like you
> >   were expecting someone else to dig around.
> 
> The distro scripts include the code below. My reading of it is that it
> only considers bootable partitions, except that if there is none, then
> it always scans partition 1.
> 
> "scan_dev_for_boot_part="                                         \
> "part list ${devtype} ${devnum} -bootable devplist; "     \
> "env exists devplist || setenv devplist 1; "              \
> "for distro_bootpart in ${devplist}; do "                 \
> "if fstype ${devtype} "                           \
> "${devnum}:${distro_bootpart} "   \
> "bootfstype; then "               \
> "part uuid ${devtype} "                   \
> "${devnum}:${distro_bootpart} "   \
> "distro_bootpart_uuid ; "         \
> "run scan_dev_for_boot; "                 \
> "fi; "                                            \
> "done; "                                                  \
> "setenv devplist\0"   \
> \

OK, thanks, I agree with your reading of that as well.

-- 
Tom

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

Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Simon Glass]

Hi Tom,

On Thu, 3 Apr 2025 at 13:26, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Apr 03, 2025 at 12:54:42PM +1300, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 3 Apr 2025 at 08:57, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Apr 02, 2025 at 01:50:33PM -0600, Tom Rini wrote:
> > > > On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 3 Apr 2025 at 03:29, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sat, Mar 15, 2025 at 02:26:00PM +0000, Simon Glass wrote:
> > > > > > > Update the string to take account of UARTs which are connected on I/O
> > > > > > > ports, as on x86.
> > > > > > >
> > > > > > > Fix a typo in an error message in the same command, while we are here.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > (no changes since v3)
> > > > > > >
> > > > > > > Changes in v3:
> > > > > > > - Add new patch to support IO UARTs for earlycon and console
> > > > > > >
> > > > > > >  boot/bootflow.c | 7 ++++---
> > > > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > > > index 58a1afa7a75..4054a966af8 100644
> > > > > > > --- a/boot/bootflow.c
> > > > > > > +++ b/boot/bootflow.c
> > > > > > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
> > > > > > >       *buf = '\0';
> > > > > > >       if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
> > > > > > >               snprintf(buf, sizeof(buf),
> > > > > > > -                      "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > > > > > -                      info.baudrate);
> > > > > > > +                      "uart8250,%s,%#lx,%dn8",
> > > > > > > +                      info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> > > > > > > +                      "mmio", info.addr, info.baudrate);
> > > > > > >       } else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
> > > > > > >               snprintf(buf, sizeof(buf),
> > > > > > >                        "pl011,mmio32,%#lx,%dn8", info.addr,
> > > > > >
> > > > > > I suppose we're well past the point where we can delete
> > > > > > bootflow_cmdline_auto() itself because that's just going to lead us to
> > > > > > trouble down the line (5 years from now when the kernel adopts a new
> > > > > > preferred way to pass this info) and grows every platform by some amount
> > > > > > of space every time we add something new here.
> > > > >
> > > > > Well firstly, why would you want to delete this command? It is very
> > > > > useful to be able to change the cmdline.
> > > > >
> > > > > This command is only available with BOOTSTD_FULL, which is less than
> > > > > 10% of boards.
> > > >
> > > > Because it's automatic non-obvious stuff. We should not be modifying the
> > > > command line at all. Is it even documented that we're doing this?
> >
> > Yes, the command is documented. It only happens when you run the
> > command. Otherwise the command line is not modified.
> >
> > As you know U-Boot has bootargs but that doesn't work with extlinux
> > and the like, so we have this command, which does.
> >
> > >
> > > To be clearer, the more I review your changes, the more I see a blurred
> > > line that I do not this is good between "I found this handy while
> > > doing something" and "This is a good generic design / feature".
> > >
> > > That it can be annoying at times to add the debug uart information is
> > > not a new unique problem. It's something that's generally normally
> > > solved. I assume you hit this on x86 where it's more annoying than most.
> > > But a generic feature it should not have been.
> >
> > I'm not even sure why you wrote this email, then.
>
> Because this is another little thing I'm being flexible on and not
> naking, I just think it's wrong-headed. And you seem to think I'm never
> being flexible. So I started this by saying I'm not naking it, I just
> think it's wrong.
>
> > U-Boot has all the information needed to set up the UART correctly and
> > it is extremely tedious to do otherwise. This is a bootloader, so it
> > needs to support kernel development. I actually think this is a really
> > cool feature.
>
> It's "cool" until it breaks. And grows platforms for features they
> can't/won't support.

OK, well let's drop this discussion, since you are not naking it.

Regards,
Simon

Re: [PATCH v5 00/46] x86: Improve operation under QEMU

[Tom Rini]

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

On Sat, Mar 15, 2025 at 02:25:20PM +0000, Simon Glass wrote:

> U-Boot can start and boot an OS in both qemu-x86 and qemu-x86_64 but it
> is not perfect.
> 
> With both builds, executing the VESA ROM causes an intermittent hang, at
> least on some AMD CPUs.
> 
> With qemu-x86_64 kvm cannot be used since the move to long mode (64-bit)
> is done in a way that works on real hardware but not with QEMU. This
> means that performance is 4-5x slower than it could be, at least on my
> CPU.
> 
> We can work around the first problem by using Bochs, which is anyway a
> better choice than VESA for QEMU. The second can be addressed by using
> the same descriptor across the jump to long mode.
> 
> With an MTRR fix this allows booting into Ubuntu on qemu-x86_64
> 
> In v3 some e820 patches are included to make booting reliable and avoid
> ACPI tables being dropped. Also, several MTTR problems are addressed, to
> support memory sizes above 4GB reliably.

For the series, applied to u-boot/next, thanks.

-- 
Tom

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