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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 10/26/23 3:03 AM, Jan Beulich wrote:
> 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?
>

Sure, if you'd like to make that change during commit that'd be great.

> Jan

Thanks,
Shawn