[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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 
            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)
                   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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.