xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
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>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Alexandru Isaila <aisaila@bitdefender.com>,
	Petre Pircalabu <ppircalabu@bitdefender.com>,
	bertrand.marquis@arm.com
Subject: Re: [PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
Date: Tue, 27 Apr 2021 12:09:41 +0100	[thread overview]
Message-ID: <f414e061-7afa-d781-e6ae-e6293f29cd40@xen.org> (raw)
In-Reply-To: <20210427093546.30703-11-michal.orzel@arm.com>

Hi Michal,

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 type of hsr, cpsr, spsr_el1 to uint64_t.

As I pointed out in v1, the access to SPSR_EL1 has been quite fragile 
because we relied on the padding afterwards. I think this was ought to 
be explain in the commit message because it explain why the access in 
the assembly code is not modified.

> 
> Add 32bit RES0 members to structures inside hsr union.
> 
> Remove 32bit padding in cpu_user_regs before spsr_fiq
> as it is no longer needed due to upper union being 64bit now.
> 
> Add 64bit padding in cpu_user_regs before spsr_el1
> because offset of spsr_el1 must be a multiple of 8.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> diff --git a/xen/include/asm-arm/hsr.h b/xen/include/asm-arm/hsr.h
> index 29d4531f40..fb4a3b1274 100644
> --- a/xen/include/asm-arm/hsr.h
> +++ b/xen/include/asm-arm/hsr.h
> @@ -16,11 +16,12 @@ 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 */
> +        unsigned long _res0:32;

Sorry I wasn't clear in my original comment, what I meant I would rather 
not add this field (and also the _res0) because they are not strictly 
necessary.

> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 713fd65317..c49bce2983 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -267,10 +267,10 @@ struct vcpu_guest_core_regs
>   
>       /* Return address and mode */
>       __DECL_REG(pc64,         pc32);             /* ELR_EL2 */
> -    uint32_t cpsr;                              /* SPSR_EL2 */
> +    register_t cpsr;                            /* SPSR_EL2 */

You can't use register_t here because this is a public header (we don't 
export register_t) and the header should be bitness agnostic.

Also, because this is a public header, you ought to explain why breaking 
the ABI is fine.

In this case, this is an ABI between the tools and this is not stable. 
However, we would still need to bump XEN_DOMCTL_INTERFACE_VERSION as I 
think this wasn't done for this development cycle.

Of course, this will also need a suitable mention in the commit message 
(I can help with that).

>   
>       union {
> -        uint32_t spsr_el1;       /* AArch64 */
> +        uint64_t spsr_el1;       /* AArch64 */
>           uint32_t spsr_svc;       /* AArch32 */
>       };
>   
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 36135ba4f1..ad3d141fe8 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -266,8 +266,12 @@ struct vm_event_regs_arm {
>       uint64_t ttbr1;
>       uint64_t ttbcr;
>       uint64_t pc;
> +#ifdef CONFIG_ARM_32
>       uint32_t cpsr;
>       uint32_t _pad;
> +#else
> +    uint64_t cpsr;
> +#endif

CONFIG_ARM_32 is not defined for public header. They also should be 
bitness agnostic. So cpsr should always be uint64_t.

Also, similar to public/arch-arm.h, this is not a stable ABI but you 
will need to bump VM_EVENT_INTERFACE_VERSION if this hasn't been done 
for this development cycle.

Cheers,

-- 
Julien Grall


  reply	other threads:[~2021-04-27 11:10 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
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 [this message]
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=f414e061-7afa-d781-e6ae-e6293f29cd40@xen.org \
    --to=julien@xen.org \
    --cc=Volodymyr_Babchuk@epam.com \
    --cc=aisaila@bitdefender.com \
    --cc=bertrand.marquis@arm.com \
    --cc=michal.orzel@arm.com \
    --cc=ppircalabu@bitdefender.com \
    --cc=sstabellini@kernel.org \
    --cc=tamas@tklengyel.com \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t' \
    /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

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).