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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
