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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 30/12/2022 1:01 pm, 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.
>
> Except introduction of new files the following changes were done:
> * Redefinition of ALIGN define from '.algin 2' to '.align 4' as

"align"

>   RISC-V implementations (except for with C extension) enforce 32-bit

While the C extension might mean things to RISC-V experts, it is better
to say the "compression" extension here so it's clear to non-RISC-V
experts reading the commit message too.

But, do we actually care about C?

ENTRY() needs to be 4 byte aligned because one of the few things it is
going to be used for is {m,s}tvec which requires 4-byte alignment even
with an IALIGN of 2.


I'd drop all talk about C and just say that 2 was an incorrect choice
previously.

>   instruction address alignment.  With C extension, 16-bit and 32-bit
>   are both allowed.
> * ALL_OBJ-y and ALL_LIBS-y were temporary overwritted to produce
>   a minimal hypervisor image otherwise it will be required to push
>   huge amount of headers and stubs for common, drivers, libs etc which
>   aren't necessary for now.
> * Section changed from .text to .text.header for start function
>   to make it the first one executed.
> * Rework riscv64/Makefile logic to rebase over changes since the first
>   RISC-V commit.
>
> RISC-V Xen can be built by the following instructions:
>   $ CONTAINER=riscv64 ./automation/scripts/containerize \
>        make XEN_TARGET_ARCH=riscv64 tiny64_defconfig

This needs a `-C xen` in this rune.

> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 942e4ffbc1..74386beb85 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,2 +1,18 @@
> +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 $< $(build_id_linker) -o $@
> +     $(NM) -pa --format=sysv $(@D)/$(@F) \
> +             | $(objtree)/tools/symbols --all-symbols --xensyms --sysv 
> --sort \
> +             >$(@D)/$(@F).map
> +
> +$(obj)/xen.lds: $(src)/xen.lds.S FORCE
> +     $(call if_changed_dep,cpp_lds_S)
> +
> +clean-files := $(objtree)/.xen-syms.[0-9]*

We don't need clean-files now that the main link has been simplified.

> diff --git a/xen/arch/riscv/include/asm/config.h 
> b/xen/arch/riscv/include/asm/config.h
> index e2ae21de61..e10e13ba53 100644
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -36,6 +39,10 @@
>    name:
>  #endif
>  
> +#define XEN_VIRT_START  _AT(UL,0x00200000)

Space after the comma.

Otherwise, LGTM.

~Andrew