[U-Boot] [RFC 3/6] efi_loader: support convert_pointer at runtime
Heinrich Schuchardt
xypron.glpk at gmx.de
Sat Jul 13 06:16:11 UTC 2019
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 at 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 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,
>>>>
>>>
>>
>
>
More information about the U-Boot
mailing list