Re: [PATCH v3 7/7] riscv: Add some comments to start.S
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 >> Reviewed-by: Bin Meng >> Reviewed-by: Rick Chen >> Reviewed-by: Leo Liang >> --- >> >> (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: >> csrra0, 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
Re: [PATCH v3 7/7] riscv: Add some comments to start.S
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 > Reviewed-by: Bin Meng > Reviewed-by: Rick Chen > Reviewed-by: Leo Liang > --- > > (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: > csrra0, 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 > csrwMODE_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 > + */ > csrwMODE_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 > >
[PATCH 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 Reviewed-by: Bin Meng Reviewed-by: Rick Chen Reviewed-by: Leo Liang --- (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: csrra0, 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 @@ -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 csrwMODE_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 +*/ csrwMODE_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 -- 2.28.0