Re: [PATCH 8/9] arm: Change type of hsr to register_t
Hi Michal, On 20/04/2021 08:08, Michal Orzel wrote: AArch64 system registers are 64bit whereas AArch32 ones are 32bit or 64bit. MSR/MRS are expecting 64bit values thus we should get rid of helpers READ/WRITE_SYSREG32 in favour of using READ/WRITE_SYSREG. We should also use register_t type when reading sysregs which can correspond to uint64_t or uint32_t. Even though many AArch64 sysregs have upper 32bit reserved it does not mean that they can't be widen in the future. Modify type of hsr to register_t. When on AArch64 add 32bit RES0 members to structures inside hsr union. Signed-off-by: Michal Orzel --- xen/arch/arm/arm64/entry.S| 2 +- xen/arch/arm/arm64/traps.c| 2 +- xen/arch/arm/arm64/vsysreg.c | 3 ++- xen/arch/arm/traps.c | 20 +--- xen/arch/arm/vcpreg.c | 13 +- xen/include/asm-arm/arm32/processor.h | 2 +- xen/include/asm-arm/arm64/processor.h | 5 ++-- xen/include/asm-arm/hsr.h | 34 ++- 8 files changed, 59 insertions(+), 22 deletions(-) diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S index ab9a65fc14..218b063c97 100644 --- a/xen/arch/arm/arm64/entry.S +++ b/xen/arch/arm/arm64/entry.S @@ -155,7 +155,7 @@ add x21, sp, #UREGS_CPSR mrs x22, spsr_el2 mrs x23, esr_el2 -stp w22, w23, [x21] +stp x22, x23, [x21] .endm diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c index babfc1d884..9113a15c7a 100644 --- a/xen/arch/arm/arm64/traps.c +++ b/xen/arch/arm/arm64/traps.c @@ -36,7 +36,7 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason) union hsr hsr = { .bits = regs->hsr }; printk("Bad mode in %s handler detected\n", handler[reason]); -printk("ESR=0x%08"PRIx32": EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n", +printk("ESR=%#"PRIregister": EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n", hsr.bits, hsr.ec, hsr.len, hsr.iss); local_irq_disable(); diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c index 41f18612c6..3c10d464e7 100644 --- a/xen/arch/arm/arm64/vsysreg.c +++ b/xen/arch/arm/arm64/vsysreg.c @@ -368,7 +368,8 @@ void do_sysreg(struct cpu_user_regs *regs, sysreg.op2, sysreg.read ? "=>" : "<=", sysreg.reg, regs->pc); -gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access %#x\n", +gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access" + " %#"PRIregister"\n", hsr.bits & HSR_SYSREG_REGS_MASK); inject_undef_exception(regs, hsr); return; diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index e7384381cc..db15a2c647 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -546,7 +546,7 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len) PSR_IRQ_MASK | PSR_DBG_MASK; regs->pc = handler; -WRITE_SYSREG32(esr.bits, ESR_EL1); +WRITE_SYSREG(esr.bits, ESR_EL1); } /* Inject an abort exception into a 64 bit guest */ @@ -580,7 +580,7 @@ static void inject_abt64_exception(struct cpu_user_regs *regs, regs->pc = handler; WRITE_SYSREG(addr, FAR_EL1); -WRITE_SYSREG32(esr.bits, ESR_EL1); +WRITE_SYSREG(esr.bits, ESR_EL1); } static void inject_dabt64_exception(struct cpu_user_regs *regs, @@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs, printk(" HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2)); printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2)); printk("\n"); -printk(" ESR_EL2: %08"PRIx32"\n", regs->hsr); +printk(" ESR_EL2: %"PRIregister"\n", regs->hsr); printk(" HPFAR_EL2: %"PRIregister"\n", READ_SYSREG(HPFAR_EL2)); #ifdef CONFIG_ARM_32 @@ -2004,13 +2004,15 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, break; default: -gprintk(XENLOG_WARNING, "Unsupported FSC: HSR=%#x DFSC=%#x\n", +gprintk(XENLOG_WARNING, "Unsupported FSC:" +" HSR=%#"PRIregister" DFSC=%#x\n", Please don't split the message in two. This makes more difficult to grep bits of the message in the log. Instead the code should be: gprintk(XENLOG_WARNING, "mystring", ...); For this case, we are tolerated to go past the 80 characters mark. hsr.bits, xabt.fsc); } inject_abt: -gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr - " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, gva, gpa); +gdprintk(XENLOG_DEBUG, "HSR=%#"PRIregister" pc=%#"PRIregister"" + " gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n", Same here. + hsr.bits, regs->pc, gva, gpa); if ( is_data ) inject_dabt_exception(regs, gva,
[PATCH 8/9] arm: Change type of hsr to register_t
AArch64 system registers are 64bit whereas AArch32 ones are 32bit or 64bit. MSR/MRS are expecting 64bit values thus we should get rid of helpers READ/WRITE_SYSREG32 in favour of using READ/WRITE_SYSREG. We should also use register_t type when reading sysregs which can correspond to uint64_t or uint32_t. Even though many AArch64 sysregs have upper 32bit reserved it does not mean that they can't be widen in the future. Modify type of hsr to register_t. When on AArch64 add 32bit RES0 members to structures inside hsr union. Signed-off-by: Michal Orzel --- xen/arch/arm/arm64/entry.S| 2 +- xen/arch/arm/arm64/traps.c| 2 +- xen/arch/arm/arm64/vsysreg.c | 3 ++- xen/arch/arm/traps.c | 20 +--- xen/arch/arm/vcpreg.c | 13 +- xen/include/asm-arm/arm32/processor.h | 2 +- xen/include/asm-arm/arm64/processor.h | 5 ++-- xen/include/asm-arm/hsr.h | 34 ++- 8 files changed, 59 insertions(+), 22 deletions(-) diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S index ab9a65fc14..218b063c97 100644 --- a/xen/arch/arm/arm64/entry.S +++ b/xen/arch/arm/arm64/entry.S @@ -155,7 +155,7 @@ add x21, sp, #UREGS_CPSR mrs x22, spsr_el2 mrs x23, esr_el2 -stp w22, w23, [x21] +stp x22, x23, [x21] .endm diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c index babfc1d884..9113a15c7a 100644 --- a/xen/arch/arm/arm64/traps.c +++ b/xen/arch/arm/arm64/traps.c @@ -36,7 +36,7 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason) union hsr hsr = { .bits = regs->hsr }; printk("Bad mode in %s handler detected\n", handler[reason]); -printk("ESR=0x%08"PRIx32": EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n", +printk("ESR=%#"PRIregister": EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n", hsr.bits, hsr.ec, hsr.len, hsr.iss); local_irq_disable(); diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c index 41f18612c6..3c10d464e7 100644 --- a/xen/arch/arm/arm64/vsysreg.c +++ b/xen/arch/arm/arm64/vsysreg.c @@ -368,7 +368,8 @@ void do_sysreg(struct cpu_user_regs *regs, sysreg.op2, sysreg.read ? "=>" : "<=", sysreg.reg, regs->pc); -gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access %#x\n", +gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access" + " %#"PRIregister"\n", hsr.bits & HSR_SYSREG_REGS_MASK); inject_undef_exception(regs, hsr); return; diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index e7384381cc..db15a2c647 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -546,7 +546,7 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len) PSR_IRQ_MASK | PSR_DBG_MASK; regs->pc = handler; -WRITE_SYSREG32(esr.bits, ESR_EL1); +WRITE_SYSREG(esr.bits, ESR_EL1); } /* Inject an abort exception into a 64 bit guest */ @@ -580,7 +580,7 @@ static void inject_abt64_exception(struct cpu_user_regs *regs, regs->pc = handler; WRITE_SYSREG(addr, FAR_EL1); -WRITE_SYSREG32(esr.bits, ESR_EL1); +WRITE_SYSREG(esr.bits, ESR_EL1); } static void inject_dabt64_exception(struct cpu_user_regs *regs, @@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs, printk(" HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2)); printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2)); printk("\n"); -printk(" ESR_EL2: %08"PRIx32"\n", regs->hsr); +printk(" ESR_EL2: %"PRIregister"\n", regs->hsr); printk(" HPFAR_EL2: %"PRIregister"\n", READ_SYSREG(HPFAR_EL2)); #ifdef CONFIG_ARM_32 @@ -2004,13 +2004,15 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, break; default: -gprintk(XENLOG_WARNING, "Unsupported FSC: HSR=%#x DFSC=%#x\n", +gprintk(XENLOG_WARNING, "Unsupported FSC:" +" HSR=%#"PRIregister" DFSC=%#x\n", hsr.bits, xabt.fsc); } inject_abt: -gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr - " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, gva, gpa); +gdprintk(XENLOG_DEBUG, "HSR=%#"PRIregister" pc=%#"PRIregister"" + " gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n", + hsr.bits, regs->pc, gva, gpa); if ( is_data ) inject_dabt_exception(regs, gva, hsr.len); else @@ -2204,7 +2206,8 @@ void do_trap_guest_sync(struct cpu_user_regs *regs) default: gprintk(XENLOG_WARNING, -"Unknown Guest Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n", +"Unknown Guest Trap. HSR=%#"PRIregister" EC=0x%x IL=%x" +" Syndrome=0x%"PRIx32"\n", hsr.bits, hsr.ec, hsr.len, hsr.iss);