Re: [U-Boot] Unexpected saving of all environment variables during EFI boot

[Heinrich Schuchardt]

On 10/19/2017 11:28 PM, Alexander Graf wrote:
> 
> 
> On 19.10.17 23:15, Rob Clark wrote:
>> On Thu, Oct 19, 2017 at 5:05 PM, Alexander Graf  wrote:
>>>
>>>
>>> On 19.10.17 22:40, Heinrich Schuchardt wrote:
 This was merged as
 ad644e7c18238026fecc647f62584d87d2c5b0a3
 efi_loader: efi variable support

> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index ec40f41bcb..c406ff82ff 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -8,6 +8,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -942,6 +943,11 @@ static efi_status_t EFIAPI 
> efi_exit_boot_services(void *image_handle,
>  {
>  EFI_ENTRY("%p, %ld", image_handle, map_key);
>
> +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
> +/* save any EFI variables that have been written: */
> +env_save();

 You save all changed variables and not specifically those bound to EFI.
 This is completely unexpected behavior for the user and not covered by
 the commit message.

 Furthermore you call env_save even if no EFI variable has been touched
 at all.

 This leads to writing to flash on every boot via EFI. Depending on
 technology this might ruin the flash after a few hundred reboots.
>>>
>>> I agree that overwriting flash on every boot isn't a terribly smart idea.
>>>
>>> Also saving random environment variables that are set while we are in a
>>> distro loop to persistent storage isn't great either.
>>>
>>> I agree that we should probably improve this code to only save efi
>>> variables.
>>>
>>> Rob, how bad would it be for Fedora if we remove the persistent variable
>>> store call (just the env_save() line) for 2017.11 to not kill people's
>>> storage devices? Then for the next version tackle it for real and only
>>> save efi variable changes here and only if anything really did change.
>>>
>>
>> As I mentioned on #u-boot, I think we can just comment the
>> env_save().. or perhaps make it a config option.  It will be mildly
>> annoying, since it means every boot is "first boot", ie. falling back
>> to fallback.efi to populate BootOrder/Boot variables, and never
>> using bootmgr.  But since we can't set EFI variables from userspace
>> yet, it isn't really a regression (yet).
> 
> Ok, I sent a patch to remove the env_save() line.

Seems we worked in parallel :)

> 
>> Longer term, maybe we need to split out EFI variables from u-boot env
>> vars..  I thought it would be nice to store them together, since it
>> gives a convenient way to set EFI variables from script or cmdline.
>> But at least a subset of the EFI vars should be persisted to reach
>> EBBR, and if that breaks expectations about u-boot env vars, then I
>> guess we need to split them.
> 
> I really like the sharing of the env space. We just need to have a
> different write mechanism for EFI variables: Instead of writing the
> current env, we could for example reread the original env, copy all
> current efi variables into that and write it back?

That is why I said we have to add read and write methods in struct
env_driver.

> 
>>
>> Side note, devices that can't repeatedly save env (even if we split
>> out the EFI variables to be stored separately) might be problematic
>> with EFI_LOADER.  Maybe we need a build option they can set to have a
>> more crippled version of EFI_LOADER, so as to not penalize more
>> capable devices.
> 
> Every flash device self-destroys :).

There are also env implementations in U-Boot writing to disk:
- env/ext4.c
- env/fat.c

> 
> Really, it boils down to the amount of saves. Writing to flash on every
> boot is a pretty bad idea. If we simply only write when a variable write
> actually occurred (which in most cases won't), then we should be ok on
> all devices I think.

According to the UEFI standard we should save the monotonic counter on
every boot.

Regards

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

Re: [U-Boot] Unexpected saving of all environment variables during EFI boot

[Alexander Graf]

On 19.10.17 23:15, Rob Clark wrote:
> On Thu, Oct 19, 2017 at 5:05 PM, Alexander Graf  wrote:
>>
>>
>> On 19.10.17 22:40, Heinrich Schuchardt wrote:
>>> This was merged as
>>> ad644e7c18238026fecc647f62584d87d2c5b0a3
>>> efi_loader: efi variable support
>>>
 diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
 index ec40f41bcb..c406ff82ff 100644
 --- a/lib/efi_loader/efi_boottime.c
 +++ b/lib/efi_loader/efi_boottime.c
 @@ -8,6 +8,7 @@

  #include 
  #include 
 +#include 
  #include 
  #include 
  #include 
 @@ -942,6 +943,11 @@ static efi_status_t EFIAPI 
 efi_exit_boot_services(void *image_handle,
  {
  EFI_ENTRY("%p, %ld", image_handle, map_key);

 +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
 +/* save any EFI variables that have been written: */
 +env_save();
>>>
>>> You save all changed variables and not specifically those bound to EFI.
>>> This is completely unexpected behavior for the user and not covered by
>>> the commit message.
>>>
>>> Furthermore you call env_save even if no EFI variable has been touched
>>> at all.
>>>
>>> This leads to writing to flash on every boot via EFI. Depending on
>>> technology this might ruin the flash after a few hundred reboots.
>>
>> I agree that overwriting flash on every boot isn't a terribly smart idea.
>>
>> Also saving random environment variables that are set while we are in a
>> distro loop to persistent storage isn't great either.
>>
>> I agree that we should probably improve this code to only save efi
>> variables.
>>
>> Rob, how bad would it be for Fedora if we remove the persistent variable
>> store call (just the env_save() line) for 2017.11 to not kill people's
>> storage devices? Then for the next version tackle it for real and only
>> save efi variable changes here and only if anything really did change.
>>
> 
> As I mentioned on #u-boot, I think we can just comment the
> env_save().. or perhaps make it a config option.  It will be mildly
> annoying, since it means every boot is "first boot", ie. falling back
> to fallback.efi to populate BootOrder/Boot variables, and never
> using bootmgr.  But since we can't set EFI variables from userspace
> yet, it isn't really a regression (yet).

Ok, I sent a patch to remove the env_save() line.

> Longer term, maybe we need to split out EFI variables from u-boot env
> vars..  I thought it would be nice to store them together, since it
> gives a convenient way to set EFI variables from script or cmdline.
> But at least a subset of the EFI vars should be persisted to reach
> EBBR, and if that breaks expectations about u-boot env vars, then I
> guess we need to split them.

I really like the sharing of the env space. We just need to have a
different write mechanism for EFI variables: Instead of writing the
current env, we could for example reread the original env, copy all
current efi variables into that and write it back?

> 
> Side note, devices that can't repeatedly save env (even if we split
> out the EFI variables to be stored separately) might be problematic
> with EFI_LOADER.  Maybe we need a build option they can set to have a
> more crippled version of EFI_LOADER, so as to not penalize more
> capable devices.

Every flash device self-destroys :).

Really, it boils down to the amount of saves. Writing to flash on every
boot is a pretty bad idea. If we simply only write when a variable write
actually occurred (which in most cases won't), then we should be ok on
all devices I think.


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

Re: [U-Boot] Unexpected saving of all environment variables during EFI boot

[Rob Clark]

On Thu, Oct 19, 2017 at 5:05 PM, Alexander Graf  wrote:
>
>
> On 19.10.17 22:40, Heinrich Schuchardt wrote:
>> This was merged as
>> ad644e7c18238026fecc647f62584d87d2c5b0a3
>> efi_loader: efi variable support
>>
>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>> index ec40f41bcb..c406ff82ff 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -8,6 +8,7 @@
>>>
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>  #include 
>>> @@ -942,6 +943,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(void 
>>> *image_handle,
>>>  {
>>>  EFI_ENTRY("%p, %ld", image_handle, map_key);
>>>
>>> +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
>>> +/* save any EFI variables that have been written: */
>>> +env_save();
>>
>> You save all changed variables and not specifically those bound to EFI.
>> This is completely unexpected behavior for the user and not covered by
>> the commit message.
>>
>> Furthermore you call env_save even if no EFI variable has been touched
>> at all.
>>
>> This leads to writing to flash on every boot via EFI. Depending on
>> technology this might ruin the flash after a few hundred reboots.
>
> I agree that overwriting flash on every boot isn't a terribly smart idea.
>
> Also saving random environment variables that are set while we are in a
> distro loop to persistent storage isn't great either.
>
> I agree that we should probably improve this code to only save efi
> variables.
>
> Rob, how bad would it be for Fedora if we remove the persistent variable
> store call (just the env_save() line) for 2017.11 to not kill people's
> storage devices? Then for the next version tackle it for real and only
> save efi variable changes here and only if anything really did change.
>

As I mentioned on #u-boot, I think we can just comment the
env_save().. or perhaps make it a config option.  It will be mildly
annoying, since it means every boot is "first boot", ie. falling back
to fallback.efi to populate BootOrder/Boot variables, and never
using bootmgr.  But since we can't set EFI variables from userspace
yet, it isn't really a regression (yet).

Longer term, maybe we need to split out EFI variables from u-boot env
vars..  I thought it would be nice to store them together, since it
gives a convenient way to set EFI variables from script or cmdline.
But at least a subset of the EFI vars should be persisted to reach
EBBR, and if that breaks expectations about u-boot env vars, then I
guess we need to split them.

Side note, devices that can't repeatedly save env (even if we split
out the EFI variables to be stored separately) might be problematic
with EFI_LOADER.  Maybe we need a build option they can set to have a
more crippled version of EFI_LOADER, so as to not penalize more
capable devices.

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

Re: [U-Boot] Unexpected saving of all environment variables during EFI boot

[Alexander Graf]

On 19.10.17 22:40, Heinrich Schuchardt wrote:
> This was merged as
> ad644e7c18238026fecc647f62584d87d2c5b0a3
> efi_loader: efi variable support
> 
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index ec40f41bcb..c406ff82ff 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -8,6 +8,7 @@
>>  
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -942,6 +943,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(void 
>> *image_handle,
>>  {
>>  EFI_ENTRY("%p, %ld", image_handle, map_key);
>>  
>> +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
>> +/* save any EFI variables that have been written: */
>> +env_save();
> 
> You save all changed variables and not specifically those bound to EFI.
> This is completely unexpected behavior for the user and not covered by
> the commit message.
> 
> Furthermore you call env_save even if no EFI variable has been touched
> at all.
> 
> This leads to writing to flash on every boot via EFI. Depending on
> technology this might ruin the flash after a few hundred reboots.

I agree that overwriting flash on every boot isn't a terribly smart idea.

Also saving random environment variables that are set while we are in a
distro loop to persistent storage isn't great either.

I agree that we should probably improve this code to only save efi
variables.

Rob, how bad would it be for Fedora if we remove the persistent variable
store call (just the env_save() line) for 2017.11 to not kill people's
storage devices? Then for the next version tackle it for real and only
save efi variable changes here and only if anything really did change.


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

[U-Boot] Unexpected saving of all environment variables during EFI boot

[Heinrich Schuchardt]

This was merged as
ad644e7c18238026fecc647f62584d87d2c5b0a3
efi_loader: efi variable support

> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index ec40f41bcb..c406ff82ff 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -8,6 +8,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -942,6 +943,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(void 
> *image_handle,
>  {
>   EFI_ENTRY("%p, %ld", image_handle, map_key);
>  
> +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
> + /* save any EFI variables that have been written: */
> + env_save();

You save all changed variables and not specifically those bound to EFI.
This is completely unexpected behavior for the user and not covered by
the commit message.

Furthermore you call env_save even if no EFI variable has been touched
at all.

This leads to writing to flash on every boot via EFI. Depending on
technology this might ruin the flash after a few hundred reboots.

Please, revert this part of the change.

Best regards

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