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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

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