On Thu, Jul 06, 2023 at 12:41:58PM +0200, Jan Beulich wrote:
> On 05.07.2023 13:47, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -795,7 +795,30 @@ static bool __init
> > efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
> >
> > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN
> > size) { }
> >
> > -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> > *SystemTable)
> > +/* Return a pointer to the character after the first occurrence of opt in
> > cmd */
> > +static const char __init *get_option(const char *cmd, const char *opt)
>
> Nit: __init and * want to change places.
Hm, yes. I assume that placing it before the return type is not OK?
(static const __init char ...)
> > @@ -816,7 +839,54 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle,
> > EFI_SYSTEM_TABLE *SystemTable
> >
> > if ( gop )
> > {
> > - gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> > + const char *cur = cmdline;
> > + unsigned int width = 0, height = 0, depth = 0;
> > + bool keep_current = false;
> > +
> > + while ( (cur = get_option(cur, "vga=")) != NULL )
> > + {
> > +#define VALID_TERMINATOR(c) \
> > + (*(c) == ' ' || *(c) == '\t' || *(c) == '\0' || *(c) == ',')
> > + if ( !strncmp(cur, "gfx-", 4) )
> > + {
> > + width = simple_strtoul(cur + 4, &cur, 10);
> > +
> > + if ( *cur == 'x' )
> > + height = simple_strtoul(cur + 1, &cur, 10);
> > + else
> > + goto error;
> > +
> > + if ( *cur == 'x' )
> > + depth = simple_strtoul(cur + 1, &cur, 10);
> > + else
> > + goto error;
> > +
> > + if ( !VALID_TERMINATOR(cur) )
> > + {
> > +error:
>
> Nit: Labels want to be indented by at least one blank. Here I'm
> inclined to suggest indenting to the level of the enclosing curly
> braces.
>
> > + PrintStr(L"Warning: Invalid gfx- option
> > detected.\r\n");
>
> Maybe better PrintErr() and no trailing full stop?
Yes, please adjust if you don't mind.
>
> > + width = height = depth = 0;
> > + }
> > + keep_current = false;
> > + }
> > + else if ( !strncmp(cur, "current", 7) && VALID_TERMINATOR(cur
> > + 7) )
> > + keep_current = true;
> > + else if ( !strncmp(cur, "keep", 4) && VALID_TERMINATOR(cur +
> > 4) )
> > + {
> > + /* Ignore, handled in later vga= parsing. */
> > + }
> > + else
> > + {
> > + /* Fallback to defaults if unimplemented. */
> > + width = height = depth = 0;
> > + keep_current = false;
> > + PrintStr(L"Warning: Cannot use selected vga option.\r\n");
>
> Same here then?
>
> With these addressed (which are all mechanical and hence can probably
> be done while committing, as long as we can reach agreement)
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
LGTM, please adjust if you don't mind, otherwise I can send an
adjusted version.
Thanks, Roger.
