|
[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 11:42 AM, Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
> 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;
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.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |