Re: [PATCH v4 2/4] xen: Add files needed for minimal ppc64le build

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.06.2023 19:48, Shawn Anastasio wrote:
> --- /dev/null
> +++ b/xen/arch/ppc/Kconfig
> @@ -0,0 +1,42 @@
> +config PPC
> +     def_bool y
> +
> +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

I think the "if" here is at best confusing. If / when ppc32 support
is added, a potentially different default here would make necessary
adjustments, yet imo would not be very likely to introduce this very
"if".

> --- /dev/null
> +++ b/xen/arch/ppc/arch.mk
> @@ -0,0 +1,12 @@
> +########################################
> +# Power-specific definitions
> +
> +ppc-march-$(CONFIG_POWER_ISA_2_07B) := power8
> +ppc-march-$(CONFIG_POWER_ISA_3_00) := power9
> +
> +CFLAGS += -m64 -mlittle-endian -mcpu=$(ppc-march-y)
> +CFLAGS += -mstrict-align -mcmodel=large -mabi=elfv2 -mno-altivec -mno-vsx

Just for my own education: Besides the expected effect, -mstrict-align
also appears to imply -mbit-align, which I'm not sure is intended here.
Could you clarify the intentions for me?

As to -mabi=elfv2, it looks as if this limits us to gcc12 and newer.
That's fine, but I think it wants pointing out in ./README (which has
a section for this kind of information).

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/config.h
> @@ -0,0 +1,63 @@
> +#ifndef __PPC_CONFIG_H__
> +#define __PPC_CONFIG_H__
> +
> +#include <xen/const.h>
> +#include <xen/page-size.h>
> +
> +#if defined(CONFIG_PPC64)
> +#define LONG_BYTEORDER 3
> +#define ELFSIZE        64
> +#define MAX_VIRT_CPUS  1024u
> +#else
> +#error "Unsupported PowerPC 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_PPC_L1_CACHE_SHIFT  7
> +#define CONFIG_PAGEALLOC_MAX_ORDER 18
> +#define CONFIG_DOMU_MAX_ORDER      9
> +#define CONFIG_HWDOM_MAX_ORDER     10
> +
> +#define OPT_CONSOLE_STR "dtuart"
> +#define INVALID_VCPU_ID MAX_VIRT_CPUS
> +
> +/* Linkage for PPC */
> +#ifdef __ASSEMBLY__
> +#define ALIGN .align 2

I think I did ask for the same on RISC-V (yet sadly it's still .align
there): .align is notoriously ambiguous. May I ask that you use .p2align
(which I think is what is meant here, else .balign)?

Jan