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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 6/21/23 12:54 PM, Andrew Cooper wrote:
> On 21/06/2023 5:59 pm, Shawn Anastasio wrote:
>>  xen/arch/ppc/Kconfig.debug              |   5 +
>>  xen/arch/ppc/Makefile                   |   3 +
>>  xen/arch/ppc/boot-of.c                  | 116 +++++++++++++
>>  xen/arch/ppc/configs/ppc64_defconfig    |   1 +
>>  xen/arch/ppc/early_printk.c             |  28 +++
>>  xen/arch/ppc/include/asm/boot.h         |  24 +++
>>  xen/arch/ppc/include/asm/bug.h          |   6 +
>>  xen/arch/ppc/include/asm/byteorder.h    |  37 ++++
>>  xen/arch/ppc/include/asm/cache.h        |   6 +
>>  xen/arch/ppc/include/asm/early_printk.h |  15 ++
>>  xen/arch/ppc/include/asm/msr.h          |  67 ++++++++
>>  xen/arch/ppc/include/asm/processor.h    | 207 ++++++++++++++++++++++
>>  xen/arch/ppc/include/asm/reg_defs.h     | 218 ++++++++++++++++++++++++
>>  xen/arch/ppc/include/asm/string.h       |   6 +
>>  xen/arch/ppc/include/asm/types.h        |  35 ++++
>>  xen/arch/ppc/ppc64/Makefile             |   1 +
>>  xen/arch/ppc/ppc64/asm-offsets.c        |  55 ++++++
>>  xen/arch/ppc/ppc64/head.S               |  48 +++---
>>  xen/arch/ppc/ppc64/of-call.S            |  85 +++++++++
>>  xen/arch/ppc/setup.c                    |  31 ++++
>>  20 files changed, 972 insertions(+), 22 deletions(-)
>>  create mode 100644 xen/arch/ppc/boot-of.c
>>  create mode 100644 xen/arch/ppc/early_printk.c
>>  create mode 100644 xen/arch/ppc/include/asm/boot.h
>>  create mode 100644 xen/arch/ppc/include/asm/bug.h
>>  create mode 100644 xen/arch/ppc/include/asm/byteorder.h
>>  create mode 100644 xen/arch/ppc/include/asm/cache.h
>>  create mode 100644 xen/arch/ppc/include/asm/early_printk.h
>>  create mode 100644 xen/arch/ppc/include/asm/msr.h
>>  create mode 100644 xen/arch/ppc/include/asm/processor.h
>>  create mode 100644 xen/arch/ppc/include/asm/reg_defs.h
>>  create mode 100644 xen/arch/ppc/include/asm/string.h
>>  create mode 100644 xen/arch/ppc/include/asm/types.h
>>  create mode 100644 xen/arch/ppc/ppc64/of-call.S
>>  create mode 100644 xen/arch/ppc/setup.c
> 
> This is a surprising amount of infrastructure.  I'm guessing it's a
> consequence of needing byteorder ?

Right, byteorder as well as va_{start,end,arg}. I could try to trim it
down further.

> There's a series still out deleting swathes of junk in byteorder.  I
> guess I need to kick that thread again, but it mostly boils down to
> using __builtin_bswap$N() (and on x86, reimplementing them on old enough
> compilers).  Presumably all versions of GCC (and eventually Clang) we
> care to support with ppc64 understand this builtin ?

Yes, those builtins should work just fine on any reasonably recent gcc
or clang toolchain. What would be the correct approach to integrating
these into byteorder.h? Would adding some `#define __arch_swab$N
__builtin_bswap$N` macros be the way to go?

> I've noticed a couple of other things.  asm/types.h repeats some
> antipatterns which we're trying to delete for MISRA reasons in other
> architectures.  I was already planning to fix that up xen-wide, and I
> guess now is the better time to do so.
> 
> Elsewhere, you've got a number of __inline__'s.  We think those are all
> vestigial now, so should be switched to using a plain inline.

Will do.

> Also, there are a bunch of UL() or ULL() macros which encoding a
> difference between asm and C.  In Xen, we use _AC() for that, which you
> can find in <xen/const.h>.

Thanks for the pointer, I'll update the usage of UL/ULL.

> Similarly, there are some functions which ought to be __init, so it
> would be good to get them correct from the start.

Good catch. This actually goes along with your subsequent observation
about Open Firmware residing in the bottom 4G of memory. See below.

> Maybe as an intermediate step, just get the infinite loop moved from asm
> up into C?  That gets the stack setup, and various of the asm helpers,
> without all the rest of the C required to get early_printk() to work.

Would you like that plus the serial patch in this same series, or would
you like me to just get the C infinite loop going for this series?

> Something we've been trying very hard to do generally is declutter
> processor.h, and on x86, we've got asm-defns.h as a more appropriate
> place to have the stuff which is expected to be common to all asm code,
> and never encountered in C.

Sounds good, I'll move the common asm-only stuff to an asm-defns.h.

> A couple of questions before I dive further in.
> 
> Given:
> 
> #define r0  0
> 
> do the assemblers really not understand register names?  That seems mad.

Yeah as surprising as it is, ppc64 assemblers don't handle register
names by default. I think at some point a flag was added to GAS (and
probably llvm? will have to check) to define them for you, but I'm not
sure which version introduced the flag.

I'll do some digging and if the flag is available a reasonable versions
of both toolchains (what exactly would constitute this? Are there
project-wide expectations of older toolchains working, and if so, how
old?) then I can get rid of these.

> Also, given the transformations to call into OpenFirmware, presumably
> this limits Xen to running below the 4G boundary and on identity mappings?

Interaction with Open Firmware directly shouldn't be necessary past
early init, so it shouldn't impose any restrictions on the memory map
once paging is up and going. It will just require grabbing all of the
required information from OF (essentially dumping the device tree into a
local copy) before making the switch.

I'll also mark the relevant functions as __init.

> ~Andrew

Thanks,
Shawn