Re: [PATCH 5/5] xen/ppc: Implement early serial console on PowerNV

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.07.2023 23:35, Shawn Anastasio wrote:
> --- a/xen/arch/ppc/include/asm/asm-defns.h
> +++ b/xen/arch/ppc/include/asm/asm-defns.h
> @@ -23,6 +23,18 @@
>      addis reg,%r2,name@toc@ha;                                               
> \
>      addi  reg,reg,name@toc@l

Noticing only now, because of the issue ...

> +/*
> + * Declare a global assembly function with a proper TOC setup prologue
> + */
> +#define _GLOBAL_TOC(name)                                                   \
> +    .balign 4;                                                              \
> +    .type name,@function;                                                   \
> +    .globl name;                                                            \
> +name:                                                                       \
> +0:  addis %r2,%r12,(.TOC.-0b)@ha;                                           \
> +    addi %r2,%r2,(.TOC.-0b)@l;                                              \
> +    .localentry name,.-name

... being widened - could we gain blanks after the commas? Down here
(to match the code in context) another padding blank after "addi"
would also be nice.

> --- a/xen/arch/ppc/opal.c
> +++ b/xen/arch/ppc/opal.c
> @@ -8,9 +8,28 @@
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  
> -/* Global OPAL struct containing entrypoint and base */
> +/* Global OPAL struct containing entrypoint and base used by opal-calls.S */
>  struct opal opal;
>  
> +int64_t opal_console_write(int64_t term_number, uint64_t *length,
> +                           uint8_t *buffer);

Would this perhaps better be void *, eliminating the need for casting
in calls of this function?

> +int64_t opal_console_flush(int64_t term_number);
> +int64_t opal_reinit_cpus(uint64_t flags);
> +
> +static void opal_putchar(char c)

Can't this be __init?

> +{
> +    uint64_t len;
> +    if (c == '\n')

Nit: Blank line please between declaration(s) and statement(s). (At
least one more instance below.)

Also please add the missing blanks in the if(), seeing that otherwise
the file is aiming at being Xen style.

> +    {
> +        char buf = '\r';
> +        len = cpu_to_be64(1);
> +        opal_console_write(0, &len, (uint8_t *) &buf);
> +    }
> +    len = cpu_to_be64(1);
> +    opal_console_write(0, &len, (uint8_t *) &c);
> +    opal_console_flush(0);
> +}
> +
>  void __init boot_opal_init(const void *fdt)
>  {
>      int opal_node;
> @@ -45,4 +64,10 @@ void __init boot_opal_init(const void *fdt)
>  
>      opal.base = be64_to_cpu(*opal_base);
>      opal.entry = be64_to_cpu(*opal_entry);
> +
> +    early_printk_init(opal_putchar);
> +
> +    /* Ask OPAL to set HID0 for Little Endian interrupts + Radix TLB support 
> */
> +    opal_reinit_cpus(OPAL_REINIT_CPUS_HILE_LE | OPAL_REINIT_CPUS_MMU_RADIX
> +                     | OPAL_REINIT_CPUS_MMU_HASH);

Nit: operators on continued lines go at the end of the earlier line.

> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/opal-calls.S
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Adapted from Linux's arch/powerpc/boot/opal-calls.S
> + *
> + * Copyright (c) 2016 IBM Corporation.
> + * Copyright Raptor Engineering, LLC
> + */
> +
> +#include <asm/asm-defns.h>
> +#include <asm/asm-offsets.h>

Would it make sense to have asm-defns.h include asm-offsets.h, like
x86 and Arm do?

> +#include <asm/opal-api.h>
> +#include <asm/msr.h>
> +
> +    .text

Is any of this code still needed post-init?

> +#define OPAL_CALL(name, token)  \
> +    .globl name;                \
> +name:                           \
> +    li      %r0, token;         \
> +    b       opal_call;

I think the trailing semicolon wants omitting.

Jan