Re: [PATCH 1/3] xen: Add files needed for minimal Power build

From: Jan Beulich <jbeulich@suse.com>
To: Shawn Anastasio <shawn@anastas.io>
Cc: tpearson@raptorengineering.com,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Shawn Anastasio <shawnanastasio@raptorengineering.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 1/3] xen: Add files needed for minimal Power build
Date: Fri, 9 Jun 2023 11:15:38 +0200	[thread overview]
Message-ID: <9676af82-7af2-24d4-838d-6f5a90c1d604@suse.com> (raw)
In-Reply-To: <e2e1435f5d0fadaa00dfff6c04af29165439db5d.1686148363.git.shawn@anastas.io>

On 07.06.2023 17:06, Shawn Anastasio wrote:
> Add the build system changes required to build for ppc64le (POWER8+).
> Following in the footsteps of the initial riscv port, only building
> the head.o target, which boots to an infinite loop, is supported:
> 
> $ make XEN_TARGET_ARCH=ppc64 -C xen openpower_defconfig
> $ make XEN_TARGET_ARCH=ppc64 SUBSYSTEMS=xen -C xen TARGET=ppc64/head.o
> 
> This port targets POWER8+ CPUs running in Little Endian mode specifically,
> and does not boot on older machines. Additionally, this initial skeleton
> only implements the PaPR/pseries boot protocol which allows it to be
> booted in a standard QEMU virtual machine:
> 
> $ qemu-system-ppc64 -M pseries-5.2 -m 256M -kernel ppc64/head.o
> 
> Where possible, this patch uses header definitions and support routines
> from the original Xen PowerPC port present in Xen 3.2.3. Though we are
> targeting a much newer ISA than that original port did, some of the
> definitions have remained similar enough for reuse.
> 
> Signed-off-by: Shawn Anastasio <shawnanastasio@raptorengineering.com>

Just a few small remarks, as following Andrew's comments I expect the
patch will shrink quite a bit:

> --- /dev/null
> +++ b/config/ppc64.mk
> @@ -0,0 +1,5 @@
> +CONFIG_PPC64 := y
> +CONFIG_PPC64_64 := y
> +CONFIG_PPC64_$(XEN_OS) := y

The first of the 64-s here are a little odd; looking at RISC-V's
counterpart, wouldn't this want to be

CONFIG_PPC := y
CONFIG_PPC_64 := y
CONFIG_PPC_$(XEN_OS) := y

> --- /dev/null
> +++ b/xen/arch/ppc/Kconfig
> @@ -0,0 +1,42 @@
> +config PPC
> +	def_bool y

Is this necessary? Iirc PPC is frequently used as a name for 32-bit PPC
(but then also elsewhere as covering both 32- and 64-bit), so I'm not
sure we want this without having a need for it.

> +config PPC64
> +	def_bool y
> +	select 64BIT
> +
> +config ARCH_DEFCONFIG
> +	string
> +	default "arch/ppc/configs/openpower_defconfig"
> +
> +menu "Architecture Features"
> +
> +source "arch/Kconfig"
> +
> +endmenu
> +
> +menu "ISA Selection"
> +
> +choice
> +	prompt "Base ISA"
> +	default POWER_ISA_2_07B if PPC64
> +	help
> +	  This selects the base ISA version that Xen will target.
> +
> +config POWER_ISA_2_07B
> +	bool "POWER ISA 2.07B+"
> +	help
> +	  Target version 2.07B+ of the POWER ISA (POWER8+)
> +
> +config POWER_ISA_3_00
> +	bool "POWER ISA 3.00+"
> +	help
> +	  Target version 3.00+ of the POWER ISA (POWER9+)

What are the + in here meant to indicate? Since this is about a baseline
ISA, I find such a use (presumably standing for "or newer") ambiguous.

Jan