Re: [U-Boot] [PATCH v0 20/20] efi_loader: add bootmgr

[Peter Jones]

On Fri, Aug 04, 2017 at 04:28:05PM -0400, Rob Clark wrote:
> On Fri, Aug 4, 2017 at 4:06 PM, Heinrich Schuchardt  
> wrote:
> > On 08/04/2017 09:32 PM, Rob Clark wrote:
> >> Similar to a "real" UEFI implementation, the bootmgr looks at the
> >> BootOrder and Boot variables to try to find an EFI payload to load
> >> and boot.  This is added as a sub-command of bootefi.
> >>
> >> The idea is that the distro bootcmd would first try loading a payload
> >> via the bootmgr, and then if that fails (ie. first boot or corrupted
> >> EFI variables) it would fallback to loading bootaa64.efi.  (Which
> >> would then load fallback.efi which would look for \EFI\*\boot.csv and
> >> populate BootOrder and Boot based on what it found.)
> >
> >
> > I wonder if this implementation is Fedora specific.
> >
> > For Debian I could not find any reference to boot.csv.
> > And it is not mentioned in the UEFI 2.7 spec.
> >
> > Please, provide the specification that your work is based on.
> 
> The references to boot.csv are based on looking at how shim/fallback
> work.. perhaps that is not standardized.  I'll let Peter Jones comment
> on that if he knows better what windows and other linux distro's do.

The boot.csv parts are part of fallback.efi, which is part of shim.
It's not distro specific, though not every distro uses it (though I
thought debian had started doing so very recently), and fallback.efi +
boot.csv is still strictly optional.  But as Rob says below - the boot
methodology fallback employs is based on what the spec says the boot
manager will do.  A distro could also just stick a second copy of their
normal bootloader in /EFI/BOOT/BOOTAA64.EFI and it would boot through
the recovery path every time until somebody manually created a boot
variable.  fallback is just shim's method of setting the variables
automatically, and it is fairly widely deployed.  I'm pretty sure SuSE
and Ubuntu both use it, for example.

> The bootmanager implementation is based on UEFI spec (sect 3.1 in
> v2.7), which does not depend on boot.csv or how shim/fallback program
> the BootOrder/Boot variables.  But simply that they do.  I'm not
> particularly familiar with the boot chain on Debian, it is entirely
> possible that it works differently.  My comments about boot.csv where
> merely to try to provide context (and are quite possibly misleading on
> some distro's), but are irrelevant to the bootmgr implementation which
> only cares about BootOrder/Boot, as described in sect 3.1.
> 
> (There are a lot of details that I skipped over in the bootmgr
> implementation, simply because secure boot or setting of efi variables
> from the OS is not implemented, so they are not yet relevant.)

This is still all correct.

-- 
  Peter
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v0 20/20] efi_loader: add bootmgr

[Rob Clark]

On Fri, Aug 4, 2017 at 4:28 PM, Rob Clark  wrote:
> On Fri, Aug 4, 2017 at 4:06 PM, Heinrich Schuchardt  
> wrote:
>> On 08/04/2017 09:32 PM, Rob Clark wrote:
>>> Similar to a "real" UEFI implementation, the bootmgr looks at the
>>> BootOrder and Boot variables to try to find an EFI payload to load
>>> and boot.  This is added as a sub-command of bootefi.
>>>
>>> The idea is that the distro bootcmd would first try loading a payload
>>> via the bootmgr, and then if that fails (ie. first boot or corrupted
>>> EFI variables) it would fallback to loading bootaa64.efi.  (Which
>>> would then load fallback.efi which would look for \EFI\*\boot.csv and
>>> populate BootOrder and Boot based on what it found.)
>>
>>
>> I wonder if this implementation is Fedora specific.
>>
>> For Debian I could not find any reference to boot.csv.
>> And it is not mentioned in the UEFI 2.7 spec.
>>
>> Please, provide the specification that your work is based on.
>
> The references to boot.csv are based on looking at how shim/fallback
> work.. perhaps that is not standardized.  I'll let Peter Jones comment
> on that if he knows better what windows and other linux distro's do.
>
> The bootmanager implementation is based on UEFI spec (sect 3.1 in
> v2.7), which does not depend on boot.csv or how shim/fallback program
> the BootOrder/Boot variables.  But simply that they do.  I'm not
> particularly familiar with the boot chain on Debian, it is entirely
> possible that it works differently.  My comments about boot.csv where
> merely to try to provide context (and are quite possibly misleading on
> some distro's), but are irrelevant to the bootmgr implementation which
> only cares about BootOrder/Boot, as described in sect 3.1.
>
> (There are a lot of details that I skipped over in the bootmgr
> implementation, simply because secure boot or setting of efi variables
> from the OS is not implemented, so they are not yet relevant.)

and in case it wasn't clear, that means the implementation is not
fedora specific, even if my description in the cover letter is.

BR,
-R


> BR,
> -R
>
>
>> Best regards
>>
>> Heinrich
>>
>>
>>>
>>> Signed-off-by: Rob Clark 
>>> ---
>>>  cmd/bootefi.c |  48 ++-
>>>  include/config_distro_bootcmd.h   |   5 ++
>>>  include/efi_api.h |   4 +
>>>  include/efi_loader.h  |   6 ++
>>>  lib/efi_loader/Makefile   |   2 +-
>>>  lib/efi_loader/efi_bootmgr.c  | 169 
>>> ++
>>>  lib/efi_loader/efi_boottime.c |   6 +-
>>>  lib/efi_loader/efi_image_loader.c |   1 +
>>>  8 files changed, 235 insertions(+), 6 deletions(-)
>>>  create mode 100644 lib/efi_loader/efi_bootmgr.c
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 80f52e9e35..02a0dd159b 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -219,6 +219,36 @@ exit:
>>>   return ret;
>>>  }
>>>
>>> +static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
>>> +{
>>> + struct efi_device_path *device_path, *file_path;
>>> + void *addr;
>>> + efi_status_t r;
>>> +
>>> + /* Initialize and populate EFI object list */
>>> + if (!efi_obj_list_initalized)
>>> + efi_init_obj_list();
>>> +
>>> + /*
>>> +  * gd lives in a fixed register which may get clobbered while we 
>>> execute
>>> +  * the payload. So save it here and restore it on every callback entry
>>> +  */
>>> + efi_save_gd();
>>> +
>>> + addr = efi_bootmgr_load(_path, _path);
>>> + if (!addr)
>>> + return 1;
>>> +
>>> + printf("## Starting EFI application at %p ...\n", addr);
>>> + r = do_bootefi_exec(addr, (void*)fdt_addr, device_path, file_path);
>>> + printf("## Application terminated, r = %lu\n",
>>> +r & ~EFI_ERROR_MASK);
>>> +
>>> + if (r != EFI_SUCCESS)
>>> + return 1;
>>> +
>>> + return 0;
>>> +}
>>>
>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const 
>>> argv[])
>>> @@ -237,7 +267,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int 
>>> argc, char * const argv[])
>>>   memcpy((char *)addr, __efi_hello_world_begin, size);
>>>   } else
>>>  #endif
>>> - {
>>> + if (!strcmp(argv[1], "bootmgr")) {
>>> + unsigned long fdt_addr = 0;
>>> +
>>> + if (argc > 2)
>>> + fdt_addr = simple_strtoul(argv[2], NULL, 16);
>>> +
>>> + return do_bootefi_bootmgr_exec(fdt_addr);
>>> + } else {
>>>   saddr = argv[1];
>>>
>>>   addr = simple_strtoul(saddr, NULL, 16);
>>> @@ -270,7 +307,11 @@ static char bootefi_help_text[] =
>>>   "hello\n"
>>>   "  - boot a sample Hello World application stored within U-Boot"
>>>  #endif
>>> - ;
>>> + "bootmgr [fdt addr]\n"
>>> + "  - load and boot 

Re: [U-Boot] [PATCH v0 20/20] efi_loader: add bootmgr

[Rob Clark]

On Fri, Aug 4, 2017 at 4:06 PM, Heinrich Schuchardt  wrote:
> On 08/04/2017 09:32 PM, Rob Clark wrote:
>> Similar to a "real" UEFI implementation, the bootmgr looks at the
>> BootOrder and Boot variables to try to find an EFI payload to load
>> and boot.  This is added as a sub-command of bootefi.
>>
>> The idea is that the distro bootcmd would first try loading a payload
>> via the bootmgr, and then if that fails (ie. first boot or corrupted
>> EFI variables) it would fallback to loading bootaa64.efi.  (Which
>> would then load fallback.efi which would look for \EFI\*\boot.csv and
>> populate BootOrder and Boot based on what it found.)
>
>
> I wonder if this implementation is Fedora specific.
>
> For Debian I could not find any reference to boot.csv.
> And it is not mentioned in the UEFI 2.7 spec.
>
> Please, provide the specification that your work is based on.

The references to boot.csv are based on looking at how shim/fallback
work.. perhaps that is not standardized.  I'll let Peter Jones comment
on that if he knows better what windows and other linux distro's do.

The bootmanager implementation is based on UEFI spec (sect 3.1 in
v2.7), which does not depend on boot.csv or how shim/fallback program
the BootOrder/Boot variables.  But simply that they do.  I'm not
particularly familiar with the boot chain on Debian, it is entirely
possible that it works differently.  My comments about boot.csv where
merely to try to provide context (and are quite possibly misleading on
some distro's), but are irrelevant to the bootmgr implementation which
only cares about BootOrder/Boot, as described in sect 3.1.

(There are a lot of details that I skipped over in the bootmgr
implementation, simply because secure boot or setting of efi variables
from the OS is not implemented, so they are not yet relevant.)

BR,
-R


> Best regards
>
> Heinrich
>
>
>>
>> Signed-off-by: Rob Clark 
>> ---
>>  cmd/bootefi.c |  48 ++-
>>  include/config_distro_bootcmd.h   |   5 ++
>>  include/efi_api.h |   4 +
>>  include/efi_loader.h  |   6 ++
>>  lib/efi_loader/Makefile   |   2 +-
>>  lib/efi_loader/efi_bootmgr.c  | 169 
>> ++
>>  lib/efi_loader/efi_boottime.c |   6 +-
>>  lib/efi_loader/efi_image_loader.c |   1 +
>>  8 files changed, 235 insertions(+), 6 deletions(-)
>>  create mode 100644 lib/efi_loader/efi_bootmgr.c
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 80f52e9e35..02a0dd159b 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -219,6 +219,36 @@ exit:
>>   return ret;
>>  }
>>
>> +static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
>> +{
>> + struct efi_device_path *device_path, *file_path;
>> + void *addr;
>> + efi_status_t r;
>> +
>> + /* Initialize and populate EFI object list */
>> + if (!efi_obj_list_initalized)
>> + efi_init_obj_list();
>> +
>> + /*
>> +  * gd lives in a fixed register which may get clobbered while we 
>> execute
>> +  * the payload. So save it here and restore it on every callback entry
>> +  */
>> + efi_save_gd();
>> +
>> + addr = efi_bootmgr_load(_path, _path);
>> + if (!addr)
>> + return 1;
>> +
>> + printf("## Starting EFI application at %p ...\n", addr);
>> + r = do_bootefi_exec(addr, (void*)fdt_addr, device_path, file_path);
>> + printf("## Application terminated, r = %lu\n",
>> +r & ~EFI_ERROR_MASK);
>> +
>> + if (r != EFI_SUCCESS)
>> + return 1;
>> +
>> + return 0;
>> +}
>>
>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const 
>> argv[])
>> @@ -237,7 +267,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int 
>> argc, char * const argv[])
>>   memcpy((char *)addr, __efi_hello_world_begin, size);
>>   } else
>>  #endif
>> - {
>> + if (!strcmp(argv[1], "bootmgr")) {
>> + unsigned long fdt_addr = 0;
>> +
>> + if (argc > 2)
>> + fdt_addr = simple_strtoul(argv[2], NULL, 16);
>> +
>> + return do_bootefi_bootmgr_exec(fdt_addr);
>> + } else {
>>   saddr = argv[1];
>>
>>   addr = simple_strtoul(saddr, NULL, 16);
>> @@ -270,7 +307,11 @@ static char bootefi_help_text[] =
>>   "hello\n"
>>   "  - boot a sample Hello World application stored within U-Boot"
>>  #endif
>> - ;
>> + "bootmgr [fdt addr]\n"
>> + "  - load and boot EFI payload based on BootOrder/Boot 
>> variables.\n"
>> + "\n"
>> + "If specified, the device tree located at  gets\n"
>> + "exposed as EFI configuration table.\n";
>>  #endif
>>
>>  U_BOOT_CMD(
>> @@ -308,6 +349,9 @@ void efi_set_bootdev(const char *dev, const char *devnr, 
>> const char *path)
>>  #endif
>>   }
>>
>> + if 

Re: [U-Boot] [PATCH v0 20/20] efi_loader: add bootmgr

[Heinrich Schuchardt]

On 08/04/2017 09:32 PM, Rob Clark wrote:
> Similar to a "real" UEFI implementation, the bootmgr looks at the
> BootOrder and Boot variables to try to find an EFI payload to load
> and boot.  This is added as a sub-command of bootefi.
> 
> The idea is that the distro bootcmd would first try loading a payload
> via the bootmgr, and then if that fails (ie. first boot or corrupted
> EFI variables) it would fallback to loading bootaa64.efi.  (Which
> would then load fallback.efi which would look for \EFI\*\boot.csv and
> populate BootOrder and Boot based on what it found.)


I wonder if this implementation is Fedora specific.

For Debian I could not find any reference to boot.csv.
And it is not mentioned in the UEFI 2.7 spec.

Please, provide the specification that your work is based on.

Best regards

Heinrich


> 
> Signed-off-by: Rob Clark 
> ---
>  cmd/bootefi.c |  48 ++-
>  include/config_distro_bootcmd.h   |   5 ++
>  include/efi_api.h |   4 +
>  include/efi_loader.h  |   6 ++
>  lib/efi_loader/Makefile   |   2 +-
>  lib/efi_loader/efi_bootmgr.c  | 169 
> ++
>  lib/efi_loader/efi_boottime.c |   6 +-
>  lib/efi_loader/efi_image_loader.c |   1 +
>  8 files changed, 235 insertions(+), 6 deletions(-)
>  create mode 100644 lib/efi_loader/efi_bootmgr.c
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 80f52e9e35..02a0dd159b 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -219,6 +219,36 @@ exit:
>   return ret;
>  }
>  
> +static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
> +{
> + struct efi_device_path *device_path, *file_path;
> + void *addr;
> + efi_status_t r;
> +
> + /* Initialize and populate EFI object list */
> + if (!efi_obj_list_initalized)
> + efi_init_obj_list();
> +
> + /*
> +  * gd lives in a fixed register which may get clobbered while we execute
> +  * the payload. So save it here and restore it on every callback entry
> +  */
> + efi_save_gd();
> +
> + addr = efi_bootmgr_load(_path, _path);
> + if (!addr)
> + return 1;
> +
> + printf("## Starting EFI application at %p ...\n", addr);
> + r = do_bootefi_exec(addr, (void*)fdt_addr, device_path, file_path);
> + printf("## Application terminated, r = %lu\n",
> +r & ~EFI_ERROR_MASK);
> +
> + if (r != EFI_SUCCESS)
> + return 1;
> +
> + return 0;
> +}
>  
>  /* Interpreter command to boot an arbitrary EFI image from memory */
>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const 
> argv[])
> @@ -237,7 +267,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int 
> argc, char * const argv[])
>   memcpy((char *)addr, __efi_hello_world_begin, size);
>   } else
>  #endif
> - {
> + if (!strcmp(argv[1], "bootmgr")) {
> + unsigned long fdt_addr = 0;
> +
> + if (argc > 2)
> + fdt_addr = simple_strtoul(argv[2], NULL, 16);
> +
> + return do_bootefi_bootmgr_exec(fdt_addr);
> + } else {
>   saddr = argv[1];
>  
>   addr = simple_strtoul(saddr, NULL, 16);
> @@ -270,7 +307,11 @@ static char bootefi_help_text[] =
>   "hello\n"
>   "  - boot a sample Hello World application stored within U-Boot"
>  #endif
> - ;
> + "bootmgr [fdt addr]\n"
> + "  - load and boot EFI payload based on BootOrder/Boot variables.\n"
> + "\n"
> + "If specified, the device tree located at  gets\n"
> + "exposed as EFI configuration table.\n";
>  #endif
>  
>  U_BOOT_CMD(
> @@ -308,6 +349,9 @@ void efi_set_bootdev(const char *dev, const char *devnr, 
> const char *path)
>  #endif
>   }
>  
> + if (!path)
> + return;
> +
>   if (strcmp(dev, "Net")) {
>   /* Add leading / to fs paths, because they're absolute */
>   snprintf(filename, sizeof(filename), "/%s", path);
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index d8dab8e46a..94ccab02d2 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -112,6 +112,11 @@
>  
>  #define BOOTENV_SHARED_EFI\
>   "boot_efi_binary="\
> + "if fdt addr ${fdt_addr_r}; then "\
> + "bootefi bootmgr ${fdt_addr_r};"  \
> + "else "   \
> + "bootefi bootmgr ${fdtcontroladdr};"  \
> + "fi;" \
>   "load ${devtype} ${devnum}:${distro_bootpart} "   \
>   "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; "  \
>   "if fdt addr 

[U-Boot] [PATCH v0 20/20] efi_loader: add bootmgr

[Rob Clark]

Similar to a "real" UEFI implementation, the bootmgr looks at the
BootOrder and Boot variables to try to find an EFI payload to load
and boot.  This is added as a sub-command of bootefi.

The idea is that the distro bootcmd would first try loading a payload
via the bootmgr, and then if that fails (ie. first boot or corrupted
EFI variables) it would fallback to loading bootaa64.efi.  (Which
would then load fallback.efi which would look for \EFI\*\boot.csv and
populate BootOrder and Boot based on what it found.)

Signed-off-by: Rob Clark 
---
 cmd/bootefi.c |  48 ++-
 include/config_distro_bootcmd.h   |   5 ++
 include/efi_api.h |   4 +
 include/efi_loader.h  |   6 ++
 lib/efi_loader/Makefile   |   2 +-
 lib/efi_loader/efi_bootmgr.c  | 169 ++
 lib/efi_loader/efi_boottime.c |   6 +-
 lib/efi_loader/efi_image_loader.c |   1 +
 8 files changed, 235 insertions(+), 6 deletions(-)
 create mode 100644 lib/efi_loader/efi_bootmgr.c

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 80f52e9e35..02a0dd159b 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -219,6 +219,36 @@ exit:
return ret;
 }
 
+static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
+{
+   struct efi_device_path *device_path, *file_path;
+   void *addr;
+   efi_status_t r;
+
+   /* Initialize and populate EFI object list */
+   if (!efi_obj_list_initalized)
+   efi_init_obj_list();
+
+   /*
+* gd lives in a fixed register which may get clobbered while we execute
+* the payload. So save it here and restore it on every callback entry
+*/
+   efi_save_gd();
+
+   addr = efi_bootmgr_load(_path, _path);
+   if (!addr)
+   return 1;
+
+   printf("## Starting EFI application at %p ...\n", addr);
+   r = do_bootefi_exec(addr, (void*)fdt_addr, device_path, file_path);
+   printf("## Application terminated, r = %lu\n",
+  r & ~EFI_ERROR_MASK);
+
+   if (r != EFI_SUCCESS)
+   return 1;
+
+   return 0;
+}
 
 /* Interpreter command to boot an arbitrary EFI image from memory */
 static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const 
argv[])
@@ -237,7 +267,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int 
argc, char * const argv[])
memcpy((char *)addr, __efi_hello_world_begin, size);
} else
 #endif
-   {
+   if (!strcmp(argv[1], "bootmgr")) {
+   unsigned long fdt_addr = 0;
+
+   if (argc > 2)
+   fdt_addr = simple_strtoul(argv[2], NULL, 16);
+
+   return do_bootefi_bootmgr_exec(fdt_addr);
+   } else {
saddr = argv[1];
 
addr = simple_strtoul(saddr, NULL, 16);
@@ -270,7 +307,11 @@ static char bootefi_help_text[] =
"hello\n"
"  - boot a sample Hello World application stored within U-Boot"
 #endif
-   ;
+   "bootmgr [fdt addr]\n"
+   "  - load and boot EFI payload based on BootOrder/Boot variables.\n"
+   "\n"
+   "If specified, the device tree located at  gets\n"
+   "exposed as EFI configuration table.\n";
 #endif
 
 U_BOOT_CMD(
@@ -308,6 +349,9 @@ void efi_set_bootdev(const char *dev, const char *devnr, 
const char *path)
 #endif
}
 
+   if (!path)
+   return;
+
if (strcmp(dev, "Net")) {
/* Add leading / to fs paths, because they're absolute */
snprintf(filename, sizeof(filename), "/%s", path);
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index d8dab8e46a..94ccab02d2 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -112,6 +112,11 @@
 
 #define BOOTENV_SHARED_EFI\
"boot_efi_binary="\
+   "if fdt addr ${fdt_addr_r}; then "\
+   "bootefi bootmgr ${fdt_addr_r};"  \
+   "else "   \
+   "bootefi bootmgr ${fdtcontroladdr};"  \
+   "fi;" \
"load ${devtype} ${devnum}:${distro_bootpart} "   \
"${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; "  \
"if fdt addr ${fdt_addr_r}; then "\
diff --git a/include/efi_api.h b/include/efi_api.h
index 1a542846b3..ef91e34c7b 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -211,6 +211,10 @@ struct efi_runtime_services {
EFI_GUID(0x, 0x, 0x, 0x00, 0x00, \
 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
 
+#define EFI_GLOBAL_VARIABLE_GUID \
+   EFI_GUID(0x8be4df61, 0x93ca, 0x11d2,