[U-Boot] [RFC 3/6] efi_loader: support convert_pointer at runtime

Heinrich Schuchardt xypron.glpk at gmx.de
Mon Jun 17 05:41:30 UTC 2019


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