OpenBSI and U-Boot

[Sean Anderson]

On 8/8/20 2:56 PM, Heinrich Schuchardt wrote:
> On 8/8/20 7:22 PM, Sean Anderson wrote:
>> On 8/8/20 12:17 PM, Heinrich Schuchardt wrote:
>>> On 8/8/20 5:32 PM, Sean Anderson wrote:
>>>> On 8/8/20 10:59 AM, Heinrich Schuchardt wrote:
>>>>> Hello Anup,
>>>>>
>>>>> I have looking at you OpenSBI code firmware/payloads/test_head.S. Here
>>>>
>>>> I think the real start is in firmware/fw_base.S. From there, secondary
>>>> harts loop first in _wait_relocate_copy_done, and then in
>>>> _wait_for_boot_hart, and then they execute the next stage via
>>>> _start_warm and sbi_init.
>>>>
>>>>> like in U-Boot's common/spl/spl_opensbi.c you put all but one hart in to
>>>>> an enless loop (hang).
>>>>
>>>> As far as I can tell, U-Boot has all harts execute the next stage when
>>>> SMP is enabled. smp_call_function has all harts execute that function.
>>>
>>> U-Boot can only run on one hart. Are the other harts trapped in
>>> secondary_hart_loop()?
>>
>> Yes. They also need handle_ipi, and by extension riscv_clear_ipi. This
>> latter function currently requires that gd_t be valid, and may require
>> other structures (e.g. a struct udevice) to be valid in the future.
>>
>>> How do we ensure that an UEFI payload does not overwrite this memory location?
>>
>> The most foolproof is probably to wait for all harts to start running
>> UEFI code before making any modifications to ram outside the binary. One
>> easy way to do this is to use amoadd instead of amoswap (e.g. a semaphor
>> and not a mutex) in the standard boot lottery code. Whichever hart gets
>> to it first then waits for the value of hart_lottery to reach the
>> expected number of harts.
> 
> There is no such requirement in the UEFI specification.

Hm, well perhaps there should be a shim (or patch to U-Boot) which
implements such a requirement. AFAIK (and please correct me if there is
another option) there is no way to communicate between harts except by
interrupt or shared memory. Interrupts may require substantial code to
handle properly (which could be difficult to trace the requirements of).
However, shared memory only requires one or two functions to be valid,
plus the memory location itself. I think that solution has been mostly
avoided by U-Boot since the rest of U-Boot is not designed for SMP, so
there is not much infrastructure for atomics, etc.
 
> The way to tell which memory should not be overwritten by the UEFI
> payload is an entry in the memory map that the payload can read via
> GetMemoryMap(). So we have to make reservations in the memory map, by
> calling efi_add_memory_map() or by putting the code into the
> __efi_runtime section.
> 
> Do I understand it correctly that the secondary harts stay in the
> unrelocated secondary_hart_loop()? In this case __efi_runtime would not
> be the right section, because that memory section is also relocated and
> only the relocated code is reserved in the memory map.

The other harts get relocated in relocate_secondary_harts.

> 
> We would also have to consider the location of secondary_hart_loop()
> when defining the load address of any payload be it UEFI or not.

Yes. I was looking at your patch to add load addresses [1], and I'm
concerned that 80400000 may be too high for the average kernel. Since
U-Boot relocates to just below 80600000, that leaves only 1.5M or so for
the kernel to be loaded into.

--Sean

[1] https://patchwork.ozlabs.org/project/uboot/patch/20200729154235.90766-4-xypron.glpk@gmx.de/