Re: [PATCH v4 2/4] xen/ppc: Set up a basic C environment

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2023 22:20, Shawn Anastasio wrote:
> Update ppc64/head.S to set up an initial boot stack, zero the .bss
> section, and jump to C.
> 
> Also refactor the endian fixup trampoline into its own macro, since it
> will need to be used in multiple places, including every time we make a
> call into firmware (see next commit).

Please avoid statements making relative connections between patches or
commits. You can't know how many commits will end up between both
patches; there's no guarantee any series will be committed all in one
go. (In rare cases, where needed for one reason or another, committers
may be asked to keep things together.)

That said, such statements are much less of a problem if given in the
remarks section, which isn't going to be committed.

> --- a/xen/arch/ppc/include/asm/config.h
> +++ b/xen/arch/ppc/include/asm/config.h
> @@ -43,7 +43,7 @@
>  
>  #define SMP_CACHE_BYTES (1 << 6)
>  
> -#define STACK_ORDER 2
> +#define STACK_ORDER 0

>From the v3 discussion I thought it would follow that the description
gain mention of this change (and the why behind it). Strictly speaking
this could be a separate change (at which point stating the "why"
would hopefully be an obvious part), but I wouldn't want to go as far
as demanding this to be split off.

> --- a/xen/arch/ppc/ppc64/head.S
> +++ b/xen/arch/ppc/ppc64/head.S
> @@ -1,30 +1,28 @@
>  /* SPDX-License-Identifier: GPL-2.0-or-later */
>  
> +#include <asm/asm-defns.h>
> +
>      .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.
> +     * NOTE: argument registers (r3-r9) must be preserved until the C 
> entrypoint
>       */
> -    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
> +    FIXUP_ENDIAN
> +
> +    /* set up the TOC pointer */
> +    LOAD_IMM32(%r2, .TOC.)
> +
> +    /* set up the initial stack */
> +    LOAD_IMM32(%r1, cpu0_boot_stack)

Similarly I had hoped that if not a code comment, a sentence in the
description would appear regarding the (temporary) non-PIC-ness.

Jan