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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 8/7/23 10:05 AM, Jan Beulich wrote:
> On 02.08.2023 02:11, Shawn Anastasio wrote:
>> Implement the OPAL firmware calls required to write to the serial
>> console on PowerNV systems. Unlike pseries/Open Firmware, the OPAL
>> firmware interface can be used past early boot and as such the relevant
>> functions are not marked as __init.
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> 
> Constraint as before and with two nits
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
>> @@ -20,8 +22,20 @@
>>   * Load the address of a symbol from the TOC into the specified GPR.
>>   */
>>  #define LOAD_REG_ADDR(reg,name)                                             
>>  \
>> -    addis reg,%r2,name@toc@ha;                                              
>>  \
>> -    addi  reg,reg,name@toc@l
>> +    addis reg, %r2, name@toc@ha;                                            
>>  \
>> +    addi  reg, reg, name@toc@l
>> +
>> +/*
>> + * 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;                                           
>> \
> 
> Strictly speaking the - want surrounding by blanks, but I wonder whether
> to PPC eyes these constructs look more natural without. Please clarify.

This is admittedly very subjective, but to my eyes the code as-is looks
perfectly natural. That said, I wouldn't be opposed to adding spaces if
that's what you prefer.

>> --- /dev/null
>> +++ b/xen/arch/ppc/ppc64/opal-calls.S
>> @@ -0,0 +1,81 @@
>> +/* 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/opal-api.h>
>> +#include <asm/msr.h>
>> +
>> +    .text
>> +
>> +#define OPAL_CALL(name, token)  \
>> +    .globl name;                \
>> +name:                           \
>> +    li      %r0, token;         \
>> +    b       opal_call
>> +
>> + _GLOBAL_TOC(opal_call)
> 
> Any reason for the leading blank here?

No -- that was a mistake on my part.

> 
> Where necessary I again think these small items can be taken care of
> while committing.

Sounds good.

> Jan

Thanks,
Shawn