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.aka...@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(s...@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,
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
