Re: [PATCH v2 2/2] xen/ppc: Implement a basic exception handler

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2023 20:13, Shawn Anastasio wrote:
> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/exceptions-asm.S
> @@ -0,0 +1,129 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <asm/asm-defns.h>
> +#include <asm/processor.h>
> +
> +    .section .text.exceptions, "ax", %progbits
> +
> +    /* Helper to dump CPU state to struct cpu_user_regs pointed to by r1. */
> +ENTRY(exception_common)
> +    /* Save GPRs 1-31 */
> +    SAVE_GPRS(1, 31, %r1)

This won't save the entry value of %r1, but the one that was already updated.
If this is not a problem, perhaps worth a comment?

> +    /* Save LR, CTR, CR */
> +    mflr    %r0
> +    std     %r0, UREGS_lr(%r1)
> +    mfctr   %r0
> +    std     %r0, UREGS_ctr(%r1)
> +    mfcr    %r0
> +    stw     %r0, UREGS_cr(%r1) /* 32-bit */
> +
> +    /* Save Exception Registers */
> +    mfsrr0  %r0
> +    std     %r0, UREGS_pc(%r1)
> +    mfsrr1  %r0
> +    std     %r0, UREGS_msr(%r1)
> +    mfdsisr %r0
> +    stw     %r0, UREGS_dsisr(%r1) /* 32-bit */
> +    mfdar   %r0
> +    std     %r0, UREGS_dar(%r1)
> +    li      %r0, -1 /* OS's SRR0/SRR1 have been clobbered */
> +    std     %r0, UREGS_srr0(%r1)
> +    std     %r0, UREGS_srr1(%r1)
> +
> +    /* Setup TOC and a stack frame then call C exception handler */
> +    mr      %r3, %r1
> +    bcl     20, 31, 1f
> +1:  mflr    %r12
> +    addis   %r2, %r12, .TOC.-1b@ha
> +    addi    %r2, %r2, .TOC.-1b@l
> +
> +    li      %r0, 0
> +    stdu    %r0, -STACK_FRAME_OVERHEAD(%r1)
> +    bl      exception_handler
> +
> +    .size exception_common, . - exception_common
> +    .type exception_common, %function
> +
> +    /* Same as exception_common, but for exceptions that set HSRR{0,1} */
> +ENTRY(h_exception_common)
> +    /* Save GPRs 1-31 */
> +    SAVE_GPRS(1, 31, %r1)
> +
> +    /* Save LR, CTR, CR */
> +    mflr    %r0
> +    std     %r0, UREGS_lr(%r1)
> +    mfctr   %r0
> +    std     %r0, UREGS_ctr(%r1)
> +    mfcr    %r0
> +    stw     %r0, UREGS_cr(%r1) /* 32-bit */
> +
> +    /* Save Exception Registers */
> +    mfhsrr0 %r0
> +    std     %r0, UREGS_pc(%r1)
> +    mfhsrr1 %r0
> +    std     %r0, UREGS_msr(%r1)
> +    mfsrr0  %r0
> +    std     %r0, UREGS_srr0(%r1)
> +    mfsrr1  %r0
> +    std     %r0, UREGS_srr1(%r1)

Except for these four lines the two functions look identical. Is it
really good to duplicate all of the rest of the code?

> +    mfdsisr %r0
> +    stw     %r0, UREGS_dsisr(%r1) /* 32-bit */
> +    mfdar   %r0
> +    std     %r0, UREGS_dar(%r1)
> +
> +    /* Setup TOC and a stack frame then call C exception handler */
> +    mr      %r3, %r1
> +    bcl     20, 31, 1f
> +1:  mflr    %r12
> +    addis   %r2, %r12, .TOC.-1b@ha
> +    addi    %r2, %r2, .TOC.-1b@l
> +
> +    li      %r0, 0
> +    stdu    %r0, -STACK_FRAME_OVERHEAD(%r1)
> +    bl      exception_handler
> +
> +    .size h_exception_common, . - h_exception_common
> +    .type h_exception_common, %function
> +
> +/*
> + * Declare an ISR for the provided exception that jumps to the specified 
> handler
> + */
> +.macro ISR name, exc, handler
> +    . = (AIL_VECTOR_BASE - EXCEPTION_VECTORS_START) + \exc
> +    ENTRY(\name)
> +    /* TODO: switch stack */
> +
> +    /* Reserve space for struct cpu_user_regs */
> +    subi    %r1, %r1, UREGS_sizeof
> +
> +    /* Save r0 immediately so we can use it as scratch space */
> +    SAVE_GPR(0, %r1)
> +
> +    /* Save exception vector number */
> +    li      %r0, \exc
> +    std     %r0, UREGS_entry_vector(%r1)
> +
> +    /* Branch to common code */
> +    b       \handler
> +
> +    .size \name, . - \name
> +    .type \name, %function
> +.endm
> +
> +

Nit: No double blank lines please.

> +ISR exc_sysreset,   EXC_SYSTEM_RESET,   exception_common
> +ISR exc_mcheck,     EXC_MACHINE_CHECK,  exception_common
> +ISR exc_dstore,     EXC_DATA_STORAGE,   exception_common
> +ISR exc_dsegment,   EXC_DATA_SEGMENT,   exception_common
> +ISR exc_istore,     EXC_INSN_STORAGE,   exception_common
> +ISR exc_isegment,   EXC_INSN_SEGMENT,   exception_common
> +ISR exc_extern,     EXC_EXTERNAL,       exception_common
> +ISR exc_align,      EXC_ALIGNMENT,      exception_common
> +ISR exc_program,    EXC_PROGRAM,        exception_common
> +ISR exc_fpu,        EXC_FPU_UNAVAIL,    exception_common
> +ISR exc_dec,        EXC_DECREMENTER,    exception_common
> +ISR exc_h_dec,      EXC_H_DECREMENTER,  h_exception_common
> +/* EXC_PRIV_DOORBELL ... EXC_TRACE */
> +ISR exc_h_dstore,   EXC_H_DATA_STORAGE, h_exception_common
> +ISR exc_h_istore,   EXC_H_INSN_STORAGE, h_exception_common

Aiui these are required to be in order, or else the assembler will choke.
Maybe worth another comment?

> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/exceptions.c
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include <xen/lib.h>
> +
> +#include <asm/processor.h>
> +
> +static const char *exception_name_from_vec(uint32_t vec)
> +{
> +    switch ( vec )
> +    {
> +    case EXC_SYSTEM_RESET:
> +        return "System Reset Interrupt";
> +    case EXC_MACHINE_CHECK:
> +        return "Machine Check Interrupt";
> +    case EXC_DATA_STORAGE:
> +        return "Data Storage Interrupt";
> +    case EXC_DATA_SEGMENT:
> +        return "Data Segment Interrupt";
> +    case EXC_INSN_STORAGE:
> +        return "Instruction Storage Interrupt";
> +    case EXC_INSN_SEGMENT:
> +        return "Instruction Segment Interrupt";
> +    case EXC_EXTERNAL:
> +        return "External Interrupt";
> +    case EXC_ALIGNMENT:
> +        return "Alignment Interrupt";
> +    case EXC_PROGRAM:
> +        return "Program Interrupt";
> +    case EXC_FPU_UNAVAIL:
> +        return "Floating-Point Unavailable Interrupt";
> +    case EXC_DECREMENTER:
> +        return "Decrementer Interrupt";
> +    case EXC_H_DECREMENTER:
> +        return "Hypervisor Decrementer Interrupt";
> +    case EXC_PRIV_DOORBELL:
> +        return "Directed Privileged Doorbell Interrupt";
> +    case EXC_SYSTEM_CALL:
> +        return "System Call Interrupt";
> +    case EXC_TRACE:
> +        return "Trace Interrupt";
> +    case EXC_H_DATA_STORAGE:
> +        return "Hypervisor Data Storage Interrupt";
> +    case EXC_H_INSN_STORAGE:
> +        return "Hypervisor Instruction Storage Interrupt";
> +    case EXC_H_EMUL_ASST:
> +        return "Hypervisor Emulation Assistance Interrupt";
> +    case EXC_H_MAINTENANCE:
> +        return "Hypervisor Maintenance Interrupt";
> +    case EXC_H_DOORBELL:
> +        return "Directed Hypervisor Doorbell Interrupt";
> +    case EXC_H_VIRT:
> +        return "Hypervisor Virtualization Interrupt";
> +    case EXC_PERF_MON:
> +        return "Performance Monitor Interrupt";
> +    case EXC_VECTOR_UNAVAIL:
> +        return "Vector Unavailable Interrupt";
> +    case EXC_VSX_UNAVAIL:
> +        return "VSX Unavailable Interrupt";
> +    case EXC_FACIL_UNAVAIL:
> +        return "Facility Unavailable Interrupt";
> +    case EXC_H_FACIL_UNAVAIL:
> +        return "Hypervisor Facility Unavailable Interrupt";

Every string here has Interrupt at the end - any chance of collapsing
all of them?

> +    default:
> +        return "(unknown)";
> +    }
> +}
> +
> +void exception_handler(struct cpu_user_regs *regs)
> +{
> +    /* TODO: this is currently only useful for debugging */
> +
> +    printk("UNRECOVERABLE EXCEPTION: %s (0x%04x)\n\n"
> +           "GPR 0-3   : 0x%016lx 0x%016lx 0x%016lx 0x%016lx\n"
> +           "GPR 4-7   : 0x%016lx 0x%016lx 0x%016lx 0x%016lx\n"
> +           "GPR 8-11  : 0x%016lx 0x%016lx 0x%016lx 0x%016lx\n"
> +           "GPR 12-15 : 0x%016lx 0x%016lx 0x%016lx 0x%016lx\n"
> +           "GPR 16-19 : 0x%016lx 0x%016lx 0x%016lx 0x%016lx\n"
> +           "GPR 20-23 : 0x%016lx 0x%016lx 0x%016lx 0x%016lx\n"
> +           "GPR 24-27 : 0x%016lx 0x%016lx 0x%016lx 0x%016lx\n"
> +           "GPR 28-31 : 0x%016lx 0x%016lx 0x%016lx 0x%016lx\n\n",
> +           exception_name_from_vec(regs->entry_vector), regs->entry_vector,
> +           regs->gprs[0], regs->gprs[1], regs->gprs[2], regs->gprs[3],
> +           regs->gprs[4], regs->gprs[5], regs->gprs[6], regs->gprs[7],
> +           regs->gprs[8], regs->gprs[9], regs->gprs[10], regs->gprs[11],
> +           regs->gprs[12], regs->gprs[13], regs->gprs[14], regs->gprs[15],
> +           regs->gprs[16], regs->gprs[17], regs->gprs[18], regs->gprs[19],
> +           regs->gprs[20], regs->gprs[21], regs->gprs[22], regs->gprs[23],
> +           regs->gprs[24], regs->gprs[25], regs->gprs[26], regs->gprs[27],
> +           regs->gprs[28], regs->gprs[29], regs->gprs[30], regs->gprs[31]);
> +    printk("LR        : 0x%016lx\n"
> +           "CTR       : 0x%016lx\n"
> +           "CR        : 0x%08x\n"
> +           "PC        : 0x%016lx\n"
> +           "MSR       : 0x%016lx\n"
> +           "SRR0      : 0x%016lx\n"
> +           "SRR1      : 0x%016lx\n"
> +           "DAR       : 0x%016lx\n"
> +           "DSISR     : 0x%08x\n",
> +           regs->lr, regs->ctr, regs->cr, regs->pc, regs->msr, regs->srr0,
> +           regs->srr1, regs->dar, regs->dsisr);

While surely a matter of taste, I'd still like to ask: In dumping like
this, are 0x prefixes (taking space) actually useful?

> --- a/xen/arch/ppc/setup.c
> +++ b/xen/arch/ppc/setup.c
> @@ -11,6 +11,15 @@
>  /* Xen stack for bringing up the first CPU. */
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE);
> 
> +void setup_exceptions(void)
> +{
> +    unsigned long lpcr;
> +
> +    /* Set appropriate interrupt location in LPCR */
> +    lpcr = mfspr(SPRN_LPCR);
> +    mtspr(SPRN_LPCR, lpcr | LPCR_AIL_3);
> +}

Please forgive my lack of PPC knowledge: There's no use of any address
here afaict. Where's the link to (presumably) AIL_VECTOR_BASE? If that
address is kind of magic, I'm then lacking a connection between
XEN_VIRT_START and that constant. (If Xen needed moving around in
address space, it would be nice if changing a single constant would
suffice.)

Also you only OR in LPCR_AIL_3 - is it guaranteed that the respective
field is zero when control is passed to Xen?

Jan