From: Julien Grall <julien@xen.org>
To: Michal Orzel <michal.orzel@arm.com>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
bertrand.marquis@arm.com
Subject: Re: [PATCH 8/9] arm: Change type of hsr to register_t
Date: Wed, 21 Apr 2021 20:02:36 +0100 [thread overview]
Message-ID: <ce44efef-db10-b000-3a11-46fbfdf4ccbb@xen.org> (raw)
In-Reply-To: <20210420070853.8918-9-michal.orzel@arm.com>
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 <michal.orzel@arm.com>
> ---
> 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, 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",
Same here.
> hsr.bits, hsr.ec, hsr.len, hsr.iss);
> inject_undef_exception(regs, hsr);
> }
> @@ -2242,7 +2245,8 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
> break;
> }
> default:
> - printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
> + printk("Hypervisor Trap. HSR=%#"PRIregister" EC=0x%x IL=%x"
> + " Syndrome=0x%"PRIx32"\n",
Same here.
> hsr.bits, hsr.ec, hsr.len, hsr.iss);
> do_unexpected_trap("Hypervisor", regs);
> }
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index 55351fc087..c7f516ee0a 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -385,7 +385,7 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
> cp32.read ? "mrc" : "mcr",
> cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#"PRIregister"\n",
> hsr.bits & HSR_CP32_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> @@ -454,7 +454,8 @@ void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> cp64.read ? "mrrc" : "mcrr",
> cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access" > + " %#"PRIregister"\n",
Same here.
> hsr.bits & HSR_CP64_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> @@ -585,7 +586,7 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
> cp32.read ? "mrc" : "mcr",
> cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 32-bit cp14 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 32-bit cp14 access %#"PRIregister"\n",
> hsr.bits & HSR_CP32_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> @@ -627,7 +628,7 @@ void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> cp64.read ? "mrrc" : "mcrr",
> cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 access %#"PRIregister"\n",
> hsr.bits & HSR_CP64_REGS_MASK);
> inject_undef_exception(regs, hsr);
> }
> @@ -658,7 +659,7 @@ void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> cp64.read ? "mrrc" : "mcrr",
> cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 DBG access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 DBG access %#"PRIregister"\n",
> hsr.bits & HSR_CP64_REGS_MASK);
>
> inject_undef_exception(regs, hsr);
> @@ -692,7 +693,7 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr hsr)
> "%s p10, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
> cp32.read ? "mrc" : "mcr",
> cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
> - gdprintk(XENLOG_ERR, "unhandled 32-bit CP10 access %#x\n",
> + gdprintk(XENLOG_ERR, "unhandled 32-bit CP10 access %#"PRIregister"\n",
> hsr.bits & HSR_CP32_REGS_MASK);
> inject_undef_exception(regs, hsr);
> return;
> diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
> index 4e679f3273..395ce10692 100644
> --- a/xen/include/asm-arm/arm32/processor.h
> +++ b/xen/include/asm-arm/arm32/processor.h
> @@ -37,7 +37,7 @@ struct cpu_user_regs
> uint32_t pc, pc32;
> };
> uint32_t cpsr; /* Return mode */
> - uint32_t hsr; /* Exception Syndrome */
> + register_t hsr; /* Exception Syndrome */
I would rather keep this one as uint32_t to stay consistent with the
rest of the structure. If you still want to switch it, then please
switch everything in cpu_user_regs.
>
> /* Outer guest frame only from here on... */
>
> diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
> index 81dfc5e615..40f628d216 100644
> --- a/xen/include/asm-arm/arm64/processor.h
> +++ b/xen/include/asm-arm/arm64/processor.h
> @@ -64,8 +64,9 @@ struct cpu_user_regs
> /* Return address and mode */
> __DECL_REG(pc, pc32); /* ELR_EL2 */
> uint32_t cpsr; /* SPSR_EL2 */
Above you use an STP instructions that will write 2 64-bit values: one
in CPSR, the other in HSR.
So this wants to be switched to uint64_t. I guess you didn't notice any
issue because there will thankfully be an implicit padding.
> - uint32_t hsr; /* ESR_EL2 */
> + register_t hsr; /* ESR_EL2 */
All the structure is using uint64_t rather than register_t. Could we do
the same here?
> > + register_t pad1; /* Doubleword-align the user half of the frame */
May I asked, why the padding is moved here rather than kept below?
> /* Outer guest frame only from here on... */
>
> union {
> @@ -73,8 +74,6 @@ struct cpu_user_regs
> uint32_t spsr_svc; /* AArch32 */
> };
>
> - uint32_t pad1; /* Doubleword-align the user half of the frame */
> -
I think this deserves an explanation in the commit message. However, I
believe this is going to corrupt spsr_fiq because we are writing a
64-bit value to spsr_el1 (see the macro entry_guest in
arch/arm/arm64/entry.S).
So the union likely want to be 64-bit.
> /* AArch32 guests only */
> uint32_t spsr_fiq, spsr_irq, spsr_und, spsr_abt;
>
> diff --git a/xen/include/asm-arm/hsr.h b/xen/include/asm-arm/hsr.h
> index 29d4531f40..7424402c6e 100644
> --- a/xen/include/asm-arm/hsr.h
> +++ b/xen/include/asm-arm/hsr.h
> @@ -16,11 +16,14 @@ enum dabt_size {
> };
>
> union hsr {
> - uint32_t bits;
> + register_t bits;
> struct {
> unsigned long iss:25; /* Instruction Specific Syndrome */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
Given that we don't define any useful, could we avoid the #ifdefery?
> };
>
> /* Common to all conditional exception classes (0x0N, except 0x00). */
> @@ -30,6 +33,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } cond;
>
> struct hsr_wfi_wfe {
> @@ -39,6 +45,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } wfi_wfe;
>
> /* reg, reg0, reg1 are 4 bits on AArch32, the fifth bit is sbzp. */
> @@ -53,6 +62,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } cp32; /* HSR_EC_CP15_32, CP14_32, CP10 */
>
> struct hsr_cp64 {
> @@ -66,6 +78,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } cp64; /* HSR_EC_CP15_64, HSR_EC_CP14_64 */
>
> struct hsr_cp {
> @@ -77,6 +92,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } cp; /* HSR_EC_CP */
>
> /*
> @@ -94,6 +112,9 @@ union hsr {
> unsigned long ccvalid:1;/* CC Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } smc32; /* HSR_EC_SMC32 */
>
> #ifdef CONFIG_ARM_64
> @@ -108,6 +129,7 @@ union hsr {
> unsigned long res0:3;
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6;
> + unsigned long _res0:32;
> } sysreg; /* HSR_EC_SYSREG */
> #endif
>
> @@ -121,6 +143,9 @@ union hsr {
> unsigned long res2:14;
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } iabt; /* HSR_EC_INSTR_ABORT_* */
>
> struct hsr_dabt {
> @@ -143,6 +168,9 @@ union hsr {
> unsigned long valid:1; /* Syndrome Valid */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } dabt; /* HSR_EC_DATA_ABORT_* */
>
> /* Contain the common bits between DABT and IABT */
> @@ -156,6 +184,9 @@ union hsr {
> unsigned long pad3:14; /* Not common */
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> +#ifdef CONFIG_ARM_64
> + unsigned long _res0:32;
> +#endif
> } xabt;
>
> #ifdef CONFIG_ARM_64
> @@ -164,6 +195,7 @@ union hsr {
> unsigned long res0:9;
> unsigned long len:1; /* Instruction length */
> unsigned long ec:6; /* Exception Class */
> + unsigned long _res0:32;
> } brk;
> #endif
> };
>
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2021-04-21 19:03 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-20 7:08 [PATCH 0/9] xen/arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
2021-04-20 7:08 ` [PATCH 1/9] arm64/vfp: " Michal Orzel
2021-04-20 13:04 ` Julien Grall
2021-04-20 7:08 ` [PATCH 2/9] arm/domain: " Michal Orzel
2021-04-20 13:12 ` Julien Grall
2021-04-21 7:36 ` Michal Orzel
2021-04-21 10:20 ` Julien Grall
2021-04-20 7:08 ` [PATCH 3/9] arm/gic: " Michal Orzel
2021-04-20 13:28 ` Julien Grall
2021-04-21 7:48 ` Michal Orzel
2021-04-21 10:21 ` Julien Grall
2021-04-20 7:08 ` [PATCH 4/9] arm/p2m: " Michal Orzel
2021-04-20 13:31 ` Julien Grall
2021-04-20 7:08 ` [PATCH 5/9] arm/mm: " Michal Orzel
2021-04-20 13:37 ` Julien Grall
2021-04-22 11:47 ` Michal Orzel
2021-04-22 13:40 ` Julien Grall
2021-04-20 7:08 ` [PATCH 6/9] arm/page: " Michal Orzel
2021-04-21 15:11 ` Julien Grall
2021-04-20 7:08 ` [PATCH 7/9] arm/time,vtimer: " Michal Orzel
2021-04-21 16:01 ` Julien Grall
2021-04-20 7:08 ` [PATCH 8/9] arm: Change type of hsr to register_t Michal Orzel
2021-04-21 19:02 ` Julien Grall [this message]
2021-04-20 7:08 ` [PATCH 9/9] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros Michal Orzel
2021-04-21 19:16 ` Julien Grall
2021-04-27 7:16 ` Michal Orzel
2021-04-27 8:30 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ce44efef-db10-b000-3a11-46fbfdf4ccbb@xen.org \
--to=julien@xen.org \
--cc=Volodymyr_Babchuk@epam.com \
--cc=bertrand.marquis@arm.com \
--cc=michal.orzel@arm.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.