From: Oleksii <oleksii.kurochko@gmail.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
xen-devel@lists.xenproject.org
Cc: Jan Beulich <jbeulich@suse.com>,
Stefano Stabellini <sstabellini@kernel.org>,
Gianluca Guida <gianluca@rivosinc.com>,
Bob Eshleman <bobbyeshleman@gmail.com>,
Alistair Francis <alistair.francis@wdc.com>,
Connor Davis <connojdavis@gmail.com>
Subject: Re: [PATCH v2 1/3] xen/riscv: read/save hart_id and dtb_base passed by bootloader
Date: Thu, 02 Mar 2023 16:53:49 +0200 [thread overview]
Message-ID: <f0d1e5b78482639a25f1ef0cf878309344383e7e.camel@gmail.com> (raw)
In-Reply-To: <881fd332-91c1-fea2-d1a2-3a5444a6f272@citrix.com>
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
next prev parent reply other threads:[~2023-03-02 14:54 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-02 13:23 [PATCH v2 0/3] Do basic initialization things Oleksii Kurochko
2023-03-02 13:23 ` [PATCH v2 1/3] xen/riscv: read/save hart_id and dtb_base passed by bootloader Oleksii Kurochko
2023-03-02 14:02 ` Andrew Cooper
2023-03-02 14:53 ` Oleksii [this message]
2023-03-02 18:58 ` Andrew Cooper
2023-03-03 8:48 ` Oleksii
2023-03-02 13:23 ` [PATCH v2 2/3] xen/riscv: initialize .bss section Oleksii Kurochko
2023-03-02 14:12 ` Andrew Cooper
2023-03-02 14:55 ` Oleksii
2023-03-02 14:22 ` Jan Beulich
2023-03-02 15:55 ` Oleksii
2023-03-02 16:01 ` Jan Beulich
2023-03-02 20:34 ` Andrew Cooper
2023-03-02 13:23 ` [PATCH v2 3/3] xen/riscv: disable fpu Oleksii Kurochko
2023-03-02 14:20 ` Andrew Cooper
2023-03-02 17:11 ` Oleksii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f0d1e5b78482639a25f1ef0cf878309344383e7e.camel@gmail.com \
--to=oleksii.kurochko@gmail.com \
--cc=alistair.francis@wdc.com \
--cc=andrew.cooper3@citrix.com \
--cc=bobbyeshleman@gmail.com \
--cc=connojdavis@gmail.com \
--cc=gianluca@rivosinc.com \
--cc=jbeulich@suse.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.