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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 8/1/23 6:19 AM, Jan Beulich wrote:
> 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.

Sure, will do in v2.

>> --- 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?

I made it uint8_t to match the official OPAL API documentation (though I
now see I missed a `const`):
https://open-power.github.io/skiboot/doc/opal-api/opal-console-read-write-1-2.html#opal-console-write

In this case though, the type information of this parameter might not be
that important and changing it to void* to avoid the cast is fine with
me.

>> +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?

Unlike OpenFirmware, OPAL calls are expected to be used by the OS during
its entire lifecycle, not just during early boot, so the full
(non-early) serial console driver would likely want to use these
functions as well.

> 
>> +{
>> +    uint64_t len;
>> +    if (c == '\n')
> 
> Nit: Blank line please between declaration(s) and statement(s). (At
> least one more instance below.)

Will fix.

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

Ditto.

>> +    {
>> +        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.

Will fix.

>> --- /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?

Sure, I'll make this change along with the formatting updates in
asm-defns.h

>> +#include <asm/opal-api.h>
>> +#include <asm/msr.h>
>> +
>> +    .text
> 
> Is any of this code still needed post-init?

Yes, see above.

>> +#define OPAL_CALL(name, token)  \
>> +    .globl name;                \
>> +name:                           \
>> +    li      %r0, token;         \
>> +    b       opal_call;
> 
> I think the trailing semicolon wants omitting.

Will fix.

> Jan

Thanks,
Shawn