All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Early exception handlers on Power
@ 2023-10-13 18:13 Shawn Anastasio
  2023-10-13 18:13 ` [PATCH v2 1/2] xen/ppc: Add .text.exceptions section for exception vectors Shawn Anastasio
  2023-10-13 18:13 ` [PATCH v2 2/2] xen/ppc: Implement a basic exception handler Shawn Anastasio
  0 siblings, 2 replies; 8+ messages in thread
From: Shawn Anastasio @ 2023-10-13 18:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Timothy Pearson, Jan Beulich, Andrew Cooper, Shawn Anastasio

Hello all,

This series implements a basic exception handler and the required
support infrastructure on Power. Currently the handler just dumps all
registers to the serial console and nothing else, but even this is
useful for debugging during early bring-up.

Thanks,

Shawn Anastasio (2):
  xen/ppc: Add .text.exceptions section for exception vectors
  xen/ppc: Implement a basic exception handler

 xen/arch/ppc/include/asm/config.h    |   3 +
 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 +++
 xen/arch/ppc/xen.lds.S               |   7 ++
 8 files changed, 286 insertions(+)
 create mode 100644 xen/arch/ppc/ppc64/exceptions-asm.S
 create mode 100644 xen/arch/ppc/ppc64/exceptions.c

--
2.30.2



^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v2 1/2] xen/ppc: Add .text.exceptions section for exception vectors
  2023-10-13 18:13 [PATCH v2 0/2] Early exception handlers on Power Shawn Anastasio
@ 2023-10-13 18:13 ` Shawn Anastasio
  2023-10-18 15:20   ` Jan Beulich
  2023-10-13 18:13 ` [PATCH v2 2/2] xen/ppc: Implement a basic exception handler Shawn Anastasio
  1 sibling, 1 reply; 8+ messages in thread
From: Shawn Anastasio @ 2023-10-13 18:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Timothy Pearson, Jan Beulich, Andrew Cooper, Shawn Anastasio

On Power, the exception vectors must lie at a fixed address, depending
on the state of the Alternate Interrupt Location (AIL) field of the
Logical Partition Control Register (LPCR). Create a .text.exceptions
section in the linker script at an address suitable for AIL=3 plus an
accompanying assertion to pave the way for implementing exception
support.

Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
v2:
  - Wrap _stext_exceptions definition with HIDDEN()
  - Drop unnecessary line-continuing backslash in ASSERT

 xen/arch/ppc/include/asm/config.h | 3 +++
 xen/arch/ppc/xen.lds.S            | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/xen/arch/ppc/include/asm/config.h b/xen/arch/ppc/include/asm/config.h
index a11a09c570..e012b75beb 100644
--- a/xen/arch/ppc/include/asm/config.h
+++ b/xen/arch/ppc/include/asm/config.h
@@ -42,6 +42,9 @@

 #define XEN_VIRT_START _AC(0xc000000000000000, UL)

+/* Fixed address for start of the section containing exception vectors */
+#define EXCEPTION_VECTORS_START _AC(0xc000000000000100, UL)
+
 #define VMAP_VIRT_START (XEN_VIRT_START + GB(1))
 #define VMAP_VIRT_SIZE  GB(1)

diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
index 9e46035155..1479ec9f9f 100644
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -24,6 +24,10 @@ SECTIONS
         _stext = .;            /* Text section */
         *(.text.header)

+        . = ALIGN(256);
+        HIDDEN(_stext_exceptions = .);
+        *(.text.exceptions)
+
         *(.text.cold)
         *(.text.unlikely .text.*_unlikely .text.unlikely.*)

@@ -184,3 +188,6 @@ ASSERT(IS_ALIGNED(__bss_end,        POINTER_ALIGN), "__bss_end is misaligned")

 ASSERT(!SIZEOF(.got),      ".got non-empty")
 ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
+
+ASSERT(_stext_exceptions == EXCEPTION_VECTORS_START,
+       ".text.exceptions not at expected location -- .text.header too big?");
--
2.30.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH v2 2/2] xen/ppc: Implement a basic exception handler
  2023-10-13 18:13 [PATCH v2 0/2] Early exception handlers on Power Shawn Anastasio
  2023-10-13 18:13 ` [PATCH v2 1/2] xen/ppc: Add .text.exceptions section for exception vectors Shawn Anastasio
@ 2023-10-13 18:13 ` Shawn Anastasio
  2023-10-18 15:43   ` Jan Beulich
  1 sibling, 1 reply; 8+ messages in thread
From: Shawn Anastasio @ 2023-10-13 18:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Timothy Pearson, Jan Beulich, Andrew Cooper, Shawn Anastasio

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

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");
--
2.30.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] xen/ppc: Add .text.exceptions section for exception vectors
  2023-10-13 18:13 ` [PATCH v2 1/2] xen/ppc: Add .text.exceptions section for exception vectors Shawn Anastasio
@ 2023-10-18 15:20   ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2023-10-18 15:20 UTC (permalink / raw)
  To: Shawn Anastasio; +Cc: Timothy Pearson, Andrew Cooper, xen-devel

On 13.10.2023 20:13, Shawn Anastasio wrote:
> On Power, the exception vectors must lie at a fixed address, depending
> on the state of the Alternate Interrupt Location (AIL) field of the
> Logical Partition Control Register (LPCR). Create a .text.exceptions
> section in the linker script at an address suitable for AIL=3 plus an
> accompanying assertion to pave the way for implementing exception
> support.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>

Acked-by: Jan Beulich <jbeulich@suse.com>




^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] xen/ppc: Implement a basic exception handler
  2023-10-13 18:13 ` [PATCH v2 2/2] xen/ppc: Implement a basic exception handler Shawn Anastasio
@ 2023-10-18 15:43   ` Jan Beulich
  2023-10-19 20:02     ` Shawn Anastasio
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2023-10-18 15:43 UTC (permalink / raw)
  To: Shawn Anastasio; +Cc: Timothy Pearson, Andrew Cooper, xen-devel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] xen/ppc: Implement a basic exception handler
  2023-10-18 15:43   ` Jan Beulich
@ 2023-10-19 20:02     ` Shawn Anastasio
  2023-10-20  6:22       ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: Shawn Anastasio @ 2023-10-19 20:02 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Timothy Pearson, Andrew Cooper, xen-devel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] xen/ppc: Implement a basic exception handler
  2023-10-19 20:02     ` Shawn Anastasio
@ 2023-10-20  6:22       ` Jan Beulich
  2023-10-25 22:30         ` Shawn Anastasio
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2023-10-20  6:22 UTC (permalink / raw)
  To: Shawn Anastasio; +Cc: Timothy Pearson, Andrew Cooper, xen-devel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] xen/ppc: Implement a basic exception handler
  2023-10-20  6:22       ` Jan Beulich
@ 2023-10-25 22:30         ` Shawn Anastasio
  0 siblings, 0 replies; 8+ messages in thread
From: Shawn Anastasio @ 2023-10-25 22:30 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Timothy Pearson, Andrew Cooper, xen-devel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-10-25 22:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-13 18:13 [PATCH v2 0/2] Early exception handlers on Power Shawn Anastasio
2023-10-13 18:13 ` [PATCH v2 1/2] xen/ppc: Add .text.exceptions section for exception vectors Shawn Anastasio
2023-10-18 15:20   ` Jan Beulich
2023-10-13 18:13 ` [PATCH v2 2/2] xen/ppc: Implement a basic exception handler Shawn Anastasio
2023-10-18 15:43   ` Jan Beulich
2023-10-19 20:02     ` Shawn Anastasio
2023-10-20  6:22       ` Jan Beulich
2023-10-25 22:30         ` Shawn Anastasio

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.