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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 22/06/2023 17:05, Shawn Anastasio wrote:
> On 6/21/23 3:48 PM, Julien Grall wrote:
> > 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.
I am only suggesting to remove the license text. The copyright are fine 
to keep.
Note that in Xen, we don't tend to add them for new file (I guess this 
is not the case here) and instead rely on signed-off/author in the 
commit message.
> > > + *
> > > + * 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.
I am not aware of such convention in Xen. But if you want to distinguish 
assembly vs C function, then I think it would be better to add asm_ in 
the name so it is clearer.
> > > 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?
In general we tend to split code movement/refactoring from new code. 
This helps during reviews. Not sure how small will be your patch. If it 
is only a few dozen of lines, then it should be fine :).
Cheers,

--
Julien Grall