On 19.10.17 23:15, Rob Clark wrote: > On Thu, Oct 19, 2017 at 5:05 PM, Alexander Graf <[email protected]> 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 <common.h> >>>> #include <efi_loader.h> >>>> +#include <environment.h> >>>> #include <malloc.h> >>>> #include <asm/global_data.h> >>>> #include <libfdt_env.h> >>>> @@ -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/BootNNNN 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 [email protected] https://lists.denx.de/listinfo/u-boot
