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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

> +     $(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.

> 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.

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.

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