[PATCH] efi_loader: Improve console screen clearing and reset

[PATCH] efi_loader: Improve console screen clearing and reset

[Jan Kiszka]

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

Ensure that the default colors are set when clearing the screen so that
the background is properly filled and succeeding colored outputs will
have no differently colored trail.

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

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

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index ba68a150172..0270b20bafe 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -463,8 +463,18 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
 static efi_status_t EFIAPI efi_cout_clear_screen(
 			struct efi_simple_text_output_protocol *this)
 {
+	unsigned int row;
+
 	EFI_ENTRY("%p", this);
 
+	/* Avoid overwriting previous outputs on streaming consoles */
+	for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
+		printf("\n");
+
+	/* Set default colors if not done yet */
+	if (efi_con_mode.attribute == 0)
+		efi_cout_set_attribute(this, 0x07);
+
 	/*
 	 * The Linux console wants both a clear and a home command. The video
 	 * uclass does not support <ESC>[H without coordinates, yet.
@@ -522,11 +532,11 @@ static efi_status_t EFIAPI efi_cout_reset(
 {
 	EFI_ENTRY("%p, %d", this, extended_verification);
 
+	/* Trigger reset to default colors */
+	efi_con_mode.attribute = 0;
+
 	/* Clear screen */
 	EFI_CALL(efi_cout_clear_screen(this));
-	/* Set default colors */
-	efi_con_mode.attribute = 0x07;
-	printf(ESC "[0;37;40m");
 
 	return EFI_EXIT(EFI_SUCCESS);
 }
-- 
2.34.1

Re: [PATCH] efi_loader: Improve console screen clearing and reset

[Heinrich Schuchardt]

On 4/25/22 11:23, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Ensure that the default colors are set when clearing the screen so that
> the background is properly filled and succeeding colored outputs will
> have no differently colored trail.
>
> Before clearing, ensure that no previous output of firmware or UEFI
> programs will overwritten on serial devices or other streaming consoles.
> This helps generating complete boot logs.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   lib/efi_loader/efi_console.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index ba68a150172..0270b20bafe 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -463,8 +463,18 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
>   static efi_status_t EFIAPI efi_cout_clear_screen(
>   			struct efi_simple_text_output_protocol *this)
>   {
> +	unsigned int row;
> +
>   	EFI_ENTRY("%p", this);
>
> +	/* Avoid overwriting previous outputs on streaming consoles */

Thank you for addressing the inconvenience of the partially deleted
scrollback buffer.

Don't assume that the cursor is in the last line. You first have to move
it there:

     printf(ESC "%d;1H", efi_cout_modes[efi_con_mode.mode].rows);

> +	for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
> +		printf("\n");

We may have both video output and serial output. The proposed logic may
not work out correctly if the video screen size differs from the screen
size of the serial console.

Your solution does not cover the cls command.

It will not work with the proposed UEFI boot menu
(https://patchwork.ozlabs.org/project/uboot/list/?series=297320).

A complete solution would have to reside in _serial_puts(). But I doubt
that we want to enlarge the code size there.

Therefore I am a bit skeptical about the suggested change.

> +
> +	/* Set default colors if not done yet */
> +	if (efi_con_mode.attribute == 0)
> +		efi_cout_set_attribute(this, 0x07);

The UEFI specification has this sentence: "The ClearScreen() function
clears the output device(s) display to the currently selected background
color."

So we should not switch colors here.

The currently selected background when starting the first UEFI
application depends on CONFIG_SYS_WHITE_ON_BLACK.

> +
>   	/*
>   	 * The Linux console wants both a clear and a home command. The video
>   	 * uclass does not support <ESC>[H without coordinates, yet.
> @@ -522,11 +532,11 @@ static efi_status_t EFIAPI efi_cout_reset(
>   {
>   	EFI_ENTRY("%p, %d", this, extended_verification);
>
> +	/* Trigger reset to default colors */
> +	efi_con_mode.attribute = 0;
> +
>   	/* Clear screen */
>   	EFI_CALL(efi_cout_clear_screen(this));
> -	/* Set default colors */
> -	efi_con_mode.attribute = 0x07;

This value should depend on CONFIG_SYS_WHITE_ON_BLACK:

Also efi_cout_set_attribute(efi_con_out, 0) should consider
CONFIG_SYS_WHITE_ON_BLACK.

The best thing to do is fixing efi_cout_set_attribute() and invoking

     EFI_CALL(efi_cout_set_attributed(efi_con_out, 0));

But this is a subject for a separate patch.

Best regards

Heinrich

> -	printf(ESC "[0;37;40m");
>
>   	return EFI_EXIT(EFI_SUCCESS);
>   }

Re: [PATCH] efi_loader: Improve console screen clearing and reset

[Jan Kiszka]

On 28.04.22 13:52, Heinrich Schuchardt wrote:
> On 4/25/22 11:23, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Ensure that the default colors are set when clearing the screen so that
>> the background is properly filled and succeeding colored outputs will
>> have no differently colored trail.
>>
>> Before clearing, ensure that no previous output of firmware or UEFI
>> programs will overwritten on serial devices or other streaming consoles.
>> This helps generating complete boot logs.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   lib/efi_loader/efi_console.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
>> index ba68a150172..0270b20bafe 100644
>> --- a/lib/efi_loader/efi_console.c
>> +++ b/lib/efi_loader/efi_console.c
>> @@ -463,8 +463,18 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
>>   static efi_status_t EFIAPI efi_cout_clear_screen(
>>               struct efi_simple_text_output_protocol *this)
>>   {
>> +    unsigned int row;
>> +
>>       EFI_ENTRY("%p", this);
>>
>> +    /* Avoid overwriting previous outputs on streaming consoles */
> 
> Thank you for addressing the inconvenience of the partially deleted
> scrollback buffer.
> 
> Don't assume that the cursor is in the last line. You first have to move
> it there:
> 
>     printf(ESC "%d;1H", efi_cout_modes[efi_con_mode.mode].rows);

Not necessarily - we may just inject more lines than needed. But we
could also avoid that, true.

> 
>> +    for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
>> +        printf("\n");
> 
> We may have both video output and serial output. The proposed logic may
> not work out correctly if the video screen size differs from the screen
> size of the serial console.

OK, but video is not a streaming output with scroll-back buffer, is it?
All we need to ensure is that the geometry used here is that of the
largest streaming output - and rather scroll more than needed to avoid
overwriting. Other outputs should not notice the difference.

> 
> Your solution does not cover the cls command.

Not sure I can follow here yet - which cls are you referring to?

> 
> It will not work with the proposed UEFI boot menu
> (https://patchwork.ozlabs.org/project/uboot/list/?series=297320).
> 

How will it fail? Guess I need to try this out.

> A complete solution would have to reside in _serial_puts(). But I doubt
> that we want to enlarge the code size there.

What is the logic right now when there are multiple outputs attached?

> 
> Therefore I am a bit skeptical about the suggested change.
> 
>> +
>> +    /* Set default colors if not done yet */
>> +    if (efi_con_mode.attribute == 0)
>> +        efi_cout_set_attribute(this, 0x07);
> 
> The UEFI specification has this sentence: "The ClearScreen() function
> clears the output device(s) display to the currently selected background
> color."
> 
> So we should not switch colors here.

We don't do that. We only do that if none was selected so far.

> 
> The currently selected background when starting the first UEFI
> application depends on CONFIG_SYS_WHITE_ON_BLACK.

On ClearScreen, we switch to 0x07 unconditionally (white on black) so far.

> 
>> +
>>       /*
>>        * The Linux console wants both a clear and a home command. The
>> video
>>        * uclass does not support <ESC>[H without coordinates, yet.
>> @@ -522,11 +532,11 @@ static efi_status_t EFIAPI efi_cout_reset(
>>   {
>>       EFI_ENTRY("%p, %d", this, extended_verification);
>>
>> +    /* Trigger reset to default colors */
>> +    efi_con_mode.attribute = 0;
>> +
>>       /* Clear screen */
>>       EFI_CALL(efi_cout_clear_screen(this));
>> -    /* Set default colors */
>> -    efi_con_mode.attribute = 0x07;
> 
> This value should depend on CONFIG_SYS_WHITE_ON_BLACK:
> 
> Also efi_cout_set_attribute(efi_con_out, 0) should consider
> CONFIG_SYS_WHITE_ON_BLACK.
> 
> The best thing to do is fixing efi_cout_set_attribute() and invoking
> 
>     EFI_CALL(efi_cout_set_attributed(efi_con_out, 0));
> 
> But this is a subject for a separate patch.

Indeed, I was just preserving existing behavior (or issues).

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux