diff mbox series

[U-Boot,v3,14/21] efi_loader: efi variable support

Message ID 20170913220546.19560-15-robdclark@gmail.com
State Accepted
Delegated to: Alexander Graf
Headers show
Series efi_loader: enough UEFI for standard distro boot | expand

Commit Message

Rob Clark Sept. 13, 2017, 10:05 p.m. UTC
Add EFI variable support, mapping to u-boot environment variables.
Variables are pretty important for setting up boot order, among other
things.  If the board supports saveenv, then it will be called in
ExitBootServices() to persist variables set by the efi payload.  (For
example, fallback.efi configuring BootOrder and BootXXXX load-option
variables.)

Variables are *not* currently exposed at runtime, post ExitBootServices.
On boards without a dedicated device for storage, which the loaded OS
is not trying to also use, this is rather tricky.  One idea, at least
for boards that can persist RAM across reboot, is to keep a "journal"
of modified variables in RAM, and then turn halt into a reboot into
u-boot, plus store variables, plus halt.  Whatever the solution, it
likely involves some per-board support.

Mapping between EFI variables and u-boot variables:

  efi_$guid_$varname = {attributes}(type)value

For example:

  efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported=
     "{ro,boot,run}(blob)0000000000000000"
  efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_BootOrder=
     "(blob)00010000"

The attributes are a comma separated list of these possible
attributes:

  + ro   - read-only
  + boot - boot-services access
  + run  - runtime access

NOTE: with current implementation, no variables are available after
ExitBootServices, and all are persisted (if possible).

If not specified, the attributes default to "{boot}".

The required type is one of:

  + utf8 - raw utf8 string
  + blob - arbitrary length hex string

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 cmd/bootefi.c                 |   4 +
 include/efi.h                 |  19 +++
 include/efi_loader.h          |  10 ++
 lib/efi_loader/Makefile       |   2 +-
 lib/efi_loader/efi_boottime.c |   6 +
 lib/efi_loader/efi_runtime.c  |  17 ++-
 lib/efi_loader/efi_variable.c | 335 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 388 insertions(+), 5 deletions(-)
 create mode 100644 lib/efi_loader/efi_variable.c

Comments

Alexander Graf Sept. 21, 2017, 7:05 a.m. UTC | #1
> Add EFI variable support, mapping to u-boot environment variables.
> Variables are pretty important for setting up boot order, among other
> things.  If the board supports saveenv, then it will be called in
> ExitBootServices() to persist variables set by the efi payload.  (For
> example, fallback.efi configuring BootOrder and BootXXXX load-option
> variables.)
> 
> Variables are *not* currently exposed at runtime, post ExitBootServices.
> On boards without a dedicated device for storage, which the loaded OS
> is not trying to also use, this is rather tricky.  One idea, at least
> for boards that can persist RAM across reboot, is to keep a "journal"
> of modified variables in RAM, and then turn halt into a reboot into
> u-boot, plus store variables, plus halt.  Whatever the solution, it
> likely involves some per-board support.
> 
> Mapping between EFI variables and u-boot variables:
> 
>   efi_$guid_$varname = {attributes}(type)value
> 
> For example:
> 
>   efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported=
>      "{ro,boot,run}(blob)0000000000000000"
>   efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_BootOrder=
>      "(blob)00010000"
> 
> The attributes are a comma separated list of these possible
> attributes:
> 
>   + ro   - read-only
>   + boot - boot-services access
>   + run  - runtime access
> 
> NOTE: with current implementation, no variables are available after
> ExitBootServices, and all are persisted (if possible).
> 
> If not specified, the attributes default to "{boot}".
> 
> The required type is one of:
> 
>   + utf8 - raw utf8 string
>   + blob - arbitrary length hex string
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Thanks, applied to efi-next

Alex
Heinrich Schuchardt Oct. 19, 2017, 8:40 p.m. UTC | #2
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.

Please, revert this part of the change.

Best regards

Heinrich Schuchardt
Alexander Graf Oct. 19, 2017, 9:05 p.m. UTC | #3
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.


Alex
Rob Clark Oct. 19, 2017, 9:15 p.m. UTC | #4
On Thu, Oct 19, 2017 at 5:05 PM, Alexander Graf <agraf@suse.de> 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).

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
Alexander Graf Oct. 19, 2017, 9:28 p.m. UTC | #5
On 19.10.17 23:15, Rob Clark wrote:
> On Thu, Oct 19, 2017 at 5:05 PM, Alexander Graf <agraf@suse.de> 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
Heinrich Schuchardt Oct. 19, 2017, 10:25 p.m. UTC | #6
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 <agraf@suse.de> 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.

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
diff mbox series

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 0980088668..d3ae33e25b 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -181,6 +181,10 @@  static unsigned long do_bootefi_exec(void *efi, void *fdt,
 		goto exit;
 	}
 
+	/* we don't support much: */
+	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
+		"{ro,boot}(blob)0000000000000000");
+
 	/* Call our payload! */
 	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
 
diff --git a/include/efi.h b/include/efi.h
index ddd2b96417..04e83220b4 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -324,6 +324,25 @@  extern char image_base[];
 /* Start and end of U-Boot image (for payload) */
 extern char _binary_u_boot_bin_start[], _binary_u_boot_bin_end[];
 
+/*
+ * Variable Attributes
+ */
+#define EFI_VARIABLE_NON_VOLATILE       0x0000000000000001
+#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x0000000000000002
+#define EFI_VARIABLE_RUNTIME_ACCESS     0x0000000000000004
+#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x0000000000000008
+#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x0000000000000010
+#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x0000000000000020
+#define EFI_VARIABLE_APPEND_WRITE	0x0000000000000040
+
+#define EFI_VARIABLE_MASK	(EFI_VARIABLE_NON_VOLATILE | \
+				EFI_VARIABLE_BOOTSERVICE_ACCESS | \
+				EFI_VARIABLE_RUNTIME_ACCESS | \
+				EFI_VARIABLE_HARDWARE_ERROR_RECORD | \
+				EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS | \
+				EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS | \
+				EFI_VARIABLE_APPEND_WRITE)
+
 /**
  * efi_get_sys_table() - Get access to the main EFI system table
  *
diff --git a/include/efi_loader.h b/include/efi_loader.h
index b0c1e8fb78..9eee62dc9c 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -277,6 +277,16 @@  efi_status_t __efi_runtime EFIAPI efi_get_time(
 			struct efi_time_cap *capabilities);
 void efi_get_time_init(void);
 
+efi_status_t EFIAPI efi_get_variable(s16 *variable_name,
+		efi_guid_t *vendor, u32 *attributes,
+		unsigned long *data_size, void *data);
+efi_status_t EFIAPI efi_get_next_variable(
+		unsigned long *variable_name_size,
+		s16 *variable_name, efi_guid_t *vendor);
+efi_status_t EFIAPI efi_set_variable(s16 *variable_name,
+		efi_guid_t *vendor, u32 attributes,
+		unsigned long data_size, void *data);
+
 #else /* defined(EFI_LOADER) && !defined(CONFIG_SPL_BUILD) */
 
 /* Without CONFIG_EFI_LOADER we don't have a runtime section, stub it out */
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index cce92cfeb5..f58cb13337 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -16,7 +16,7 @@  always := $(efiprogs-y)
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
 obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
 obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
-obj-y += efi_file.o
+obj-y += efi_file.o efi_variable.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
 obj-$(CONFIG_PARTITIONS) += efi_disk.o
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();
+#endif
+
 	board_quiesce_devices();
 
 	/* Fix up caches for EFI payloads if necessary */
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index ad7f3754bd..2f95c766ac 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -184,7 +184,16 @@  static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
 		/* Clean up system table */
 		.ptr = &systab.boottime,
 		.patchto = NULL,
-	},
+	}, {
+		.ptr = &efi_runtime_services.get_variable,
+		.patchto = &efi_device_error,
+	}, {
+		.ptr = &efi_runtime_services.get_next_variable,
+		.patchto = &efi_device_error,
+	}, {
+		.ptr = &efi_runtime_services.set_variable,
+		.patchto = &efi_device_error,
+	}
 };
 
 static bool efi_runtime_tobedetached(void *p)
@@ -382,9 +391,9 @@  struct efi_runtime_services __efi_runtime_data efi_runtime_services = {
 	.set_wakeup_time = (void *)&efi_unimplemented,
 	.set_virtual_address_map = &efi_set_virtual_address_map,
 	.convert_pointer = (void *)&efi_invalid_parameter,
-	.get_variable = (void *)&efi_device_error,
-	.get_next_variable = (void *)&efi_device_error,
-	.set_variable = (void *)&efi_device_error,
+	.get_variable = efi_get_variable,
+	.get_next_variable = efi_get_next_variable,
+	.set_variable = efi_set_variable,
 	.get_next_high_mono_count = (void *)&efi_device_error,
 	.reset_system = &efi_reset_system_boottime,
 };
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
new file mode 100644
index 0000000000..5569b3d3f0
--- /dev/null
+++ b/lib/efi_loader/efi_variable.c
@@ -0,0 +1,335 @@ 
+/*
+ *  EFI utils
+ *
+ *  Copyright (c) 2017 Rob Clark
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <malloc.h>
+#include <charset.h>
+#include <efi_loader.h>
+
+#define READ_ONLY BIT(31)
+
+/*
+ * Mapping between EFI variables and u-boot variables:
+ *
+ *   efi_$guid_$varname = {attributes}(type)value
+ *
+ * For example:
+ *
+ *   efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported=
+ *      "{ro,boot,run}(blob)0000000000000000"
+ *   efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_BootOrder=
+ *      "(blob)00010000"
+ *
+ * The attributes are a comma separated list of these possible
+ * attributes:
+ *
+ *   + ro   - read-only
+ *   + boot - boot-services access
+ *   + run  - runtime access
+ *
+ * NOTE: with current implementation, no variables are available after
+ * ExitBootServices, and all are persisted (if possible).
+ *
+ * If not specified, the attributes default to "{boot}".
+ *
+ * The required type is one of:
+ *
+ *   + utf8 - raw utf8 string
+ *   + blob - arbitrary length hex string
+ *
+ * Maybe a utf16 type would be useful to for a string value to be auto
+ * converted to utf16?
+ */
+
+#define MAX_VAR_NAME 31
+#define MAX_NATIVE_VAR_NAME \
+	(strlen("efi_xxxxxxxx-xxxx-xxxx-xxxxxxxxxxxxxxxx_") + \
+		(MAX_VAR_NAME * MAX_UTF8_PER_UTF16))
+
+static int hex(unsigned char ch)
+{
+	if (ch >= 'a' && ch <= 'f')
+		return ch-'a'+10;
+	if (ch >= '0' && ch <= '9')
+		return ch-'0';
+	if (ch >= 'A' && ch <= 'F')
+		return ch-'A'+10;
+	return -1;
+}
+
+static const char *hex2mem(u8 *mem, const char *hexstr, int count)
+{
+	memset(mem, 0, count/2);
+
+	do {
+		int nibble;
+
+		*mem = 0;
+
+		if (!count || !*hexstr)
+			break;
+
+		nibble = hex(*hexstr);
+		if (nibble < 0)
+			break;
+
+		*mem = nibble;
+		count--;
+		hexstr++;
+
+		if (!count || !*hexstr)
+			break;
+
+		nibble = hex(*hexstr);
+		if (nibble < 0)
+			break;
+
+		*mem = (*mem << 4) | nibble;
+		count--;
+		hexstr++;
+		mem++;
+
+	} while (1);
+
+	if (*hexstr)
+		return hexstr;
+
+	return NULL;
+}
+
+static char *mem2hex(char *hexstr, const u8 *mem, int count)
+{
+	static const char hexchars[] = "0123456789abcdef";
+
+	while (count-- > 0) {
+		u8 ch = *mem++;
+		*hexstr++ = hexchars[ch >> 4];
+		*hexstr++ = hexchars[ch & 0xf];
+	}
+
+	return hexstr;
+}
+
+static efi_status_t efi_to_native(char *native, s16 *variable_name,
+		efi_guid_t *vendor)
+{
+	size_t len;
+
+	len = utf16_strlen((u16 *)variable_name);
+	if (len >= MAX_VAR_NAME)
+		return EFI_DEVICE_ERROR;
+
+	native += sprintf(native, "efi_%pUl_", vendor);
+	native  = (char *)utf16_to_utf8((u8 *)native, (u16 *)variable_name, len);
+	*native = '\0';
+
+	return EFI_SUCCESS;
+}
+
+static const char *prefix(const char *str, const char *prefix)
+{
+	size_t n = strlen(prefix);
+	if (!strncmp(prefix, str, n))
+		return str + n;
+	return NULL;
+}
+
+/* parse attributes part of variable value, if present: */
+static const char *parse_attr(const char *str, u32 *attrp)
+{
+	u32 attr = 0;
+	char sep = '{';
+
+	if (*str != '{') {
+		*attrp = EFI_VARIABLE_BOOTSERVICE_ACCESS;
+		return str;
+	}
+
+	while (*str == sep) {
+		const char *s;
+
+		str++;
+
+		if ((s = prefix(str, "ro"))) {
+			attr |= READ_ONLY;
+		} else if ((s = prefix(str, "boot"))) {
+			attr |= EFI_VARIABLE_BOOTSERVICE_ACCESS;
+		} else if ((s = prefix(str, "run"))) {
+			attr |= EFI_VARIABLE_RUNTIME_ACCESS;
+		} else {
+			printf("invalid attribute: %s\n", str);
+			break;
+		}
+
+		str = s;
+		sep = ',';
+	}
+
+	str++;
+
+	*attrp = attr;
+
+	return str;
+}
+
+/* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetVariable.28.29 */
+efi_status_t EFIAPI efi_get_variable(s16 *variable_name,
+		efi_guid_t *vendor, u32 *attributes,
+		unsigned long *data_size, void *data)
+{
+	char native_name[MAX_NATIVE_VAR_NAME + 1];
+	efi_status_t ret;
+	unsigned long in_size;
+	const char *val, *s;
+	u32 attr;
+
+	EFI_ENTRY("%p %p %p %p %p", variable_name, vendor, attributes,
+		  data_size, data);
+
+	if (!variable_name || !vendor || !data_size)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	ret = efi_to_native(native_name, variable_name, vendor);
+	if (ret)
+		return EFI_EXIT(ret);
+
+	debug("%s: get '%s'\n", __func__, native_name);
+
+	val = env_get(native_name);
+	if (!val)
+		return EFI_EXIT(EFI_NOT_FOUND);
+
+	val = parse_attr(val, &attr);
+
+	in_size = *data_size;
+
+	if ((s = prefix(val, "(blob)"))) {
+		unsigned len = strlen(s);
+
+		/* two characters per byte: */
+		len = DIV_ROUND_UP(len, 2);
+		*data_size = len;
+
+		if (in_size < len)
+			return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
+
+		if (!data)
+			return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+		if (hex2mem(data, s, len * 2))
+			return EFI_EXIT(EFI_DEVICE_ERROR);
+
+		debug("%s: got value: \"%s\"\n", __func__, s);
+	} else if ((s = prefix(val, "(utf8)"))) {
+		unsigned len = strlen(s) + 1;
+
+		*data_size = len;
+
+		if (in_size < len)
+			return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
+
+		if (!data)
+			return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+		memcpy(data, s, len);
+		((char *)data)[len] = '\0';
+
+		debug("%s: got value: \"%s\"\n", __func__, (char *)data);
+	} else {
+		debug("%s: invalid value: '%s'\n", __func__, val);
+		return EFI_EXIT(EFI_DEVICE_ERROR);
+	}
+
+	if (attributes)
+		*attributes = attr & EFI_VARIABLE_MASK;
+
+	return EFI_EXIT(EFI_SUCCESS);
+}
+
+/* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */
+efi_status_t EFIAPI efi_get_next_variable(
+		unsigned long *variable_name_size,
+		s16 *variable_name, efi_guid_t *vendor)
+{
+	EFI_ENTRY("%p %p %p", variable_name_size, variable_name, vendor);
+
+	return EFI_EXIT(EFI_DEVICE_ERROR);
+}
+
+/* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#SetVariable.28.29 */
+efi_status_t EFIAPI efi_set_variable(s16 *variable_name,
+		efi_guid_t *vendor, u32 attributes,
+		unsigned long data_size, void *data)
+{
+	char native_name[MAX_NATIVE_VAR_NAME + 1];
+	efi_status_t ret = EFI_SUCCESS;
+	char *val, *s;
+	u32 attr;
+
+	EFI_ENTRY("%p %p %x %lu %p", variable_name, vendor, attributes,
+		  data_size, data);
+
+	if (!variable_name || !vendor)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	ret = efi_to_native(native_name, variable_name, vendor);
+	if (ret)
+		return EFI_EXIT(ret);
+
+#define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
+
+	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
+		/* delete the variable: */
+		env_set(native_name, NULL);
+		return EFI_EXIT(EFI_SUCCESS);
+	}
+
+	val = env_get(native_name);
+	if (val) {
+		parse_attr(val, &attr);
+
+		if (attr & READ_ONLY)
+			return EFI_EXIT(EFI_WRITE_PROTECTED);
+	}
+
+	val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1);
+	if (!val)
+		return EFI_EXIT(EFI_OUT_OF_RESOURCES);
+
+	s = val;
+
+	/* store attributes: */
+	attributes &= (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS);
+	s += sprintf(s, "{");
+	while (attributes) {
+		u32 attr = 1 << (ffs(attributes) - 1);
+
+		if (attr == EFI_VARIABLE_BOOTSERVICE_ACCESS)
+			s += sprintf(s, "boot");
+		else if (attr == EFI_VARIABLE_RUNTIME_ACCESS)
+			s += sprintf(s, "run");
+
+		attributes &= ~attr;
+		if (attributes)
+			s += sprintf(s, ",");
+	}
+	s += sprintf(s, "}");
+
+	/* store payload: */
+	s += sprintf(s, "(blob)");
+	s = mem2hex(s, data, data_size);
+	*s = '\0';
+
+	debug("%s: setting: %s=%s\n", __func__, native_name, val);
+
+	if (env_set(native_name, val))
+		ret = EFI_DEVICE_ERROR;
+
+	free(val);
+
+	return EFI_EXIT(ret);
+}