| Message ID | ddbe1239-326f-7d8e-4ecf-af03b3698570@siemens.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [v2] efi_loader: Improve console screen clearing and reset | expand |
On 5/6/22 16:50, 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> > --- > > Changes in v2: > - rebased and tested against more scenarios, sucessfully > > 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 60a3fc85ac4..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"); Unfortunately you seem to have missed to consider my review comments in https://lists.denx.de/pipermail/u-boot/2022-April/482754.html Best regards Heinrich > + > + /* 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,9 +532,9 @@ static efi_status_t EFIAPI efi_cout_reset( > { > EFI_ENTRY("%p, %d", this, extended_verification); > > - /* Set default colors */ > - efi_con_mode.attribute = 0x07; > - printf(ESC "[0;37;40m"); > + /* Trigger reset to default colors */ > + efi_con_mode.attribute = 0; > + > /* Clear screen */ > EFI_CALL(efi_cout_clear_screen(this)); >
On 06.05.22 18:46, Heinrich Schuchardt wrote: > On 5/6/22 16:50, 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> >> --- >> >> Changes in v2: >> - rebased and tested against more scenarios, sucessfully >> >> 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 60a3fc85ac4..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"); > > Unfortunately you seem to have missed to consider my review comments in > https://lists.denx.de/pipermail/u-boot/2022-April/482754.html Nope, I address them or verified them to not apply. Please highlight what I either missed or what I have to reproduce how. Jan
On 09.05.22 10:31, Jan Kiszka wrote: > On 06.05.22 18:46, Heinrich Schuchardt wrote: >> On 5/6/22 16:50, 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> >>> --- >>> >>> Changes in v2: >>> - rebased and tested against more scenarios, sucessfully >>> >>> 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 60a3fc85ac4..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"); >> >> Unfortunately you seem to have missed to consider my review comments in >> https://lists.denx.de/pipermail/u-boot/2022-April/482754.html > > Nope, I address them or verified them to not apply. Please highlight > what I either missed or what I have to reproduce how. > Ok, one thing I actually ignored as not directly related was the "cls" command and possibly other U-Boot internal clear-screen triggers. Those might be addressed similarly, but that is a different story for me. The purpose of this patch is to avoid that U-Boot external workload, EFI executables, can trigger overwriting console logs of U-Boot. If U-Boot decides to do that itself, or if the user does that on the command line, that is a different thing for me. BTW, the bootmenu would likely be fine an in-place approach as well, but nothing at UART level. Jan
On 09.05.22 11:05, Jan Kiszka wrote: > On 09.05.22 10:31, Jan Kiszka wrote: >> On 06.05.22 18:46, Heinrich Schuchardt wrote: >>> On 5/6/22 16:50, 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> >>>> --- >>>> >>>> Changes in v2: >>>> - rebased and tested against more scenarios, sucessfully >>>> >>>> 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 60a3fc85ac4..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"); >>> >>> Unfortunately you seem to have missed to consider my review comments in >>> https://lists.denx.de/pipermail/u-boot/2022-April/482754.html >> >> Nope, I address them or verified them to not apply. Please highlight >> what I either missed or what I have to reproduce how. >> > > Ok, one thing I actually ignored as not directly related was the "cls" > command and possibly other U-Boot internal clear-screen triggers. Those > might be addressed similarly, but that is a different story for me. The > purpose of this patch is to avoid that U-Boot external workload, EFI > executables, can trigger overwriting console logs of U-Boot. If U-Boot > decides to do that itself, or if the user does that on the command line, > that is a different thing for me. > > BTW, the bootmenu would likely be fine an in-place approach as well, but > nothing at UART level. > This one is still open. Can you please help clarifying what you would like me to address in addition? The patch is required for our boards which are being migrated to UEFI and shall maintain their complete boot logs. Thanks, Jan
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index 60a3fc85ac4..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,9 +532,9 @@ static efi_status_t EFIAPI efi_cout_reset( { EFI_ENTRY("%p, %d", this, extended_verification); - /* Set default colors */ - efi_con_mode.attribute = 0x07; - printf(ESC "[0;37;40m"); + /* Trigger reset to default colors */ + efi_con_mode.attribute = 0; + /* Clear screen */ EFI_CALL(efi_cout_clear_screen(this));