Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.02.2022 20:28, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Currently, the EFI stub will only query the console information and
> get the GOP when using the configuration file.
> 
> However, GRUB is never providing the a configuration file. So the
> EFI framebuffer will not be usable at least on Arm (support will
> be added in a follow-up patch).
> 
> Move out the code outside of the configuration file section.
> 
> Take the opportunity to remove the variable 'size' which was
> set but never used (interestingly GCC is only complaining if it is
> initialization when declaring the variable).
> 
> With this change, GCC 8.3 will complain of argc potentially been
> used unitiatlized. I suspect this is because the argc will
> be iniitalized and used in a different if code-blocks. Yet they
> are using the same check.

I'm inclined to suggest this wants to be a separate change, with its
own justification. You're not touching any use of argc here, after
all.

> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> ----
> 
> It is not entirely clear to me why the GOP was only fetched when
> the configuration file is used.
> 
> I have tested this on RPI4 and it seems to work. Any chance this
> was done to workaround an x86 platform?

This was done so in the context of making the code work for Arm. See
commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
when booted from GRUB"), the description of which explicitly says

"Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
 code does some console and video initialization to support native EFI boot from
 the EFI boot manager or EFI shell.  This initlization should not be done when
 booted using GRUB."

What you say now is effectively the opposite (and unlike back then
x86 is now able to use this code path as well, so needs considering
too). Cc-ing Daniel for possibly having a GrUB-side opinion.

Jan

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1129,9 +1129,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>      static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
>      EFI_LOADED_IMAGE *loaded_image;
>      EFI_STATUS status;
> -    unsigned int i, argc;
> +    /* Initialize argc to stop GCC complaining */
> +    unsigned int i, argc = 0;
>      CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
> -    UINTN gop_mode = ~0;
> +    UINTN gop_mode = ~0, cols = 0, rows = 0;
> +
>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>      union string section = { NULL }, name;
> @@ -1219,18 +1221,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  
>      efi_arch_relocate_image(0);
>  
> +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> +                           &cols, &rows) == EFI_SUCCESS )
> +        efi_arch_console_init(cols, rows);
> +
> +    gop = efi_get_gop();
> +
>      if ( use_cfg_file )
>      {
>          EFI_FILE_HANDLE dir_handle;
> -        UINTN depth, cols, rows, size;
> -
> -        size = cols = rows = depth = 0;
> -
> -        if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> -                               &cols, &rows) == EFI_SUCCESS )
> -            efi_arch_console_init(cols, rows);
> -
> -        gop = efi_get_gop();
> +        UINTN depth = 0;
>  
>          /* Get the file system interface. */
>          dir_handle = get_parent_handle(loaded_image, &file_name);