[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Alexander Graf]

On 09/11/2016 10:50, Stephen Warren wrote:
> On 11/09/2016 04:43 AM, Alexander Graf wrote:
>>
>>
>> On 07/11/2016 22:26, Stephen Warren wrote:
>>> On 11/06/2016 03:24 AM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 05/11/2016 23:01, Stephen Warren wrote:
>>>>> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>>>>>> The rpi has a pretty simple way of resetting the whole system. All it
>>>>>> takes
>>>>>> is to poke a few registers at a well defined location in MMIO space.
>>>>>>
>>>>>> This patch adds support for the EFI loader implementation to allow an
>>>>>> OS to
>>>>>> reset and power off the system when we're outside of boot time.
>>>>>
>>>>> (As an aside, I'm not sure why someone wanting EFI wouldn't just use a
>>>>> complete EFI implementation such as TianoCore.)
>>>>>
>>>>>> diff --git a/arch/arm/mach-bcm283x/reset.c
>>>>>> b/arch/arm/mach-bcm283x/reset.c
>>>>>
>>>>>> +__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
>>>>>> +    (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>>>> +
>>>>>> +void __efi_runtime reset_cpu(ulong addr)
>>>>>>  {
>>>>>> -    struct bcm2835_wdog_regs *regs =
>>>>>> -        (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>>>
>>>>> I'm not sure why that change is required. The value of the variable is
>>>>> the same in both cases?
>>>>
>>>> Take a look a few lines down in the patch:
>>>>
>>>>> +void efi_reset_system_init(void)
>>>>> +{
>>>>> +    efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
>>>>> +}
>>>>
>>>> What this does is register a *pointer* as run time service pointer.
>>>> What
>>>> does that mean?
>>>>
>>>> When we enter RTS, Linux can map any region in the EFI memory map
>>>> into a
>>>> different place in its own virtual memory map. So any pointers we use
>>>> inside RTS have to be relocated to the new locations.
>>>>
>>>> For normal relocations, we move the relocations from linker time to run
>>>> time, so that we can relocate ourselves when Linux does the switch-over
>>>> to a new address space.
>>>>
>>>> However, for MMIO that's trickier. That's where the
>>>> efi_add_runtime_mmio() function comes into play. It takes care of
>>>> adding
>>>> the page around the references address to the EFI memory map as RTS
>>>> MMIO
>>>> and relocates the pointer when Linux switches us into the new address
>>>> space.
>>>>
>>>> Does that explain why we need to move from an inline address to an
>>>> address stored in a memory location?
>>>
>>> So EFI RTS runs in the same exception level as the rich OS, and not in
>>> EL3? I would have expected EFI to run in EL3 with a completely separate
>>> MMU configuration. If that's not the case, then this part of the patch
>>> does make sense.
>>
>> Right, it runs in EL2/EL1 with a virtual memory layout that is provided
>> by the OS.
>>
>>>
>>>>> Perhaps it's trying to ensure that if this gets compiled into an ldr
>>>>> instruction, the referenced data value is in a linker section that's
>>>>> still around when EFI runs? If so fine, but how is that ensured for
>>>>> all
>>>>> the other constants that this code uses, and if that happens
>>>>> automatically due to the __efi_runtime marker above, why doesn't it
>>>>> work
>>>>> for this one constant?
>>>>>
>>>>> Does U-Boot have a halt/poweroff/shutdown shell command? If so, it
>>>>> might
>>>>> be nice to enable it as part of this series, since the code to perform
>>>>> that operation is now present.
>>>>
>>>> That's what I originally wanted, yes :). Unfortunately due to the
>>>> relocation explained above, it's basically impossible for any reset
>>>> function that calls into MMIO space.
>>>>
>>>> However, we do have it now for PSCI. If you have a PSCI enabled system,
>>>> we don't need to call into MMIO space and thus make the common reset
>>>> function available as RTS.
>>>
>>> Can't the same U-Boot function be called both (a) during U-Boot runtime,
>>> where wdog_regs are pre-initialized to match U-Boot's MMU configuration,
>>> and (b) once the OS has booted, where wdog_regs has been modified
>>> according to the new memory map?
>>
>> That's exactly what this patch does, no?
>
> I assume not, since you said just a few lines above that doing so was
> impossible, hence why it doesn't implement any halt/poweroff/shutdown
> shell commands.

So this patch transforms the code in a way that allows us to call the 
same function from boot time (1:1 map) as well as run time 
(non-voluntary map). I thought that's what you were suggesting.

Either way, does the approach make sense to you now?


Alex