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

From: "Shawn Anastasio" <shawn@anastas.io>
To: "Jan Beulich" <jbeulich@suse.com>
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, 09 Jun 2023 10:34:41 -0500	[thread overview]
Message-ID: <CT88A3XMI9ZS.WUDTG1UJGQVC@Shawns-Mac-mini.lan> (raw)
In-Reply-To: <9676af82-7af2-24d4-838d-6f5a90c1d604@suse.com>

On Fri Jun 9, 2023 at 4:15 AM CDT, Jan Beulich wrote:
> > --- /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>

Good point, this was definitely an oversight on my part. I'll clean this
up in v2.

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

This was mostly modeled after riscv/Kconfig. The idea was that this
option guards PPC support in general (both 32- and 64-bit) and the
subsequent option specifically opts in to 64-bit.

Given that this effort is only targeting 64-bit mode, though, the
argument could definitely be made that having the two options is
redundant, but I believe that is also the case for the risc-v port.

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

Fair point. The + was intended as an "or newer" but I agree that it's
ambiguous (for instance, it might be interpreted as a part of the ISA's
name). I'll remove it in v2, along with a renaming of "POWER ISA" to
"Power ISA" which I originally overlooked.

> Jan

Thanks,
Shawn