[v3,7/7] riscv: Add some comments to start.S

This adds comments regarding the ordering and purpose of certain
instructions as I understand them.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Rick Chen <rick@andestech.com>
Reviewed-by: Leo Liang <ycliang@andestech.com>
---

(no changes since v2)

Changes in v2:
- Clarify comments regarding tp

 arch/riscv/cpu/start.S | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

On 21.09.20 13:51, Sean Anderson wrote:
> This adds comments regarding the ordering and purpose of certain
> instructions as I understand them.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
> Reviewed-by: Rick Chen <rick@andestech.com>
> Reviewed-by: Leo Liang <ycliang@andestech.com>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Clarify comments regarding tp
>
>  arch/riscv/cpu/start.S | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index eb852538ca..bbc737ed9a 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -43,7 +43,10 @@ _start:
>  	csrr	a0, CSR_MHARTID
>  #endif
>
> -	/* save hart id and dtb pointer */
> +	/*
> +	 * Save hart id and dtb pointer. The thread pointer register is not
> +	 * modified by C code. It is used by secondary_hart_loop.
> +	 */
>  	mv	tp, a0
>  	mv	s1, a1

I would like to understand the implications for the UEFI sub-system.

The current design only takes care of the conservation of the global
data gd.

When U-Boot calls a UEFI payload it saves the value of gd (register gp,
x3) in a variable.

When the payload calls the UEFI API implemented in U-Boot we store the
payload's value of register gp and restore the U-Boot value (see macro
EFI_ENTRY).

Before returning from the UEFI call we restore the payload's value of gp
(see macro EFI_EXIT).

When a payload returns to the U-Boot shell we restore gp.

Up to now we do not take care of any other register. I am not aware of
any specification requiring register tp to be conserved by the UEFI payload.

Is there any other register that has to be conserved except gp?

Best regards

Heinrich

>
> @@ -54,10 +57,18 @@ _start:
>  	 */
>  	mv	gp, zero
>
> +	/*
> +	 * Set the trap handler. This must happen after initializing gp because
> +	 * the handler may use it.
> +	 */
>  	la	t0, trap_entry
>  	csrw	MODE_PREFIX(tvec), t0
>
> -	/* mask all interrupts */
> +	/*
> +	 * Mask all interrupts. Interrupts are disabled globally (in m/sstatus)
> +	 * for U-Boot, but we will need to read m/sip to determine if we get an
> +	 * IPI
> +	 */
>  	csrw	MODE_PREFIX(ie), zero
>
>  #if CONFIG_IS_ENABLED(SMP)
> @@ -412,6 +423,10 @@ secondary_hart_relocate:
>  	mv	gp, a2
>  #endif
>
> +/*
> + * Interrupts are disabled globally, but they can still be read from m/sip. The
> + * wfi function will wake us up if we get an IPI, even if we do not trap.
> + */
>  secondary_hart_loop:
>  	wfi
>
>

On 9/21/20 12:23 PM, Heinrich Schuchardt wrote:
> On 21.09.20 13:51, Sean Anderson wrote:
>> This adds comments regarding the ordering and purpose of certain
>> instructions as I understand them.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> Reviewed-by: Bin Meng <bin.meng@windriver.com>
>> Reviewed-by: Rick Chen <rick@andestech.com>
>> Reviewed-by: Leo Liang <ycliang@andestech.com>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - Clarify comments regarding tp
>>
>>  arch/riscv/cpu/start.S | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>> index eb852538ca..bbc737ed9a 100644
>> --- a/arch/riscv/cpu/start.S
>> +++ b/arch/riscv/cpu/start.S
>> @@ -43,7 +43,10 @@ _start:
>>  	csrr	a0, CSR_MHARTID
>>  #endif
>>
>> -	/* save hart id and dtb pointer */
>> +	/*
>> +	 * Save hart id and dtb pointer. The thread pointer register is not
>> +	 * modified by C code. It is used by secondary_hart_loop.
>> +	 */
>>  	mv	tp, a0
>>  	mv	s1, a1
> 
> I would like to understand the implications for the UEFI sub-system.
> 
> The current design only takes care of the conservation of the global
> data gd.
> 
> When U-Boot calls a UEFI payload it saves the value of gd (register gp,
> x3) in a variable.
> 
> When the payload calls the UEFI API implemented in U-Boot we store the
> payload's value of register gp and restore the U-Boot value (see macro
> EFI_ENTRY).
> 
> Before returning from the UEFI call we restore the payload's value of gp
> (see macro EFI_EXIT).
> 
> When a payload returns to the U-Boot shell we restore gp.
> 
> Up to now we do not take care of any other register. I am not aware of
> any specification requiring register tp to be conserved by the UEFI payload.
> 
> Is there any other register that has to be conserved except gp?

So the core problem here is that there is no way to get the current hart
id from S-Mode, except by saving it from a0 on entry. I believe an SBI
call was proposed, but it was deemed unnecessary. Unfortunately, gp is
already in-use for gd. On the boot hart, the current hart id is stored
in gd->arch.boot_hart, so tp is not used. On secondary harts, the hart
id is stored in tp, and must be valid for secondary_hart_loop. AFAIK tp
is the only other register besides gp which needs to be preserved on
secondary harts.

When we spoke on IRC a few weeks ago, I you said that only the boot hart
enters UEFI payloads. So there should be no need to save tp for UEFI,
because it is unused on the boot hart. If a payload does not return to
U-Boot, there is also no need to save tp on secondary harts. The only
issue would be if a payload is started on secondary harts and then
returns to U-Boot.

--Sean