On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

OS does not always need to call SetVirtualAddressMap. (Ard confirmed this on arm64 linux.) So let this API configurable. If disabled, it will return EFI_UNSUPPORTED as UEFI specification requires.

Currently we do not support this scenario. Alex's patch should go in first.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/Kconfig | 7 +++++++ lib/efi_loader/efi_runtime.c | 8 ++++++++ 2 files changed, 15 insertions(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 8bf4b1754d06..bb9c7582b14d 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -44,6 +44,13 @@ config EFI_SET_TIME Provide the SetTime() runtime service at boottime. This service can be used by an EFI application to adjust the real time clock.

+config EFI_RUNTIME_SET_VIRTUAL_ADDRESS_MAP

• bool "runtime service: SetVirtualAddressMap"
• default n
• help

•  Enable SetVirtualAddressMap runtime service. This API will be
  

•  called by OS just before it enters into virtual address mode.
  

• config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 9c50955c9bd0..60442cb21d37 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -374,10 +374,12 @@ static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { /* do_reset is gone */ .ptr = &efi_runtime_services.reset_system, .patchto = efi_reset_system, +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP }, { /* invalidate_*cache_all are gone */ .ptr = &efi_runtime_services.set_virtual_address_map, .patchto = &efi_unimplemented, +#endif }, { /* RTC accessors are gone */ .ptr = &efi_runtime_services.get_time, @@ -512,6 +514,7 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) invalidate_icache_all(); }

+#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP /**

• efi_set_virtual_address_map() - change from physical to virtual mapping

@@ -619,6 +622,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(

return EFI_EXIT(EFI_INVALID_PARAMETER); } +#endif /* CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP */

/**

• efi_add_runtime_mmio() - add memory-mapped IO region

@@ -796,7 +800,11 @@ struct efi_runtime_services __efi_runtime_data efi_runtime_services = { .set_time = &efi_set_time_boottime, .get_wakeup_time = (void *)&efi_unimplemented, .set_wakeup_time = (void *)&efi_unimplemented, +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP .set_virtual_address_map = &efi_set_virtual_address_map, +#else

• .set_virtual_address_map = (void *)&efi_unimplemented,

Depending on the ABI it is not save to use a function with another set of parameters.

Best regards

Heinrich

+#endif .convert_pointer = (void *)&efi_invalid_parameter, .get_variable = efi_get_variable, .get_next_variable_name = efi_get_next_variable_name,

On 6/15/19 11:14 PM, Mark Kettenis wrote:

...

From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Sat, 15 Jun 2019 21:46:02 +0200

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

...

OS does not always need to call SetVirtualAddressMap. (Ard confirmed this on arm64 linux.) So let this API configurable. If disabled, it will return EFI_UNSUPPORTED as UEFI specification requires.

Currently we do not support this scenario. Alex's patch should go in first.

OpenBSD/arm64 will always call this function. It does this in order

OpenBSD/arm32 does not call it. I have no clue why the 32bit and 64bit code have diverged.

to randomize the virtual addresses used by runtime services to make it harder for an attacker to call into UEFI runtime services. Note that the UEFI 2.7 standard provides no indication that this interface might be optional, so I don't think OpenBSD is doing anything wrong here.

I think it is unwise to make this API configurable. But disabling it by default like this diff does would be a seriously bad idea. It means U-Boot would no longer be backwards compatible with UEFI 2.7.

The current UEFI specification is 2.8. It foresees that SetVirtualAddressMap() can be marked as unsupported but that does not imply that we have to make it customizable. And disabling it by default does not make much sense while most operating systems use it.

Best regards

Heinrich

...

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/Kconfig | 7 +++++++ lib/efi_loader/efi_runtime.c | 8 ++++++++ 2 files changed, 15 insertions(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 8bf4b1754d06..bb9c7582b14d 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -44,6 +44,13 @@ config EFI_SET_TIME Provide the SetTime() runtime service at boottime. This service can be used by an EFI application to adjust the real time clock.

+config EFI_RUNTIME_SET_VIRTUAL_ADDRESS_MAP

• bool "runtime service: SetVirtualAddressMap"
• default n
• help

•  Enable SetVirtualAddressMap runtime service. This API will be
  

•  called by OS just before it enters into virtual address mode.
  

• config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 9c50955c9bd0..60442cb21d37 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -374,10 +374,12 @@ static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { /* do_reset is gone */ .ptr = &efi_runtime_services.reset_system, .patchto = efi_reset_system, +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP }, { /* invalidate_*cache_all are gone */ .ptr = &efi_runtime_services.set_virtual_address_map, .patchto = &efi_unimplemented, +#endif }, { /* RTC accessors are gone */ .ptr = &efi_runtime_services.get_time, @@ -512,6 +514,7 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) invalidate_icache_all(); }

+#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP /** * efi_set_virtual_address_map() - change from physical to virtual mapping * @@ -619,6 +622,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(

return EFI_EXIT(EFI_INVALID_PARAMETER);

} +#endif /* CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP */

/** * efi_add_runtime_mmio() - add memory-mapped IO region @@ -796,7 +800,11 @@ struct efi_runtime_services __efi_runtime_data efi_runtime_services = { .set_time = &efi_set_time_boottime, .get_wakeup_time = (void *)&efi_unimplemented, .set_wakeup_time = (void *)&efi_unimplemented, +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP .set_virtual_address_map = &efi_set_virtual_address_map, +#else

• .set_virtual_address_map = (void *)&efi_unimplemented,

Depending on the ABI it is not save to use a function with another set of parameters.

Best regards

Heinrich

...

+#endif .convert_pointer = (void *)&efi_invalid_parameter, .get_variable = efi_get_variable, .get_next_variable_name = efi_get_next_variable_name,

---

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

On Sat, Jun 15, 2019 at 09:46:02PM +0200, Heinrich Schuchardt wrote:

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

...

OS does not always need to call SetVirtualAddressMap. (Ard confirmed this on arm64 linux.) So let this API configurable. If disabled, it will return EFI_UNSUPPORTED as UEFI specification requires.

Currently we do not support this scenario. Alex's patch should go in first.

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/Kconfig | 7 +++++++ lib/efi_loader/efi_runtime.c | 8 ++++++++ 2 files changed, 15 insertions(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 8bf4b1754d06..bb9c7582b14d 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -44,6 +44,13 @@ config EFI_SET_TIME Provide the SetTime() runtime service at boottime. This service can be used by an EFI application to adjust the real time clock.

+config EFI_RUNTIME_SET_VIRTUAL_ADDRESS_MAP

• bool "runtime service: SetVirtualAddressMap"
• default n
• help

•  Enable SetVirtualAddressMap runtime service. This API will be
  

•  called by OS just before it enters into virtual address mode.
  

config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 9c50955c9bd0..60442cb21d37 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -374,10 +374,12 @@ static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { /* do_reset is gone */ .ptr = &efi_runtime_services.reset_system, .patchto = efi_reset_system, +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP }, { /* invalidate_*cache_all are gone */ .ptr = &efi_runtime_services.set_virtual_address_map, .patchto = &efi_unimplemented, +#endif }, { /* RTC accessors are gone */ .ptr = &efi_runtime_services.get_time, @@ -512,6 +514,7 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) invalidate_icache_all(); }

+#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP /**

• efi_set_virtual_address_map() - change from physical to virtual mapping

@@ -619,6 +622,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(

return EFI_EXIT(EFI_INVALID_PARAMETER); } +#endif /* CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP */

/**

• efi_add_runtime_mmio() - add memory-mapped IO region

@@ -796,7 +800,11 @@ struct efi_runtime_services __efi_runtime_data efi_runtime_services = { .set_time = &efi_set_time_boottime, .get_wakeup_time = (void *)&efi_unimplemented, .set_wakeup_time = (void *)&efi_unimplemented, +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP .set_virtual_address_map = &efi_set_virtual_address_map, +#else

• .set_virtual_address_map = (void *)&efi_unimplemented,

Depending on the ABI it is not save to use a function with another set of parameters.

I don't think it's a good practice, but there already are a couple of precedents: get_wakeup_time set_wakeup_time get_next_high_mono_count, and convert_pointer

-Takahiro Akashi

Best regards

Heinrich

...

+#endif .convert_pointer = (void *)&efi_invalid_parameter, .get_variable = efi_get_variable, .get_next_variable_name = efi_get_next_variable_name,

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

This variable is defined in UEFI specification 2.8, section 8.1. Its value should be updated whenever we add any usable runtime services function.

It is also required by the EBBR specification.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

include/efi_api.h | 15 +++++++++++++++ include/efi_loader.h | 3 +++ lib/efi_loader/efi_runtime.c | 28 ++++++++++++++++++++++++++++ lib/efi_loader/efi_setup.c | 5 +++++ 4 files changed, 51 insertions(+)

diff --git a/include/efi_api.h b/include/efi_api.h index 65584dd2d82a..d7d95edd4dfc 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -213,6 +213,21 @@ struct efi_capsule_header { u32 capsule_image_size; };

+#define EFI_RT_SUPPORTED_GET_TIME 0x0001 +#define EFI_RT_SUPPORTED_SET_TIME 0x0002 +#define EFI_RT_SUPPORTED_GET_WAKEUP_TIME 0x0004 +#define EFI_RT_SUPPORTED_SET_WAKEUP_TIME 0x0008 +#define EFI_RT_SUPPORTED_GET_VARIABLE 0x0010 +#define EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME 0x0020 +#define EFI_RT_SUPPORTED_SET_VARIABLE 0x0040 +#define EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP 0x0080 +#define EFI_RT_SUPPORTED_CONVERT_POINTER 0x0100 +#define EFI_RT_SUPPORTED_GET_NEXT_HIGH_MONOTONIC_COUNT 0x0200 +#define EFI_RT_SUPPORTED_RESET_SYSTEM 0x0400 +#define EFI_RT_SUPPORTED_UPDATE_CAPSULE 0x0800 +#define EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES 0x1000 +#define EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO 0x2000

struct efi_runtime_services { struct efi_table_hdr hdr; efi_status_t (EFIAPI *get_time)(struct efi_time *time, diff --git a/include/efi_loader.h b/include/efi_loader.h index 23ce73226762..7bd8002e303e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -573,6 +573,9 @@ static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2) #define __efi_runtime_data __attribute__ ((section (".data.efi_runtime"))) #define __efi_runtime __attribute__ ((section (".text.efi_runtime")))

+/* Indicate supported runtime services */ +efi_status_t efi_init_runtime_supported(void);

/* Update CRC32 in table header */ void __efi_runtime efi_update_table_header_crc32(struct efi_table_hdr *table);

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 60442cb21d37..cf202bb9ec07 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -89,6 +89,34 @@ struct elf_rela {

• handle a good number of runtime callbacks

*/

+efi_status_t efi_init_runtime_supported(void) +{

• u16 efi_runtime_services_supported;
• /*

• * This value must be synced with efi_runtime_detach_list
  

• * as well as efi_runtime_services.
  

• */
  

• efi_runtime_services_supported = EFI_RT_SUPPORTED_RESET_SYSTEM;

This support is system dependent, e.g. on RK3288 systems it does not exist.

+#ifdef CONFIG_EFI_GET_TIME

• efi_runtime_services_supported |= EFI_RT_SUPPORTED_GET_TIME;

We do not support this at runtime.

+#endif +#ifdef CONFIG_EFI_SET_TIME

• efi_runtime_services_supported |= EFI_RT_SUPPORTED_SET_TIME;

We do not support this at runtime.

+#endif +#ifdef CONFIG_EFI_RUNTIME_SET_VIRTUAL_ADDRESS_MAP

• efi_runtime_services_supported |=

• 		EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP;
  

Our support is incomplete as we have not implemented ConvertPointer().

For unsupported services we will have to change the return value to EFI_UNSUPPORTED. But that will be a separate patch.

Best regards

Heinrich

+#endif

• return EFI_CALL(efi_set_variable(L"RuntimeServicesSupported",

• 			 &efi_global_variable_guid,
  

• 			 EFI_VARIABLE_BOOTSERVICE_ACCESS |
  

• 			 EFI_VARIABLE_RUNTIME_ACCESS,
  

• 			 sizeof(efi_runtime_services_supported),
  

• 			 &efi_runtime_services_supported));
  

+}

/**

• efi_update_table_header_crc32() - Update crc32 in table header

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index 45d6aca051f3..75fa344060d5 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -123,6 +123,11 @@ efi_status_t efi_init_obj_list(void) if (ret != EFI_SUCCESS) goto out;

• /* Indicate supported runtime services */
• ret = efi_init_runtime_supported();
• if (ret != EFI_SUCCESS)

• goto out;
  

• /* Initialize system table */ ret = efi_initialize_system_table(); if (ret != EFI_SUCCESS)

On 6/13/19 9:06 AM, AKASHI Takahiro wrote:

On Thu, Jun 13, 2019 at 07:56:19AM +0200, Heinrich Schuchardt wrote:

...

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

...

This variable is defined in UEFI specification 2.8, section 8.1. Its value should be updated whenever we add any usable runtime services function.

It is also required by the EBBR specification.

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

include/efi_api.h | 15 +++++++++++++++ include/efi_loader.h | 3 +++ lib/efi_loader/efi_runtime.c | 28 ++++++++++++++++++++++++++++ lib/efi_loader/efi_setup.c | 5 +++++ 4 files changed, 51 insertions(+)

diff --git a/include/efi_api.h b/include/efi_api.h index 65584dd2d82a..d7d95edd4dfc 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -213,6 +213,21 @@ struct efi_capsule_header { u32 capsule_image_size; };

+#define EFI_RT_SUPPORTED_GET_TIME 0x0001 +#define EFI_RT_SUPPORTED_SET_TIME 0x0002 +#define EFI_RT_SUPPORTED_GET_WAKEUP_TIME 0x0004 +#define EFI_RT_SUPPORTED_SET_WAKEUP_TIME 0x0008 +#define EFI_RT_SUPPORTED_GET_VARIABLE 0x0010 +#define EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME 0x0020 +#define EFI_RT_SUPPORTED_SET_VARIABLE 0x0040 +#define EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP 0x0080 +#define EFI_RT_SUPPORTED_CONVERT_POINTER 0x0100 +#define EFI_RT_SUPPORTED_GET_NEXT_HIGH_MONOTONIC_COUNT 0x0200 +#define EFI_RT_SUPPORTED_RESET_SYSTEM 0x0400 +#define EFI_RT_SUPPORTED_UPDATE_CAPSULE 0x0800 +#define EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES 0x1000 +#define EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO 0x2000

struct efi_runtime_services { struct efi_table_hdr hdr; efi_status_t (EFIAPI *get_time)(struct efi_time *time, diff --git a/include/efi_loader.h b/include/efi_loader.h index 23ce73226762..7bd8002e303e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -573,6 +573,9 @@ static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2) #define __efi_runtime_data __attribute__ ((section (".data.efi_runtime"))) #define __efi_runtime __attribute__ ((section (".text.efi_runtime")))

+/* Indicate supported runtime services */ +efi_status_t efi_init_runtime_supported(void);

/* Update CRC32 in table header */ void __efi_runtime efi_update_table_header_crc32(struct efi_table_hdr *table);

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 60442cb21d37..cf202bb9ec07 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -89,6 +89,34 @@ struct elf_rela {

• handle a good number of runtime callbacks

*/

+efi_status_t efi_init_runtime_supported(void) +{

• u16 efi_runtime_services_supported;
• /*

• * This value must be synced with efi_runtime_detach_list
  

• * as well as efi_runtime_services.
  

• */
  

• efi_runtime_services_supported = EFI_RT_SUPPORTED_RESET_SYSTEM;

This support is system dependent, e.g. on RK3288 systems it does not exist.

efi_reset_system() is defined as a weak function and so there is no easy way to determine whether this API is supported or not.

...

...

+#ifdef CONFIG_EFI_GET_TIME

• efi_runtime_services_supported |= EFI_RT_SUPPORTED_GET_TIME;

We do not support this at runtime.

Okay, I will drop it.

...

...

+#endif +#ifdef CONFIG_EFI_SET_TIME

• efi_runtime_services_supported |= EFI_RT_SUPPORTED_SET_TIME;

We do not support this at runtime.

ditto

...

...

+#endif +#ifdef CONFIG_EFI_RUNTIME_SET_VIRTUAL_ADDRESS_MAP

• efi_runtime_services_supported |=

• 		EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP;
  

Our support is incomplete as we have not implemented ConvertPointer().

Incomplete? My patch#3 adds ConvertPointer().

That is a later patch. So I did not consider it for this patch. Furthermore the implementation of ConvertPointer depends on CONFIG_EFI_RUNTIME_CONVERT_POINTER.

Best regards

Heinrich

-Takahiro Akashi

...

For unsupported services we will have to change the return value to EFI_UNSUPPORTED. But that will be a separate patch.

Best regards

Heinrich

...

+#endif

• return EFI_CALL(efi_set_variable(L"RuntimeServicesSupported",

• 			 &efi_global_variable_guid,
  

• 			 EFI_VARIABLE_BOOTSERVICE_ACCESS |
  

• 			 EFI_VARIABLE_RUNTIME_ACCESS,
  

• 			 sizeof(efi_runtime_services_supported),
  

• 			 &efi_runtime_services_supported));
  

+}

/**

• efi_update_table_header_crc32() - Update crc32 in table header

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index 45d6aca051f3..75fa344060d5 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -123,6 +123,11 @@ efi_status_t efi_init_obj_list(void) if (ret != EFI_SUCCESS) goto out;

• /* Indicate supported runtime services */
• ret = efi_init_runtime_supported();
• if (ret != EFI_SUCCESS)

• goto out;
  

• /* Initialize system table */ ret = efi_initialize_system_table(); if (ret != EFI_SUCCESS)

On Sat, Jun 15, 2019 at 09:41:31PM +0200, Heinrich Schuchardt wrote:

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

...

With this patch, ConvertPointer runtime service is enabled. This function will be useful only after SetVirtualAddressMap is called and before it exits according to UEFI specification.

ConvertPointer() is called by drivers upon calling the notification functions of events registered as EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE.

We still lack support for these events.

So? What do you want me to do here?

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/Kconfig | 8 ++++ lib/efi_loader/efi_runtime.c | 81 ++++++++++++++++++++++++++---------- 2 files changed, 66 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index bb9c7582b14d..e2ef43157568 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -51,6 +51,14 @@ config EFI_RUNTIME_SET_VIRTUAL_ADDRESS_MAP Enable SetVirtualAddressMap runtime service. This API will be called by OS just before it enters into virtual address mode.

+config EFI_RUNTIME_CONVERT_POINTER

• bool "runtime service: ConvertPointer"
• default n
• help

•  Enable ConvertPointer runtime service. This API will be expected
  

•  to be called by UEFI drivers in relocating themselves to virtual
  

•  address space.
  

config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index cf202bb9ec07..ff3684a4b692 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -27,7 +27,6 @@ LIST_HEAD(efi_runtime_mmio);

static efi_status_t __efi_runtime EFIAPI efi_unimplemented(void); static efi_status_t __efi_runtime EFIAPI efi_device_error(void); -static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void);

/*

• TODO(sjg@chromium.org): These defines and structures should come from the ELF

@@ -108,6 +107,10 @@ efi_status_t efi_init_runtime_supported(void) efi_runtime_services_supported |= EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP; #endif +#ifdef CONFIG_EFI_RUNTIME_CONVERT_POINTER

• efi_runtime_services_supported |=

• 		EFI_RT_SUPPORTED_CONVERT_POINTER;
  

+#endif

return EFI_CALL(efi_set_variable(L"RuntimeServicesSupported", &efi_global_variable_guid, @@ -392,6 +395,39 @@ efi_status_t __weak __efi_runtime EFIAPI efi_set_time(struct efi_time *time) return EFI_UNSUPPORTED; }

+#ifdef CONFIG_EFI_RUNTIME_CONVERT_POINTER +static struct efi_mem_desc *efi_virtmap __efi_runtime_data; +static int efi_virtmap_num __efi_runtime_data;

Please, put a description of the function and its parameters here.

Okay.

...

+static efi_status_t __efi_runtime EFIAPI efi_convert_pointer(unsigned long dbg,

• 					     void **address)
  

+{

• struct efi_mem_desc *map;
• efi_physical_addr_t addr;
• int i;
• if (!efi_virtmap)

• return EFI_UNSUPPORTED;
  

• if (!address)

• return EFI_INVALID_PARAMETER;
  

• for (i = 0, map = efi_virtmap; i < efi_virtmap_num; i++, map++) {

• addr = (efi_physical_addr_t)*address;
  

This line should be before the loop.

...

• if (addr >= map->physical_start &&
  

•     (addr < (map->physical_start
  

%s/(addr/addr/ No need for that extra parenthesis.

...

• 	     + (map->num_pages << EFI_PAGE_SHIFT)))) {
  

• 	*address = (void *)map->virtual_start;
  

I guess on 32bit this will end in a warning? Wouldn't you need to convert to uintptr_t first?

...

• 	*address += addr - map->physical_start;
  

I think a single assignment is enough. Here you are updating a 32bit pointer with the difference of two u64. I would expect a warning.

I will check.

*address = (void *)(uintptr_t) (addr - map->physical_start + map->virtual_start);

Or do all calculation with addr before copying to *address.

...

• 	return EFI_SUCCESS;
  

• }
  

• }
• return EFI_NOT_FOUND;

+} +#endif

struct efi_runtime_detach_list_struct { void *ptr; void *patchto; @@ -599,6 +635,10 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( return EFI_EXIT(EFI_INVALID_PARAMETER); }

• efi_virtmap = virtmap;
• efi_virtmap_num = n;

+#if 0 /* FIXME: This code is fragile as calloc is used in add_runtime_mmio */ /* Rebind mmio pointers */ for (i = 0; i < n; i++) { struct efi_mem_desc *map = (void*)virtmap + @@ -622,14 +662,14 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( *lmmio->ptr = (void *)new_addr; } }

• if ((map_start <= (uintptr_t)systab.tables) &&
  

•     (map_end >= (uintptr_t)systab.tables)) {
  

• 	char *ptr = (char *)systab.tables;
  

• 	ptr += off;
  

• 	systab.tables = (struct efi_configuration_table *)ptr;
  

• }
  

This looks like an unrelated change.

It does. This code should be replaced by:

...

• /* FIXME */
• efi_convert_pointer(0, (void **)&systab.tables);

-Takahiro Akashi

Put it into a separate patch, please.

Best regards

Heinrich

...

} +#endif

• /* FIXME */

• efi_convert_pointer(0, (void **)&systab.tables);

• /* All fixes must be done before this line */

• efi_virtmap = NULL;

  /* Move the actual runtime code over */ for (i = 0; i < n; i++) {

@@ -644,6 +684,11 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( /* Once we're virtual, we can no longer handle complex callbacks */ efi_runtime_detach(new_offset);

• 	/*
  

• 	 * FIXME:
  

• 	 * We can no longer update RuntimeServicesSupported.
  

• 	 */
  return EFI_EXIT(EFI_SUCCESS);
  

  } }

@@ -733,20 +778,6 @@ static efi_status_t __efi_runtime EFIAPI efi_device_error(void) return EFI_DEVICE_ERROR; }

-/**

• • efi_invalid_parameter() - replacement function, returns EFI_INVALID_PARAMETER
• • This function is used after SetVirtualAddressMap() is called as replacement
• • for services that are not available anymore due to constraints of the U-Boot
• • implementation.
• • Return: EFI_INVALID_PARAMETER
• */

-static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void) -{

• return EFI_INVALID_PARAMETER;

-}

/**

• efi_update_capsule() - process information from operating system

@@ -833,7 +864,11 @@ struct efi_runtime_services __efi_runtime_data efi_runtime_services = { #else .set_virtual_address_map = (void *)&efi_unimplemented, #endif

• .convert_pointer = (void *)&efi_invalid_parameter,

+#ifdef CONFIG_EFI_RUNTIME_CONVERT_POINTER

• .convert_pointer = &efi_convert_pointer,

+#else

• .convert_pointer = (void *)&efi_unimplemented,

I feel uneasy using a function efi_unimplemented() with a different number of parameters here. Depending on the ABI this may lead to a crash.

Best regards

Heinrich

...

+#endif .get_variable = efi_get_variable, .get_next_variable_name = efi_get_next_variable_name, .set_variable = efi_set_variable,

On 6/17/19 7:41 AM, Heinrich Schuchardt wrote:

On 6/17/19 3:15 AM, AKASHI Takahiro wrote:

...

On Sat, Jun 15, 2019 at 09:41:31PM +0200, Heinrich Schuchardt wrote:

...

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

...

With this patch, ConvertPointer runtime service is enabled. This function will be useful only after SetVirtualAddressMap is called and before it exits according to UEFI specification.

ConvertPointer() is called by drivers upon calling the notification functions of events registered as EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE.

We still lack support for these events.

So? What do you want me to do here?

We will have to address this in a future patch.

Now that EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE is implemented and a unit test for ConvertPointer() provided we should proceed with implementing ConvertPointer().

Regards Heinrich

Regards Heinrich

...

...

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/Kconfig       |  8 ++++   lib/efi_loader/efi_runtime.c | 81 ++++++++++++++++++++++++++----------   2 files changed, 66 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index bb9c7582b14d..e2ef43157568 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -51,6 +51,14 @@ config EFI_RUNTIME_SET_VIRTUAL_ADDRESS_MAP         Enable SetVirtualAddressMap runtime service. This API will be         called by OS just before it enters into virtual address mode.

+config EFI_RUNTIME_CONVERT_POINTER +    bool "runtime service: ConvertPointer" +    default n +    help +      Enable ConvertPointer runtime service. This API will be expected +      to be called by UEFI drivers in relocating themselves to virtual +      address space.

config EFI_DEVICE_PATH_TO_TEXT       bool "Device path to text protocol"       default y diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index cf202bb9ec07..ff3684a4b692 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -27,7 +27,6 @@ LIST_HEAD(efi_runtime_mmio);

static efi_status_t __efi_runtime EFIAPI efi_unimplemented(void);   static efi_status_t __efi_runtime EFIAPI efi_device_error(void); -static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void);

/*    * TODO(sjg@chromium.org): These defines and structures should come from the ELF @@ -108,6 +107,10 @@ efi_status_t efi_init_runtime_supported(void)       efi_runtime_services_supported |=                   EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP;   #endif +#ifdef CONFIG_EFI_RUNTIME_CONVERT_POINTER +    efi_runtime_services_supported |= +                EFI_RT_SUPPORTED_CONVERT_POINTER; +#endif

return EFI_CALL(efi_set_variable(L"RuntimeServicesSupported",                        &efi_global_variable_guid, @@ -392,6 +395,39 @@ efi_status_t __weak __efi_runtime EFIAPI efi_set_time(struct efi_time *time)       return EFI_UNSUPPORTED;   }

+#ifdef CONFIG_EFI_RUNTIME_CONVERT_POINTER +static struct efi_mem_desc *efi_virtmap __efi_runtime_data; +static int efi_virtmap_num __efi_runtime_data;

Please, put a description of the function and its parameters here.

Okay.

...

...

+static efi_status_t __efi_runtime EFIAPI efi_convert_pointer(unsigned long dbg, +                                 void **address) +{ +    struct efi_mem_desc *map; +    efi_physical_addr_t addr; +    int i;

+    if (!efi_virtmap) +        return EFI_UNSUPPORTED;

+    if (!address) +        return EFI_INVALID_PARAMETER;

+    for (i = 0, map = efi_virtmap; i < efi_virtmap_num; i++, map++) { +        addr = (efi_physical_addr_t)*address;

This line should be before the loop.

...

+        if (addr >= map->physical_start && +            (addr < (map->physical_start

%s/(addr/addr/  No need for that extra parenthesis.

...

+                 + (map->num_pages << EFI_PAGE_SHIFT)))) { +            *address = (void *)map->virtual_start;

I guess on 32bit this will end in a warning? Wouldn't you need to convert to uintptr_t first?

...

+            *address += addr - map->physical_start;

I think a single assignment is enough. Here you are updating a 32bit pointer with the difference of two u64. I would expect a warning.

I will check.

...

*address = (void *)(uintptr_t) (addr - map->physical_start + map->virtual_start);

Or do all calculation with addr before copying to *address.

...

+            return EFI_SUCCESS; +        } +    }

+    return EFI_NOT_FOUND; +} +#endif

struct efi_runtime_detach_list_struct {       void *ptr;       void *patchto; @@ -599,6 +635,10 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(           return EFI_EXIT(EFI_INVALID_PARAMETER);       }

+    efi_virtmap = virtmap; +    efi_virtmap_num = n;

+#if 0 /* FIXME: This code is fragile as calloc is used in add_runtime_mmio */       /* Rebind mmio pointers */       for (i = 0; i < n; i++) {           struct efi_mem_desc *map = (void*)virtmap + @@ -622,14 +662,14 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(                   *lmmio->ptr = (void *)new_addr;               }           } -        if ((map_start <= (uintptr_t)systab.tables) && -            (map_end >= (uintptr_t)systab.tables)) { -            char *ptr = (char *)systab.tables;

-            ptr += off; -            systab.tables = (struct efi_configuration_table *)ptr; -        }

This looks like an unrelated change.

It does. This code should be replaced by:

...

...

+    /* FIXME */ +    efi_convert_pointer(0, (void **)&systab.tables);

-Takahiro Akashi

...

Put it into a separate patch, please.

Best regards

Heinrich

...

} +#endif

+    /* FIXME */ +    efi_convert_pointer(0, (void **)&systab.tables);

+    /* All fixes must be done before this line */ +    efi_virtmap = NULL;

/* Move the actual runtime code over */       for (i = 0; i < n; i++) { @@ -644,6 +684,11 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(               /* Once we're virtual, we can no longer handle                  complex callbacks */               efi_runtime_detach(new_offset);

+            /* +             * FIXME: +             * We can no longer update RuntimeServicesSupported. +             */               return EFI_EXIT(EFI_SUCCESS);           }       } @@ -733,20 +778,6 @@ static efi_status_t __efi_runtime EFIAPI efi_device_error(void)       return EFI_DEVICE_ERROR;   }

-/**

• • efi_invalid_parameter() - replacement function, returns

EFI_INVALID_PARAMETER

• • This function is used after SetVirtualAddressMap() is called as

replacement

• • for services that are not available anymore due to constraints

of the U-Boot

• • implementation.
• • Return:    EFI_INVALID_PARAMETER
• */

-static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void) -{ -    return EFI_INVALID_PARAMETER; -}

/**    * efi_update_capsule() - process information from operating system    * @@ -833,7 +864,11 @@ struct efi_runtime_services __efi_runtime_data efi_runtime_services = {   #else       .set_virtual_address_map = (void *)&efi_unimplemented,   #endif -    .convert_pointer = (void *)&efi_invalid_parameter, +#ifdef CONFIG_EFI_RUNTIME_CONVERT_POINTER +    .convert_pointer = &efi_convert_pointer, +#else +    .convert_pointer = (void *)&efi_unimplemented,

I feel uneasy using a function efi_unimplemented() with a different number of parameters here. Depending on the ABI this may lead to a crash.

Best regards

Heinrich

...

+#endif       .get_variable = efi_get_variable,       .get_next_variable_name = efi_get_next_variable_name,       .set_variable = efi_set_variable,

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

With this patch, cache buffer for UEFI variables will be created so that we will still be able to access, at least retrieve, UEFI variables when we exit from boottime services,

This feature is a "should" behavior described in EBBR v1.0 section 2.5.3.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

include/efi_loader.h | 17 ++ lib/efi_loader/Kconfig | 9 + lib/efi_loader/efi_boottime.c | 10 +- lib/efi_loader/efi_runtime.c | 13 + lib/efi_loader/efi_variable.c | 467 ++++++++++++++++++++++++++++++++++ 5 files changed, 515 insertions(+), 1 deletion(-)

Please, put the cache into a separate file.

diff --git a/include/efi_loader.h b/include/efi_loader.h index 93f7ece814a0..acab657b9d70 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -620,6 +620,23 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data);

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING +efi_status_t efi_freeze_variable_table(void);

+/* runtime version of APIs */ +efi_status_t +__efi_runtime EFIAPI efi_get_variable_runtime(u16 *variable_name,

I think one version of the functions serving at runtime and boottime is enough.

The cache should be used both at runtime and at boottime. Essentially I expect three modules working together:

UEFI API implementation <-> Cache <-> Persistence driver

I would suggest to put each of these into a separate file.

Both the API implementation and the Cache have to be available at Boottime and at Runtime. A first version of the persistence driver may only be working at boottime.

The NV-cache content should be written to non-volatile memory on Reset() and on ExitBootServices() and if possible when updating variables at runtime.

• 			      const efi_guid_t *vendor,
  

• 			      u32 *attributes,
  

• 			      efi_uintn_t *data_size,
  

• 			      void *data);
  

+efi_status_t +__efi_runtime EFIAPI efi_get_next_variable_name_runtime(

• 				efi_uintn_t *variable_name_size,
  

• 				u16 *variable_name,
  

• 				const efi_guid_t *vendor);
  

+#endif /* CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING */

• /*
  • See section 3.1.3 in the v2.7 UEFI spec for more details on
  • the layout of EFI_LOAD_OPTION. In short it is:

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index e2ef43157568..3f284795648f 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -59,6 +59,15 @@ config EFI_RUNTIME_CONVERT_POINTER to be called by UEFI drivers in relocating themselves to virtual address space.

+config EFI_RUNTIME_GET_VARIABLE_CACHING

• bool "runtime_service: GetVariable: Enable runtime access via cache (read-only)"
• default y
• help

•  Select this option if you want to access UEFI variables at
  

•  runtime even though you cannot update values on the fly.
  

•  With or without this option, you can access UEFI variables
  

•  at boottime.
  

Updates of volatile variables should always be possible.

• config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index e4abaf3601d9..14e343abbd43 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1892,6 +1892,9 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, efi_uintn_t map_key) { struct efi_event *evt; +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• efi_status_t ret;

+#endif

EFI_ENTRY("%p, %zx", image_handle, map_key);

@@ -1921,7 +1924,12 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, } }

• /* TODO: Should persist EFI variables here */

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

Can we have weak functions for initializing and persisting the cache, e.g.

efi_status_t __weak efi_load_variable_cache(cache_entry *cache, size_t *size) {         cache->len = 0;         return EFI_SUCCESS; }

efi_status_t __runtime __weak efi_write_variable_cache(cache_entry *cache, size_t size) {         return EFI_UNSUPPORTED; }

Then we can override these in whatever driver we implement.

• /* No more variable update */
• ret = efi_freeze_variable_table();
• if (ret != EFI_SUCCESS)

• return EFI_EXIT(ret);
  

+#endif

board_quiesce_devices();

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index fc5bdee80e00..b60f70f04613 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -111,6 +111,11 @@ efi_status_t efi_init_runtime_supported(void) efi_runtime_services_supported |= EFI_RT_SUPPORTED_CONVERT_POINTER; #endif +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• efi_runtime_services_supported |=

• 		(EFI_RT_SUPPORTED_GET_VARIABLE |
  

• 		 EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME);
  

+#endif

return EFI_CALL(efi_set_variable(L"RuntimeServicesSupported", &efi_global_variable_guid, @@ -469,10 +474,18 @@ static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { .patchto = NULL, }, { .ptr = &efi_runtime_services.get_variable, +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• .patchto = &efi_get_variable_runtime,
  

+#else .patchto = &efi_device_error, +#endif }, { .ptr = &efi_runtime_services.get_next_variable_name, +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• .patchto = &efi_get_next_variable_name,
  

+#else .patchto = &efi_device_error, +#endif }, { .ptr = &efi_runtime_services.set_variable, .patchto = &efi_device_error, diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index d9887be938c2..ee21892dd291 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -706,3 +706,470 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

return EFI_EXIT(ret); }

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING +/*

• • runtime version of APIs
• • We only support read-only variable access.
• • The table is in U-Boot's hash table format, but has its own
• • _ENTRY structure for specific use.
• • Except for efi_freeze_variable_table(), which is to be called in
• • exit_boot_services(), all the functions and data below must be
• • placed in either RUNTIME_SERVICES_CODE or RUNTIME_SERVICES_DATA.
• */

+typedef struct _ENTRY {

• unsigned int used; /* hash value; 0 for not used */
• size_t name; /* name offset from itself */
• efi_guid_t vendor;
• u32 attributes;
• size_t data; /* data offset from itself */
• size_t data_size;

+} _ENTRY;

+static inline u16 *entry_name(_ENTRY *e) { return (void *)e + e->name; } +static inline u16 *entry_data(_ENTRY *e) { return (void *)e + e->data; }

+static struct hsearch_data *efi_variable_table __efi_runtime_data;

+static size_t __efi_runtime u16_strlen_runtime(const u16 *s1)

Please, do not duplicate existing functions. If they have to be runtime simply change the existing function to __runtime.

+{

• size_t n = 0;
• while (*s1) {

• n++;
  

• s1++;
  

• }
• return n;

+}

+static int __efi_runtime memcmp_runtime(const void *m1, const void *m2,

• 			size_t n)
  

I dislike duplicate code. Can't we simply define the existing memcmp function as __runtime?

+{

• while (n && *(u8 *)m1 == *(u8 *)m2) {

• n--;
  

• m1++;
  

• m2++;
  

• }
• if (n)

• return *(u8 *)m1 - *(u8 *)m2;
  

• return 0;

+}

+static void __efi_runtime memcpy_runtime(void *m1, const void *m2, size_t n) +{

Can't we simply define the existing memcpy function as __runtime?

• for (; n; n--, m1++, m2++)

• *(u8 *)m1 = *(u8 *)m2;
  

+}

+static int __efi_runtime efi_cmpkey(_ENTRY *e, const u16 *name,

• 		    const efi_guid_t *vendor)
  

+{

• size_t name_len;
• name_len = u16_strlen_runtime(entry_name(e));
• /* return zero if matched */
• return name_len != u16_strlen_runtime(name) ||

•       memcmp_runtime(entry_name(e), name, name_len * 2) ||
  

•       memcmp_runtime(e->vendor.b, vendor->b, sizeof(vendor));
  

+}

+/* simplified and slightly different version of hsearch_r() */

These hash functions are so complicated that they really need a unit test testing them rigorously.

+static int __efi_runtime hsearch_runtime(const u16 *name,

• 			 const efi_guid_t *vendor,
  

• 			 ACTION action,
  

• 			 _ENTRY **retval,
  

• 			 struct hsearch_data *htab)
  

+{

• unsigned int hval;
• unsigned int count;
• unsigned int len;
• unsigned int idx, new;
• /* Compute an value for the given string. */
• len = u16_strlen_runtime(name);

Can't the same variable name exist for different GUIDs? Why is the GUID not considered in the hash?

• hval = len;
• count = len;
• while (count-- > 0) {

• hval <<= 4;
  

• hval += name[count];
  

• }
• /*

• * First hash function:
  

• * simply take the modulo but prevent zero.
  

• */
  

• hval %= htab->size;
• if (hval == 0)

• ++hval;
  

• /* The first index tried. */
• new = -1; /* not found */
• idx = hval;
• if (htab->table[idx].used) {

• /*
  

•  * Further action might be required according to the
  

•  * action value.
  

•  */
  

• unsigned int hval2;
  

• if (htab->table[idx].used == hval &&
  

•     !efi_cmpkey(&htab->table[idx], name, vendor)) {
  

• 	if (action == FIND) {
  

• 		*retval = &htab->table[idx];
  

• 		return idx;
  

• 	}
  

• 	/* we don't need to support overwrite */
  

• 	return -1;
  

• }
  

• /*
  

•  * Second hash function:
  

•  * as suggested in [Knuth]
  

•  */
  

• hval2 = 1 + hval % (htab->size - 2);
  

• do {
  

• 	/*
  

• 	 * Because SIZE is prime this guarantees to
  

• 	 * step through all available indices.
  

• 	 */
  

• 	if (idx <= hval2)
  

• 		idx = htab->size + idx - hval2;
  

• 	else
  

• 		idx -= hval2;
  

• 	/*
  

• 	 * If we visited all entries leave the loop
  

• 	 * unsuccessfully.
  

• 	 */
  

• 	if (idx == hval)
  

• 		break;
  

• 	/* If entry is found use it. */
  

• 	if (htab->table[idx].used == hval &&
  

• 	    !efi_cmpkey(&htab->table[idx], name, vendor)) {
  

• 		if (action == FIND) {
  

• 			*retval = &htab->table[idx];
  

• 			return idx;
  

• 		}
  

• 		/* we don't need to support overwrite */
  

• 		return -1;
  

• 	}
  

• } while (htab->table[idx].used);
  

• if (!htab->table[idx].used)
  

• 	new = idx;
  

• } else {

• new = idx;
  

• }
• /*

• * An empty bucket has been found.
  

• * The following code should never be executed after
  

• * exit_boot_services()
  

• */
  

• if (action == ENTER) {

• /*
  

•  * If table is full and another entry should be
  

•  * entered return with error.
  

•  */
  

• if (htab->filled == htab->size) {
  

• 	*retval = NULL;
  

• 	return 0;
  

• }
  

• /* Create new entry */
  

• htab->table[new].used = hval;
  

• ++htab->filled;
  

• /* return new entry */
  

• *retval = &htab->table[new];
  

• return 1;
  

• }
• *retval = NULL;
• return 0;

+}

+/* from lib/hashtable.c */ +static inline int isprime(unsigned int number) +{

• /* no even number will be passed */
• unsigned int div = 3;
• while (div * div < number && number % div != 0)

• div += 2;
  

• return number % div != 0;

+}

+efi_status_t efi_freeze_variable_table(void)

Please, add comments to your functions. It is not self-evident what this function is meant to do.

I cannot imagine why a variable cache should be frozen. It is a living data structure until the system is switched off.

For a variable cache I expect that you allocate memory before handling the first variable and never again. At runtime you will not have chance to allocate memory anyway.

This function is way too long. Pleae, break it down.

Best regards

Heinrich

+{

• int var_num = 0;
• size_t var_data_size = 0;
• u16 *name;
• efi_uintn_t name_buf_len, name_len;
• efi_guid_t vendor;
• u32 attributes;
• u8 *mem_pool, *var_buf = NULL;
• size_t table_size, var_size, var_buf_size;
• _ENTRY *new = NULL;
• efi_status_t ret;
• /* phase-1 loop */
• name_buf_len = 128;
• name = malloc(name_buf_len);
• if (!name)

• return EFI_OUT_OF_RESOURCES;
  

• name[0] = 0;
• for (;;) {

• name_len = name_buf_len;
  

• ret = EFI_CALL(efi_get_next_variable_name(&name_len, name,
  

• 					  &vendor));
  

• if (ret == EFI_NOT_FOUND) {
  

• 	break;
  

• } else if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	u16 *buf;
  

• 	name_buf_len = name_len;
  

• 	buf = realloc(name, name_buf_len);
  

• 	if (!buf) {
  

• 		free(name);
  

• 		return EFI_OUT_OF_RESOURCES;
  

• 	}
  

• 	name = buf;
  

• 	name_len = name_buf_len;
  

• 	ret = EFI_CALL(efi_get_next_variable_name(&name_len,
  

• 						  name,
  

• 						  &vendor));
  

• }
  

• if (ret != EFI_SUCCESS)
  

• 	return ret;
  

• var_size = 0;
  

• ret = EFI_CALL(efi_get_variable(name, &vendor, &attributes,
  

• 				&var_size, NULL));
  

• if (ret != EFI_BUFFER_TOO_SMALL)
  

• 	return ret;
  

• if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
  

• 	continue;
  

• var_num++;
  

• var_data_size += (u16_strlen_runtime(name) + 1) * sizeof(u16);
  

• var_data_size += var_size;
  

• /* mem_pool must 2-byte aligned for u16 variable name */
  

• if (var_data_size & 0x1)
  

• 	var_data_size++;
  

• }
• /*

• * total of entries in hash table must be a prime number.
  

• * The logic below comes from lib/hashtable.c
  

• */
  

• var_num |= 1; /* make odd */
• while (!isprime(var_num))

• var_num += 2;
  

• /* We need table[var_num] for hsearch_runtime algo */
• table_size = sizeof(*efi_variable_table)

• 	+ sizeof(_ENTRY) * (var_num + 1) + var_data_size;
  

• ret = efi_allocate_pool(EFI_RUNTIME_SERVICES_DATA,

• 		table_size, (void **)&efi_variable_table);
  

• if (ret != EFI_SUCCESS)

• return ret;
  

• efi_variable_table->size = var_num;
• efi_variable_table->table = (void *)efi_variable_table

• 			+ sizeof(*efi_variable_table);
  

• mem_pool = (u8 *)efi_variable_table->table

• 	+ sizeof(_ENTRY) * (var_num + 1);
  

• var_buf_size = 128;
• var_buf = malloc(var_buf_size);
• if (!var_buf) {

• ret = EFI_OUT_OF_RESOURCES;
  

• goto err;
  

• }
• /* phase-2 loop */
• name[0] = 0;
• name_len = name_buf_len;
• for (;;) {

• name_len = name_buf_len;
  

• ret = EFI_CALL(efi_get_next_variable_name(&name_len, name,
  

• 					  &vendor));
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• else if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• var_size = var_buf_size;
  

• ret = EFI_CALL(efi_get_variable(name, &vendor, &attributes,
  

• 				&var_size, var_buf));
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	free(var_buf);
  

• 	var_buf_size = var_size;
  

• 	var_buf = malloc(var_buf_size);
  

• 	if (!var_buf) {
  

• 		ret = EFI_OUT_OF_RESOURCES;
  

• 		goto err;
  

• 	}
  

• 	ret = EFI_CALL(efi_get_variable(name, &vendor,
  

• 					&attributes,
  

• 					&var_size, var_buf));
  

• }
  

• if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
  

• 	continue;
  

• if (hsearch_runtime(name, &vendor, ENTER, &new,
  

• 		    efi_variable_table) <= 0) {
  

• 	/* This should not happen */
  

• 	ret = EFI_INVALID_PARAMETER;
  

• 	goto err;
  

• }
  

• /* allocate space from RUNTIME DATA */
  

• name_len = (u16_strlen_runtime(name) + 1) * sizeof(u16);
  

• memcpy_runtime(mem_pool, name, name_len);
  

• new->name = mem_pool - (u8 *)new; /* offset */
  

• mem_pool += name_len;
  

• memcpy_runtime(&new->vendor.b, &vendor.b, sizeof(vendor));
  

• new->attributes = attributes;
  

• memcpy_runtime(mem_pool, var_buf, var_size);
  

• new->data = mem_pool - (u8 *)new; /* offset */
  

• new->data_size = var_size;
  

• mem_pool += var_size;
  

• /* mem_pool must 2-byte aligned for u16 variable name */
  

• if ((uintptr_t)mem_pool & 0x1)
  

• 	mem_pool++;
  

• }

+#ifdef DEBUG

• name[0] = 0;
• name_len = name_buf_len;
• for (;;) {

• name_len = name_buf_len;
  

• ret = efi_get_next_variable_name_runtime(&name_len, name,
  

• 					 &vendor);
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• else if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• var_size = var_buf_size;
  

• ret = efi_get_variable_runtime(name, &vendor, &attributes,
  

• 			       &var_size, var_buf);
  

• if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• printf("%ls_%pUl:\n", name, &vendor);
  

• printf("    attributes: 0x%x\n", attributes);
  

• printf("    value (size: 0x%lx)\n", var_size);
  

• }

+#endif

• ret = EFI_SUCCESS;

+err:

• free(name);
• free(var_buf);
• if (ret != EFI_SUCCESS && efi_variable_table) {

• efi_free_pool(efi_variable_table);
  

• efi_variable_table = NULL;
  

• }
• return ret;

+}

+efi_status_t +__efi_runtime EFIAPI efi_get_variable_runtime(u16 *variable_name,

• 			      const efi_guid_t *vendor,
  

• 			      u32 *attributes,
  

• 			      efi_uintn_t *data_size,
  

• 			      void *data)
  

+{

• _ENTRY *new;
• if (!variable_name || !vendor || !data_size)

• return EFI_EXIT(EFI_INVALID_PARAMETER);
  

• if (hsearch_runtime(variable_name, vendor, FIND, &new,

• 	    efi_variable_table) <= 0)
  

• return EFI_NOT_FOUND;
  

• if (attributes)

• *attributes = new->attributes;
  

• if (*data_size < new->data_size) {

• *data_size = new->data_size;
  

• return EFI_BUFFER_TOO_SMALL;
  

• }
• *data_size = new->data_size;
• memcpy_runtime(data, entry_data(new), new->data_size);
• return EFI_SUCCESS;

+}

+static int prev_idx __efi_runtime_data;

+efi_status_t +__efi_runtime EFIAPI efi_get_next_variable_name_runtime(

• 				efi_uintn_t *variable_name_size,
  

• 				u16 *variable_name,
  

• 				const efi_guid_t *vendor)
  

+{

• _ENTRY *e;
• u16 *name;
• efi_uintn_t name_size;
• if (!variable_name_size || !variable_name || !vendor)

• return EFI_INVALID_PARAMETER;
  

• if (variable_name[0]) {

• /* sanity check for previous variable */
  

• if (prev_idx < 0)
  

• 	return EFI_INVALID_PARAMETER;
  

• e = &efi_variable_table->table[prev_idx];
  

• if (!e->used || efi_cmpkey(e, variable_name, vendor))
  

• 	return EFI_INVALID_PARAMETER;
  

• } else {

• prev_idx = -1;
  

• }
• /* next variable */
• while (++prev_idx <= efi_variable_table->size) {

• e = &efi_variable_table->table[prev_idx];
  

• if (e->used)
  

• 	break;
  

• }
• if (prev_idx > efi_variable_table->size)

• return EFI_NOT_FOUND;
  

• name = entry_name(e);
• name_size = (u16_strlen_runtime(name) + 1)

• 	* sizeof(u16);
  

• if (*variable_name_size < name_size) {

• *variable_name_size = name_size;
  

• return EFI_BUFFER_TOO_SMALL;
  

• }
• memcpy_runtime(variable_name, name, name_size);
• memcpy_runtime((void *)&vendor->b, &e->vendor.b, sizeof(vendor));
• return EFI_SUCCESS;

+} +#endif /* CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING */

On 6/17/19 3:51 AM, AKASHI Takahiro wrote:

On Sat, Jun 15, 2019 at 09:01:56PM +0200, Heinrich Schuchardt wrote:

...

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

...

With this patch, cache buffer for UEFI variables will be created so that we will still be able to access, at least retrieve, UEFI variables when we exit from boottime services,

This feature is a "should" behavior described in EBBR v1.0 section 2.5.3.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

include/efi_loader.h | 17 ++ lib/efi_loader/Kconfig | 9 + lib/efi_loader/efi_boottime.c | 10 +- lib/efi_loader/efi_runtime.c | 13 + lib/efi_loader/efi_variable.c | 467 ++++++++++++++++++++++++++++++++++ 5 files changed, 515 insertions(+), 1 deletion(-)

Please, put the cache into a separate file.

Why?

It is a separate set of functions. In C++ programming you wouldn't put two classes into the same file.

...

...

diff --git a/include/efi_loader.h b/include/efi_loader.h index 93f7ece814a0..acab657b9d70 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -620,6 +620,23 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data);

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING +efi_status_t efi_freeze_variable_table(void);

+/* runtime version of APIs */ +efi_status_t +__efi_runtime EFIAPI efi_get_variable_runtime(u16 *variable_name,

I think one version of the functions serving at runtime and boottime is enough.

The cache should be used both at runtime and at boottime.

So do you mean that we should replace the existing "boottime" version of get/set_variable with my code (algorithm)?

This is a bit complicated work because we should be able to *udpate* UEFI variables at boottime, but my version of hsearch_runtime() is a stripped (and modified) version and doesn't support it.

Do we really need a multilevel hash table? I would not expect hundreds of variables.

Making the existing hsearch_r() executable at UEFI runtime is, as I said before, quite painful.

You could start the cache implementation with a less complicated data structure like a linked list.

...

Essentially I expect three modules working together:

UEFI API implementation <-> Cache <-> Persistence driver

I would suggest to put each of these into a separate file.

Both the API implementation and the Cache have to be available at Boottime and at Runtime. A first version of the persistence driver may only be working at boottime.

Unfortunately, this is not practical right now because there is already some sort of assumption (and consensus) that we would re-use "Standalone MM services", which is already there in EDK2, as secure storage for UEFI variables. In the case, all the cache would be bypassed. In my old prototype, I utilized the cache but dropped that feature for several reasons.

What has EDK2 code to do with it?

In case of write you could do a write-through in your cache if needed.

...

The NV-cache content should be written to non-volatile memory on Reset() and on ExitBootServices() and if possible when updating variables at runtime.

I'm not sure your intent here, but are you going to write back the cache only once? It won't work as every change of UEFI variable must be flushed to persistent storage instantly.

The cache should support write and read. Only NV variables have to be written to a medium. If you do not support this currently just return some error code vor NV variables. But you could accept still accept changes to non-NV variables. This way we can test the code at runtime even before implementing runtime persistence.

...

...

• 			      const efi_guid_t *vendor,
  

• 			      u32 *attributes,
  

• 			      efi_uintn_t *data_size,
  

• 			      void *data);
  

+efi_status_t +__efi_runtime EFIAPI efi_get_next_variable_name_runtime(

• 				efi_uintn_t *variable_name_size,
  

• 				u16 *variable_name,
  

• 				const efi_guid_t *vendor);
  

+#endif /* CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING */

• /*
  • See section 3.1.3 in the v2.7 UEFI spec for more details on
  • the layout of EFI_LOAD_OPTION. In short it is:

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index e2ef43157568..3f284795648f 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -59,6 +59,15 @@ config EFI_RUNTIME_CONVERT_POINTER to be called by UEFI drivers in relocating themselves to virtual address space.

+config EFI_RUNTIME_GET_VARIABLE_CACHING

• bool "runtime_service: GetVariable: Enable runtime access via cache (read-only)"
• default y
• help

•  Select this option if you want to access UEFI variables at
  

•  runtime even though you cannot update values on the fly.
  

•  With or without this option, you can access UEFI variables
  

•  at boottime.
  

Updates of volatile variables should always be possible.

Why "should"? Give me any use case. UEFI spec does not describe such a variant implementation at all.

See above. I would like to be able to test setting variables at runtime even if persisting NV variables is not yet implemented.

...

...

• config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index e4abaf3601d9..14e343abbd43 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1892,6 +1892,9 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, efi_uintn_t map_key) { struct efi_event *evt; +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• efi_status_t ret;

+#endif

EFI_ENTRY("%p, %zx", image_handle, map_key);

@@ -1921,7 +1924,12 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, } }

• /* TODO: Should persist EFI variables here */

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

Can we have weak functions for initializing and persisting the cache, e.g.

efi_status_t __weak efi_load_variable_cache(cache_entry *cache, size_t *size) {         cache->len = 0;         return EFI_SUCCESS; }

efi_status_t __runtime __weak efi_write_variable_cache(cache_entry *cache, size_t size) {         return EFI_UNSUPPORTED; }

Then we can override these in whatever driver we implement.

What is the difference between yours and my env_efi_load/save() with backing-storage driver?

I cannot see that you clearly separate the functions API, cache, persistence.

I would like to see clearly define interfaces into which we can plug different persistence implementations.

...

...

• /* No more variable update */
• ret = efi_freeze_variable_table();
• if (ret != EFI_SUCCESS)

• return EFI_EXIT(ret);
  

+#endif

board_quiesce_devices();

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index fc5bdee80e00..b60f70f04613 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -111,6 +111,11 @@ efi_status_t efi_init_runtime_supported(void) efi_runtime_services_supported |= EFI_RT_SUPPORTED_CONVERT_POINTER; #endif +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• efi_runtime_services_supported |=

• 		(EFI_RT_SUPPORTED_GET_VARIABLE |
  

• 		 EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME);
  

+#endif

return EFI_CALL(efi_set_variable(L"RuntimeServicesSupported", &efi_global_variable_guid, @@ -469,10 +474,18 @@ static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { .patchto = NULL, }, { .ptr = &efi_runtime_services.get_variable, +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• .patchto = &efi_get_variable_runtime,
  

+#else .patchto = &efi_device_error, +#endif }, { .ptr = &efi_runtime_services.get_next_variable_name, +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• .patchto = &efi_get_next_variable_name,
  

+#else .patchto = &efi_device_error, +#endif }, { .ptr = &efi_runtime_services.set_variable, .patchto = &efi_device_error, diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index d9887be938c2..ee21892dd291 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -706,3 +706,470 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

return EFI_EXIT(ret); }

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING +/*

• • runtime version of APIs
• • We only support read-only variable access.
• • The table is in U-Boot's hash table format, but has its own
• • _ENTRY structure for specific use.
• • Except for efi_freeze_variable_table(), which is to be called in
• • exit_boot_services(), all the functions and data below must be
• • placed in either RUNTIME_SERVICES_CODE or RUNTIME_SERVICES_DATA.
• */

+typedef struct _ENTRY {

• unsigned int used; /* hash value; 0 for not used */
• size_t name; /* name offset from itself */
• efi_guid_t vendor;
• u32 attributes;
• size_t data; /* data offset from itself */
• size_t data_size;

+} _ENTRY;

+static inline u16 *entry_name(_ENTRY *e) { return (void *)e + e->name; } +static inline u16 *entry_data(_ENTRY *e) { return (void *)e + e->data; }

+static struct hsearch_data *efi_variable_table __efi_runtime_data;

+static size_t __efi_runtime u16_strlen_runtime(const u16 *s1)

Please, do not duplicate existing functions. If they have to be runtime simply change the existing function to __runtime.

We should do that if possible, but please note that [str|mem]xxx() functions are *architecture* dependent. Do you want to mark all the functions across all the architectures?

...

...

+{

• size_t n = 0;
• while (*s1) {

• n++;
  

• s1++;
  

• }
• return n;

+}

+static int __efi_runtime memcmp_runtime(const void *m1, const void *m2,

• 			size_t n)
  

I dislike duplicate code. Can't we simply define the existing memcmp function as __runtime?

ditto

...

...

+{

• while (n && *(u8 *)m1 == *(u8 *)m2) {

• n--;
  

• m1++;
  

• m2++;
  

• }
• if (n)

• return *(u8 *)m1 - *(u8 *)m2;
  

• return 0;

+}

+static void __efi_runtime memcpy_runtime(void *m1, const void *m2, size_t n) +{

Can't we simply define the existing memcpy function as __runtime?

...

• for (; n; n--, m1++, m2++)

• *(u8 *)m1 = *(u8 *)m2;
  

+}

+static int __efi_runtime efi_cmpkey(_ENTRY *e, const u16 *name,

• 		    const efi_guid_t *vendor)
  

+{

• size_t name_len;
• name_len = u16_strlen_runtime(entry_name(e));
• /* return zero if matched */
• return name_len != u16_strlen_runtime(name) ||

•       memcmp_runtime(entry_name(e), name, name_len * 2) ||
  

•       memcmp_runtime(e->vendor.b, vendor->b, sizeof(vendor));
  

+}

+/* simplified and slightly different version of hsearch_r() */

These hash functions are so complicated that they really need a unit test testing them rigorously.

Do you know that there are no any tests for hsearch_r()? Moreover, this function would better be exercised well if we could add *runtime* tests.

Have a look at test/env/hashtable.c

I think efi_selftest could work after SetVirtualMemoryMap if we do not change the map for memory actually used by U-Boot.

Or we use a minimum Linux kernel and put our tests into an init binary.

...

...

+static int __efi_runtime hsearch_runtime(const u16 *name,

• 			 const efi_guid_t *vendor,
  

• 			 ACTION action,
  

• 			 _ENTRY **retval,
  

• 			 struct hsearch_data *htab)
  

+{

• unsigned int hval;
• unsigned int count;
• unsigned int len;
• unsigned int idx, new;
• /* Compute an value for the given string. */
• len = u16_strlen_runtime(name);

Can't the same variable name exist for different GUIDs? Why is the GUID not considered in the hash?

efi_cmpkey() does take into consideration GUID as well as the name.

My question concerned the hash.

...

...

• hval = len;
• count = len;
• while (count-- > 0) {

• hval <<= 4;
  

• hval += name[count];
  

• }
• /*

• * First hash function:
  

• * simply take the modulo but prevent zero.
  

• */
  

• hval %= htab->size;
• if (hval == 0)

• ++hval;
  

• /* The first index tried. */
• new = -1; /* not found */
• idx = hval;
• if (htab->table[idx].used) {

• /*
  

•  * Further action might be required according to the
  

•  * action value.
  

•  */
  

• unsigned int hval2;
  

• if (htab->table[idx].used == hval &&
  

•     !efi_cmpkey(&htab->table[idx], name, vendor)) {
  

• 	if (action == FIND) {
  

• 		*retval = &htab->table[idx];
  

• 		return idx;
  

• 	}
  

• 	/* we don't need to support overwrite */
  

• 	return -1;
  

• }
  

• /*
  

•  * Second hash function:
  

•  * as suggested in [Knuth]
  

•  */
  

• hval2 = 1 + hval % (htab->size - 2);
  

• do {
  

• 	/*
  

• 	 * Because SIZE is prime this guarantees to
  

• 	 * step through all available indices.
  

• 	 */
  

• 	if (idx <= hval2)
  

• 		idx = htab->size + idx - hval2;
  

• 	else
  

• 		idx -= hval2;
  

• 	/*
  

• 	 * If we visited all entries leave the loop
  

• 	 * unsuccessfully.
  

• 	 */
  

• 	if (idx == hval)
  

• 		break;
  

• 	/* If entry is found use it. */
  

• 	if (htab->table[idx].used == hval &&
  

• 	    !efi_cmpkey(&htab->table[idx], name, vendor)) {
  

• 		if (action == FIND) {
  

• 			*retval = &htab->table[idx];
  

• 			return idx;
  

• 		}
  

• 		/* we don't need to support overwrite */
  

• 		return -1;
  

• 	}
  

• } while (htab->table[idx].used);
  

• if (!htab->table[idx].used)
  

• 	new = idx;
  

• } else {

• new = idx;
  

• }
• /*

• * An empty bucket has been found.
  

• * The following code should never be executed after
  

• * exit_boot_services()
  

• */
  

• if (action == ENTER) {

• /*
  

•  * If table is full and another entry should be
  

•  * entered return with error.
  

•  */
  

• if (htab->filled == htab->size) {
  

• 	*retval = NULL;
  

• 	return 0;
  

• }
  

• /* Create new entry */
  

• htab->table[new].used = hval;
  

• ++htab->filled;
  

• /* return new entry */
  

• *retval = &htab->table[new];
  

• return 1;
  

• }
• *retval = NULL;
• return 0;

+}

+/* from lib/hashtable.c */ +static inline int isprime(unsigned int number) +{

• /* no even number will be passed */
• unsigned int div = 3;
• while (div * div < number && number % div != 0)

• div += 2;
  

• return number % div != 0;

+}

+efi_status_t efi_freeze_variable_table(void)

Please, add comments to your functions. It is not self-evident what this function is meant to do.

I cannot imagine why a variable cache should be frozen. It is a living data structure until the system is switched off.

I don't get your point.

Please, explain what you mean by freeze.

I suggest that the cache is read/write at all times.

Best regards

Heinrich

...

For a variable cache I expect that you allocate memory before handling the first variable and never again. At runtime you will not have chance to allocate memory anyway.

I don't get your point.

...

This function is way too long. Pleae, break it down.

Freezing is implemented in 2-phase steps, and some complexity is inevitable. I suppose adding some comments would be enough.

-Takahiro Akashi

...

Best regards

Heinrich

...

+{

• int var_num = 0;
• size_t var_data_size = 0;
• u16 *name;
• efi_uintn_t name_buf_len, name_len;
• efi_guid_t vendor;
• u32 attributes;
• u8 *mem_pool, *var_buf = NULL;
• size_t table_size, var_size, var_buf_size;
• _ENTRY *new = NULL;
• efi_status_t ret;
• /* phase-1 loop */
• name_buf_len = 128;
• name = malloc(name_buf_len);
• if (!name)

• return EFI_OUT_OF_RESOURCES;
  

• name[0] = 0;
• for (;;) {

• name_len = name_buf_len;
  

• ret = EFI_CALL(efi_get_next_variable_name(&name_len, name,
  

• 					  &vendor));
  

• if (ret == EFI_NOT_FOUND) {
  

• 	break;
  

• } else if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	u16 *buf;
  

• 	name_buf_len = name_len;
  

• 	buf = realloc(name, name_buf_len);
  

• 	if (!buf) {
  

• 		free(name);
  

• 		return EFI_OUT_OF_RESOURCES;
  

• 	}
  

• 	name = buf;
  

• 	name_len = name_buf_len;
  

• 	ret = EFI_CALL(efi_get_next_variable_name(&name_len,
  

• 						  name,
  

• 						  &vendor));
  

• }
  

• if (ret != EFI_SUCCESS)
  

• 	return ret;
  

• var_size = 0;
  

• ret = EFI_CALL(efi_get_variable(name, &vendor, &attributes,
  

• 				&var_size, NULL));
  

• if (ret != EFI_BUFFER_TOO_SMALL)
  

• 	return ret;
  

• if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
  

• 	continue;
  

• var_num++;
  

• var_data_size += (u16_strlen_runtime(name) + 1) * sizeof(u16);
  

• var_data_size += var_size;
  

• /* mem_pool must 2-byte aligned for u16 variable name */
  

• if (var_data_size & 0x1)
  

• 	var_data_size++;
  

• }
• /*

• * total of entries in hash table must be a prime number.
  

• * The logic below comes from lib/hashtable.c
  

• */
  

• var_num |= 1; /* make odd */
• while (!isprime(var_num))

• var_num += 2;
  

• /* We need table[var_num] for hsearch_runtime algo */
• table_size = sizeof(*efi_variable_table)

• 	+ sizeof(_ENTRY) * (var_num + 1) + var_data_size;
  

• ret = efi_allocate_pool(EFI_RUNTIME_SERVICES_DATA,

• 		table_size, (void **)&efi_variable_table);
  

• if (ret != EFI_SUCCESS)

• return ret;
  

• efi_variable_table->size = var_num;
• efi_variable_table->table = (void *)efi_variable_table

• 			+ sizeof(*efi_variable_table);
  

• mem_pool = (u8 *)efi_variable_table->table

• 	+ sizeof(_ENTRY) * (var_num + 1);
  

• var_buf_size = 128;
• var_buf = malloc(var_buf_size);
• if (!var_buf) {

• ret = EFI_OUT_OF_RESOURCES;
  

• goto err;
  

• }
• /* phase-2 loop */
• name[0] = 0;
• name_len = name_buf_len;
• for (;;) {

• name_len = name_buf_len;
  

• ret = EFI_CALL(efi_get_next_variable_name(&name_len, name,
  

• 					  &vendor));
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• else if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• var_size = var_buf_size;
  

• ret = EFI_CALL(efi_get_variable(name, &vendor, &attributes,
  

• 				&var_size, var_buf));
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	free(var_buf);
  

• 	var_buf_size = var_size;
  

• 	var_buf = malloc(var_buf_size);
  

• 	if (!var_buf) {
  

• 		ret = EFI_OUT_OF_RESOURCES;
  

• 		goto err;
  

• 	}
  

• 	ret = EFI_CALL(efi_get_variable(name, &vendor,
  

• 					&attributes,
  

• 					&var_size, var_buf));
  

• }
  

• if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
  

• 	continue;
  

• if (hsearch_runtime(name, &vendor, ENTER, &new,
  

• 		    efi_variable_table) <= 0) {
  

• 	/* This should not happen */
  

• 	ret = EFI_INVALID_PARAMETER;
  

• 	goto err;
  

• }
  

• /* allocate space from RUNTIME DATA */
  

• name_len = (u16_strlen_runtime(name) + 1) * sizeof(u16);
  

• memcpy_runtime(mem_pool, name, name_len);
  

• new->name = mem_pool - (u8 *)new; /* offset */
  

• mem_pool += name_len;
  

• memcpy_runtime(&new->vendor.b, &vendor.b, sizeof(vendor));
  

• new->attributes = attributes;
  

• memcpy_runtime(mem_pool, var_buf, var_size);
  

• new->data = mem_pool - (u8 *)new; /* offset */
  

• new->data_size = var_size;
  

• mem_pool += var_size;
  

• /* mem_pool must 2-byte aligned for u16 variable name */
  

• if ((uintptr_t)mem_pool & 0x1)
  

• 	mem_pool++;
  

• }

+#ifdef DEBUG

• name[0] = 0;
• name_len = name_buf_len;
• for (;;) {

• name_len = name_buf_len;
  

• ret = efi_get_next_variable_name_runtime(&name_len, name,
  

• 					 &vendor);
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• else if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• var_size = var_buf_size;
  

• ret = efi_get_variable_runtime(name, &vendor, &attributes,
  

• 			       &var_size, var_buf);
  

• if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• printf("%ls_%pUl:\n", name, &vendor);
  

• printf("    attributes: 0x%x\n", attributes);
  

• printf("    value (size: 0x%lx)\n", var_size);
  

• }

+#endif

• ret = EFI_SUCCESS;

+err:

• free(name);
• free(var_buf);
• if (ret != EFI_SUCCESS && efi_variable_table) {

• efi_free_pool(efi_variable_table);
  

• efi_variable_table = NULL;
  

• }
• return ret;

+}

+efi_status_t +__efi_runtime EFIAPI efi_get_variable_runtime(u16 *variable_name,

• 			      const efi_guid_t *vendor,
  

• 			      u32 *attributes,
  

• 			      efi_uintn_t *data_size,
  

• 			      void *data)
  

+{

• _ENTRY *new;
• if (!variable_name || !vendor || !data_size)

• return EFI_EXIT(EFI_INVALID_PARAMETER);
  

• if (hsearch_runtime(variable_name, vendor, FIND, &new,

• 	    efi_variable_table) <= 0)
  

• return EFI_NOT_FOUND;
  

• if (attributes)

• *attributes = new->attributes;
  

• if (*data_size < new->data_size) {

• *data_size = new->data_size;
  

• return EFI_BUFFER_TOO_SMALL;
  

• }
• *data_size = new->data_size;
• memcpy_runtime(data, entry_data(new), new->data_size);
• return EFI_SUCCESS;

+}

+static int prev_idx __efi_runtime_data;

+efi_status_t +__efi_runtime EFIAPI efi_get_next_variable_name_runtime(

• 				efi_uintn_t *variable_name_size,
  

• 				u16 *variable_name,
  

• 				const efi_guid_t *vendor)
  

+{

• _ENTRY *e;
• u16 *name;
• efi_uintn_t name_size;
• if (!variable_name_size || !variable_name || !vendor)

• return EFI_INVALID_PARAMETER;
  

• if (variable_name[0]) {

• /* sanity check for previous variable */
  

• if (prev_idx < 0)
  

• 	return EFI_INVALID_PARAMETER;
  

• e = &efi_variable_table->table[prev_idx];
  

• if (!e->used || efi_cmpkey(e, variable_name, vendor))
  

• 	return EFI_INVALID_PARAMETER;
  

• } else {

• prev_idx = -1;
  

• }
• /* next variable */
• while (++prev_idx <= efi_variable_table->size) {

• e = &efi_variable_table->table[prev_idx];
  

• if (e->used)
  

• 	break;
  

• }
• if (prev_idx > efi_variable_table->size)

• return EFI_NOT_FOUND;
  

• name = entry_name(e);
• name_size = (u16_strlen_runtime(name) + 1)

• 	* sizeof(u16);
  

• if (*variable_name_size < name_size) {

• *variable_name_size = name_size;
  

• return EFI_BUFFER_TOO_SMALL;
  

• }
• memcpy_runtime(variable_name, name, name_size);
• memcpy_runtime((void *)&vendor->b, &e->vendor.b, sizeof(vendor));
• return EFI_SUCCESS;

+} +#endif /* CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING */

Heinrich,

On Tue, Jun 18, 2019 at 10:19:06AM +0900, AKASHI Takahiro wrote:

On Mon, Jun 17, 2019 at 09:52:34PM +0200, Heinrich Schuchardt wrote:

...

On 6/17/19 3:51 AM, AKASHI Takahiro wrote:

...

On Sat, Jun 15, 2019 at 09:01:56PM +0200, Heinrich Schuchardt wrote:

...

On 6/5/19 6:21 AM, AKASHI Takahiro wrote:

...

With this patch, cache buffer for UEFI variables will be created so that we will still be able to access, at least retrieve, UEFI variables when we exit from boottime services,

This feature is a "should" behavior described in EBBR v1.0 section 2.5.3.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

include/efi_loader.h | 17 ++ lib/efi_loader/Kconfig | 9 + lib/efi_loader/efi_boottime.c | 10 +- lib/efi_loader/efi_runtime.c | 13 + lib/efi_loader/efi_variable.c | 467 ++++++++++++++++++++++++++++++++++ 5 files changed, 515 insertions(+), 1 deletion(-)

Please, put the cache into a separate file.

Why?

It is a separate set of functions. In C++ programming you wouldn't put two classes into the same file.

? I don't get your point. This is not C++, but C.

...

...

...

...

diff --git a/include/efi_loader.h b/include/efi_loader.h index 93f7ece814a0..acab657b9d70 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -620,6 +620,23 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data);

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING +efi_status_t efi_freeze_variable_table(void);

+/* runtime version of APIs */ +efi_status_t +__efi_runtime EFIAPI efi_get_variable_runtime(u16 *variable_name,

I think one version of the functions serving at runtime and boottime is enough.

The cache should be used both at runtime and at boottime.

So do you mean that we should replace the existing "boottime" version of get/set_variable with my code (algorithm)?

This is a bit complicated work because we should be able to *udpate* UEFI variables at boottime, but my version of hsearch_runtime() is a stripped (and modified) version and doesn't support it.

Do we really need a multilevel hash table? I would not expect hundreds of variables.

Please don't change your point suddenly. Here we are discussing whether "The cache should be used both at runtime and at boottime" or not.

...

...

Making the existing hsearch_r() executable at UEFI runtime is, as I said before, quite painful.

You could start the cache implementation with a less complicated data structure like a linked list.

This is totally a different issue. I listed this issue in my cover letter.

...

...

...

Essentially I expect three modules working together:

UEFI API implementation <-> Cache <-> Persistence driver

I would suggest to put each of these into a separate file.

Both the API implementation and the Cache have to be available at Boottime and at Runtime. A first version of the persistence driver may only be working at boottime.

Unfortunately, this is not practical right now because there is already some sort of assumption (and consensus) that we would re-use "Standalone MM services", which is already there in EDK2, as secure storage for UEFI variables. In the case, all the cache would be bypassed. In my old prototype, I utilized the cache but dropped that feature for several reasons.

What has EDK2 code to do with it?

Did you follow my comment below?

...

...

Unfortunately, this is not practical right now because there is already some sort of assumption (and consensus) that we would re-use "Standalone MM services", which is already there in EDK2, as secure storage for UEFI variables.

Let me elaborate more: My tentative "secure boot" patch is self-contained on non-secure side and provides the functionality that UEFI specification describes, but there is one missing and essential aspect: Tamper-resistant. Standalone MM services is expected to provides this semantics with the same interfaces as [Get|Set]Variable for both volatile and non-volatile variables alike. There can be another implementation (with yet another interfaces to secure side), but it is currently the only implementation available as far as I know.

-Takahiro Akashi

...

In case of write you could do a write-through in your cache if needed.

...

...

The NV-cache content should be written to non-volatile memory on Reset() and on ExitBootServices() and if possible when updating variables at runtime.

I'm not sure your intent here, but are you going to write back the cache only once? It won't work as every change of UEFI variable must be flushed to persistent storage instantly.

The cache should support write and read. Only NV variables have to be

Why? I don't think it make sense that we support volatile variable, but not non-volatile variable at runtime. Even UEFI specification doesn't describe such an irregular behavior. See EFI_RT_SUPPORTED_XXX definitions. Do you have a meaningful use case that you want to support?

...

written to a medium. If you do not support this currently just return some error code vor NV variables. But you could accept still accept changes to non-NV variables. This way we can test the code at runtime even before implementing runtime persistence.

...

...

...

• 			      const efi_guid_t *vendor,
  

• 			      u32 *attributes,
  

• 			      efi_uintn_t *data_size,
  

• 			      void *data);
  

+efi_status_t +__efi_runtime EFIAPI efi_get_next_variable_name_runtime(

• 				efi_uintn_t *variable_name_size,
  

• 				u16 *variable_name,
  

• 				const efi_guid_t *vendor);
  

+#endif /* CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING */

/*

• See section 3.1.3 in the v2.7 UEFI spec for more details on
• the layout of EFI_LOAD_OPTION. In short it is:

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index e2ef43157568..3f284795648f 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -59,6 +59,15 @@ config EFI_RUNTIME_CONVERT_POINTER to be called by UEFI drivers in relocating themselves to virtual address space.

+config EFI_RUNTIME_GET_VARIABLE_CACHING

• bool "runtime_service: GetVariable: Enable runtime access via cache (read-only)"
• default y
• help

•  Select this option if you want to access UEFI variables at
  

•  runtime even though you cannot update values on the fly.
  

•  With or without this option, you can access UEFI variables
  

•  at boottime.
  

Updates of volatile variables should always be possible.

Why "should"? Give me any use case. UEFI spec does not describe such a variant implementation at all.

See above. I would like to be able to test setting variables at runtime even if persisting NV variables is not yet implemented.

Just for testing? No real use case? Please note that if we *have* persistent storage at runtime, we don't need cache because we can access that storage.

...

...

...

...

config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index e4abaf3601d9..14e343abbd43 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1892,6 +1892,9 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, efi_uintn_t map_key) { struct efi_event *evt; +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• efi_status_t ret;

+#endif

EFI_ENTRY("%p, %zx", image_handle, map_key);

@@ -1921,7 +1924,12 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, } }

• /* TODO: Should persist EFI variables here */

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

Can we have weak functions for initializing and persisting the cache, e.g.

efi_status_t __weak efi_load_variable_cache(cache_entry *cache, size_t *size) {         cache->len = 0;         return EFI_SUCCESS; }

efi_status_t __runtime __weak efi_write_variable_cache(cache_entry *cache, size_t size) {         return EFI_UNSUPPORTED; }

Then we can override these in whatever driver we implement.

What is the difference between yours and my env_efi_load/save() with backing-storage driver?

I cannot see that you clearly separate the functions API, cache, persistence.

I don't get your point. For instance, efi_boottime.c has a lot of different type of functions. Do you want to separate them into different files?

...

I would like to see clearly define interfaces into which we can plug different persistence implementations.

Your assertion doesn't clarify why you want to separate API and cache.

Regarding persistent storage, as I said, efi_[get|set]_variable() will be completely replaced with the corresponding functions (at least, in case of Standalone MM services).

...

...

...

...

• /* No more variable update */
• ret = efi_freeze_variable_table();
• if (ret != EFI_SUCCESS)

• return EFI_EXIT(ret);
  

+#endif

board_quiesce_devices();

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index fc5bdee80e00..b60f70f04613 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -111,6 +111,11 @@ efi_status_t efi_init_runtime_supported(void) efi_runtime_services_supported |= EFI_RT_SUPPORTED_CONVERT_POINTER; #endif +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• efi_runtime_services_supported |=

• 		(EFI_RT_SUPPORTED_GET_VARIABLE |
  

• 		 EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME);
  

+#endif

return EFI_CALL(efi_set_variable(L"RuntimeServicesSupported", &efi_global_variable_guid, @@ -469,10 +474,18 @@ static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { .patchto = NULL, }, { .ptr = &efi_runtime_services.get_variable, +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• .patchto = &efi_get_variable_runtime,
  

+#else .patchto = &efi_device_error, +#endif }, { .ptr = &efi_runtime_services.get_next_variable_name, +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

• .patchto = &efi_get_next_variable_name,
  

+#else .patchto = &efi_device_error, +#endif }, { .ptr = &efi_runtime_services.set_variable, .patchto = &efi_device_error, diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index d9887be938c2..ee21892dd291 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -706,3 +706,470 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

return EFI_EXIT(ret); }

+#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING +/*

• • runtime version of APIs
• • We only support read-only variable access.
• • The table is in U-Boot's hash table format, but has its own
• • _ENTRY structure for specific use.
• • Except for efi_freeze_variable_table(), which is to be called in
• • exit_boot_services(), all the functions and data below must be
• • placed in either RUNTIME_SERVICES_CODE or RUNTIME_SERVICES_DATA.
• */

+typedef struct _ENTRY {

• unsigned int used; /* hash value; 0 for not used */
• size_t name; /* name offset from itself */
• efi_guid_t vendor;
• u32 attributes;
• size_t data; /* data offset from itself */
• size_t data_size;

+} _ENTRY;

+static inline u16 *entry_name(_ENTRY *e) { return (void *)e + e->name; } +static inline u16 *entry_data(_ENTRY *e) { return (void *)e + e->data; }

+static struct hsearch_data *efi_variable_table __efi_runtime_data;

+static size_t __efi_runtime u16_strlen_runtime(const u16 *s1)

Please, do not duplicate existing functions. If they have to be runtime simply change the existing function to __runtime.

We should do that if possible, but please note that [str|mem]xxx() functions are *architecture* dependent. Do you want to mark all the functions across all the architectures?

Agree or not agree?

...

...

...

...

+{

• size_t n = 0;
• while (*s1) {

• n++;
  

• s1++;
  

• }
• return n;

+}

+static int __efi_runtime memcmp_runtime(const void *m1, const void *m2,

• 			size_t n)
  

I dislike duplicate code. Can't we simply define the existing memcmp function as __runtime?

ditto

...

...

+{

• while (n && *(u8 *)m1 == *(u8 *)m2) {

• n--;
  

• m1++;
  

• m2++;
  

• }
• if (n)

• return *(u8 *)m1 - *(u8 *)m2;
  

• return 0;

+}

+static void __efi_runtime memcpy_runtime(void *m1, const void *m2, size_t n) +{

Can't we simply define the existing memcpy function as __runtime?

...

• for (; n; n--, m1++, m2++)

• *(u8 *)m1 = *(u8 *)m2;
  

+}

+static int __efi_runtime efi_cmpkey(_ENTRY *e, const u16 *name,

• 		    const efi_guid_t *vendor)
  

+{

• size_t name_len;
• name_len = u16_strlen_runtime(entry_name(e));
• /* return zero if matched */
• return name_len != u16_strlen_runtime(name) ||

•       memcmp_runtime(entry_name(e), name, name_len * 2) ||
  

•       memcmp_runtime(e->vendor.b, vendor->b, sizeof(vendor));
  

+}

+/* simplified and slightly different version of hsearch_r() */

These hash functions are so complicated that they really need a unit test testing them rigorously.

Do you know that there are no any tests for hsearch_r()? Moreover, this function would better be exercised well if we could add *runtime* tests.

Have a look at test/env/hashtable.c

Okay, but why not re-use this test for my stripped version?

...

I think efi_selftest could work after SetVirtualMemoryMap if we do not change the map for memory actually used by U-Boot.

Maybe.

-Takahiro Akashi

...

Or we use a minimum Linux kernel and put our tests into an init binary.

...

...

...

+static int __efi_runtime hsearch_runtime(const u16 *name,

• 			 const efi_guid_t *vendor,
  

• 			 ACTION action,
  

• 			 _ENTRY **retval,
  

• 			 struct hsearch_data *htab)
  

+{

• unsigned int hval;
• unsigned int count;
• unsigned int len;
• unsigned int idx, new;
• /* Compute an value for the given string. */
• len = u16_strlen_runtime(name);

Can't the same variable name exist for different GUIDs? Why is the GUID not considered in the hash?

efi_cmpkey() does take into consideration GUID as well as the name.

My question concerned the hash.

...

...

...

• hval = len;
• count = len;
• while (count-- > 0) {

• hval <<= 4;
  

• hval += name[count];
  

• }
• /*

• * First hash function:
  

• * simply take the modulo but prevent zero.
  

• */
  

• hval %= htab->size;
• if (hval == 0)

• ++hval;
  

• /* The first index tried. */
• new = -1; /* not found */
• idx = hval;
• if (htab->table[idx].used) {

• /*
  

•  * Further action might be required according to the
  

•  * action value.
  

•  */
  

• unsigned int hval2;
  

• if (htab->table[idx].used == hval &&
  

•     !efi_cmpkey(&htab->table[idx], name, vendor)) {
  

• 	if (action == FIND) {
  

• 		*retval = &htab->table[idx];
  

• 		return idx;
  

• 	}
  

• 	/* we don't need to support overwrite */
  

• 	return -1;
  

• }
  

• /*
  

•  * Second hash function:
  

•  * as suggested in [Knuth]
  

•  */
  

• hval2 = 1 + hval % (htab->size - 2);
  

• do {
  

• 	/*
  

• 	 * Because SIZE is prime this guarantees to
  

• 	 * step through all available indices.
  

• 	 */
  

• 	if (idx <= hval2)
  

• 		idx = htab->size + idx - hval2;
  

• 	else
  

• 		idx -= hval2;
  

• 	/*
  

• 	 * If we visited all entries leave the loop
  

• 	 * unsuccessfully.
  

• 	 */
  

• 	if (idx == hval)
  

• 		break;
  

• 	/* If entry is found use it. */
  

• 	if (htab->table[idx].used == hval &&
  

• 	    !efi_cmpkey(&htab->table[idx], name, vendor)) {
  

• 		if (action == FIND) {
  

• 			*retval = &htab->table[idx];
  

• 			return idx;
  

• 		}
  

• 		/* we don't need to support overwrite */
  

• 		return -1;
  

• 	}
  

• } while (htab->table[idx].used);
  

• if (!htab->table[idx].used)
  

• 	new = idx;
  

• } else {

• new = idx;
  

• }
• /*

• * An empty bucket has been found.
  

• * The following code should never be executed after
  

• * exit_boot_services()
  

• */
  

• if (action == ENTER) {

• /*
  

•  * If table is full and another entry should be
  

•  * entered return with error.
  

•  */
  

• if (htab->filled == htab->size) {
  

• 	*retval = NULL;
  

• 	return 0;
  

• }
  

• /* Create new entry */
  

• htab->table[new].used = hval;
  

• ++htab->filled;
  

• /* return new entry */
  

• *retval = &htab->table[new];
  

• return 1;
  

• }
• *retval = NULL;
• return 0;

+}

+/* from lib/hashtable.c */ +static inline int isprime(unsigned int number) +{

• /* no even number will be passed */
• unsigned int div = 3;
• while (div * div < number && number % div != 0)

• div += 2;
  

• return number % div != 0;

+}

+efi_status_t efi_freeze_variable_table(void)

Please, add comments to your functions. It is not self-evident what this function is meant to do.

I cannot imagine why a variable cache should be frozen. It is a living data structure until the system is switched off.

I don't get your point.

Please, explain what you mean by freeze.

I suggest that the cache is read/write at all times.

Best regards

Heinrich

...

...

For a variable cache I expect that you allocate memory before handling the first variable and never again. At runtime you will not have chance to allocate memory anyway.

I don't get your point.

...

This function is way too long. Pleae, break it down.

Freezing is implemented in 2-phase steps, and some complexity is inevitable. I suppose adding some comments would be enough.

-Takahiro Akashi

...

Best regards

Heinrich

...

+{

• int var_num = 0;
• size_t var_data_size = 0;
• u16 *name;
• efi_uintn_t name_buf_len, name_len;
• efi_guid_t vendor;
• u32 attributes;
• u8 *mem_pool, *var_buf = NULL;
• size_t table_size, var_size, var_buf_size;
• _ENTRY *new = NULL;
• efi_status_t ret;
• /* phase-1 loop */
• name_buf_len = 128;
• name = malloc(name_buf_len);
• if (!name)

• return EFI_OUT_OF_RESOURCES;
  

• name[0] = 0;
• for (;;) {

• name_len = name_buf_len;
  

• ret = EFI_CALL(efi_get_next_variable_name(&name_len, name,
  

• 					  &vendor));
  

• if (ret == EFI_NOT_FOUND) {
  

• 	break;
  

• } else if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	u16 *buf;
  

• 	name_buf_len = name_len;
  

• 	buf = realloc(name, name_buf_len);
  

• 	if (!buf) {
  

• 		free(name);
  

• 		return EFI_OUT_OF_RESOURCES;
  

• 	}
  

• 	name = buf;
  

• 	name_len = name_buf_len;
  

• 	ret = EFI_CALL(efi_get_next_variable_name(&name_len,
  

• 						  name,
  

• 						  &vendor));
  

• }
  

• if (ret != EFI_SUCCESS)
  

• 	return ret;
  

• var_size = 0;
  

• ret = EFI_CALL(efi_get_variable(name, &vendor, &attributes,
  

• 				&var_size, NULL));
  

• if (ret != EFI_BUFFER_TOO_SMALL)
  

• 	return ret;
  

• if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
  

• 	continue;
  

• var_num++;
  

• var_data_size += (u16_strlen_runtime(name) + 1) * sizeof(u16);
  

• var_data_size += var_size;
  

• /* mem_pool must 2-byte aligned for u16 variable name */
  

• if (var_data_size & 0x1)
  

• 	var_data_size++;
  

• }
• /*

• * total of entries in hash table must be a prime number.
  

• * The logic below comes from lib/hashtable.c
  

• */
  

• var_num |= 1; /* make odd */
• while (!isprime(var_num))

• var_num += 2;
  

• /* We need table[var_num] for hsearch_runtime algo */
• table_size = sizeof(*efi_variable_table)

• 	+ sizeof(_ENTRY) * (var_num + 1) + var_data_size;
  

• ret = efi_allocate_pool(EFI_RUNTIME_SERVICES_DATA,

• 		table_size, (void **)&efi_variable_table);
  

• if (ret != EFI_SUCCESS)

• return ret;
  

• efi_variable_table->size = var_num;
• efi_variable_table->table = (void *)efi_variable_table

• 			+ sizeof(*efi_variable_table);
  

• mem_pool = (u8 *)efi_variable_table->table

• 	+ sizeof(_ENTRY) * (var_num + 1);
  

• var_buf_size = 128;
• var_buf = malloc(var_buf_size);
• if (!var_buf) {

• ret = EFI_OUT_OF_RESOURCES;
  

• goto err;
  

• }
• /* phase-2 loop */
• name[0] = 0;
• name_len = name_buf_len;
• for (;;) {

• name_len = name_buf_len;
  

• ret = EFI_CALL(efi_get_next_variable_name(&name_len, name,
  

• 					  &vendor));
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• else if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• var_size = var_buf_size;
  

• ret = EFI_CALL(efi_get_variable(name, &vendor, &attributes,
  

• 				&var_size, var_buf));
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	free(var_buf);
  

• 	var_buf_size = var_size;
  

• 	var_buf = malloc(var_buf_size);
  

• 	if (!var_buf) {
  

• 		ret = EFI_OUT_OF_RESOURCES;
  

• 		goto err;
  

• 	}
  

• 	ret = EFI_CALL(efi_get_variable(name, &vendor,
  

• 					&attributes,
  

• 					&var_size, var_buf));
  

• }
  

• if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
  

• 	continue;
  

• if (hsearch_runtime(name, &vendor, ENTER, &new,
  

• 		    efi_variable_table) <= 0) {
  

• 	/* This should not happen */
  

• 	ret = EFI_INVALID_PARAMETER;
  

• 	goto err;
  

• }
  

• /* allocate space from RUNTIME DATA */
  

• name_len = (u16_strlen_runtime(name) + 1) * sizeof(u16);
  

• memcpy_runtime(mem_pool, name, name_len);
  

• new->name = mem_pool - (u8 *)new; /* offset */
  

• mem_pool += name_len;
  

• memcpy_runtime(&new->vendor.b, &vendor.b, sizeof(vendor));
  

• new->attributes = attributes;
  

• memcpy_runtime(mem_pool, var_buf, var_size);
  

• new->data = mem_pool - (u8 *)new; /* offset */
  

• new->data_size = var_size;
  

• mem_pool += var_size;
  

• /* mem_pool must 2-byte aligned for u16 variable name */
  

• if ((uintptr_t)mem_pool & 0x1)
  

• 	mem_pool++;
  

• }

+#ifdef DEBUG

• name[0] = 0;
• name_len = name_buf_len;
• for (;;) {

• name_len = name_buf_len;
  

• ret = efi_get_next_variable_name_runtime(&name_len, name,
  

• 					 &vendor);
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• else if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• var_size = var_buf_size;
  

• ret = efi_get_variable_runtime(name, &vendor, &attributes,
  

• 			       &var_size, var_buf);
  

• if (ret != EFI_SUCCESS)
  

• 	goto err;
  

• printf("%ls_%pUl:\n", name, &vendor);
  

• printf("    attributes: 0x%x\n", attributes);
  

• printf("    value (size: 0x%lx)\n", var_size);
  

• }

+#endif

• ret = EFI_SUCCESS;

+err:

• free(name);
• free(var_buf);
• if (ret != EFI_SUCCESS && efi_variable_table) {

• efi_free_pool(efi_variable_table);
  

• efi_variable_table = NULL;
  

• }
• return ret;

+}

+efi_status_t +__efi_runtime EFIAPI efi_get_variable_runtime(u16 *variable_name,

• 			      const efi_guid_t *vendor,
  

• 			      u32 *attributes,
  

• 			      efi_uintn_t *data_size,
  

• 			      void *data)
  

+{

• _ENTRY *new;
• if (!variable_name || !vendor || !data_size)

• return EFI_EXIT(EFI_INVALID_PARAMETER);
  

• if (hsearch_runtime(variable_name, vendor, FIND, &new,

• 	    efi_variable_table) <= 0)
  

• return EFI_NOT_FOUND;
  

• if (attributes)

• *attributes = new->attributes;
  

• if (*data_size < new->data_size) {

• *data_size = new->data_size;
  

• return EFI_BUFFER_TOO_SMALL;
  

• }
• *data_size = new->data_size;
• memcpy_runtime(data, entry_data(new), new->data_size);
• return EFI_SUCCESS;

+}

+static int prev_idx __efi_runtime_data;

+efi_status_t +__efi_runtime EFIAPI efi_get_next_variable_name_runtime(

• 				efi_uintn_t *variable_name_size,
  

• 				u16 *variable_name,
  

• 				const efi_guid_t *vendor)
  

+{

• _ENTRY *e;
• u16 *name;
• efi_uintn_t name_size;
• if (!variable_name_size || !variable_name || !vendor)

• return EFI_INVALID_PARAMETER;
  

• if (variable_name[0]) {

• /* sanity check for previous variable */
  

• if (prev_idx < 0)
  

• 	return EFI_INVALID_PARAMETER;
  

• e = &efi_variable_table->table[prev_idx];
  

• if (!e->used || efi_cmpkey(e, variable_name, vendor))
  

• 	return EFI_INVALID_PARAMETER;
  

• } else {

• prev_idx = -1;
  

• }
• /* next variable */
• while (++prev_idx <= efi_variable_table->size) {

• e = &efi_variable_table->table[prev_idx];
  

• if (e->used)
  

• 	break;
  

• }
• if (prev_idx > efi_variable_table->size)

• return EFI_NOT_FOUND;
  

• name = entry_name(e);
• name_size = (u16_strlen_runtime(name) + 1)

• 	* sizeof(u16);
  

• if (*variable_name_size < name_size) {

• *variable_name_size = name_size;
  

• return EFI_BUFFER_TOO_SMALL;
  

• }
• memcpy_runtime(variable_name, name, name_size);
• memcpy_runtime((void *)&vendor->b, &e->vendor.b, sizeof(vendor));
• return EFI_SUCCESS;

+} +#endif /* CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING */