[PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

Mon Jan 6 13:40:11 CET 2025

Hi Kever,

On Sun, 5 Jan 2025 at 23:47, Kever Yang <kever.yang at rock-chips.com> wrote:
>
> Hi Heinrich and Simon,
>
>      I send these two change because I want to fix the build error
> below(from[1]) which is for disable the DEBUG_UART[2].
> Do you have clear suggestion on how to do this in efi_stub, since I have
> no idea about this module, so I just follow how
> it works in other serial driver.
> +/binman/rom/intel-mrc (mrc.bin):
> +Image 'rom' has faked external blobs and is non-functional:
> descriptor.bin me.bin refcode.bin vga.bin mrc.bin
> +In file included from lib/efi/efi_stub.c:12:
> +include/debug_uart.h:205:9: error: stray '#' in program
> +  205 |         #warning "DEBUG_UART not defined!"
> +      |         ^
> +lib/efi/efi_stub.c:91:1: note: in expansion of macro 'DEBUG_UART_FUNCS'
> +   91 | DEBUG_UART_FUNCS
> +      | ^~~~~~~~~~~~~~~~
> +include/debug_uart.h:205:18: error: expected '=', ',', ';', 'asm' or
> '__attribute__' before string constant
> +      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~
> +lib/efi/efi_stub.c:86:13: error: '_debug_uart_putc' defined but not
> used [-Werror=unused-function]
> +   86 | static void _debug_uart_putc(int ch)
> +      |             ^~~~~~~~~~~~~~~~
> +make[3]: *** [scripts/Makefile.build:257: lib/efi/efi_stub.o] Error 1
> +make[2]: *** [scripts/Makefile.build:398: lib/efi] Error 2

Yes, I see what you are doing.

The real fix is probably to not use the debug UART in the stub. There
is a large comment:

 * EFI uses Unicode and we don't. The easiest way to get a sensible output
 * function is to use the U-Boot debug UART. We use EFI's console output
 * function where available, and assume the built-in UART after that. We rely
 * on EFI to set up the UART for us and just bring in the functions here.
 * This last bit is a bit icky, but it's only for debugging anyway. We could
 * build in ns16550.c with some effort, but this is a payload loader after
 * all.
 *
 * Note: We avoid using printf() so we don't need to bring in lib/vsprintf.c.
 * That would require some refactoring since we already build this for U-Boot.
 * Building an EFI shared library version would have to be a separate stem.
 * That might push us to using the SPL framework to build this stub. However
 * that would involve a round of EFI-specific changes in SPL. Worth
 * considering if we start needing more U-Boot functionality. Note that we
 * could then move get_codeseg32() to arch/x86/cpu/cpu.c.

We don't have a way of building the same file twice (once for the stub
and once for U-Boot). That would require my split-config series - Tom
rejected the last 10 patches (which actually did the work) a few years
ago[1]. But I do plan to get back to it.

For now, I suggest adding a new Kconfig, .e.g DEBUG_UART_DUMMY
(dependent on DEBUG_UART) which defines the dummy functions. Set it to
y by default, but n for the EFI-stub builds. Then you don't need to
enable the debug UART here.

Regards,
Simoon

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=341504&state=*

>
> Thanks,
> - Kever
> [1] https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/jobs/987462
> [2]
> https://patchwork.ozlabs.org/project/uboot/patch/20240918130155.299353-2-lukasz.czechowski@thaumatec.com/
>
> On 2025/1/5 07:02, Heinrich Schuchardt wrote:
> > Am 4. Januar 2025 20:29:43 MEZ schrieb Simon Glass <sjg at chromium.org>:
> >> Hi Heinrich,
> >>
> >> On Sat, 4 Jan 2025 at 05:16, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>> On 28.11.24 04:47, Kever Yang wrote:
> >>>> The efi_stub is useing DEBUG_UART interface by default, Enable it.
> >>> As Simon already wrote in a code comment the implementation of the EFI
> >>> stub is broken as it is not hardware agnostic.
> >> If I said that I was wrong, sorry. Actually that is the stub's intent,
> >> to take over the machine, exit boot-services and carry on. It is the
> >> EFI app which is designed to use boot services.
> >>
> >> So the stub cannot be hardware-agnostic, unfortunately. It only runs
> >> on x86 hardware, unless, for example, EFI has something like
> >> serial_getinfo() or we disable the UART / keyboard input.
> > I would assume that there is no serial console that the user has access to on many x86 systems (like on my laptop).
> >
> > In the stub itself we can use the Simple Text Output Protocol and the Simple Text Input Protocol offered by the UEFI implementation that started the stub.
> >
> > The UEFI specification defines a Serial Io Protocol which may be implemented but I would not know why we should restrict the usability in this way.
> >
> > Once we start U-Boot of course we will use hardware specific IO, e.g. on a RISC-V system use the debug console offered by the SBI
> >
> > Best regards
> >
> > Heinrich
> >
> >>> In the EFI stub we should never directly access hardware. Please, use
> >>> SimpleTextOutputProtocol.OutputString() for printing.
> >> Once the stub starts, boot services are gone. See efi_main() for how
> >> this works. There is a definite point at which we stop using that and
> >> start using U-Boot's own thing.
> >>
> >> At this point I think I may be misunderstanding what you said...
> >>
> >>>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
> >>>> ---
> >>>>
> >>>>    configs/efi-x86_payload32_defconfig | 1 +
> >>>>    configs/efi-x86_payload64_defconfig | 1 +
> >>>>    2 files changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/configs/efi-x86_payload32_defconfig b/configs/efi-x86_payload32_defconfig
> >>>> index 071ddb8e36d..f0b9acc358d 100644
> >>>> --- a/configs/efi-x86_payload32_defconfig
> >>>> +++ b/configs/efi-x86_payload32_defconfig
> >>>> @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload"
> >>>>    CONFIG_PRE_CON_BUF_ADDR=0x100000
> >>>>    CONFIG_VENDOR_EFI=y
> >>>>    CONFIG_TARGET_EFI_PAYLOAD=y
> >>>> +CONFIG_DEBUG_UART=y
> >>> On x86 this uses the NS16550 UART driver. This is fine as long as we
> >>> don't use the debug UART in the EFI stub. But correcting this is for a
> >>> separate patch.
> >>>
> >>> Having a debug UART available after the EFI stub makes sense.
> >>>
> >>> I will update the commit message when merging.
> >>>
> >>> Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >>>
> >>>>    CONFIG_EFI=y
> >>>>    CONFIG_EFI_STUB=y
> >>>>    CONFIG_FIT=y
> >>>> diff --git a/configs/efi-x86_payload64_defconfig b/configs/efi-x86_payload64_defconfig
> >>>> index 71612d7fb19..b02a861e59c 100644
> >>>> --- a/configs/efi-x86_payload64_defconfig
> >>>> +++ b/configs/efi-x86_payload64_defconfig
> >>>> @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload"
> >>>>    CONFIG_PRE_CON_BUF_ADDR=0x100000
> >>>>    CONFIG_VENDOR_EFI=y
> >>>>    CONFIG_TARGET_EFI_PAYLOAD=y
> >>>> +CONFIG_DEBUG_UART=y
> >>>>    CONFIG_EFI=y
> >>>>    CONFIG_EFI_STUB=y
> >>>>    CONFIG_EFI_STUB_64BIT=y
> >> Regards,
> >> Simon
> >