[PATCH v3 0/3] efi_loader: console and network dependency improvements

[PATCH v3 0/3] efi_loader: console and network dependency improvements

[Jan Kiszka]

Update of previously sent patches

"efi_loader: Improve console screen clearing and reset"
"efi_loader: Let networking support depend on NETDEVICES"

bundled into one series. Changes to earlier versions:

 - rebased console reset improvements over master
 - split up into overwrite protection and background color fix
 - addressed missing efi_net_set_dhcp_ack symbol
 - converted some #ifdefs to CONFIG_IS_ENABLED

Jan

Jan Kiszka (3):
  efi_loader: Avoid overwriting previous outputs on console screen
    clearing
  efi_loader: Set default console colors on efi_cout_clear_screen if
    needed
  efi_loader: Let networking support depend on NETDEVICES

 include/efi_loader.h             | 10 +++++++---
 lib/efi_loader/Kconfig           |  1 -
 lib/efi_loader/Makefile          |  2 +-
 lib/efi_loader/efi_console.c     | 14 +++++++++++++-
 lib/efi_loader/efi_device_path.c |  8 +++-----
 lib/efi_loader/efi_setup.c       | 10 +++++-----
 lib/efi_selftest/Makefile        |  2 +-
 7 files changed, 30 insertions(+), 17 deletions(-)

-- 
2.35.3

[PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Before clearing the screen, ensure that no previous output of firmware
or UEFI programs will be overwritten on serial devices or other
streaming consoles. This helps generating complete boot logs.

Tested regarding multi-output against qemu-x86_defconfig.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 lib/efi_loader/efi_console.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index 4d08dd3763a..6ce0fcc168d 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -461,10 +461,16 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
 }
 
 /**
- * efi_cout_clear_screen() - clear screen
+ * efi_clear_screen() - clear screen
  */
 static void efi_clear_screen(void)
 {
+	unsigned int row;
+
+	/* Avoid overwriting previous outputs on streaming consoles */
+	for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
+		printf("\n");
+
 	/*
 	 * The Linux console wants both a clear and a home command. The video
 	 * uclass does not support <ESC>[H without coordinates, yet.
-- 
2.35.3

[PATCH v3 2/3] efi_loader: Set default console colors on efi_cout_clear_screen if needed

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Ensures a consistent background color of the whole screen for succeeding
outputs as both demanded by the spec and implemented in EDK2 as well.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 lib/efi_loader/efi_console.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index 6ce0fcc168d..4228a509caf 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -495,6 +495,12 @@ static efi_status_t EFIAPI efi_cout_clear_screen(
 {
 	EFI_ENTRY("%p", this);
 
+	/* Set default colors if not done yet */
+	if (efi_con_mode.attribute == 0) {
+		efi_con_mode.attribute = 0x07;
+		printf(ESC "[0;37;40m");
+	}
+
 	efi_clear_screen();
 
 	return EFI_EXIT(EFI_SUCCESS);
-- 
2.35.3

[PATCH v3 3/3] efi_loader: Let networking support depend on NETDEVICES

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

CONFIG_NET does not imply that there are actually network devices
available. Neither does CONFIG_NETDEVICES, but it is much closer, and
changing to this dependency obsoletes the check in Kconfig because it
means DM_ETH.

Along this change, make sure that efi_net_set_dhcp_ack is always
defined, at least as stub.

Suggested-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/efi_loader.h             | 10 +++++++---
 lib/efi_loader/Kconfig           |  1 -
 lib/efi_loader/Makefile          |  2 +-
 lib/efi_loader/efi_device_path.c |  8 +++-----
 lib/efi_loader/efi_setup.c       | 10 +++++-----
 lib/efi_selftest/Makefile        |  2 +-
 6 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0c6c95ba464..43816491a23 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -94,8 +94,6 @@ void efi_restore_gd(void);
 /* Call this to set the current device name */
 void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
 		     void *buffer, size_t buffer_size);
-/* Called by networking code to memorize the dhcp ack package */
-void efi_net_set_dhcp_ack(void *pkt, int len);
 /* Print information about all loaded images */
 void efi_print_image_infos(void *pc);
 
@@ -118,7 +116,6 @@ static inline void efi_restore_gd(void) { }
 static inline void efi_set_bootdev(const char *dev, const char *devnr,
 				   const char *path, void *buffer,
 				   size_t buffer_size) { }
-static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
 static inline void efi_print_image_infos(void *pc) { }
 static inline efi_status_t efi_launch_capsules(void)
 {
@@ -127,6 +124,13 @@ static inline efi_status_t efi_launch_capsules(void)
 
 #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
 
+#if CONFIG_IS_ENABLED(EFI_LOADER) && CONFIG_IS_ENABLED(NETDEVICES)
+/* Called by networking code to memorize the dhcp ack package */
+void efi_net_set_dhcp_ack(void *pkt, int len);
+#else
+static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
+#endif
+
 /* Maximum number of configuration tables */
 #define EFI_MAX_CONFIGURATION_TABLES 16
 
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 41756ea5396..68e6c2531e1 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -11,7 +11,6 @@ config EFI_LOADER
 	# We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB
 	depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT
 	depends on BLK
-	depends on DM_ETH || !NET
 	depends on !EFI_APP
 	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
 	select CHARSET
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index f8e8afe1284..8738757dd2c 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -68,7 +68,7 @@ obj-y += efi_watchdog.o
 obj-$(CONFIG_EFI_ESRT) += efi_esrt.o
 obj-$(CONFIG_VIDEO) += efi_gop.o
 obj-$(CONFIG_BLK) += efi_disk.o
-obj-$(CONFIG_NET) += efi_net.o
+obj-$(CONFIG_NETDEVICES) += efi_net.o
 obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
 obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
 obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index acae007f26f..171ee2b4679 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -613,7 +613,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev)
 		*vdp = ROOT;
 		return &vdp[1];
 	}
-#ifdef CONFIG_NET
+#ifdef CONFIG_NETDEVICES
 	case UCLASS_ETH: {
 		struct efi_device_path_mac_addr *dp =
 			dp_fill(buf, dev->parent);
@@ -1052,7 +1052,7 @@ struct efi_device_path *efi_dp_from_uart(void)
 	return buf;
 }
 
-#ifdef CONFIG_NET
+#ifdef CONFIG_NETDEVICES
 struct efi_device_path *efi_dp_from_eth(void)
 {
 	void *buf, *start;
@@ -1169,10 +1169,8 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 		return EFI_INVALID_PARAMETER;
 
 	if (!strcmp(dev, "Net")) {
-#ifdef CONFIG_NET
-		if (device)
+		if (CONFIG_IS_ENABLED(NETDEVICES) && device)
 			*device = efi_dp_from_eth();
-#endif
 	} else if (!strcmp(dev, "Uart")) {
 		if (device)
 			*device = efi_dp_from_uart();
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index a340bc38806..cec799d93e2 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -331,11 +331,11 @@ efi_status_t efi_init_obj_list(void)
 		if (ret != EFI_SUCCESS)
 			goto out;
 	}
-#ifdef CONFIG_NET
-	ret = efi_net_register();
-	if (ret != EFI_SUCCESS)
-		goto out;
-#endif
+	if (IS_ENABLED(CONFIG_NETDEVICES)) {
+		ret = efi_net_register();
+		if (ret != EFI_SUCCESS)
+			goto out;
+	}
 #ifdef CONFIG_GENERATE_ACPI_TABLE
 	ret = efi_acpi_register();
 	if (ret != EFI_SUCCESS)
diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index daac6c39682..e4d75420bff 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -50,7 +50,7 @@ efi_selftest_variables_runtime.o \
 efi_selftest_watchdog.o
 
 obj-$(CONFIG_EFI_ECPT) += efi_selftest_ecpt.o
-obj-$(CONFIG_NET) += efi_selftest_snp.o
+obj-$(CONFIG_NETDEVICES) += efi_selftest_snp.o
 
 obj-$(CONFIG_EFI_DEVICE_PATH_TO_TEXT) += efi_selftest_devicepath.o
 obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += \
-- 
2.35.3

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Simon Glass]

Hi,

On Fri, 4 Nov 2022 at 02:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Before clearing the screen, ensure that no previous output of firmware
> or UEFI programs will be overwritten on serial devices or other
> streaming consoles. This helps generating complete boot logs.
>
> Tested regarding multi-output against qemu-x86_defconfig.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  lib/efi_loader/efi_console.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Please see this thread...some things need to be improved in this area.

https://patchwork.ozlabs.org/project/uboot/patch/20221022092058.106052-1-heinrich.schuchardt@canonical.com/

>
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index 4d08dd3763a..6ce0fcc168d 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -461,10 +461,16 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
>  }
>
>  /**
> - * efi_cout_clear_screen() - clear screen
> + * efi_clear_screen() - clear screen
>   */
>  static void efi_clear_screen(void)
>  {
> +       unsigned int row;
> +
> +       /* Avoid overwriting previous outputs on streaming consoles */
> +       for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
> +               printf("\n");
> +
>         /*
>          * The Linux console wants both a clear and a home command. The video
>          * uclass does not support <ESC>[H without coordinates, yet.
> --
> 2.35.3
>

Regards,
SImon

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Jan Kiszka]

On 04.11.22 20:08, Simon Glass wrote:
> Hi,
> 
> On Fri, 4 Nov 2022 at 02:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Before clearing the screen, ensure that no previous output of firmware
>> or UEFI programs will be overwritten on serial devices or other
>> streaming consoles. This helps generating complete boot logs.
>>
>> Tested regarding multi-output against qemu-x86_defconfig.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  lib/efi_loader/efi_console.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> Please see this thread...some things need to be improved in this area.
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20221022092058.106052-1-heinrich.schuchardt@canonical.com/
> 

Is there a conclusion already? Is there something that I can attach to
to resolve this issue here?

Jan

>>
>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
>> index 4d08dd3763a..6ce0fcc168d 100644
>> --- a/lib/efi_loader/efi_console.c
>> +++ b/lib/efi_loader/efi_console.c
>> @@ -461,10 +461,16 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
>>  }
>>
>>  /**
>> - * efi_cout_clear_screen() - clear screen
>> + * efi_clear_screen() - clear screen
>>   */
>>  static void efi_clear_screen(void)
>>  {
>> +       unsigned int row;
>> +
>> +       /* Avoid overwriting previous outputs on streaming consoles */
>> +       for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
>> +               printf("\n");
>> +
>>         /*
>>          * The Linux console wants both a clear and a home command. The video
>>          * uclass does not support <ESC>[H without coordinates, yet.
>> --
>> 2.35.3
>>
> 
> Regards,
> SImon

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Simon Glass]

Hi Jan,

On Mon, 7 Nov 2022 at 06:50, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 04.11.22 20:08, Simon Glass wrote:
> > Hi,
> >
> > On Fri, 4 Nov 2022 at 02:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Before clearing the screen, ensure that no previous output of firmware
> >> or UEFI programs will be overwritten on serial devices or other
> >> streaming consoles. This helps generating complete boot logs.
> >>
> >> Tested regarding multi-output against qemu-x86_defconfig.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  lib/efi_loader/efi_console.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > Please see this thread...some things need to be improved in this area.
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20221022092058.106052-1-heinrich.schuchardt@canonical.com/
> >
>
> Is there a conclusion already? Is there something that I can attach to
> to resolve this issue here?

So far as I am concerned, there is a conclusion :) I provided a lot of
detail in the thread linked above.

We need to fix this properly (by suppressing ANSI output in logs) as
these hacks (and others) are just annoying.

Regards,
SImon

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Heinrich Schuchardt]

On 11/7/22 16:28, Simon Glass wrote:
> Hi Jan,
>
> On Mon, 7 Nov 2022 at 06:50, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 04.11.22 20:08, Simon Glass wrote:
>>> Hi,
>>>
>>> On Fri, 4 Nov 2022 at 02:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Before clearing the screen, ensure that no previous output of firmware
>>>> or UEFI programs will be overwritten on serial devices or other
>>>> streaming consoles. This helps generating complete boot logs.
>>>>
>>>> Tested regarding multi-output against qemu-x86_defconfig.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>   lib/efi_loader/efi_console.c | 8 +++++++-
>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> Please see this thread...some things need to be improved in this area.
>>>
>>> https://patchwork.ozlabs.org/project/uboot/patch/20221022092058.106052-1-heinrich.schuchardt@canonical.com/
>>>
>>
>> Is there a conclusion already? Is there something that I can attach to
>> to resolve this issue here?
>
> So far as I am concerned, there is a conclusion :) I provided a lot of
> detail in the thread linked above.

Simon, if you want to filter output on the sandbox you may do so in the
serial driver of the sandbox. It is not related to the UEFI code.

Best regards

Heinrich

>
> We need to fix this properly (by suppressing ANSI output in logs) as
> these hacks (and others) are just annoying.
>
> Regards,
> SImon

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Heinrich Schuchardt]

On 11/4/22 09:06, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Before clearing the screen, ensure that no previous output of firmware
> or UEFI programs will be overwritten on serial devices or other
> streaming consoles. This helps generating complete boot logs.
>
> Tested regarding multi-output against qemu-x86_defconfig.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   lib/efi_loader/efi_console.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index 4d08dd3763a..6ce0fcc168d 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -461,10 +461,16 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
>   }
>
>   /**
> - * efi_cout_clear_screen() - clear screen
> + * efi_clear_screen() - clear screen
>    */
>   static void efi_clear_screen(void)
>   {
> +	unsigned int row;
> +
> +	/* Avoid overwriting previous outputs on streaming consoles */
> +	for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
> +		printf("\n");
> +

Scrolling on a framebuffer is a very expensive operations: For each of
the 135 lines of an UHD display you have to copy 32 MiB. This would
create a delay of multiple seconds on a slow device.

So keep the patch for your debugging purposed. But we cannot merge it
into upstream.

Best regards

Heinrich

>   	/*
>   	 * The Linux console wants both a clear and a home command. The video
>   	 * uclass does not support <ESC>[H without coordinates, yet.

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Jan Kiszka]

On 07.11.22 17:29, Heinrich Schuchardt wrote:
> On 11/4/22 09:06, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Before clearing the screen, ensure that no previous output of firmware
>> or UEFI programs will be overwritten on serial devices or other
>> streaming consoles. This helps generating complete boot logs.
>>
>> Tested regarding multi-output against qemu-x86_defconfig.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   lib/efi_loader/efi_console.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
>> index 4d08dd3763a..6ce0fcc168d 100644
>> --- a/lib/efi_loader/efi_console.c
>> +++ b/lib/efi_loader/efi_console.c
>> @@ -461,10 +461,16 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
>>   }
>>
>>   /**
>> - * efi_cout_clear_screen() - clear screen
>> + * efi_clear_screen() - clear screen
>>    */
>>   static void efi_clear_screen(void)
>>   {
>> +    unsigned int row;
>> +
>> +    /* Avoid overwriting previous outputs on streaming consoles */
>> +    for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
>> +        printf("\n");
>> +
> 
> Scrolling on a framebuffer is a very expensive operations: For each of
> the 135 lines of an UHD display you have to copy 32 MiB. This would
> create a delay of multiple seconds on a slow device.

Thanks for explaining this - now. I didn't observe this delay in QEMU,
but maybe that wasn't representative

> 
> So keep the patch for your debugging purposed. But we cannot merge it
> into upstream.

It's in production - we can't afford shipping without it.

To avoid that this will be the last patch between full upstream and our
device one day: Do you see a way to confine overwrite protection to real
streaming device, UARTs and such?

Thanks,
Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH v3 2/3] efi_loader: Set default console colors on efi_cout_clear_screen if needed

[Jan Kiszka]

On 04.11.22 09:06, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Ensures a consistent background color of the whole screen for succeeding
> outputs as both demanded by the spec and implemented in EDK2 as well.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  lib/efi_loader/efi_console.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index 6ce0fcc168d..4228a509caf 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -495,6 +495,12 @@ static efi_status_t EFIAPI efi_cout_clear_screen(
>  {
>  	EFI_ENTRY("%p", this);
>  
> +	/* Set default colors if not done yet */
> +	if (efi_con_mode.attribute == 0) {
> +		efi_con_mode.attribute = 0x07;
> +		printf(ESC "[0;37;40m");
> +	}
> +
>  	efi_clear_screen();
>  
>  	return EFI_EXIT(EFI_SUCCESS);

And what about this visual fix? It does not depend on patch 1.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH v3 2/3] efi_loader: Set default console colors on efi_cout_clear_screen if needed

[Heinrich Schuchardt]

On 11/4/22 09:06, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Ensures a consistent background color of the whole screen for succeeding
> outputs as both demanded by the spec and implemented in EDK2 as well.

To which sentence in the UEFI 2.10 spec are you relating?

>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   lib/efi_loader/efi_console.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index 6ce0fcc168d..4228a509caf 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -495,6 +495,12 @@ static efi_status_t EFIAPI efi_cout_clear_screen(
>   {
>   	EFI_ENTRY("%p", this);
>
> +	/* Set default colors if not done yet */
> +	if (efi_con_mode.attribute == 0) {
> +		efi_con_mode.attribute = 0x07;
> +		printf(ESC "[0;37;40m");

These values are correct for CONFIG_SYS_WHITE_ON_BLACK=y. But for
CONFIG_SYS_WHITE_ON_BLACK=n I would expect black letters on white as
default.

This requires adding support background color values 100 - 107 (cf.
https://en.wikipedia.org/w/index.php?title=ANSI_escape_code&section=15#3-bit_and_4-bit)
in vidconsole_escape_char().

UEFI background color white should be translated to 107 if
CONFIG_SYS_WHITE_ON_BLACK=n.

It seems this patch is trying to solve a problem caused by patch 1 of
the series. Or do I miss something?

Best regards

Heinrich

> +	}
> +
>   	efi_clear_screen();
>
>   	return EFI_EXIT(EFI_SUCCESS);

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Heinrich Schuchardt]

On 11/7/22 17:41, Jan Kiszka wrote:
> On 07.11.22 17:29, Heinrich Schuchardt wrote:
>> On 11/4/22 09:06, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Before clearing the screen, ensure that no previous output of firmware
>>> or UEFI programs will be overwritten on serial devices or other
>>> streaming consoles. This helps generating complete boot logs.
>>>
>>> Tested regarding multi-output against qemu-x86_defconfig.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>    lib/efi_loader/efi_console.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
>>> index 4d08dd3763a..6ce0fcc168d 100644
>>> --- a/lib/efi_loader/efi_console.c
>>> +++ b/lib/efi_loader/efi_console.c
>>> @@ -461,10 +461,16 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
>>>    }
>>>
>>>    /**
>>> - * efi_cout_clear_screen() - clear screen
>>> + * efi_clear_screen() - clear screen
>>>     */
>>>    static void efi_clear_screen(void)
>>>    {
>>> +    unsigned int row;
>>> +
>>> +    /* Avoid overwriting previous outputs on streaming consoles */
>>> +    for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
>>> +        printf("\n");
>>> +
>>
>> Scrolling on a framebuffer is a very expensive operations: For each of
>> the 135 lines of an UHD display you have to copy 32 MiB. This would
>> create a delay of multiple seconds on a slow device.
>
> Thanks for explaining this - now. I didn't observe this delay in QEMU,
> but maybe that wasn't representative

My BananaPi is really slow on it.

>
>>
>> So keep the patch for your debugging purposed. But we cannot merge it
>> into upstream.
>
> It's in production - we can't afford shipping without it.
>
> To avoid that this will be the last patch between full upstream and our
> device one day: Do you see a way to confine overwrite protection to real
> streaming device, UARTs and such?

How about putting this behind a Kconfig switch?

You probably should first move the cursor to the lower right corner
before sending carriage returns and then place the cursor in the upper
left corner. Otherwise you cannot be sure that you scrolled all of the
text out of the visible area.

If you use scrolling, why would you still send an ANSI sequence for
clear screen?

Best regards

Heinrich

>
> Thanks,
> Jan
>

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Simon Glass]

Hi Heinrich,

On Mon, 7 Nov 2022 at 09:18, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/7/22 16:28, Simon Glass wrote:
> > Hi Jan,
> >
> > On Mon, 7 Nov 2022 at 06:50, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 04.11.22 20:08, Simon Glass wrote:
> >>> Hi,
> >>>
> >>> On Fri, 4 Nov 2022 at 02:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> Before clearing the screen, ensure that no previous output of firmware
> >>>> or UEFI programs will be overwritten on serial devices or other
> >>>> streaming consoles. This helps generating complete boot logs.
> >>>>
> >>>> Tested regarding multi-output against qemu-x86_defconfig.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> ---
> >>>>   lib/efi_loader/efi_console.c | 8 +++++++-
> >>>>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> Please see this thread...some things need to be improved in this area.
> >>>
> >>> https://patchwork.ozlabs.org/project/uboot/patch/20221022092058.106052-1-heinrich.schuchardt@canonical.com/
> >>>
> >>
> >> Is there a conclusion already? Is there something that I can attach to
> >> to resolve this issue here?
> >
> > So far as I am concerned, there is a conclusion :) I provided a lot of
> > detail in the thread linked above.
>
> Simon, if you want to filter output on the sandbox you may do so in the
> serial driver of the sandbox. It is not related to the UEFI code.

From what I can tell this problem also affects the qemu CI tests. It
seems that UEFI needs ANSI so this appears to be a UEFI thing. I
haven't actually noticed any other use of ANSI sequences in some years
of using U-Boot.

Regards,
Simon

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Heinrich Schuchardt]

On 11/7/22 18:32, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 7 Nov 2022 at 09:18, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 11/7/22 16:28, Simon Glass wrote:
>>> Hi Jan,
>>>
>>> On Mon, 7 Nov 2022 at 06:50, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> On 04.11.22 20:08, Simon Glass wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, 4 Nov 2022 at 02:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> Before clearing the screen, ensure that no previous output of firmware
>>>>>> or UEFI programs will be overwritten on serial devices or other
>>>>>> streaming consoles. This helps generating complete boot logs.
>>>>>>
>>>>>> Tested regarding multi-output against qemu-x86_defconfig.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>>    lib/efi_loader/efi_console.c | 8 +++++++-
>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> Please see this thread...some things need to be improved in this area.
>>>>>
>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20221022092058.106052-1-heinrich.schuchardt@canonical.com/
>>>>>
>>>>
>>>> Is there a conclusion already? Is there something that I can attach to
>>>> to resolve this issue here?
>>>
>>> So far as I am concerned, there is a conclusion :) I provided a lot of
>>> detail in the thread linked above.
>>
>> Simon, if you want to filter output on the sandbox you may do so in the
>> serial driver of the sandbox. It is not related to the UEFI code.
>
>>From what I can tell this problem also affects the qemu CI tests. It
> seems that UEFI needs ANSI so this appears to be a UEFI thing. I
> haven't actually noticed any other use of ANSI sequences in some years
> of using U-Boot.

Yes, you reviewed patch a085aa1f2737 ("dm: video: Add basic ANSI escape
sequence support") exactly for this purpose.

Both CI tests on QEMU and sandbox pass. It is good that the CI sees
exactly the same output as a productive system. We should not change that.

Best regards

Heinrich

Re: [PATCH v3 2/3] efi_loader: Set default console colors on efi_cout_clear_screen if needed

[Jan Kiszka]

On 07.11.22 18:06, Heinrich Schuchardt wrote:
> On 11/4/22 09:06, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Ensures a consistent background color of the whole screen for succeeding
>> outputs as both demanded by the spec and implemented in EDK2 as well.
> 
> To which sentence in the UEFI 2.10 spec are you relating?
> 

12.4:

"EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.ClearScreen()

Clears the output device(s) display to the currently selected background
color."

That generally works - except for this default case.

>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   lib/efi_loader/efi_console.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
>> index 6ce0fcc168d..4228a509caf 100644
>> --- a/lib/efi_loader/efi_console.c
>> +++ b/lib/efi_loader/efi_console.c
>> @@ -495,6 +495,12 @@ static efi_status_t EFIAPI efi_cout_clear_screen(
>>   {
>>       EFI_ENTRY("%p", this);
>>
>> +    /* Set default colors if not done yet */
>> +    if (efi_con_mode.attribute == 0) {
>> +        efi_con_mode.attribute = 0x07;
>> +        printf(ESC "[0;37;40m");
> 
> These values are correct for CONFIG_SYS_WHITE_ON_BLACK=y. But for
> CONFIG_SYS_WHITE_ON_BLACK=n I would expect black letters on white as
> default.
> 
> This requires adding support background color values 100 - 107 (cf.
> https://en.wikipedia.org/w/index.php?title=ANSI_escape_code&section=15#3-bit_and_4-bit)
> in vidconsole_escape_char().
> 
> UEFI background color white should be translated to 107 if
> CONFIG_SYS_WHITE_ON_BLACK=n.

I don't think we handle CONFIG_SYS_WHITE_ON_BLACK=n in UEFI so far. It
likely requires translating all colors that apps so far select.
Otherwise, you will end up with white-on-EFI_WHITE.

> 
> It seems this patch is trying to solve a problem caused by patch 1 of
> the series. Or do I miss something?

Not at all, problem persists if you drop that patch. I've accidentally
accounted to "efi_loader: avoid EFI_CALL() for clearing screen", see
https://lore.kernel.org/u-boot/532ff3e2-06ba-3a3e-5cf9-2f4a9b7abb8c@siemens.com/
for a screen shot.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH v3 1/3] efi_loader: Avoid overwriting previous outputs on console screen clearing

[Jan Kiszka]

On 07.11.22 18:18, Heinrich Schuchardt wrote:
> On 11/7/22 17:41, Jan Kiszka wrote:
>> On 07.11.22 17:29, Heinrich Schuchardt wrote:
>>> On 11/4/22 09:06, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Before clearing the screen, ensure that no previous output of firmware
>>>> or UEFI programs will be overwritten on serial devices or other
>>>> streaming consoles. This helps generating complete boot logs.
>>>>
>>>> Tested regarding multi-output against qemu-x86_defconfig.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>    lib/efi_loader/efi_console.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_console.c
>>>> b/lib/efi_loader/efi_console.c
>>>> index 4d08dd3763a..6ce0fcc168d 100644
>>>> --- a/lib/efi_loader/efi_console.c
>>>> +++ b/lib/efi_loader/efi_console.c
>>>> @@ -461,10 +461,16 @@ static efi_status_t EFIAPI
>>>> efi_cout_set_attribute(
>>>>    }
>>>>
>>>>    /**
>>>> - * efi_cout_clear_screen() - clear screen
>>>> + * efi_clear_screen() - clear screen
>>>>     */
>>>>    static void efi_clear_screen(void)
>>>>    {
>>>> +    unsigned int row;
>>>> +
>>>> +    /* Avoid overwriting previous outputs on streaming consoles */
>>>> +    for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
>>>> +        printf("\n");
>>>> +
>>>
>>> Scrolling on a framebuffer is a very expensive operations: For each of
>>> the 135 lines of an UHD display you have to copy 32 MiB. This would
>>> create a delay of multiple seconds on a slow device.
>>
>> Thanks for explaining this - now. I didn't observe this delay in QEMU,
>> but maybe that wasn't representative
> 
> My BananaPi is really slow on it.
> 
>>
>>>
>>> So keep the patch for your debugging purposed. But we cannot merge it
>>> into upstream.
>>
>> It's in production - we can't afford shipping without it.
>>
>> To avoid that this will be the last patch between full upstream and our
>> device one day: Do you see a way to confine overwrite protection to real
>> streaming device, UARTs and such?
> 
> How about putting this behind a Kconfig switch?

Sounds like a plan.

> 
> You probably should first move the cursor to the lower right corner
> before sending carriage returns and then place the cursor in the upper
> left corner. Otherwise you cannot be sure that you scrolled all of the
> text out of the visible area.

I'm scrolling based on the screen size so far. Not sure how moving the
cursor should improve that.

> 
> If you use scrolling, why would you still send an ANSI sequence for
> clear screen?

Indeed, that should be unneeded. Or is there a cheaper way to scroll via
ANSI sequences?

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux