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 18:56 +0000, Andrew Cooper wrote:
> On 23/12/2022 11:16 am, Oleksii Kurochko wrote:
> > 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 $@
> 
> The conditional emptying of SYMBOLS_DUMMY_OBJ in arch.mk will break
> this
> logic if it actually gets emptied, but you can drop almost all of it.
> 
> This should be just
> 
> $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
>     $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) -
> o $@
> 
> in the short term, I think.
> 
Originally it was made in the same way but I decided to create
addiotional variable SYMBOLS_DUMMY_OBJ.
I will back the previous solution.
> > +       $(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)
> 
> You've got a tab and some spaces here.  It wants to be just one tab.
> 
Thanks. Will re-check.

> > 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
> > @@ -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.
> > + */
> > +#define XEN_VIRT_START  0x80200000
> > +/*
> > + * 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.
> > + *
> > + * TODO: remove it when <asm/page-*.h> will be needed
> > + *       defintion of PAGE_SIZE should be remove from
> > + *       this header.
> > + */
> > +#define PAGE_SIZE       4096
> 
> I've committed Alistair's patch which adds page-bits.h, so this
> section
> can go away.
> 
Nice. Thanks a lot.
> 
> We still need XEN_VIRT_START, but we don't really need the commentary
> explaining that it's temporary - that is very clear from the subject
> of
> the patch.
> 
> > 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.
> > + *
> > + */
> 
> Again, no need for this comment.
> 
> However, I think we want to rearrange the build system to have a
> final
> -I option for xen/include/stub/asm/ or so, so we can put some empty
> files there and avoid having architectures needing to provide empty
> files.
> 
Agree.
And do you expect to see these changes as a part of this patch series
or it someting that should be done in future?

> If this file is needed, then it needs a more specific header guard
> than
> __TYPES_H__.  That's the general guard for xen/types.h meaning that
> nothing inside this file will actually survive preprocessing.
> 
> ~Andrew