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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5150 bytes --]

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?

-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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