Re: [PATCH v2 2/3] xen/ppc: Implement early serial printk on pseries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.06.2023 22:57, Shawn Anastasio wrote:
> On typical Power VMs (e.g. QEMU's -M pseries), a variety of services
> including an early serial console are provided by Open Firmware.
> Implement the required interfaces to call into Open Firmware and write
> to the serial console.
> 
> Since Open Firmware runs in 32-bit Big Endian mode and Xen runs in
> 64-bit Little Endian mode, a thunk is required to save/restore
> any potentially-clobbered registers as well as to perform the
> required endianness switch. Thankfully, linux already has such
> a routine, which was imported into ppc64/of-call.S.
> 
> Support for bare metal (PowerNV) will be implemented in a future
> patch.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>

Just two minor remarks; I can't really review most of this for
lack of knowledge.

> --- a/xen/arch/ppc/Makefile
> +++ b/xen/arch/ppc/Makefile
> @@ -1,4 +1,6 @@
>  obj-$(CONFIG_PPC64) += ppc64/
> +obj-y += boot-of.init.o
> +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>  obj-y += setup.o

While you've followed Julien's earlier comment partly, I think,
sorting still isn't alphabetical. If you mean ppc64/ to
explicitly be first, you will want to introduce a separating
blank line.

> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/of-call.S
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Adapted from Linux's arch/powerpc/kernel/entry_64.S, with the
> + * following copyright notice:
> + *
> + *  PowerPC version
> + *    Copyright (C) 1995-1996 Gary Thomas (gdt@xxxxxxxxxxxx)
> + *  Rewritten by Cort Dougan (cort@xxxxxxxxxx) for PReP
> + *    Copyright (C) 1996 Cort Dougan <cort@xxxxxxxxxx>
> + *  Adapted for Power Macintosh by Paul Mackerras.
> + *  Low-level exception handlers and MMU support
> + *  rewritten by Paul Mackerras.
> + *    Copyright (C) 1996 Paul Mackerras.
> + *  MPC8xx modifications Copyright (C) 1997 Dan Malek (dmalek@xxxxxxx).
> + */
> +
> +#include <asm/asm-offsets.h>
> +#include <asm/asm-defns.h>
> +#include <asm/msr.h>
> +
> +/* size of minimum stack frame that can hold an entire cpu_user_regs struct 
> */
> +#define STACK_SWITCH_FRAME_SIZE (UREGS_sizeof + STACK_FRAME_OVERHEAD)
> +
> +    .section .init.text, "ax", @progbits
> +
> +ENTRY(enter_of)
> +    mflr %r0
> +    std %r0, 16(%r1)
> +    stdu %r1,-STACK_SWITCH_FRAME_SIZE(%r1) /* Save SP and create stack space 
> */
> +
> +    /*
> +     * Because PROM is running in 32b mode, it clobbers the high order half
> +     * of all registers that it saves.  We therefore save those registers
> +     * PROM might touch to the stack.  (%r0, %r3-%r13 are caller saved)
> +     */
> +    SAVE_GPR(2, %r1)
> +    SAVE_GPR(13, %r1)
> +    SAVE_NVGPRS(%r1)
> +    mfcr %r10
> +    mfmsr %r11
> +    std %r10, UREGS_cr(%r1)
> +    std %r11, UREGS_msr(%r1)
> +
> +    /* Put PROM address in SRR0 */
> +    mtsrr0 %r4
> +
> +    /* Setup our trampoline return addr in LR */
> +    bcl 20, 31,.+4
> +0:  mflr %r4
> +    addi %r4, %r4,(1f - 0b)
> +    mtlr %r4
> +
> +    /* Prepare a 32-bit mode big endian MSR */
> +    LOAD_IMM64(%r12, MSR_SF | MSR_LE)
> +    andc %r11, %r11, %r12
> +    mtsrr1 %r11
> +    rfid

I think readability would improve if you reserved a minimum number
of characters (6?) for the mnemonics, padding with blanks when
they're shorter. (This, if you'd be willing to switch, would then
also apply to patch 1, iirc.)

Jan