Re: [PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2023 18:02, Roger Pau Monné wrote:
> On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote:
>> On 31.03.2023 11:59, Roger Pau Monne wrote:
>>> Only set the GOP mode if vga is selected in the console option,
>>
>> This particular aspect of the behavior is inconsistent with legacy
>> boot behavior: There "vga=" isn't qualified by what "console=" has.
> 
> Hm, I find this very odd, why would we fiddle with the VGA (or the GOP
> here) if we don't intend to use it for output?

Because we also need to arrange for what Dom0 possibly wants to use.
It has no way of setting the mode the low-level (BIOS or EFI) way.

>>> otherwise just fetch the information from the current mode in order to
>>> make it available to dom0.
>>>
>>> Introduce support for passing the command line to the efi_multiboot2()
>>> helper, and parse the console= and vga= options if present.
>>>
>>> Add support for the 'gfx' and 'current' vga options, ignore the 'keep'
>>> option, and print a warning message about other options not being
>>> currently implemented.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> [...] 
>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -786,7 +786,30 @@ static bool __init 
>>> efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>>>  
>>>  static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN 
>>> size) { }
>>>  
>>> -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>>> *SystemTable)
>>> +/* Return the next occurrence of opt in cmd. */
>>> +static const char __init *get_option(const char *cmd, const char *opt)
>>> +{
>>> +    const char *s = cmd, *o = NULL;
>>> +
>>> +    if ( !cmd || !opt )
>>
>> I can see why you need to check "cmd", but there's no need to check "opt"
>> I would say.
> 
> Given this is executed without a page-fault handler in place I thought
> it was best to be safe rather than sorry, and avoid any pointer
> dereferences.

Hmm, I see. We don't do so elsewhere, though, I think.

>>> @@ -807,7 +830,60 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, 
>>> EFI_SYSTEM_TABLE *SystemTable
>>>  
>>>      if ( gop )
>>>      {
>>> -        gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
>>> +        const char *opt = NULL, *last = cmdline;
>>> +        /* Default console selection is "com1,vga". */
>>> +        bool vga = true;
>>> +
>>> +        /* For the console option the last occurrence is the enforced one. 
>>> */
>>> +        while ( (last = get_option(last, "console=")) != NULL )
>>> +            opt = last;
>>> +
>>> +        if ( opt )
>>> +        {
>>> +            const char *s = strstr(opt, "vga");
>>> +
>>> +            if ( !s || s > strpbrk(opt, " ") )
>>
>> Why strpbrk() and not the simpler strchr()? Or did you mean to also look
>> for tabs, but then didn't include \t here (and in get_option())? (Legacy
>> boot code also takes \r and \n as separators, btw, but I'm unconvinced
>> of the need.)
> 
> I was originally checking for more characters here and didn't switch
> when removing those.  I will add \t.
> 
>> Also aiui this is UB when the function returns NULL, as relational operators
>> (excluding equality ones) may only be applied when both addresses refer to
>> the same object (or to the end of an involved array).
> 
> Hm, I see, thanks for spotting. So I would need to do:
> 
> s > (strpbrk(opt, " ") ?: s)
> 
> So that we don't compare against NULL.
> 
> Also the original code was wrong AFAICT, as strpbrk() returning NULL
> should result in vga=true (as it would imply console= is the last
> option on the command line).

I'm afraid I'm in trouble what "original code" you're referring to here.
Iirc you really only add code to the function. And boot/cmdline.c has no
use of strpbrk() afaics.

>>> +                vga = false;
>>> +        }
>>> +
>>> +        if ( vga )
>>> +        {
>>> +            unsigned int width = 0, height = 0, depth = 0;
>>> +            bool keep_current = false;
>>> +
>>> +            last = cmdline;
>>> +            while ( (last = get_option(last, "vga=")) != NULL )
>>
>> It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c)
>> finds the first instance only. Normal command line handling respects the
>> last instance only. So while "vga=gfx-... vga=keep" will have the expected
>> effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be
>> less inflexible here, but I think this then wants accompanying by an update
>> to the command line doc, no matter that presently it doesn't really
>> describe these peculiarities.
> 
> But if we then describe this behavior in the documentation people
> could rely on it.  Right now this is just an implementation detail (or
> a bug I would say), and that would justify fixing boot/cmdline.c to
> also respect the last instance only.

Yes, fixing the non-EFI code is certainly an option (and then describing
the hopefully consistent result in the doc).

Jan

>> Otoh it would end up being slightly cheaper
>> to only look for the first instance here as well. In particular ...
>>
>>> +            {
>>> +                if ( !strncmp(last, "gfx-", 4) )
>>> +                {
>>> +                    width = simple_strtoul(last + 4, &last, 10);
>>> +                    if ( *last == 'x' )
>>> +                        height = simple_strtoul(last + 1, &last, 10);
>>> +                    if ( *last == 'x' )
>>> +                        depth = simple_strtoul(last + 1, &last, 10);
>>> +                    /* Allow depth to be 0 or unset. */
>>> +                    if ( !width || !height )
>>> +                        width = height = depth = 0;
>>> +                    keep_current = false;
>>> +                }
>>> +                else if ( !strncmp(last, "current", 7) )
>>> +                    keep_current = true;
>>> +                else if ( !strncmp(last, "keep", 4) )
>>> +                {
>>> +                    /* Ignore. */
>>> +                }
>>> +                else
>>> +                {
>>> +                    /* Fallback to defaults if unimplemented. */
>>> +                    width = height = depth = 0;
>>> +                    keep_current = false;
>>
>> ... this zapping of what was successfully parsed before would then not be
>> needed in any event (else I would question why this is necessary).
> 
> Hm, I don't have a strong opinion, the expected behavior I have of
> command line options is that the last one is the one that takes
> effect, but it would simplify the change if we only cared about the
> first one, albeit that's an odd behavior.
> 
> My preference would be to leave the code here respecting the last
> instance only, and attempt to fix boot/cmdline.c so it does the same.
> 
> Thanks, Roger.