Re: [XEN PATCH v1 1/4] arch/riscv: initial RISC-V support to build/run minimal Xen

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2022-12-28 at 22:22 +0000, Julien Grall wrote:
> 
> 
> On Mon, 26 Dec 2022 at 12:14, Oleksii <oleksii.kurochko@xxxxxxxxx>
> wrote:
> > > 
> > > > +/*
> > > > + * 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.
> 

Thanks for clarification.

> > 
> > > 
> > > > + *
> > > > + * 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?

The current mainline Xen can only build head.o directly using the
following command:
make XEN_TARGET_ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-
KBUILD_DEFCONFIG=tiny64_defconfig arch/riscv/riscv64/head.o

Comments can be found in the commit: 2a04f396a34c5a43b9a09d72e8c4f

> > 
> > 
> > > 
> > > > 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.
> 
Totally agree then.
I missed that there is .bss.*.
Actually I reworked a little bit xen.lds.S. As a basis I took xen.lds.S
from ARM and removed all arch specific sections. So xen.lds.S contains
stuff which isn't used for now (for example, *(.data.schedulers)) but
will be used in future.
Would it be better to go with the reworked linker script or remove
unneeded sections for now and make it get a linking warning/error when
sections will be used? 

> Cheers,

BR,
 Oleksii