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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/06/2023 7:21 pm, Shawn Anastasio wrote:
> 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.

<xen/stdarg.h> should be entirely standalone.

But in general, it's nice to see if we can reduce the
inter-dependencies, and bringup of a new arch is the only time we get to
see them.

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

In the short term, that's perhaps best.

https://lore.kernel.org/xen-devel/cover.1653314499.git.lin.liu@xxxxxxxxxx/T/#u
is the full series

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

Whatever is easier, although I expect splitting this patch into two will
make things simpler overall.

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

You can pick a minimum version as you see fit, which is basically
anything goes prior to PPC64 being declared supported in Xen.

It needs to end up in the root README (but a patch for that probably
wants to wait until the port is a little more mature), but if I were you
I wouldn't care about supporting anything older than comes in current
distro released this year.

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

Ah, in which case there's something magic in the build system which is
(AFAIU) unique to Xen.

obj-y += foo.init.o

for a foo.o which are expected to contain no runtime code at all.  This
trick also manages to string literals into .init.rodata, rather than
living in the general mergable string section.

~Andrew