Re: [Xen-devel] [PATCH] EFI: add efi=mapbs option and parse efi= early

[Jan Beulich <JBeulich@xxxxxxxx>]

On 08.08.2019 02:31, Marek Marczykowski-Górecki  wrote:
> When booting Xen via xen.efi, there is /mapbs option to workaround
> certain platform issues (added in f36886bdf4 "EFI/early: add /mapbs to
> map EfiBootServices{Code,Data}"). Add support for efi=mapbs on Xen
> cmdline for the same effect and parse it very early in the
> multiboot2+EFI boot path.
> 
> Normally cmdline is parsed after relocating MB2 structure, which happens
> too late. To have efi= parsed early enough, save cmdline pointer in
> head.S and pass it as yet another argument to efi_multiboot2(). This
> way we avoid introducing yet another MB2 structure parser.

I can where you're coming from here, but I'm not at all happy to
see the amount of assembly code further grow.

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -886,7 +886,7 @@ disable it (edid=no). This option should not normally be 
> required
>   except for debugging purposes.
>   
>   ### efi
> -    = List of [ rs=<bool>, attr=no|uc ]
> +    = List of [ rs=<bool>, attr=no|uc, mapbs=<bool> ]
>   
>   Controls for interacting with the system Extended Firmware Interface.
>   
> @@ -899,6 +899,10 @@ Controls for interacting with the system Extended 
> Firmware Interface.
>       leave the memory regions unmapped, while `attr=uc` will map them as 
> fully
>       uncacheable.
>   
> +*   The `mapbs=` boolean controls whether EfiBootServices{Code,Data} should
> +    remain mapped after Exit() BootServices call. By default those memory 
> regions
> +    will not be mapped after Exit() BootServices call.

There are restrictions necessary (see below) which should be
mentioned here imo.

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -315,8 +315,10 @@ static void __init efi_arch_handle_cmdline(CHAR16 
> *image_name,
>           name.s = "xen";
>       place_string(&mbi.cmdline, name.s);
>   
> -    if ( mbi.cmdline )
> +    if ( mbi.cmdline ) {
>           mbi.flags |= MBI_CMDLINE;
> +        efi_early_parse_cmdline(mbi.cmdline);
> +    }

Why? This is the xen.efi boot path, isn't it? (Also, if this
change was to stay, the opening brace would need to go on its
own line.)

> @@ -685,6 +688,9 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, 
> EFI_SYSTEM_TABLE *SystemTable
>   
>       efi_init(ImageHandle, SystemTable);
>   
> +    if (cmdline)
> +        efi_early_parse_cmdline(cmdline);

Style again (missing blanks in if()).

> @@ -1412,16 +1417,32 @@ static int __init parse_efi_param(const char *s)
>              else
>                  rc = -EINVAL;
>          }
> +        else if ( (val = parse_boolean("mapbs", s, ss)) >= 0 )
> +        {
> +            map_bs = val;
> +        }

This may _not_ be accepted when called the "normal" way, since it
would then fail to affect efi_arch_process_memory_map(), but it
would affect efi_init_memory(). I therefore think you don't want
to call this function from efi_early_parse_cmdline(), and instead
simply ignore the option here.

Also (again if for some reason the change was to stay as is) -
stray braces.

>          else
>              rc = -EINVAL;
>  
>          s = ss + 1;
> -    } while ( *ss );
> +        /*
> +         * End parsing on both '\0' and ' ',
> +         * to make efi_early_parse_cmdline simpler.
> +         */
> +    } while ( *ss && *ss != ' ');
>   
>      return rc;
>  }
>  custom_param("efi", parse_efi_param);
>   
> +/* Extract efi= param early in the boot */
> +static void __init efi_early_parse_cmdline(const char *cmdline)
> +{
> +    const char *efi_opt = strstr(cmdline, "efi=");
> +    if ( efi_opt )

Blank line missing above here.

> +        parse_efi_param(efi_opt + 4);
> +}

What about multiple "efi=" on the command line? And what about
a (currently bogus) "abcefi=" on the command line, or yet some
other pattern wrongly matching the string you search for?

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel