Hi Dave, You're welcome, glad you find the patches useful :) Have a nice day, Marek On 4/22/25 15:57, Dave Henderson wrote:> Thanks for your contributions Marek!? I still use this bootloader and > appreciate all patches that continue to keep it working! > > > Dave > > > > On 4/21/25 15:45, Marek Benc via Syslinux wrote: >> Hello, >> >> While debugging #1103692 on Debian [0], I came across an issue where >> error messages generated early in the syslinux boot process through >> writestr() didn't end up appearing on-screen. Instead, it appeared >> that only the cursor moved. >> >> I discovered that it's because the early code passes a video attribute >> of 0 to the efi_write_char() function, which is black-on-black, and >> produces invisible output. >> >> The output can actually be read on the serial line. For example, if we >> have a serial line dump in the file serial.dump, and we have problems >> with loading ldlinux.e32, the following can be used to read the message: >> >> $ ansi2txt < serial.dump > serial.log >> $ tail -n4 serial.log >> Shell> FS0:\EFI\syslinux\syslinux.efi >> >> Failed to load ldlinux.e32 >> Shell> >> >> Detailed steps on reproducing this issue are present in [0], >> and if the issue reported in that bug gets fixed, you can easily >> trigger this error message by deleting the ldlinux.e32 file. >> >> Another issue which might render the error message unreadable is that >> the error message doesn't contain a newline marker, which might cause >> the EFI shell command prompt to overwrite the message. This happens >> with OVMF under Qemu. >> >> >> Yet another minor issue is that the efi_scroll_up() function generates >> a CRLF pair on the reverse order, LFCR, which can confuse some text >> editors when viewing the logs, so it would be good to put them into >> the correct order. >> >> The efi_scroll_up() function also contains a bug where it ignores >> the number of rows to scroll up by and always scrolls once, but this >> seems to be intentional, as it's used to mask a different bug in >> the ansicon module that causes it to scroll way more than it should >> under EFI. >> >> The ansicon module is used by ldlinux, and this masked bug can be seen >> if you implement the rows parameter in efi_scroll_up() and try to boot >> a kernel with the default boot prompt, the "Loading <xyz>..." messages >> will be spaced apart with a ludicrous amount of spaces. Fixing this >> properly is outside of the scope of this task, so for now, I think it's >> okay to ignore the "rows" parameter in efi_scroll_up(), as ansicon >> appears to be the only place where it's used. >> >> >> Another related thing to keep in mind is that printf() and friends >> don't work in this early mode. That appears to be intentional though, >> and it only affects debugging messages. The printf() family tends >> to be enabled through openconsole(), and this happens once the ldlinux >> module is loaded, but in the bare syslinux.efi environment, the function >> is not present (trying to use it gives an undefined symbol error), >> though its behavior can thankfully be recreated pretty easily by just >> adding the following lines somewhere before you start using debugging >> statements with the printf() family of functions: >> >> close(1); >> close(2); >> opendev(&dev_null_r, &dev_stdcon_w, O_WRONLY); >> opendev(&dev_null_r, &dev_stdcon_w, O_WRONLY); >> >> >> I'm attaching a patch that updates the writechr() implementation >> for EFI by including a default color attribute of light gray on black, >> which matches what the BIOS implementation of writechr() does. >> >> The patch additionally also updates efi_write_char() to not change >> the color attribute unconditionally whenever a character is written, >> and fixes the CR LF order in efi_scroll_up(). >> >> A newline character is also added at the end of the ldlinux loading >> error message. This newline is added in efi_main() and not >> in load_env32() to be consistent with how it's implemented under BIOS. >> The load_env32() function is shared between EFI and BIOS, and under >> BIOS, the newline is also added as part of the following error message >> which tells the user that boot failed, e.g. the err_bootfailed string >> in core/diskfs.inc. >> >> >> Only the writechr() update and newline addition is strictly neccessary >> to fix the bug this bug report is about, but the other updates make >> the screen output process more efficient and improve the usability >> of the generated logs with text editors. >> >> >> With this patch, the error message about ldlinux is dispalyed correctly, >> informing the user about a potentially misconfigured setup, e.g. >> with the ldlinux.e32 / ldlinux.e64 file not being present. >> >> >> Best Regards, >> Marek >> >> [0] - https://bugs.debian.org/1103692 >> >> >> --- a/efi/console.c >> +++ b/efi/console.c >> @@ -34,7 +34,7 @@ void efi_console_restore(void) >> >> __export void writechr(char data) >> { >> - efi_write_char(data, 0); >> + efi_write_char(data, EFI_BACKGROUND_BLACK | EFI_LIGHTGRAY); >> } >> >> static inline EFI_STATUS open_protocol(EFI_HANDLE handle, EFI_GUID *protocol, >> --- a/efi/main.c >> +++ b/efi/main.c >> @@ -255,7 +255,10 @@ void efi_write_char(uint8_t ch, uint8_t >> SIMPLE_TEXT_OUTPUT_INTERFACE *out = ST->ConOut; >> uint16_t c[2]; >> >> - uefi_call_wrapper(out->SetAttribute, 2, out, attribute); >> + if (ST->ConOut->Mode->Attribute != attribute) >> + { >> + uefi_call_wrapper(out->SetAttribute, 2, out, attribute); >> + } >> >> /* Lookup primary Unicode encoding in the system codepage */ >> c[0] = codepage.uni[0][ch]; >> @@ -281,8 +284,8 @@ static void efi_set_cursor(int x, int y, >> >> static void efi_scroll_up(uint8_t cols, uint8_t rows, uint8_t attribute) >> { >> - efi_write_char('\n', 0); >> - efi_write_char('\r', 0); >> + efi_write_char('\r', attribute); >> + efi_write_char('\n', attribute); >> } >> >> static void efi_get_mode(int *cols, int *rows) >> @@ -1384,7 +1387,10 @@ EFI_STATUS efi_main(EFI_HANDLE image, EF >> } while (status == EFI_SUCCESS); >> >> if (!setjmp(load_error_buf)) >> + { >> load_env32(NULL); >> + writestr("\r\n"); >> + } >> >> /* load_env32() failed.. cancel timer and bailout */ >> status = cancel_timer(timer_ev); >> >> >> _______________________________________________ >> Syslinux mailing list >> Submissions to Syslinux at syslinux.org >> Unsubscribe or set options at: >> https://lists.syslinux.org/syslinux >>