diff mbox series

[02/10] efi_loader: add option to initialise EFI subsystem early

Message ID 20200427094829.1140-3-takahiro.akashi@linaro.org
State Superseded
Headers show
Series efi_loader: add capsule update support | expand

Commit Message

AKASHI Takahiro April 27, 2020, 9:48 a.m. UTC 
If this option is enabled, the initialisation of UEFI subsystem will be
done as part of U-Boot initialisation.

This feature will be utilised in implementing capsule-on-disk feature.

Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
---
 common/board_r.c       | 6 ++++++
 lib/efi_loader/Kconfig | 4 ++++
 2 files changed, 10 insertions(+)

Comments

Heinrich Schuchardt April 27, 2020, 8:09 p.m. UTC | #1 
On 4/27/20 11:48 AM, AKASHI Takahiro wrote:
> If this option is enabled, the initialisation of UEFI subsystem will be
> done as part of U-Boot initialisation.
>
> This feature will be utilised in implementing capsule-on-disk feature.

This would mean that we allow unaligned access very early. Something
Siarhei was against:

https://lists.denx.de/pipermail/u-boot/2018-March/324242.html
https://patchwork.ozlabs.org/project/uboot/patch/20180329213350.7868-1-xypron.glpk at gmx.de/

Why can't you wait with the capsule update until any command initializes
the UEFI sub-system.

Best regards

Heinrich

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> ---
>  common/board_r.c       | 6 ++++++
>  lib/efi_loader/Kconfig | 4 ++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/common/board_r.c b/common/board_r.c
> index 0bbeaa7594c6..7cf21a6078f9 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -64,6 +64,9 @@
>  #if defined(CONFIG_GPIO_HOG)
>  #include <asm/gpio.h>
>  #endif
> +#ifdef CONFIG_EFI_SETUP_EARLY
> +#include <efi_loader.h>
> +#endif
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -867,6 +870,9 @@ static init_fnc_t init_sequence_r[] = {
>  #endif
>  #if defined(CONFIG_M68K) && defined(CONFIG_BLOCK_CACHE)
>  	blkcache_init,
> +#endif
> +#ifdef CONFIG_EFI_SETUP_EARLY
> +	(init_fnc_t)efi_init_obj_list,
>  #endif
>  	run_main_loop,
>  };
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 1cfa24ffcf72..7cc2d940f848 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -25,6 +25,10 @@ config EFI_LOADER
>
>  if EFI_LOADER
>
> +config EFI_SETUP_EARLY
> +	bool
> +	default n
> +
>  config EFI_GET_TIME
>  	bool "GetTime() runtime service"
>  	depends on DM_RTC
>
AKASHI Takahiro April 28, 2020, 12:16 a.m. UTC | #2 
Heinrich,

On Mon, Apr 27, 2020 at 10:09:11PM +0200, Heinrich Schuchardt wrote:
> On 4/27/20 11:48 AM, AKASHI Takahiro wrote:
> > If this option is enabled, the initialisation of UEFI subsystem will be
> > done as part of U-Boot initialisation.
> >
> > This feature will be utilised in implementing capsule-on-disk feature.
> 
> This would mean that we allow unaligned access very early. Something
> Siarhei was against:

?
Even with CONFIG_EFI_CAPSULE_ON_DISK_EARLY enabled,
efi_init_obj_list() is called at the last of "init" list
and efi_launch_capsules() is called just before the main
command loop.
So "unalignment" issue won't happen.

> https://lists.denx.de/pipermail/u-boot/2018-March/324242.html
> https://patchwork.ozlabs.org/project/uboot/patch/20180329213350.7868-1-xypron.glpk at gmx.de/
> 
> Why can't you wait with the capsule update until any command initializes
> the UEFI sub-system.

This topic is the one the I mentioned in RFC's cover letter
and asked you for comments several time.
Anyway, there are a couple of reasons:
1. Updated firmware may have some effects on not only UEFI
   subsystem but also U-Boot's other features.
2. Firmware update should surely take place after reboot
   as UEFI specification expects.
3. Firmware update should not rely on user's interactions
   or whatever "bootcmd" is set to.
4. In case of failure of firmware update, some recovery should
   be automatically taken "before" the command line is handed over
   to users. (The feature is not implemented yet though.)

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  common/board_r.c       | 6 ++++++
> >  lib/efi_loader/Kconfig | 4 ++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/common/board_r.c b/common/board_r.c
> > index 0bbeaa7594c6..7cf21a6078f9 100644
> > --- a/common/board_r.c
> > +++ b/common/board_r.c
> > @@ -64,6 +64,9 @@
> >  #if defined(CONFIG_GPIO_HOG)
> >  #include <asm/gpio.h>
> >  #endif
> > +#ifdef CONFIG_EFI_SETUP_EARLY
> > +#include <efi_loader.h>
> > +#endif
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -867,6 +870,9 @@ static init_fnc_t init_sequence_r[] = {
> >  #endif
> >  #if defined(CONFIG_M68K) && defined(CONFIG_BLOCK_CACHE)
> >  	blkcache_init,
> > +#endif
> > +#ifdef CONFIG_EFI_SETUP_EARLY
> > +	(init_fnc_t)efi_init_obj_list,
> >  #endif
> >  	run_main_loop,
> >  };
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 1cfa24ffcf72..7cc2d940f848 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -25,6 +25,10 @@ config EFI_LOADER
> >
> >  if EFI_LOADER
> >
> > +config EFI_SETUP_EARLY
> > +	bool
> > +	default n
> > +
> >  config EFI_GET_TIME
> >  	bool "GetTime() runtime service"
> >  	depends on DM_RTC
> >
>
Heinrich Schuchardt May 17, 2020, 7:29 a.m. UTC | #3 
On 4/28/20 2:16 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Mon, Apr 27, 2020 at 10:09:11PM +0200, Heinrich Schuchardt wrote:
>> On 4/27/20 11:48 AM, AKASHI Takahiro wrote:
>>> If this option is enabled, the initialisation of UEFI subsystem will be
>>> done as part of U-Boot initialisation.
>>>
>>> This feature will be utilised in implementing capsule-on-disk feature.
>>
>> This would mean that we allow unaligned access very early. Something
>> Siarhei was against:
>
> ?
> Even with CONFIG_EFI_CAPSULE_ON_DISK_EARLY enabled,
> efi_init_obj_list() is called at the last of "init" list
> and efi_launch_capsules() is called just before the main
> command loop.
> So "unalignment" issue won't happen.

efi_init_obj_list() is even called when booting via booti and therefore
before a lot of other code.

Best regards

Heinrich

>
>> https://lists.denx.de/pipermail/u-boot/2018-March/324242.html
>> https://patchwork.ozlabs.org/project/uboot/patch/20180329213350.7868-1-xypron.glpk at gmx.de/
>>
>> Why can't you wait with the capsule update until any command initializes
>> the UEFI sub-system.
>
> This topic is the one the I mentioned in RFC's cover letter
> and asked you for comments several time.
> Anyway, there are a couple of reasons:
> 1. Updated firmware may have some effects on not only UEFI
>    subsystem but also U-Boot's other features.
> 2. Firmware update should surely take place after reboot
>    as UEFI specification expects.
> 3. Firmware update should not rely on user's interactions
>    or whatever "bootcmd" is set to.
> 4. In case of failure of firmware update, some recovery should
>    be automatically taken "before" the command line is handed over
>    to users. (The feature is not implemented yet though.)
>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> ---
>>>  common/board_r.c       | 6 ++++++
>>>  lib/efi_loader/Kconfig | 4 ++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/common/board_r.c b/common/board_r.c
>>> index 0bbeaa7594c6..7cf21a6078f9 100644
>>> --- a/common/board_r.c
>>> +++ b/common/board_r.c
>>> @@ -64,6 +64,9 @@
>>>  #if defined(CONFIG_GPIO_HOG)
>>>  #include <asm/gpio.h>
>>>  #endif
>>> +#ifdef CONFIG_EFI_SETUP_EARLY
>>> +#include <efi_loader.h>
>>> +#endif
>>>
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -867,6 +870,9 @@ static init_fnc_t init_sequence_r[] = {
>>>  #endif
>>>  #if defined(CONFIG_M68K) && defined(CONFIG_BLOCK_CACHE)
>>>  	blkcache_init,
>>> +#endif
>>> +#ifdef CONFIG_EFI_SETUP_EARLY
>>> +	(init_fnc_t)efi_init_obj_list,
>>>  #endif
>>>  	run_main_loop,
>>>  };
>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>> index 1cfa24ffcf72..7cc2d940f848 100644
>>> --- a/lib/efi_loader/Kconfig
>>> +++ b/lib/efi_loader/Kconfig
>>> @@ -25,6 +25,10 @@ config EFI_LOADER
>>>
>>>  if EFI_LOADER
>>>
>>> +config EFI_SETUP_EARLY
>>> +	bool
>>> +	default n
>>> +
>>>  config EFI_GET_TIME
>>>  	bool "GetTime() runtime service"
>>>  	depends on DM_RTC
>>>
>>
AKASHI Takahiro May 18, 2020, 1:43 a.m. UTC | #4 
Heinrich,

On Sun, May 17, 2020 at 09:29:44AM +0200, Heinrich Schuchardt wrote:
> On 4/28/20 2:16 AM, AKASHI Takahiro wrote:
> > Heinrich,
> >
> > On Mon, Apr 27, 2020 at 10:09:11PM +0200, Heinrich Schuchardt wrote:
> >> On 4/27/20 11:48 AM, AKASHI Takahiro wrote:
> >>> If this option is enabled, the initialisation of UEFI subsystem will be
> >>> done as part of U-Boot initialisation.
> >>>
> >>> This feature will be utilised in implementing capsule-on-disk feature.
> >>
> >> This would mean that we allow unaligned access very early. Something
> >> Siarhei was against:
> >
> > ?
> > Even with CONFIG_EFI_CAPSULE_ON_DISK_EARLY enabled,
> > efi_init_obj_list() is called at the last of "init" list
> > and efi_launch_capsules() is called just before the main
> > command loop.
> > So "unalignment" issue won't happen.
> 
> efi_init_obj_list() is even called when booting via booti and therefore
> before a lot of other code.

How is booti related to "unalignment" issue?

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >> https://lists.denx.de/pipermail/u-boot/2018-March/324242.html
> >> https://patchwork.ozlabs.org/project/uboot/patch/20180329213350.7868-1-xypron.glpk at gmx.de/
> >>
> >> Why can't you wait with the capsule update until any command initializes
> >> the UEFI sub-system.
> >
> > This topic is the one the I mentioned in RFC's cover letter
> > and asked you for comments several time.
> > Anyway, there are a couple of reasons:
> > 1. Updated firmware may have some effects on not only UEFI
> >    subsystem but also U-Boot's other features.
> > 2. Firmware update should surely take place after reboot
> >    as UEFI specification expects.
> > 3. Firmware update should not rely on user's interactions
> >    or whatever "bootcmd" is set to.
> > 4. In case of failure of firmware update, some recovery should
> >    be automatically taken "before" the command line is handed over
> >    to users. (The feature is not implemented yet though.)
> >
> > -Takahiro Akashi
> >
> >
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> >>> ---
> >>>  common/board_r.c       | 6 ++++++
> >>>  lib/efi_loader/Kconfig | 4 ++++
> >>>  2 files changed, 10 insertions(+)
> >>>
> >>> diff --git a/common/board_r.c b/common/board_r.c
> >>> index 0bbeaa7594c6..7cf21a6078f9 100644
> >>> --- a/common/board_r.c
> >>> +++ b/common/board_r.c
> >>> @@ -64,6 +64,9 @@
> >>>  #if defined(CONFIG_GPIO_HOG)
> >>>  #include <asm/gpio.h>
> >>>  #endif
> >>> +#ifdef CONFIG_EFI_SETUP_EARLY
> >>> +#include <efi_loader.h>
> >>> +#endif
> >>>
> >>>  DECLARE_GLOBAL_DATA_PTR;
> >>>
> >>> @@ -867,6 +870,9 @@ static init_fnc_t init_sequence_r[] = {
> >>>  #endif
> >>>  #if defined(CONFIG_M68K) && defined(CONFIG_BLOCK_CACHE)
> >>>  	blkcache_init,
> >>> +#endif
> >>> +#ifdef CONFIG_EFI_SETUP_EARLY
> >>> +	(init_fnc_t)efi_init_obj_list,
> >>>  #endif
> >>>  	run_main_loop,
> >>>  };
> >>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >>> index 1cfa24ffcf72..7cc2d940f848 100644
> >>> --- a/lib/efi_loader/Kconfig
> >>> +++ b/lib/efi_loader/Kconfig
> >>> @@ -25,6 +25,10 @@ config EFI_LOADER
> >>>
> >>>  if EFI_LOADER
> >>>
> >>> +config EFI_SETUP_EARLY
> >>> +	bool
> >>> +	default n
> >>> +
> >>>  config EFI_GET_TIME
> >>>  	bool "GetTime() runtime service"
> >>>  	depends on DM_RTC
> >>>
> >>
>
diff mbox series

Patch

diff --git a/common/board_r.c b/common/board_r.c
index 0bbeaa7594c6..7cf21a6078f9 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -64,6 +64,9 @@ 
 #if defined(CONFIG_GPIO_HOG)
 #include <asm/gpio.h>
 #endif
+#ifdef CONFIG_EFI_SETUP_EARLY
+#include <efi_loader.h>
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -867,6 +870,9 @@  static init_fnc_t init_sequence_r[] = {
 #endif
 #if defined(CONFIG_M68K) && defined(CONFIG_BLOCK_CACHE)
 	blkcache_init,
+#endif
+#ifdef CONFIG_EFI_SETUP_EARLY
+	(init_fnc_t)efi_init_obj_list,
 #endif
 	run_main_loop,
 };
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 1cfa24ffcf72..7cc2d940f848 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -25,6 +25,10 @@  config EFI_LOADER
 
 if EFI_LOADER
 
+config EFI_SETUP_EARLY
+	bool
+	default n
+
 config EFI_GET_TIME
 	bool "GetTime() runtime service"
 	depends on DM_RTC