| Message ID | 3edbb40e86e480b2b71d596ff61c05336004b14c.1677762812.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Do basic initialization things | expand |
On 02/03/2023 1:23 pm, Oleksii Kurochko wrote: > diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S > index ffd95f9f89..851b4691a5 100644 > --- a/xen/arch/riscv/riscv64/head.S > +++ b/xen/arch/riscv/riscv64/head.S > @@ -6,8 +7,31 @@ ENTRY(start) > /* Mask all interrupts */ > csrw CSR_SIE, zero > > + /* Save HART ID and DTB base */ > + lla a6, _bootcpu_id > + REG_S a0, (a6) > + lla a6, _dtb_base > + REG_S a1, (a6) > + > la sp, cpu0_boot_stack > li t0, STACK_SIZE > add sp, sp, t0 > > + lla a6, _bootcpu_id > + REG_L a0, (a6) > + lla a6, _dtb_base > + REG_L a1, (a6) This is overkill. Put a comment at start identifying which parameters are in which registers, and just make sure not to clobber them - RISCV has plenty of registers. Right now, we don't touch the a registers at all, which is why your v1 patch worked. (a0 and a1 still have the same value when we get into C). If we do need to start calling more complex things here (and I'm not sure that we do), we could either store the parameters in s2-11, or spill them onto the stack; both of which are preferable to spilling to global variables like this. > + > tail start_xen > + > + /* > + * Boot cpu id is passed by a bootloader > + */ > +_bootcpu_id: > + RISCV_PTR 0x0 Just a note (as you want to delete this anyway), this isn't a PTR, it's a LONG. > + > + /* > + * DTB base is passed by a bootloader > + */ > +_dtb_base: > + RISCV_PTR 0x0 > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c > index 1c87899e8e..d9723fe1c0 100644 > --- a/xen/arch/riscv/setup.c > +++ b/xen/arch/riscv/setup.c > @@ -7,7 +7,8 @@ > unsigned char __initdata cpu0_boot_stack[STACK_SIZE] > __aligned(STACK_SIZE); > > -void __init noreturn start_xen(void) > +void __init noreturn start_xen(unsigned long bootcpu_id, > + unsigned long dtb_base) To be clear, this change should be this hunk exactly as it is, and a comment immediately ahead of ENTRY(start) describing the entry ABI. There is no need currently to change any of the asm code. ~Andrew
On Thu, 2023-03-02 at 14:02 +0000, Andrew Cooper wrote: > On 02/03/2023 1:23 pm, Oleksii Kurochko wrote: > > diff --git a/xen/arch/riscv/riscv64/head.S > > b/xen/arch/riscv/riscv64/head.S > > index ffd95f9f89..851b4691a5 100644 > > --- a/xen/arch/riscv/riscv64/head.S > > +++ b/xen/arch/riscv/riscv64/head.S > > @@ -6,8 +7,31 @@ ENTRY(start) > > /* Mask all interrupts */ > > csrw CSR_SIE, zero > > > > + /* Save HART ID and DTB base */ > > + lla a6, _bootcpu_id > > + REG_S a0, (a6) > > + lla a6, _dtb_base > > + REG_S a1, (a6) > > + > > la sp, cpu0_boot_stack > > li t0, STACK_SIZE > > add sp, sp, t0 > > > > + lla a6, _bootcpu_id > > + REG_L a0, (a6) > > + lla a6, _dtb_base > > + REG_L a1, (a6) > > This is overkill. > > Put a comment at start identifying which parameters are in which > registers, and just make sure not to clobber them - RISCV has plenty > of > registers. > > Right now, we don't touch the a registers at all, which is why your > v1 > patch worked. (a0 and a1 still have the same value when we get into > C). > > If we do need to start calling more complex things here (and I'm not > sure that we do), we could either store the parameters in s2-11, or > spill them onto the stack; both of which are preferable to spilling > to > global variables like this. > > > + > > tail start_xen > > + > > + /* > > + * Boot cpu id is passed by a bootloader > > + */ > > +_bootcpu_id: > > + RISCV_PTR 0x0 > > Just a note (as you want to delete this anyway), this isn't a PTR, > it's > a LONG. > > > + > > + /* > > + * DTB base is passed by a bootloader > > + */ > > +_dtb_base: > > + RISCV_PTR 0x0 > > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c > > index 1c87899e8e..d9723fe1c0 100644 > > --- a/xen/arch/riscv/setup.c > > +++ b/xen/arch/riscv/setup.c > > @@ -7,7 +7,8 @@ > > unsigned char __initdata cpu0_boot_stack[STACK_SIZE] > > __aligned(STACK_SIZE); > > > > -void __init noreturn start_xen(void) > > +void __init noreturn start_xen(unsigned long bootcpu_id, > > + unsigned long dtb_base) > > To be clear, this change should be this hunk exactly as it is, and a > comment immediately ahead of ENTRY(start) describing the entry ABI. > > There is no need currently to change any of the asm code. I think that I'll use s2 and s3 to save bootcpu_id. But I am unsure I understand why the asm code shouldn't be changed. I mean that a0-7 are used as function arguments, a0-1 are used for return value so they are expected to be changed. That is why we have to save them somewhere. If I understand you correctly I can write in a comment ahead of ENTRY(start) that a0, and a1 are reserved for hart_id and dtb_base which are passed from a bootloader but it will work only if start_xen will be only C function called from head.S. I probably misunderstand you... ~ Oleksii
On 02/03/2023 2:53 pm, Oleksii wrote: > On Thu, 2023-03-02 at 14:02 +0000, Andrew Cooper wrote: >> On 02/03/2023 1:23 pm, Oleksii Kurochko wrote: >>> + >>> + /* >>> + * DTB base is passed by a bootloader >>> + */ >>> +_dtb_base: >>> + RISCV_PTR 0x0 >>> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c >>> index 1c87899e8e..d9723fe1c0 100644 >>> --- a/xen/arch/riscv/setup.c >>> +++ b/xen/arch/riscv/setup.c >>> @@ -7,7 +7,8 @@ >>> unsigned char __initdata cpu0_boot_stack[STACK_SIZE] >>> __aligned(STACK_SIZE); >>> >>> -void __init noreturn start_xen(void) >>> +void __init noreturn start_xen(unsigned long bootcpu_id, >>> + unsigned long dtb_base) >> To be clear, this change should be this hunk exactly as it is, and a >> comment immediately ahead of ENTRY(start) describing the entry ABI. >> >> There is no need currently to change any of the asm code. > I think that I'll use s2 and s3 to save bootcpu_id. > > But I am unsure I understand why the asm code shouldn't be changed. Because nothing in the asm code (right now) touches any of the a registers. Therefore the parameters that OpenSBI prepared for start() (i.e. a0 and a1 here) are still correct for start_xen(). If, and only if, we need to modify a* for other reasons in start() do we need to preserve their values somehow. > If I understand you correctly I can write in a comment ahead of > ENTRY(start) that a0, and a1 are reserved for hart_id and dtb_base > which are passed from a bootloader but it will work only if start_xen > will be only C function called from head.S. Not quite. You want a comment explaining what the OpenSBI -> start() ABI is. So people know what a0/etc is at ENTRY(start). Here is an example from a different project: https://github.com/TrenchBoot/secure-kernel-loader/blob/master/head.S#L52-L68 There is no need to do unnecessary work (i.e. preserving them right now), until you find a reason to need to spill them. Right now, there's not need, and this isn't obviously going to change in the short term. ~Andrew
On Thu, 2023-03-02 at 18:58 +0000, Andrew Cooper wrote: > On 02/03/2023 2:53 pm, Oleksii wrote: > > On Thu, 2023-03-02 at 14:02 +0000, Andrew Cooper wrote: > > > On 02/03/2023 1:23 pm, Oleksii Kurochko wrote: > > > > + > > > > + /* > > > > + * DTB base is passed by a bootloader > > > > + */ > > > > +_dtb_base: > > > > + RISCV_PTR 0x0 > > > > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c > > > > index 1c87899e8e..d9723fe1c0 100644 > > > > --- a/xen/arch/riscv/setup.c > > > > +++ b/xen/arch/riscv/setup.c > > > > @@ -7,7 +7,8 @@ > > > > unsigned char __initdata cpu0_boot_stack[STACK_SIZE] > > > > __aligned(STACK_SIZE); > > > > > > > > -void __init noreturn start_xen(void) > > > > +void __init noreturn start_xen(unsigned long bootcpu_id, > > > > + unsigned long dtb_base) > > > To be clear, this change should be this hunk exactly as it is, > > > and a > > > comment immediately ahead of ENTRY(start) describing the entry > > > ABI. > > > > > > There is no need currently to change any of the asm code. > > I think that I'll use s2 and s3 to save bootcpu_id. > > > > But I am unsure I understand why the asm code shouldn't be changed. > > Because nothing in the asm code (right now) touches any of the a > registers. > > Therefore the parameters that OpenSBI prepared for start() (i.e. a0 > and > a1 here) are still correct for start_xen(). > > If, and only if, we need to modify a* for other reasons in start() do > we > need to preserve their values somehow. > > > If I understand you correctly I can write in a comment ahead of > > ENTRY(start) that a0, and a1 are reserved for hart_id and dtb_base > > which are passed from a bootloader but it will work only if > > start_xen > > will be only C function called from head.S. > > Not quite. You want a comment explaining what the OpenSBI -> start() > ABI is. So people know what a0/etc is at ENTRY(start). > > Here is an example from a different project: > https://github.com/TrenchBoot/secure-kernel-loader/blob/master/head.S#L52-L68 > > > There is no need to do unnecessary work (i.e. preserving them right > now), until you find a reason to need to spill them. Right now, > there's > not need, and this isn't obviously going to change in the short term. > Thanks for clarification. I will document ABI for start and that's it for now ~ Oleksii
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S index ffd95f9f89..851b4691a5 100644 --- a/xen/arch/riscv/riscv64/head.S +++ b/xen/arch/riscv/riscv64/head.S @@ -1,3 +1,4 @@ +#include <asm/asm.h> #include <asm/riscv_encoding.h> .section .text.header, "ax", %progbits @@ -6,8 +7,31 @@ ENTRY(start) /* Mask all interrupts */ csrw CSR_SIE, zero + /* Save HART ID and DTB base */ + lla a6, _bootcpu_id + REG_S a0, (a6) + lla a6, _dtb_base + REG_S a1, (a6) + la sp, cpu0_boot_stack li t0, STACK_SIZE add sp, sp, t0 + lla a6, _bootcpu_id + REG_L a0, (a6) + lla a6, _dtb_base + REG_L a1, (a6) + tail start_xen + + /* + * Boot cpu id is passed by a bootloader + */ +_bootcpu_id: + RISCV_PTR 0x0 + + /* + * DTB base is passed by a bootloader + */ +_dtb_base: + RISCV_PTR 0x0 diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 1c87899e8e..d9723fe1c0 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -7,7 +7,8 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE); -void __init noreturn start_xen(void) +void __init noreturn start_xen(unsigned long bootcpu_id, + unsigned long dtb_base) { early_printk("Hello from C env\n");
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes since v1: * read/save/pass of hart_id and dtb_base passed by a bootloader were moved to head.S. * Update start_xen() to recieve hard_id & dtb_base --- xen/arch/riscv/riscv64/head.S | 24 ++++++++++++++++++++++++ xen/arch/riscv/setup.c | 3 ++- 2 files changed, 26 insertions(+), 1 deletion(-)