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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2023 21:04, Shawn Anastasio wrote:
> --- a/xen/arch/ppc/Makefile
> +++ b/xen/arch/ppc/Makefile
> @@ -1,5 +1,7 @@
>  obj-$(CONFIG_PPC64) += ppc64/
>  
> +obj-y += boot-of.init.o
> +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o

While in this case I could accept the name as is to be the same as Arm's,
as said before it would be nice if new files could avoid underscores (and
use dashes instead) unless strictly required by something.

Also, with boot-of.c using early_printk.c, shouldn't the latter also
build into early_printk.init.o?

> --- /dev/null
> +++ b/xen/arch/ppc/boot-of.c
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright IBM Corp. 2005, 2006, 2007

Judging from the years the file was taken from somewhere. Is the license
there permitting "2.0 or later"? (For files [partly] taken from somewhere,
a clarification as to the originals' licenses would be helpful to have in
the description, or maybe in the post-commit-message area.)

> + * Copyright Raptor Engineering, LLC
> + *
> + * Authors: Jimi Xenidis <jimix@xxxxxxxxxxxxxx>
> + *          Hollis Blanchard <hollisb@xxxxxxxxxx>
> + *          Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> + */
> +
> +#include <xen/init.h>
> +#include <xen/stdarg.h>
> +#include <xen/types.h>
> +#include <asm/boot.h>
> +#include <asm/byteorder.h>
> +#include <asm/early_printk.h>
> +
> +#define ADDR(x) (uint32_t)(unsigned long)(x)

Besides needing to be wrapped in parentheses, this likely needs a comment:
It is hard to see how in 64-bit code uint32_t can suffice as an address.
Unless (see the question on patch 1) you intend to link/run Xen at an
address firmly below 4Gb, now and going forward.

> +/* OF entrypoint*/
> +static unsigned long __initdata of_vec;
> +
> +/* OF device handles*/
> +static int __initdata bof_chosen;
> +static int __initdata of_out;
> +
> +static int __init of_call(const char *service, uint32_t nargs, uint32_t 
> nrets,
> +                   int32_t rets[], ...)

Nit: Indentation.

> +{
> +    int rc;
> +    va_list args;
> +    int i;

unsigned int?

> +    struct of_service s = { 0 };
> +
> +    s.ofs_service = cpu_to_be32(ADDR(service));
> +    s.ofs_nargs = cpu_to_be32(nargs);
> +    s.ofs_nrets = cpu_to_be32(nrets);
> +
> +    /* copy all the params into the args array */

Nit: Style (comments want to start with a capital letter). Again below.

> +    va_start(args, rets);
> +
> +    for ( i = 0; i < nargs; i++ )
> +        s.ofs_args[i] = cpu_to_be32(va_arg(args, uint32_t));
> +
> +    va_end(args);
> +
> +    rc = enter_of(&s, of_vec);
> +
> +    /* copy all return values to the output rets array */
> +    for ( i = 0; i < nrets; i++ )
> +        rets[i] = be32_to_cpu(s.ofs_args[i + nargs]);
> +
> +    return rc;
> +}
> +
> +static int __init of_finddevice(const char *devspec)
> +{
> +    int rets[1] = { OF_FAILURE };

Hmm, of_call() uses int32_t. Again below several times.

> +    of_call("finddevice", 1, 1, rets, devspec);

This could do with using ARRAY_SIZE(rets). Same again below.

Also don't you need to wrap devspec in ADDR()? Similar issues
then again below.

> +    return rets[0];
> +}
> +
> +static int __init of_getprop(int ph, const char *name, void *buf, uint32_t 
> buflen)
> +{
> +    int rets[1] = { OF_FAILURE };
> +
> +    of_call("getprop", 4, 1, rets, ph, ADDR(name), buf, buflen);
> +    return rets[0];
> +}
> +
> +int __init of_write(int ih, const char *addr, uint32_t len)
> +{
> +    int rets[1] = { OF_FAILURE };
> +
> +    of_call("write", 3, 1, rets, ih, addr, len);
> +    return rets[0];
> +}
> +
> +static void __init of_putchar(char c)
> +{
> +    if ( c == '\n' )
> +    {
> +        char buf = '\r';
> +        of_write(of_out, &buf, 1);
> +    }
> +    of_write(of_out, &c, 1);
> +}
> +
> +void __init boot_of_init(unsigned long vec)
> +{
> +    of_vec = vec;
> +
> +    /* Get a handle to the default console */
> +    bof_chosen = of_finddevice("/chosen");
> +    of_getprop(bof_chosen, "stdout", &of_out, sizeof(of_out));
> +    of_out = be32_to_cpu(of_out);
> +
> +    early_printk_init(of_putchar);
> +}

Considering that bof_chosen is used just here, why does it need to be
a file-scope variable?

> --- /dev/null
> +++ b/xen/arch/ppc/early_printk.c
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#include <xen/init.h>
> +#include <asm/boot.h>
> +
> +static void (*putchar_func)(char);

__initdata? (Connected to the question of building into .init.o.)

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/processor.h
> @@ -0,0 +1,139 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright IBM Corp. 2005, 2006, 2007
> + * Copyright Raptor Engineering, LLC
> + *
> + * Authors: Hollis Blanchard <hollisb@xxxxxxxxxx>
> + *          Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx>
> + *          Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>
> + *          Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> + */
> +
> +#ifndef _ASM_PPC_PROCESSOR_H
> +#define _ASM_PPC_PROCESSOR_H
> +
> +#include <xen/config.h>

No need to - this is force-included by the Makefile machinery.

> +#include <xen/types.h>

Note this. Why ...

> +#define IOBMP_BYTES          8192
> +#define IOBMP_INVALID_OFFSET 0x8000
> +
> +/* Processor Version Register (PVR) field extraction */
> +
> +#define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */
> +#define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF)  /* Revison field */
> +
> +#define __is_processor(pv) (PVR_VER(mfspr(SPRN_PVR)) == (pv))
> +
> +/*
> + * IBM has further subdivided the standard PowerPC 16-bit version and
> + * revision subfields of the PVR for the PowerPC 403s into the following:
> + */
> +
> +#define PVR_FAM(pvr)  (((pvr) >> 20) & 0xFFF) /* Family field */
> +#define PVR_MEM(pvr)  (((pvr) >> 16) & 0xF)   /* Member field */
> +#define PVR_CORE(pvr) (((pvr) >> 12) & 0xF)   /* Core field */
> +#define PVR_CFG(pvr)  (((pvr) >> 8) & 0xF)    /* Configuration field */
> +#define PVR_MAJ(pvr)  (((pvr) >> 4) & 0xF)    /* Major revision field */
> +#define PVR_MIN(pvr)  (((pvr) >> 0) & 0xF)    /* Minor revision field */
> +
> +/* Processor Version Numbers */
> +
> +#define PVR_403GA    0x00200000
> +#define PVR_403GB    0x00200100
> +#define PVR_403GC    0x00200200
> +#define PVR_403GCX   0x00201400
> +#define PVR_405GP    0x40110000
> +#define PVR_STB03XXX 0x40310000
> +#define PVR_NP405H   0x41410000
> +#define PVR_NP405L   0x41610000
> +#define PVR_601      0x00010000
> +#define PVR_602      0x00050000
> +#define PVR_603      0x00030000
> +#define PVR_603e     0x00060000
> +#define PVR_603ev    0x00070000
> +#define PVR_603r     0x00071000
> +#define PVR_604      0x00040000
> +#define PVR_604e     0x00090000
> +#define PVR_604r     0x000A0000
> +#define PVR_620      0x00140000
> +#define PVR_740      0x00080000
> +#define PVR_750      PVR_740
> +#define PVR_740P     0x10080000
> +#define PVR_750P     PVR_740P
> +#define PVR_7400     0x000C0000
> +#define PVR_7410     0x800C0000
> +#define PVR_7450     0x80000000
> +#define PVR_8540     0x80200000
> +#define PVR_8560     0x80200000
> +/*
> + * For the 8xx processors, all of them report the same PVR family for
> + * the PowerPC core. The various versions of these processors must be
> + * differentiated by the version number in the Communication Processor
> + * Module (CPM).
> + */
> +#define PVR_821  0x00500000
> +#define PVR_823  PVR_821
> +#define PVR_850  PVR_821
> +#define PVR_860  PVR_821
> +#define PVR_8240 0x00810100
> +#define PVR_8245 0x80811014
> +#define PVR_8260 PVR_8240
> +
> +/* 64-bit processors */
> +#define PVR_NORTHSTAR 0x0033
> +#define PVR_PULSAR    0x0034
> +#define PVR_POWER4    0x0035
> +#define PVR_ICESTAR   0x0036
> +#define PVR_SSTAR     0x0037
> +#define PVR_POWER4p   0x0038
> +#define PVR_970       0x0039
> +#define PVR_POWER5    0x003A
> +#define PVR_POWER5p   0x003B
> +#define PVR_970FX     0x003C
> +#define PVR_POWER6    0x003E
> +#define PVR_POWER7    0x003F
> +#define PVR_630       0x0040
> +#define PVR_630p      0x0041
> +#define PVR_970MP     0x0044
> +#define PVR_970GX     0x0045
> +#define PVR_POWER7p   0x004A
> +#define PVR_POWER8E   0x004B
> +#define PVR_POWER8NVL 0x004C
> +#define PVR_POWER8    0x004D
> +#define PVR_POWER9    0x004E
> +#define PVR_POWER10   0x0080
> +#define PVR_BE        0x0070
> +#define PVR_PA6T      0x0090
> +
> +/* Macro to adjust thread priority for hardware multithreading */
> +#define HMT_very_low()  asm volatile (" or %r31, %r31, %r31 ")
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/types.h>

... another time here? Or is the first one perhaps too early?

> +/* User-accessible registers: nost of these need to be saved/restored
> + * for every nested Xen invocation. */

Nit: Comment style. Multi-line comments want to be

/*
 * User-accessible registers: most of these need to be saved/restored
 * for every nested Xen invocation.
 */

(also note the s/nost/most/ I did)

> --- a/xen/arch/ppc/ppc64/head.S
> +++ b/xen/arch/ppc/ppc64/head.S
> @@ -18,6 +18,15 @@ ENTRY(start)
>      li %r11, 0
>      stdu %r11, -STACK_FRAME_OVERHEAD(%r1)
>  
> +    /* clear .bss */
> +    LOAD_IMM32(%r14, __bss_start)
> +    LOAD_IMM32(%r15, __bss_end)

Question regarding addressing again.

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

Nit: Missing blank after last comma. I also wonder whether the parentheses
are doing any good here.

Jan