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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/06/2023 4:49 pm, Shawn Anastasio wrote:
> 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?

Or just rename openpower_defconfig to ppc64_defconfig ?

Is there any reason to keep openpower differently?

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

Yeah, that would definitely help.

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

Maybe keep as 64k for now, with 4k as a backup if major problems are
encountered?

I honestly don't know how much of Xen's common code is non-4k-clean, and
this is the best opportunity to find out.

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

There are reasons why this doesn't exist/work yet, and that's a swamp
you swamp you absolutely don't want to wade into at this point.

Just leave the raw annotations for now, and they'll be cleaned up when
the bugs with the various proposed options have been fixed.

~Andrew