[XEN PATCH v1 0/4] Add minimal RISC-V Xen build and build testing

[XEN PATCH v1 0/4] Add minimal RISC-V Xen build and build testing

[Oleksii Kurochko]

The patch series introduces the following:
- provide a minimal amount of changes to add initial RISC-V support
  to make Xen binary buildable and runnable for RISC-V architecture
   which can be used for future development and testing.
- add RISC-V 64 cross-compile build jobs to check if any new changes
  break RISC-V build.
- minor fixes to make automation build script work with cross-compilers.

Oleksii Kurochko (4):
  arch/riscv: initial RISC-V support to build/run minimal Xen
  automation: add cross-compiler support for the build script
  automation: add python3 package for riscv64.dockerfile
  automation: add RISC-V 64 cross-build tests for Xen

 automation/build/archlinux/riscv64.dockerfile |  3 +-
 automation/gitlab-ci/build.yaml               | 43 ++++++++++++
 automation/scripts/build                      | 10 +--
 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 +++++++++++++++++++
 10 files changed, 197 insertions(+), 9 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/types.h
 create mode 100644 xen/arch/riscv/xen.lds.S

-- 
2.38.1

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

[Oleksii Kurochko]

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]*
+
 .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
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
 
 #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.
+ */
+#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
+
 #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
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
 
 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)
+       *(.gnu.warning)
+       . = ALIGN(POINTER_ALIGN);
+       _etext = .;
+  } :text
+
+    . = ALIGN(PAGE_SIZE);
+  .rodata : {
+        _srodata = .;
+       *(.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.*)
+  } :text
+
+  . = ALIGN(PAGE_SIZE);
+  .bss : {
+       __bss_start = .;
+       *(.bss .bss.*)
+       . = ALIGN(POINTER_ALIGN);
+       __bss_end = .;
+  } :text
+  _end = . ;
+
+  DWARF2_DEBUG_SECTIONS
+
+  DISCARD_SECTIONS
+
+  STABS_DEBUG_SECTIONS
+
+  ELF_DETAILS_SECTIONS
+}
-- 
2.38.1

[XEN PATCH v1 2/4] automation: add cross-compiler support for the build script

[Oleksii Kurochko]

Docker images that are currently available and used for
cross-compilation is additionally installed GCC/Binutils
which is why the build script doesn't crash.

RISC-V docker image doesn't have native GCC only          
cross-compiler which will lead to the fact  that the build
script will fail.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 automation/scripts/build | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/automation/scripts/build b/automation/scripts/build
index a593419063..026f480e76 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -2,12 +2,12 @@
 
 test -f /etc/os-release && cat "$_"
 
-$CC --version
+${CROSS_COMPILE}${CC} --version
 
 # Express the compiler version as an integer.  e.g. GCC 4.9.2 => 0x040902
 cc-ver()
 {
-    $CC -dumpversion | awk -F. '{ printf "0x%02x%02x%02x", $1, $2, $3 }'
+    ${CROSS_COMPILE}${CC} -dumpversion | awk -F. '{ printf "0x%02x%02x%02x", $1, $2, $3 }'
 }
 
 # random config or default config
@@ -66,7 +66,7 @@ if ! type python3 || python3 -c "import sys; res = sys.version_info < (3, 5); ex
 fi
 
 # SeaBIOS requires GCC 4.6 or later
-if [[ "${CC}" == "gcc" && `cc-ver` -lt 0x040600 ]]; then
+if [[ "${CROSS_COMPILE}${CC}" == "gcc" && `cc-ver` -lt 0x040600 ]]; then
     cfgargs+=("--with-system-seabios=/bin/false")
 fi
 
-- 
2.38.1

[XEN PATCH v1 3/4] automation: add python3 package for riscv64.dockerfile

[Oleksii Kurochko]

Pyhton3 package is requited by automation/scripts/build
script so it shoud be installed to riscv64 docker image

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 automation/build/archlinux/riscv64.dockerfile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/automation/build/archlinux/riscv64.dockerfile b/automation/build/archlinux/riscv64.dockerfile
index ff8b2b955d..a52852c2f7 100644
--- a/automation/build/archlinux/riscv64.dockerfile
+++ b/automation/build/archlinux/riscv64.dockerfile
@@ -9,7 +9,8 @@ RUN pacman --noconfirm --needed -Syu \
     inetutils \
     riscv64-linux-gnu-binutils \
     riscv64-linux-gnu-gcc \
-    riscv64-linux-gnu-glibc
+    riscv64-linux-gnu-glibc \
+    python3
 
 # Add compiler path
 ENV CROSS_COMPILE=riscv64-linux-gnu-
-- 
2.38.1

[XEN PATCH v1 4/4] automation: add RISC-V 64 cross-build tests for Xen

[Oleksii Kurochko]

Add build jobs to cross-compile Xen-only for RISC-V 64.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 automation/gitlab-ci/build.yaml | 43 +++++++++++++++++++++++++++++++++
 automation/scripts/build        |  4 +--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 93d9ff69a9..d97b2aa788 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -172,6 +172,33 @@
   variables:
     <<: *gcc
 
+.riscv64-cross-build-tmpl:
+  <<: *build
+  variables:
+    XEN_TARGET_ARCH: riscv64
+  tags:
+    - x86_64
+
+.riscv64-cross-build:
+  extends: .riscv64-cross-build-tmpl
+  variables:
+    debug: n
+
+.riscv64-cross-build-debug:
+  extends: .riscv64-cross-build-tmpl
+  variables:
+    debug: y
+
+.gcc-riscv64-cross-build:
+  extends: .riscv64-cross-build
+  variables:
+    <<: *gcc
+
+.gcc-riscv64-cross-build-debug:
+  extends: .riscv64-cross-build-debug
+  variables:
+    <<: *gcc
+
 # Jobs below this line
 
 archlinux-gcc:
@@ -615,6 +642,19 @@ alpine-3.12-gcc-debug-arm64-boot-cpupools:
     EXTRA_XEN_CONFIG: |
       CONFIG_BOOT_TIME_CPUPOOLS=y
 
+# RISC-V 64 cross-build
+riscv64-cross-gcc:
+  extends: .gcc-riscv64-cross-build
+  variables:
+    CONTAINER: archlinux:riscv64
+    KBUILD_DEFCONFIG: tiny64_defconfig
+
+riscv64-cross-gcc-debug:
+  extends: .gcc-riscv64-cross-build-debug
+  variables:
+    CONTAINER: archlinux:riscv64
+    KBUILD_DEFCONFIG: tiny64_defconfig
+
 ## Test artifacts common
 
 .test-jobs-artifact-common:
@@ -690,3 +730,6 @@ kernel-5.10.74-export:
       - binaries/bzImage
   tags:
     - x86_64
+
+# # RISC-V 64 test artificats
+# # TODO: add RISC-V 64 test artitifacts
diff --git a/automation/scripts/build b/automation/scripts/build
index 026f480e76..d0632a2bd5 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -34,8 +34,8 @@ fi
 # to exit early -- bash is invoked with -e.
 cp xen/.config xen-config
 
-# arm32 only cross-compiles the hypervisor
-if [[ "${XEN_TARGET_ARCH}" = "arm32" ]]; then
+# arm32 & riscv64 only cross-compiles the hypervisor
+if [[ "${XEN_TARGET_ARCH}" = "arm32" ]] || [[ "${XEN_TARGET_ARCH}" = "riscv64" ]]; then
     hypervisor_only="y"
 fi
 
-- 
2.38.1

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

[Julien Grall]

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?

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

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

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

> + */
> +#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.

> +/*
> + * 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.

> + *
> + * TODO: remove it when <asm/page-*.h> will be needed
> + *       defintion of PAGE_SIZE should be remove from

s/defintion/definition/

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

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

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

> +       *(.gnu.warning)
> +       . = ALIGN(POINTER_ALIGN);
> +       _etext = .;
> +  } :text
> +
> +    . = ALIGN(PAGE_SIZE);
> +  .rodata : {
> +        _srodata = .;

You introduce _srodata but I can't find the matching _erodata.

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

> +  } :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.

Cheers,

-- 
Julien Grall

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

[Oleksii]

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?

> 
> > +/*
> > + * 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. 

> 
> Cheers,
> 

Best regards,
 Oleksii

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

[Alistair Francis]

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
>

Re: [XEN PATCH v1 4/4] automation: add RISC-V 64 cross-build tests for Xen

[Alistair Francis]

On Fri, Dec 23, 2022 at 9:17 PM Oleksii Kurochko
<oleksii.kurochko@gmail.com> wrote:
>
> Add build jobs to cross-compile Xen-only for RISC-V 64.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  automation/gitlab-ci/build.yaml | 43 +++++++++++++++++++++++++++++++++
>  automation/scripts/build        |  4 +--
>  2 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
> index 93d9ff69a9..d97b2aa788 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -172,6 +172,33 @@
>    variables:
>      <<: *gcc
>
> +.riscv64-cross-build-tmpl:
> +  <<: *build
> +  variables:
> +    XEN_TARGET_ARCH: riscv64
> +  tags:
> +    - x86_64
> +
> +.riscv64-cross-build:
> +  extends: .riscv64-cross-build-tmpl
> +  variables:
> +    debug: n
> +
> +.riscv64-cross-build-debug:
> +  extends: .riscv64-cross-build-tmpl
> +  variables:
> +    debug: y
> +
> +.gcc-riscv64-cross-build:
> +  extends: .riscv64-cross-build
> +  variables:
> +    <<: *gcc
> +
> +.gcc-riscv64-cross-build-debug:
> +  extends: .riscv64-cross-build-debug
> +  variables:
> +    <<: *gcc
> +
>  # Jobs below this line
>
>  archlinux-gcc:
> @@ -615,6 +642,19 @@ alpine-3.12-gcc-debug-arm64-boot-cpupools:
>      EXTRA_XEN_CONFIG: |
>        CONFIG_BOOT_TIME_CPUPOOLS=y
>
> +# RISC-V 64 cross-build
> +riscv64-cross-gcc:
> +  extends: .gcc-riscv64-cross-build
> +  variables:
> +    CONTAINER: archlinux:riscv64
> +    KBUILD_DEFCONFIG: tiny64_defconfig
> +
> +riscv64-cross-gcc-debug:
> +  extends: .gcc-riscv64-cross-build-debug
> +  variables:
> +    CONTAINER: archlinux:riscv64
> +    KBUILD_DEFCONFIG: tiny64_defconfig
> +
>  ## Test artifacts common
>
>  .test-jobs-artifact-common:
> @@ -690,3 +730,6 @@ kernel-5.10.74-export:
>        - binaries/bzImage
>    tags:
>      - x86_64
> +
> +# # RISC-V 64 test artificats
> +# # TODO: add RISC-V 64 test artitifacts
> diff --git a/automation/scripts/build b/automation/scripts/build
> index 026f480e76..d0632a2bd5 100755
> --- a/automation/scripts/build
> +++ b/automation/scripts/build
> @@ -34,8 +34,8 @@ fi
>  # to exit early -- bash is invoked with -e.
>  cp xen/.config xen-config
>
> -# arm32 only cross-compiles the hypervisor
> -if [[ "${XEN_TARGET_ARCH}" = "arm32" ]]; then
> +# arm32 & riscv64 only cross-compiles the hypervisor
> +if [[ "${XEN_TARGET_ARCH}" = "arm32" ]] || [[ "${XEN_TARGET_ARCH}" = "riscv64" ]]; then
>      hypervisor_only="y"
>  fi
>
> --
> 2.38.1
>
>

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

[Oleksii]

Alistair,

Thanks for your comments!

On Wed, 2022-12-28 at 14:51 +1000, Alistair Francis wrote:
> 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.

Probably I am not correct here but if I understand correcly the
sections that are mentioned in riscv/xen.lds.S now they are "default"
sections.

All other sections such as *(.bss.percpu.*) *(.bss.*algined) etc are
sections defined by user using __section directive or .section.
Thereby it will be hard to get a nasty bug because if a section is
needed and isn't defined then linker will produce an error about unkown
section.

Am i wrong?

Best regards,
 Oleksii

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

[Andrew Cooper]

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

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

[Andrew Cooper]

On 28/12/2022 4:51 am, Alistair Francis wrote:
> On Mon, Dec 26, 2022 at 9:14 PM Oleksii <oleksii.kurochko@gmail.com> wrote:
>> On Fri, 2022-12-23 at 13:50 +0000, Julien Grall wrote:
>>> On 23/12/2022 11:16, Oleksii Kurochko wrote:
>>>> +  . = 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.

What needs to happen is actually for Xen to start using a common linker
script, rather than a per-arch linker script.

The vast majority of the linker script is not architecture specific to
begin with, and the rest is easy to parametrise.

But in the short term, it's more important to get something working and
properly into CI, rather than to block this change waiting for feature
parity with a whole load of features not currently used.

~Andrew

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

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 5255 bytes --]

On Mon, 26 Dec 2022 at 12:14, Oleksii <oleksii.kurochko@gmail.com> 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.


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

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

Cheers,
-- 
Julien Grall

[-- Attachment #2: Type: text/html, Size: 7921 bytes --]

Re: [XEN PATCH v1 2/4] automation: add cross-compiler support for the build script

[Andrew Cooper]

On 23/12/2022 11:16 am, Oleksii Kurochko wrote:
> diff --git a/automation/scripts/build b/automation/scripts/build
> index a593419063..026f480e76 100755
> --- a/automation/scripts/build
> +++ b/automation/scripts/build
> @@ -2,12 +2,12 @@
>  
>  test -f /etc/os-release && cat "$_"
>  
> -$CC --version
> +${CROSS_COMPILE}${CC} --version
>  
>  # Express the compiler version as an integer.  e.g. GCC 4.9.2 => 0x040902
>  cc-ver()
>  {
> -    $CC -dumpversion | awk -F. '{ printf "0x%02x%02x%02x", $1, $2, $3 }'
> +    ${CROSS_COMPILE}${CC} -dumpversion | awk -F. '{ printf "0x%02x%02x%02x", $1, $2, $3 }'
>  }
>  
>  # random config or default config
> @@ -66,7 +66,7 @@ if ! type python3 || python3 -c "import sys; res = sys.version_info < (3, 5); ex
>  fi
>  
>  # SeaBIOS requires GCC 4.6 or later
> -if [[ "${CC}" == "gcc" && `cc-ver` -lt 0x040600 ]]; then
> +if [[ "${CROSS_COMPILE}${CC}" == "gcc" && `cc-ver` -lt 0x040600 ]]; then

This change won't work, because it's no longer a plain "gcc".

Also, prepreding CROSS_COMPILE isn't compatible with LLVM toolchains,
but that's not something you've made any worse with your change (just
more obvious).

We probably want a stanza near the top which sets
CC="${CROSS_COMPILE}${CC}" which can be adjusted to support LLVM in due
course, and we'll need to detect GCC using --version | grep as its done
elsewhere.

But the other logic wants reworking too so we don't play around with
bits of the tools build when we're doing a hypervisor-only build anyway...

~Andrew

Re: [XEN PATCH v1 3/4] automation: add python3 package for riscv64.dockerfile

[Andrew Cooper]

On 23/12/2022 11:16 am, Oleksii Kurochko wrote:
> Pyhton3 package is requited by automation/scripts/build
> script so it shoud be installed to riscv64 docker image

Is it?  This series runs fine without it.

When we get around to compiling userspace, then we do need a python
interpreter for the build, but we need a load of other things too.

~Andrew

Re: [XEN PATCH v1 3/4] automation: add python3 package for riscv64.dockerfile

[Oleksii]

On Wed, 2022-12-28 at 23:26 +0000, Andrew Cooper wrote:
> On 23/12/2022 11:16 am, Oleksii Kurochko wrote:
> > Pyhton3 package is requited by automation/scripts/build
> > script so it shoud be installed to riscv64 docker image
> 
> Is it?  This series runs fine without it.
> 

It is used by automation/scripts/build here:
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/automation/scripts/build#L63

It looks like this patch can be skipped for now as it doesn't affect
results of RISC-V cross-build test.

> When we get around to compiling userspace, then we do need a python
> interpreter for the build, but we need a load of other things too.
> 
> ~Andrew

~Oleksii

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

[Oleksii]

On Wed, 2022-12-28 at 19:01 +0000, Andrew Cooper wrote:
> On 28/12/2022 4:51 am, Alistair Francis wrote:
> > On Mon, Dec 26, 2022 at 9:14 PM Oleksii
> > <oleksii.kurochko@gmail.com> wrote:
> > > On Fri, 2022-12-23 at 13:50 +0000, Julien Grall wrote:
> > > > On 23/12/2022 11:16, Oleksii Kurochko wrote:
> > > > > +  . = 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.
> 
> What needs to happen is actually for Xen to start using a common
> linker
> script, rather than a per-arch linker script.
> 

Do you expect to see common linker script as a part of this patch
series?

> The vast majority of the linker script is not architecture specific
> to
> begin with, and the rest is easy to parametrise.
> 

I reworked xen.lds.S file.

I took xen.lds.S from ARM as a basis and
remove all arch specific sections such as *(.proc.info), *(.ex_table),
*(.ex_table.pre), *(.altinstructions), *(.teemediator.info),
*(.adev.info), *(.dev.info), *(.arch.info), *(.bug_frames.*).

So it would be possible to use xen.lds.S as a common linker script.

> But in the short term, it's more important to get something working
> and
> properly into CI, rather than to block this change waiting for
> feature
> parity with a whole load of features not currently used.
>
> ~Andrew

~Oleksii

Re: [XEN PATCH v1 2/4] automation: add cross-compiler support for the build script

[Oleksii]

On Wed, 2022-12-28 at 23:24 +0000, Andrew Cooper wrote:
> On 23/12/2022 11:16 am, Oleksii Kurochko wrote:
> > diff --git a/automation/scripts/build b/automation/scripts/build
> > index a593419063..026f480e76 100755
> > --- a/automation/scripts/build
> > +++ b/automation/scripts/build
> > @@ -2,12 +2,12 @@
> >  
> >  test -f /etc/os-release && cat "$_"
> >  
> > -$CC --version
> > +${CROSS_COMPILE}${CC} --version
> >  
> >  # Express the compiler version as an integer.  e.g. GCC 4.9.2 =>
> > 0x040902
> >  cc-ver()
> >  {
> > -    $CC -dumpversion | awk -F. '{ printf "0x%02x%02x%02x", $1, $2,
> > $3 }'
> > +    ${CROSS_COMPILE}${CC} -dumpversion | awk -F. '{ printf
> > "0x%02x%02x%02x", $1, $2, $3 }'
> >  }
> >  
> >  # random config or default config
> > @@ -66,7 +66,7 @@ if ! type python3 || python3 -c "import sys; res
> > = sys.version_info < (3, 5); ex
> >  fi
> >  
> >  # SeaBIOS requires GCC 4.6 or later
> > -if [[ "${CC}" == "gcc" && `cc-ver` -lt 0x040600 ]]; then
> > +if [[ "${CROSS_COMPILE}${CC}" == "gcc" && `cc-ver` -lt 0x040600
> > ]]; then
> 
> This change won't work, because it's no longer a plain "gcc".
> 

If look at tests on GitLab CI&CD these changes don't break anything.

> Also, prepreding CROSS_COMPILE isn't compatible with LLVM toolchains,
> but that's not something you've made any worse with your change (just
> more obvious).
> 

CROSS_COMPILE isn't use with LLVM toolchain. I mean that in case of
LLVM toolchain CROSS_COMPILE would be equal to empty string plus
CC=clang or something similar.

> We probably want a stanza near the top which sets
> CC="${CROSS_COMPILE}${CC}" which can be adjusted to support LLVM in
> due
> course, and we'll need to detect GCC using --version | grep as its
> done
> elsewhere.
> 
> But the other logic wants reworking too so we don't play around with
> bits of the tools build when we're doing a hypervisor-only build
> anyway...
> 

I think that I can get rid of CROSS_COMPILE variable at all and use
CC=riscv64-linux-gnu-gcc direcly for RISC-V 64 targers in build.yaml.
Would this be a better solution?

> ~Andrew

~Oleksii

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

[Oleksii]

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

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

[Oleksii]

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

Re: [XEN PATCH v1 3/4] automation: add python3 package for riscv64.dockerfile

[Andrew Cooper]

On 29/12/2022 7:45 am, Oleksii wrote:
> On Wed, 2022-12-28 at 23:26 +0000, Andrew Cooper wrote:
>> On 23/12/2022 11:16 am, Oleksii Kurochko wrote:
>>> Pyhton3 package is requited by automation/scripts/build
>>> script so it shoud be installed to riscv64 docker image
>> Is it?  This series runs fine without it.
>>
> It is used by automation/scripts/build here:
> https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/automation/scripts/build#L63

That logic reads "if ( !py3 || !(py3.ver > 3.5) ) skip" so is fine with
no py3.  Some of the older build containers only have py2.

>
> It looks like this patch can be skipped for now as it doesn't affect
> results of RISC-V cross-build test.

Yeah, it can be dropped for now.

~Andrew

Re: [XEN PATCH v1 2/4] automation: add cross-compiler support for the build script

[Oleksii]

On Thu, 2022-12-29 at 10:13 +0200, Oleksii wrote:
> On Wed, 2022-12-28 at 23:24 +0000, Andrew Cooper wrote:
> > On 23/12/2022 11:16 am, Oleksii Kurochko wrote:
> > > diff --git a/automation/scripts/build b/automation/scripts/build
> > > index a593419063..026f480e76 100755
> > > --- a/automation/scripts/build
> > > +++ b/automation/scripts/build
> > > @@ -2,12 +2,12 @@
> > >  
> > >  test -f /etc/os-release && cat "$_"
> > >  
> > > -$CC --version
> > > +${CROSS_COMPILE}${CC} --version
> > >  
> > >  # Express the compiler version as an integer.  e.g. GCC 4.9.2 =>
> > > 0x040902
> > >  cc-ver()
> > >  {
> > > -    $CC -dumpversion | awk -F. '{ printf "0x%02x%02x%02x", $1,
> > > $2,
> > > $3 }'
> > > +    ${CROSS_COMPILE}${CC} -dumpversion | awk -F. '{ printf
> > > "0x%02x%02x%02x", $1, $2, $3 }'
> > >  }
> > >  
> > >  # random config or default config
> > > @@ -66,7 +66,7 @@ if ! type python3 || python3 -c "import sys;
> > > res
> > > = sys.version_info < (3, 5); ex
> > >  fi
> > >  
> > >  # SeaBIOS requires GCC 4.6 or later
> > > -if [[ "${CC}" == "gcc" && `cc-ver` -lt 0x040600 ]]; then
> > > +if [[ "${CROSS_COMPILE}${CC}" == "gcc" && `cc-ver` -lt 0x040600
> > > ]]; then
> > 
> > This change won't work, because it's no longer a plain "gcc".
> > 
> 
> If look at tests on GitLab CI&CD these changes don't break anything.
> 
> > Also, prepreding CROSS_COMPILE isn't compatible with LLVM
> > toolchains,
> > but that's not something you've made any worse with your change
> > (just
> > more obvious).
> > 
> 
> CROSS_COMPILE isn't use with LLVM toolchain. I mean that in case of
> LLVM toolchain CROSS_COMPILE would be equal to empty string plus
> CC=clang or something similar.
> 
> > We probably want a stanza near the top which sets
> > CC="${CROSS_COMPILE}${CC}" which can be adjusted to support LLVM in
> > due
> > course, and we'll need to detect GCC using --version | grep as its
> > done
> > elsewhere.
> > 
> > But the other logic wants reworking too so we don't play around
> > with
> > bits of the tools build when we're doing a hypervisor-only build
> > anyway...
> > 
> 
> I think that I can get rid of CROSS_COMPILE variable at all and use
> CC=riscv64-linux-gnu-gcc direcly for RISC-V 64 targers in build.yaml.
> Would this be a better solution?
> 
It will not work.
At least that CROSS_COMPILE variable is set in riscv64.dockerfile.
So it will definitely mess the hypervisor build.
> > ~Andrew
> 
> ~Oleksii
> 

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

[Julien Grall]

Hi,

On 29/12/2022 08:45, Oleksii wrote:
> 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?

I don't have a strong opinion either way. I would say what's the easiest 
for you. In the long term, we will want to have a common linker script.

But that's a separate discussion and not a request for you to do it.

Cheers,

-- 
Julien Grall