| Message ID | 20190605042142.15113-4-takahiro.akashi@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: support runtime variable access via cache | expand |
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. > > 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. > +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. *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. 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 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 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 <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, >>>> >>> >> > >
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; + +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; + if (addr >= map->physical_start && + (addr < (map->physical_start + + (map->num_pages << EFI_PAGE_SHIFT)))) { + *address = (void *)map->virtual_start; + *address += addr - map->physical_start; + + 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; - } } +#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, +#endif .get_variable = efi_get_variable, .get_next_variable_name = efi_get_next_variable_name, .set_variable = efi_set_variable,
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. 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(-)