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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/09/2023 12:19 am, Shawn Anastasio wrote:
> Implement a basic exception handler that dumps the CPU state to the
> console, as well as the code required to set the correct exception
> vector table's base address in setup.c.
>
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  xen/arch/ppc/include/asm/processor.h |  31 +++++++
>  xen/arch/ppc/ppc64/Makefile          |   2 +
>  xen/arch/ppc/ppc64/asm-offsets.c     |   1 +
>  xen/arch/ppc/ppc64/exceptions-asm.S  | 122 +++++++++++++++++++++++++++
>  xen/arch/ppc/ppc64/exceptions.c      | 102 ++++++++++++++++++++++
>  xen/arch/ppc/setup.c                 |  11 +++
>  6 files changed, 269 insertions(+)
>  create mode 100644 xen/arch/ppc/ppc64/exceptions-asm.S
>  create mode 100644 xen/arch/ppc/ppc64/exceptions.c
>
> diff --git a/xen/arch/ppc/include/asm/processor.h 
> b/xen/arch/ppc/include/asm/processor.h
> index d3dd943c20..a01b62b8a4 100644
> --- a/xen/arch/ppc/include/asm/processor.h
> +++ b/xen/arch/ppc/include/asm/processor.h
> @@ -103,6 +103,37 @@
>  #define PVR_BE        0x0070
>  #define PVR_PA6T      0x0090
>  
> +/* Exception Definitions */
> +#define EXC_SYSTEM_RESET    0x0100 /* System Reset Interrupt */
> +#define EXC_MACHINE_CHECK   0x0200 /* Machine Check Interrupt */
> +#define EXC_DATA_STORAGE    0x0300 /* Data Storage Interrupt */
> +#define EXC_DATA_SEGMENT    0x0380 /* Data Segment Interrupt */
> +#define EXC_INSN_STORAGE    0x0400 /* Instruction Storage Interrupt */
> +#define EXC_INSN_SEGMENT    0x0480 /* Instruction Segment Interrupt */
> +#define EXC_EXTERNAL        0x0500 /* External Interrupt */
> +#define EXC_ALIGNMENT       0x0600 /* Alignment Interrupt */
> +#define EXC_PROGRAM         0x0700 /* Program Interrupt */
> +#define EXC_FPU_UNAVAIL     0x0800 /* Floating-Point Unavailable Interrupt */
> +#define EXC_DECREMENTER     0x0900 /* Decrementer Interrupt */
> +#define EXC_H_DECREMENTER   0x0980 /* Hypervisor Decrementer Interrupt */
> +#define EXC_PRIV_DOORBELL   0x0A00 /* Directed Privileged Doorbell Interrupt 
> */
> +#define EXC_SYSTEM_CALL     0x0C00 /* System Call Interrupt */
> +#define EXC_TRACE           0x0D00 /* Trace Interrupt */
> +#define EXC_H_DATA_STORAGE  0x0E00 /* Hypervisor Data Storage Interrupt */
> +#define EXC_H_INSN_STORAGE  0x0E20 /* Hypervisor Instruction Storage 
> Interrupt */
> +#define EXC_H_EMUL_ASST     0x0E40 /* Hypervisor Emulation Assistance 
> Interrupt */
> +#define EXC_H_MAINTENANCE   0x0E60 /* Hypervisor Maintenance Interrupt */
> +#define EXC_H_DOORBELL      0x0E80 /* Directed Hypervisor Doorbell Interrupt 
> */
> +#define EXC_H_VIRT          0x0EA0 /* Hypervisor Virtualization Interrupt */
> +#define EXC_PERF_MON        0x0F00 /* Performance Monitor Interrupt */
> +#define EXC_VECTOR_UNAVAIL  0x0F20 /* Vector Unavailable Interrupt */
> +#define EXC_VSX_UNAVAIL     0x0F40 /* VSX Unavailable Interrupt */
> +#define EXC_FACIL_UNAVAIL   0x0F60 /* Facility Unavailable Interrupt */
> +#define EXC_H_FACIL_UNAVAIL 0x0F80 /* Hypervisor Facility Unavailable 
> Interrupt */
> +
> +/* Base address of interrupt vector table when LPCR[AIL]=3 */
> +#define AIL_VECTOR_BASE _AC(0xc000000000004000, UL)
> +
>  #ifndef __ASSEMBLY__
>  
>  #include <xen/types.h>
> diff --git a/xen/arch/ppc/ppc64/Makefile b/xen/arch/ppc/ppc64/Makefile
> index 5b88355bb2..914bb21c40 100644
> --- a/xen/arch/ppc/ppc64/Makefile
> +++ b/xen/arch/ppc/ppc64/Makefile
> @@ -1,2 +1,4 @@
> +obj-y += exceptions.o
> +obj-y += exceptions-asm.o
>  obj-y += head.o
>  obj-y += opal-calls.o
> diff --git a/xen/arch/ppc/ppc64/asm-offsets.c 
> b/xen/arch/ppc/ppc64/asm-offsets.c
> index c15c1bf136..634d7260e3 100644
> --- a/xen/arch/ppc/ppc64/asm-offsets.c
> +++ b/xen/arch/ppc/ppc64/asm-offsets.c
> @@ -46,6 +46,7 @@ void __dummy__(void)
>      OFFSET(UREGS_dsisr, struct cpu_user_regs, dsisr);
>      OFFSET(UREGS_cr, struct cpu_user_regs, cr);
>      OFFSET(UREGS_fpscr, struct cpu_user_regs, fpscr);
> +    OFFSET(UREGS_entry_vector, struct cpu_user_regs, entry_vector);
>      DEFINE(UREGS_sizeof, sizeof(struct cpu_user_regs));
>  
>      OFFSET(OPAL_base, struct opal, base);
> diff --git a/xen/arch/ppc/ppc64/exceptions-asm.S 
> b/xen/arch/ppc/ppc64/exceptions-asm.S
> new file mode 100644
> index 0000000000..877df97c9b
> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/exceptions-asm.S
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <asm/asm-defns.h>
> +#include <asm/processor.h>
> +
> +    /* 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)
> +
> +    /* 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)
> +    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

These two are almost identical, and differ only by the handling of
srr{0,1} as far as I can tell.

Is that just because the handler is fatal, or are they likely to stay
this similar longterm?

One thing you could do, which would remove the duplicated logic
constructing cpu_regs would be

exception_common_ssr
    mfsrr0  %r0
    std     %r0, UREGS_srr0(%r1)
    mfsrr1  %r0
    std     %r0, UREGS_srr1(%r1)
    b exception_common

exception_common_clobber_ssr
    li      %r0, -1 /* OS's SRR0/SRR1 have been clobbered */
    std     %r0, UREGS_srr0(%r1)
    std     %r0, UREGS_srr1(%r1)
    b exception_common

but this only works if the internal differences aren't going to get larger.

> +
> +/*
> + * Declare an ISR for the provided exception that jumps to `continue`
> + */
> +#define DEFINE_ISR(name, exc, continue)                                      
>   \
> +    . = (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       (continue);                                                      
>   \
> +    .size name, . - name;                                                    
>   \
> +    .type name, %function;

This will be much nicer as an ASM macro rather than a preprocessor macro.

.macro ISR name, exc, handler
    ...
.endm

Everything inside can stay the same, but no need for ; or \, and a few
newlines go a long way for readability.


> +
> +    .section .text.exceptions, "ax", %progbits
> +
> +DEFINE_ISR(exc_sysreset, EXC_SYSTEM_RESET, exception_common)
> +DEFINE_ISR(exc_mcheck, EXC_MACHINE_CHECK, exception_common)
> +DEFINE_ISR(exc_dstore, EXC_DATA_STORAGE, exception_common)
> +DEFINE_ISR(exc_dsegment, EXC_DATA_SEGMENT, exception_common)
> +DEFINE_ISR(exc_istore, EXC_INSN_STORAGE, exception_common)
> +DEFINE_ISR(exc_isegment, EXC_INSN_SEGMENT, exception_common)
> +DEFINE_ISR(exc_extern, EXC_EXTERNAL, exception_common)
> +DEFINE_ISR(exc_align, EXC_ALIGNMENT, exception_common)
> +DEFINE_ISR(exc_program, EXC_PROGRAM, exception_common)
> +DEFINE_ISR(exc_fpu, EXC_FPU_UNAVAIL, exception_common)
> +DEFINE_ISR(exc_dec, EXC_DECREMENTER, exception_common)
> +DEFINE_ISR(exc_h_dec, EXC_H_DECREMENTER, h_exception_common)
> +/* EXC_PRIV_DOORBELL ... EXC_TRACE */
> +DEFINE_ISR(exc_h_dstore, EXC_H_DATA_STORAGE, h_exception_common)
> +DEFINE_ISR(exc_h_istore, EXC_H_INSN_STORAGE, h_exception_common)

It also makes this less cluttered, and I'd recommend tabulating it too.

~Andrew