OpenBSI and U-Boot

[Heinrich Schuchardt]

On 8/9/20 3:48 AM, Sean Anderson wrote:
> 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.

I added the following:

 secondary_hart_loop:
+
+       lui     a5,0x20040
+       addi    a5,a5,0x48c
+       slli    a5,a5,0x2
+       /* a5 = 80101230 */
+       auipc   a4, 0
+       sw      a4,0(a5)
+       lw      a4,8(a5)
+       addw    a4,a4,1
+       sw      a4,8(a5)
+

With the mm.w command I can see what is stored at 0x80101230 - 0x8010123f.

On the MaixDuino secondary_hart_loop() is executed 1 to 2 times.
Shouldn't it be running all the time?

relocate_secondary_harts() is called once.

hang() is never executed.

Any idea what the secondary hart is executing after
relocate_secondary_harts()?

Best regards

Heinrich

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