On 8/1/23 6:19 AM, Jan Beulich wrote: > On 28.07.2023 23:35, Shawn Anastasio wrote: >> --- a/xen/arch/ppc/include/asm/asm-defns.h >> +++ b/xen/arch/ppc/include/asm/asm-defns.h >> @@ -23,6 +23,18 @@ >> addis reg,%r2,name@toc@ha; >> \ >> addi reg,reg,name@toc@l > > Noticing only now, because of the issue ... > >> +/* >> + * 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; >> \ >> + .localentry name,.-name > > ... being widened - could we gain blanks after the commas? Down here > (to match the code in context) another padding blank after "addi" > would also be nice.
Sure, will do in v2. >> --- a/xen/arch/ppc/opal.c >> +++ b/xen/arch/ppc/opal.c >> @@ -8,9 +8,28 @@ >> #include <xen/init.h> >> #include <xen/lib.h> >> >> -/* Global OPAL struct containing entrypoint and base */ >> +/* Global OPAL struct containing entrypoint and base used by opal-calls.S */ >> struct opal opal; >> >> +int64_t opal_console_write(int64_t term_number, uint64_t *length, >> + uint8_t *buffer); > > Would this perhaps better be void *, eliminating the need for casting > in calls of this function? I made it uint8_t to match the official OPAL API documentation (though I now see I missed a `const`): https://open-power.github.io/skiboot/doc/opal-api/opal-console-read-write-1-2.html#opal-console-write In this case though, the type information of this parameter might not be that important and changing it to void* to avoid the cast is fine with me. >> +int64_t opal_console_flush(int64_t term_number); >> +int64_t opal_reinit_cpus(uint64_t flags); >> + >> +static void opal_putchar(char c) > > Can't this be __init? Unlike OpenFirmware, OPAL calls are expected to be used by the OS during its entire lifecycle, not just during early boot, so the full (non-early) serial console driver would likely want to use these functions as well. > >> +{ >> + uint64_t len; >> + if (c == '\n') > > Nit: Blank line please between declaration(s) and statement(s). (At > least one more instance below.) Will fix. > Also please add the missing blanks in the if(), seeing that otherwise > the file is aiming at being Xen style. Ditto. >> + { >> + char buf = '\r'; >> + len = cpu_to_be64(1); >> + opal_console_write(0, &len, (uint8_t *) &buf); >> + } >> + len = cpu_to_be64(1); >> + opal_console_write(0, &len, (uint8_t *) &c); >> + opal_console_flush(0); >> +} >> + >> void __init boot_opal_init(const void *fdt) >> { >> int opal_node; >> @@ -45,4 +64,10 @@ void __init boot_opal_init(const void *fdt) >> >> opal.base = be64_to_cpu(*opal_base); >> opal.entry = be64_to_cpu(*opal_entry); >> + >> + early_printk_init(opal_putchar); >> + >> + /* Ask OPAL to set HID0 for Little Endian interrupts + Radix TLB >> support */ >> + opal_reinit_cpus(OPAL_REINIT_CPUS_HILE_LE | OPAL_REINIT_CPUS_MMU_RADIX >> + | OPAL_REINIT_CPUS_MMU_HASH); > > Nit: operators on continued lines go at the end of the earlier line. Will fix. >> --- /dev/null >> +++ b/xen/arch/ppc/ppc64/opal-calls.S >> @@ -0,0 +1,82 @@ >> +/* 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/asm-offsets.h> > > Would it make sense to have asm-defns.h include asm-offsets.h, like > x86 and Arm do? Sure, I'll make this change along with the formatting updates in asm-defns.h >> +#include <asm/opal-api.h> >> +#include <asm/msr.h> >> + >> + .text > > Is any of this code still needed post-init? Yes, see above. >> +#define OPAL_CALL(name, token) \ >> + .globl name; \ >> +name: \ >> + li %r0, token; \ >> + b opal_call; > > I think the trailing semicolon wants omitting. Will fix. > Jan Thanks, Shawn
