[PATCH v4 00/12] Resolve issues with booting distros on x86

[PATCH v4 00/12] Resolve issues with booting distros on x86

[Simon Glass]

This little series reprises the EFI-video fix, fixes a USB problem and
enables a boot script for coreboot.

It also moves to truetype fonts for coreboot and qemu-x86, since the
menus look much better and there are no strong size constraints.

With these changes it is possible to boot a Linux distro automatically
with U-Boot on x86, including when U-Boot is the second-stage
bootloader.

Changes in v4:
- Don't rename the legacy-USB functions
- Add a bit more detail to the comment
- Use a Kconfig option

Changes in v3:
- Add new patch to refactor mmc prep to allow a different scan
- Add missing word 'function' in the commit message
- Clear the screen before booting
- Add new patch to drop unnecessary truetype operations from SPL
- Add new patch to enable truetype fonts in coreboot
- Add new patch to enable truetype fonts in qemu-x86 and qemu-x86_64

Changes in v2:
- Rebase to -next
- Add some more comments to the header file
- Add fixes tag
- Add new patch to add a return code to bootflow menu
- Add new patch to add a coreboot boot script
- Add new patch to avoid unbinding devices in use by bootflows

Simon Glass (12):
  efi: Correct handling of frame buffer
  bootstd: Refactor mmc prep to allow a different scan
  bootstd: Add a return code to bootflow menu
  x86: coreboot: Add a boot script
  usb: Avoid unbinding devices in use by bootflows
  expo: Correct background colour
  video: Correct setting of cursor position
  video: Drop unnecessary truetype operations from SPL
  x86: Enable SSE in 64-bit mode
  x86: coreboot: Enable truetype fonts
  x86: qemu: Expand ROM size
  x86: qemu: Enable truetype fonts

 arch/x86/Kconfig                  |  8 ++++
 arch/x86/config.mk                |  4 ++
 arch/x86/cpu/x86_64/cpu.c         | 12 ++++++
 arch/x86/dts/coreboot.dts         | 10 +++++
 board/emulation/qemu-x86/Kconfig  |  3 +-
 boot/bootm.c                      |  2 +-
 boot/expo.c                       |  4 +-
 cmd/bootflow.c                    | 53 ++++++++++++++++++------
 common/usb.c                      |  5 +++
 configs/coreboot64_defconfig      |  2 +
 configs/coreboot_defconfig        |  2 +
 configs/qemu-x86_64_defconfig     |  5 ++-
 configs/qemu-x86_defconfig        |  1 +
 doc/usage/cmd/bootflow.rst        | 67 +++++++++++++++++++++++++++++++
 drivers/usb/host/usb-uclass.c     | 14 ++++++-
 drivers/video/Kconfig             |  1 +
 drivers/video/console_truetype.c  | 10 +++++
 drivers/video/vidconsole-uclass.c | 15 +++----
 include/usb.h                     | 21 +++++++++-
 include/video.h                   |  9 +++--
 lib/efi_loader/efi_gop.c          | 12 +++---
 test/boot/bootflow.c              | 64 ++++++++++++++++++++++++-----
 22 files changed, 280 insertions(+), 44 deletions(-)

-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 01/12] efi: Correct handling of frame buffer

[Simon Glass]

The efi_gop driver uses private fields from the video uclass to obtain a
pointer to the frame buffer. Use the platform data instead.

Check the VIDEO_COPY setting to determine which frame buffer to use. Once
the next stage is running (and making use of U-Boot's EFI boot services)
U-Boot does not handle copying from priv->fb to the hardware framebuffer,
so we must allow EFI to write directly to the hardware framebuffer.

We could provide a function to read this, but it seems better to just
document how it works. The original change ignored an explicit comment
in the video.h file ("Things that are private to the uclass: don't use
these in the driver") which is why this was missed when the VIDEO_COPY
feature was added.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: 8f661a5b662 ("efi_loader: gop: Expose fb when 32bpp")
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v2)

Changes in v2:
- Rebase to -next
- Add some more comments to the header file
- Add fixes tag

 include/video.h          |  9 ++++++---
 lib/efi_loader/efi_gop.c | 12 +++++++-----
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/video.h b/include/video.h
index 5048116a3d57..4d8df9baaada 100644
--- a/include/video.h
+++ b/include/video.h
@@ -21,9 +21,12 @@ struct udevice;
  * @align: Frame-buffer alignment, indicating the memory boundary the frame
  *	buffer should start on. If 0, 1MB is assumed
  * @size: Frame-buffer size, in bytes
- * @base: Base address of frame buffer, 0 if not yet known
- * @copy_base: Base address of a hardware copy of the frame buffer. See
- *	CONFIG_VIDEO_COPY.
+ * @base: Base address of frame buffer, 0 if not yet known. If CONFIG_VIDEO_COPY
+ *	is enabled, this is the software copy, so writes to this will not be
+ *	visible until vidconsole_sync_copy() is called. If CONFIG_VIDEO_COPY is
+ *	disabled, this is the hardware framebuffer.
+ * @copy_base: Base address of a hardware copy of the frame buffer. If
+ *	CONFIG_VIDEO_COPY is disabled, this is not used.
  * @copy_size: Size of copy framebuffer, used if @size is 0
  * @hide_logo: Hide the logo (used for testing)
  */
diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
index 778b693f983a..a09db31eb465 100644
--- a/lib/efi_loader/efi_gop.c
+++ b/lib/efi_loader/efi_gop.c
@@ -10,6 +10,7 @@
 #include <efi_loader.h>
 #include <log.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <video.h>
 #include <asm/global_data.h>
 
@@ -467,10 +468,10 @@ efi_status_t efi_gop_register(void)
 	struct efi_gop_obj *gopobj;
 	u32 bpix, format, col, row;
 	u64 fb_base, fb_size;
-	void *fb;
 	efi_status_t ret;
 	struct udevice *vdev;
 	struct video_priv *priv;
+	struct video_uc_plat *plat;
 
 	/* We only support a single video output device for now */
 	if (uclass_first_device_err(UCLASS_VIDEO, &vdev)) {
@@ -483,9 +484,10 @@ efi_status_t efi_gop_register(void)
 	format = priv->format;
 	col = video_get_xsize(vdev);
 	row = video_get_ysize(vdev);
-	fb_base = (uintptr_t)priv->fb;
-	fb_size = priv->fb_size;
-	fb = priv->fb;
+
+	plat = dev_get_uclass_plat(vdev);
+	fb_base = IS_ENABLED(CONFIG_VIDEO_COPY) ? plat->copy_base : plat->base;
+	fb_size = plat->size;
 
 	switch (bpix) {
 	case VIDEO_BPP16:
@@ -547,7 +549,7 @@ efi_status_t efi_gop_register(void)
 	}
 	gopobj->info.pixels_per_scanline = col;
 	gopobj->bpix = bpix;
-	gopobj->fb = fb;
+	gopobj->fb = map_sysmem(fb_base, fb_size);
 
 	return EFI_SUCCESS;
 }
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 02/12] bootstd: Refactor mmc prep to allow a different scan

[Simon Glass]

Adjust scan_mmc4_bootdev() and related function so that the caller can
do its own 'bootflow scan' command. This allows it to change the flags
if needed.

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

(no changes since v3)

Changes in v3:
- Add new patch to refactor mmc prep to allow a different scan

 test/boot/bootflow.c | 49 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 9 deletions(-)

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index f640db8a2418..102b2b561356 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -511,19 +511,27 @@ BOOTSTD_TEST(bootflow_cmd_boot, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
 /**
  * prep_mmc_bootdev() - Set up an mmc bootdev so we can access other distros
  *
+ * After calling this function, set std->bootdev_order to *@old_orderp to
+ * restore normal operation of bootstd (i.e. with the original bootdev order)
+ *
  * @uts: Unit test state
- * @mmc_dev: MMC device to use, e.g. "mmc4"
+ * @mmc_dev: MMC device to use, e.g. "mmc4". Note that this must remain valid
+ *	in the caller until
+ * @bind_cros: true to bind the ChromiumOS bootmeth
+ * @old_orderp: Returns the original bootdev order, which must be restored
  * Returns 0 on success, -ve on failure
  */
 static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,
-			    bool bind_cros)
+			    bool bind_cros, const char ***old_orderp)
 {
-	const char *order[] = {"mmc2", "mmc1", mmc_dev, NULL};
+	static const char *order[] = {"mmc2", "mmc1", NULL, NULL};
 	struct udevice *dev, *bootstd;
 	struct bootstd_priv *std;
 	const char **old_order;
 	ofnode root, node;
 
+	order[2] = mmc_dev;
+
 	/* Enable the mmc4 node since we need a second bootflow */
 	root = oftree_root(oftree_default());
 	node = ofnode_find_subnode(root, mmc_dev);
@@ -546,26 +554,49 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,
 	std = dev_get_priv(bootstd);
 	old_order = std->bootdev_order;
 	std->bootdev_order = order;
+	*old_orderp = old_order;
+
+	return 0;
+}
+
+/**
+ * scan_mmc_bootdev() - Set up an mmc bootdev so we can access other distros
+ *
+ * @uts: Unit test state
+ * @mmc_dev: MMC device to use, e.g. "mmc4"
+ * @bind_cros: true to bind the ChromiumOS bootmeth
+ * Returns 0 on success, -ve on failure
+ */
+static int scan_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,
+			    bool bind_cros)
+{
+	struct bootstd_priv *std;
+	struct udevice *bootstd;
+	const char **old_order;
+
+	ut_assertok(prep_mmc_bootdev(uts, mmc_dev, bind_cros, &old_order));
 
 	console_record_reset_enable();
 	ut_assertok(run_command("bootflow scan", 0));
 	ut_assert_console_end();
 
 	/* Restore the order used by the device tree */
+	ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
+	std = dev_get_priv(bootstd);
 	std->bootdev_order = old_order;
 
 	return 0;
 }
 
 /**
- * prep_mmc4_bootdev() - Set up the mmc4 bootdev so we can access a fake Armbian
+ * scan_mmc4_bootdev() - Set up the mmc4 bootdev so we can access a fake Armbian
  *
  * @uts: Unit test state
  * Returns 0 on success, -ve on failure
  */
-static int prep_mmc4_bootdev(struct unit_test_state *uts)
+static int scan_mmc4_bootdev(struct unit_test_state *uts)
 {
-	ut_assertok(prep_mmc_bootdev(uts, "mmc4", false));
+	ut_assertok(scan_mmc_bootdev(uts, "mmc4", false));
 
 	return 0;
 }
@@ -575,7 +606,7 @@ static int bootflow_cmd_menu(struct unit_test_state *uts)
 {
 	char prev[3];
 
-	ut_assertok(prep_mmc4_bootdev(uts));
+	ut_assertok(scan_mmc4_bootdev(uts));
 
 	/* Add keypresses to move to and select the second one in the list */
 	prev[0] = CTL_CH('n');
@@ -681,7 +712,7 @@ static int bootflow_menu_theme(struct unit_test_state *uts)
 	ofnode node;
 	int i;
 
-	ut_assertok(prep_mmc4_bootdev(uts));
+	ut_assertok(scan_mmc4_bootdev(uts));
 
 	ut_assertok(bootflow_menu_new(&exp));
 	node = ofnode_path("/bootstd/theme");
@@ -996,7 +1027,7 @@ BOOTSTD_TEST(bootflow_cmdline_special, 0);
 /* Test ChromiumOS bootmeth */
 static int bootflow_cros(struct unit_test_state *uts)
 {
-	ut_assertok(prep_mmc_bootdev(uts, "mmc5", true));
+	ut_assertok(scan_mmc_bootdev(uts, "mmc5", true));
 	ut_assertok(run_command("bootflow list", 0));
 
 	ut_assert_nextlinen("Showing all");
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 03/12] bootstd: Add a return code to bootflow menu

[Simon Glass]

Return an error when the user does not select an OS, so we know whether
to boot or not.

Move calling of bootflow_menu_run() into a separate function so we can
call it from other places.

Expand the test to cover these cases.

Add some documentation also, while we are here.

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

(no changes since v3)

Changes in v3:
- Add missing word 'function' in the commit message

Changes in v2:
- Add new patch to add a return code to bootflow menu

 cmd/bootflow.c             | 53 +++++++++++++++++++++++-------
 doc/usage/cmd/bootflow.rst | 67 ++++++++++++++++++++++++++++++++++++++
 test/boot/bootflow.c       | 15 +++++++++
 3 files changed, 123 insertions(+), 12 deletions(-)

diff --git a/cmd/bootflow.c b/cmd/bootflow.c
index ad39ebe4379f..3aeb40d690f4 100644
--- a/cmd/bootflow.c
+++ b/cmd/bootflow.c
@@ -89,6 +89,44 @@ static void show_footer(int count, int num_valid)
 	       num_valid);
 }
 
+/**
+ * bootflow_handle_menu() - Handle running the menu and updating cur bootflow
+ *
+ * This shows the menu, allows the user to select something and then prints
+ * what happened
+ *
+ * @std: bootstd information
+ * @text_mode: true to run the menu in text mode
+ * @bflowp: Returns selected bootflow, on success
+ * Return: 0 on success (a bootflow was selected), -EAGAIN if nothing was
+ *	chosen, other -ve value on other error
+ */
+__maybe_unused static int bootflow_handle_menu(struct bootstd_priv *std,
+					       bool text_mode,
+					       struct bootflow **bflowp)
+{
+	struct bootflow *bflow;
+	int ret;
+
+	ret = bootflow_menu_run(std, text_mode, &bflow);
+	if (ret) {
+		if (ret == -EAGAIN) {
+			printf("Nothing chosen\n");
+			std->cur_bootflow = NULL;
+		} else {
+			printf("Menu failed (err=%d)\n", ret);
+		}
+
+		return ret;
+	}
+
+	printf("Selected: %s\n", bflow->os_name ? bflow->os_name : bflow->name);
+	std->cur_bootflow = bflow;
+	*bflowp = bflow;
+
+	return 0;
+}
+
 static int do_bootflow_scan(struct cmd_tbl *cmdtp, int flag, int argc,
 			    char *const argv[])
 {
@@ -455,18 +493,9 @@ static int do_bootflow_menu(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (ret)
 		return CMD_RET_FAILURE;
 
-	ret = bootflow_menu_run(std, text_mode, &bflow);
-	if (ret) {
-		if (ret == -EAGAIN)
-			printf("Nothing chosen\n");
-		else {
-			printf("Menu failed (err=%d)\n", ret);
-			return CMD_RET_FAILURE;
-		}
-	}
-
-	printf("Selected: %s\n", bflow->os_name ? bflow->os_name : bflow->name);
-	std->cur_bootflow = bflow;
+	ret = bootflow_handle_menu(std, text_mode, &bflow);
+	if (ret)
+		return CMD_RET_FAILURE;
 
 	return 0;
 }
diff --git a/doc/usage/cmd/bootflow.rst b/doc/usage/cmd/bootflow.rst
index 9c5ea9c5d84e..2198ff60493e 100644
--- a/doc/usage/cmd/bootflow.rst
+++ b/doc/usage/cmd/bootflow.rst
@@ -15,6 +15,7 @@ Synopis
     bootflow read
     bootflow boot
     bootflow cmdline [set|get|clear|delete|auto] <param> [<value>]
+    bootfloe menu [-t]
 
 Description
 -----------
@@ -24,6 +25,9 @@ locate bootflows, list them and boot them.
 
 See :doc:`../../develop/bootstd` for more information.
 
+Note that `CONFIG_BOOTSTD_FULL` (which enables `CONFIG_CMD_BOOTFLOW_FULL) must
+be enabled to obtain full functionality with this command. Otherwise, it only
+supports `bootflow scan` which scans and boots the first available bootflow.
 
 bootflow scan
 ~~~~~~~~~~~~~
@@ -247,6 +251,16 @@ can be used to set the early console (or console) to a suitable value so that
 output appears on the serial port. This is only supported by the 16550 serial
 driver so far.
 
+bootflow menu
+~~~~~~~~~~~~~
+
+This shows a menu with available bootflows. The user can select a particular
+bootflow, which then becomes the current one.
+
+The `-t` flag requests a text menu. Otherwise, if a display is available, a
+graphical menu is shown.
+
+
 Example
 -------
 
@@ -658,6 +672,56 @@ Now the buffer can be accessed::
     77b7e4e0: 320fc000 08e8ba0f c031300f b8d0000f  ...2.....01.....
     77b7e4f0: 00000020 6ad8000f 00858d10 50000002   ......j.......P
 
+This shows using a text menu to boot an OS::
+
+    => bootflow scan
+    => bootfl list
+    => bootfl menu -t
+    U-Boot    :    Boot Menu
+
+    UP and DOWN to choose, ENTER to select
+
+      >    0  mmc1        mmc1.bootdev.whole
+           1  mmc1        Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
+           2  mmc1        mmc1.bootdev.part_1
+           3  mmc4        mmc4.bootdev.whole
+           4  mmc4        Armbian
+           5  mmc4        mmc4.bootdev.part_1
+           6  mmc5        mmc5.bootdev.whole
+           7  mmc5        ChromeOS
+           8  mmc5        ChromeOS
+    U-Boot    :    Boot Menu
+
+    UP and DOWN to choose, ENTER to select
+
+           0  mmc1        mmc1.bootdev.whole
+      >    1  mmc1        Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
+           2  mmc1        mmc1.bootdev.part_1
+           3  mmc4        mmc4.bootdev.whole
+           4  mmc4        Armbian
+           5  mmc4        mmc4.bootdev.part_1
+           6  mmc5        mmc5.bootdev.whole
+           7  mmc5        ChromeOS
+           8  mmc5        ChromeOS
+    U-Boot    :    Boot Menu
+
+    Selected: Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
+    => bootfl boot
+    ** Booting bootflow 'mmc1.bootdev.part_1' with extlinux
+    Ignoring unknown command: ui
+    Ignoring malformed menu command:  autoboot
+    Ignoring malformed menu command:  hidden
+    Ignoring unknown command: totaltimeout
+    Fedora-Workstation-armhfp-31-1.9 Boot Options.
+    1:	Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
+    Enter choice: 1
+    1:	Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
+    Retrieving file: /vmlinuz-5.3.7-301.fc31.armv7hl
+    Retrieving file: /initramfs-5.3.7-301.fc31.armv7hl.img
+    append: ro root=UUID=9732b35b-4cd5-458b-9b91-80f7047e0b8a rhgb quiet LANG=en_US.UTF-8 cma=192MB cma=256MB
+    Retrieving file: /dtb-5.3.7-301.fc31.armv7hl/sandbox.dtb
+    ...
+
 
 Return value
 ------------
@@ -667,6 +731,9 @@ return to U-Boot. If something about the U-Boot processing fails, then the
 return value $? is 1. If the boot succeeds but for some reason the Operating
 System returns, then $? is 0, indicating success.
 
+For `bootflow menu` the return value is $? is 0 (true) if an option was choses,
+else 1.
+
 For other subcommands, the return value $? is always 0 (true).
 
 
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 102b2b561356..b97c566f000b 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -604,8 +604,12 @@ static int scan_mmc4_bootdev(struct unit_test_state *uts)
 /* Check 'bootflow menu' to select a bootflow */
 static int bootflow_cmd_menu(struct unit_test_state *uts)
 {
+	struct bootstd_priv *std;
 	char prev[3];
 
+	/* get access to the current bootflow */
+	ut_assertok(bootstd_get_priv(&std));
+
 	ut_assertok(scan_mmc4_bootdev(uts));
 
 	/* Add keypresses to move to and select the second one in the list */
@@ -616,6 +620,17 @@ static int bootflow_cmd_menu(struct unit_test_state *uts)
 
 	ut_assertok(run_command("bootflow menu", 0));
 	ut_assert_nextline("Selected: Armbian");
+	ut_assertnonnull(std->cur_bootflow);
+	ut_assert_console_end();
+
+	/* Check not selecting anything */
+	prev[0] = '\e';
+	prev[1] = '\0';
+	ut_asserteq(1, console_in_puts(prev));
+
+	ut_asserteq(1, run_command("bootflow menu", 0));
+	ut_assertnull(std->cur_bootflow);
+	ut_assert_nextline("Nothing chosen");
 	ut_assert_console_end();
 
 	return 0;
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 04/12] x86: coreboot: Add a boot script

[Simon Glass]

Provide the user with a list of available boot options. Selecting one
causes it to be booted. Pressing <ESC> causes U-Boot to return to the
command-line prompt.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v3)

Changes in v3:
- Clear the screen before booting

Changes in v2:
- Add new patch to add a coreboot boot script

 configs/coreboot64_defconfig | 1 +
 configs/coreboot_defconfig   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/coreboot64_defconfig b/configs/coreboot64_defconfig
index 1f1327f5bfec..92cca6e6aaef 100644
--- a/configs/coreboot64_defconfig
+++ b/configs/coreboot64_defconfig
@@ -17,6 +17,7 @@ CONFIG_BOOTSTD_DEFAULTS=y
 CONFIG_SHOW_BOOT_PROGRESS=y
 CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="root=/dev/sdb3 init=/sbin/init rootwait ro"
+CONFIG_BOOTCOMMAND="bootflow scan -l; if bootflow menu; then cls; bootflow boot; fi"
 CONFIG_PRE_CONSOLE_BUFFER=y
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
 CONFIG_LOG=y
diff --git a/configs/coreboot_defconfig b/configs/coreboot_defconfig
index 8356bc12659c..3762cde6f3a6 100644
--- a/configs/coreboot_defconfig
+++ b/configs/coreboot_defconfig
@@ -15,6 +15,7 @@ CONFIG_BOOTSTD_DEFAULTS=y
 CONFIG_SHOW_BOOT_PROGRESS=y
 CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="root=/dev/sdb3 init=/sbin/init rootwait ro"
+CONFIG_BOOTCOMMAND="bootflow scan -l; if bootflow menu; then cls; bootflow boot; fi"
 CONFIG_PRE_CONSOLE_BUFFER=y
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
 CONFIG_LOG=y
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Simon Glass]

When a USB device is unbound, it causes any bootflows attached to it to
be removed, via a call to bootdev_clear_bootflows() from
bootdev_pre_unbind(). This obviously makes it impossible to boot the
bootflow.

However, when booting a bootflow that relies on USB, usb_stop() is
called, which unbinds the device. For EFI, this happens in
efi_exit_boot_services() which means that the bootflow disappears
before it is finished with.

There is no need to unbind all the USB devices just to quiesce them.
Add a new usb_pause() call which removes them but leaves them bound.

This resolves a hang on x86 when booting a distro from USB. This was
found using a device with 4 bootflows, the last of which was USB.


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

Changes in v4:
- Don't rename the legacy-USB functions
- Add a bit more detail to the comment

Changes in v2:
- Add new patch to avoid unbinding devices in use by bootflows

 boot/bootm.c                  |  2 +-
 common/usb.c                  |  5 +++++
 drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
 include/usb.h                 | 21 ++++++++++++++++++++-
 4 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index cb61485c226c..d9c3fa8dad99 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -502,7 +502,7 @@ ulong bootm_disable_interrupts(void)
 	 * updated every 1 ms within the HCCA structure in SDRAM! For more
 	 * details see the OpenHCI specification.
 	 */
-	usb_stop();
+	usb_pause();
 #endif
 	return iflag;
 }
diff --git a/common/usb.c b/common/usb.c
index 836506dcd9e9..a486d1c87d4d 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -144,6 +144,11 @@ int usb_stop(void)
 	return 0;
 }
 
+int usb_pause(void)
+{
+	return usb_stop();
+}
+
 /******************************************************************************
  * Detect if a USB device has been plugged or unplugged.
  */
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index a1cd0ad2d669..c26c65d7986b 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
 	return ops->get_max_xfer_size(bus, size);
 }
 
-int usb_stop(void)
+static int usb_finish(bool unbind_all)
 {
 	struct udevice *bus;
 	struct udevice *rh;
@@ -195,7 +195,7 @@ int usb_stop(void)
 
 		/* Locate root hub device */
 		device_find_first_child(bus, &rh);
-		if (rh) {
+		if (rh && unbind_all) {
 			/*
 			 * All USB devices are children of root hub.
 			 * Unbinding root hub will unbind all of its children.
@@ -222,6 +222,16 @@ int usb_stop(void)
 	return err;
 }
 
+int usb_stop(void)
+{
+	return usb_finish(true);
+}
+
+int usb_pause(void)
+{
+	return usb_finish(false);
+}
+
 static void usb_scan_bus(struct udevice *bus, bool recurse)
 {
 	struct usb_bus_priv *priv;
diff --git a/include/usb.h b/include/usb.h
index 09e3f0cb309c..b964e2e1f6b2 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -265,7 +265,26 @@ int usb_kbd_deregister(int force);
  */
 int usb_init(void);
 
-int usb_stop(void); /* stop the USB Controller */
+/**
+ * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
+ *
+ * This unbinds all devices on the USB buses.
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int usb_stop(void);
+
+/**
+ * usb_pause() - stop the USB Controller DMA, etc.
+ *
+ * Note that this does not unbind bus devices, so using usb_init() after this
+ * call is not permitted. This function is only useful just before booting, to
+ * ensure that devices are dormant.
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int usb_pause(void);
+
 int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
 
 
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 06/12] expo: Correct background colour

[Simon Glass]

Use the correct background colour when using white-on-black.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v1)

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

diff --git a/boot/expo.c b/boot/expo.c
index 139d684f8e6e..cadb6a0ad6e3 100644
--- a/boot/expo.c
+++ b/boot/expo.c
@@ -190,10 +190,12 @@ int expo_render(struct expo *exp)
 	struct udevice *dev = exp->display;
 	struct video_priv *vid_priv = dev_get_uclass_priv(dev);
 	struct scene *scn = NULL;
+	enum colour_idx back;
 	u32 colour;
 	int ret;
 
-	colour = video_index_to_colour(vid_priv, VID_WHITE);
+	back = CONFIG_IS_ENABLED(SYS_WHITE_ON_BLACK) ? VID_BLACK : VID_WHITE;
+	colour = video_index_to_colour(vid_priv, back);
 	ret = video_fill(dev, colour);
 	if (ret)
 		return log_msg_ret("fill", ret);
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 07/12] video: Correct setting of cursor position

[Simon Glass]

The ANSI codes are not correctly handled at present, in that the
requested X position is added to the current one.

Correct this and also call vidconsole_entry_start() to start a new text
line.

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

(no changes since v1)

 drivers/video/vidconsole-uclass.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
index 22d55df71f63..a312a1985242 100644
--- a/drivers/video/vidconsole-uclass.c
+++ b/drivers/video/vidconsole-uclass.c
@@ -125,6 +125,7 @@ void vidconsole_set_cursor_pos(struct udevice *dev, int x, int y)
 	priv->xcur_frac = VID_TO_POS(x);
 	priv->xstart_frac = priv->xcur_frac;
 	priv->ycur = y;
+	vidconsole_entry_start(dev);
 }
 
 /**
@@ -134,8 +135,10 @@ void vidconsole_set_cursor_pos(struct udevice *dev, int x, int y)
  * @row:	new row
  * @col:	new column
  */
-static void set_cursor_position(struct vidconsole_priv *priv, int row, int col)
+static void set_cursor_position(struct udevice *dev, int row, int col)
 {
+	struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
+
 	/*
 	 * Ensure we stay in the bounds of the screen.
 	 */
@@ -144,9 +147,7 @@ static void set_cursor_position(struct vidconsole_priv *priv, int row, int col)
 	if (col >= priv->cols)
 		col = priv->cols - 1;
 
-	priv->ycur = row * priv->y_charsize;
-	priv->xcur_frac = priv->xstart_frac +
-			  VID_TO_POS(col * priv->x_charsize);
+	vidconsole_position_cursor(dev, col, row);
 }
 
 /**
@@ -193,7 +194,7 @@ static void vidconsole_escape_char(struct udevice *dev, char ch)
 			int row = priv->row_saved;
 			int col = priv->col_saved;
 
-			set_cursor_position(priv, row, col);
+			set_cursor_position(dev, row, col);
 			priv->escape = 0;
 			return;
 		}
@@ -255,7 +256,7 @@ static void vidconsole_escape_char(struct udevice *dev, char ch)
 		if (row < 0)
 			row = 0;
 		/* Right and bottom overflows are handled in the callee. */
-		set_cursor_position(priv, row, col);
+		set_cursor_position(dev, row, col);
 		break;
 	}
 	case 'H':
@@ -279,7 +280,7 @@ static void vidconsole_escape_char(struct udevice *dev, char ch)
 		if (col)
 			--col;
 
-		set_cursor_position(priv, row, col);
+		set_cursor_position(dev, row, col);
 
 		break;
 	}
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 08/12] video: Drop unnecessary truetype operations from SPL

[Simon Glass]

Saving and restoring entries is used for expo and for the command line,
which we don't use in SPL. Drop these methods.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v3)

Changes in v3:
- Add new patch to drop unnecessary truetype operations from SPL

 drivers/video/console_truetype.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/video/console_truetype.c b/drivers/video/console_truetype.c
index 14fb81e9563c..602133575f8c 100644
--- a/drivers/video/console_truetype.c
+++ b/drivers/video/console_truetype.c
@@ -8,6 +8,7 @@
 #include <dm.h>
 #include <log.h>
 #include <malloc.h>
+#include <spl.h>
 #include <video.h>
 #include <video_console.h>
 
@@ -799,6 +800,9 @@ static int truetype_entry_save(struct udevice *dev, struct abuf *buf)
 	struct console_tt_store store;
 	const uint size = sizeof(store);
 
+	if (spl_phase() <= PHASE_SPL)
+		return -ENOSYS;
+
 	/*
 	 * store the whole priv structure as it is simpler that picking out
 	 * what we need
@@ -820,6 +824,9 @@ static int truetype_entry_restore(struct udevice *dev, struct abuf *buf)
 	struct console_tt_priv *priv = dev_get_priv(dev);
 	struct console_tt_store store;
 
+	if (spl_phase() <= PHASE_SPL)
+		return -ENOSYS;
+
 	memcpy(&store, abuf_data(buf), sizeof(store));
 
 	vc_priv->xcur_frac = store.cur.xpos_frac;
@@ -844,6 +851,9 @@ static int truetype_set_cursor_visible(struct udevice *dev, bool visible,
 	uint out, val;
 	int ret;
 
+	if (spl_phase() <= PHASE_SPL)
+		return -ENOSYS;
+
 	if (!visible)
 		return 0;
 
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Simon Glass]

This is needed to support Truetype fonts. In any case, the compiler
expects SSE to be available in 64-bit mode. Provide an option to enable
SSE so that hardware floating-point arithmetic works.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Bin Meng <bmeng.cn@gmail.com>
---

Changes in v4:
- Use a Kconfig option

 arch/x86/Kconfig          |  8 ++++++++
 arch/x86/config.mk        |  4 ++++
 arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
 drivers/video/Kconfig     |  1 +
 4 files changed, 25 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 99e59d94c606..6b532d712ee8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
 	hex
 	default 0x10000
 
+config X86_HARDFP
+	bool "Support hardware floating point"
+	help
+	  U-Boot generally does not make use of floating point. Where this is
+	  needed, it can be enabled using this option. This adjusts the
+	  start-up code for 64-bit mode and changes the compiler options for
+	  64-bit to enable SSE.
+
 config HAVE_ITSS
 	bool "Enable ITSS"
 	help
diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index 26ec1af2f0b0..2e3a7119e798 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
 PLATFORM_CPPFLAGS += -march=i386 -m32
 else
 PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
+
+ifndef CONFIG_X86_HARDFP
 PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
 endif
 
+endif # IS_32BIT
+
 PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
 
 KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
index 2647bff891f8..5ea746ecce4d 100644
--- a/arch/x86/cpu/x86_64/cpu.c
+++ b/arch/x86/cpu/x86_64/cpu.c
@@ -10,6 +10,7 @@
 #include <init.h>
 #include <asm/cpu.h>
 #include <asm/global_data.h>
+#include <asm/processor-flags.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -39,11 +40,22 @@ int x86_mp_init(void)
 	return 0;
 }
 
+/* enable SSE features for hardware floating point */
+static void setup_sse_features(void)
+{
+	asm ("mov %%cr4, %%rax\n" \
+	"or  %0, %%rax\n" \
+	"mov %%rax, %%cr4\n" \
+	: : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
+}
+
 int x86_cpu_reinit_f(void)
 {
 	/* set the vendor to Intel so that native_calibrate_tsc() works */
 	gd->arch.x86_vendor = X86_VENDOR_INTEL;
 	gd->arch.has_mtrr = true;
+	if (IS_ENABLED(CONFIG_X86_HARDFP))
+		setup_sse_features();
 
 	return 0;
 }
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 6f319ba0d544..39c82521be16 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -180,6 +180,7 @@ config CONSOLE_ROTATION
 
 config CONSOLE_TRUETYPE
 	bool "Support a console that uses TrueType fonts"
+	select X86_HARDFP if X86
 	help
 	  TrueTrype fonts can provide outline-drawing capability rather than
 	  needing to provide a bitmap for each font and size that is needed.
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 10/12] x86: coreboot: Enable truetype fonts

[Simon Glass]

Truetype fonts look better in the menu, so enable them.

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

(no changes since v3)

Changes in v3:
- Add new patch to enable truetype fonts in coreboot

 arch/x86/dts/coreboot.dts    | 10 ++++++++++
 configs/coreboot64_defconfig |  1 +
 configs/coreboot_defconfig   |  1 +
 3 files changed, 12 insertions(+)

diff --git a/arch/x86/dts/coreboot.dts b/arch/x86/dts/coreboot.dts
index 0eb31cae42c1..83beb82f37c7 100644
--- a/arch/x86/dts/coreboot.dts
+++ b/arch/x86/dts/coreboot.dts
@@ -45,4 +45,14 @@
 		bootph-some-ram;
 		compatible = "coreboot-fb";
 	};
+
+	bootstd {
+		compatible = "u-boot,boot-std";
+
+		theme {
+			font-size = <30>;
+			menu-inset = <3>;
+			menuitem-gap-y = <1>;
+		};
+	};
 };
diff --git a/configs/coreboot64_defconfig b/configs/coreboot64_defconfig
index 92cca6e6aaef..42b46ba0d9b9 100644
--- a/configs/coreboot64_defconfig
+++ b/configs/coreboot64_defconfig
@@ -61,6 +61,7 @@ CONFIG_SYS_NS16550_MEM32=y
 CONFIG_SOUND=y
 CONFIG_SOUND_I8254=y
 CONFIG_VIDEO_COPY=y
+CONFIG_CONSOLE_TRUETYPE=y
 CONFIG_CONSOLE_SCROLL_LINES=5
 CONFIG_SPL_ACPI=y
 CONFIG_CMD_DHRYSTONE=y
diff --git a/configs/coreboot_defconfig b/configs/coreboot_defconfig
index 3762cde6f3a6..7d744e9962ae 100644
--- a/configs/coreboot_defconfig
+++ b/configs/coreboot_defconfig
@@ -55,6 +55,7 @@ CONFIG_SYS_NS16550_MEM32=y
 CONFIG_SOUND=y
 CONFIG_SOUND_I8254=y
 CONFIG_VIDEO_COPY=y
+CONFIG_CONSOLE_TRUETYPE=y
 CONFIG_CONSOLE_SCROLL_LINES=5
 CONFIG_CMD_DHRYSTONE=y
 # CONFIG_GZIP is not set
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 11/12] x86: qemu: Expand ROM size

[Simon Glass]

Expand the ROM for x86_64 to 2MB to make space for the font, as it is
already on the edge.

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

(no changes since v1)

 board/emulation/qemu-x86/Kconfig | 3 ++-
 configs/qemu-x86_64_defconfig    | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/board/emulation/qemu-x86/Kconfig b/board/emulation/qemu-x86/Kconfig
index 01dc1d497aec..34d665a3e4c0 100644
--- a/board/emulation/qemu-x86/Kconfig
+++ b/board/emulation/qemu-x86/Kconfig
@@ -21,7 +21,8 @@ config BOARD_SPECIFIC_OPTIONS # dummy
 	select X86_RESET_VECTOR
 	select QEMU
 	select QFW_PIO if CMD_QFW
-	select BOARD_ROMSIZE_KB_1024
+	select BOARD_ROMSIZE_KB_1024 if TARGET_QEMU_X86
+	select BOARD_ROMSIZE_KB_2048 if TARGET_QEMU_X86_64
 	imply VIRTIO_PCI
 	imply VIRTIO_NET
 	imply VIRTIO_BLK
diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
index 2ff49fbd6acc..a71111697e38 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -6,7 +6,7 @@ CONFIG_ENV_SIZE=0x40000
 CONFIG_MAX_CPUS=2
 CONFIG_SPL_DM_SPI=y
 CONFIG_DEFAULT_DEVICE_TREE="qemu-x86_i440fx"
-CONFIG_SPL_TEXT_BASE=0xfffd8000
+CONFIG_SPL_TEXT_BASE=0xfffd4000
 CONFIG_SPL_SYS_MALLOC_F_LEN=0x2000
 CONFIG_DEBUG_UART_BASE=0x3f8
 CONFIG_DEBUG_UART_CLOCK=1843200
@@ -17,7 +17,7 @@ CONFIG_DEBUG_UART=y
 CONFIG_SMP=y
 CONFIG_GENERATE_PIRQ_TABLE=y
 CONFIG_GENERATE_MP_TABLE=y
-CONFIG_X86_OFFSET_U_BOOT=0xfff00000
+CONFIG_X86_OFFSET_U_BOOT=0xffe00000
 CONFIG_SYS_MONITOR_BASE=0x01110000
 CONFIG_FIT=y
 CONFIG_SPL_LOAD_FIT=y
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 12/12] x86: qemu: Enable truetype fonts

[Simon Glass]

Enable this feature to provide a larger font choice and more attractive
menus. Expand the ROM for x86_64 to 2MB to make space for the font.

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

(no changes since v3)

Changes in v3:
- Add new patch to enable truetype fonts in qemu-x86 and qemu-x86_64

 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 a71111697e38..99855df64442 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -82,6 +82,7 @@ CONFIG_SPL_DM_RTC=y
 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
diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
index 246ac6b6b8ad..65bb5f9bd83d 100644
--- a/configs/qemu-x86_defconfig
+++ b/configs/qemu-x86_defconfig
@@ -59,6 +59,7 @@ CONFIG_NVME_PCI=y
 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
-- 
2.42.0.869.gea05f2083d-goog

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Heinrich Schuchardt]

Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass <sjg@chromium.org>:
>When a USB device is unbound, it causes any bootflows attached to it to
>be removed, via a call to bootdev_clear_bootflows() from
>bootdev_pre_unbind(). This obviously makes it impossible to boot the
>bootflow.
>
>However, when booting a bootflow that relies on USB, usb_stop() is
>called, which unbinds the device. For EFI, this happens in
>efi_exit_boot_services() which means that the bootflow disappears
>before it is finished with.

After ExitBootServices() no driver of U-Boot can be used as the operating system is in charge.

Any bootflow inside U-Boot is terminated by definition when reaching ExitBootServices.

>
>There is no need to unbind all the USB devices just to quiesce them.
>Add a new usb_pause() call which removes them but leaves them bound.

As U-Boot must not access any device after ExitBootServices() it is unclear to me what you want to achieve.

Best regards

Heinrich

>
>This resolves a hang on x86 when booting a distro from USB. This was
>found using a device with 4 bootflows, the last of which was USB.
>
>
>Signed-off-by: Simon Glass <sjg@chromium.org>
>---
>
>Changes in v4:
>- Don't rename the legacy-USB functions
>- Add a bit more detail to the comment
>
>Changes in v2:
>- Add new patch to avoid unbinding devices in use by bootflows
>
> boot/bootm.c                  |  2 +-
> common/usb.c                  |  5 +++++
> drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
> include/usb.h                 | 21 ++++++++++++++++++++-
> 4 files changed, 38 insertions(+), 4 deletions(-)
>
>diff --git a/boot/bootm.c b/boot/bootm.c
>index cb61485c226c..d9c3fa8dad99 100644
>--- a/boot/bootm.c
>+++ b/boot/bootm.c
>@@ -502,7 +502,7 @@ ulong bootm_disable_interrupts(void)
> 	 * updated every 1 ms within the HCCA structure in SDRAM! For more
> 	 * details see the OpenHCI specification.
> 	 */
>-	usb_stop();
>+	usb_pause();
> #endif
> 	return iflag;
> }
>diff --git a/common/usb.c b/common/usb.c
>index 836506dcd9e9..a486d1c87d4d 100644
>--- a/common/usb.c
>+++ b/common/usb.c
>@@ -144,6 +144,11 @@ int usb_stop(void)
> 	return 0;
> }
> 
>+int usb_pause(void)
>+{
>+	return usb_stop();
>+}
>+
> /******************************************************************************
>  * Detect if a USB device has been plugged or unplugged.
>  */
>diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>index a1cd0ad2d669..c26c65d7986b 100644
>--- a/drivers/usb/host/usb-uclass.c
>+++ b/drivers/usb/host/usb-uclass.c
>@@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
> 	return ops->get_max_xfer_size(bus, size);
> }
> 
>-int usb_stop(void)
>+static int usb_finish(bool unbind_all)
> {
> 	struct udevice *bus;
> 	struct udevice *rh;
>@@ -195,7 +195,7 @@ int usb_stop(void)
> 
> 		/* Locate root hub device */
> 		device_find_first_child(bus, &rh);
>-		if (rh) {
>+		if (rh && unbind_all) {
> 			/*
> 			 * All USB devices are children of root hub.
> 			 * Unbinding root hub will unbind all of its children.
>@@ -222,6 +222,16 @@ int usb_stop(void)
> 	return err;
> }
> 
>+int usb_stop(void)
>+{
>+	return usb_finish(true);
>+}
>+
>+int usb_pause(void)
>+{
>+	return usb_finish(false);
>+}
>+
> static void usb_scan_bus(struct udevice *bus, bool recurse)
> {
> 	struct usb_bus_priv *priv;
>diff --git a/include/usb.h b/include/usb.h
>index 09e3f0cb309c..b964e2e1f6b2 100644
>--- a/include/usb.h
>+++ b/include/usb.h
>@@ -265,7 +265,26 @@ int usb_kbd_deregister(int force);
>  */
> int usb_init(void);
> 
>-int usb_stop(void); /* stop the USB Controller */
>+/**
>+ * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
>+ *
>+ * This unbinds all devices on the USB buses.
>+ *
>+ * Return: 0 if OK, -ve on error
>+ */
>+int usb_stop(void);
>+
>+/**
>+ * usb_pause() - stop the USB Controller DMA, etc.
>+ *
>+ * Note that this does not unbind bus devices, so using usb_init() after this
>+ * call is not permitted. This function is only useful just before booting, to
>+ * ensure that devices are dormant.
>+ *
>+ * Return: 0 if OK, -ve on error
>+ */
>+int usb_pause(void);
>+
> int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
> 
> 

Re: [PATCH v4 07/12] video: Correct setting of cursor position

[Anatolij Gustschin]

On Sun, 12 Nov 2023 13:02:44 -0700
Simon Glass sjg@chromium.org wrote:

> The ANSI codes are not correctly handled at present, in that the
> requested X position is added to the current one.
> 
> Correct this and also call vidconsole_entry_start() to start a new text
> line.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  drivers/video/vidconsole-uclass.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
 
Reviewed-by: Anatolij Gustschin <agust@denx.de>

Re: [PATCH v4 08/12] video: Drop unnecessary truetype operations from SPL

[Anatolij Gustschin]

On Sun, 12 Nov 2023 13:02:45 -0700
Simon Glass sjg@chromium.org wrote:

> Saving and restoring entries is used for expo and for the command line,
> which we don't use in SPL. Drop these methods.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
> (no changes since v3)
> 
> Changes in v3:
> - Add new patch to drop unnecessary truetype operations from SPL
> 
>  drivers/video/console_truetype.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Anatolij Gustschin <agust@denx.de>

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Simon Glass]

Hi Heinrich,

On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >When a USB device is unbound, it causes any bootflows attached to it to
> >be removed, via a call to bootdev_clear_bootflows() from
> >bootdev_pre_unbind(). This obviously makes it impossible to boot the
> >bootflow.
> >
> >However, when booting a bootflow that relies on USB, usb_stop() is
> >called, which unbinds the device. For EFI, this happens in
> >efi_exit_boot_services() which means that the bootflow disappears
> >before it is finished with.
>
> After ExitBootServices() no driver of U-Boot can be used as the operating system is in charge.
>
> Any bootflow inside U-Boot is terminated by definition when reaching ExitBootServices.
>
> >
> >There is no need to unbind all the USB devices just to quiesce them.
> >Add a new usb_pause() call which removes them but leaves them bound.
>
> As U-Boot must not access any device after ExitBootServices() it is unclear to me what you want to achieve.

I can't remember exactly what is needed from the bootflow, but
something is. Perhaps the kernel, or the cmdline, or fdt? It would
have been better if I had mentioned this explicitly,  But then this
patch has been sitting around for ages...

In any case, the intent of exit-boot-services is not to free all the
memory used, since some of it may have been used to hold data that the
app continues to use.

Also there is U-Boot code between when the devices are unbound and
when U-Boot actually exits back to the app.

This hang was 100% repeatable on brya (an x86 Chromebook) and it took
quite a while to find.

Regards,
Simon


>
> Best regards
>
> Heinrich
>
> >
> >This resolves a hang on x86 when booting a distro from USB. This was
> >found using a device with 4 bootflows, the last of which was USB.
> >
> >
> >Signed-off-by: Simon Glass <sjg@chromium.org>
> >---
> >
> >Changes in v4:
> >- Don't rename the legacy-USB functions
> >- Add a bit more detail to the comment
> >
> >Changes in v2:
> >- Add new patch to avoid unbinding devices in use by bootflows
> >
> > boot/bootm.c                  |  2 +-
> > common/usb.c                  |  5 +++++
> > drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
> > include/usb.h                 | 21 ++++++++++++++++++++-
> > 4 files changed, 38 insertions(+), 4 deletions(-)
> >
> >diff --git a/boot/bootm.c b/boot/bootm.c
> >index cb61485c226c..d9c3fa8dad99 100644
> >--- a/boot/bootm.c
> >+++ b/boot/bootm.c
> >@@ -502,7 +502,7 @@ ulong bootm_disable_interrupts(void)
> >        * updated every 1 ms within the HCCA structure in SDRAM! For more
> >        * details see the OpenHCI specification.
> >        */
> >-      usb_stop();
> >+      usb_pause();
> > #endif
> >       return iflag;
> > }
> >diff --git a/common/usb.c b/common/usb.c
> >index 836506dcd9e9..a486d1c87d4d 100644
> >--- a/common/usb.c
> >+++ b/common/usb.c
> >@@ -144,6 +144,11 @@ int usb_stop(void)
> >       return 0;
> > }
> >
> >+int usb_pause(void)
> >+{
> >+      return usb_stop();
> >+}
> >+
> > /******************************************************************************
> >  * Detect if a USB device has been plugged or unplugged.
> >  */
> >diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> >index a1cd0ad2d669..c26c65d7986b 100644
> >--- a/drivers/usb/host/usb-uclass.c
> >+++ b/drivers/usb/host/usb-uclass.c
> >@@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
> >       return ops->get_max_xfer_size(bus, size);
> > }
> >
> >-int usb_stop(void)
> >+static int usb_finish(bool unbind_all)
> > {
> >       struct udevice *bus;
> >       struct udevice *rh;
> >@@ -195,7 +195,7 @@ int usb_stop(void)
> >
> >               /* Locate root hub device */
> >               device_find_first_child(bus, &rh);
> >-              if (rh) {
> >+              if (rh && unbind_all) {
> >                       /*
> >                        * All USB devices are children of root hub.
> >                        * Unbinding root hub will unbind all of its children.
> >@@ -222,6 +222,16 @@ int usb_stop(void)
> >       return err;
> > }
> >
> >+int usb_stop(void)
> >+{
> >+      return usb_finish(true);
> >+}
> >+
> >+int usb_pause(void)
> >+{
> >+      return usb_finish(false);
> >+}
> >+
> > static void usb_scan_bus(struct udevice *bus, bool recurse)
> > {
> >       struct usb_bus_priv *priv;
> >diff --git a/include/usb.h b/include/usb.h
> >index 09e3f0cb309c..b964e2e1f6b2 100644
> >--- a/include/usb.h
> >+++ b/include/usb.h
> >@@ -265,7 +265,26 @@ int usb_kbd_deregister(int force);
> >  */
> > int usb_init(void);
> >
> >-int usb_stop(void); /* stop the USB Controller */
> >+/**
> >+ * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
> >+ *
> >+ * This unbinds all devices on the USB buses.
> >+ *
> >+ * Return: 0 if OK, -ve on error
> >+ */
> >+int usb_stop(void);
> >+
> >+/**
> >+ * usb_pause() - stop the USB Controller DMA, etc.
> >+ *
> >+ * Note that this does not unbind bus devices, so using usb_init() after this
> >+ * call is not permitted. This function is only useful just before booting, to
> >+ * ensure that devices are dormant.
> >+ *
> >+ * Return: 0 if OK, -ve on error
> >+ */
> >+int usb_pause(void);
> >+
> > int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
> >
> >

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Heinrich Schuchardt]

Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass <sjg@chromium.org>:
>> >When a USB device is unbound, it causes any bootflows attached to it to
>> >be removed, via a call to bootdev_clear_bootflows() from
>> >bootdev_pre_unbind(). This obviously makes it impossible to boot the
>> >bootflow.
>> >
>> >However, when booting a bootflow that relies on USB, usb_stop() is
>> >called, which unbinds the device. For EFI, this happens in
>> >efi_exit_boot_services() which means that the bootflow disappears
>> >before it is finished with.
>>
>> After ExitBootServices() no driver of U-Boot can be used as the operating system is in charge.
>>
>> Any bootflow inside U-Boot is terminated by definition when reaching ExitBootServices.
>>
>> >
>> >There is no need to unbind all the USB devices just to quiesce them.
>> >Add a new usb_pause() call which removes them but leaves them bound.
>>
>> As U-Boot must not access any device after ExitBootServices() it is unclear to me what you want to achieve.
>
>I can't remember exactly what is needed from the bootflow, but
>something is. Perhaps the kernel, or the cmdline, or fdt? It would
>have been better if I had mentioned this explicitly,  But then this
>patch has been sitting around for ages...
>
>In any case, the intent of exit-boot-services is not to free all the
>memory used, since some of it may have been used to hold data that the
>app continues to use.

The EFI application reads the memory map and receives an ID which it passes to ExitBootServiced() after this point any memory not marked as EFI runtime can be used by the EFI app. This includes the memory that harbors the U-Boot USB drivers. Therefore no drivers can be used here.

Starting the EFI app via StartImage() must  terminate any running U-Boot "boot flow".

After ExitBootServices() the EFI application cannot return to U-Boot except for SetVirtualAdressMspsetting which relocates the EFI runtime.

Bootflows and U-Boot drivers have no meaning after ExitBootServices().



>
>Also there is U-Boot code between when the devices are unbound and
>when U-Boot actually exits back to the app.
>
>This hang was 100% repeatable on brya (an x86 Chromebook) and it took
>quite a while to find.

We need a better understanding of the problem that you experience to find an adequate solution. Why does removing all devices lead to hanging the system?

Best regards

Heinrich

>
>Regards,
>Simon
>
>
>>
>> Best regards
>>
>> Heinrich
>>
>> >
>> >This resolves a hang on x86 when booting a distro from USB. This was
>> >found using a device with 4 bootflows, the last of which was USB.
>> >
>> >
>> >Signed-off-by: Simon Glass <sjg@chromium.org>
>> >---
>> >
>> >Changes in v4:
>> >- Don't rename the legacy-USB functions
>> >- Add a bit more detail to the comment
>> >
>> >Changes in v2:
>> >- Add new patch to avoid unbinding devices in use by bootflows
>> >
>> > boot/bootm.c                  |  2 +-
>> > common/usb.c                  |  5 +++++
>> > drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
>> > include/usb.h                 | 21 ++++++++++++++++++++-
>> > 4 files changed, 38 insertions(+), 4 deletions(-)
>> >
>> >diff --git a/boot/bootm.c b/boot/bootm.c
>> >index cb61485c226c..d9c3fa8dad99 100644
>> >--- a/boot/bootm.c
>> >+++ b/boot/bootm.c
>> >@@ -502,7 +502,7 @@ ulong bootm_disable_interrupts(void)
>> >        * updated every 1 ms within the HCCA structure in SDRAM! For more
>> >        * details see the OpenHCI specification.
>> >        */
>> >-      usb_stop();
>> >+      usb_pause();
>> > #endif
>> >       return iflag;
>> > }
>> >diff --git a/common/usb.c b/common/usb.c
>> >index 836506dcd9e9..a486d1c87d4d 100644
>> >--- a/common/usb.c
>> >+++ b/common/usb.c
>> >@@ -144,6 +144,11 @@ int usb_stop(void)
>> >       return 0;
>> > }
>> >
>> >+int usb_pause(void)
>> >+{
>> >+      return usb_stop();
>> >+}
>> >+
>> > /******************************************************************************
>> >  * Detect if a USB device has been plugged or unplugged.
>> >  */
>> >diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>> >index a1cd0ad2d669..c26c65d7986b 100644
>> >--- a/drivers/usb/host/usb-uclass.c
>> >+++ b/drivers/usb/host/usb-uclass.c
>> >@@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
>> >       return ops->get_max_xfer_size(bus, size);
>> > }
>> >
>> >-int usb_stop(void)
>> >+static int usb_finish(bool unbind_all)
>> > {
>> >       struct udevice *bus;
>> >       struct udevice *rh;
>> >@@ -195,7 +195,7 @@ int usb_stop(void)
>> >
>> >               /* Locate root hub device */
>> >               device_find_first_child(bus, &rh);
>> >-              if (rh) {
>> >+              if (rh && unbind_all) {
>> >                       /*
>> >                        * All USB devices are children of root hub.
>> >                        * Unbinding root hub will unbind all of its children.
>> >@@ -222,6 +222,16 @@ int usb_stop(void)
>> >       return err;
>> > }
>> >
>> >+int usb_stop(void)
>> >+{
>> >+      return usb_finish(true);
>> >+}
>> >+
>> >+int usb_pause(void)
>> >+{
>> >+      return usb_finish(false);
>> >+}
>> >+
>> > static void usb_scan_bus(struct udevice *bus, bool recurse)
>> > {
>> >       struct usb_bus_priv *priv;
>> >diff --git a/include/usb.h b/include/usb.h
>> >index 09e3f0cb309c..b964e2e1f6b2 100644
>> >--- a/include/usb.h
>> >+++ b/include/usb.h
>> >@@ -265,7 +265,26 @@ int usb_kbd_deregister(int force);
>> >  */
>> > int usb_init(void);
>> >
>> >-int usb_stop(void); /* stop the USB Controller */
>> >+/**
>> >+ * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
>> >+ *
>> >+ * This unbinds all devices on the USB buses.
>> >+ *
>> >+ * Return: 0 if OK, -ve on error
>> >+ */
>> >+int usb_stop(void);
>> >+
>> >+/**
>> >+ * usb_pause() - stop the USB Controller DMA, etc.
>> >+ *
>> >+ * Note that this does not unbind bus devices, so using usb_init() after this
>> >+ * call is not permitted. This function is only useful just before booting, to
>> >+ * ensure that devices are dormant.
>> >+ *
>> >+ * Return: 0 if OK, -ve on error
>> >+ */
>> >+int usb_pause(void);
>> >+
>> > int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
>> >
>> >

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Bin Meng]

Hi Simon,

On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
>
> This is needed to support Truetype fonts. In any case, the compiler
> expects SSE to be available in 64-bit mode. Provide an option to enable
> SSE so that hardware floating-point arithmetic works.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> Changes in v4:
> - Use a Kconfig option
>
>  arch/x86/Kconfig          |  8 ++++++++
>  arch/x86/config.mk        |  4 ++++
>  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
>  drivers/video/Kconfig     |  1 +
>  4 files changed, 25 insertions(+)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 99e59d94c606..6b532d712ee8 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
>         hex
>         default 0x10000
>
> +config X86_HARDFP
> +       bool "Support hardware floating point"
> +       help
> +         U-Boot generally does not make use of floating point. Where this is
> +         needed, it can be enabled using this option. This adjusts the
> +         start-up code for 64-bit mode and changes the compiler options for
> +         64-bit to enable SSE.

As discussed in another thread, this option should be made global to
all architectures and by default no.

> +
>  config HAVE_ITSS
>         bool "Enable ITSS"
>         help
> diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> index 26ec1af2f0b0..2e3a7119e798 100644
> --- a/arch/x86/config.mk
> +++ b/arch/x86/config.mk
> @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
>  PLATFORM_CPPFLAGS += -march=i386 -m32
>  else
>  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> +
> +ifndef CONFIG_X86_HARDFP
>  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
>  endif
>
> +endif # IS_32BIT
> +
>  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
>
>  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> index 2647bff891f8..5ea746ecce4d 100644
> --- a/arch/x86/cpu/x86_64/cpu.c
> +++ b/arch/x86/cpu/x86_64/cpu.c
> @@ -10,6 +10,7 @@
>  #include <init.h>
>  #include <asm/cpu.h>
>  #include <asm/global_data.h>
> +#include <asm/processor-flags.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -39,11 +40,22 @@ int x86_mp_init(void)
>         return 0;
>  }
>
> +/* enable SSE features for hardware floating point */
> +static void setup_sse_features(void)
> +{
> +       asm ("mov %%cr4, %%rax\n" \
> +       "or  %0, %%rax\n" \
> +       "mov %%rax, %%cr4\n" \
> +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> +}
> +
>  int x86_cpu_reinit_f(void)
>  {
>         /* set the vendor to Intel so that native_calibrate_tsc() works */
>         gd->arch.x86_vendor = X86_VENDOR_INTEL;
>         gd->arch.has_mtrr = true;
> +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> +               setup_sse_features();
>
>         return 0;
>  }
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 6f319ba0d544..39c82521be16 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
>
>  config CONSOLE_TRUETYPE
>         bool "Support a console that uses TrueType fonts"
> +       select X86_HARDFP if X86

This should be "depends on HARDFP", indicating that the TrueType
library is using hardware fp itself, and user has to explicitly turn
the hardware fp Kconfig option on.

"Select" does not work for architectures that does not have the
"enabling hardware fp" logic in place.

>         help
>           TrueTrype fonts can provide outline-drawing capability rather than
>           needing to provide a bitmap for each font and size that is needed.
> --

Regards,
Bin

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Simon Glass]

Hi Bin,

On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > This is needed to support Truetype fonts. In any case, the compiler
> > expects SSE to be available in 64-bit mode. Provide an option to enable
> > SSE so that hardware floating-point arithmetic works.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> > Changes in v4:
> > - Use a Kconfig option
> >
> >  arch/x86/Kconfig          |  8 ++++++++
> >  arch/x86/config.mk        |  4 ++++
> >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> >  drivers/video/Kconfig     |  1 +
> >  4 files changed, 25 insertions(+)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 99e59d94c606..6b532d712ee8 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> >         hex
> >         default 0x10000
> >
> > +config X86_HARDFP
> > +       bool "Support hardware floating point"
> > +       help
> > +         U-Boot generally does not make use of floating point. Where this is
> > +         needed, it can be enabled using this option. This adjusts the
> > +         start-up code for 64-bit mode and changes the compiler options for
> > +         64-bit to enable SSE.
>
> As discussed in another thread, this option should be made global to
> all architectures and by default no.
>
> > +
> >  config HAVE_ITSS
> >         bool "Enable ITSS"
> >         help
> > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > index 26ec1af2f0b0..2e3a7119e798 100644
> > --- a/arch/x86/config.mk
> > +++ b/arch/x86/config.mk
> > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> >  PLATFORM_CPPFLAGS += -march=i386 -m32
> >  else
> >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > +
> > +ifndef CONFIG_X86_HARDFP
> >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> >  endif
> >
> > +endif # IS_32BIT
> > +
> >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> >
> >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > index 2647bff891f8..5ea746ecce4d 100644
> > --- a/arch/x86/cpu/x86_64/cpu.c
> > +++ b/arch/x86/cpu/x86_64/cpu.c
> > @@ -10,6 +10,7 @@
> >  #include <init.h>
> >  #include <asm/cpu.h>
> >  #include <asm/global_data.h>
> > +#include <asm/processor-flags.h>
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> >         return 0;
> >  }
> >
> > +/* enable SSE features for hardware floating point */
> > +static void setup_sse_features(void)
> > +{
> > +       asm ("mov %%cr4, %%rax\n" \
> > +       "or  %0, %%rax\n" \
> > +       "mov %%rax, %%cr4\n" \
> > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > +}
> > +
> >  int x86_cpu_reinit_f(void)
> >  {
> >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> >         gd->arch.has_mtrr = true;
> > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > +               setup_sse_features();
> >
> >         return 0;
> >  }
> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index 6f319ba0d544..39c82521be16 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> >
> >  config CONSOLE_TRUETYPE
> >         bool "Support a console that uses TrueType fonts"
> > +       select X86_HARDFP if X86
>
> This should be "depends on HARDFP", indicating that the TrueType
> library is using hardware fp itself, and user has to explicitly turn
> the hardware fp Kconfig option on.

So you mean 'depends on HARDFP if X86'  ? After all, this is only for
X86 - other archs can use softfp which is already enabled, as I
understand it.

>
> "Select" does not work for architectures that does not have the
> "enabling hardware fp" logic in place.
>
> >         help
> >           TrueTrype fonts can provide outline-drawing capability rather than
> >           needing to provide a bitmap for each font and size that is needed.
> > --

I still don't think we are on the same page here. I would prefer to
just enable the options without any option. I really don't want to get
into RISC-V stuff - that is a separate concern.

From my POV it seems that x86 is special in that:
- it uses hardfp
- hardfp is always available in any CPU with 64-bit support (I think?)

So please can you be a bit more specific here?

Regards,
Simon

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Tom Rini]

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

On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> Hi Bin,
> 
> On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > This is needed to support Truetype fonts. In any case, the compiler
> > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > SSE so that hardware floating-point arithmetic works.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > ---
> > >
> > > Changes in v4:
> > > - Use a Kconfig option
> > >
> > >  arch/x86/Kconfig          |  8 ++++++++
> > >  arch/x86/config.mk        |  4 ++++
> > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > >  drivers/video/Kconfig     |  1 +
> > >  4 files changed, 25 insertions(+)
> > >
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index 99e59d94c606..6b532d712ee8 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > >         hex
> > >         default 0x10000
> > >
> > > +config X86_HARDFP
> > > +       bool "Support hardware floating point"
> > > +       help
> > > +         U-Boot generally does not make use of floating point. Where this is
> > > +         needed, it can be enabled using this option. This adjusts the
> > > +         start-up code for 64-bit mode and changes the compiler options for
> > > +         64-bit to enable SSE.
> >
> > As discussed in another thread, this option should be made global to
> > all architectures and by default no.
> >
> > > +
> > >  config HAVE_ITSS
> > >         bool "Enable ITSS"
> > >         help
> > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > --- a/arch/x86/config.mk
> > > +++ b/arch/x86/config.mk
> > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > >  else
> > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > +
> > > +ifndef CONFIG_X86_HARDFP
> > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > >  endif
> > >
> > > +endif # IS_32BIT
> > > +
> > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > >
> > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > index 2647bff891f8..5ea746ecce4d 100644
> > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > @@ -10,6 +10,7 @@
> > >  #include <init.h>
> > >  #include <asm/cpu.h>
> > >  #include <asm/global_data.h>
> > > +#include <asm/processor-flags.h>
> > >
> > >  DECLARE_GLOBAL_DATA_PTR;
> > >
> > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > >         return 0;
> > >  }
> > >
> > > +/* enable SSE features for hardware floating point */
> > > +static void setup_sse_features(void)
> > > +{
> > > +       asm ("mov %%cr4, %%rax\n" \
> > > +       "or  %0, %%rax\n" \
> > > +       "mov %%rax, %%cr4\n" \
> > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > +}
> > > +
> > >  int x86_cpu_reinit_f(void)
> > >  {
> > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > >         gd->arch.has_mtrr = true;
> > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > +               setup_sse_features();
> > >
> > >         return 0;
> > >  }
> > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > index 6f319ba0d544..39c82521be16 100644
> > > --- a/drivers/video/Kconfig
> > > +++ b/drivers/video/Kconfig
> > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > >
> > >  config CONSOLE_TRUETYPE
> > >         bool "Support a console that uses TrueType fonts"
> > > +       select X86_HARDFP if X86
> >
> > This should be "depends on HARDFP", indicating that the TrueType
> > library is using hardware fp itself, and user has to explicitly turn
> > the hardware fp Kconfig option on.
> 
> So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> X86 - other archs can use softfp which is already enabled, as I
> understand it.
> 
> >
> > "Select" does not work for architectures that does not have the
> > "enabling hardware fp" logic in place.
> >
> > >         help
> > >           TrueTrype fonts can provide outline-drawing capability rather than
> > >           needing to provide a bitmap for each font and size that is needed.
> > > --
> 
> I still don't think we are on the same page here. I would prefer to
> just enable the options without any option. I really don't want to get
> into RISC-V stuff - that is a separate concern.
> 
> From my POV it seems that x86 is special in that:
> - it uses hardfp
> - hardfp is always available in any CPU with 64-bit support (I think?)

Maybe the issue even is that on x86 we're being too imprecise in our
build rules (and also on RISC-V, another issue). Today on x86 this fails
because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
turn that on, on all x86 targets today and things build. Would that not
also fix the truetype issue?

-- 
Tom

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

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Bin Meng]

Hi Tom,

On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > Hi Bin,
> >
> > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > SSE so that hardware floating-point arithmetic works.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > ---
> > > >
> > > > Changes in v4:
> > > > - Use a Kconfig option
> > > >
> > > >  arch/x86/Kconfig          |  8 ++++++++
> > > >  arch/x86/config.mk        |  4 ++++
> > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > >  drivers/video/Kconfig     |  1 +
> > > >  4 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > index 99e59d94c606..6b532d712ee8 100644
> > > > --- a/arch/x86/Kconfig
> > > > +++ b/arch/x86/Kconfig
> > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > >         hex
> > > >         default 0x10000
> > > >
> > > > +config X86_HARDFP
> > > > +       bool "Support hardware floating point"
> > > > +       help
> > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > +         needed, it can be enabled using this option. This adjusts the
> > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > +         64-bit to enable SSE.
> > >
> > > As discussed in another thread, this option should be made global to
> > > all architectures and by default no.
> > >
> > > > +
> > > >  config HAVE_ITSS
> > > >         bool "Enable ITSS"
> > > >         help
> > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > --- a/arch/x86/config.mk
> > > > +++ b/arch/x86/config.mk
> > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > >  else
> > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > +
> > > > +ifndef CONFIG_X86_HARDFP
> > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > >  endif
> > > >
> > > > +endif # IS_32BIT
> > > > +
> > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > >
> > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include <init.h>
> > > >  #include <asm/cpu.h>
> > > >  #include <asm/global_data.h>
> > > > +#include <asm/processor-flags.h>
> > > >
> > > >  DECLARE_GLOBAL_DATA_PTR;
> > > >
> > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > >         return 0;
> > > >  }
> > > >
> > > > +/* enable SSE features for hardware floating point */
> > > > +static void setup_sse_features(void)
> > > > +{
> > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > +       "or  %0, %%rax\n" \
> > > > +       "mov %%rax, %%cr4\n" \
> > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > +}
> > > > +
> > > >  int x86_cpu_reinit_f(void)
> > > >  {
> > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > >         gd->arch.has_mtrr = true;
> > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > +               setup_sse_features();
> > > >
> > > >         return 0;
> > > >  }
> > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > index 6f319ba0d544..39c82521be16 100644
> > > > --- a/drivers/video/Kconfig
> > > > +++ b/drivers/video/Kconfig
> > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > >
> > > >  config CONSOLE_TRUETYPE
> > > >         bool "Support a console that uses TrueType fonts"
> > > > +       select X86_HARDFP if X86
> > >
> > > This should be "depends on HARDFP", indicating that the TrueType
> > > library is using hardware fp itself, and user has to explicitly turn
> > > the hardware fp Kconfig option on.
> >
> > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > X86 - other archs can use softfp which is already enabled, as I
> > understand it.
> >
> > >
> > > "Select" does not work for architectures that does not have the
> > > "enabling hardware fp" logic in place.
> > >
> > > >         help
> > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > >           needing to provide a bitmap for each font and size that is needed.
> > > > --
> >
> > I still don't think we are on the same page here. I would prefer to
> > just enable the options without any option. I really don't want to get
> > into RISC-V stuff - that is a separate concern.
> >
> > From my POV it seems that x86 is special in that:
> > - it uses hardfp
> > - hardfp is always available in any CPU with 64-bit support (I think?)
>
> Maybe the issue even is that on x86 we're being too imprecise in our
> build rules (and also on RISC-V, another issue). Today on x86 this fails
> because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> turn that on, on all x86 targets today and things build. Would that not
> also fix the truetype issue?

One can easily turn on compiler flags for x86 (and for RISC-V too) to
tell the compiler to generate floating point instructions if it sees
fit.

However on x86 and RISC-V there are configurations needed to program
the CPU registers to turn on the hardware FP, otherwise an exception
will be generated.

Regards,
Bin

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Tom Rini]

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

On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote:
> Hi Tom,
> 
> On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > > Hi Bin,
> > >
> > > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > > SSE so that hardware floating-point arithmetic works.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > ---
> > > > >
> > > > > Changes in v4:
> > > > > - Use a Kconfig option
> > > > >
> > > > >  arch/x86/Kconfig          |  8 ++++++++
> > > > >  arch/x86/config.mk        |  4 ++++
> > > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > > >  drivers/video/Kconfig     |  1 +
> > > > >  4 files changed, 25 insertions(+)
> > > > >
> > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > index 99e59d94c606..6b532d712ee8 100644
> > > > > --- a/arch/x86/Kconfig
> > > > > +++ b/arch/x86/Kconfig
> > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > > >         hex
> > > > >         default 0x10000
> > > > >
> > > > > +config X86_HARDFP
> > > > > +       bool "Support hardware floating point"
> > > > > +       help
> > > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > > +         needed, it can be enabled using this option. This adjusts the
> > > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > > +         64-bit to enable SSE.
> > > >
> > > > As discussed in another thread, this option should be made global to
> > > > all architectures and by default no.
> > > >
> > > > > +
> > > > >  config HAVE_ITSS
> > > > >         bool "Enable ITSS"
> > > > >         help
> > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > > --- a/arch/x86/config.mk
> > > > > +++ b/arch/x86/config.mk
> > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > > >  else
> > > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > > +
> > > > > +ifndef CONFIG_X86_HARDFP
> > > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > > >  endif
> > > > >
> > > > > +endif # IS_32BIT
> > > > > +
> > > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > > >
> > > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > > @@ -10,6 +10,7 @@
> > > > >  #include <init.h>
> > > > >  #include <asm/cpu.h>
> > > > >  #include <asm/global_data.h>
> > > > > +#include <asm/processor-flags.h>
> > > > >
> > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > >
> > > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +/* enable SSE features for hardware floating point */
> > > > > +static void setup_sse_features(void)
> > > > > +{
> > > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > > +       "or  %0, %%rax\n" \
> > > > > +       "mov %%rax, %%cr4\n" \
> > > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > > +}
> > > > > +
> > > > >  int x86_cpu_reinit_f(void)
> > > > >  {
> > > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > > >         gd->arch.has_mtrr = true;
> > > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > > +               setup_sse_features();
> > > > >
> > > > >         return 0;
> > > > >  }
> > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > index 6f319ba0d544..39c82521be16 100644
> > > > > --- a/drivers/video/Kconfig
> > > > > +++ b/drivers/video/Kconfig
> > > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > > >
> > > > >  config CONSOLE_TRUETYPE
> > > > >         bool "Support a console that uses TrueType fonts"
> > > > > +       select X86_HARDFP if X86
> > > >
> > > > This should be "depends on HARDFP", indicating that the TrueType
> > > > library is using hardware fp itself, and user has to explicitly turn
> > > > the hardware fp Kconfig option on.
> > >
> > > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > > X86 - other archs can use softfp which is already enabled, as I
> > > understand it.
> > >
> > > >
> > > > "Select" does not work for architectures that does not have the
> > > > "enabling hardware fp" logic in place.
> > > >
> > > > >         help
> > > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > > >           needing to provide a bitmap for each font and size that is needed.
> > > > > --
> > >
> > > I still don't think we are on the same page here. I would prefer to
> > > just enable the options without any option. I really don't want to get
> > > into RISC-V stuff - that is a separate concern.
> > >
> > > From my POV it seems that x86 is special in that:
> > > - it uses hardfp
> > > - hardfp is always available in any CPU with 64-bit support (I think?)
> >
> > Maybe the issue even is that on x86 we're being too imprecise in our
> > build rules (and also on RISC-V, another issue). Today on x86 this fails
> > because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> > turn that on, on all x86 targets today and things build. Would that not
> > also fix the truetype issue?
> 
> One can easily turn on compiler flags for x86 (and for RISC-V too) to
> tell the compiler to generate floating point instructions if it sees
> fit.
> 
> However on x86 and RISC-V there are configurations needed to program
> the CPU registers to turn on the hardware FP, otherwise an exception
> will be generated.

Right, which is why I'm saying why don't we just use -msoft-float
instead, so that we don't have to worry about enabling features (and
also additional registers on the stack yes?) ?

-- 
Tom

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

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Bin Meng]

Hi Tom,

On Tue, Nov 14, 2023 at 7:52 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote:
> > Hi Tom,
> >
> > On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > > > Hi Bin,
> > > >
> > > > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > > > SSE so that hardware floating-point arithmetic works.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > > ---
> > > > > >
> > > > > > Changes in v4:
> > > > > > - Use a Kconfig option
> > > > > >
> > > > > >  arch/x86/Kconfig          |  8 ++++++++
> > > > > >  arch/x86/config.mk        |  4 ++++
> > > > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > > > >  drivers/video/Kconfig     |  1 +
> > > > > >  4 files changed, 25 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > > index 99e59d94c606..6b532d712ee8 100644
> > > > > > --- a/arch/x86/Kconfig
> > > > > > +++ b/arch/x86/Kconfig
> > > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > > > >         hex
> > > > > >         default 0x10000
> > > > > >
> > > > > > +config X86_HARDFP
> > > > > > +       bool "Support hardware floating point"
> > > > > > +       help
> > > > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > > > +         needed, it can be enabled using this option. This adjusts the
> > > > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > > > +         64-bit to enable SSE.
> > > > >
> > > > > As discussed in another thread, this option should be made global to
> > > > > all architectures and by default no.
> > > > >
> > > > > > +
> > > > > >  config HAVE_ITSS
> > > > > >         bool "Enable ITSS"
> > > > > >         help
> > > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > > > --- a/arch/x86/config.mk
> > > > > > +++ b/arch/x86/config.mk
> > > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > > > >  else
> > > > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > > > +
> > > > > > +ifndef CONFIG_X86_HARDFP
> > > > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > > > >  endif
> > > > > >
> > > > > > +endif # IS_32BIT
> > > > > > +
> > > > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > > > >
> > > > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > > > @@ -10,6 +10,7 @@
> > > > > >  #include <init.h>
> > > > > >  #include <asm/cpu.h>
> > > > > >  #include <asm/global_data.h>
> > > > > > +#include <asm/processor-flags.h>
> > > > > >
> > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > >
> > > > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +/* enable SSE features for hardware floating point */
> > > > > > +static void setup_sse_features(void)
> > > > > > +{
> > > > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > > > +       "or  %0, %%rax\n" \
> > > > > > +       "mov %%rax, %%cr4\n" \
> > > > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > > > +}
> > > > > > +
> > > > > >  int x86_cpu_reinit_f(void)
> > > > > >  {
> > > > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > > > >         gd->arch.has_mtrr = true;
> > > > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > > > +               setup_sse_features();
> > > > > >
> > > > > >         return 0;
> > > > > >  }
> > > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > > index 6f319ba0d544..39c82521be16 100644
> > > > > > --- a/drivers/video/Kconfig
> > > > > > +++ b/drivers/video/Kconfig
> > > > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > > > >
> > > > > >  config CONSOLE_TRUETYPE
> > > > > >         bool "Support a console that uses TrueType fonts"
> > > > > > +       select X86_HARDFP if X86
> > > > >
> > > > > This should be "depends on HARDFP", indicating that the TrueType
> > > > > library is using hardware fp itself, and user has to explicitly turn
> > > > > the hardware fp Kconfig option on.
> > > >
> > > > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > > > X86 - other archs can use softfp which is already enabled, as I
> > > > understand it.
> > > >
> > > > >
> > > > > "Select" does not work for architectures that does not have the
> > > > > "enabling hardware fp" logic in place.
> > > > >
> > > > > >         help
> > > > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > > > >           needing to provide a bitmap for each font and size that is needed.
> > > > > > --
> > > >
> > > > I still don't think we are on the same page here. I would prefer to
> > > > just enable the options without any option. I really don't want to get
> > > > into RISC-V stuff - that is a separate concern.
> > > >
> > > > From my POV it seems that x86 is special in that:
> > > > - it uses hardfp
> > > > - hardfp is always available in any CPU with 64-bit support (I think?)
> > >
> > > Maybe the issue even is that on x86 we're being too imprecise in our
> > > build rules (and also on RISC-V, another issue). Today on x86 this fails
> > > because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> > > turn that on, on all x86 targets today and things build. Would that not
> > > also fix the truetype issue?
> >
> > One can easily turn on compiler flags for x86 (and for RISC-V too) to
> > tell the compiler to generate floating point instructions if it sees
> > fit.
> >
> > However on x86 and RISC-V there are configurations needed to program
> > the CPU registers to turn on the hardware FP, otherwise an exception
> > will be generated.
>
> Right, which is why I'm saying why don't we just use -msoft-float
> instead, so that we don't have to worry about enabling features (and
> also additional registers on the stack yes?) ?

Yes, we should be using -msoft-float for all architectures by default
if the compiler supports that on each arch. IIRC, the RISC-V back-end
didn't support that years ago but things may change recently.

Regards,
Bin

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Tom Rini]

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

On Tue, Nov 14, 2023 at 09:49:08AM +0800, Bin Meng wrote:
> Hi Tom,
> 
> On Tue, Nov 14, 2023 at 7:52 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote:
> > > Hi Tom,
> > >
> > > On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > > > > Hi Bin,
> > > > >
> > > > > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > > > > SSE so that hardware floating-point arithmetic works.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v4:
> > > > > > > - Use a Kconfig option
> > > > > > >
> > > > > > >  arch/x86/Kconfig          |  8 ++++++++
> > > > > > >  arch/x86/config.mk        |  4 ++++
> > > > > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > > > > >  drivers/video/Kconfig     |  1 +
> > > > > > >  4 files changed, 25 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > > > index 99e59d94c606..6b532d712ee8 100644
> > > > > > > --- a/arch/x86/Kconfig
> > > > > > > +++ b/arch/x86/Kconfig
> > > > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > > > > >         hex
> > > > > > >         default 0x10000
> > > > > > >
> > > > > > > +config X86_HARDFP
> > > > > > > +       bool "Support hardware floating point"
> > > > > > > +       help
> > > > > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > > > > +         needed, it can be enabled using this option. This adjusts the
> > > > > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > > > > +         64-bit to enable SSE.
> > > > > >
> > > > > > As discussed in another thread, this option should be made global to
> > > > > > all architectures and by default no.
> > > > > >
> > > > > > > +
> > > > > > >  config HAVE_ITSS
> > > > > > >         bool "Enable ITSS"
> > > > > > >         help
> > > > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > > > > --- a/arch/x86/config.mk
> > > > > > > +++ b/arch/x86/config.mk
> > > > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > > > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > > > > >  else
> > > > > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > > > > +
> > > > > > > +ifndef CONFIG_X86_HARDFP
> > > > > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > > > > >  endif
> > > > > > >
> > > > > > > +endif # IS_32BIT
> > > > > > > +
> > > > > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > > > > >
> > > > > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > @@ -10,6 +10,7 @@
> > > > > > >  #include <init.h>
> > > > > > >  #include <asm/cpu.h>
> > > > > > >  #include <asm/global_data.h>
> > > > > > > +#include <asm/processor-flags.h>
> > > > > > >
> > > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > > >
> > > > > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +/* enable SSE features for hardware floating point */
> > > > > > > +static void setup_sse_features(void)
> > > > > > > +{
> > > > > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > > > > +       "or  %0, %%rax\n" \
> > > > > > > +       "mov %%rax, %%cr4\n" \
> > > > > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > > > > +}
> > > > > > > +
> > > > > > >  int x86_cpu_reinit_f(void)
> > > > > > >  {
> > > > > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > > > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > > > > >         gd->arch.has_mtrr = true;
> > > > > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > > > > +               setup_sse_features();
> > > > > > >
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > > > index 6f319ba0d544..39c82521be16 100644
> > > > > > > --- a/drivers/video/Kconfig
> > > > > > > +++ b/drivers/video/Kconfig
> > > > > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > > > > >
> > > > > > >  config CONSOLE_TRUETYPE
> > > > > > >         bool "Support a console that uses TrueType fonts"
> > > > > > > +       select X86_HARDFP if X86
> > > > > >
> > > > > > This should be "depends on HARDFP", indicating that the TrueType
> > > > > > library is using hardware fp itself, and user has to explicitly turn
> > > > > > the hardware fp Kconfig option on.
> > > > >
> > > > > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > > > > X86 - other archs can use softfp which is already enabled, as I
> > > > > understand it.
> > > > >
> > > > > >
> > > > > > "Select" does not work for architectures that does not have the
> > > > > > "enabling hardware fp" logic in place.
> > > > > >
> > > > > > >         help
> > > > > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > > > > >           needing to provide a bitmap for each font and size that is needed.
> > > > > > > --
> > > > >
> > > > > I still don't think we are on the same page here. I would prefer to
> > > > > just enable the options without any option. I really don't want to get
> > > > > into RISC-V stuff - that is a separate concern.
> > > > >
> > > > > From my POV it seems that x86 is special in that:
> > > > > - it uses hardfp
> > > > > - hardfp is always available in any CPU with 64-bit support (I think?)
> > > >
> > > > Maybe the issue even is that on x86 we're being too imprecise in our
> > > > build rules (and also on RISC-V, another issue). Today on x86 this fails
> > > > because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> > > > turn that on, on all x86 targets today and things build. Would that not
> > > > also fix the truetype issue?
> > >
> > > One can easily turn on compiler flags for x86 (and for RISC-V too) to
> > > tell the compiler to generate floating point instructions if it sees
> > > fit.
> > >
> > > However on x86 and RISC-V there are configurations needed to program
> > > the CPU registers to turn on the hardware FP, otherwise an exception
> > > will be generated.
> >
> > Right, which is why I'm saying why don't we just use -msoft-float
> > instead, so that we don't have to worry about enabling features (and
> > also additional registers on the stack yes?) ?
> 
> Yes, we should be using -msoft-float for all architectures by default
> if the compiler supports that on each arch. IIRC, the RISC-V back-end
> didn't support that years ago but things may change recently.

OK, so for this series, lets please just simplify the logic in
arch/x86/config.mk (and do some boot testing too of course) to
-msoft-float everyone, and then the fonts should also be working and we
don't have to deal with some other details as well, yes? And having said
that, just for sanity sake keep a stopwatch nearby and do some normal
functional tests too, to make sure we don't suddenly speed-regress?

-- 
Tom

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

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Bin Meng]

Hi Tom,

On Wed, Nov 15, 2023 at 12:22 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Nov 14, 2023 at 09:49:08AM +0800, Bin Meng wrote:
> > Hi Tom,
> >
> > On Tue, Nov 14, 2023 at 7:52 AM Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > > > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > > > > > SSE so that hardware floating-point arithmetic works.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes in v4:
> > > > > > > > - Use a Kconfig option
> > > > > > > >
> > > > > > > >  arch/x86/Kconfig          |  8 ++++++++
> > > > > > > >  arch/x86/config.mk        |  4 ++++
> > > > > > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > > > > > >  drivers/video/Kconfig     |  1 +
> > > > > > > >  4 files changed, 25 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > > > > index 99e59d94c606..6b532d712ee8 100644
> > > > > > > > --- a/arch/x86/Kconfig
> > > > > > > > +++ b/arch/x86/Kconfig
> > > > > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > > > > > >         hex
> > > > > > > >         default 0x10000
> > > > > > > >
> > > > > > > > +config X86_HARDFP
> > > > > > > > +       bool "Support hardware floating point"
> > > > > > > > +       help
> > > > > > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > > > > > +         needed, it can be enabled using this option. This adjusts the
> > > > > > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > > > > > +         64-bit to enable SSE.
> > > > > > >
> > > > > > > As discussed in another thread, this option should be made global to
> > > > > > > all architectures and by default no.
> > > > > > >
> > > > > > > > +
> > > > > > > >  config HAVE_ITSS
> > > > > > > >         bool "Enable ITSS"
> > > > > > > >         help
> > > > > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > > > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > > > > > --- a/arch/x86/config.mk
> > > > > > > > +++ b/arch/x86/config.mk
> > > > > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > > > > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > > > > > >  else
> > > > > > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > > > > > +
> > > > > > > > +ifndef CONFIG_X86_HARDFP
> > > > > > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > > > > > >  endif
> > > > > > > >
> > > > > > > > +endif # IS_32BIT
> > > > > > > > +
> > > > > > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > > > > > >
> > > > > > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > > > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > > > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > >  #include <init.h>
> > > > > > > >  #include <asm/cpu.h>
> > > > > > > >  #include <asm/global_data.h>
> > > > > > > > +#include <asm/processor-flags.h>
> > > > > > > >
> > > > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > > > >
> > > > > > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +/* enable SSE features for hardware floating point */
> > > > > > > > +static void setup_sse_features(void)
> > > > > > > > +{
> > > > > > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > > > > > +       "or  %0, %%rax\n" \
> > > > > > > > +       "mov %%rax, %%cr4\n" \
> > > > > > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  int x86_cpu_reinit_f(void)
> > > > > > > >  {
> > > > > > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > > > > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > > > > > >         gd->arch.has_mtrr = true;
> > > > > > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > > > > > +               setup_sse_features();
> > > > > > > >
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > > > > index 6f319ba0d544..39c82521be16 100644
> > > > > > > > --- a/drivers/video/Kconfig
> > > > > > > > +++ b/drivers/video/Kconfig
> > > > > > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > > > > > >
> > > > > > > >  config CONSOLE_TRUETYPE
> > > > > > > >         bool "Support a console that uses TrueType fonts"
> > > > > > > > +       select X86_HARDFP if X86
> > > > > > >
> > > > > > > This should be "depends on HARDFP", indicating that the TrueType
> > > > > > > library is using hardware fp itself, and user has to explicitly turn
> > > > > > > the hardware fp Kconfig option on.
> > > > > >
> > > > > > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > > > > > X86 - other archs can use softfp which is already enabled, as I
> > > > > > understand it.
> > > > > >
> > > > > > >
> > > > > > > "Select" does not work for architectures that does not have the
> > > > > > > "enabling hardware fp" logic in place.
> > > > > > >
> > > > > > > >         help
> > > > > > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > > > > > >           needing to provide a bitmap for each font and size that is needed.
> > > > > > > > --
> > > > > >
> > > > > > I still don't think we are on the same page here. I would prefer to
> > > > > > just enable the options without any option. I really don't want to get
> > > > > > into RISC-V stuff - that is a separate concern.
> > > > > >
> > > > > > From my POV it seems that x86 is special in that:
> > > > > > - it uses hardfp
> > > > > > - hardfp is always available in any CPU with 64-bit support (I think?)
> > > > >
> > > > > Maybe the issue even is that on x86 we're being too imprecise in our
> > > > > build rules (and also on RISC-V, another issue). Today on x86 this fails
> > > > > because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> > > > > turn that on, on all x86 targets today and things build. Would that not
> > > > > also fix the truetype issue?
> > > >
> > > > One can easily turn on compiler flags for x86 (and for RISC-V too) to
> > > > tell the compiler to generate floating point instructions if it sees
> > > > fit.
> > > >
> > > > However on x86 and RISC-V there are configurations needed to program
> > > > the CPU registers to turn on the hardware FP, otherwise an exception
> > > > will be generated.
> > >
> > > Right, which is why I'm saying why don't we just use -msoft-float
> > > instead, so that we don't have to worry about enabling features (and
> > > also additional registers on the stack yes?) ?
> >
> > Yes, we should be using -msoft-float for all architectures by default
> > if the compiler supports that on each arch. IIRC, the RISC-V back-end
> > didn't support that years ago but things may change recently.
>
> OK, so for this series, lets please just simplify the logic in
> arch/x86/config.mk (and do some boot testing too of course) to
> -msoft-float everyone, and then the fonts should also be working and we
> don't have to deal with some other details as well, yes? And having said
> that, just for sanity sake keep a stopwatch nearby and do some normal
> functional tests too, to make sure we don't suddenly speed-regress?

To make fonts work with -msoft-float for everyone, we need U-Boot to
link with the compiler intrinsics library (e.g.: libgcc, or
compler-rt). As of today some architectures choose a private libgcc
implementation within U-Boot.

Regards,
Bin

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Simon Glass]

Hi,

On Tue, 14 Nov 2023 at 17:44, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Tom,
>
> On Wed, Nov 15, 2023 at 12:22 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Nov 14, 2023 at 09:49:08AM +0800, Bin Meng wrote:
> > > Hi Tom,
> > >
> > > On Tue, Nov 14, 2023 at 7:52 AM Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > > > > > > Hi Bin,
> > > > > > >
> > > > > > > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > > > > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > > > > > > SSE so that hardware floating-point arithmetic works.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes in v4:
> > > > > > > > > - Use a Kconfig option
> > > > > > > > >
> > > > > > > > >  arch/x86/Kconfig          |  8 ++++++++
> > > > > > > > >  arch/x86/config.mk        |  4 ++++
> > > > > > > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > > > > > > >  drivers/video/Kconfig     |  1 +
> > > > > > > > >  4 files changed, 25 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > > > > > index 99e59d94c606..6b532d712ee8 100644
> > > > > > > > > --- a/arch/x86/Kconfig
> > > > > > > > > +++ b/arch/x86/Kconfig
> > > > > > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > > > > > > >         hex
> > > > > > > > >         default 0x10000
> > > > > > > > >
> > > > > > > > > +config X86_HARDFP
> > > > > > > > > +       bool "Support hardware floating point"
> > > > > > > > > +       help
> > > > > > > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > > > > > > +         needed, it can be enabled using this option. This adjusts the
> > > > > > > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > > > > > > +         64-bit to enable SSE.
> > > > > > > >
> > > > > > > > As discussed in another thread, this option should be made global to
> > > > > > > > all architectures and by default no.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > >  config HAVE_ITSS
> > > > > > > > >         bool "Enable ITSS"
> > > > > > > > >         help
> > > > > > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > > > > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > > > > > > --- a/arch/x86/config.mk
> > > > > > > > > +++ b/arch/x86/config.mk
> > > > > > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > > > > > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > > > > > > >  else
> > > > > > > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > > > > > > +
> > > > > > > > > +ifndef CONFIG_X86_HARDFP
> > > > > > > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > > > > > > >  endif
> > > > > > > > >
> > > > > > > > > +endif # IS_32BIT
> > > > > > > > > +
> > > > > > > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > > > > > > >
> > > > > > > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > > > > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > > > > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > > >  #include <init.h>
> > > > > > > > >  #include <asm/cpu.h>
> > > > > > > > >  #include <asm/global_data.h>
> > > > > > > > > +#include <asm/processor-flags.h>
> > > > > > > > >
> > > > > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > > > > >
> > > > > > > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > > > > > > >         return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +/* enable SSE features for hardware floating point */
> > > > > > > > > +static void setup_sse_features(void)
> > > > > > > > > +{
> > > > > > > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > > > > > > +       "or  %0, %%rax\n" \
> > > > > > > > > +       "mov %%rax, %%cr4\n" \
> > > > > > > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  int x86_cpu_reinit_f(void)
> > > > > > > > >  {
> > > > > > > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > > > > > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > > > > > > >         gd->arch.has_mtrr = true;
> > > > > > > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > > > > > > +               setup_sse_features();
> > > > > > > > >
> > > > > > > > >         return 0;
> > > > > > > > >  }
> > > > > > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > > > > > index 6f319ba0d544..39c82521be16 100644
> > > > > > > > > --- a/drivers/video/Kconfig
> > > > > > > > > +++ b/drivers/video/Kconfig
> > > > > > > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > > > > > > >
> > > > > > > > >  config CONSOLE_TRUETYPE
> > > > > > > > >         bool "Support a console that uses TrueType fonts"
> > > > > > > > > +       select X86_HARDFP if X86
> > > > > > > >
> > > > > > > > This should be "depends on HARDFP", indicating that the TrueType
> > > > > > > > library is using hardware fp itself, and user has to explicitly turn
> > > > > > > > the hardware fp Kconfig option on.
> > > > > > >
> > > > > > > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > > > > > > X86 - other archs can use softfp which is already enabled, as I
> > > > > > > understand it.
> > > > > > >
> > > > > > > >
> > > > > > > > "Select" does not work for architectures that does not have the
> > > > > > > > "enabling hardware fp" logic in place.
> > > > > > > >
> > > > > > > > >         help
> > > > > > > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > > > > > > >           needing to provide a bitmap for each font and size that is needed.
> > > > > > > > > --
> > > > > > >
> > > > > > > I still don't think we are on the same page here. I would prefer to
> > > > > > > just enable the options without any option. I really don't want to get
> > > > > > > into RISC-V stuff - that is a separate concern.
> > > > > > >
> > > > > > > From my POV it seems that x86 is special in that:
> > > > > > > - it uses hardfp
> > > > > > > - hardfp is always available in any CPU with 64-bit support (I think?)
> > > > > >
> > > > > > Maybe the issue even is that on x86 we're being too imprecise in our
> > > > > > build rules (and also on RISC-V, another issue). Today on x86 this fails
> > > > > > because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> > > > > > turn that on, on all x86 targets today and things build. Would that not
> > > > > > also fix the truetype issue?
> > > > >
> > > > > One can easily turn on compiler flags for x86 (and for RISC-V too) to
> > > > > tell the compiler to generate floating point instructions if it sees
> > > > > fit.
> > > > >
> > > > > However on x86 and RISC-V there are configurations needed to program
> > > > > the CPU registers to turn on the hardware FP, otherwise an exception
> > > > > will be generated.
> > > >
> > > > Right, which is why I'm saying why don't we just use -msoft-float
> > > > instead, so that we don't have to worry about enabling features (and
> > > > also additional registers on the stack yes?) ?
> > >
> > > Yes, we should be using -msoft-float for all architectures by default
> > > if the compiler supports that on each arch. IIRC, the RISC-V back-end
> > > didn't support that years ago but things may change recently.
> >
> > OK, so for this series, lets please just simplify the logic in
> > arch/x86/config.mk (and do some boot testing too of course) to
> > -msoft-float everyone, and then the fonts should also be working and we
> > don't have to deal with some other details as well, yes? And having said
> > that, just for sanity sake keep a stopwatch nearby and do some normal
> > functional tests too, to make sure we don't suddenly speed-regress?
>
> To make fonts work with -msoft-float for everyone, we need U-Boot to
> link with the compiler intrinsics library (e.g.: libgcc, or
> compler-rt). As of today some architectures choose a private libgcc
> implementation within U-Boot.

I thought I mentioned this but softfp did not work for me on x86 and
my limited research suggests it is experimental / not used.

So look, I have a fairly trivial patch here. Perhaps we should just
apply it and worry about RISC-V when needed?

Regards,
Simon

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Tom Rini]

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

On Wed, Nov 15, 2023 at 08:44:22AM +0800, Bin Meng wrote:
> Hi Tom,
> 
> On Wed, Nov 15, 2023 at 12:22 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Nov 14, 2023 at 09:49:08AM +0800, Bin Meng wrote:
> > > Hi Tom,
> > >
> > > On Tue, Nov 14, 2023 at 7:52 AM Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > > > > > > Hi Bin,
> > > > > > >
> > > > > > > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > > > > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > > > > > > SSE so that hardware floating-point arithmetic works.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes in v4:
> > > > > > > > > - Use a Kconfig option
> > > > > > > > >
> > > > > > > > >  arch/x86/Kconfig          |  8 ++++++++
> > > > > > > > >  arch/x86/config.mk        |  4 ++++
> > > > > > > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > > > > > > >  drivers/video/Kconfig     |  1 +
> > > > > > > > >  4 files changed, 25 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > > > > > index 99e59d94c606..6b532d712ee8 100644
> > > > > > > > > --- a/arch/x86/Kconfig
> > > > > > > > > +++ b/arch/x86/Kconfig
> > > > > > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > > > > > > >         hex
> > > > > > > > >         default 0x10000
> > > > > > > > >
> > > > > > > > > +config X86_HARDFP
> > > > > > > > > +       bool "Support hardware floating point"
> > > > > > > > > +       help
> > > > > > > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > > > > > > +         needed, it can be enabled using this option. This adjusts the
> > > > > > > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > > > > > > +         64-bit to enable SSE.
> > > > > > > >
> > > > > > > > As discussed in another thread, this option should be made global to
> > > > > > > > all architectures and by default no.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > >  config HAVE_ITSS
> > > > > > > > >         bool "Enable ITSS"
> > > > > > > > >         help
> > > > > > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > > > > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > > > > > > --- a/arch/x86/config.mk
> > > > > > > > > +++ b/arch/x86/config.mk
> > > > > > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > > > > > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > > > > > > >  else
> > > > > > > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > > > > > > +
> > > > > > > > > +ifndef CONFIG_X86_HARDFP
> > > > > > > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > > > > > > >  endif
> > > > > > > > >
> > > > > > > > > +endif # IS_32BIT
> > > > > > > > > +
> > > > > > > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > > > > > > >
> > > > > > > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > > > > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > > > > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > > >  #include <init.h>
> > > > > > > > >  #include <asm/cpu.h>
> > > > > > > > >  #include <asm/global_data.h>
> > > > > > > > > +#include <asm/processor-flags.h>
> > > > > > > > >
> > > > > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > > > > >
> > > > > > > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > > > > > > >         return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +/* enable SSE features for hardware floating point */
> > > > > > > > > +static void setup_sse_features(void)
> > > > > > > > > +{
> > > > > > > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > > > > > > +       "or  %0, %%rax\n" \
> > > > > > > > > +       "mov %%rax, %%cr4\n" \
> > > > > > > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  int x86_cpu_reinit_f(void)
> > > > > > > > >  {
> > > > > > > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > > > > > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > > > > > > >         gd->arch.has_mtrr = true;
> > > > > > > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > > > > > > +               setup_sse_features();
> > > > > > > > >
> > > > > > > > >         return 0;
> > > > > > > > >  }
> > > > > > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > > > > > index 6f319ba0d544..39c82521be16 100644
> > > > > > > > > --- a/drivers/video/Kconfig
> > > > > > > > > +++ b/drivers/video/Kconfig
> > > > > > > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > > > > > > >
> > > > > > > > >  config CONSOLE_TRUETYPE
> > > > > > > > >         bool "Support a console that uses TrueType fonts"
> > > > > > > > > +       select X86_HARDFP if X86
> > > > > > > >
> > > > > > > > This should be "depends on HARDFP", indicating that the TrueType
> > > > > > > > library is using hardware fp itself, and user has to explicitly turn
> > > > > > > > the hardware fp Kconfig option on.
> > > > > > >
> > > > > > > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > > > > > > X86 - other archs can use softfp which is already enabled, as I
> > > > > > > understand it.
> > > > > > >
> > > > > > > >
> > > > > > > > "Select" does not work for architectures that does not have the
> > > > > > > > "enabling hardware fp" logic in place.
> > > > > > > >
> > > > > > > > >         help
> > > > > > > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > > > > > > >           needing to provide a bitmap for each font and size that is needed.
> > > > > > > > > --
> > > > > > >
> > > > > > > I still don't think we are on the same page here. I would prefer to
> > > > > > > just enable the options without any option. I really don't want to get
> > > > > > > into RISC-V stuff - that is a separate concern.
> > > > > > >
> > > > > > > From my POV it seems that x86 is special in that:
> > > > > > > - it uses hardfp
> > > > > > > - hardfp is always available in any CPU with 64-bit support (I think?)
> > > > > >
> > > > > > Maybe the issue even is that on x86 we're being too imprecise in our
> > > > > > build rules (and also on RISC-V, another issue). Today on x86 this fails
> > > > > > because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> > > > > > turn that on, on all x86 targets today and things build. Would that not
> > > > > > also fix the truetype issue?
> > > > >
> > > > > One can easily turn on compiler flags for x86 (and for RISC-V too) to
> > > > > tell the compiler to generate floating point instructions if it sees
> > > > > fit.
> > > > >
> > > > > However on x86 and RISC-V there are configurations needed to program
> > > > > the CPU registers to turn on the hardware FP, otherwise an exception
> > > > > will be generated.
> > > >
> > > > Right, which is why I'm saying why don't we just use -msoft-float
> > > > instead, so that we don't have to worry about enabling features (and
> > > > also additional registers on the stack yes?) ?
> > >
> > > Yes, we should be using -msoft-float for all architectures by default
> > > if the compiler supports that on each arch. IIRC, the RISC-V back-end
> > > didn't support that years ago but things may change recently.
> >
> > OK, so for this series, lets please just simplify the logic in
> > arch/x86/config.mk (and do some boot testing too of course) to
> > -msoft-float everyone, and then the fonts should also be working and we
> > don't have to deal with some other details as well, yes? And having said
> > that, just for sanity sake keep a stopwatch nearby and do some normal
> > functional tests too, to make sure we don't suddenly speed-regress?
> 
> To make fonts work with -msoft-float for everyone, we need U-Boot to
> link with the compiler intrinsics library (e.g.: libgcc, or
> compler-rt). As of today some architectures choose a private libgcc
> implementation within U-Boot.

OK, so playing around further, I thought there was already a config that
hit this as a run-time problem, which isn't the case. So, yes, I was
wrong and for x86_64 I guess you just need to do what needs doing for
SSE to be available and build those files with the right flags. Everyone
else can safely turn off floating point and use a software library (or
at least, RISC-V is the only outstanding question, we can deal with it
later). Sorry for the confusion.

-- 
Tom

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

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Tom Rini]

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

On Sun, Nov 12, 2023 at 01:02:46PM -0700, Simon Glass wrote:

> This is needed to support Truetype fonts. In any case, the compiler
> expects SSE to be available in 64-bit mode. Provide an option to enable
> SSE so that hardware floating-point arithmetic works.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Bin Meng <bmeng.cn@gmail.com>

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

-- 
Tom

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

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Shantur Rathore]

On Sun, Nov 12, 2023 at 8:04 PM Simon Glass <sjg@chromium.org> wrote:
>
> When a USB device is unbound, it causes any bootflows attached to it to
> be removed, via a call to bootdev_clear_bootflows() from
> bootdev_pre_unbind(). This obviously makes it impossible to boot the
> bootflow.
>
> However, when booting a bootflow that relies on USB, usb_stop() is
> called, which unbinds the device. For EFI, this happens in
> efi_exit_boot_services() which means that the bootflow disappears
> before it is finished with.
>
> There is no need to unbind all the USB devices just to quiesce them.
> Add a new usb_pause() call which removes them but leaves them bound.
>
> This resolves a hang on x86 when booting a distro from USB. This was
> found using a device with 4 bootflows, the last of which was USB.
>
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
Tested-by: Shantur Rathore <i@shantur.com>
> ---
>
> Changes in v4:
> - Don't rename the legacy-USB functions
> - Add a bit more detail to the comment
>
> Changes in v2:
> - Add new patch to avoid unbinding devices in use by bootflows
>
>  boot/bootm.c                  |  2 +-
>  common/usb.c                  |  5 +++++
>  drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
>  include/usb.h                 | 21 ++++++++++++++++++++-
>  4 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/boot/bootm.c b/boot/bootm.c
> index cb61485c226c..d9c3fa8dad99 100644
> --- a/boot/bootm.c
> +++ b/boot/bootm.c
> @@ -502,7 +502,7 @@ ulong bootm_disable_interrupts(void)
>          * updated every 1 ms within the HCCA structure in SDRAM! For more
>          * details see the OpenHCI specification.
>          */
> -       usb_stop();
> +       usb_pause();
>  #endif
>         return iflag;
>  }
> diff --git a/common/usb.c b/common/usb.c
> index 836506dcd9e9..a486d1c87d4d 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -144,6 +144,11 @@ int usb_stop(void)
>         return 0;
>  }
>
> +int usb_pause(void)
> +{
> +       return usb_stop();
> +}
> +
>  /******************************************************************************
>   * Detect if a USB device has been plugged or unplugged.
>   */
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index a1cd0ad2d669..c26c65d7986b 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
>         return ops->get_max_xfer_size(bus, size);
>  }
>
> -int usb_stop(void)
> +static int usb_finish(bool unbind_all)
>  {
>         struct udevice *bus;
>         struct udevice *rh;
> @@ -195,7 +195,7 @@ int usb_stop(void)
>
>                 /* Locate root hub device */
>                 device_find_first_child(bus, &rh);
> -               if (rh) {
> +               if (rh && unbind_all) {
>                         /*
>                          * All USB devices are children of root hub.
>                          * Unbinding root hub will unbind all of its children.
> @@ -222,6 +222,16 @@ int usb_stop(void)
>         return err;
>  }
>
> +int usb_stop(void)
> +{
> +       return usb_finish(true);
> +}
> +
> +int usb_pause(void)
> +{
> +       return usb_finish(false);
> +}
> +
>  static void usb_scan_bus(struct udevice *bus, bool recurse)
>  {
>         struct usb_bus_priv *priv;
> diff --git a/include/usb.h b/include/usb.h
> index 09e3f0cb309c..b964e2e1f6b2 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -265,7 +265,26 @@ int usb_kbd_deregister(int force);
>   */
>  int usb_init(void);
>
> -int usb_stop(void); /* stop the USB Controller */
> +/**
> + * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
> + *
> + * This unbinds all devices on the USB buses.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int usb_stop(void);
> +
> +/**
> + * usb_pause() - stop the USB Controller DMA, etc.
> + *
> + * Note that this does not unbind bus devices, so using usb_init() after this
> + * call is not permitted. This function is only useful just before booting, to
> + * ensure that devices are dormant.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int usb_pause(void);
> +
>  int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
>
>
> --
> 2.42.0.869.gea05f2083d-goog
>

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Mark Kettenis]

> From: Simon Glass <sjg@chromium.org>
> Date: Tue, 14 Nov 2023 17:48:25 -0700
> 
> Hi,
> 
> On Tue, 14 Nov 2023 at 17:44, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Tom,
> >
> > On Wed, Nov 15, 2023 at 12:22 AM Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Nov 14, 2023 at 09:49:08AM +0800, Bin Meng wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, Nov 14, 2023 at 7:52 AM Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > > > > > > > Hi Bin,
> > > > > > > >
> > > > > > > > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > > > > > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > > > > > > > SSE so that hardware floating-point arithmetic works.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > Changes in v4:
> > > > > > > > > > - Use a Kconfig option
> > > > > > > > > >
> > > > > > > > > >  arch/x86/Kconfig          |  8 ++++++++
> > > > > > > > > >  arch/x86/config.mk        |  4 ++++
> > > > > > > > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > > > > > > > >  drivers/video/Kconfig     |  1 +
> > > > > > > > > >  4 files changed, 25 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > > > > > > index 99e59d94c606..6b532d712ee8 100644
> > > > > > > > > > --- a/arch/x86/Kconfig
> > > > > > > > > > +++ b/arch/x86/Kconfig
> > > > > > > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > > > > > > > >         hex
> > > > > > > > > >         default 0x10000
> > > > > > > > > >
> > > > > > > > > > +config X86_HARDFP
> > > > > > > > > > +       bool "Support hardware floating point"
> > > > > > > > > > +       help
> > > > > > > > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > > > > > > > +         needed, it can be enabled using this option. This adjusts the
> > > > > > > > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > > > > > > > +         64-bit to enable SSE.
> > > > > > > > >
> > > > > > > > > As discussed in another thread, this option should be made global to
> > > > > > > > > all architectures and by default no.
> > > > > > > > >
> > > > > > > > > > +
> > > > > > > > > >  config HAVE_ITSS
> > > > > > > > > >         bool "Enable ITSS"
> > > > > > > > > >         help
> > > > > > > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > > > > > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > > > > > > > --- a/arch/x86/config.mk
> > > > > > > > > > +++ b/arch/x86/config.mk
> > > > > > > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > > > > > > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > > > > > > > >  else
> > > > > > > > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > > > > > > > +
> > > > > > > > > > +ifndef CONFIG_X86_HARDFP
> > > > > > > > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > > > > > > > >  endif
> > > > > > > > > >
> > > > > > > > > > +endif # IS_32BIT
> > > > > > > > > > +
> > > > > > > > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > > > > > > > >
> > > > > > > > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > > > > > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > > > > > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > > > >  #include <init.h>
> > > > > > > > > >  #include <asm/cpu.h>
> > > > > > > > > >  #include <asm/global_data.h>
> > > > > > > > > > +#include <asm/processor-flags.h>
> > > > > > > > > >
> > > > > > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > > > > > >
> > > > > > > > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > > > > > > > >         return 0;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +/* enable SSE features for hardware floating point */
> > > > > > > > > > +static void setup_sse_features(void)
> > > > > > > > > > +{
> > > > > > > > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > > > > > > > +       "or  %0, %%rax\n" \
> > > > > > > > > > +       "mov %%rax, %%cr4\n" \
> > > > > > > > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  int x86_cpu_reinit_f(void)
> > > > > > > > > >  {
> > > > > > > > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > > > > > > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > > > > > > > >         gd->arch.has_mtrr = true;
> > > > > > > > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > > > > > > > +               setup_sse_features();
> > > > > > > > > >
> > > > > > > > > >         return 0;
> > > > > > > > > >  }
> > > > > > > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > > > > > > index 6f319ba0d544..39c82521be16 100644
> > > > > > > > > > --- a/drivers/video/Kconfig
> > > > > > > > > > +++ b/drivers/video/Kconfig
> > > > > > > > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > > > > > > > >
> > > > > > > > > >  config CONSOLE_TRUETYPE
> > > > > > > > > >         bool "Support a console that uses TrueType fonts"
> > > > > > > > > > +       select X86_HARDFP if X86
> > > > > > > > >
> > > > > > > > > This should be "depends on HARDFP", indicating that the TrueType
> > > > > > > > > library is using hardware fp itself, and user has to explicitly turn
> > > > > > > > > the hardware fp Kconfig option on.
> > > > > > > >
> > > > > > > > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > > > > > > > X86 - other archs can use softfp which is already enabled, as I
> > > > > > > > understand it.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > "Select" does not work for architectures that does not have the
> > > > > > > > > "enabling hardware fp" logic in place.
> > > > > > > > >
> > > > > > > > > >         help
> > > > > > > > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > > > > > > > >           needing to provide a bitmap for each font and size that is needed.
> > > > > > > > > > --
> > > > > > > >
> > > > > > > > I still don't think we are on the same page here. I would prefer to
> > > > > > > > just enable the options without any option. I really don't want to get
> > > > > > > > into RISC-V stuff - that is a separate concern.
> > > > > > > >
> > > > > > > > From my POV it seems that x86 is special in that:
> > > > > > > > - it uses hardfp
> > > > > > > > - hardfp is always available in any CPU with 64-bit support (I think?)
> > > > > > >
> > > > > > > Maybe the issue even is that on x86 we're being too imprecise in our
> > > > > > > build rules (and also on RISC-V, another issue). Today on x86 this fails
> > > > > > > because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> > > > > > > turn that on, on all x86 targets today and things build. Would that not
> > > > > > > also fix the truetype issue?
> > > > > >
> > > > > > One can easily turn on compiler flags for x86 (and for RISC-V too) to
> > > > > > tell the compiler to generate floating point instructions if it sees
> > > > > > fit.
> > > > > >
> > > > > > However on x86 and RISC-V there are configurations needed to program
> > > > > > the CPU registers to turn on the hardware FP, otherwise an exception
> > > > > > will be generated.
> > > > >
> > > > > Right, which is why I'm saying why don't we just use -msoft-float
> > > > > instead, so that we don't have to worry about enabling features (and
> > > > > also additional registers on the stack yes?) ?
> > > >
> > > > Yes, we should be using -msoft-float for all architectures by default
> > > > if the compiler supports that on each arch. IIRC, the RISC-V back-end
> > > > didn't support that years ago but things may change recently.
> > >
> > > OK, so for this series, lets please just simplify the logic in
> > > arch/x86/config.mk (and do some boot testing too of course) to
> > > -msoft-float everyone, and then the fonts should also be working and we
> > > don't have to deal with some other details as well, yes? And having said
> > > that, just for sanity sake keep a stopwatch nearby and do some normal
> > > functional tests too, to make sure we don't suddenly speed-regress?
> >
> > To make fonts work with -msoft-float for everyone, we need U-Boot to
> > link with the compiler intrinsics library (e.g.: libgcc, or
> > compler-rt). As of today some architectures choose a private libgcc
> > implementation within U-Boot.
> 
> I thought I mentioned this but softfp did not work for me on x86 and
> my limited research suggests it is experimental / not used.
> 
> So look, I have a fairly trivial patch here. Perhaps we should just
> apply it and worry about RISC-V when needed?

Well, the thing I would worry about is handing over to the OS with
certain CR4 features enabled that the OS doesn't expect to be enabled.
I think your diff should disable those bits again before handing over
to the OS.  Or is that already done?

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Simon Glass]

Hi Heinrich,

On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >>
> >>
> >> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >> >When a USB device is unbound, it causes any bootflows attached to it to
> >> >be removed, via a call to bootdev_clear_bootflows() from
> >> >bootdev_pre_unbind(). This obviously makes it impossible to boot the
> >> >bootflow.
> >> >
> >> >However, when booting a bootflow that relies on USB, usb_stop() is
> >> >called, which unbinds the device. For EFI, this happens in
> >> >efi_exit_boot_services() which means that the bootflow disappears
> >> >before it is finished with.
> >>
> >> After ExitBootServices() no driver of U-Boot can be used as the operating system is in charge.
> >>
> >> Any bootflow inside U-Boot is terminated by definition when reaching ExitBootServices.
> >>
> >> >
> >> >There is no need to unbind all the USB devices just to quiesce them.
> >> >Add a new usb_pause() call which removes them but leaves them bound.
> >>
> >> As U-Boot must not access any device after ExitBootServices() it is unclear to me what you want to achieve.
> >
> >I can't remember exactly what is needed from the bootflow, but
> >something is. Perhaps the kernel, or the cmdline, or fdt? It would
> >have been better if I had mentioned this explicitly,  But then this
> >patch has been sitting around for ages...
> >
> >In any case, the intent of exit-boot-services is not to free all the
> >memory used, since some of it may have been used to hold data that the
> >app continues to use.
>
> The EFI application reads the memory map and receives an ID which it passes to ExitBootServiced() after this point any memory not marked as EFI runtime can be used by the EFI app. This includes the memory that harbors the U-Boot USB drivers. Therefore no drivers can be used here.
>
> Starting the EFI app via StartImage() must  terminate any running U-Boot "boot flow".
>
> After ExitBootServices() the EFI application cannot return to U-Boot except for SetVirtualAdressMspsetting which relocates the EFI runtime.
>
> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
>
>
>
> >
> >Also there is U-Boot code between when the devices are unbound and
> >when U-Boot actually exits back to the app.
> >
> >This hang was 100% repeatable on brya (an x86 Chromebook) and it took
> >quite a while to find.
>
> We need a better understanding of the problem that you experience to find an adequate solution. Why does removing all devices lead to hanging the system?

Are you able to test this? With your better knowledge of EFI you might
be able to figure out something else that is going on. But in my case
it causes some memory to be freed (perhaps the memory holding the EFI
app?), which is then overwritten by something else being malloc()'d,
so the boot hangs. It is hard to see what is going on after the app
starts.

This patch was sent almost two months ago and fixes a real bug. The
first few versions attracted no comment now it is being blocked
because you don't understand how it fixes anything.

I can get the hardware up again and try this but it will take a while.

Even without the hang, this still fixes a bug. We should be using
device_remove() to quiesce devices. There is no need to unbind them.

BTW another patch that suffered a similar fate was [1]. I just applied
it based on a review from Bin.

[..]

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20231001191444.v3.1.I9f7f373d00947c704aeae0088dfedd8df07fab60@changeid/

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Heinrich Schuchardt]

On 11/15/23 16:50, Simon Glass wrote:
> Hi Heinrich,
>
> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass <sjg@chromium.org>:
>>> Hi Heinrich,
>>>
>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>>
>>>>
>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass <sjg@chromium.org>:
>>>>> When a USB device is unbound, it causes any bootflows attached to it to
>>>>> be removed, via a call to bootdev_clear_bootflows() from
>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the
>>>>> bootflow.
>>>>>
>>>>> However, when booting a bootflow that relies on USB, usb_stop() is
>>>>> called, which unbinds the device. For EFI, this happens in
>>>>> efi_exit_boot_services() which means that the bootflow disappears
>>>>> before it is finished with.
>>>>
>>>> After ExitBootServices() no driver of U-Boot can be used as the operating system is in charge.
>>>>
>>>> Any bootflow inside U-Boot is terminated by definition when reaching ExitBootServices.
>>>>
>>>>>
>>>>> There is no need to unbind all the USB devices just to quiesce them.
>>>>> Add a new usb_pause() call which removes them but leaves them bound.
>>>>
>>>> As U-Boot must not access any device after ExitBootServices() it is unclear to me what you want to achieve.
>>>
>>> I can't remember exactly what is needed from the bootflow, but
>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would
>>> have been better if I had mentioned this explicitly,  But then this
>>> patch has been sitting around for ages...
>>>
>>> In any case, the intent of exit-boot-services is not to free all the
>>> memory used, since some of it may have been used to hold data that the
>>> app continues to use.
>>
>> The EFI application reads the memory map and receives an ID which it passes to ExitBootServiced() after this point any memory not marked as EFI runtime can be used by the EFI app. This includes the memory that harbors the U-Boot USB drivers. Therefore no drivers can be used here.
>>
>> Starting the EFI app via StartImage() must  terminate any running U-Boot "boot flow".
>>
>> After ExitBootServices() the EFI application cannot return to U-Boot except for SetVirtualAdressMspsetting which relocates the EFI runtime.
>>
>> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
>>
>>
>>
>>>
>>> Also there is U-Boot code between when the devices are unbound and
>>> when U-Boot actually exits back to the app.
>>>
>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took
>>> quite a while to find.
>>
>> We need a better understanding of the problem that you experience to find an adequate solution. Why does removing all devices lead to hanging the system?
>
> Are you able to test this? With your better knowledge of EFI you might
> be able to figure out something else that is going on. But in my case
> it causes some memory to be freed (perhaps the memory holding the EFI
> app?), which is then overwritten by something else being malloc()'d,

The memory for the EFI app is not assigned via malloc(). It is allocated
by AllocatedPages().

Reading "some memory freed" does not give me confidence that the problem
is sufficiently analyzed.

> so the boot hangs. It is hard to see what is going on after the app
> starts.
>
> This patch was sent almost two months ago and fixes a real bug. The
> first few versions attracted no comment now it is being blocked
> because you don't understand how it fixes anything.

Do you understand why unbinding the devices causes the problem?

>
> I can get the hardware up again and try this but it will take a while.

Digging a bit deeper seems to be the right approach.

>
> Even without the hang, this still fixes a bug. We should be using
> device_remove() to quiesce devices. There is no need to unbind them.

Is this what the patch does?

>
> BTW another patch that suffered a similar fate was [1]. I just applied
> it based on a review from Bin.

Please, ping Ilias and me if you don't get the feedback that you deserve.

Best regards

Heinrich

>
> [..]
>
> Regards,
> Simon
>
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20231001191444.v3.1.I9f7f373d00947c704aeae0088dfedd8df07fab60@changeid/

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Heinrich Schuchardt]

On 11/15/23 17:23, Heinrich Schuchardt wrote:
> On 11/15/23 16:50, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk@gmx.de>
>> wrote:
>>>
>>>
>>>
>>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass
>>> <sjg@chromium.org>:
>>>> Hi Heinrich,
>>>>
>>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt
>>>> <xypron.glpk@gmx.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass
>>>>> <sjg@chromium.org>:
>>>>>> When a USB device is unbound, it causes any bootflows attached to
>>>>>> it to
>>>>>> be removed, via a call to bootdev_clear_bootflows() from
>>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the
>>>>>> bootflow.
>>>>>>
>>>>>> However, when booting a bootflow that relies on USB, usb_stop() is
>>>>>> called, which unbinds the device. For EFI, this happens in
>>>>>> efi_exit_boot_services() which means that the bootflow disappears
>>>>>> before it is finished with.
>>>>>
>>>>> After ExitBootServices() no driver of U-Boot can be used as the
>>>>> operating system is in charge.
>>>>>
>>>>> Any bootflow inside U-Boot is terminated by definition when
>>>>> reaching ExitBootServices.
>>>>>
>>>>>>
>>>>>> There is no need to unbind all the USB devices just to quiesce them.
>>>>>> Add a new usb_pause() call which removes them but leaves them bound.
>>>>>
>>>>> As U-Boot must not access any device after ExitBootServices() it is
>>>>> unclear to me what you want to achieve.
>>>>
>>>> I can't remember exactly what is needed from the bootflow, but
>>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would
>>>> have been better if I had mentioned this explicitly,  But then this
>>>> patch has been sitting around for ages...
>>>>
>>>> In any case, the intent of exit-boot-services is not to free all the
>>>> memory used, since some of it may have been used to hold data that the
>>>> app continues to use.
>>>
>>> The EFI application reads the memory map and receives an ID which it
>>> passes to ExitBootServiced() after this point any memory not marked
>>> as EFI runtime can be used by the EFI app. This includes the memory
>>> that harbors the U-Boot USB drivers. Therefore no drivers can be used
>>> here.
>>>
>>> Starting the EFI app via StartImage() must  terminate any running
>>> U-Boot "boot flow".
>>>
>>> After ExitBootServices() the EFI application cannot return to U-Boot
>>> except for SetVirtualAdressMspsetting which relocates the EFI runtime.
>>>
>>> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
>>>
>>>
>>>
>>>>
>>>> Also there is U-Boot code between when the devices are unbound and
>>>> when U-Boot actually exits back to the app.
>>>>
>>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took
>>>> quite a while to find.
>>>
>>> We need a better understanding of the problem that you experience to
>>> find an adequate solution. Why does removing all devices lead to
>>> hanging the system?
>>
>> Are you able to test this? With your better knowledge of EFI you might
>> be able to figure out something else that is going on. But in my case
>> it causes some memory to be freed (perhaps the memory holding the EFI
>> app?), which is then overwritten by something else being malloc()'d,
>
> The memory for the EFI app is not assigned via malloc(). It is allocated
> by AllocatedPages().
>
> Reading "some memory freed" does not give me confidence that the problem
> is sufficiently analyzed.
>
>> so the boot hangs. It is hard to see what is going on after the app
>> starts.
>>
>> This patch was sent almost two months ago and fixes a real bug. The
>> first few versions attracted no comment now it is being blocked
>> because you don't understand how it fixes anything.
>
> Do you understand why unbinding the devices causes the problem?
>
>>
>> I can get the hardware up again and try this but it will take a while.
>
> Digging a bit deeper seems to be the right approach.

Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
https://lore.kernel.org/u-boot/CAHc5_t1H39YV=HSSa7TCEdzRctAX+zibkx1vsuwEW-raOL9TcA@mail.gmail.com/

points to a probable root cause:

https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470
free(bflow->buf)

In the EFI boot bflow->buf points to $kernel_addr_r which never was
allocated and therefore must not be freed.

Best regards

Heinrich

>
>>
>> Even without the hang, this still fixes a bug. We should be using
>> device_remove() to quiesce devices. There is no need to unbind them.
>
> Is this what the patch does?
>
>>
>> BTW another patch that suffered a similar fate was [1]. I just applied
>> it based on a review from Bin.
>
> Please, ping Ilias and me if you don't get the feedback that you deserve.
>
> Best regards
>
> Heinrich
>
>>
>> [..]
>>
>> Regards,
>> Simon
>>
>> [1]
>> https://patchwork.ozlabs.org/project/uboot/patch/20231001191444.v3.1.I9f7f373d00947c704aeae0088dfedd8df07fab60@changeid/
>

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Simon Glass]

Hi Heinrich,

On Wed, 15 Nov 2023 at 18:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/15/23 17:23, Heinrich Schuchardt wrote:
> > On 11/15/23 16:50, Simon Glass wrote:
> >> Hi Heinrich,
> >>
> >> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> wrote:
> >>>
> >>>
> >>>
> >>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass
> >>> <sjg@chromium.org>:
> >>>> Hi Heinrich,
> >>>>
> >>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt
> >>>> <xypron.glpk@gmx.de> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass
> >>>>> <sjg@chromium.org>:
> >>>>>> When a USB device is unbound, it causes any bootflows attached to
> >>>>>> it to
> >>>>>> be removed, via a call to bootdev_clear_bootflows() from
> >>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the
> >>>>>> bootflow.
> >>>>>>
> >>>>>> However, when booting a bootflow that relies on USB, usb_stop() is
> >>>>>> called, which unbinds the device. For EFI, this happens in
> >>>>>> efi_exit_boot_services() which means that the bootflow disappears
> >>>>>> before it is finished with.
> >>>>>
> >>>>> After ExitBootServices() no driver of U-Boot can be used as the
> >>>>> operating system is in charge.
> >>>>>
> >>>>> Any bootflow inside U-Boot is terminated by definition when
> >>>>> reaching ExitBootServices.
> >>>>>
> >>>>>>
> >>>>>> There is no need to unbind all the USB devices just to quiesce them.
> >>>>>> Add a new usb_pause() call which removes them but leaves them bound.
> >>>>>
> >>>>> As U-Boot must not access any device after ExitBootServices() it is
> >>>>> unclear to me what you want to achieve.
> >>>>
> >>>> I can't remember exactly what is needed from the bootflow, but
> >>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would
> >>>> have been better if I had mentioned this explicitly,  But then this
> >>>> patch has been sitting around for ages...
> >>>>
> >>>> In any case, the intent of exit-boot-services is not to free all the
> >>>> memory used, since some of it may have been used to hold data that the
> >>>> app continues to use.
> >>>
> >>> The EFI application reads the memory map and receives an ID which it
> >>> passes to ExitBootServiced() after this point any memory not marked
> >>> as EFI runtime can be used by the EFI app. This includes the memory
> >>> that harbors the U-Boot USB drivers. Therefore no drivers can be used
> >>> here.
> >>>
> >>> Starting the EFI app via StartImage() must  terminate any running
> >>> U-Boot "boot flow".
> >>>
> >>> After ExitBootServices() the EFI application cannot return to U-Boot
> >>> except for SetVirtualAdressMspsetting which relocates the EFI runtime.
> >>>
> >>> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
> >>>
> >>>
> >>>
> >>>>
> >>>> Also there is U-Boot code between when the devices are unbound and
> >>>> when U-Boot actually exits back to the app.
> >>>>
> >>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took
> >>>> quite a while to find.
> >>>
> >>> We need a better understanding of the problem that you experience to
> >>> find an adequate solution. Why does removing all devices lead to
> >>> hanging the system?
> >>
> >> Are you able to test this? With your better knowledge of EFI you might
> >> be able to figure out something else that is going on. But in my case
> >> it causes some memory to be freed (perhaps the memory holding the EFI
> >> app?), which is then overwritten by something else being malloc()'d,
> >
> > The memory for the EFI app is not assigned via malloc(). It is allocated
> > by AllocatedPages().
> >
> > Reading "some memory freed" does not give me confidence that the problem
> > is sufficiently analyzed.
> >
> >> so the boot hangs. It is hard to see what is going on after the app
> >> starts.
> >>
> >> This patch was sent almost two months ago and fixes a real bug. The
> >> first few versions attracted no comment now it is being blocked
> >> because you don't understand how it fixes anything.
> >
> > Do you understand why unbinding the devices causes the problem?
> >
> >>
> >> I can get the hardware up again and try this but it will take a while.
> >
> > Digging a bit deeper seems to be the right approach.
>
> Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
> https://lore.kernel.org/u-boot/CAHc5_t1H39YV=HSSa7TCEdzRctAX+zibkx1vsuwEW-raOL9TcA@mail.gmail.com/
>
> points to a probable root cause:
>
> https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470
> free(bflow->buf)
>
> In the EFI boot bflow->buf points to $kernel_addr_r which never was
> allocated and therefore must not be freed.

Yes, this is the root cause of the crash.

However, this patch should still be applied. I can update the commit
message if you like.

Freeing the FDT and kernel before booting them is just not safe,
whether EFI or anything else. Freed memory is not guaranteed to hang
around for any length of time. For example, with truetype fonts,
malloc() is called during any console output and will likely corrupt
the images just as they are being booted.

We should leave the devices bound when booting.

Regards,
Simon

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Heinrich Schuchardt]

On 11/16/23 02:42, Simon Glass wrote:
> Hi Heinrich,
>
> On Wed, 15 Nov 2023 at 18:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 11/15/23 17:23, Heinrich Schuchardt wrote:
>>> On 11/15/23 16:50, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass
>>>>> <sjg@chromium.org>:
>>>>>> Hi Heinrich,
>>>>>>
>>>>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt
>>>>>> <xypron.glpk@gmx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass
>>>>>>> <sjg@chromium.org>:
>>>>>>>> When a USB device is unbound, it causes any bootflows attached to
>>>>>>>> it to
>>>>>>>> be removed, via a call to bootdev_clear_bootflows() from
>>>>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the
>>>>>>>> bootflow.
>>>>>>>>
>>>>>>>> However, when booting a bootflow that relies on USB, usb_stop() is
>>>>>>>> called, which unbinds the device. For EFI, this happens in
>>>>>>>> efi_exit_boot_services() which means that the bootflow disappears
>>>>>>>> before it is finished with.
>>>>>>>
>>>>>>> After ExitBootServices() no driver of U-Boot can be used as the
>>>>>>> operating system is in charge.
>>>>>>>
>>>>>>> Any bootflow inside U-Boot is terminated by definition when
>>>>>>> reaching ExitBootServices.
>>>>>>>
>>>>>>>>
>>>>>>>> There is no need to unbind all the USB devices just to quiesce them.
>>>>>>>> Add a new usb_pause() call which removes them but leaves them bound.
>>>>>>>
>>>>>>> As U-Boot must not access any device after ExitBootServices() it is
>>>>>>> unclear to me what you want to achieve.
>>>>>>
>>>>>> I can't remember exactly what is needed from the bootflow, but
>>>>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would
>>>>>> have been better if I had mentioned this explicitly,  But then this
>>>>>> patch has been sitting around for ages...
>>>>>>
>>>>>> In any case, the intent of exit-boot-services is not to free all the
>>>>>> memory used, since some of it may have been used to hold data that the
>>>>>> app continues to use.
>>>>>
>>>>> The EFI application reads the memory map and receives an ID which it
>>>>> passes to ExitBootServiced() after this point any memory not marked
>>>>> as EFI runtime can be used by the EFI app. This includes the memory
>>>>> that harbors the U-Boot USB drivers. Therefore no drivers can be used
>>>>> here.
>>>>>
>>>>> Starting the EFI app via StartImage() must  terminate any running
>>>>> U-Boot "boot flow".
>>>>>
>>>>> After ExitBootServices() the EFI application cannot return to U-Boot
>>>>> except for SetVirtualAdressMspsetting which relocates the EFI runtime.
>>>>>
>>>>> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Also there is U-Boot code between when the devices are unbound and
>>>>>> when U-Boot actually exits back to the app.
>>>>>>
>>>>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took
>>>>>> quite a while to find.
>>>>>
>>>>> We need a better understanding of the problem that you experience to
>>>>> find an adequate solution. Why does removing all devices lead to
>>>>> hanging the system?
>>>>
>>>> Are you able to test this? With your better knowledge of EFI you might
>>>> be able to figure out something else that is going on. But in my case
>>>> it causes some memory to be freed (perhaps the memory holding the EFI
>>>> app?), which is then overwritten by something else being malloc()'d,
>>>
>>> The memory for the EFI app is not assigned via malloc(). It is allocated
>>> by AllocatedPages().
>>>
>>> Reading "some memory freed" does not give me confidence that the problem
>>> is sufficiently analyzed.
>>>
>>>> so the boot hangs. It is hard to see what is going on after the app
>>>> starts.
>>>>
>>>> This patch was sent almost two months ago and fixes a real bug. The
>>>> first few versions attracted no comment now it is being blocked
>>>> because you don't understand how it fixes anything.
>>>
>>> Do you understand why unbinding the devices causes the problem?
>>>
>>>>
>>>> I can get the hardware up again and try this but it will take a while.
>>>
>>> Digging a bit deeper seems to be the right approach.
>>
>> Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
>> https://lore.kernel.org/u-boot/CAHc5_t1H39YV=HSSa7TCEdzRctAX+zibkx1vsuwEW-raOL9TcA@mail.gmail.com/
>>
>> points to a probable root cause:
>>
>> https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470
>> free(bflow->buf)
>>
>> In the EFI boot bflow->buf points to $kernel_addr_r which never was
>> allocated and therefore must not be freed.
>
> Yes, this is the root cause of the crash.
>
> However, this patch should still be applied. I can update the commit
> message if you like.
>
> Freeing the FDT and kernel before booting them is just not safe,
> whether EFI or anything else. Freed memory is not guaranteed to hang
> around for any length of time. For example, with truetype fonts,
> malloc() is called during any console output and will likely corrupt
> the images just as they are being booted.

Please, observe that malloc() and efi_allocate_pages() use completely
separate parts of the memory.

When reaching ExitBootServices() the memory used by the EFI binary
(relocated in efi_load_pe()) and for the configuration table with the
device-tree have been allocated by efi_allocate_pages(). These addresses
cannot neither allocated by malloc() nor freed with free().

Best regards

Heinrich

>
> We should leave the devices bound when booting.
>
> Regards,
> Simon

Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Simon Glass]

Hi Heinrich,

On Wed, 15 Nov 2023 at 19:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/16/23 02:42, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 15 Nov 2023 at 18:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 11/15/23 17:23, Heinrich Schuchardt wrote:
> >>> On 11/15/23 16:50, Simon Glass wrote:
> >>>> Hi Heinrich,
> >>>>
> >>>> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass
> >>>>> <sjg@chromium.org>:
> >>>>>> Hi Heinrich,
> >>>>>>
> >>>>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt
> >>>>>> <xypron.glpk@gmx.de> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass
> >>>>>>> <sjg@chromium.org>:
> >>>>>>>> When a USB device is unbound, it causes any bootflows attached to
> >>>>>>>> it to
> >>>>>>>> be removed, via a call to bootdev_clear_bootflows() from
> >>>>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the
> >>>>>>>> bootflow.
> >>>>>>>>
> >>>>>>>> However, when booting a bootflow that relies on USB, usb_stop() is
> >>>>>>>> called, which unbinds the device. For EFI, this happens in
> >>>>>>>> efi_exit_boot_services() which means that the bootflow disappears
> >>>>>>>> before it is finished with.
> >>>>>>>
> >>>>>>> After ExitBootServices() no driver of U-Boot can be used as the
> >>>>>>> operating system is in charge.
> >>>>>>>
> >>>>>>> Any bootflow inside U-Boot is terminated by definition when
> >>>>>>> reaching ExitBootServices.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> There is no need to unbind all the USB devices just to quiesce them.
> >>>>>>>> Add a new usb_pause() call which removes them but leaves them bound.
> >>>>>>>
> >>>>>>> As U-Boot must not access any device after ExitBootServices() it is
> >>>>>>> unclear to me what you want to achieve.
> >>>>>>
> >>>>>> I can't remember exactly what is needed from the bootflow, but
> >>>>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would
> >>>>>> have been better if I had mentioned this explicitly,  But then this
> >>>>>> patch has been sitting around for ages...
> >>>>>>
> >>>>>> In any case, the intent of exit-boot-services is not to free all the
> >>>>>> memory used, since some of it may have been used to hold data that the
> >>>>>> app continues to use.
> >>>>>
> >>>>> The EFI application reads the memory map and receives an ID which it
> >>>>> passes to ExitBootServiced() after this point any memory not marked
> >>>>> as EFI runtime can be used by the EFI app. This includes the memory
> >>>>> that harbors the U-Boot USB drivers. Therefore no drivers can be used
> >>>>> here.
> >>>>>
> >>>>> Starting the EFI app via StartImage() must  terminate any running
> >>>>> U-Boot "boot flow".
> >>>>>
> >>>>> After ExitBootServices() the EFI application cannot return to U-Boot
> >>>>> except for SetVirtualAdressMspsetting which relocates the EFI runtime.
> >>>>>
> >>>>> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
> >>>>>
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Also there is U-Boot code between when the devices are unbound and
> >>>>>> when U-Boot actually exits back to the app.
> >>>>>>
> >>>>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took
> >>>>>> quite a while to find.
> >>>>>
> >>>>> We need a better understanding of the problem that you experience to
> >>>>> find an adequate solution. Why does removing all devices lead to
> >>>>> hanging the system?
> >>>>
> >>>> Are you able to test this? With your better knowledge of EFI you might
> >>>> be able to figure out something else that is going on. But in my case
> >>>> it causes some memory to be freed (perhaps the memory holding the EFI
> >>>> app?), which is then overwritten by something else being malloc()'d,
> >>>
> >>> The memory for the EFI app is not assigned via malloc(). It is allocated
> >>> by AllocatedPages().
> >>>
> >>> Reading "some memory freed" does not give me confidence that the problem
> >>> is sufficiently analyzed.
> >>>
> >>>> so the boot hangs. It is hard to see what is going on after the app
> >>>> starts.
> >>>>
> >>>> This patch was sent almost two months ago and fixes a real bug. The
> >>>> first few versions attracted no comment now it is being blocked
> >>>> because you don't understand how it fixes anything.
> >>>
> >>> Do you understand why unbinding the devices causes the problem?
> >>>
> >>>>
> >>>> I can get the hardware up again and try this but it will take a while.
> >>>
> >>> Digging a bit deeper seems to be the right approach.
> >>
> >> Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
> >> https://lore.kernel.org/u-boot/CAHc5_t1H39YV=HSSa7TCEdzRctAX+zibkx1vsuwEW-raOL9TcA@mail.gmail.com/
> >>
> >> points to a probable root cause:
> >>
> >> https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470
> >> free(bflow->buf)
> >>
> >> In the EFI boot bflow->buf points to $kernel_addr_r which never was
> >> allocated and therefore must not be freed.
> >
> > Yes, this is the root cause of the crash.
> >
> > However, this patch should still be applied. I can update the commit
> > message if you like.
> >
> > Freeing the FDT and kernel before booting them is just not safe,
> > whether EFI or anything else. Freed memory is not guaranteed to hang
> > around for any length of time. For example, with truetype fonts,
> > malloc() is called during any console output and will likely corrupt
> > the images just as they are being booted.
>
> Please, observe that malloc() and efi_allocate_pages() use completely
> separate parts of the memory.
>
> When reaching ExitBootServices() the memory used by the EFI binary
> (relocated in efi_load_pe()) and for the configuration table with the
> device-tree have been allocated by efi_allocate_pages(). These addresses
> cannot neither allocated by malloc() nor freed with free().

OK...

To my reading, efi_load_pe() pulls the image apart into memory
allocated with efi_allocate_pages(). So that is separate memory? In
any case, it would end up in a 'struct bootflow'.

IMO the EFI memory-allocation functions should be called only when
booting, not before. I do worry about leakage...

[..]

Regards,
Simon

Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode

[Simon Glass]

Hi Mark,

On Wed, 15 Nov 2023 at 08:46, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Simon Glass <sjg@chromium.org>
> > Date: Tue, 14 Nov 2023 17:48:25 -0700
> >
> > Hi,
> >
> > On Tue, 14 Nov 2023 at 17:44, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Wed, Nov 15, 2023 at 12:22 AM Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Nov 14, 2023 at 09:49:08AM +0800, Bin Meng wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, Nov 14, 2023 at 7:52 AM Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Tue, Nov 14, 2023 at 6:59 AM Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote:
> > > > > > > > > Hi Bin,
> > > > > > > > >
> > > > > > > > > On Mon, 13 Nov 2023 at 15:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Simon,
> > > > > > > > > >
> > > > > > > > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > This is needed to support Truetype fonts. In any case, the compiler
> > > > > > > > > > > expects SSE to be available in 64-bit mode. Provide an option to enable
> > > > > > > > > > > SSE so that hardware floating-point arithmetic works.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > Suggested-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > Changes in v4:
> > > > > > > > > > > - Use a Kconfig option
> > > > > > > > > > >
> > > > > > > > > > >  arch/x86/Kconfig          |  8 ++++++++
> > > > > > > > > > >  arch/x86/config.mk        |  4 ++++
> > > > > > > > > > >  arch/x86/cpu/x86_64/cpu.c | 12 ++++++++++++
> > > > > > > > > > >  drivers/video/Kconfig     |  1 +
> > > > > > > > > > >  4 files changed, 25 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > > > > > > > index 99e59d94c606..6b532d712ee8 100644
> > > > > > > > > > > --- a/arch/x86/Kconfig
> > > > > > > > > > > +++ b/arch/x86/Kconfig
> > > > > > > > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > > > > > > > > > >         hex
> > > > > > > > > > >         default 0x10000
> > > > > > > > > > >
> > > > > > > > > > > +config X86_HARDFP
> > > > > > > > > > > +       bool "Support hardware floating point"
> > > > > > > > > > > +       help
> > > > > > > > > > > +         U-Boot generally does not make use of floating point. Where this is
> > > > > > > > > > > +         needed, it can be enabled using this option. This adjusts the
> > > > > > > > > > > +         start-up code for 64-bit mode and changes the compiler options for
> > > > > > > > > > > +         64-bit to enable SSE.
> > > > > > > > > >
> > > > > > > > > > As discussed in another thread, this option should be made global to
> > > > > > > > > > all architectures and by default no.
> > > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > >  config HAVE_ITSS
> > > > > > > > > > >         bool "Enable ITSS"
> > > > > > > > > > >         help
> > > > > > > > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > > > > > > > > > > index 26ec1af2f0b0..2e3a7119e798 100644
> > > > > > > > > > > --- a/arch/x86/config.mk
> > > > > > > > > > > +++ b/arch/x86/config.mk
> > > > > > > > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > > > > > > > > > >  PLATFORM_CPPFLAGS += -march=i386 -m32
> > > > > > > > > > >  else
> > > > > > > > > > >  PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -march=core2 -m64
> > > > > > > > > > > +
> > > > > > > > > > > +ifndef CONFIG_X86_HARDFP
> > > > > > > > > > >  PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > > > > > > > > > >  endif
> > > > > > > > > > >
> > > > > > > > > > > +endif # IS_32BIT
> > > > > > > > > > > +
> > > > > > > > > > >  PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections -fvisibility=hidden
> > > > > > > > > > >
> > > > > > > > > > >  KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > > > > > > > > > > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > > > index 2647bff891f8..5ea746ecce4d 100644
> > > > > > > > > > > --- a/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > > > +++ b/arch/x86/cpu/x86_64/cpu.c
> > > > > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > > > > >  #include <init.h>
> > > > > > > > > > >  #include <asm/cpu.h>
> > > > > > > > > > >  #include <asm/global_data.h>
> > > > > > > > > > > +#include <asm/processor-flags.h>
> > > > > > > > > > >
> > > > > > > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > > > > > > >
> > > > > > > > > > > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > > > > > > > > > >         return 0;
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > > +/* enable SSE features for hardware floating point */
> > > > > > > > > > > +static void setup_sse_features(void)
> > > > > > > > > > > +{
> > > > > > > > > > > +       asm ("mov %%cr4, %%rax\n" \
> > > > > > > > > > > +       "or  %0, %%rax\n" \
> > > > > > > > > > > +       "mov %%rax, %%cr4\n" \
> > > > > > > > > > > +       : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > >  int x86_cpu_reinit_f(void)
> > > > > > > > > > >  {
> > > > > > > > > > >         /* set the vendor to Intel so that native_calibrate_tsc() works */
> > > > > > > > > > >         gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > > > > > > > > > >         gd->arch.has_mtrr = true;
> > > > > > > > > > > +       if (IS_ENABLED(CONFIG_X86_HARDFP))
> > > > > > > > > > > +               setup_sse_features();
> > > > > > > > > > >
> > > > > > > > > > >         return 0;
> > > > > > > > > > >  }
> > > > > > > > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > > > > > > > index 6f319ba0d544..39c82521be16 100644
> > > > > > > > > > > --- a/drivers/video/Kconfig
> > > > > > > > > > > +++ b/drivers/video/Kconfig
> > > > > > > > > > > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> > > > > > > > > > >
> > > > > > > > > > >  config CONSOLE_TRUETYPE
> > > > > > > > > > >         bool "Support a console that uses TrueType fonts"
> > > > > > > > > > > +       select X86_HARDFP if X86
> > > > > > > > > >
> > > > > > > > > > This should be "depends on HARDFP", indicating that the TrueType
> > > > > > > > > > library is using hardware fp itself, and user has to explicitly turn
> > > > > > > > > > the hardware fp Kconfig option on.
> > > > > > > > >
> > > > > > > > > So you mean 'depends on HARDFP if X86'  ? After all, this is only for
> > > > > > > > > X86 - other archs can use softfp which is already enabled, as I
> > > > > > > > > understand it.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > "Select" does not work for architectures that does not have the
> > > > > > > > > > "enabling hardware fp" logic in place.
> > > > > > > > > >
> > > > > > > > > > >         help
> > > > > > > > > > >           TrueTrype fonts can provide outline-drawing capability rather than
> > > > > > > > > > >           needing to provide a bitmap for each font and size that is needed.
> > > > > > > > > > > --
> > > > > > > > >
> > > > > > > > > I still don't think we are on the same page here. I would prefer to
> > > > > > > > > just enable the options without any option. I really don't want to get
> > > > > > > > > into RISC-V stuff - that is a separate concern.
> > > > > > > > >
> > > > > > > > > From my POV it seems that x86 is special in that:
> > > > > > > > > - it uses hardfp
> > > > > > > > > - hardfp is always available in any CPU with 64-bit support (I think?)
> > > > > > > >
> > > > > > > > Maybe the issue even is that on x86 we're being too imprecise in our
> > > > > > > > build rules (and also on RISC-V, another issue). Today on x86 this fails
> > > > > > > > because we say -mno-mmx -mno-sse and not also -msoft-float. I can just
> > > > > > > > turn that on, on all x86 targets today and things build. Would that not
> > > > > > > > also fix the truetype issue?
> > > > > > >
> > > > > > > One can easily turn on compiler flags for x86 (and for RISC-V too) to
> > > > > > > tell the compiler to generate floating point instructions if it sees
> > > > > > > fit.
> > > > > > >
> > > > > > > However on x86 and RISC-V there are configurations needed to program
> > > > > > > the CPU registers to turn on the hardware FP, otherwise an exception
> > > > > > > will be generated.
> > > > > >
> > > > > > Right, which is why I'm saying why don't we just use -msoft-float
> > > > > > instead, so that we don't have to worry about enabling features (and
> > > > > > also additional registers on the stack yes?) ?
> > > > >
> > > > > Yes, we should be using -msoft-float for all architectures by default
> > > > > if the compiler supports that on each arch. IIRC, the RISC-V back-end
> > > > > didn't support that years ago but things may change recently.
> > > >
> > > > OK, so for this series, lets please just simplify the logic in
> > > > arch/x86/config.mk (and do some boot testing too of course) to
> > > > -msoft-float everyone, and then the fonts should also be working and we
> > > > don't have to deal with some other details as well, yes? And having said
> > > > that, just for sanity sake keep a stopwatch nearby and do some normal
> > > > functional tests too, to make sure we don't suddenly speed-regress?
> > >
> > > To make fonts work with -msoft-float for everyone, we need U-Boot to
> > > link with the compiler intrinsics library (e.g.: libgcc, or
> > > compler-rt). As of today some architectures choose a private libgcc
> > > implementation within U-Boot.
> >
> > I thought I mentioned this but softfp did not work for me on x86 and
> > my limited research suggests it is experimental / not used.
> >
> > So look, I have a fairly trivial patch here. Perhaps we should just
> > apply it and worry about RISC-V when needed?
>
> Well, the thing I would worry about is handing over to the OS with
> certain CR4 features enabled that the OS doesn't expect to be enabled.
> I think your diff should disable those bits again before handing over
> to the OS.  Or is that already done?

I don't see much mention of the required flags Documentation/

I found this code in arch/x86/kernel/fpu/init.c which seems to be
executed on each CPU when it starts up.

/*
 * Initialize the registers found in all CPUs, CR0 and CR4:
 */
static void fpu__init_cpu_generic(void)
{
    unsigned long cr0;
    unsigned long cr4_mask = 0;

    if (boot_cpu_has(X86_FEATURE_FXSR))
        cr4_mask |= X86_CR4_OSFXSR;
    if (boot_cpu_has(X86_FEATURE_XMM))
        cr4_mask |= X86_CR4_OSXMMEXCPT;
    if (cr4_mask)
        cr4_set_bits(cr4_mask);

So it seems that Linux already does this.

Regards,
Simon