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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 6/16/23 3:24 PM, Andrew Cooper wrote:
> 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.

It seems like the build system expects an `$(ARCH)_defconfig` present if
you don't manually specify a defconfig target. I see riscv64 has a
tiny64_defconfig and a riscv64_defconfig that are idential, probably for
this same reason. Would you like me to take the same approach of
duplicating openpower_defconfig to ppc64_defconfig?

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

This was a remnant of the old head.o-only TARGET build command that I
got from the initial riscv commit. You're correct that it's unnecessary
now and I'll drop it from the commit message.

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

Sure, I'll drop this in v5.

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

They're not that different, and a kernel built for POWER8 should just
work on POWER9. The intent was to specify a baseline feature set that
guards whether, i.e., POWER9/ISA3.0-only features should be built.

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

Originally the help text for the two ISA config options ended in a "+"
but that was deemed ambiguous. Would adding "or later" to the help text
for the two options clarify it sufficiently?

>> 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?

Both 4K and 64K are supported by the hardware.

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

Interesting. Given this I'm inclined to go with 4k just to reduce pain
points during initial bring up, though supporting 64k would definitely
be desirable going forward.

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

Good point. Following the example in the Power ELFv2 ABI
specification [1] for function type annotation, I'll place

.type start, @function

in the ENTRY macro. It's not clear what the difference between %function
and @function are in this context (my toolchain seems to accept both and
produce the same ELF metadata), but the latter is more idiomatic in
Power assembly. The same goes for its placement before the entrypoint
vs. after the last instruction.

As for the size annotation, I'll follow Julien's suggestion and
introduce an END macro.

>> 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?

No plans on supporting dynamic relocation (for now), so I can go ahead
and add these assertions.

> ~Andrew

Thanks,
Shawn

[1] Page 77 https://wiki.raptorcs.com/w/images/7/70/Leabi-20170510.pdf