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

[Jan Beulich <JBeulich@xxxxxxxx>]

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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel