[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 7/19/23 9:05 AM, Jan Beulich wrote:
> On 18.07.2023 22:20, Shawn Anastasio wrote:
>> +void __init boot_of_init(unsigned long vec)
>> +{
>> +    int bof_chosen;
>> +
>> +    of_vec = vec;
>> +
>> +    /* Get a handle to the default console */
>> +    bof_chosen = of_finddevice("/chosen");
>> +    of_getprop(bof_chosen, "stdout", &of_out, sizeof(of_out));
>> +    of_out = be32_to_cpu(of_out);
> 
> Can any of these fail, and hence lead to ...

These calls are allowed to fail, but their return value in those cases
is well-defined (an invalid handle), so...

> 
>> +    early_printk_init(of_putchar);
> 
> ... this better not getting invoked?

this being invoked is fine even in those cases. It will just result in
the invalid handle being passed to of_write and the firmware will refuse
to service the writes.

>> --- a/xen/arch/ppc/ppc64/asm-offsets.c
>> +++ b/xen/arch/ppc/ppc64/asm-offsets.c
>> @@ -0,0 +1,59 @@
>> +/*
>> + * Generate definitions needed by assembly language modules.
>> + * This code generates raw asm output which is post-processed
>> + * to extract and format the required data.
>> + */
>> +
>> +#include <asm/processor.h>
>> +
>> +#define DEFINE(_sym, _val)                                                 \
>> +    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
>> +                  : : "i" (_val) )
> 
> Nit: There's a blank missing after the opening paren, which will then want
> the 2nd line to be indented by one more character. (Instead, as a matter of
> your taste, you may omit the blank between the two colons.)

Will fix.

>> +#define BLANK()                                                            \
>> +    asm volatile ( "\n.ascii\"==><==\"" : : )
>> +#define OFFSET(_sym, _str, _mem)                                           \
>> +    DEFINE(_sym, offsetof(_str, _mem));
>> +
>> +/* base-2 logarithm */
>> +#define __L2(_x)  (((_x) & 0x00000002) ?   1 : 0)
>> +#define __L4(_x)  (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( _x))
>> +#define __L8(_x)  (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( _x))
>> +#define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( _x))
>> +#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
>> +
>> +void __dummy__(void)
>> +{
>> +    DEFINE(GPR_WIDTH, sizeof(unsigned long));
>> +    DEFINE(FPR_WIDTH, sizeof(double));
>> +
>> +    OFFSET(UREGS_gprs, struct cpu_user_regs, gprs);
>> +    OFFSET(UREGS_r0, struct cpu_user_regs, gprs[0]);
>> +    OFFSET(UREGS_r1, struct cpu_user_regs, gprs[1]);
>> +    OFFSET(UREGS_r13, struct cpu_user_regs, gprs[13]);
>> +    OFFSET(UREGS_srr0, struct cpu_user_regs, srr0);
>> +    OFFSET(UREGS_srr1, struct cpu_user_regs, srr1);
>> +    OFFSET(UREGS_pc, struct cpu_user_regs, pc);
>> +    OFFSET(UREGS_msr, struct cpu_user_regs, msr);
>> +    OFFSET(UREGS_lr, struct cpu_user_regs, lr);
>> +    OFFSET(UREGS_ctr, struct cpu_user_regs, ctr);
>> +    OFFSET(UREGS_xer, struct cpu_user_regs, xer);
>> +    OFFSET(UREGS_hid4, struct cpu_user_regs, hid4);
>> +    OFFSET(UREGS_dar, struct cpu_user_regs, dar);
>> +    OFFSET(UREGS_dsisr, struct cpu_user_regs, dsisr);
>> +    OFFSET(UREGS_cr, struct cpu_user_regs, cr);
>> +    OFFSET(UREGS_fpscr, struct cpu_user_regs, fpscr);
>> +    DEFINE(UREGS_sizeof, sizeof(struct cpu_user_regs));
>> +}
>> +
>> +/* TODO: Replace with BUILD_BUG_ON + IS_ALIGNED once we can use <xen/lib.h> 
>> */
>> +_Static_assert(sizeof(struct cpu_user_regs) % STACK_ALIGN == 0,
>> +               "struct cpu_user_regs not stack aligned!");
> 
> But patch 1 makes BUILD_BUG_ON() available now.

Good point, will fix.

>> --- /dev/null
>> +++ b/xen/arch/ppc/ppc64/of-call.S
>> @@ -0,0 +1,83 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Adapted from Linux's arch/powerpc/kernel/entry_64.S, with the
>> + * following copyright notice:
>> + *
>> + *  PowerPC version
>> + *    Copyright (C) 1995-1996 Gary Thomas (gdt@xxxxxxxxxxxx)
>> + *  Rewritten by Cort Dougan (cort@xxxxxxxxxx) for PReP
>> + *    Copyright (C) 1996 Cort Dougan <cort@xxxxxxxxxx>
>> + *  Adapted for Power Macintosh by Paul Mackerras.
>> + *  Low-level exception handlers and MMU support
>> + *  rewritten by Paul Mackerras.
>> + *    Copyright (C) 1996 Paul Mackerras.
>> + *  MPC8xx modifications Copyright (C) 1997 Dan Malek (dmalek@xxxxxxx).
>> + */
>> +
>> +#include <asm/asm-offsets.h>
>> +#include <asm/asm-defns.h>
>> +#include <asm/msr.h>
>> +
>> +/* size of minimum stack frame that can hold an entire cpu_user_regs struct 
>> */
>> +#define STACK_SWITCH_FRAME_SIZE UREGS_sizeof
>> +
>> +    .section .init.text, "ax", @progbits
>> +
>> +ENTRY(enter_of)
>> +    mflr %r0
>> +    std %r0, 16(%r1)
>> +    stdu %r1,-STACK_SWITCH_FRAME_SIZE(%r1) /* Save SP and create stack 
>> space */
> 
> Nit: A blank after the comma would again be nice.

Will fix.

>> +    /*
>> +     * Because PROM is running in 32b mode, it clobbers the high order half
>> +     * of all registers that it saves.  We therefore save those registers
>> +     * PROM might touch to the stack.  (%r0, %r3-%r13 are caller saved)
>> +     */
>> +    SAVE_GPR(2, %r1)
>> +    SAVE_GPR(13, %r1)
>> +    SAVE_NVGPRS(%r1)
>> +    mfcr %r10
>> +    mfmsr %r11
>> +    std %r10, UREGS_cr(%r1)
>> +    std %r11, UREGS_msr(%r1)
>> +
>> +    /* Put PROM address in SRR0 */
>> +    mtsrr0 %r4
>> +
>> +    /* Setup our trampoline return addr in LR */
>> +    bcl 20, 31, .+4
>> +0:  mflr %r4
>> +    addi %r4, %r4, 1f - 0b
>> +    mtlr %r4
>> +
>> +    /* Prepare a 32-bit mode big endian MSR */
>> +    LOAD_IMM64(%r12, MSR_SF | MSR_LE)
>> +    andc %r11, %r11, %r12
>> +    mtsrr1 %r11
>> +    rfid
>> +
>> +1:  /* Return from OF */
>> +    FIXUP_ENDIAN
>> +
>> +    /* Just make sure that %r1 top 32 bits didn't get corrupt by OF */
>> +    rldicl %r1, %r1, 0, 32
>> +
>> +    /* Restore the MSR (back to 64 bits) */
>> +    ld %r0, UREGS_msr(%r1)
>> +    mtmsrd %r0
>> +    isync
>> +
>> +    /* Restore other registers */
>> +    REST_GPR(2, %r1)
>> +    REST_GPR(13, %r1)
>> +    REST_NVGPRS(%r1)
>> +    ld %r4, UREGS_cr(%r1)
>> +    mtcr %r4
>> +
>> +    addi %r1, %r1, STACK_SWITCH_FRAME_SIZE
>> +    ld %r0, 16(%r1)
>> +    mtlr %r0
>> +    blr
>> +
>> +    .size enter_of, . - enter_of
>> +    .type enter_of, %function

Thanks,
Shawn



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.