xen/efi: Use PrintErrMsg() rather than printk() in efi_exit_boot()

Commit Message

Julien Grall Feb. 8, 2022, 10:52 a.m. UTC

From: Julien Grall <[e-mail]>

The function efi_exit_boot() will be called within the UEFI stub. This
means printk() is not available will actually result to a crash when
called (at least on Arm).

Replace the call to printk() with PrintErrMsg().

Fixes: 49450415d6 ("efi: optionally call SetVirtualAddressMap()")
Signed-off-by: Julien Grall <[e-mail]>
---
 xen/common/efi/boot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich Feb. 8, 2022, 11:10 a.m. UTC | #1

On 08.02.2022 11:52, Julien Grall wrote:
> From: Julien Grall <[e-mail]>
> 
> The function efi_exit_boot() will be called within the UEFI stub. This
> means printk() is not available will actually result to a crash when
> called (at least on Arm).
> 
> Replace the call to printk() with PrintErrMsg().
> 
> Fixes: 49450415d6 ("efi: optionally call SetVirtualAddressMap()")
> Signed-off-by: Julien Grall <[e-mail]>

I think it was intentional to use printk() here, so I'd like to ask
for more details about the observed crash. That's also to try to
figure whether x86 would also be affected. The problem is that
without serial console configured in EFI, the output from
PrintErrMesg() is going to be very unlikely to actually be observable
(on the console), whereas the printk() output would at least be
retrievable by "xl dmesg" after the system is up.

What's worse though:

1) PrintErrMesg() invokes blexit() as the last thing. Yet we don't
   want to prevent Xen from booting; all we want is to disable use of
   runtime services.

2) I'm not convinced you can use StdErr anymore after ExitBootServices()
   was already called.

Jan

Julien Grall Feb. 8, 2022, 1:32 p.m. UTC | #2

Hi Jan,

On 08/02/2022 11:10, Jan Beulich wrote:
> On 08.02.2022 11:52, Julien Grall wrote:
>> From: Julien Grall <[e-mail]>
>>
>> The function efi_exit_boot() will be called within the UEFI stub. This
>> means printk() is not available will actually result to a crash when
>> called (at least on Arm).
>>
>> Replace the call to printk() with PrintErrMsg().
>>
>> Fixes: 49450415d6 ("efi: optionally call SetVirtualAddressMap()")
>> Signed-off-by: Julien Grall <[e-mail]>
> 
> I think it was intentional to use printk() here, so I'd like to ask
> for more details about the observed crash.
I have reproduced with the following diff:

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 8d65e9ce16ea..032e5ddf0c67 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1089,6 +1089,8 @@ static void __init efi_exit_boot(EFI_HANDLE 
ImageHandle, EFI_SYSTEM_TABLE *Syste
      if ( EFI_ERROR(status) )
          PrintErrMesg(L"Cannot exit boot services", status);

+    printk("Test\n");
+
  #ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
      for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
      {


And got:

Using modules provided by bootloader in FDT
Xen 4.17-unstable (c/s Mon Feb 7 21:14:25 2022 +0000 
git:4ecd67a592-dirty) EFI loader


Synchronous Exception at 0x000000000026BAF8

This is pointing to:

42sh> addr2line -e xen-syms 0x000000000026BAF8 
 

/home/julien/works/upstream/xen/xen/arch/arm/early_printk.c:21

If I disable early printk it seems to work. Hmmm... I think this might 
be related to the issue I posted a few years ago [1].

> That's also to try to
> figure whether x86 would also be affected.

It looks like my x86 setup is not boot using xen.efi. I will need to 
configure it for more testing.

> The problem is that
> without serial console configured in EFI, the output from
> PrintErrMesg() is going to be very unlikely to actually be observable
> (on the console), whereas the printk() output would at least be
> retrievable by "xl dmesg" after the system is up.
> 
> What's worse though:
> 
> 1) PrintErrMesg() invokes blexit() as the last thing. Yet we don't
>     want to prevent Xen from booting; all we want is to disable use of
>     runtime services.

Fair point.

> 
> 2) I'm not convinced you can use StdErr anymore after ExitBootServices()
>     was already called.

I think you are right. From "UEFI Specification, Version 2.9" page 226, 
StdErr should be set to NULL after ExitBootServices() succeeded.

Insterestingly, the EFI firmware I had was still happy to print afterwards.

Anyway, my long term plan for UEFI on Arm is to separate the EFI stub 
from Xen itself (similar to what Linux did). One of the main reason is 
to keep to interface between the two clean and it helps to enforce what 
is used by who.

Therefore, I think I would prefer to move the printk() to Xen (maybe 
runtime.c?). I will have a look as part of the work to support runtime 
services on Arm.

So I will park this patch for now.

Cheers,

[1] 
https://patches.linaro.org/project/Xen/patch/[e-mail]/

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 2bc38ae40fff..4ef75e472d29 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1181,8 +1181,8 @@  static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
                                           mdesc_ver, efi_memmap);
     if ( status != EFI_SUCCESS )
     {
-        printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), disabling runtime services\n",
-               status);
+        PrintErrMesg(L"EFI: SetVirtualAddressMap() failed, disabling runtime services",
+                     status);
         __clear_bit(EFI_RS, &efi_flags);
     }
 #endif