Re: [PATCH v3 1/3] multiboot2: parse vga= option when setting GOP mode

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 07, 2023 at 11:41:24AM +0200, Jan Beulich wrote:
> On 01.06.2023 15:05, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -329,7 +337,8 @@ __efi64_mb2_start:
> >  
> >          /*
> >           * efi_multiboot2() is called according to System V AMD64 ABI:
> > -         *   - IN:  %rdi - EFI ImageHandle, %rsi - EFI SystemTable.
> > +         *   - IN:  %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> > +         *          %rdx - MB2 cmdline
> >           */
> >          call    efi_multiboot2
> 
> All you obtain is a pointer to the string, which is fine from what I
> was able to establish, but that's not entirely obvious: The MBI
> structure used has a size field, so it could have been that the
> string isn't nul-terminated, and hence the size would also need
> passing. May I ask that this be mentioned at least in the description?

Sure.  The multiboot2 specification already states that the string
must be zero terminated.  It's my understanding the size field is part
of all the tags, so that a parser can find the next tag even if it
doesn't know how to parse the current one.

> > --- 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 first 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 )
> > +        return NULL;
> > +
> > +    while ( (s = strstr(s, opt)) != NULL )
> > +    {
> > +        if ( s == cmd || *(s - 1) == ' ' )
> 
> Iirc I had asked before: Not allowing for at least tab? (See
> cmdline.c:delim_chars_comma[] for what the non-EFI parsing permits,
> which in turn might be going a little too far especially with
> permitting comma as well.)

I've fixed this when parsing the gfx- option but not here, sorry.

> 
> > +        {
> > +            o = s + strlen(opt);
> 
> I don't think the comment ahead of the function describes this
> behavior, i.e. in particular the adding of the length of the
> option.

I've changed the comment to:

Return a pointer to the character after the first occurrence of opt in cmd.

Thanks, Roger.