Re: [PATCH] efi_loader: Allow capsule update on-disk without checking OsIndications

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

> > > 
[...]
> > > +config EFI_IGNORE_OSINDICATIONS
> > > +	bool "Ignore OsIndications for CapsuleUpdate on-disk"
> > > +	depends on EFI_CAPSULE_ON_DISK
> > > +	default n
> > > +	help
> > > +	  There are boards were we can't support SetVariable at runtime.
> > > +	  Select this option if you want to use capsule-on-disk feature,
> > > +	  without setting the OsIndications bit.
> 
> Obviously, this option breaks the conformance to UEFI specification
> and must be turned on carefully in the limited use cases.
> You should describe that here explicitly.

Fair enough, I'll send a v2

> 
> Additionally, you may add
>    depends on !EFI_MM_COMM_TEE (or better config?)

That's not the case (yet).  I do have a kernel branch were SetVariable at
runtime is supported, but that's not merged yet.  So until this gets accepted
*all* the boards that want to perform a capsuleupdate on-disk without setting
the variable need this. 

Cheers
/Ilias
> 
> -Takahiro Akashi
> 
> 
> > >   config EFI_CAPSULE_ON_DISK_EARLY
> > >   	bool "Initiate capsule-on-disk at U-Boot boottime"
> > >   	depends on EFI_CAPSULE_ON_DISK
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index d7136035d8f9..50bed32bfb3b 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -948,6 +948,33 @@ efi_status_t __weak efi_load_capsule_drivers(void)
> > >   	return ret;
> > >   }
> > > 
> > > +/**
> > > + * check_run_capsules - Check whether capsule update should run
> > > + *
> > > + * The spec says OsIndications must be set in order to run the capsule update
> > > + * on-disk.  Since U-Boot doesn't support runtime SetVariable, allow capsules to
> > > + * run explicitly if CONFIG_EFI_IGNORE_OSINDICATIONS is selected
> > > + */
> > > +static bool check_run_capsules(void)
> > > +{
> > > +	u64 os_indications;
> > > +	efi_uintn_t size;
> > > +	efi_status_t ret;
> > > +
> > > +	if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS))
> > > +		return true;
> > > +
> > > +	size = sizeof(os_indications);
> > > +	ret = efi_get_variable_int(L"OsIndications", &efi_global_variable_guid,
> > > +				   NULL, &size, &os_indications, NULL);
> > > +	if (ret == EFI_SUCCESS &&
> > > +	    (os_indications
> > > +	      & EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED))
> > 
> > The bit EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED must be
> > cleared in OsIndications after successfully applying the capsule. See
> > related patch
> > 
> > [PATCH 1/1] efi_loader: fix set_capsule_result()
> > https://lists.denx.de/pipermail/u-boot/2021-June/453111.html
> > 
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > 
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > > +
> > >   /**
> > >    * efi_launch_capsule - launch capsules
> > >    *
> > > @@ -958,20 +985,13 @@ efi_status_t __weak efi_load_capsule_drivers(void)
> > >    */
> > >   efi_status_t efi_launch_capsules(void)
> > >   {
> > > -	u64 os_indications;
> > > -	efi_uintn_t size;
> > >   	struct efi_capsule_header *capsule = NULL;
> > >   	u16 **files;
> > >   	unsigned int nfiles, index, i;
> > >   	u16 variable_name16[12];
> > >   	efi_status_t ret;
> > > 
> > > -	size = sizeof(os_indications);
> > > -	ret = efi_get_variable_int(L"OsIndications", &efi_global_variable_guid,
> > > -				   NULL, &size, &os_indications, NULL);
> > > -	if (ret != EFI_SUCCESS ||
> > > -	    !(os_indications
> > > -	      & EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED))
> > > +	if (!check_run_capsules())
> > >   		return EFI_SUCCESS;
> > > 
> > >   	index = get_last_capsule();
> > > 
> >