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 v2 07/10] arm/mm: Get rid of READ/WRITE_SYSREG32
Date: Tue, 27 Apr 2021 10:59:07 +0100 [thread overview]
Message-ID: <04adaf9b-e2fe-460e-35e4-09e5e5ff7b6f@xen.org> (raw)
In-Reply-To: <20210427093546.30703-8-michal.orzel@arm.com>
On 27/04/2021 10:35, Michal Orzel wrote:
> AArch64 registers are 64bit whereas AArch32 registers
> 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 registers have upper 32bit reserved
> it does not mean that they can't be widen in the future.
>
> Modify SCTLR_EL2 accesses to use READ/WRITE_SYSREG.
>
> SCTLR_EL2 already has bits defined in the range [32:63].
> The ARM ARM defines them as unknown if implemented.
This is a bit ambiguous.
> By writing in head.S SCTLR_EL2_SET we are zeroing the upper
> 32bit half which is correct but referring to this sysreg
> as 32bit is a latent bug because the top 32bit was not used
> by Xen.
This seems to suggest the patch below will call SCTLR_EL2_SET whereas
this is already existing code.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> Changes since v1:
> -Update commit message with SCTLR_EL2 analysis
> ---
> xen/arch/arm/mm.c | 2 +-
> xen/arch/arm/traps.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 59f8a3f15f..0e07335291 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -613,7 +613,7 @@ void __init remove_early_mappings(void)
> */
> static void xen_pt_enforce_wnx(void)
> {
> - WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
> + WRITE_SYSREG(READ_SYSREG(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
> /*
> * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
> * before flushing the TLBs.
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index c7acdb2087..e7384381cc 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -915,7 +915,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
> printk(" VTTBR_EL2: %016"PRIx64"\n", ctxt->vttbr_el2);
> printk("\n");
>
> - printk(" SCTLR_EL2: %08"PRIx32"\n", READ_SYSREG32(SCTLR_EL2));
> + printk(" SCTLR_EL2: %"PRIregister"\n", READ_SYSREG(SCTLR_EL2));
Your commit title suggests that you will modify mm.c but you are also
modifying traps.c. So how about the following commit message:
"
xen/arm: Always access SCTLR_EL2 using {READ, WRITE}_SYSREG()
The Armv8 specification describes the system register as a 64-bit value
on AArch64 and 32-bit value on AArch32 (same as Armv7).
Unfortunately, Xen is accessing the system registers using {READ,
WRITE}_SYSREG32() which means the top 32-bit are clobbered.
This is only a latent bug so far because Xen will not yet use the top
32-bit.
There is also no change in behavior because arch/arm/arm64/head.S will
initialize SCTLR_EL2 to a sane value with the top 32-bit zeroed.
"
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2021-04-27 9:59 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-27 9:35 [PATCH v2 00/10] Get rid of READ/WRITE_SYSREG32 Michal Orzel
2021-04-27 9:35 ` [PATCH v2 01/10] arm64/vfp: " Michal Orzel
2021-04-27 9:35 ` [PATCH v2 02/10] arm/domain: " Michal Orzel
2021-04-27 9:45 ` Julien Grall
2021-04-29 6:58 ` Michal Orzel
2021-04-27 9:35 ` [PATCH v2 03/10] arm: Modify type of actlr to register_t Michal Orzel
2021-04-27 9:47 ` Julien Grall
2021-04-29 7:10 ` Michal Orzel
2021-04-27 9:35 ` [PATCH v2 04/10] arm/gic: Remove member hcr of structure gic_v3 Michal Orzel
2021-04-27 9:48 ` Julien Grall
2021-04-27 9:35 ` [PATCH v2 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32 Michal Orzel
2021-04-27 10:02 ` Julien Grall
2021-04-29 7:14 ` Michal Orzel
2021-04-27 9:35 ` [PATCH v2 06/10] arm/p2m: " Michal Orzel
2021-04-27 9:35 ` [PATCH v2 07/10] arm/mm: " Michal Orzel
2021-04-27 9:59 ` Julien Grall [this message]
2021-04-29 7:16 ` Michal Orzel
2021-04-27 9:35 ` [PATCH v2 08/10] arm/page: " Michal Orzel
2021-04-27 9:35 ` [PATCH v2 09/10] arm/time,vtimer: " Michal Orzel
2021-04-27 10:09 ` Julien Grall
2021-04-27 9:35 ` [PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t Michal Orzel
2021-04-27 11:09 ` Julien Grall
2021-04-29 8:53 ` Michal Orzel
2021-04-29 10:31 ` Tamas K Lengyel
2021-04-29 10:35 ` 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=04adaf9b-e2fe-460e-35e4-09e5e5ff7b6f@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).