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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.06.2023 17:06, Shawn Anastasio wrote:
> On typical Power VMs (e.g. QEMU's -M pseries), a variety of services
> are provided by OpenFirmware, including an early serial console.
> Implement the required interfaces to call into OpenFirmware and write
> to the serial console.
> 
> Since OpenFirmware 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 head.S.
> 
> Support for bare metal (PowerNV) will be implemented in a future
> patch.
> 
> Signed-off-by: Shawn Anastasio <shawnanastasio@xxxxxxxxxxxxxxxxxxxxx>

Just a couple of nits:

>  xen/arch/ppc/Kconfig.debug               |   5 +
>  xen/arch/ppc/Makefile                    |   3 +-
>  xen/arch/ppc/boot_of.c                   | 122 +++++++++++++++++++++++
>  xen/arch/ppc/configs/openpower_defconfig |   1 +
>  xen/arch/ppc/early_printk.c              |  36 +++++++
>  xen/arch/ppc/include/asm/boot.h          |  31 ++++++
>  xen/arch/ppc/include/asm/bug.h           |   6 ++
>  xen/arch/ppc/include/asm/byteorder.h     |  74 ++++++++++++++
>  xen/arch/ppc/include/asm/cache.h         |   6 ++
>  xen/arch/ppc/include/asm/config.h        |   3 +
>  xen/arch/ppc/include/asm/early_printk.h  |  14 +++
>  xen/arch/ppc/include/asm/processor.h     |  54 +++++++++-
>  xen/arch/ppc/include/asm/string.h        |   6 ++
>  xen/arch/ppc/include/asm/types.h         |  64 ++++++++++++
>  xen/arch/ppc/ppc64/asm-offsets.c         |  55 ++++++++++
>  xen/arch/ppc/ppc64/head.S                |  59 +++++++++++
>  xen/arch/ppc/setup.c                     |  20 +++-
>  17 files changed, 555 insertions(+), 4 deletions(-)
>  create mode 100644 xen/arch/ppc/boot_of.c

Unless required, in new additions we tend to prefer dashes over
underscores. In filenames it is pretty rare that dashes really need
avoiding.

> --- a/xen/arch/ppc/Kconfig.debug
> +++ b/xen/arch/ppc/Kconfig.debug
> @@ -0,0 +1,5 @@
> +config EARLY_PRINTK
> +    bool "Enable early printk"
> +    default DEBUG
> +    help
> +      Enables early printk debug messages
> \ No newline at end of file

There are many examples of this throughout the patch, which you want to
take care of.

> --- /dev/null
> +++ b/xen/arch/ppc/boot_of.c
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

By default we mean to use ...

> --- /dev/null
> +++ b/xen/arch/ppc/early_printk.c
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

... the more modern form of this (GPL-2.0-only). Anything deviating from
that may want justifying in the description.

Jan