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

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



On Thu, May 17, 2018 at 2:03 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 07.02.18 at 17:00, <tamas@xxxxxxxxxxxxx> wrote:
>> This patch as-is correctly tells the two possible formats apart. I
>> tested and Xen boots correctly both from the Shell and from the
>> firmware boot menu. I would not like to start addressing hypothetical
>> scenarios that I have no reasonable way to test against. If you are
>> inclined to do that, that's your call but I'll just leave this patch
>> here for now and I hope you would consider merging it.
>
> Would you mind giving the tentative v4 (below) a try?

Unfortunately this does not seem to work as intended:

# cat /boot/efi/EFI/xen/xen.cfg
[global]
default=old

[old]
options=console=vga
kernel=vmlinuz-4.9.0-6-amd64
root=UUID=19f184db-23a8-42c6-8dfa-67816c822573 ro quiet
ramdisk=initrd.img-4.9.0-6-amd64

[new]
options=console=vga,com1 com1=115200,8n1,amt loglvl=all
guest_loglvl=all altp2m=1
kernel=vmlinuz-4.9.0-6-amd64
root=UUID=19f184db-23a8-42c6-8dfa-67816c822573 ro quiet
ramdisk=initrd.img-4.9.0-6-amd64


# efibootmgr -v
BootCurrent: 0001
Timeout: 0 seconds
BootOrder: 0001,0000,0003,0004,0005,0006,0007
Boot0000* Xen
HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x100000)/File(\EFI\xen\xen.efi)
Boot0001* Xen altp2m
HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x100000)/File(\EFI\xen\xen.efi)n.e.w.

# xl info
...
xen_commandline        : console=vga

As you can see boot option 1 (Xen altp2m) was used for booting but Xen
still used the default global option from the config file instead of
the one specified by the OptionalData.

Tamas

>
> Jan
>
> 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,49 @@ 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 ( *offset < size )
> +        cmdline = data + *offset;
> +    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;
> +            }
> +        }
> +    }
> +
> +    if ( !cmdline )
> +        return 0;
> +
> +    for ( ; size > sizeof(*cmdline) && *cmdline;
> +            size -= sizeof(*cmdline), ++cmdline )
>      {
>          bool cur_sep = *cmdline == L' ' || *cmdline == L'\t';
>
> @@ -1095,15 +1148,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>
>      if ( use_cfg_file )
>      {
> +        UINTN offset = ~(UINTN)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 )
>

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

 


Rackspace

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