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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 6/21/23 3:48 PM, Julien Grall wrote:
> Hi Shawn,
> 
> Below some remark about the coding style. I will try to spot all of them
> so please go through your code and check if my comments applies in other
> places.

Thank you for the detailed comments. I had a couple of follow-up
questions for you.

> On 21/06/2023 17:59, Shawn Anastasio wrote:
>> diff --git a/xen/arch/ppc/boot-of.c b/xen/arch/ppc/boot-of.c
>> new file mode 100644
>> index 0000000000..1ceeaf1250
>> --- /dev/null
>> +++ b/xen/arch/ppc/boot-of.c
>> @@ -0,0 +1,116 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 
>> 02110-1301, USA.
> 
> As you already have the SPDX license, the full license should be dropped.

To clarify, you're suggesting that I remove the license text but keep
the copyright lines below? I wouldn't feel comfortable removing the
latter since this is derived from code that wasn't written by me.

>> + *
>> + * Copyright IBM Corp. 2005, 2006, 2007
>> + * Copyright Raptor Engineering, LLC
>> + *
>> + * Authors: Jimi Xenidis <jimix@xxxxxxxxxxxxxx>
>> + *          Hollis Blanchard <hollisb@xxxxxxxxxx>
>> + *          Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>> + */

>> +extern int enter_of(struct of_service *args, unsigned long entry);
> 
> Here you add 'extern' but ...
> 
>> +
>> +void boot_of_init(unsigned long);
> 
> not here. In Xen, we tend to not add 'extern' for prototypes. Also,
> please name the parameter as this makes clear what the value is. This
> would also make MISRA happy (IIRC this would rule 8.2).

I used extern to mark the `enter_of` since it's an assembly function
rather than a C one, but if this isn't a convention used in the Xen
codebase I can drop it.

>> diff --git a/xen/arch/ppc/include/asm/bug.h
>> b/xen/arch/ppc/include/asm/bug.h
>> new file mode 100644
>> index 0000000000..a23ab1fa74
>> --- /dev/null
>> +++ b/xen/arch/ppc/include/asm/bug.h
>> @@ -0,0 +1,6 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _ASM_PPC_BUG_H
>> +#define _ASM_PPC_BUG_H
>> +
>> +#endif /* _ASM_PPC_BUG_H */
> 
> Can you clarify why you do need this empty header?

Some empty headers were necessary to include <xen/lib.h> which in turn
includes various asm/ headers. I have since dropped many of these
headers following Andrew's earlier comments, though, and they won't be
present in v2.

>> diff --git a/xen/arch/ppc/ppc64/head.S b/xen/arch/ppc/ppc64/head.S
>> index 2fcefb40d8..589c72e382 100644
>> --- a/xen/arch/ppc/ppc64/head.S
>> +++ b/xen/arch/ppc/ppc64/head.S
>> @@ -1,30 +1,34 @@
>>   /* SPDX-License-Identifier: GPL-2.0-or-later */
>>   +#include <asm/processor.h>
>> +
>>       .section .text.header, "ax", %progbits
>>     ENTRY(start)
>> -    /*
>> -     * Depending on how we were booted, the CPU could be running in
>> either
>> -     * Little Endian or Big Endian mode. The following trampoline
>> from Linux
>> -     * cleverly uses an instruction that encodes to a NOP if the CPU's
>> -     * endianness matches the assumption of the assembler (LE, in our
>> case)
>> -     * or a branch to code that performs the endian switch in the
>> other case.
>> -     */
>> -    tdi 0, 0, 0x48    /* Reverse endian of b . + 8          */
>> -    b . + 44          /* Skip trampoline if endian is good  */
>> -    .long 0xa600607d  /* mfmsr r11                          */
>> -    .long 0x01006b69  /* xori r11,r11,1                     */
>> -    .long 0x00004039  /* li r10,0                           */
>> -    .long 0x6401417d  /* mtmsrd r10,1                       */
>> -    .long 0x05009f42  /* bcl 20,31,$+4                      */
>> -    .long 0xa602487d  /* mflr r10                           */
>> -    .long 0x14004a39  /* addi r10,r10,20                    */
>> -    .long 0xa6035a7d  /* mtsrr0 r10                         */
>> -    .long 0xa6037b7d  /* mtsrr1 r11                         */
>> -    .long 0x2400004c  /* rfid                               */
> 
> This seems to be refactoring. It would be best if this is done in a
> separate patch as this is easier to review.

Following Andrew's suggestion earlier, I've split this patch into two
with the first one only doing the bare minimum to get an infinite loop
going in C rather than assembly. That first patch still includes the
refactoring of this trampoline into a macro, but the overall patch size
is much smaller. Do you think that would be fine to review, or would you
like a third commit for only this trampoline's refactoring?

> Cheers,

Thanks,
Shawn