Re: [PATCH v3 4/5] xen: Add files needed for minimal riscv build

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.05.2021 20:53, Connor Davis wrote:
> --- /dev/null
> +++ b/config/riscv64.mk
> @@ -0,0 +1,5 @@
> +CONFIG_RISCV := y
> +CONFIG_RISCV_64 := y
> +CONFIG_RISCV_$(XEN_OS) := y

I wonder whether the last one actually gets used anywhere, but I do
realize that other architectures have similar definitions.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -26,7 +26,9 @@ MAKEFLAGS += -rR
>  EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
>  
>  ARCH=$(XEN_TARGET_ARCH)
> -SRCARCH=$(shell echo $(ARCH) | sed -e 's/x86.*/x86/' -e 
> s'/arm\(32\|64\)/arm/g')
> +SRCARCH=$(shell echo $(ARCH) | \
> +       sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
> +           -e s'/riscv.*/riscv/g')

I think in makefiles tab indentation would better be restricted to
rules. While here it's a minor nit, ...

> @@ -35,7 +37,8 @@ include $(XEN_ROOT)/Config.mk
>  # Set ARCH/SUBARCH appropriately.
>  export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
>  export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
> -                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g')
> +                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' 
> \
> +                             -e s'/riscv.*/riscv/g')

... here you're actually introducing locally inconsistent indentation.

> --- /dev/null
> +++ b/xen/arch/riscv/Kconfig
> @@ -0,0 +1,52 @@
> +config 64BIT
> +     bool

I'm afraid this is stale now - the option now lives in xen/arch/Kconfig,
available to all architectures.

> +config RISCV_64
> +     bool
> +     depends on 64BIT

Such a "depends on" is relatively pointless - they're more important to
have when there is a prompt.

> +config ARCH_DEFCONFIG
> +     string
> +     default "arch/riscv/configs/riscv64_defconfig" if RISCV_64

For the RISCV_64 dependency to be really useful (at least with the
command line kconfig), you want its selection to live above the use.

> +menu "Architecture Features"
> +
> +source "arch/Kconfig"
> +
> +endmenu
> +
> +menu "ISA Selection"
> +
> +choice
> +     prompt "Base ISA"
> +        default RISCV_ISA_RV64IMA
> +        help
> +          This selects the base ISA extensions that Xen will target.
> +
> +config RISCV_ISA_RV64IMA
> +     bool "RV64IMA"
> +        select 64BIT
> +        select RISCV_64

I think you want only the latter here, and the former done by RISCV_64
(or select the former here, and have "default y if 64BIT" at RISCV_64).
That way, not every party wanting 64-bit has to select both.

> +        help
> +           Use the RV64I base ISA, plus the "M" and "A" extensions
> +           for integer multiply/divide and atomic instructions, respectively.
> +
> +endchoice
> +
> +config RISCV_ISA_C
> +     bool "Compressed extension"
> +        help
> +           Add "C" to the ISA subsets that the toolchain is allowed
> +           to emit when building Xen, which results in compressed
> +           instructions in the Xen binary.
> +
> +           If unsure, say N.

For all of the above, please adjust indentation to be consistent with
(the bulk of) what we have elsewhere.

> --- /dev/null
> +++ b/xen/arch/riscv/arch.mk
> @@ -0,0 +1,16 @@
> +########################################
> +# RISCV-specific definitions
> +
> +ifeq ($(CONFIG_RISCV_64),y)
> +    CFLAGS += -mabi=lp64
> +endif

Wherever possible I think we should prefer the list approach:

CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64

> --- /dev/null
> +++ b/xen/arch/riscv/configs/riscv64_defconfig
> @@ -0,0 +1,12 @@
> +# CONFIG_SCHED_CREDIT is not set
> +# CONFIG_SCHED_RTDS is not set
> +# CONFIG_SCHED_NULL is not set
> +# CONFIG_SCHED_ARINC653 is not set
> +# CONFIG_TRACEBUFFER is not set
> +# CONFIG_DEBUG is not set
> +# CONFIG_DEBUG_INFO is not set
> +# CONFIG_HYPFS is not set
> +# CONFIG_GRANT_TABLE is not set
> +# CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
> +
> +CONFIG_EXPERT=y

These are rather odd defaults, more like for a special purpose
config than a general purpose one. None of what you turn off here
will guarantee to be off for people actually trying to build
things, so it's not clear to me what the idea here is. As a
specific remark, especially during bringup work I think it is
quite important to not default DEBUG to off: You definitely want
to see whether any assertions trigger.

> --- /dev/null
> +++ b/xen/include/asm-riscv/config.h
> @@ -0,0 +1,110 @@
> +/******************************************************************************
> + * config.h
> + *
> + * A Linux-style configuration list.

May I suggest to not further replicate this now inapplicable
comment? It was already somewhat bogus for Arm to clone from x86.

> + */
> +
> +#ifndef __RISCV_CONFIG_H__
> +#define __RISCV_CONFIG_H__
> +
> +#if defined(CONFIG_RISCV_64)
> +# define LONG_BYTEORDER 3
> +# define ELFSIZE 64
> +#else
> +# error "Unsupported RISCV variant"
> +#endif
> +
> +#define BYTES_PER_LONG (1 << LONG_BYTEORDER)
> +#define BITS_PER_LONG  (BYTES_PER_LONG << 3)
> +#define POINTER_ALIGN  BYTES_PER_LONG
> +
> +#define BITS_PER_LLONG 64
> +
> +/* xen_ulong_t is always 64 bits */
> +#define BITS_PER_XEN_ULONG 64
> +
> +#define CONFIG_RISCV 1

This duplicates a "real" Kconfig setting, doesn't it?

> +#define CONFIG_RISCV_L1_CACHE_SHIFT 6
> +
> +#define CONFIG_PAGEALLOC_MAX_ORDER 18
> +#define CONFIG_DOMU_MAX_ORDER      9
> +#define CONFIG_HWDOM_MAX_ORDER     10
> +
> +#define OPT_CONSOLE_STR "dtuart"
> +
> +#ifdef CONFIG_RISCV_64
> +#define MAX_VIRT_CPUS 128u
> +#else
> +#error "Unsupported RISCV variant"
> +#endif

Could this be folded with the other similar construct further up?

Jan