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

[Alistair Francis <alistair23@gmail.com>]

On Mon, Dec 26, 2022 at 9:14 PM Oleksii <oleksii.kurochko@gmail.com> wrote:
>
> Hi Julien,
>
> Thanks for your comments.
>
> On Fri, 2022-12-23 at 13:50 +0000, Julien Grall wrote:
> > Hi Oleksii,
> >
> > + Anthony for the Makefile changes.
> >
> > On 23/12/2022 11:16, Oleksii Kurochko wrote:
> > > The patch provides a minimal amount of changes to start
> > > build and run minimal Xen binary at GitLab CI&CD that will
> > > allow continuous checking of the build status of RISC-V Xen.
> > >
> > > RISC-V Xen can be built by the following instructions:
> > >    $ CONTAINER=riscv64 ./automation/scripts/containerize \
> > >         make XEN_TARGET_ARCH=riscv64 tiny64_defconfig
> > >    $ CONTAINER=riscv64 ./automation/scripts/containerize \
> > >         make XEN_TARGET_ARCH=riscv64 -C xen build
> > >
> > > RISC-V Xen can be run as:
> > >    $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \
> > >         -kernel xen/xen
> > >
> > > To run in debug mode should be done the following instructions:
> > >   $ qemu-system-riscv64 -M virt -smp 1 -nographic -m 2g \
> > >          -kernel xen/xen -s -S
> > >   # In separate terminal:
> > >   $ riscv64-buildroot-linux-gnu-gdb
> > >   $ target remote :1234
> > >   $ add-symbol-file <xen_src>/xen/xen-syms 0x80200000
> > >   $ hb *0x80200000
> > >   $ c # it should stop at instruction j 0x80200000 <start>
> > >
> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > > ---
> > >   xen/arch/riscv/Makefile             | 30 +++++++++++++
> > >   xen/arch/riscv/arch.mk              | 10 +++++
> > >   xen/arch/riscv/include/asm/config.h | 26 ++++++++++-
> > >   xen/arch/riscv/include/asm/types.h  | 11 +++++
> > >   xen/arch/riscv/riscv64/Makefile     |  2 +-
> > >   xen/arch/riscv/riscv64/head.S       |  2 +-
> > >   xen/arch/riscv/xen.lds.S            | 69
> > > +++++++++++++++++++++++++++++
> > >   7 files changed, 147 insertions(+), 3 deletions(-)
> > >   create mode 100644 xen/arch/riscv/include/asm/types.h
> > >   create mode 100644 xen/arch/riscv/xen.lds.S
> > >
> > > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > > index 942e4ffbc1..893dd19ea6 100644
> > > --- a/xen/arch/riscv/Makefile
> > > +++ b/xen/arch/riscv/Makefile
> > > @@ -1,2 +1,32 @@
> > > +obj-$(CONFIG_RISCV_64) += riscv64/
> > > +
> > > +$(TARGET): $(TARGET)-syms
> > > +       $(OBJCOPY) -O binary -S $< $@
> > > +
> > > +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
> > > +       $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
> > > +               $(SYMBOLS_DUMMY_OBJ) -o $(@D)/.$(@F).0
> > > +       $(NM) -pa --format=sysv $(@D)/.$(@F).0 \
> > > +               | $(objtree)/tools/symbols $(all_symbols) --sysv --
> > > sort >$(@D)/.$(@F).0.S
> > > +       $(MAKE) $(build)=$(@D) $(@D)/.$(@F).0.o
> > > +       $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
> > > +               $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
> > > +       $(NM) -pa --format=sysv $(@D)/.$(@F).1 \
> > > +               | $(objtree)/tools/symbols $(all_symbols) --sysv --
> > > sort >$(@D)/.$(@F).1.S
> > > +       $(MAKE) $(build)=$(@D) $(@D)/.$(@F).1.o
> > > +       $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $<
> > > $(build_id_linker) \
> > > +               $(@D)/.$(@F).1.o -o $@
> > > +       $(NM) -pa --format=sysv $(@D)/$(@F) \
> > > +               | $(objtree)/tools/symbols --all-symbols --xensyms
> > > --sysv --sort \
> > > +               >$(@D)/$(@F).map
> > > +       rm -f $(@D)/.$(@F).[0-9]*
> > > +
> > > +$(obj)/xen.lds: $(src)/xen.lds.S FORCE
> > > +               $(call if_changed_dep,cpp_lds_S)
> > > +
> > > +.PHONY: clean
> > > +clean::
> > > +       rm -f $(objtree)/.xen-syms.[0-9]*
> >
> > Any reason to not use the variable clean-files as this is done for
> > the
> > other architectures?
>
> There is no reason not use the variable clean-files. I missed the
> vairable clean-files so the patch will be updated to use the
> variable clean-files instead of the variable clean.
>
> >
> > > +
> > >   .PHONY: include
> > >   include:
> > > diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
> > > index ae8fe9dec7..9292b72718 100644
> > > --- a/xen/arch/riscv/arch.mk
> > > +++ b/xen/arch/riscv/arch.mk
> > > @@ -11,3 +11,13 @@ riscv-march-$(CONFIG_RISCV_ISA_C)       :=
> > > $(riscv-march-y)c
> > >   # -mcmodel=medlow would force Xen into the lower half.
> > >
> > >   CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany
> > > +
> > > +# TODO: Drop override and SYMBOLS_DUMMY_OBJ when more
> > > +# of the build is working
> > > +override ALL_OBJS-y = arch/$(TARGET_ARCH)/built_in.o
> > > +override ALL_LIBS-y =
> > > +ifneq ($(wildcard $(objtree)/common/symbols-dummy.o),)
> > > +SYMBOLS_DUMMY_OBJ=$(objtree)/common/symbols-dummy.o
> > > +else
> > > +SYMBOLS_DUMMY_OBJ=
> > > +endif
> >
> > Why do you need the ifneq here?
>
> The only reason for the ifneq here is to skip common
> stuff from the build of minimal RISC-V Xen binary as it
> requires pushing from the start a big amount of headers and function
> stubs which at least will lead to a huge patch and complicate a code
> review.
>
> It is possible to remove <common/symbols-dummy.o> from xen-syms
> target in <xen/arch/riscv/Makefile> but an intention here was
> to remain processing of xen-syms target similar to the original
> one and it is the second reason why the ifneq was introduced and
> added the comment "TODO: Drop ... SYMBOLS_DUMMY_OBJ when more
> of the build is working".
>
> >
> > > diff --git a/xen/arch/riscv/include/asm/config.h
> > > b/xen/arch/riscv/include/asm/config.h
> > > index e2ae21de61..756607a4a2 100644
> > > --- a/xen/arch/riscv/include/asm/config.h
> > > +++ b/xen/arch/riscv/include/asm/config.h
> > > @@ -28,7 +28,7 @@
> > >
> > >   /* Linkage for RISCV */
> > >   #ifdef __ASSEMBLY__
> > > -#define ALIGN .align 2
> > > +#define ALIGN .align 4
> >
> > Can you explain in the commit message why you need to change this?
> > But
> > ideally, this change should be part of a separate one.
>
> ALIGN was changed to equal the implementation-enforced instruction
> address alignment (4-bytes), in order to ensure that entry points are
> properly aligned.
> If to be honest I haven't verified that and took these changes from
> the initial patch series pushed by Bobby Eshleman.
>
> >
> > >
> > >   #define ENTRY(name)                                \
> > >     .globl name;                                     \
> > > @@ -36,6 +36,30 @@
> > >     name:
> > >   #endif
> > >
> > > +/*
> > > + * Definition of XEN_VIRT_START should look like:
> > > + *   define XEN_VIRT_START _AT(vaddr_t,0x00200000)
> > > + * It requires including of additional headers which
> > > + * will increase an amount of files unnecessary for
> > > + * minimal RISC-V Xen build so set value of
> > > + * XEN_VIRT_START explicitly.
> > > + *
> > > + * TODO: change it to _AT(vaddr_t,0x00200000) when
> > > + *       necessary header will be pushed.
> >
> > The address here doesn't match the one below. I know this is an
> > example
> > but this is fairly confusing.
>
> This was done only as an example.
>
> >
> > > + */
> > > +#define XEN_VIRT_START  0x80200000
> >
> > I think you at least want to use _AT(unsigned long, ...) here to make
> > clear this value should be interpreted as an unsigned value.
> >
> > You could even define vaddr_t now as you introduce a dummy types.h
> > below.
>
> It makes sense to push the definition of vaddr_t and use <xen/const.h>
> to be able to use _AT() macros.
>
> Probably it would be nice to introduce others from types.h from the
> start, wouldn't it? Or would it be better to leave the patch minimal
> and introduce only types necessary for vaddr_t?

It would be best to add types.h early. I don't really see a reason not to

>
> >
> > > +/*
> > > + * 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.
>
> The idea was to introduce PAGE_SIZE in the config.h directly for now
> to keep build of RISC-V Xen minimal as much as possible otherwise
> it would be required to push dummy <asm/page-bits.h> to get only one
> needed definition of PAGE_SIZE to have buildable Xen.
> Thereby it was decided to define PAGE_SIZE directly in <asm/config.h>
> and remove it after all definitions from <{asm,xen}/page-*.h> will be
> needed.
>
> >
> > > + *
> > > + * 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.
>
> >
> > > 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.
>
> >
> > >
> > >   ENTRY(start)
> > >           j  start
> > > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
> > > new file mode 100644
> > > index 0000000000..60628b3856
> > > --- /dev/null
> > > +++ b/xen/arch/riscv/xen.lds.S
> > > @@ -0,0 +1,69 @@
> > > +#include <xen/xen.lds.h>
> > > +
> > > +#undef ENTRY
> > > +#undef ALIGN
> > > +
> > > +OUTPUT_ARCH(riscv)
> > > +ENTRY(start)
> > > +
> > > +PHDRS
> > > +{
> > > +  text PT_LOAD ;
> > > +#if defined(BUILD_ID)
> > > +  note PT_NOTE ;
> > > +#endif
> > > +}
> > > +
> > > +SECTIONS
> > > +{
> > > +  . = XEN_VIRT_START;
> > > +  _start = .;
> > > +  .text : {
> > > +        _stext = .;
> > > +       *(.text.header)
> > > +       *(.text)
> >
> > You are missing some sections here such as .text.cold,
> > .text.unlikely...
> >
> > I understand they are probably not used yet. But it will avoid any
> > nasty
> > surprise if they are forgotten.
>
> They were skipped because they aren't use for now. Will add it in
> the next version of the patch.
>
> >
> > > +       *(.gnu.warning)
> > > +       . = ALIGN(POINTER_ALIGN);
> > > +       _etext = .;
> > > +  } :text
> > > +
> > > +    . = ALIGN(PAGE_SIZE);
> > > +  .rodata : {
> > > +        _srodata = .;
> >
> > You introduce _srodata but I can't find the matching _erodata.
>
> My fault. Thanks. Will add it in the the next version of the patch.
>
> >
> > > +       *(.rodata)
> > > +       *(.rodata.*)
> > > +       *(.data.rel.ro)
> > > +       *(.data.rel.ro.*)
> > > +  } :text
> > > +
> > > +#if defined(BUILD_ID)
> > > +  . = ALIGN(4);
> > > +  .note.gnu.build-id : {
> > > +       __note_gnu_build_id_start = .;
> > > +       *(.note.gnu.build-id)
> > > +       __note_gnu_build_id_end = .;
> > > +  } :note :text
> > > +#endif
> > > +
> > > +  . = ALIGN(PAGE_SIZE);
> > > +  .data : { /* Data */
> > > +       *(.data .data.*)
> >
> > It would be better if you introduce .data.read_mostly right now
> > separately.
> >
> > You also want *.data.page_aligned in .data.
> >
> > Lastly you are missing CONSTRUCTORS
>
> I will add offred sections and CONSTUCTORS in the next version of
> the patch
>
> >
> > > +  } :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 think we should aim to get the linker file sorted right from the
start. Otherwise someone is going to hit a nasty bug at some point in
the future.

Alistair

>
> >
> > Cheers,
> >
>
> Best regards,
>  Oleksii
>