[U-Boot] efi_loader: runtime services implementation broken?

[Heinrich Schuchardt]

On 07/03/2018 06:51 PM, Bin Meng wrote:
> Hi Alex,
> 
> On Tue, Jul 3, 2018 at 11:05 PM, Alexander Graf <agraf at suse.de> wrote:
>> On 07/03/2018 04:56 PM, Bin Meng wrote:
>>>
>>> Hi Alex,
>>>
>>> On Tue, Jul 3, 2018 at 10:32 PM, Alexander Graf <agraf at suse.de> wrote:
>>>>
>>>> On 07/03/2018 04:24 PM, Bin Meng wrote:
>>>>>
>>>>> Hi Alex, Heinrich,
>>>>>
>>>>> At present not all interfaces in lib/efi_loader/efi_runtime.c are
>>>>> declared as __efi_runtime. But only declaring those as __efi_runtime
>>>>> is not enough. The data these interfaces use should be declared as
>>>>> __efi_runtime_data too. More worse, any U-Boot APIs called by these
>>>>> interfaces should be __efi_runtime and __efi_runtime_data
>>>>> respectively.
>>>>
>>>>
>>>> Correct. This is done for everything right now, no?
>>>
>>> For example, efi_set_virtual_address_map() is not declared as
>>> __efi_runtime.
>>
>>
>> Is that called post exit boot services on x86?
> 
> Yes, if you look at efi_runtime_services structure in
> lib/efi_loader/efi_runtime.c, you can see
> efi_set_virtual_address_map() is hooked up there, and all interfaces
> in efi_runtime_services are supposed to be called by OS as their names
> indicate, runtime not bootime.

No, this not correct. Runtime functions may be called before and after
ExitBootServices. Otherwise the EFI shell would not be able to read
variables for instance.

The SetVirtualAdressMap service is only called after ExitBootServices.
At that time the caller is the owner of the memory map. So this function
must be marked as __efi_runtime.

Our current coding makes the invalid assumption that U-Boot is owner of
the memory until SetVirtualAddressMap() is called.

Other deficiencies are that we do not provide the ConvertPointer()
service and do not raise notify an event when SetVirtualAddressMap() is
called.

> 
>>
>>>
>>>>> Eventually we need mark the RAM where the whole U-Boot image lives as
>>>>> runtime service code and data and end up leaving whole U-Boot image in
>>>>> the RAM after kernel boots?
>>>>
>>>>
>>>> Why?
>>>>
>>> Unless we track down every APIs called by runtime services and mark
>>> them correctly as __efi_runtime code/data.
>>
>>
>> That's the idea, yes. The vector should be quite small as long as we don't
>> actually implement / reuse drivers.
> 
> However this currently seems not to be the case ..
> 
>>
>>>
>>>>> Right now I am testing booting Linux on Intel Galileo with 'bootefi'
>>>>> and kernel just hangs during the boot. Initial debugging shows that
>>>>> kernel crashes when calling runtime service
>>>>> efi_set_virtual_address_map().
>>>>

Is this a regression or did it never work?

Best regards

Heinrich

>>>>
>>>> Can you please try with my efi-next branch? I added a few patches that
>>>> allow
>>>> gcc to put implicit data and code into the runtime section. Without
>>>> those,
>>>> you may get constant propagation into a the common .rodata segment for
>>>> example.
>>>>
>>> Still the same.
>>
>>
>> Ok, maybe the x86 efi stub does things different from the ARM one then.
>>
> 
> maybe :(
> 
> Regards,
> Bin
>