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.
