Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, May 22, 2018 at 6:24 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 21.05.18 at 18:59, <tamas@xxxxxxxxxxxxx> wrote:
>> After closer inspection the problem is with the following line here:
>>
>>>>          for ( i = 1; i < argc; ++i )
>>
>> This assumes that argv[0] is the EFI executable filename, which is not
>> true when EFI_LOAD_OPTION is used. That's why in my v3 patch I had the
>> "elo_active" variable to determine whether to start the iteration from
>> 0 or from 1.
>
> How about this one then?
>

Success! Although I have to say it's pretty hard to follow the code
and what it's actually doing. Perhaps adding more comments would be
helpful.

Tamas

>
>
> EFI: add EFI_LOAD_OPTION support
>
> When booting Xen via UEFI the Xen config file can contain multiple
> sections each describing different boot options. It is currently only
> possible to choose which section to boot with if the buffer contains a
> string. UEFI provides a different standard to pass optional arguments
> to an application, and in this patch we make Xen properly parse this
> buffer, thus making it possible to have separate EFI boot options
> present for the different config sections.
>
> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v4: Address my own review comments.
>
> --- unstable.orig/xen/common/efi/boot.c
> +++ unstable/xen/common/efi/boot.c
> @@ -88,6 +88,14 @@ typedef struct _EFI_APPLE_PROPERTIES {
>      EFI_APPLE_PROPERTIES_GETALL GetAll;
>  } EFI_APPLE_PROPERTIES;
>
> +typedef struct _EFI_LOAD_OPTION {
> +    UINT32 Attributes;
> +    UINT16 FilePathListLength;
> +    CHAR16 Description[];
> +} EFI_LOAD_OPTION;
> +
> +#define LOAD_OPTION_ACTIVE              0x00000001
> +
>  union string {
>      CHAR16 *w;
>      char *s;
> @@ -275,6 +283,16 @@ static int __init wstrncmp(const CHAR16
>      return n ? *s1 - *s2 : 0;
>  }
>
> +static const CHAR16 *__init wmemchr(const CHAR16 *s, CHAR16 c, UINTN n)
> +{
> +    while ( n && *s != c )
> +    {
> +        --n;
> +        ++s;
> +    }
> +    return n ? s : NULL;
> +}
> +
>  static CHAR16 *__init s2w(union string *str)
>  {
>      const char *s = str->s;
> @@ -374,14 +392,58 @@ static void __init PrintErrMesg(const CH
>  }
>
>  static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> -                                    CHAR16 *cmdline, UINTN cmdsize,
> +                                    VOID *data, UINTN size, UINTN *offset,
>                                      CHAR16 **options)
>  {
> -    CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL;
> +    CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
>      bool prev_sep = true;
>
> -    for ( ; cmdsize > sizeof(*cmdline) && *cmdline;
> -            cmdsize -= sizeof(*cmdline), ++cmdline )
> +    if ( argc )
> +    {
> +        cmdline = data + *offset;
> +        /* EFI_LOAD_OPTION does not supply an image name as first component. 
> */
> +        if ( *offset )
> +            *argv++ = NULL;
> +    }
> +    else if ( size > sizeof(*cmdline) && !(size % sizeof(*cmdline)) &&
> +              (wmemchr(data, 0, size / sizeof(*cmdline)) ==
> +               data + size - sizeof(*cmdline)) )
> +    {
> +        *offset = 0;
> +        cmdline = data;
> +    }
> +    else if ( size > sizeof(EFI_LOAD_OPTION) )
> +    {
> +        const EFI_LOAD_OPTION *elo = data;
> +        /* The minimum size the buffer needs to be. */
> +        size_t elo_min = offsetof(EFI_LOAD_OPTION, Description[1]) +
> +                         elo->FilePathListLength;
> +
> +        if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size > elo_min &&
> +             !((size - elo_min) % sizeof(*cmdline)) )
> +        {
> +            const CHAR16 *desc = elo->Description;
> +            const CHAR16 *end = wmemchr(desc, 0,
> +                                        (size - elo_min) / sizeof(*desc) + 
> 1);
> +
> +            if ( end )
> +            {
> +                *offset = elo_min + (end - desc) * sizeof(*desc);
> +                if ( (size -= *offset) > sizeof(*cmdline) )
> +                {
> +                    cmdline = data + *offset;
> +                    /* Cater for the image name as first component. */
> +                    ++argc;
> +                }
> +            }
> +        }
> +    }
> +
> +    if ( !cmdline )
> +        return 0;
> +
> +    for ( ; size > sizeof(*cmdline) && *cmdline;
> +            size -= sizeof(*cmdline), ++cmdline )
>      {
>          bool cur_sep = *cmdline == L' ' || *cmdline == L'\t';
>
> @@ -1095,15 +1157,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>
>      if ( use_cfg_file )
>      {
> +        UINTN offset = 0;
> +
>          argc = get_argv(0, NULL, loaded_image->LoadOptions,
> -                        loaded_image->LoadOptionsSize, NULL);
> +                        loaded_image->LoadOptionsSize, &offset, NULL);
>          if ( argc > 0 &&
>               efi_bs->AllocatePool(EfiLoaderData,
>                                    (argc + 1) * sizeof(*argv) +
>                                        loaded_image->LoadOptionsSize,
>                                    (void **)&argv) == EFI_SUCCESS )
>              get_argv(argc, argv, loaded_image->LoadOptions,
> -                     loaded_image->LoadOptionsSize, &options);
> +                     loaded_image->LoadOptionsSize, &offset, &options);
>          else
>              argc = 0;
>          for ( i = 1; i < argc; ++i )
> @@ -1244,6 +1308,15 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>              efi_bs->FreePool(name.w);
>          }
>
> +        if ( argc && !*argv )
> +        {
> +            EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
> +                                                       &file_name);
> +
> +            handle->Close(handle);
> +            *argv = file_name;
> +        }
> +
>          name.s = get_value(&cfg, section.s, "options");
>          efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s);
>

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