>
> > +/*
> > + * PAGE_SIZE is defined in <{asm,xen}/page-*.h> but to
> > + * remove unnecessary headers for minimal
> > + * build headers it will be better to set PAGE_SIZE
> > + * explicitly.
>
> TBH, I think this is a shortcut that is unnecessary and will only
> introduce extra churn in the future (for instance, you will need to
> modify the include in xen.lds.S).
>
> But I am not the maintainer, so I will leave that decision to them
> whether this is acceptable.
I didn't get what you mean by a shortcut here.
config.h is automatically included everywhere. So anything defined in the header will also be available everywhere. Once you move the definition in a separate header, then you will have have to chase where the definition is used.
An alternative would be to include the new header in config.h. However, this is not always wanted (we are trying to limit the scope of some definitions).
So maybe you are saving a few minutes now. But you will spend a lot more to chase the places where the new header needs to be included.
>
> > + *
> > + * TODO: remove it when <asm/page-*.h> will be needed
> > + * defintion of PAGE_SIZE should be remove from
>
> s/defintion/definition/
Thanks for finding the typo. Will update it in the next version of
the patch.
>
> > + * this header.
> > + */
> > +#define PAGE_SIZE 4096 > +
> > #endif /* __RISCV_CONFIG_H__ */
> > /*
> > * Local variables:
> > diff --git a/xen/arch/riscv/include/asm/types.h
> > b/xen/arch/riscv/include/asm/types.h
> > new file mode 100644
> > index 0000000000..afbca6b15c
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/types.h
> > @@ -0,0 +1,11 @@
> > +#ifndef __TYPES_H__
> > +#define __TYPES_H__
> > +
> > +/*
> > + *
> > + * asm/types.h is required for xen-syms.S file which
> > + * is produced by tools/symbols.
> > + *
> > + */
> > +
> > +#endif
> > diff --git a/xen/arch/riscv/riscv64/Makefile
> > b/xen/arch/riscv/riscv64/Makefile
> > index 15a4a65f66..3340058c08 100644
> > --- a/xen/arch/riscv/riscv64/Makefile
> > +++ b/xen/arch/riscv/riscv64/Makefile
> > @@ -1 +1 @@
> > -extra-y += head.o
> > +obj-y += head.o
>
> This changes want to be explained. So does...
RISC-V 64 would be supported now and minimal build is built
now only obj-y stuff.
I am a bit confused. It thought what was checked in was meant to work. Did I misremembered?
>
> > diff --git a/xen/arch/riscv/riscv64/head.S
> > b/xen/arch/riscv/riscv64/head.S
> > index 0dbc27ba75..0330b29c01 100644
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -1,6 +1,6 @@
> > #include <asm/config.h>
> >
> > - .text
> > + .section .text.header, "ax", %progbits
>
> ... AFAICT this is to match the recent change in the build system.
I am not sure that I get you here but it was done to make 'start'
instructions to be the first instructions that will be executed and
to make 'start' symbol to be the first in xen-syms.map file otherwise
gdb shows that Xen starts from the wrong instructions, fails to find
correct source file and goes crazy.
When the file head.S was originally created, there was no section .text.header. Instead head.S was included at the top with some different runes.
>
> > + } :text
> > +
>
> I am assuming you are going to add .init.* afterwards?
>
> > + . = ALIGN(PAGE_SIZE);
> > + .bss : {
> > + __bss_start = .;
> > + *(.bss .bss.*)
> > + . = ALIGN(POINTER_ALIGN);
> > + __bss_end = .;
>
> Same as .data, I would recommend to properly populate it.
Do you mean to add .bss.stack_aligned, .bss.page_aligned, .bss.percpu*?
One of the reasons they were skipped is they aren't used now and one
more reason if to believe xen.lds.S file from ARM
.bss.percpu.read_mostly should be aligned by SMP_CACHE_BYTES which
requires dummy <asm/cache.h> (or not ?) which will increase the patch
with unneeded stuff for now.
I will answer your reply to Alistair here at the same time.
The problem is .bss.* will include any section start with .bss.. IOW section like .bss.page_aligned will also be covered and therefore you will not get a linker failure.
Instead , the linker will fold the section wherever it wants. Therefore, there is no guarantee, the content will be page aligned. When using the binary, you could end up with weird behavior that will be hard to debug.
I understand you are trying to get a basic build. But, I feel the linker is not something you want to quickly rush. 1h may turn into hours lost in a few months because not everyone may remember that you didn’t special case .bss.page_aligned.
Short of suggesting to have a common linker script, you could simply not use * (IOW explictly list the section). With that, you should get a linking warning/error if the section is not listed.
Cheers,
