OpenBSI and U-Boot

[Sean Anderson]

On 8/9/20 12:23 AM, Heinrich Schuchardt wrote:
> Am 9. August 2020 03:48:19 MESZ schrieb Sean Anderson <seanga2 at gmail.com>:
>> 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.
> 
> The patch has:
> 
> kernel_addr_r=0x80040000
> 
> You somehow missed the position of "4".

Ah, so I did. That looks fine then.

--Sean