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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

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,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