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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2023 00:41, 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>

Despite me being unhappy about the disconnect of the constants
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
One further remark/suggestion though (happy to take care of while
committing):

> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/exceptions-asm.S
> @@ -0,0 +1,135 @@
> +/* 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. TODO: The value of %r1 has already been modified by 
> the
> +     * ISR, so the value we save isn't the exact value we had on entry.
> +     */
> +    SAVE_GPRS(1, 31, %r1)

Wouldn't this 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)

... better be repeated here?

Jan