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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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