| Message ID | 1941d4ed64ff6edcc6354d09d6a40db4d6f63c44.1678976127.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
On 16.03.2023 15:39, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/setup.c > +++ b/xen/arch/riscv/setup.c > @@ -1,12 +1,16 @@ > #include <xen/compile.h> > #include <xen/init.h> > +#include <xen/kernel.h> > > +#include <asm/boot-info.h> > #include <asm/early_printk.h> > > /* Xen stack for bringing up the first CPU. */ > unsigned char __initdata cpu0_boot_stack[STACK_SIZE] > __aligned(STACK_SIZE); > > +struct boot_info boot_info = { (unsigned long)&_start, (unsigned long)&_end, 0UL, 0UL }; You add no declaration in a header, in which case this wants to be static. It may also want to be __initdata, depending on further planned uses. I would also like to suggest using C99 dedicated initializers and in any event omit the 0UL initializer values (as that's what the compiler will use anyway). Yet even then I think the line is still too long and hence needs wrapping. > @@ -15,11 +19,19 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE] > */ > int dummy_bss __attribute__((unused)); > > +static void fill_boot_info(void) __init? > +{ > + boot_info.load_start = (unsigned long)_start; > + boot_info.load_end = (unsigned long)_end; > +} I'd like to understand how this is intended to work: _start and _end are known at compile time, and their value won't change (unless you process relocations alongside switching from 1:1 mapping to some other virtual memory layout). Therefore - why can't these be put in the initializer as well? Guessing that the values derived here are different because of being calculated in a PC-relative way, I think this really needs a comment. If, of course, this can be relied upon in the first place. Also please be consistent about the use of unary & when taking the address of arrays (or functions). Personally I prefer the & to be omitted in such cases, but what I consider relevant is that there be no mix (in new code at least). Jan
On Tue, 2023-03-21 at 12:27 +0100, Jan Beulich wrote: > On 16.03.2023 15:39, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/setup.c > > +++ b/xen/arch/riscv/setup.c > > @@ -1,12 +1,16 @@ > > #include <xen/compile.h> > > #include <xen/init.h> > > +#include <xen/kernel.h> > > > > +#include <asm/boot-info.h> > > #include <asm/early_printk.h> > > > > /* Xen stack for bringing up the first CPU. */ > > unsigned char __initdata cpu0_boot_stack[STACK_SIZE] > > __aligned(STACK_SIZE); > > > > +struct boot_info boot_info = { (unsigned long)&_start, (unsigned > > long)&_end, 0UL, 0UL }; > > You add no declaration in a header, in which case this wants to be > static. Sure. > It may also want to be __initdata, depending on further planned uses. I am going to use it only for initial page table initialization. At least for now. > I > would also like to suggest using C99 dedicated initializers and in > any > event omit the 0UL initializer values (as that's what the compiler > will > use anyway). Yet even then I think the line is still too long and > hence > needs wrapping. > > > @@ -15,11 +19,19 @@ unsigned char __initdata > > cpu0_boot_stack[STACK_SIZE] > > */ > > int dummy_bss __attribute__((unused)); > > > > +static void fill_boot_info(void) > > __init? Yes, should it be __init. > > > +{ > > + boot_info.load_start = (unsigned long)_start; > > + boot_info.load_end = (unsigned long)_end; > > +} > > I'd like to understand how this is intended to work: _start and _end > are known at compile time, and their value won't change (unless you > process relocations alongside switching from 1:1 mapping to some > other virtual memory layout). Therefore - why can't these be put in > the initializer as well? Guessing that the values derived here are > different because of being calculated in a PC-relative way, I think > this really needs a comment. If, of course, this can be relied upon > in the first place. Your guessing is correct. In this case addresses of _start and _end will be calculated in a PC-relative way. So I'll add a comment. > > Also please be consistent about the use of unary & when taking the > address of arrays (or functions). Personally I prefer the & to be > omitted in such cases, but what I consider relevant is that there > be no mix (in new code at least). Sure. Thanks for the comments. I'll fix that in the new version of the patch. ~ Oleksii
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 0908bdb9f9..36556eb779 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -1,12 +1,16 @@ #include <xen/compile.h> #include <xen/init.h> +#include <xen/kernel.h> +#include <asm/boot-info.h> #include <asm/early_printk.h> /* Xen stack for bringing up the first CPU. */ unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE); +struct boot_info boot_info = { (unsigned long)&_start, (unsigned long)&_end, 0UL, 0UL }; + /* * To be sure that .bss isn't zero. It will simplify code of * .bss initialization. @@ -15,11 +19,19 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE] */ int dummy_bss __attribute__((unused)); +static void fill_boot_info(void) +{ + boot_info.load_start = (unsigned long)_start; + boot_info.load_end = (unsigned long)_end; +} + void __init noreturn start_xen(unsigned long bootcpu_id, unsigned long dtb_paddr) { early_printk("Hello from C env\n"); + fill_boot_info(); + early_printk("All set up\n"); for ( ;; ) asm volatile ("wfi");
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V5: * the patch was introduced in the current patch series (V5) --- xen/arch/riscv/setup.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)