Re: [PATCH v2 00/14] efi/gop: Refactoring + mode-setting feature

[Arvind Sankar]

On Wed, Mar 25, 2020 at 05:41:43PM +0100, Ard Biesheuvel wrote:
> On Fri, 20 Mar 2020 at 03:00, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote:
> >
> > This series is against tip:efi/core.
> >
> > Patches 1-9 are small cleanups and refactoring of the code in
> > libstub/gop.c.
> >
> > The rest of the patches add the ability to use a command-line option to
> > switch the gop's display mode.
> >
> > The options supported are:
> > video=efifb:mode=n
> >         Choose a specific mode number
> > video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)]
> >         Specify mode by resolution and optionally color depth
> > video=efifb:auto
> >         Let the EFI stub choose the highest resolution mode available.
> >
> > The mode-setting additions increase code size of gop.o by about 3k on
> > x86-64 with EFI_MIXED enabled.
> >
> > Changes in v2 (HT lkp@xxxxxxxxx):
> > - Fix __efistub_global attribute to be after the variable.
> >   (NB: bunch of other places should ideally be fixed, those I guess
> >   don't matter as they are scalars?)
> > - Silence -Wmaybe-uninitialized warning in set_mode function.
> >
> 
> These look good to me. The only question I have is whether it would be
> possible to use the existing next_arg() and parse_option_str()
> functions to replace some of the open code parsing that goes on in
> patches 11 - 14.
> 

I don't think so -- next_arg is for parsing space-separated param=value
pairs, so efi_parse_options can use it, but it doesn't work for the
comma-separated options we'll have within the value.

parse_option_str would only work for the "auto" option, but it scans the
entire option string and just returns whether it was there or not, so it
wouldn't be too useful either, since we have to check for the other
possibilities anyway.

It would be nice to have a more generic library for cmdline parsing,
there are a lot of places that have to open-code option parsing like
this.

There's one thing I noticed while working at this, btw. The Makefile
specifies -ffreestanding, but at least x86 builds without having to
specify that. With -ffreestanding, the compiler doesn't optimize string
functions -- strlen(string literal) into a compile-time constant, for
eg. A couple hundred bytes or so can be saved by removing that option,
if it also works for ARM.