| Message ID | 20190808003158.5256-1-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EFI: add efi=mapbs option and parse efi= early | expand |
On Thu, Aug 08, 2019 at 02:31:57AM +0200, 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. > > To keep consistency, handle efi= parameter early in xen.efi too, both in > xen.efi command line and cfg file. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > docs/misc/xen-command-line.pandoc | 6 +++++- > xen/arch/x86/boot/head.S | 21 ++++++++++++++++++--- > xen/arch/x86/efi/efi-boot.h | 10 ++++++++-- > xen/arch/x86/x86_64/asm-offsets.c | 1 + > xen/common/efi/boot.c | 23 ++++++++++++++++++++++- > 5 files changed, 54 insertions(+), 7 deletions(-) > ... > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index 7a13a30bc0..df5e98e6bc 100644 > --- 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); Compiler complains here, because mbi.cmdline is u32 (int vs pointer, and also a different size). What is the proper way to make compiler happy here? "(const char *)(uint64_t)" doesn't seems right.
On 08.08.2019 02:52, Marek Marczykowski-Górecki wrote: >> --- 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); > > Compiler complains here, because mbi.cmdline is u32 (int vs pointer, and > also a different size). What is the proper way to make compiler happy > here? "(const char *)(uint64_t)" doesn't seems right. Well, if the conversion to a pointer is correct here (i.e. the specified address has a mapping), then (const char*)(unsigned long) would be the canonical way to go; using uintptr_t would also be an option, but we make pretty little use of it. Jan
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
On Thu, Aug 08, 2019 at 08:21:54AM +0000, Jan Beulich wrote: > 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. I need to add some anyway, because otherwise efi_multiboot2() don't have pointer to MB2 structure. But yes, it would probably be less new asm code. Just to be clear: do you prefer third MB2 parser instead of adding this into the one in head.S? > > --- 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? Yes, as explained in commit message, this is to make it less confusing what option can be used when. To say "efi=mapbs does X" instead of "efi=mapbs does X, but only if Y, Z and K". > (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(). What do you mean? Have I missed some EFI boot code path? Where it would miss to affect efi_arch_process_memory_map() ? > 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? Good points, I'll extend this function. Unless you can suggest some existing function that could be used this early instead?
On 08/08/2019 01: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. > > To keep consistency, handle efi= parameter early in xen.efi too, both in > xen.efi command line and cfg file. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> I'm very sorry to do this to you, but I'm going to object to the patch (in principle. I think the command line option itself is fine but...) What does Linux do differently here? It is actively damaging to the Xen community to users to force users tweak command lines in order to boot/recover their system, and it looks especially embarrassing when other OSes cope automatically. We have compatibility for all kinds of other firmware screw-ups, except EFI it seems, and this needs to change. So while I have no objection to the option per say, I don't think this patch is reasonable as a "fix" to the problem as far as end users are concerned. ~Andrew
On Thu, Aug 08, 2019 at 10:40:36AM +0100, Andrew Cooper wrote: > On 08/08/2019 01: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. > > > > To keep consistency, handle efi= parameter early in xen.efi too, both in > > xen.efi command line and cfg file. > > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > I'm very sorry to do this to you, but I'm going to object to the patch > (in principle. I think the command line option itself is fine but...) > > What does Linux do differently here? I've found this comment: /* * The UEFI specification makes it clear that the operating system is * free to do whatever it wants with boot services code after * ExitBootServices() has been called. Ignoring this recommendation a * significant bunch of EFI implementations continue calling into boot * services code (SetVirtualAddressMap). In order to work around such * buggy implementations we reserve boot services region during EFI * init and make sure it stays executable. Then, after * SetVirtualAddressMap(), it is discarded. * * However, some boot services regions contain data that is required * by drivers, so we need to track which memory ranges can never be * freed. This is done by tagging those regions with the * EFI_MEMORY_RUNTIME attribute. * * Any driver that wants to mark a region as reserved must use * efi_mem_reserve() which will insert a new EFI memory descriptor * into efi.memmap (splitting existing regions if necessary) and tag * it with EFI_MEMORY_RUNTIME. */ So, for start, Linux has "/mapbs" enabled by default. But as you see in the other thread, it isn't enough. Given the above comment, I more and more think it is also about SetVirtualAddressMap() call. According to a git log, SetVirtualAddressMap is not called in Xen, as it's incompatible with kexec (can be called only once). Looking at Linux code, this is worked around by passing efi memory regions from the old map to the new one - at least this is my understanding of kexec_enter_virtual_mode() in arch/x86/platform/efi/efi.c. For example this comment: /* * Map efi regions which were passed via setup_data. The virt_addr is a * fixed addr which was used in first kernel of a kexec boot. */ For my use case, I don't care about kexec, so I'd happily enable SetVirtualAddressMap() call (it's under #ifdef), but according to comments and that it wasn't touched since 2011, I don't expect it work anymore. > It is actively damaging to the Xen community to users to force users > tweak command lines in order to boot/recover their system, and it looks > especially embarrassing when other OSes cope automatically. We have > compatibility for all kinds of other firmware screw-ups, except EFI it > seems, and this needs to change. I _guess_ calling SetVirtualAddressMap() would help here, but it is much more complex change than I'd like to do now. What do you think about adding this option _and_ a heuristic when to enable it automatically? In this case I'd say to start with vendor=Lenovo based on my experience... > So while I have no objection to the option per say, I don't think this > patch is reasonable as a "fix" to the problem as far as end users are > concerned. > > ~Andrew
On 08.08.2019 11:40, Andrew Cooper wrote: > On 08/08/2019 01: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. >> >> To keep consistency, handle efi= parameter early in xen.efi too, both in >> xen.efi command line and cfg file. >> >> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > I'm very sorry to do this to you, but I'm going to object to the patch > (in principle. I think the command line option itself is fine but...) > > What does Linux do differently here? > > It is actively damaging to the Xen community to users to force users > tweak command lines in order to boot/recover their system, and it looks > especially embarrassing when other OSes cope automatically. We have > compatibility for all kinds of other firmware screw-ups, except EFI it > seems, and this needs to change. > > So while I have no objection to the option per say, I don't think this > patch is reasonable as a "fix" to the problem as far as end users are > concerned. And your suggestion then is to always map (and not use) all boot services memory, contrary to all intentions of the EFI spec? As to your more general rant: I think it is wrong to have more than very simple and low overhead workarounds enabled "just in case". The vast majority of the workarounds we have in place are keyed to specific versions of something, or e.g. DMI properties of systems. I certainly wouldn't mind DMI based enabling of workarounds like the one here, but I'm going to continue to push back on attempts to cripple the EFI code just because _some_ systems don't have a sensible EFI implementation. Jan
On 08.08.2019 11:21, Marek Marczykowski-Górecki wrote: > On Thu, Aug 08, 2019 at 08:21:54AM +0000, Jan Beulich wrote: >> 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. > > I need to add some anyway, because otherwise efi_multiboot2() don't have > pointer to MB2 structure. But yes, it would probably be less new asm > code. Just to be clear: do you prefer third MB2 parser instead of adding > this into the one in head.S? No, I don't. I'm not happy about either variant. Looking at the code I can't help thinking that it shouldn't be overly difficult to have mbi_reloc2() peek into the command line as it relocates it. head.S would simply need to pass in the address of opt_map_bs (or a suitable intermediate variable / structure) as it seems. >>> --- 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? > > Yes, as explained in commit message, this is to make it less confusing > what option can be used when. To say "efi=mapbs does X" instead of > "efi=mapbs does X, but only if Y, Z and K". Otoh it is going to be confusing why there are two ways of setting this with xen.efi. >>> @@ -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(). > > What do you mean? Have I missed some EFI boot code path? Where it would > miss to affect efi_arch_process_memory_map() ? I'm implying the change to efi_arch_handle_cmdline() above to not be there. And I'm also considering the case where, due to some oversight (like is the case here as mentioned in other places), the two command line processing steps potentially produce different results (the example with the current code would be "efi=no-rs efi=mapbs"). Jan
On Thu, Aug 08, 2019 at 03:25:19PM +0000, Jan Beulich wrote: > On 08.08.2019 11:21, Marek Marczykowski-Górecki wrote: > > On Thu, Aug 08, 2019 at 08:21:54AM +0000, Jan Beulich wrote: > >> 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. > > > > I need to add some anyway, because otherwise efi_multiboot2() don't have > > pointer to MB2 structure. But yes, it would probably be less new asm > > code. Just to be clear: do you prefer third MB2 parser instead of adding > > this into the one in head.S? > > No, I don't. I'm not happy about either variant. Looking at the > code I can't help thinking that it shouldn't be overly difficult > to have mbi_reloc2() peek into the command line as it relocates > it. head.S would simply need to pass in the address of opt_map_bs > (or a suitable intermediate variable / structure) as it seems. I was considering that too, but unfortunately mbi_reloc2() is too late. It's after efi_multiboot2() which needs the parameter parsed already. > >>> --- 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? > > > > Yes, as explained in commit message, this is to make it less confusing > > what option can be used when. To say "efi=mapbs does X" instead of > > "efi=mapbs does X, but only if Y, Z and K". > > Otoh it is going to be confusing why there are two ways of setting > this with xen.efi. TBH I think it's more confusing that /mapbs can be specified only on xen.efi cmdline, but for example efi=no-rs can be used on both xen.efi cmdline and normal xen options. Once efi= is parsed early, I would consider deprecating xen.efi specific options (you can use efi= there already) and move other xen.efi specific options as efi=. > >>> @@ -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(). > > > > What do you mean? Have I missed some EFI boot code path? Where it would > > miss to affect efi_arch_process_memory_map() ? > > I'm implying the change to efi_arch_handle_cmdline() above to > not be there. And I'm also considering the case where, due to > some oversight (like is the case here as mentioned in other > places), the two command line processing steps potentially > produce different results (the example with the current code > would be "efi=no-rs efi=mapbs"). Yes, buggy handling multiple efi= or other cases you mentioned is a bug that needs to be fixed. But I don't think it should prevent _unifying_ EFI options handling. Now (without this patch) we have some EFI options that can be utilized only in some EFI boot paths. This is IMO bigger issue. Anyway, your concern about map_bs being set only later can be solved by parsing relevant efi= options early _only_ (in efi_early_parse_cmdline() directly) and ignore them in parse_efi_param(). What do you think?
On 08.08.2019 18:08, Marek Marczykowski-Górecki wrote: > On Thu, Aug 08, 2019 at 03:25:19PM +0000, Jan Beulich wrote: >> On 08.08.2019 11:21, Marek Marczykowski-Górecki wrote: >>> On Thu, Aug 08, 2019 at 08:21:54AM +0000, Jan Beulich wrote: >>>> On 08.08.2019 02:31, Marek Marczykowski-Górecki wrote: >>>>> --- 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? >>> >>> Yes, as explained in commit message, this is to make it less confusing >>> what option can be used when. To say "efi=mapbs does X" instead of >>> "efi=mapbs does X, but only if Y, Z and K". >> >> Otoh it is going to be confusing why there are two ways of setting >> this with xen.efi. > > TBH I think it's more confusing that /mapbs can be specified only on > xen.efi cmdline, but for example efi=no-rs can be used on both xen.efi > cmdline and normal xen options. Once efi= is parsed early, I would > consider deprecating xen.efi specific options (you can use efi= there > already) and move other xen.efi specific options as efi=. That's an option, yes. >>>>> @@ -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(). >>> >>> What do you mean? Have I missed some EFI boot code path? Where it would >>> miss to affect efi_arch_process_memory_map() ? >> >> I'm implying the change to efi_arch_handle_cmdline() above to >> not be there. And I'm also considering the case where, due to >> some oversight (like is the case here as mentioned in other >> places), the two command line processing steps potentially >> produce different results (the example with the current code >> would be "efi=no-rs efi=mapbs"). > > Yes, buggy handling multiple efi= or other cases you mentioned is a bug > that needs to be fixed. But I don't think it should prevent _unifying_ > EFI options handling. Now (without this patch) we have some EFI options > that can be utilized only in some EFI boot paths. This is IMO bigger > issue. > > Anyway, your concern about map_bs being set only later can be solved by > parsing relevant efi= options early _only_ (in efi_early_parse_cmdline() > directly) and ignore them in parse_efi_param(). What do you think? The "normal" cmdline parsing time invocation should then become a no-op if at all possible, i.e. I would still advocate for there being exactly one time where "efi=" gets actually parsed. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 7c72e31032..b2dfd64b48 100644 --- 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. + ### ept > `= List of [ ad=<bool>, pml=<bool> ]` diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index d78bed394a..b13f46c70e 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -224,9 +224,10 @@ __efi64_mb2_start: jmp x86_32_switch .Lefi_multiboot2_proto: - /* Zero EFI SystemTable and EFI ImageHandle addresses. */ + /* Zero EFI SystemTable, EFI ImageHandle and cmdline addresses. */ xor %esi,%esi xor %edi,%edi + xor %rdx,%rdx /* Skip Multiboot2 information fixed part. */ lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx @@ -241,7 +242,7 @@ __efi64_mb2_start: /* Are EFI boot services available? */ cmpl $MULTIBOOT2_TAG_TYPE_EFI_BS,MB2_tag_type(%rcx) - jne .Lefi_mb2_st + jne .Lefi_mb2_cmdline /* We are on EFI platform and EFI boot services are available. */ incb efi_platform(%rip) @@ -253,6 +254,14 @@ __efi64_mb2_start: incb skip_realmode(%rip) jmp .Lefi_mb2_next_tag +.Lefi_mb2_cmdline: + /* Get cmdline address from Multiboot2 information. */ + cmpl $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx) + jne .Lefi_mb2_st + + lea MB2_string(%rcx),%rdx + jmp .Lefi_mb2_next_tag + .Lefi_mb2_st: /* Get EFI SystemTable address from Multiboot2 information. */ cmpl $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx) @@ -264,6 +273,11 @@ __efi64_mb2_start: cmove MB2_efi64_ih(%rcx),%rdi je .Lefi_mb2_next_tag + /* Get cmdline address from Multiboot2 information. */ + cmpl $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx) + cmove MB2_efi64_ih(%rcx),%rdi + je .Lefi_mb2_next_tag + /* Is it the end of Multiboot2 information? */ cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) je .Lrun_bs @@ -327,7 +341,8 @@ __efi64_mb2_start: /* * efi_multiboot2() is called according to System V AMD64 ABI: - * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable. + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, + * %rdx - MB2 cmdline. */ call efi_multiboot2 diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 7a13a30bc0..df5e98e6bc 100644 --- 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); + } /* * These must not be initialized statically, since the value must * not get relocated when processing base relocations later. @@ -675,7 +677,8 @@ 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) +void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, + const char *cmdline) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; UINTN cols, gop_mode = ~0, rows; @@ -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); + efi_console_set_mode(); if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c index 33930ce97c..b5be6ca30f 100644 --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -163,6 +163,7 @@ void __dummy__(void) OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower); OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer); OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer); + OFFSET(MB2_string, multiboot2_tag_string_t, string); BLANK(); DEFINE(l2_identmap_sizeof, sizeof(l2_identmap)); diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 79193784ff..a8514bbb5f 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -126,6 +126,7 @@ static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, static size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); +static void efi_early_parse_cmdline(const char *cmdline); static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); static void efi_console_set_mode(void); @@ -1386,6 +1387,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) static bool __initdata efi_map_uc; +/* + * Warning: this function must be idempotent as it may be called multiple + * times. + */ static int __init parse_efi_param(const char *s) { const char *ss; @@ -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; + } 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 ) + parse_efi_param(efi_opt + 4); +} + #ifndef USE_SET_VIRTUAL_ADDRESS_MAP static __init void copy_mapping(unsigned long mfn, unsigned long end, bool (*is_valid)(unsigned long smfn,
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. To keep consistency, handle efi= parameter early in xen.efi too, both in xen.efi command line and cfg file. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- docs/misc/xen-command-line.pandoc | 6 +++++- xen/arch/x86/boot/head.S | 21 ++++++++++++++++++--- xen/arch/x86/efi/efi-boot.h | 10 ++++++++-- xen/arch/x86/x86_64/asm-offsets.c | 1 + xen/common/efi/boot.c | 23 ++++++++++++++++++++++- 5 files changed, 54 insertions(+), 7 deletions(-)