[PATCH 0/9] x86: Fixes for distro booting

[PATCH 0/9] x86: Fixes for distro booting

[Simon Glass]

This series collects together various fixes found whn testing out booting
of distros using QEMU on x86. Together they improve functionality, but it
is not perfect yet. Some notes are included as to the current state.


Simon Glass (9):
  x86: spl: Drop unwanted debug()
  video: Tidy up Makefile rule for video
  x86: Run QEMU machine setup in SPL
  Revert "x86: Switch QEMU over to use the bochs driver"
  board_f: Fix corruption of relocaddr
  x86: Correct copying of BIOS mode information
  video: Add a Kconfig option for SPL video handoff
  x86: Enable useful options for qemu-86
  x86: Update qemu documentation

 arch/x86/cpu/qemu/Kconfig        |  2 +-
 arch/x86/cpu/qemu/qemu.c         |  2 +-
 arch/x86/include/asm/qemu.h      | 14 +++++
 arch/x86/lib/bios.c              |  2 +-
 arch/x86/lib/spl.c               |  4 +-
 common/board_f.c                 |  4 +-
 configs/qemu-x86_64_defconfig    |  4 ++
 configs/qemu-x86_defconfig       | 12 +++++
 doc/board/emulation/qemu-x86.rst | 88 ++++++++++++++++++++++++++++++--
 drivers/Makefile                 |  4 +-
 drivers/video/Kconfig            | 10 ++++
 drivers/video/video-uclass.c     |  2 +-
 include/vesa.h                   |  4 +-
 13 files changed, 137 insertions(+), 15 deletions(-)
 create mode 100644 arch/x86/include/asm/qemu.h

-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 1/9] x86: spl: Drop unwanted debug()

[Simon Glass]

This was left over from some previous debugging. Drop it.

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

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

diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
index b6812bb8ca2b..55c061570864 100644
--- a/arch/x86/lib/spl.c
+++ b/arch/x86/lib/spl.c
@@ -137,7 +137,6 @@ static int x86_spl_init(void)
 	}
 
 #ifndef CONFIG_SYS_COREBOOT
-	log_debug("bss\n");
 	debug("BSS clear from %lx to %lx len %lx\n", (ulong)&__bss_start,
 	      (ulong)&__bss_end, (ulong)&__bss_end - (ulong)&__bss_start);
 	memset(&__bss_start, 0, (ulong)&__bss_end - (ulong)&__bss_start);
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 2/9] video: Tidy up Makefile rule for video

[Simon Glass]

Drop the duplication and add a single rule which can handle SPL as well.

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

 drivers/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 3bc6d279d7cb..7272681fc741 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -39,6 +39,8 @@ obj-$(CONFIG_$(SPL_TPL_)TPM) += tpm/
 obj-$(CONFIG_$(SPL_)NVME) += nvme/
 obj-$(CONFIG_XEN) += xen/
 obj-$(CONFIG_$(SPL_)FPGA) += fpga/
+obj-$(CONFIG_$(SPL_TPL_)VIDEO) += video/
+
 obj-y += bus/
 
 ifndef CONFIG_TPL_BUILD
@@ -64,7 +66,6 @@ obj-$(CONFIG_SPL_USB_HOST) += usb/host/
 obj-$(CONFIG_SPL_SATA) += ata/ scsi/
 obj-$(CONFIG_SPL_LEGACY_BLOCK) += block/
 obj-$(CONFIG_SPL_THERMAL) += thermal/
-obj-$(CONFIG_SPL_VIDEO) +=video/
 
 endif
 endif
@@ -99,7 +100,6 @@ obj-y += rtc/
 obj-y += scsi/
 obj-y += sound/
 obj-y += spmi/
-obj-y += video/
 obj-y += watchdog/
 obj-$(CONFIG_QE) += qe/
 obj-$(CONFIG_U_QE) += qe/
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 3/9] x86: Run QEMU machine setup in SPL

[Simon Glass]

Call the hardware-init function from QEMU from SPL. This allows the video
BIOS to operate correctly.

Create an x86-wide qemu.h header to avoid having to #ifdef the header in
spl.c

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

 arch/x86/cpu/qemu/qemu.c    |  2 +-
 arch/x86/include/asm/qemu.h | 14 ++++++++++++++
 arch/x86/lib/spl.c          |  3 +++
 3 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/asm/qemu.h

diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
index 274978c023b6..70414556086c 100644
--- a/arch/x86/cpu/qemu/qemu.c
+++ b/arch/x86/cpu/qemu/qemu.c
@@ -48,7 +48,7 @@ static void enable_pm_ich9(void)
 	pci_write_config32(ICH9_PM, PMBA, CONFIG_ACPI_PM1_BASE | 1);
 }
 
-static void qemu_chipset_init(void)
+void qemu_chipset_init(void)
 {
 	u16 device, xbcs;
 	int pam, i;
diff --git a/arch/x86/include/asm/qemu.h b/arch/x86/include/asm/qemu.h
new file mode 100644
index 000000000000..f1e95ffd7a4d
--- /dev/null
+++ b/arch/x86/include/asm/qemu.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Generic QEMU header
+ *
+ * Copyright 2023 Google LLC
+ */
+
+#ifndef __QEMU_H
+#define __QEMU_H
+
+/* set up the chipset for QEMU so that video can be used */
+void qemu_chipset_init(void);
+
+#endif
diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
index 55c061570864..74949b2c13cb 100644
--- a/arch/x86/lib/spl.c
+++ b/arch/x86/lib/spl.c
@@ -27,6 +27,7 @@
 #include <asm/mtrr.h>
 #include <asm/pci.h>
 #include <asm/processor.h>
+#include <asm/qemu.h>
 #include <asm/spl.h>
 #include <asm-generic/sections.h>
 
@@ -291,6 +292,8 @@ void spl_board_init(void)
 #ifndef CONFIG_TPL
 	preloader_console_init();
 #endif
+	if (IS_ENABLED(CONFIG_X86) && IS_ENABLED(CONFIG_QEMU))
+		qemu_chipset_init();
 
 	if (CONFIG_IS_ENABLED(VIDEO)) {
 		struct udevice *dev;
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Simon Glass]

Unfortunately the bochs driver does not currently work with distros. It
causes a hang sometime between grub menu selection and the OS displaying
something.

This reverts commit b8956425d525c3c25fd218f252f89a5e44df6a9f.

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

 arch/x86/cpu/qemu/Kconfig     | 2 +-
 configs/qemu-x86_64_defconfig | 4 ++++
 configs/qemu-x86_defconfig    | 3 +++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/cpu/qemu/Kconfig b/arch/x86/cpu/qemu/Kconfig
index aa329b0dab29..f8f2f6473088 100644
--- a/arch/x86/cpu/qemu/Kconfig
+++ b/arch/x86/cpu/qemu/Kconfig
@@ -12,7 +12,7 @@ config QEMU
 	imply SYS_NS16550
 	imply USB
 	imply USB_EHCI_HCD
-	imply VIDEO_BOCHS
+	imply VIDEO_VESA
 
 if QEMU
 
diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
index 4a15c516710e..b1400876a2ff 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -84,6 +84,10 @@ CONFIG_SPL_DM_RTC=y
 CONFIG_SYS_NS16550_PORT_MAPPED=y
 CONFIG_SPI=y
 CONFIG_USB_KEYBOARD=y
+CONFIG_SPL_VIDEO=y
+CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
+CONFIG_FRAMEBUFFER_VESA_MODE_USER=y
+CONFIG_FRAMEBUFFER_VESA_MODE=0x144
 CONFIG_CONSOLE_SCROLL_LINES=5
 CONFIG_FAT_BLK_XLATE=y
 # CONFIG_SPL_USE_TINY_PRINTF is not set
diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
index 95a6ff9ae799..56788cd185fd 100644
--- a/configs/qemu-x86_defconfig
+++ b/configs/qemu-x86_defconfig
@@ -53,6 +53,9 @@ CONFIG_NVME_PCI=y
 CONFIG_SYS_NS16550_PORT_MAPPED=y
 CONFIG_SPI=y
 CONFIG_USB_KEYBOARD=y
+CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
+CONFIG_FRAMEBUFFER_VESA_MODE_USER=y
+CONFIG_FRAMEBUFFER_VESA_MODE=0x144
 CONFIG_CONSOLE_SCROLL_LINES=5
 CONFIG_GENERATE_ACPI_TABLE=y
 # CONFIG_GZIP is not set
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 5/9] board_f: Fix corruption of relocaddr

[Simon Glass]

When the video framebuffer comes from the bloblist, we should not change
relocaddr to this address, since it interfers with the normal memory
allocation.

This fixes a boot loop in qemu-x86_64

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
---

 common/board_f.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c
index 7d2c380e91e2..5c8646b22283 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -419,7 +419,6 @@ static int reserve_video(void)
 		if (!ho)
 			return log_msg_ret("blf", -ENOENT);
 		video_reserve_from_bloblist(ho);
-		gd->relocaddr = ho->fb;
 	} else if (CONFIG_IS_ENABLED(VIDEO)) {
 		ulong addr;
 		int ret;
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 6/9] x86: Correct copying of BIOS mode information

[Simon Glass]

This is copying beyond the end of the destination buffer. Correct the code
by using a constant for the buffer size.

This long-standing bug prevents virtio bootdevs working correctly on
qemu-x86 at present.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: 0ca2426beae ("x86: Add support for running option ROMs natively")
---

 arch/x86/lib/bios.c | 2 +-
 include/vesa.h      | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/lib/bios.c b/arch/x86/lib/bios.c
index e29cae78e509..3a9d7f5ddd41 100644
--- a/arch/x86/lib/bios.c
+++ b/arch/x86/lib/bios.c
@@ -204,7 +204,7 @@ static u8 vbe_get_mode_info(struct vesa_state *mi)
 
 	realmode_interrupt(0x10, VESA_GET_MODE_INFO, 0x0000, mi->video_mode,
 			   0x0000, buffer_seg, buffer_adr);
-	memcpy(mi->mode_info_block, buffer, sizeof(struct vesa_state));
+	memcpy(mi->mode_info_block, buffer, VESA_MODE_INFO_SIZE);
 	mi->valid = true;
 
 	return 0;
diff --git a/include/vesa.h b/include/vesa.h
index 9285bfa921a2..28828ab56aa4 100644
--- a/include/vesa.h
+++ b/include/vesa.h
@@ -83,12 +83,14 @@ struct __packed vesa_mode_info {
 	u8 reserved[206];
 };
 
+#define VESA_MODE_INFO_SIZE	256
+
 struct vesa_state {
 	u16 video_mode;
 	bool valid;
 	union {
 		struct vesa_mode_info vesa;
-		u8 mode_info_block[256];
+		u8 mode_info_block[VESA_MODE_INFO_SIZE];
 	};
 };
 
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 7/9] video: Add a Kconfig option for SPL video handoff

[Simon Glass]

At present this feature is enabled in SPL if a bloblist is available.
Some platforms may not want to use this, so add an option to allow the
feature to be disabled.

Note that the feature unfortunately only fills in part of the
video-handoff information, so causes failures on x86 platforms. For now,
disable it there.

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

 common/board_f.c             |  3 +--
 drivers/video/Kconfig        | 10 ++++++++++
 drivers/video/video-uclass.c |  2 +-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index 5c8646b22283..656bd716c7ec 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -411,8 +411,7 @@ __weak int arch_reserve_mmu(void)
 
 static int reserve_video(void)
 {
-	if (IS_ENABLED(CONFIG_SPL_VIDEO) && spl_phase() > PHASE_SPL &&
-	    CONFIG_IS_ENABLED(BLOBLIST)) {
+	if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
 		struct video_handoff *ho;
 
 		ho = bloblist_find(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho));
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index b41dc60cec59..0c9c14b021fe 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -1011,6 +1011,16 @@ config SPL_VIDEO
 if SPL_VIDEO
 source "drivers/video/tidss/Kconfig"
 
+config SPL_VIDEO_HANDOFF
+	bool "Pass the video frame-buffer through to U-Boot proper"
+	depends on SPL_BLOBLIST
+	default y if !X86
+	help
+	  Enable this to set up video-handoff information in SPL which can be
+	  picked up in U-Boot proper. This includes the frame buffer and
+	  various other pieces of information. With this enabled, SPL can set
+	  up video and avoid re-initing it later.
+
 config SPL_VIDEO_LOGO
 	bool "Show the U-Boot logo on the display at SPL"
 	default y if !SPL_SPLASH_SCREEN
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index 949595f1bc69..86262a3435c2 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -141,7 +141,7 @@ int video_reserve(ulong *addrp)
 	debug("Video frame buffers from %lx to %lx\n", gd->video_bottom,
 	      gd->video_top);
 
-	if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(BLOBLIST)) {
+	if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(VIDEO_HANDOFF)) {
 		struct video_handoff *ho;
 
 		ho = bloblist_add(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho), 0);
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 8/9] x86: Enable useful options for qemu-86

[Simon Glass]

This build can be used to boot 32-bit standard-distro builds. Enable some
more options, so that all possible EFI UUIDs are decoded, we can search
memory for tables, support the full set of standard-boot features, have
full logging along with debug UART and can boot from CDROM media.

This mirrors a similar patch for qemu-x86_64

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

 configs/qemu-x86_defconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
index 56788cd185fd..ff9df677ef4c 100644
--- a/configs/qemu-x86_defconfig
+++ b/configs/qemu-x86_defconfig
@@ -5,10 +5,14 @@ CONFIG_NR_DRAM_BANKS=8
 CONFIG_ENV_SIZE=0x40000
 CONFIG_MAX_CPUS=2
 CONFIG_DEFAULT_DEVICE_TREE="qemu-x86_i440fx"
+CONFIG_DEBUG_UART_BASE=0x3f8
+CONFIG_DEBUG_UART_CLOCK=1843200
+CONFIG_DEBUG_UART=y
 CONFIG_SMP=y
 CONFIG_GENERATE_PIRQ_TABLE=y
 CONFIG_GENERATE_MP_TABLE=y
 CONFIG_FIT=y
+CONFIG_BOOTSTD_FULL=y
 CONFIG_BOOTSTD_DEFAULTS=y
 CONFIG_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
@@ -16,6 +20,8 @@ CONFIG_SHOW_BOOT_PROGRESS=y
 CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="root=/dev/sdb3 init=/sbin/init rootwait ro"
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
+CONFIG_LOG=y
+CONFIG_LOGF_FUNC=y
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_LAST_STAGE_INIT=y
 CONFIG_PCI_INIT_R=y
@@ -23,11 +29,13 @@ CONFIG_SYS_PBSIZE=532
 CONFIG_CMD_CPU=y
 CONFIG_CMD_BOOTEFI_SELFTEST=y
 CONFIG_CMD_NVEDIT_EFI=y
+CONFIG_CMD_MEM_SEARCH=y
 CONFIG_CMD_IDE=y
 CONFIG_CMD_SPI=y
 CONFIG_CMD_USB=y
 # CONFIG_CMD_SETEXPR is not set
 CONFIG_BOOTP_BOOTFILESIZE=y
+CONFIG_CMD_EFIDEBUG=y
 CONFIG_CMD_TIME=y
 CONFIG_CMD_QFW=y
 CONFIG_CMD_BOOTSTAGE=y
@@ -57,5 +65,6 @@ CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
 CONFIG_FRAMEBUFFER_VESA_MODE_USER=y
 CONFIG_FRAMEBUFFER_VESA_MODE=0x144
 CONFIG_CONSOLE_SCROLL_LINES=5
+CONFIG_FAT_BLK_XLATE=y
 CONFIG_GENERATE_ACPI_TABLE=y
 # CONFIG_GZIP is not set
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 9/9] x86: Update qemu documentation

[Simon Glass]

Add some hints and observations related to booting distros on QEMU on x86.

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

 doc/board/emulation/qemu-x86.rst | 88 ++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 4 deletions(-)

diff --git a/doc/board/emulation/qemu-x86.rst b/doc/board/emulation/qemu-x86.rst
index e7dd4e994d38..15f56b6bc706 100644
--- a/doc/board/emulation/qemu-x86.rst
+++ b/doc/board/emulation/qemu-x86.rst
@@ -113,7 +113,87 @@ sure the specified CPU supports 64-bit like '-cpu core2duo'. Conversely
 '-cpu pentium' won't work for obvious reasons that the processor only
 supports 32-bit.
 
-Note 64-bit support is very preliminary at this point. Lots of features
-are missing in the 64-bit world. One notable feature is the VGA console
-support which is currently missing, so that you must specify '-nographic'
-to get 64-bit U-Boot up and running.
+Booting distros
+---------------
+
+It is possible to install and boot a standard Linux distribution using
+qemu-x86_64 by setting up a root disk::
+
+   qemu-img create root.img 10G
+
+then using the installer to install. For example, with Ubuntu 2023.04::
+
+   qemu-system-x86_64 -m 8G -smp 4 -bios /tmp/b/qemu-x86_64/u-boot.rom \
+     -drive file=root.img,if=virtio,driver=raw \
+     -drive file=ubuntu-23.04-desktop-amd64.iso,if=virtio,driver=raw
+
+You can also add `-serial mon:stdio` if you want the serial console to show as
+well as the video.
+
+The output will be something like this::
+
+   U-Boot SPL 2023.07 (Jul 23 2023 - 08:00:12 -0600)
+   Trying to boot from SPI
+   Jumping to 64-bit U-Boot: Note many features are missing
+
+
+   U-Boot 2023.07 (Jul 23 2023 - 08:00:12 -0600)
+
+   CPU:   QEMU Virtual CPU version 2.5+
+   DRAM:  8 GiB
+   Core:  20 devices, 13 uclasses, devicetree: separate
+   Loading Environment from nowhere... OK
+   Model: QEMU x86 (I440FX)
+   Net:   e1000: 52:54:00:12:34:56
+          eth0: e1000#0
+   Hit any key to stop autoboot:  0
+   Scanning for bootflows in all bootdevs
+   Seq  Method       State   Uclass    Part  Name                      Filename
+   ---  -----------  ------  --------  ----  ------------------------  ----------------
+   Scanning global bootmeth 'efi_mgr':
+   Hunting with: nvme
+   Hunting with: qfw
+   Hunting with: scsi
+   scanning bus for devices...
+   Hunting with: virtio
+   Scanning bootdev 'qfw_pio.bootdev':
+   fatal: no kernel available
+   Scanning bootdev 'virtio-blk#0.bootdev':
+   Scanning bootdev 'virtio-blk#1.bootdev':
+     0  efi          ready   virtio       2  virtio-blk#1.bootdev.part efi/boot/bootx64.efi
+   ** Booting bootflow 'virtio-blk#1.bootdev.part_2' with efi
+   EFI using ACPI tables at f0060
+        efi_install_fdt() WARNING: Can't have ACPI table and device tree - ignoring DT.
+          efi_run_image() Booting /efi\boot\bootx64.efi
+   error: file `/boot/' not found.
+
+Standard boot looks through various available devices and finds the virtio
+disks, then boots from the first one. After a second or so the grub menu appears
+and you can work through the installer flow normally.
+
+Note that standard boot will not find 32-bit distros, since it looks for a
+different filename.
+
+Current limitations
+-------------------
+
+Only qemu-x86-64 can be used for booting distros, since qemu-x86 (the 32-bit
+version of U-Boot) seems to have an EFI bug leading to the boot handing after
+Linux is selected from grub, e.g. with `debian-12.1.0-i386-netinst.iso`::
+
+   ** Booting bootflow 'virtio-blk#1.bootdev.part_2' with efi
+   EFI using ACPI tables at f0180
+        efi_install_fdt() WARNING: Can't have ACPI table and device tree - ignoring DT.
+          efi_run_image() Booting /efi\boot\bootia32.efi
+   Failed to open efi\boot\root=/dev/sdb3 - Not Found
+   Failed to load image 큀緃: Not Found
+   start_image() returned Not Found, falling back to default loader
+   Welcome to GRUB!
+
+The bochs video driver also seems to cause problems before the OS is able to
+show a display.
+
+Finally, the use of `-M accel=kvm` is intended to use the native CPU's
+virtual-machine features to accelerate operation, but this causes U-Boot to hang
+when jumping 64-bit mode, at least on AMD machines. This may be a bug in U-Boot
+or something else.
-- 
2.41.0.487.g6d72f3e995-goog

Re: [EXTERNAL] [PATCH 2/9] video: Tidy up Makefile rule for video

[Nikhil M Jain]

On 24/07/23 20:21, Simon Glass wrote:
> Drop the duplication and add a single rule which can handle SPL as well.
> 
> Signed-off-by: Simon Glass<sjg@chromium.org>
> ---
> 
>   drivers/Makefile | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions

Reviewed-by: Nikhil M Jain <n-jain1@ti.com>

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Devarsh Thakkar]

Hi Simon,

On 24/07/23 20:22, Simon Glass wrote:
> When the video framebuffer comes from the bloblist, we should not change
> relocaddr to this address, since it interfers with the normal memory
> allocation.
> 
> This fixes a boot loop in qemu-x86_64
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
> ---
> 
>  common/board_f.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/common/board_f.c b/common/board_f.c
> index 7d2c380e91e2..5c8646b22283 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -419,7 +419,6 @@ static int reserve_video(void)
>  		if (!ho)
>  			return log_msg_ret("blf", -ENOENT);
>  		video_reserve_from_bloblist(ho);
> -		gd->relocaddr = ho->fb;

I think this change was done as relocaddr pointer was required to be updated
to move after frame-buffer reserved area to ensure that any further memory
reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
don't overlap with frame-buffer reserved area passed from blob, so I think
removing this line may cause further memory reservations to overlap with
reserved framebuffer.

Could you please confirm?


Regards
Devarsh

>  	} else if (CONFIG_IS_ENABLED(VIDEO)) {
>  		ulong addr;
>  		int ret;

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Simon Glass]

Hi Devarsh,

On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Hi Simon,
>
> On 24/07/23 20:22, Simon Glass wrote:
> > When the video framebuffer comes from the bloblist, we should not change
> > relocaddr to this address, since it interfers with the normal memory
> > allocation.
> >
> > This fixes a boot loop in qemu-x86_64
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
> > ---
> >
> >  common/board_f.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/common/board_f.c b/common/board_f.c
> > index 7d2c380e91e2..5c8646b22283 100644
> > --- a/common/board_f.c
> > +++ b/common/board_f.c
> > @@ -419,7 +419,6 @@ static int reserve_video(void)
> >               if (!ho)
> >                       return log_msg_ret("blf", -ENOENT);
> >               video_reserve_from_bloblist(ho);
> > -             gd->relocaddr = ho->fb;
>
> I think this change was done as relocaddr pointer was required to be updated
> to move after frame-buffer reserved area to ensure that any further memory
> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
> don't overlap with frame-buffer reserved area passed from blob, so I think
> removing this line may cause further memory reservations to overlap with
> reserved framebuffer.
>
> Could you please confirm?

SPL and U-Boot have different memory layouts. The current code is
interrupting U-Boot's careful building up of chunks of memory used for
different purposes.

The video memory has already been allocated by SPL, so we don't need
to insert a new one here, as your code demonstrates.

But also we have no way of knowing if it is legal to relocate U-Boot
(and various other things) just below the frame buffer chosen by SPL.

The SPL frame buffer needs to be considered pre-allocated. It makes no
sense to set relocaddr to this value. It will break all sorts of
things. E.g. qemu-x86_64 crashes with this.

>
>
> Regards
> Devarsh
>
> >       } else if (CONFIG_IS_ENABLED(VIDEO)) {
> >               ulong addr;
> >               int ret;

Regards,
Simon

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Devarsh Thakkar]

Hi Simon,

On 26/07/23 02:58, Simon Glass wrote:
> Hi Devarsh,
> 
> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 24/07/23 20:22, Simon Glass wrote:
>>> When the video framebuffer comes from the bloblist, we should not change
>>> relocaddr to this address, since it interfers with the normal memory
>>> allocation.
>>>
>>> This fixes a boot loop in qemu-x86_64
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
>>> ---
>>>
>>>  common/board_f.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/common/board_f.c b/common/board_f.c
>>> index 7d2c380e91e2..5c8646b22283 100644
>>> --- a/common/board_f.c
>>> +++ b/common/board_f.c
>>> @@ -419,7 +419,6 @@ static int reserve_video(void)
>>>               if (!ho)
>>>                       return log_msg_ret("blf", -ENOENT);
>>>               video_reserve_from_bloblist(ho);
>>> -             gd->relocaddr = ho->fb;
>>
>> I think this change was done as relocaddr pointer was required to be updated
>> to move after frame-buffer reserved area to ensure that any further memory
>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
>> don't overlap with frame-buffer reserved area passed from blob, so I think
>> removing this line may cause further memory reservations to overlap with
>> reserved framebuffer.
>>
>> Could you please confirm?
> 
> SPL and U-Boot have different memory layouts. The current code is
> interrupting U-Boot's careful building up of chunks of memory used for
> different purposes.
> 

But it is possible they could be using similar memory layout for some
components like framebuffer.
For e.g. in our case we are using same video_reserve func in A53 SPL too
and which allocates framebuffer from gd->relocaddr as seen here:

https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427

> The video memory has already been allocated by SPL, so we don't need
> to insert a new one here, as your code demonstrates.
> 

Agreed.

> But also we have no way of knowing if it is legal to relocate U-Boot
> (and various other things) just below the frame buffer chosen by SPL.
> 

Yes, so i suppose your case is that framebuffer address which is being passed
by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is
at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in
any manner since relocaddr points to ramtop (i.e. near to end address of DDR).

In that case I agree it doesn't make sense to move relocaddr to ho->fb.

But for the scenario where gd->relocaddr and ho->fb are nearby there is every
possibility that gd->relocaddr may overlap with framebuffer, also the code in
reserve_trace, reserve_uboot doesn't have any intelligence or check that it is
overlapping with framebuffer area or not.

I think one thing that can probably be done here is to have a check that if
passed framebuffer area falls within current relocaddr region, then update the
relocaddr else don't touch relocaddr :

if (ho->fb <= gd->relocaddr - ho->size)
  //It means framebuffer are is overlapping with current relocaddr so update
relocaddr
    gd->relocaddr = ho->fb
else
  //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr

Could you please share your opinion on this and if above logic suffice your
case too ?

Regards
Devarsh

> The SPL frame buffer needs to be considered pre-allocated. It makes no
> sense to set relocaddr to this value. It will break all sorts of
> things. E.g. qemu-x86_64 crashes with this.
> 
>>
>>
>> Regards
>> Devarsh
>>
>>>       } else if (CONFIG_IS_ENABLED(VIDEO)) {
>>>               ulong addr;
>>>               int ret;
> 
> Regards,
> Simon

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Simon Glass]

Hi Devarsh,

On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Hi Simon,
>
> On 26/07/23 02:58, Simon Glass wrote:
> > Hi Devarsh,
> >
> > On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 24/07/23 20:22, Simon Glass wrote:
> >>> When the video framebuffer comes from the bloblist, we should not change
> >>> relocaddr to this address, since it interfers with the normal memory
> >>> allocation.
> >>>
> >>> This fixes a boot loop in qemu-x86_64
> >>>
> >>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
> >>> ---
> >>>
> >>>  common/board_f.c | 1 -
> >>>  1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/common/board_f.c b/common/board_f.c
> >>> index 7d2c380e91e2..5c8646b22283 100644
> >>> --- a/common/board_f.c
> >>> +++ b/common/board_f.c
> >>> @@ -419,7 +419,6 @@ static int reserve_video(void)
> >>>               if (!ho)
> >>>                       return log_msg_ret("blf", -ENOENT);
> >>>               video_reserve_from_bloblist(ho);
> >>> -             gd->relocaddr = ho->fb;
> >>
> >> I think this change was done as relocaddr pointer was required to be updated
> >> to move after frame-buffer reserved area to ensure that any further memory
> >> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
> >> don't overlap with frame-buffer reserved area passed from blob, so I think
> >> removing this line may cause further memory reservations to overlap with
> >> reserved framebuffer.
> >>
> >> Could you please confirm?
> >
> > SPL and U-Boot have different memory layouts. The current code is
> > interrupting U-Boot's careful building up of chunks of memory used for
> > different purposes.
> >
>
> But it is possible they could be using similar memory layout for some
> components like framebuffer.
> For e.g. in our case we are using same video_reserve func in A53 SPL too
> and which allocates framebuffer from gd->relocaddr as seen here:
>
> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427

Even if it is similar, the point is that U-Boot proper needs to do its
own allocation stuff.

Of course, if SPL sets up the video, it will provide the framebuffer
address, but only that. The other addresses need to be done using the
normal mechanism.

>
> > The video memory has already been allocated by SPL, so we don't need
> > to insert a new one here, as your code demonstrates.
> >
>
> Agreed.
>
> > But also we have no way of knowing if it is legal to relocate U-Boot
> > (and various other things) just below the frame buffer chosen by SPL.
> >
>
> Yes, so i suppose your case is that framebuffer address which is being passed
> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is
> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in
> any manner since relocaddr points to ramtop (i.e. near to end address of DDR).
>
> In that case I agree it doesn't make sense to move relocaddr to ho->fb.
>
> But for the scenario where gd->relocaddr and ho->fb are nearby there is every
> possibility that gd->relocaddr may overlap with framebuffer, also the code in
> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is
> overlapping with framebuffer area or not.
>
> I think one thing that can probably be done here is to have a check that if
> passed framebuffer area falls within current relocaddr region, then update the
> relocaddr else don't touch relocaddr :
>
> if (ho->fb <= gd->relocaddr - ho->size)
>   //It means framebuffer are is overlapping with current relocaddr so update
> relocaddr
>     gd->relocaddr = ho->fb
> else
>   //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr
>
> Could you please share your opinion on this and if above logic suffice your
> case too ?

I don't think this line is needed at all, which is why this patch
removes it. What problem are you seeing?

Regards,
Simon


>
> Regards
> Devarsh
>
> > The SPL frame buffer needs to be considered pre-allocated. It makes no
> > sense to set relocaddr to this value. It will break all sorts of
> > things. E.g. qemu-x86_64 crashes with this.
> >
> >>
> >>
> >> Regards
> >> Devarsh
> >>
> >>>       } else if (CONFIG_IS_ENABLED(VIDEO)) {
> >>>               ulong addr;
> >>>               int ret;
> >
> > Regards,
> > Simon

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Nikhil M Jain]

Hi Simon,

On 27/07/23 06:23, Simon Glass wrote:
> Hi Devarsh,
> 
> On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 26/07/23 02:58, Simon Glass wrote:
>>> Hi Devarsh,
>>>
>>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 24/07/23 20:22, Simon Glass wrote:
>>>>> When the video framebuffer comes from the bloblist, we should not change
>>>>> relocaddr to this address, since it interfers with the normal memory
>>>>> allocation.
>>>>>
>>>>> This fixes a boot loop in qemu-x86_64
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
>>>>> ---
>>>>>
>>>>>   common/board_f.c | 1 -
>>>>>   1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>>> index 7d2c380e91e2..5c8646b22283 100644
>>>>> --- a/common/board_f.c
>>>>> +++ b/common/board_f.c
>>>>> @@ -419,7 +419,6 @@ static int reserve_video(void)
>>>>>                if (!ho)
>>>>>                        return log_msg_ret("blf", -ENOENT);
>>>>>                video_reserve_from_bloblist(ho);
>>>>> -             gd->relocaddr = ho->fb;
>>>>
>>>> I think this change was done as relocaddr pointer was required to be updated
>>>> to move after frame-buffer reserved area to ensure that any further memory
>>>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
>>>> don't overlap with frame-buffer reserved area passed from blob, so I think
>>>> removing this line may cause further memory reservations to overlap with
>>>> reserved framebuffer.
>>>>
>>>> Could you please confirm?
>>>
>>> SPL and U-Boot have different memory layouts. The current code is
>>> interrupting U-Boot's careful building up of chunks of memory used for
>>> different purposes.
>>>
>>
>> But it is possible they could be using similar memory layout for some
>> components like framebuffer.
>> For e.g. in our case we are using same video_reserve func in A53 SPL too
>> and which allocates framebuffer from gd->relocaddr as seen here:
>>
>> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427
> 
> Even if it is similar, the point is that U-Boot proper needs to do its
> own allocation stuff.
> 
> Of course, if SPL sets up the video, it will provide the framebuffer
> address, but only that. The other addresses need to be done using the
> normal mechanism.
> 
>>
>>> The video memory has already been allocated by SPL, so we don't need
>>> to insert a new one here, as your code demonstrates.
>>>
>>
>> Agreed.
>>
>>> But also we have no way of knowing if it is legal to relocate U-Boot
>>> (and various other things) just below the frame buffer chosen by SPL.
>>>
>>
>> Yes, so i suppose your case is that framebuffer address which is being passed
>> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is
>> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in
>> any manner since relocaddr points to ramtop (i.e. near to end address of DDR).
>>
>> In that case I agree it doesn't make sense to move relocaddr to ho->fb.
>>
>> But for the scenario where gd->relocaddr and ho->fb are nearby there is every
>> possibility that gd->relocaddr may overlap with framebuffer, also the code in
>> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is
>> overlapping with framebuffer area or not.
>>
>> I think one thing that can probably be done here is to have a check that if
>> passed framebuffer area falls within current relocaddr region, then update the
>> relocaddr else don't touch relocaddr :
>>
>> if (ho->fb <= gd->relocaddr - ho->size)
>>    //It means framebuffer are is overlapping with current relocaddr so update
>> relocaddr
>>      gd->relocaddr = ho->fb
>> else
>>    //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr
>>
>> Could you please share your opinion on this and if above logic suffice your
>> case too ?
> 
> I don't think this line is needed at all, which is why this patch
> removes it. What problem are you seeing?
> 
Across SPL stage and U-boot we are keeping same memory layout and
ensuring that same memory regions are used, this way it doesn't
interfere in the way of u-boot while allocating memory regions for
various purposes. This allowed us to display splash screen without any
flicker across the stages.

Now if you remove the line  gd->relocaddr = ho->fb, the frame buffer 
region will be used for reserving memory for other purposes which
corrupts the frame buffer.

One solution which we are planning to implement is move the ram_top to a
lower address leaving out a region for video buffer and u-boot can do
the allocation from the new ram_top address without spl video handoff
interfering in the u-boot's allocation of memory.The region above the 
ram_top can be used for video.

Present Scenario
+---------------------+ram_top
|                     |
|      page_table     |
|                     |
|                     |
+---------------------+
|                     |
|                     |
|                     |
|                     |
|                     |
|  video frame buffer |
|                     |
|                     |
|                     |
|                     |
|                     |
|                     |
+---------------------+
|                     |
|                     |
|    reserve_trace    |
|                     |
|                     |
|                     |
+---------------------+


Proposed Solution
+---------------------+
|                     |
|                     |
|                     |
|                     |
|                     |
| video frame buffer  |
|                     |
|                     |
|                     |
|                     |
|                     |
+---------------------+ram_top
|                     |
|                     |
|      page_table     |
|                     |
|                     |
|                     |
+---------------------+
|                     |
|                     |
|    reserve_trace    |
|                     |
|                     |
+---------------------+

> Regards,
> Simon
> 
> 
>>
>> Regards
>> Devarsh
>>
>>> The SPL frame buffer needs to be considered pre-allocated. It makes no
>>> sense to set relocaddr to this value. It will break all sorts of
>>> things. E.g. qemu-x86_64 crashes with this.
>>>
>>>>
>>>>
>>>> Regards
>>>> Devarsh
>>>>
>>>>>        } else if (CONFIG_IS_ENABLED(VIDEO)) {
>>>>>                ulong addr;
>>>>>                int ret;
>>>
>>> Regards,
>>> Simon

Regards,
Nikhil

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Simon Glass]

Hi Nikhil,

On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1@ti.com> wrote:
>
> Hi Simon,
>
> On 27/07/23 06:23, Simon Glass wrote:
> > Hi Devarsh,
> >
> > On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 26/07/23 02:58, Simon Glass wrote:
> >>> Hi Devarsh,
> >>>
> >>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 24/07/23 20:22, Simon Glass wrote:
> >>>>> When the video framebuffer comes from the bloblist, we should not change
> >>>>> relocaddr to this address, since it interfers with the normal memory
> >>>>> allocation.
> >>>>>
> >>>>> This fixes a boot loop in qemu-x86_64
> >>>>>
> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
> >>>>> ---
> >>>>>
> >>>>>   common/board_f.c | 1 -
> >>>>>   1 file changed, 1 deletion(-)
> >>>>>
> >>>>> diff --git a/common/board_f.c b/common/board_f.c
> >>>>> index 7d2c380e91e2..5c8646b22283 100644
> >>>>> --- a/common/board_f.c
> >>>>> +++ b/common/board_f.c
> >>>>> @@ -419,7 +419,6 @@ static int reserve_video(void)
> >>>>>                if (!ho)
> >>>>>                        return log_msg_ret("blf", -ENOENT);
> >>>>>                video_reserve_from_bloblist(ho);
> >>>>> -             gd->relocaddr = ho->fb;
> >>>>
> >>>> I think this change was done as relocaddr pointer was required to be updated
> >>>> to move after frame-buffer reserved area to ensure that any further memory
> >>>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
> >>>> don't overlap with frame-buffer reserved area passed from blob, so I think
> >>>> removing this line may cause further memory reservations to overlap with
> >>>> reserved framebuffer.
> >>>>
> >>>> Could you please confirm?
> >>>
> >>> SPL and U-Boot have different memory layouts. The current code is
> >>> interrupting U-Boot's careful building up of chunks of memory used for
> >>> different purposes.
> >>>
> >>
> >> But it is possible they could be using similar memory layout for some
> >> components like framebuffer.
> >> For e.g. in our case we are using same video_reserve func in A53 SPL too
> >> and which allocates framebuffer from gd->relocaddr as seen here:
> >>
> >> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427
> >
> > Even if it is similar, the point is that U-Boot proper needs to do its
> > own allocation stuff.
> >
> > Of course, if SPL sets up the video, it will provide the framebuffer
> > address, but only that. The other addresses need to be done using the
> > normal mechanism.
> >
> >>
> >>> The video memory has already been allocated by SPL, so we don't need
> >>> to insert a new one here, as your code demonstrates.
> >>>
> >>
> >> Agreed.
> >>
> >>> But also we have no way of knowing if it is legal to relocate U-Boot
> >>> (and various other things) just below the frame buffer chosen by SPL.
> >>>
> >>
> >> Yes, so i suppose your case is that framebuffer address which is being passed
> >> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is
> >> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in
> >> any manner since relocaddr points to ramtop (i.e. near to end address of DDR).
> >>
> >> In that case I agree it doesn't make sense to move relocaddr to ho->fb.
> >>
> >> But for the scenario where gd->relocaddr and ho->fb are nearby there is every
> >> possibility that gd->relocaddr may overlap with framebuffer, also the code in
> >> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is
> >> overlapping with framebuffer area or not.
> >>
> >> I think one thing that can probably be done here is to have a check that if
> >> passed framebuffer area falls within current relocaddr region, then update the
> >> relocaddr else don't touch relocaddr :
> >>
> >> if (ho->fb <= gd->relocaddr - ho->size)
> >>    //It means framebuffer are is overlapping with current relocaddr so update
> >> relocaddr
> >>      gd->relocaddr = ho->fb
> >> else
> >>    //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr
> >>
> >> Could you please share your opinion on this and if above logic suffice your
> >> case too ?
> >
> > I don't think this line is needed at all, which is why this patch
> > removes it. What problem are you seeing?
> >
> Across SPL stage and U-boot we are keeping same memory layout and
> ensuring that same memory regions are used, this way it doesn't
> interfere in the way of u-boot while allocating memory regions for
> various purposes. This allowed us to display splash screen without any
> flicker across the stages.
>
> Now if you remove the line  gd->relocaddr = ho->fb, the frame buffer
> region will be used for reserving memory for other purposes which
> corrupts the frame buffer.
>
> One solution which we are planning to implement is move the ram_top to a
> lower address leaving out a region for video buffer and u-boot can do
> the allocation from the new ram_top address without spl video handoff
> interfering in the u-boot's allocation of memory.The region above the
> ram_top can be used for video.
>
> Present Scenario
> +---------------------+ram_top
> |                     |
> |      page_table     |
> |                     |
> |                     |
> +---------------------+
> |                     |
> |                     |
> |                     |
> |                     |
> |                     |
> |  video frame buffer |
> |                     |
> |                     |
> |                     |
> |                     |
> |                     |
> |                     |
> +---------------------+
> |                     |
> |                     |
> |    reserve_trace    |
> |                     |
> |                     |
> |                     |
> +---------------------+
>
>
> Proposed Solution
> +---------------------+
> |                     |
> |                     |
> |                     |
> |                     |
> |                     |
> | video frame buffer  |
> |                     |
> |                     |
> |                     |
> |                     |
> |                     |
> +---------------------+ram_top
> |                     |
> |                     |
> |      page_table     |
> |                     |
> |                     |
> |                     |
> +---------------------+
> |                     |
> |                     |
> |    reserve_trace    |
> |                     |
> |                     |
> +---------------------+

Sure, whatever you need to do is fine. You could pass the ram top from
SPL to U-Boot perhaps.

Regards,
Simon

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Nikhil M Jain]

Hi Simon,

On 27/07/23 23:31, Simon Glass wrote:
> Hi Nikhil,
> 
> On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1@ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 27/07/23 06:23, Simon Glass wrote:
>>> Hi Devarsh,
>>>
>>> On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 26/07/23 02:58, Simon Glass wrote:
>>>>> Hi Devarsh,
>>>>>
>>>>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote:
>>>>>>
>>>>>> Hi Simon,
>>>>>>
>>>>>> On 24/07/23 20:22, Simon Glass wrote:
>>>>>>> When the video framebuffer comes from the bloblist, we should not change
>>>>>>> relocaddr to this address, since it interfers with the normal memory
>>>>>>> allocation.
>>>>>>>
>>>>>>> This fixes a boot loop in qemu-x86_64
>>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
>>>>>>> ---
>>>>>>>
>>>>>>>    common/board_f.c | 1 -
>>>>>>>    1 file changed, 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>>>>> index 7d2c380e91e2..5c8646b22283 100644
>>>>>>> --- a/common/board_f.c
>>>>>>> +++ b/common/board_f.c
>>>>>>> @@ -419,7 +419,6 @@ static int reserve_video(void)
>>>>>>>                 if (!ho)
>>>>>>>                         return log_msg_ret("blf", -ENOENT);
>>>>>>>                 video_reserve_from_bloblist(ho);
>>>>>>> -             gd->relocaddr = ho->fb;
>>>>>>
>>>>>> I think this change was done as relocaddr pointer was required to be updated
>>>>>> to move after frame-buffer reserved area to ensure that any further memory
>>>>>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
>>>>>> don't overlap with frame-buffer reserved area passed from blob, so I think
>>>>>> removing this line may cause further memory reservations to overlap with
>>>>>> reserved framebuffer.
>>>>>>
>>>>>> Could you please confirm?
>>>>>
>>>>> SPL and U-Boot have different memory layouts. The current code is
>>>>> interrupting U-Boot's careful building up of chunks of memory used for
>>>>> different purposes.
>>>>>
>>>>
>>>> But it is possible they could be using similar memory layout for some
>>>> components like framebuffer.
>>>> For e.g. in our case we are using same video_reserve func in A53 SPL too
>>>> and which allocates framebuffer from gd->relocaddr as seen here:
>>>>
>>>> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427
>>>
>>> Even if it is similar, the point is that U-Boot proper needs to do its
>>> own allocation stuff.
>>>
>>> Of course, if SPL sets up the video, it will provide the framebuffer
>>> address, but only that. The other addresses need to be done using the
>>> normal mechanism.
>>>
>>>>
>>>>> The video memory has already been allocated by SPL, so we don't need
>>>>> to insert a new one here, as your code demonstrates.
>>>>>
>>>>
>>>> Agreed.
>>>>
>>>>> But also we have no way of knowing if it is legal to relocate U-Boot
>>>>> (and various other things) just below the frame buffer chosen by SPL.
>>>>>
>>>>
>>>> Yes, so i suppose your case is that framebuffer address which is being passed
>>>> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is
>>>> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in
>>>> any manner since relocaddr points to ramtop (i.e. near to end address of DDR).
>>>>
>>>> In that case I agree it doesn't make sense to move relocaddr to ho->fb.
>>>>
>>>> But for the scenario where gd->relocaddr and ho->fb are nearby there is every
>>>> possibility that gd->relocaddr may overlap with framebuffer, also the code in
>>>> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is
>>>> overlapping with framebuffer area or not.
>>>>
>>>> I think one thing that can probably be done here is to have a check that if
>>>> passed framebuffer area falls within current relocaddr region, then update the
>>>> relocaddr else don't touch relocaddr :
>>>>
>>>> if (ho->fb <= gd->relocaddr - ho->size)
>>>>     //It means framebuffer are is overlapping with current relocaddr so update
>>>> relocaddr
>>>>       gd->relocaddr = ho->fb

We should go ahead with this check because it won't disrupt u-boot's
allocation of memory and will allow both the cases if a platform is
using same memory layout or different memory layout across SPL and
u-boot proper. Below are the logs for both scenarios.

https://gist.github.com/NikMJain/aca198ae77b6f1855459bc8fbdd683df

>>>> else
>>>>     //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr
>>>>
>>>> Could you please share your opinion on this and if above logic suffice your
>>>> case too ?
>>>
>>> I don't think this line is needed at all, which is why this patch
>>> removes it. What problem are you seeing?
>>>
>> Across SPL stage and U-boot we are keeping same memory layout and
>> ensuring that same memory regions are used, this way it doesn't
>> interfere in the way of u-boot while allocating memory regions for
>> various purposes. This allowed us to display splash screen without any
>> flicker across the stages.
>>
>> Now if you remove the line  gd->relocaddr = ho->fb, the frame buffer
>> region will be used for reserving memory for other purposes which
>> corrupts the frame buffer.
>>
>> One solution which we are planning to implement is move the ram_top to a
>> lower address leaving out a region for video buffer and u-boot can do
>> the allocation from the new ram_top address without spl video handoff
>> interfering in the u-boot's allocation of memory.The region above the
>> ram_top can be used for video.
>>
>> Present Scenario
>> +---------------------+ram_top
>> |                     |
>> |      page_table     |
>> |                     |
>> |                     |
>> +---------------------+
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> |  video frame buffer |
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> +---------------------+
>> |                     |
>> |                     |
>> |    reserve_trace    |
>> |                     |
>> |                     |
>> |                     |
>> +---------------------+
>>
>>
>> Proposed Solution
>> +---------------------+
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> | video frame buffer  |
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> |                     |
>> +---------------------+ram_top
>> |                     |
>> |                     |
>> |      page_table     |
>> |                     |
>> |                     |
>> |                     |
>> +---------------------+
>> |                     |
>> |                     |
>> |    reserve_trace    |
>> |                     |
>> |                     |
>> +---------------------+
> 
> Sure, whatever you need to do is fine. You could pass the ram top from
> SPL to U-Boot perhaps.
> 

In this solution problem arises when user doesn't want's to hand-off
buffer, the frame buffer region will be reallocated by u-boot or will
require us to hard code buffer address, which we don't want to do.

> Regards,
> Simon

Thanks,
Nikhil

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Nikhil M Jain]

On 28/07/23 14:05, Nikhil M Jain wrote:
> Hi Simon,
> 
> On 27/07/23 23:31, Simon Glass wrote:
>> Hi Nikhil,
>>
>> On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1@ti.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 27/07/23 06:23, Simon Glass wrote:
>>>> Hi Devarsh,
>>>>
>>>> On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On 26/07/23 02:58, Simon Glass wrote:
>>>>>> Hi Devarsh,
>>>>>>
>>>>>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> 
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> On 24/07/23 20:22, Simon Glass wrote:
>>>>>>>> When the video framebuffer comes from the bloblist, we should 
>>>>>>>> not change
>>>>>>>> relocaddr to this address, since it interfers with the normal 
>>>>>>>> memory
>>>>>>>> allocation.
>>>>>>>>
>>>>>>>> This fixes a boot loop in qemu-x86_64
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info 
>>>>>>>> from SPL to u-boot")
>>>>>>>> ---
>>>>>>>>
>>>>>>>>    common/board_f.c | 1 -
>>>>>>>>    1 file changed, 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>>>>>> index 7d2c380e91e2..5c8646b22283 100644
>>>>>>>> --- a/common/board_f.c
>>>>>>>> +++ b/common/board_f.c
>>>>>>>> @@ -419,7 +419,6 @@ static int reserve_video(void)
>>>>>>>>                 if (!ho)
>>>>>>>>                         return log_msg_ret("blf", -ENOENT);
>>>>>>>>                 video_reserve_from_bloblist(ho);
>>>>>>>> -             gd->relocaddr = ho->fb;
>>>>>>>
>>>>>>> I think this change was done as relocaddr pointer was required to 
>>>>>>> be updated
>>>>>>> to move after frame-buffer reserved area to ensure that any 
>>>>>>> further memory
>>>>>>> reservations done using gd->relocaddr (for e.g. in 
>>>>>>> reserve_trace/uboot/malloc)
>>>>>>> don't overlap with frame-buffer reserved area passed from blob, 
>>>>>>> so I think
>>>>>>> removing this line may cause further memory reservations to 
>>>>>>> overlap with
>>>>>>> reserved framebuffer.
>>>>>>>
>>>>>>> Could you please confirm?
>>>>>>
>>>>>> SPL and U-Boot have different memory layouts. The current code is
>>>>>> interrupting U-Boot's careful building up of chunks of memory used 
>>>>>> for
>>>>>> different purposes.
>>>>>>
>>>>>
>>>>> But it is possible they could be using similar memory layout for some
>>>>> components like framebuffer.
>>>>> For e.g. in our case we are using same video_reserve func in A53 
>>>>> SPL too
>>>>> and which allocates framebuffer from gd->relocaddr as seen here:
>>>>>
>>>>> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427
>>>>
>>>> Even if it is similar, the point is that U-Boot proper needs to do its
>>>> own allocation stuff.
>>>>
>>>> Of course, if SPL sets up the video, it will provide the framebuffer
>>>> address, but only that. The other addresses need to be done using the
>>>> normal mechanism.
>>>>
>>>>>
>>>>>> The video memory has already been allocated by SPL, so we don't need
>>>>>> to insert a new one here, as your code demonstrates.
>>>>>>
>>>>>
>>>>> Agreed.
>>>>>
>>>>>> But also we have no way of knowing if it is legal to relocate U-Boot
>>>>>> (and various other things) just below the frame buffer chosen by SPL.
>>>>>>
>>>>>
>>>>> Yes, so i suppose your case is that framebuffer address which is 
>>>>> being passed
>>>>> by SPL is totally disjoint and too far away from gd->relocaddr, for 
>>>>> e.g. it is
>>>>> at the start (or bottom of DDR) and doesn't interfere with 
>>>>> gd->relocaddr in
>>>>> any manner since relocaddr points to ramtop (i.e. near to end 
>>>>> address of DDR).
>>>>>
>>>>> In that case I agree it doesn't make sense to move relocaddr to 
>>>>> ho->fb.
>>>>>
>>>>> But for the scenario where gd->relocaddr and ho->fb are nearby 
>>>>> there is every
>>>>> possibility that gd->relocaddr may overlap with framebuffer, also 
>>>>> the code in
>>>>> reserve_trace, reserve_uboot doesn't have any intelligence or check 
>>>>> that it is
>>>>> overlapping with framebuffer area or not.
>>>>>
>>>>> I think one thing that can probably be done here is to have a check 
>>>>> that if
>>>>> passed framebuffer area falls within current relocaddr region, then 
>>>>> update the
>>>>> relocaddr else don't touch relocaddr :
>>>>>
>>>>> if (ho->fb <= gd->relocaddr - ho->size)

Just a small correction here
if (ho->fb >= gd->relocaddr - ho->size)

>>>>>     //It means framebuffer are is overlapping with current 
>>>>> relocaddr so update
>>>>> relocaddr
>>>>>       gd->relocaddr = ho->fb
> 
> We should go ahead with this check because it won't disrupt u-boot's
> allocation of memory and will allow both the cases if a platform is
> using same memory layout or different memory layout across SPL and
> u-boot proper. Below are the logs for both scenarios.
> 
> https://gist.github.com/NikMJain/aca198ae77b6f1855459bc8fbdd683df
> 
>>>>> else
>>>>>     //don't update gd->relocaddr since ho->fb is disjoint to 
>>>>> gd->relocaddr
>>>>>
>>>>> Could you please share your opinion on this and if above logic 
>>>>> suffice your
>>>>> case too ?
>>>>
>>>> I don't think this line is needed at all, which is why this patch
>>>> removes it. What problem are you seeing?
>>>>
>>> Across SPL stage and U-boot we are keeping same memory layout and
>>> ensuring that same memory regions are used, this way it doesn't
>>> interfere in the way of u-boot while allocating memory regions for
>>> various purposes. This allowed us to display splash screen without any
>>> flicker across the stages.
>>>
>>> Now if you remove the line  gd->relocaddr = ho->fb, the frame buffer
>>> region will be used for reserving memory for other purposes which
>>> corrupts the frame buffer.
>>>
>>> One solution which we are planning to implement is move the ram_top to a
>>> lower address leaving out a region for video buffer and u-boot can do
>>> the allocation from the new ram_top address without spl video handoff
>>> interfering in the u-boot's allocation of memory.The region above the
>>> ram_top can be used for video.
>>>
>>> Present Scenario
>>> +---------------------+ram_top
>>> |                     |
>>> |      page_table     |
>>> |                     |
>>> |                     |
>>> +---------------------+
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> |  video frame buffer |
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> +---------------------+
>>> |                     |
>>> |                     |
>>> |    reserve_trace    |
>>> |                     |
>>> |                     |
>>> |                     |
>>> +---------------------+
>>>
>>>
>>> Proposed Solution
>>> +---------------------+
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> | video frame buffer  |
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> +---------------------+ram_top
>>> |                     |
>>> |                     |
>>> |      page_table     |
>>> |                     |
>>> |                     |
>>> |                     |
>>> +---------------------+
>>> |                     |
>>> |                     |
>>> |    reserve_trace    |
>>> |                     |
>>> |                     |
>>> +---------------------+
>>
>> Sure, whatever you need to do is fine. You could pass the ram top from
>> SPL to U-Boot perhaps.
>>
> 
> In this solution problem arises when user doesn't want's to hand-off
> buffer, the frame buffer region will be reallocated by u-boot or will
> require us to hard code buffer address, which we don't want to do.
> 
>> Regards,
>> Simon
> 
> Thanks,
> Nikhil

Re: [PATCH 1/9] x86: spl: Drop unwanted debug()

[Bin Meng]

On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
>
> This was left over from some previous debugging. Drop it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/lib/spl.c | 1 -
>  1 file changed, 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 2/9] video: Tidy up Makefile rule for video

[Bin Meng]

Hi Simon,

On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
>
> Drop the duplication and add a single rule which can handle SPL as well.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 3bc6d279d7cb..7272681fc741 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -39,6 +39,8 @@ obj-$(CONFIG_$(SPL_TPL_)TPM) += tpm/
>  obj-$(CONFIG_$(SPL_)NVME) += nvme/
>  obj-$(CONFIG_XEN) += xen/
>  obj-$(CONFIG_$(SPL_)FPGA) += fpga/
> +obj-$(CONFIG_$(SPL_TPL_)VIDEO) += video/

This should be $(SPL_) as the original codes do not enable video build for TPL

> +
>  obj-y += bus/
>
>  ifndef CONFIG_TPL_BUILD
> @@ -64,7 +66,6 @@ obj-$(CONFIG_SPL_USB_HOST) += usb/host/
>  obj-$(CONFIG_SPL_SATA) += ata/ scsi/
>  obj-$(CONFIG_SPL_LEGACY_BLOCK) += block/
>  obj-$(CONFIG_SPL_THERMAL) += thermal/
> -obj-$(CONFIG_SPL_VIDEO) +=video/
>
>  endif
>  endif
> @@ -99,7 +100,6 @@ obj-y += rtc/
>  obj-y += scsi/
>  obj-y += sound/
>  obj-y += spmi/
> -obj-y += video/
>  obj-y += watchdog/
>  obj-$(CONFIG_QE) += qe/
>  obj-$(CONFIG_U_QE) += qe/
> --

Regards,
Bin

Re: [PATCH 3/9] x86: Run QEMU machine setup in SPL

[Bin Meng]

On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
>
> Call the hardware-init function from QEMU from SPL. This allows the video
> BIOS to operate correctly.
>
> Create an x86-wide qemu.h header to avoid having to #ifdef the header in
> spl.c
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/cpu/qemu/qemu.c    |  2 +-
>  arch/x86/include/asm/qemu.h | 14 ++++++++++++++
>  arch/x86/lib/spl.c          |  3 +++
>  3 files changed, 18 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/include/asm/qemu.h
>
> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
> index 274978c023b6..70414556086c 100644
> --- a/arch/x86/cpu/qemu/qemu.c
> +++ b/arch/x86/cpu/qemu/qemu.c
> @@ -48,7 +48,7 @@ static void enable_pm_ich9(void)
>         pci_write_config32(ICH9_PM, PMBA, CONFIG_ACPI_PM1_BASE | 1);
>  }
>
> -static void qemu_chipset_init(void)
> +void qemu_chipset_init(void)
>  {
>         u16 device, xbcs;
>         int pam, i;
> diff --git a/arch/x86/include/asm/qemu.h b/arch/x86/include/asm/qemu.h
> new file mode 100644
> index 000000000000..f1e95ffd7a4d
> --- /dev/null
> +++ b/arch/x86/include/asm/qemu.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Generic QEMU header
> + *
> + * Copyright 2023 Google LLC
> + */
> +
> +#ifndef __QEMU_H
> +#define __QEMU_H
> +
> +/* set up the chipset for QEMU so that video can be used */
> +void qemu_chipset_init(void);
> +
> +#endif
> diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
> index 55c061570864..74949b2c13cb 100644
> --- a/arch/x86/lib/spl.c
> +++ b/arch/x86/lib/spl.c
> @@ -27,6 +27,7 @@
>  #include <asm/mtrr.h>
>  #include <asm/pci.h>
>  #include <asm/processor.h>
> +#include <asm/qemu.h>
>  #include <asm/spl.h>
>  #include <asm-generic/sections.h>
>
> @@ -291,6 +292,8 @@ void spl_board_init(void)
>  #ifndef CONFIG_TPL
>         preloader_console_init();
>  #endif
> +       if (IS_ENABLED(CONFIG_X86) && IS_ENABLED(CONFIG_QEMU))

CONFIG_X86 is not necessary.

> +               qemu_chipset_init();
>
>         if (CONFIG_IS_ENABLED(VIDEO)) {
>                 struct udevice *dev;
> --

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Bin Meng]

Hi Simon,

On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
>
> Unfortunately the bochs driver does not currently work with distros. It
> causes a hang sometime between grub menu selection and the OS displaying
> something.

Does this reproduce reliably?

>
> This reverts commit b8956425d525c3c25fd218f252f89a5e44df6a9f.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/cpu/qemu/Kconfig     | 2 +-
>  configs/qemu-x86_64_defconfig | 4 ++++
>  configs/qemu-x86_defconfig    | 3 +++
>  3 files changed, 8 insertions(+), 1 deletion(-)
>

Regards,
Bin

Re: [PATCH 6/9] x86: Correct copying of BIOS mode information

[Bin Meng]

On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
>
> This is copying beyond the end of the destination buffer. Correct the code
> by using a constant for the buffer size.
>
> This long-standing bug prevents virtio bootdevs working correctly on
> qemu-x86 at present.

Nice catch!

>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Fixes: 0ca2426beae ("x86: Add support for running option ROMs natively")
> ---
>
>  arch/x86/lib/bios.c | 2 +-
>  include/vesa.h      | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/lib/bios.c b/arch/x86/lib/bios.c
> index e29cae78e509..3a9d7f5ddd41 100644
> --- a/arch/x86/lib/bios.c
> +++ b/arch/x86/lib/bios.c
> @@ -204,7 +204,7 @@ static u8 vbe_get_mode_info(struct vesa_state *mi)
>
>         realmode_interrupt(0x10, VESA_GET_MODE_INFO, 0x0000, mi->video_mode,
>                            0x0000, buffer_seg, buffer_adr);
> -       memcpy(mi->mode_info_block, buffer, sizeof(struct vesa_state));
> +       memcpy(mi->mode_info_block, buffer, VESA_MODE_INFO_SIZE);

or "sizeof(struct vesa_mode_info)"

>         mi->valid = true;
>
>         return 0;
> diff --git a/include/vesa.h b/include/vesa.h
> index 9285bfa921a2..28828ab56aa4 100644
> --- a/include/vesa.h
> +++ b/include/vesa.h
> @@ -83,12 +83,14 @@ struct __packed vesa_mode_info {
>         u8 reserved[206];
>  };
>
> +#define VESA_MODE_INFO_SIZE    256
> +
>  struct vesa_state {
>         u16 video_mode;
>         bool valid;
>         union {
>                 struct vesa_mode_info vesa;
> -               u8 mode_info_block[256];
> +               u8 mode_info_block[VESA_MODE_INFO_SIZE];
>         };
>  };
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Simon Glass]

Hi Bin,

On Fri, 28 Jul 2023 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Unfortunately the bochs driver does not currently work with distros. It
> > causes a hang sometime between grub menu selection and the OS displaying
> > something.
>
> Does this reproduce reliably?

Yes, it does.

BTW I've also hit another problem with '-M accel-kvm' which hangs on
the move to 64-bit mode. Oddly if I boot U-Boot from coreboot, the
problem goes away. So perhaps coreboot is doing some x86 init which
U-Boot is missing.

>
> >
> > This reverts commit b8956425d525c3c25fd218f252f89a5e44df6a9f.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  arch/x86/cpu/qemu/Kconfig     | 2 +-
> >  configs/qemu-x86_64_defconfig | 4 ++++
> >  configs/qemu-x86_defconfig    | 3 +++
> >  3 files changed, 8 insertions(+), 1 deletion(-)
> >
>
> Regards,
> Bin

Regards,
Simon

Re: [PATCH 2/9] video: Tidy up Makefile rule for video

[Simon Glass]

Hi Bin,

On Fri, 28 Jul 2023 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Drop the duplication and add a single rule which can handle SPL as well.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  drivers/Makefile | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 3bc6d279d7cb..7272681fc741 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -39,6 +39,8 @@ obj-$(CONFIG_$(SPL_TPL_)TPM) += tpm/
> >  obj-$(CONFIG_$(SPL_)NVME) += nvme/
> >  obj-$(CONFIG_XEN) += xen/
> >  obj-$(CONFIG_$(SPL_)FPGA) += fpga/
> > +obj-$(CONFIG_$(SPL_TPL_)VIDEO) += video/
>
> This should be $(SPL_) as the original codes do not enable video build for TPL

Yes, but SPL_TPL supports that, in that TPL_VIDEO is not enabled, so
it will not be built for TPL. In general, we never need the $(SPL)
variant.

Regards,
Simon

Re: [PATCH 5/9] board_f: Fix corruption of relocaddr

[Simon Glass]

Hi,

+Tom Rini for comment

On Fri, 28 Jul 2023 at 02:35, Nikhil M Jain <n-jain1@ti.com> wrote:
>
> Hi Simon,
>
> On 27/07/23 23:31, Simon Glass wrote:
> > Hi Nikhil,
> >
> > On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1@ti.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 27/07/23 06:23, Simon Glass wrote:
> >>> Hi Devarsh,
> >>>
> >>> On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 26/07/23 02:58, Simon Glass wrote:
> >>>>> Hi Devarsh,
> >>>>>
> >>>>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote:
> >>>>>>
> >>>>>> Hi Simon,
> >>>>>>
> >>>>>> On 24/07/23 20:22, Simon Glass wrote:
> >>>>>>> When the video framebuffer comes from the bloblist, we should not change
> >>>>>>> relocaddr to this address, since it interfers with the normal memory
> >>>>>>> allocation.
> >>>>>>>
> >>>>>>> This fixes a boot loop in qemu-x86_64
> >>>>>>>
> >>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>>>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
> >>>>>>> ---
> >>>>>>>
> >>>>>>>    common/board_f.c | 1 -
> >>>>>>>    1 file changed, 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/common/board_f.c b/common/board_f.c
> >>>>>>> index 7d2c380e91e2..5c8646b22283 100644
> >>>>>>> --- a/common/board_f.c
> >>>>>>> +++ b/common/board_f.c
> >>>>>>> @@ -419,7 +419,6 @@ static int reserve_video(void)
> >>>>>>>                 if (!ho)
> >>>>>>>                         return log_msg_ret("blf", -ENOENT);
> >>>>>>>                 video_reserve_from_bloblist(ho);
> >>>>>>> -             gd->relocaddr = ho->fb;
> >>>>>>
> >>>>>> I think this change was done as relocaddr pointer was required to be updated
> >>>>>> to move after frame-buffer reserved area to ensure that any further memory
> >>>>>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc)
> >>>>>> don't overlap with frame-buffer reserved area passed from blob, so I think
> >>>>>> removing this line may cause further memory reservations to overlap with
> >>>>>> reserved framebuffer.
> >>>>>>
> >>>>>> Could you please confirm?
> >>>>>
> >>>>> SPL and U-Boot have different memory layouts. The current code is
> >>>>> interrupting U-Boot's careful building up of chunks of memory used for
> >>>>> different purposes.
> >>>>>
> >>>>
> >>>> But it is possible they could be using similar memory layout for some
> >>>> components like framebuffer.
> >>>> For e.g. in our case we are using same video_reserve func in A53 SPL too
> >>>> and which allocates framebuffer from gd->relocaddr as seen here:
> >>>>
> >>>> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427
> >>>
> >>> Even if it is similar, the point is that U-Boot proper needs to do its
> >>> own allocation stuff.
> >>>
> >>> Of course, if SPL sets up the video, it will provide the framebuffer
> >>> address, but only that. The other addresses need to be done using the
> >>> normal mechanism.
> >>>
> >>>>
> >>>>> The video memory has already been allocated by SPL, so we don't need
> >>>>> to insert a new one here, as your code demonstrates.
> >>>>>
> >>>>
> >>>> Agreed.
> >>>>
> >>>>> But also we have no way of knowing if it is legal to relocate U-Boot
> >>>>> (and various other things) just below the frame buffer chosen by SPL.
> >>>>>
> >>>>
> >>>> Yes, so i suppose your case is that framebuffer address which is being passed
> >>>> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is
> >>>> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in
> >>>> any manner since relocaddr points to ramtop (i.e. near to end address of DDR).
> >>>>
> >>>> In that case I agree it doesn't make sense to move relocaddr to ho->fb.
> >>>>
> >>>> But for the scenario where gd->relocaddr and ho->fb are nearby there is every
> >>>> possibility that gd->relocaddr may overlap with framebuffer, also the code in
> >>>> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is
> >>>> overlapping with framebuffer area or not.
> >>>>
> >>>> I think one thing that can probably be done here is to have a check that if
> >>>> passed framebuffer area falls within current relocaddr region, then update the
> >>>> relocaddr else don't touch relocaddr :
> >>>>
> >>>> if (ho->fb <= gd->relocaddr - ho->size)
> >>>>     //It means framebuffer are is overlapping with current relocaddr so update
> >>>> relocaddr
> >>>>       gd->relocaddr = ho->fb
>
> We should go ahead with this check because it won't disrupt u-boot's
> allocation of memory and will allow both the cases if a platform is
> using same memory layout or different memory layout across SPL and
> u-boot proper. Below are the logs for both scenarios.
>
> https://gist.github.com/NikMJain/aca198ae77b6f1855459bc8fbdd683df
>
> >>>> else
> >>>>     //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr
> >>>>
> >>>> Could you please share your opinion on this and if above logic suffice your
> >>>> case too ?
> >>>
> >>> I don't think this line is needed at all, which is why this patch
> >>> removes it. What problem are you seeing?
> >>>
> >> Across SPL stage and U-boot we are keeping same memory layout and
> >> ensuring that same memory regions are used, this way it doesn't
> >> interfere in the way of u-boot while allocating memory regions for
> >> various purposes. This allowed us to display splash screen without any
> >> flicker across the stages.
> >>
> >> Now if you remove the line  gd->relocaddr = ho->fb, the frame buffer
> >> region will be used for reserving memory for other purposes which
> >> corrupts the frame buffer.
> >>
> >> One solution which we are planning to implement is move the ram_top to a
> >> lower address leaving out a region for video buffer and u-boot can do
> >> the allocation from the new ram_top address without spl video handoff
> >> interfering in the u-boot's allocation of memory.The region above the
> >> ram_top can be used for video.
> >>
> >> Present Scenario
> >> +---------------------+ram_top
> >> |                     |
> >> |      page_table     |
> >> |                     |
> >> |                     |
> >> +---------------------+
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> |  video frame buffer |
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> +---------------------+
> >> |                     |
> >> |                     |
> >> |    reserve_trace    |
> >> |                     |
> >> |                     |
> >> |                     |
> >> +---------------------+
> >>
> >>
> >> Proposed Solution
> >> +---------------------+
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> | video frame buffer  |
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> +---------------------+ram_top
> >> |                     |
> >> |                     |
> >> |      page_table     |
> >> |                     |
> >> |                     |
> >> |                     |
> >> +---------------------+
> >> |                     |
> >> |                     |
> >> |    reserve_trace    |
> >> |                     |
> >> |                     |
> >> +---------------------+
> >
> > Sure, whatever you need to do is fine. You could pass the ram top from
> > SPL to U-Boot perhaps.
> >
>
> In this solution problem arises when user doesn't want's to hand-off
> buffer, the frame buffer region will be reallocated by u-boot or will
> require us to hard code buffer address, which we don't want to do.

How about using a Kconfig instead? It seems to me that the idea of SPL
and U-Boot happening to place the video buffer at the same place is
more the exception than the rule. I've tried to explain that the
sequence of reserve...() calls that U-Boot proper does really should
not be molested by some data arriving from SPL.

If you don't want to do a Kconfig, then please send a patch that works
for you and I'll test it, then update my series.

Regards,
Simon

Re: [PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Bin Meng]

Hi Simon,

On Sat, Jul 29, 2023 at 1:32 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Fri, 28 Jul 2023 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Unfortunately the bochs driver does not currently work with distros. It
> > > causes a hang sometime between grub menu selection and the OS displaying
> > > something.
> >
> > Does this reproduce reliably?
>
> Yes, it does.
>
> BTW I've also hit another problem with '-M accel-kvm' which hangs on
> the move to 64-bit mode. Oddly if I boot U-Boot from coreboot, the
> problem goes away. So perhaps coreboot is doing some x86 init which
> U-Boot is missing.

This indicates a kvm-x86 bug. I think you can simplify a test case and
report it to the KVM mailing list.

>
> >
> > >
> > > This reverts commit b8956425d525c3c25fd218f252f89a5e44df6a9f.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >  arch/x86/cpu/qemu/Kconfig     | 2 +-
> > >  configs/qemu-x86_64_defconfig | 4 ++++
> > >  configs/qemu-x86_defconfig    | 3 +++
> > >  3 files changed, 8 insertions(+), 1 deletion(-)

Regards,
Bin

Re: [PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Simon Glass]

Hi Bin,

On Mon, 31 Jul 2023 at 08:32, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sat, Jul 29, 2023 at 1:32 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Fri, 28 Jul 2023 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Unfortunately the bochs driver does not currently work with distros. It
> > > > causes a hang sometime between grub menu selection and the OS displaying
> > > > something.
> > >
> > > Does this reproduce reliably?
> >
> > Yes, it does.
> >
> > BTW I've also hit another problem with '-M accel-kvm' which hangs on
> > the move to 64-bit mode. Oddly if I boot U-Boot from coreboot, the
> > problem goes away. So perhaps coreboot is doing some x86 init which
> > U-Boot is missing.
>
> This indicates a kvm-x86 bug. I think you can simplify a test case and
> report it to the KVM mailing list.

Firstly it doesn't really matter, since it doesn't work, so we do need
to revert.

Secondly I am not sure it kvm's fault, since the problem does not
happen with coreboot. I will see if I can isolate what coreboot is
doing differently, but not sure that that fix will make the release.

Regards,
Simon

Re: [PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Bin Meng]

Hi Simon,

On Mon, Jul 31, 2023 at 10:37 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 31 Jul 2023 at 08:32, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sat, Jul 29, 2023 at 1:32 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Fri, 28 Jul 2023 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Unfortunately the bochs driver does not currently work with distros. It
> > > > > causes a hang sometime between grub menu selection and the OS displaying
> > > > > something.
> > > >
> > > > Does this reproduce reliably?
> > >
> > > Yes, it does.
> > >
> > > BTW I've also hit another problem with '-M accel-kvm' which hangs on
> > > the move to 64-bit mode. Oddly if I boot U-Boot from coreboot, the
> > > problem goes away. So perhaps coreboot is doing some x86 init which
> > > U-Boot is missing.
> >
> > This indicates a kvm-x86 bug. I think you can simplify a test case and
> > report it to the KVM mailing list.
>
> Firstly it doesn't really matter, since it doesn't work, so we do need
> to revert.

Agreed, but before we revert this, I would like to see we investigate
this issue a little bit further.

I enabled the grub debug output today and not sure if grub jumps to
the kernel entry yet:

kern/verifiers.c:214: string: boot, type: 2
loader/efi/linux_sb.c:55: kernel_addr: 0x1000000 handover_offset: 0x190 params:
0x7ce63000

>
> Secondly I am not sure it kvm's fault, since the problem does not
> happen with coreboot. I will see if I can isolate what coreboot is
> doing differently, but not sure that that fix will make the release.

Does the 64-bit mode switch ever work on a real x86 board? If yes,
then it's very likely a KVM bug.

If not, then we can check the coreboot to find out the difference.

Regards,
Bin

Re: [PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Simon Glass]

Hi Bin,

On Mon, 31 Jul 2023 at 08:46, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Jul 31, 2023 at 10:37 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Mon, 31 Jul 2023 at 08:32, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sat, Jul 29, 2023 at 1:32 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Fri, 28 Jul 2023 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Unfortunately the bochs driver does not currently work with distros. It
> > > > > > causes a hang sometime between grub menu selection and the OS displaying
> > > > > > something.
> > > > >
> > > > > Does this reproduce reliably?
> > > >
> > > > Yes, it does.
> > > >
> > > > BTW I've also hit another problem with '-M accel-kvm' which hangs on
> > > > the move to 64-bit mode. Oddly if I boot U-Boot from coreboot, the
> > > > problem goes away. So perhaps coreboot is doing some x86 init which
> > > > U-Boot is missing.
> > >
> > > This indicates a kvm-x86 bug. I think you can simplify a test case and
> > > report it to the KVM mailing list.
> >
> > Firstly it doesn't really matter, since it doesn't work, so we do need
> > to revert.
>
> Agreed, but before we revert this, I would like to see we investigate
> this issue a little bit further.
>
> I enabled the grub debug output today and not sure if grub jumps to
> the kernel entry yet:
>
> kern/verifiers.c:214: string: boot, type: 2
> loader/efi/linux_sb.c:55: kernel_addr: 0x1000000 handover_offset: 0x190 params:
> 0x7ce63000
>
> >
> > Secondly I am not sure it kvm's fault, since the problem does not
> > happen with coreboot. I will see if I can isolate what coreboot is
> > doing differently, but not sure that that fix will make the release.
>
> Does the 64-bit mode switch ever work on a real x86 board? If yes,
> then it's very likely a KVM bug.

Yes it works fine on chromebook_link64, for example.

>
> If not, then we can check the coreboot to find out the difference.

Yes...do you have any ideas?

If it helps, I am using something like this:

 qemu-system-x86_64 -m 8G -smp 4  -bios /tmp/b/qemu-x86_64/u-boot.rom
-drive id=fdisk,file=root.img,if=virtio,driver=raw  -drive \
    file=/vid/software/linux/ubuntu/ubuntu-23.04-desktop-amd64.iso,if=virtio,driver=raw
 \
    -serial mon:stdio

For coreboot my script is:

UBTEST=/vid/software/devel/ubtest/
BUILD_DIR=/tmp/b/coreboot64
DISK=/vid/software/linux/ubuntu/ubuntu-23.04-desktop-amd64.iso

cp $UBTEST/coreboot.rom.in $BUILD_DIR/coreboot.rom
cbfstool $BUILD_DIR/coreboot.rom add-flat-binary \
        -f $BUILD_DIR/u-boot-x86-with-spl.bin \
        -n fallback/payload -c LZMA -l 0x1110000 -e 0x1110000

Add this in below:
#-M accel=kvm

qemu-system-x86_64 -m 2G -smp 4 -bios $BUILD_DIR/coreboot.rom -serial
mon:stdio \
        -drive id=disk,file=$DISK,if=none \
        -device ahci,id=ahci \
        -device ide-hd,drive=disk,bus=ahci.0

Regards,
Simon

Re: [PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Bin Meng]

On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
>
> Unfortunately the bochs driver does not currently work with distros. It
> causes a hang sometime between grub menu selection and the OS displaying
> something.

I will reword the commit message a little bit.

>
> This reverts commit b8956425d525c3c25fd218f252f89a5e44df6a9f.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/cpu/qemu/Kconfig     | 2 +-
>  configs/qemu-x86_64_defconfig | 4 ++++
>  configs/qemu-x86_defconfig    | 3 +++
>  3 files changed, 8 insertions(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 4/9] Revert "x86: Switch QEMU over to use the bochs driver"

[Simon Glass]

Hi Bin,

On Mon, 31 Jul 2023 at 11:07, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 31 Jul 2023 at 08:46, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Jul 31, 2023 at 10:37 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Mon, 31 Jul 2023 at 08:32, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sat, Jul 29, 2023 at 1:32 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Fri, 28 Jul 2023 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Mon, Jul 24, 2023 at 10:52 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Unfortunately the bochs driver does not currently work with distros. It
> > > > > > > causes a hang sometime between grub menu selection and the OS displaying
> > > > > > > something.
> > > > > >
> > > > > > Does this reproduce reliably?
> > > > >
> > > > > Yes, it does.
> > > > >
> > > > > BTW I've also hit another problem with '-M accel-kvm' which hangs on
> > > > > the move to 64-bit mode. Oddly if I boot U-Boot from coreboot, the
> > > > > problem goes away. So perhaps coreboot is doing some x86 init which
> > > > > U-Boot is missing.
> > > >
> > > > This indicates a kvm-x86 bug. I think you can simplify a test case and
> > > > report it to the KVM mailing list.
> > >
> > > Firstly it doesn't really matter, since it doesn't work, so we do need
> > > to revert.
> >
> > Agreed, but before we revert this, I would like to see we investigate
> > this issue a little bit further.
> >
> > I enabled the grub debug output today and not sure if grub jumps to
> > the kernel entry yet:
> >
> > kern/verifiers.c:214: string: boot, type: 2
> > loader/efi/linux_sb.c:55: kernel_addr: 0x1000000 handover_offset: 0x190 params:
> > 0x7ce63000
> >
> > >
> > > Secondly I am not sure it kvm's fault, since the problem does not
> > > happen with coreboot. I will see if I can isolate what coreboot is
> > > doing differently, but not sure that that fix will make the release.
> >
> > Does the 64-bit mode switch ever work on a real x86 board? If yes,
> > then it's very likely a KVM bug.
>
> Yes it works fine on chromebook_link64, for example.
>
> >
> > If not, then we can check the coreboot to find out the difference.
>
> Yes...do you have any ideas?
>
> If it helps, I am using something like this:
>
>  qemu-system-x86_64 -m 8G -smp 4  -bios /tmp/b/qemu-x86_64/u-boot.rom
> -drive id=fdisk,file=root.img,if=virtio,driver=raw  -drive \
>     file=/vid/software/linux/ubuntu/ubuntu-23.04-desktop-amd64.iso,if=virtio,driver=raw
>  \
>     -serial mon:stdio
>
> For coreboot my script is:
>
> UBTEST=/vid/software/devel/ubtest/
> BUILD_DIR=/tmp/b/coreboot64
> DISK=/vid/software/linux/ubuntu/ubuntu-23.04-desktop-amd64.iso
>
> cp $UBTEST/coreboot.rom.in $BUILD_DIR/coreboot.rom
> cbfstool $BUILD_DIR/coreboot.rom add-flat-binary \
>         -f $BUILD_DIR/u-boot-x86-with-spl.bin \
>         -n fallback/payload -c LZMA -l 0x1110000 -e 0x1110000
>
> Add this in below:
> #-M accel=kvm
>
> qemu-system-x86_64 -m 2G -smp 4 -bios $BUILD_DIR/coreboot.rom -serial
> mon:stdio \
>         -drive id=disk,file=$DISK,if=none \
>         -device ahci,id=ahci \
>         -device ide-hd,drive=disk,bus=ahci.0

I haven't looked into the problem here, but I did notice this coreboot
commit: https://review.coreboot.org/c/coreboot/+/49228

I am not sure it is related though, since we already have page tables in RAM.

Regards,
Simon