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.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
