From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Mon, 17 Jun 2019 07:41:30 +0200 Subject: [U-Boot] [RFC 3/6] efi_loader: support convert_pointer at runtime In-Reply-To: <20190617011513.GB6610@linaro.org> References: <20190605042142.15113-1-takahiro.akashi@linaro.org> <20190605042142.15113-4-takahiro.akashi@linaro.org> <20190617011513.GB6610@linaro.org> Message-ID: <79f07d7a-793d-3391-18a0-141019171ea4@gmx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. Regards Heinrich > >>> >>> Signed-off-by: AKASHI Takahiro >>> --- >>> 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 at 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, >>> >> >