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

[Julien Grall <julien@xxxxxxx>]

Hi Daniel,

Gentle ping. Your feedback on this approach would be helpful.

On 07/02/2022 08:46, Jan Beulich wrote:
> 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);
Cheers,

--
Julien Grall