| Message ID | dfd134a6c59f329b62d2ecfaa37d74d70f8d4d90.1697220184.git.sanastasio@raptorengineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Early exception handlers on Power | expand |
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
On 10/18/23 10:43 AM, Jan Beulich wrote: > 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? > This is indeed not a problem for now, but agreed that it warrants a comment. Will do. >> + /* 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? > Andrew also pointed this out and as I mentioned to him, I expect the two routines to diverge more significantly in the future, so I'd rather keep them separate even if in this early stage there's not much difference. >> + 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. > Will fix. >> +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? > Correct, the ordering does matter here. I'll add a 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? > Fair enough, I'll drop " Interrupt" from all the strings. >> + 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? > I personally prefer the prefixes, but it is definitely just a matter of taste. >> --- 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.) > AIL_VECTOR_BASE is indeed a magic address defined by the ISA for AIL=3. As for the second part of your question, I'm a bit confused as to what you're asking. The ISRs are placed at a position relative to the start of the .text.exceptions section (EXCEPTION_VECTORS_START), so Xen can be arbitrarily shuffled around in address space as long as EXCEPTION_VECTORS_START lies at or before AIL_VECTOR_BASE. > Also you only OR in LPCR_AIL_3 - is it guaranteed that the respective > field is zero when control is passed to Xen? > As AIL=3 corresponds to 0b11, the intial state of the AIL field is irrelevant and OR'ing will always yield the desired result. > Jan Thanks, Shawn
On 19.10.2023 22:02, Shawn Anastasio wrote: > On 10/18/23 10:43 AM, Jan Beulich wrote: >> On 13.10.2023 20:13, Shawn Anastasio wrote: >>> --- 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.) >> > > AIL_VECTOR_BASE is indeed a magic address defined by the ISA for AIL=3. > As for the second part of your question, I'm a bit confused as to what > you're asking. The ISRs are placed at a position relative to > the start of the .text.exceptions section (EXCEPTION_VECTORS_START), so > Xen can be arbitrarily shuffled around in address space as long as > EXCEPTION_VECTORS_START lies at or before AIL_VECTOR_BASE. Well, AIL_VECTOR_BASE is #define-d to a plain constant, not derived from EXCEPTION_VECTORS_START. In turn EXCEPTION_VECTORS_START is #define-d to a plain constant in patch 1, not derived from XEN_VIRT_START. Therefore moving Xen around would require to change (at least) 3 #define-s afaict. Jan
On 10/20/23 1:22 AM, Jan Beulich wrote: > On 19.10.2023 22:02, Shawn Anastasio wrote: >> On 10/18/23 10:43 AM, Jan Beulich wrote: >>> On 13.10.2023 20:13, Shawn Anastasio wrote: >>>> --- 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.) >>> >> >> AIL_VECTOR_BASE is indeed a magic address defined by the ISA for AIL=3. >> As for the second part of your question, I'm a bit confused as to what >> you're asking. The ISRs are placed at a position relative to >> the start of the .text.exceptions section (EXCEPTION_VECTORS_START), so >> Xen can be arbitrarily shuffled around in address space as long as >> EXCEPTION_VECTORS_START lies at or before AIL_VECTOR_BASE. > > Well, AIL_VECTOR_BASE is #define-d to a plain constant, not derived > from EXCEPTION_VECTORS_START. In turn EXCEPTION_VECTORS_START is > #define-d to a plain constant in patch 1, not derived from > XEN_VIRT_START. Therefore moving Xen around would require to change > (at least) 3 #define-s afaict. > AIL_VECTOR_BASE needs to be a plain constant since it's fixed by the ISA. EXCEPTION_VECTORS_START could be defined in terms of XEN_VIRT_START I suppose, but that would require introducing another plain constant for representing the offset of EXCEPTION_VECTORS_START from XEN_VIRT_START. I think the current approach is reasonable, especially since patch 1 introduces a linker assertion that ensures that EXCEPTION_VECTORS_START matches the actual location of _stext_exceptions, so if one was to change Xen's address and forgot to change the #define they'd get a build error. If you would prefer this to be handled in a different way though, I'm not opposed. > Jan Thanks, Shawn
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..a7a111463f --- /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) + + /* 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 + +/* + * 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 + + +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 diff --git a/xen/arch/ppc/ppc64/exceptions.c b/xen/arch/ppc/ppc64/exceptions.c new file mode 100644 index 0000000000..ad5ab545f0 --- /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"; + 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); + + die(); +} diff --git a/xen/arch/ppc/setup.c b/xen/arch/ppc/setup.c index 959c1454a0..101bdd8bb6 100644 --- 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); +} + void __init noreturn start_xen(unsigned long r3, unsigned long r4, unsigned long r5, unsigned long r6, unsigned long r7) @@ -26,6 +35,8 @@ void __init noreturn start_xen(unsigned long r3, unsigned long r4, boot_opal_init((void *)r3); } + setup_exceptions(); + setup_initial_pagetables(); early_printk("Hello, ppc64le!\n");
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@raptorengineering.com> --- v2: - Place {h_,}exception_common in .text.exceptions section - Use assembler macro instead of CPP macro for ISR definition - Tabulate ISR definitions 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 | 129 +++++++++++++++++++++++++++ xen/arch/ppc/ppc64/exceptions.c | 102 +++++++++++++++++++++ xen/arch/ppc/setup.c | 11 +++ 6 files changed, 277 insertions(+), 1 deletion(-) create mode 100644 xen/arch/ppc/ppc64/exceptions-asm.S create mode 100644 xen/arch/ppc/ppc64/exceptions.c -- 2.30.2