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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/06/2023 6:48 pm, Shawn Anastasio wrote:
> Add the build system changes required to build for ppc64le (POWER8+).
> As of now the resulting image simply boots to an infinite loop.
>
> $ make XEN_TARGET_ARCH=ppc64 -C xen openpower_defconfig
> $ make XEN_TARGET_ARCH=ppc64 SUBSYSTEMS=xen -C xen build

I think the first of these isn't needed, given the config ARCH_DEFCONFIG
default.  I'd suggest dropping it.

On the second, what is the SUBSYSTEMS=xen?  It's not needed given the
stripped down build system, but I don't see why we'd ever be compiling
Xen with some kind of subsystem configuration for something else.

> diff --git a/config/ppc64.mk b/config/ppc64.mk
> new file mode 100644
> index 0000000000..597f0668c3
> --- /dev/null
> +++ b/config/ppc64.mk
> @@ -0,0 +1,5 @@
> +CONFIG_PPC := y
> +CONFIG_PPC64 := y
> +CONFIG_PPC_$(XEN_OS) := y

I know you're copying the existing architectures, but I'm pretty certain
these $(XEN_OS) expressions are pretty bogus.  The userspace stuff in
tools/ may need to know the host OS it's being built for, but Xen really
doesn't.

I'm pretty sure it will compile with this dropped, so please do.  I'll
see about patching it out of the other architectures.

> diff --git a/xen/arch/ppc/Kconfig b/xen/arch/ppc/Kconfig
> new file mode 100644
> index 0000000000..a0a70adef4
> --- /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
> +     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)

For both of these, it will be helpful for anyone who isn't as
PPC-knowledgeable if the POWER8/9 was in the title too, seeing as
they're the most common name.

But as I'm a noob here too, how different are Power8 and 9?  Given they
share a head.S, they're presumably not too disjoint in terms of ISA.

While being able to target a specific CPU is something we're trying to
retrofit to Xen, by default we do expect it to run on as broad a set of
systems as possible.

If that's not feasible, then fine, but if it is, it ought to be the
default.  Which might be as simple as saying "or later" somewhere in
this text, or might be a giant can of worms that I shouldn't open...
 
> diff --git a/xen/arch/ppc/include/asm/page-bits.h 
> b/xen/arch/ppc/include/asm/page-bits.h
> new file mode 100644
> index 0000000000..4c01bf9716
> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/page-bits.h
> @@ -0,0 +1,7 @@
> +#ifndef __PPC_PAGE_BITS_H__
> +#define __PPC_PAGE_BITS_H__
> +
> +#define PAGE_SHIFT              16 /* 64 KiB Pages */
> +#define PADDR_BITS              48

Is 64k the minimum granularity?  Or is 4k an option?

I ask because Xen has some very short sighted ABIs which we're still
working on removing.  There are still quite a few expectations of
PAGE_SHIFT being 12.

To be clear, we're looking to fix all of these ABIs, but I suspect it
will be an easier lift to begin with at 4k.  (Or perhaps the right thing
is to double down and just get them fixed.)

> +
> +#endif /* __PPC_PAGE_BITS_H__ */
> diff --git a/xen/arch/ppc/ppc64/Makefile b/xen/arch/ppc/ppc64/Makefile
> new file mode 100644
> index 0000000000..3340058c08
> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/Makefile
> @@ -0,0 +1 @@
> +obj-y += head.o
> diff --git a/xen/arch/ppc/ppc64/asm-offsets.c 
> b/xen/arch/ppc/ppc64/asm-offsets.c
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/xen/arch/ppc/ppc64/head.S b/xen/arch/ppc/ppc64/head.S
> new file mode 100644
> index 0000000000..0b289c713a
> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/head.S
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +.section .text.header, "ax", %progbits
> +
> +ENTRY(start)
> +    /*
> +     * Depending on how we were booted, the CPU could be running in either
> +     * Little Endian or Big Endian mode. The following trampoline from Linux
> +     * cleverly uses an instruction that encodes to a NOP if the CPU's
> +     * endianness matches the assumption of the assembler (LE, in our case)
> +     * or a branch to code that performs the endian switch in the other case.
> +     */
> +    tdi 0, 0, 0x48    /* Reverse endian of b . + 8          */
> +    b . + 44          /* Skip trampoline if endian is good  */
> +    .long 0xa600607d  /* mfmsr r11                          */
> +    .long 0x01006b69  /* xori r11,r11,1                     */
> +    .long 0x00004039  /* li r10,0                           */
> +    .long 0x6401417d  /* mtmsrd r10,1                       */
> +    .long 0x05009f42  /* bcl 20,31,$+4                      */
> +    .long 0xa602487d  /* mflr r10                           */
> +    .long 0x14004a39  /* addi r10,r10,20                    */
> +    .long 0xa6035a7d  /* mtsrr0 r10                         */
> +    .long 0xa6037b7d  /* mtsrr1 r11                         */
> +    .long 0x2400004c  /* rfid                               */
> +
> +    /* Now that the endianness is confirmed, continue */
> +1:  b 1b

.size start, . - start
.type start, %function

Lets get the ELF metadata right from the start.

> diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
> new file mode 100644
> index 0000000000..a72e519c6a
> --- /dev/null
> +++ b/xen/arch/ppc/xen.lds.S
> @@ -0,0 +1,172 @@
> <snip>
> +    DISCARD_SECTIONS
> +
> +    STABS_DEBUG_SECTIONS
> +
> +    ELF_DETAILS_SECTIONS
> +}

In the other architectures, we now assert that sections such as .got are
empty, because we've had enough bugs in the past.

I'd recommend doing the same from the outset for all the dynamic
relocation sections, unless you're expecting to have to support them?

~Andrew