[PATCH v2 00/10] Get rid of READ/WRITE_SYSREG32

[PATCH v2 00/10] Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

The purpose of this patch series is to remove usage of 32bit helper
macros READ/WRITE_SYSREG32 on arm64 as the idea of them is
not following the latest ARMv8 specification and mrs/msr instructions
are expecting 64bit values.
According to ARM DDI 0487G.a all the AArch64 system registers
are 64bit wide even though many of them have upper 32bit reserved.
This does not mean that in the newer versions of ARMv8 or in the next
architecture, some of the sysregs will get widen.
Also when dealing with system registers we should use register_t
type.

This patch series removes the use of READ/WRITE_SYSREG32
and replaces these calls with READ/WRITE_SYSREG. The change was
splited into several small patches to make the review proces easier.

This patch series focuses on removing READ/WRITE_SYSREG32.
The next thing to do is to also get rid of vreg_emulate_sysreg32
and other parts related to it like TVM_REG macro.
The final part will be to completely remove macros READ/WRITE_SYSREG32.

Michal Orzel (10):
  arm64/vfp: Get rid of READ/WRITE_SYSREG32
  arm/domain: Get rid of READ/WRITE_SYSREG32
  arm: Modify type of actlr to register_t
  arm/gic: Remove member hcr of structure gic_v3
  arm/gic: Get rid of READ/WRITE_SYSREG32
  arm/p2m: Get rid of READ/WRITE_SYSREG32
  arm/mm: Get rid of READ/WRITE_SYSREG32
  arm/page: Get rid of READ/WRITE_SYSREG32
  arm/time,vtimer: Get rid of READ/WRITE_SYSREG32
  arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

 xen/arch/arm/arm64/entry.S            |  4 +-
 xen/arch/arm/arm64/traps.c            |  2 +-
 xen/arch/arm/arm64/vfp.c              | 12 ++--
 xen/arch/arm/arm64/vsysreg.c          |  3 +-
 xen/arch/arm/domain.c                 | 22 +++---
 xen/arch/arm/gic-v3-lpi.c             |  2 +-
 xen/arch/arm/gic-v3.c                 | 98 ++++++++++++++-------------
 xen/arch/arm/mm.c                     |  2 +-
 xen/arch/arm/p2m.c                    |  8 +--
 xen/arch/arm/time.c                   | 28 ++++----
 xen/arch/arm/traps.c                  | 34 ++++++----
 xen/arch/arm/vcpreg.c                 | 13 ++--
 xen/arch/arm/vtimer.c                 | 10 +--
 xen/include/asm-arm/arm64/processor.h | 11 +--
 xen/include/asm-arm/arm64/vfp.h       |  6 +-
 xen/include/asm-arm/domain.h          |  6 +-
 xen/include/asm-arm/gic.h             |  6 +-
 xen/include/asm-arm/gic_v3_defs.h     |  2 +
 xen/include/asm-arm/hsr.h             | 14 +++-
 xen/include/asm-arm/page.h            |  4 +-
 xen/include/asm-arm/processor.h       |  9 ++-
 xen/include/public/arch-arm.h         |  4 +-
 xen/include/public/vm_event.h         |  4 ++
 23 files changed, 169 insertions(+), 135 deletions(-)

-- 
2.29.0

[PATCH v2 01/10] arm64/vfp: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

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 FPCR, FPSR, FPEXC32_EL2 to register_t.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/arm64/vfp.c        | 12 ++++++------
 xen/include/asm-arm/arm64/vfp.h |  6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/arm64/vfp.c b/xen/arch/arm/arm64/vfp.c
index 999a0d58a5..47885e76ba 100644
--- a/xen/arch/arm/arm64/vfp.c
+++ b/xen/arch/arm/arm64/vfp.c
@@ -26,10 +26,10 @@ void vfp_save_state(struct vcpu *v)
                  "stp q30, q31, [%1, #16 * 30]\n\t"
                  : "=Q" (*v->arch.vfp.fpregs) : "r" (v->arch.vfp.fpregs));
 
-    v->arch.vfp.fpsr = READ_SYSREG32(FPSR);
-    v->arch.vfp.fpcr = READ_SYSREG32(FPCR);
+    v->arch.vfp.fpsr = READ_SYSREG(FPSR);
+    v->arch.vfp.fpcr = READ_SYSREG(FPCR);
     if ( is_32bit_domain(v->domain) )
-        v->arch.vfp.fpexc32_el2 = READ_SYSREG32(FPEXC32_EL2);
+        v->arch.vfp.fpexc32_el2 = READ_SYSREG(FPEXC32_EL2);
 }
 
 void vfp_restore_state(struct vcpu *v)
@@ -55,8 +55,8 @@ void vfp_restore_state(struct vcpu *v)
                  "ldp q30, q31, [%1, #16 * 30]\n\t"
                  : : "Q" (*v->arch.vfp.fpregs), "r" (v->arch.vfp.fpregs));
 
-    WRITE_SYSREG32(v->arch.vfp.fpsr, FPSR);
-    WRITE_SYSREG32(v->arch.vfp.fpcr, FPCR);
+    WRITE_SYSREG(v->arch.vfp.fpsr, FPSR);
+    WRITE_SYSREG(v->arch.vfp.fpcr, FPCR);
     if ( is_32bit_domain(v->domain) )
-        WRITE_SYSREG32(v->arch.vfp.fpexc32_el2, FPEXC32_EL2);
+        WRITE_SYSREG(v->arch.vfp.fpexc32_el2, FPEXC32_EL2);
 }
diff --git a/xen/include/asm-arm/arm64/vfp.h b/xen/include/asm-arm/arm64/vfp.h
index 6ab5d36c6c..e6e8c363bc 100644
--- a/xen/include/asm-arm/arm64/vfp.h
+++ b/xen/include/asm-arm/arm64/vfp.h
@@ -7,9 +7,9 @@
 struct vfp_state
 {
     uint64_t fpregs[64] __vfp_aligned;
-    uint32_t fpcr;
-    uint32_t fpexc32_el2;
-    uint32_t fpsr;
+    register_t fpcr;
+    register_t fpexc32_el2;
+    register_t fpsr;
 };
 
 #endif /* _ARM_ARM64_VFP_H */
-- 
2.29.0

[PATCH v2 02/10] arm/domain: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

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 register cntkctl to register_t.
Modify accesses to thumbee registers to use READ/WRITE_SYSREG.
No need to change type of thumbee registers to register_t as they
only exist on arm32.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v1:
-Move modification of ACTLR into seperate patch
---
 xen/arch/arm/domain.c        | 18 +++++++++---------
 xen/include/asm-arm/domain.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bdd3d3e5b5..621f518b83 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -113,13 +113,13 @@ static void ctxt_switch_from(struct vcpu *p)
     p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
 
     /* Arch timer */
-    p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
+    p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
     virt_timer_save(p);
 
     if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
     {
-        p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
-        p->arch.teehbr = READ_SYSREG32(TEEHBR32_EL1);
+        p->arch.teecr = READ_SYSREG(TEECR32_EL1);
+        p->arch.teehbr = READ_SYSREG(TEEHBR32_EL1);
     }
 
 #ifdef CONFIG_ARM_32
@@ -175,7 +175,7 @@ static void ctxt_switch_from(struct vcpu *p)
 
 static void ctxt_switch_to(struct vcpu *n)
 {
-    uint32_t vpidr;
+    register_t vpidr;
 
     /* When the idle VCPU is running, Xen will always stay in hypervisor
      * mode. Therefore we don't need to restore the context of an idle VCPU.
@@ -183,8 +183,8 @@ static void ctxt_switch_to(struct vcpu *n)
     if ( is_idle_vcpu(n) )
         return;
 
-    vpidr = READ_SYSREG32(MIDR_EL1);
-    WRITE_SYSREG32(vpidr, VPIDR_EL2);
+    vpidr = READ_SYSREG(MIDR_EL1);
+    WRITE_SYSREG(vpidr, VPIDR_EL2);
     WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
 
     /* VGIC */
@@ -257,8 +257,8 @@ static void ctxt_switch_to(struct vcpu *n)
 
     if ( is_32bit_domain(n->domain) && cpu_has_thumbee )
     {
-        WRITE_SYSREG32(n->arch.teecr, TEECR32_EL1);
-        WRITE_SYSREG32(n->arch.teehbr, TEEHBR32_EL1);
+        WRITE_SYSREG(n->arch.teecr, TEECR32_EL1);
+        WRITE_SYSREG(n->arch.teehbr, TEEHBR32_EL1);
     }
 
 #ifdef CONFIG_ARM_32
@@ -274,7 +274,7 @@ static void ctxt_switch_to(struct vcpu *n)
 
     /* This is could trigger an hardware interrupt from the virtual
      * timer. The interrupt needs to be injected into the guest. */
-    WRITE_SYSREG32(n->arch.cntkctl, CNTKCTL_EL1);
+    WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
     virt_timer_restore(n);
 }
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 0a74df9931..c6b59ee755 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -190,7 +190,7 @@ struct arch_vcpu
     struct vgic_cpu vgic;
 
     /* Timer registers  */
-    uint32_t cntkctl;
+    register_t cntkctl;
 
     struct vtimer phys_timer;
     struct vtimer virt_timer;
-- 
2.29.0

[PATCH v2 03/10] arm: Modify type of actlr to register_t

[Michal Orzel]

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.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/domain.c        | 2 +-
 xen/include/asm-arm/domain.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 621f518b83..c021a03c61 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -330,7 +330,7 @@ static void schedule_tail(struct vcpu *prev)
 
 static void continue_new_vcpu(struct vcpu *prev)
 {
-    current->arch.actlr = READ_SYSREG32(ACTLR_EL1);
+    current->arch.actlr = READ_SYSREG(ACTLR_EL1);
     processor_vcpu_initialise(current);
 
     schedule_tail(prev);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c6b59ee755..2d4f38c669 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -156,7 +156,7 @@ struct arch_vcpu
 
     /* Control Registers */
     register_t sctlr;
-    uint32_t actlr;
+    register_t actlr;
     uint32_t cpacr;
 
     uint32_t contextidr;
-- 
2.29.0

[PATCH v2 04/10] arm/gic: Remove member hcr of structure gic_v3

[Michal Orzel]

... as it is never used even in the patch introducing it.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/include/asm-arm/gic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ad0f7452d0..5069ab4aac 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -171,7 +171,7 @@
  * GICv3 registers that needs to be saved/restored
  */
 struct gic_v3 {
-    uint32_t hcr, vmcr, sre_el1;
+    uint32_t vmcr, sre_el1;
     uint32_t apr0[4];
     uint32_t apr1[4];
     uint64_t lr[16];
-- 
2.29.0

[PATCH v2 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

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 types of following members of struct gic_v3 to register_t:
-vmcr
-sre_el1
-apr0
-apr1

Add new macro GICC_IAR_INTID_MASK containing the mask
for INTID field of ICC_IAR0/1_EL1 register.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v1:
-Remove hcr member of gic_v3 in a seperate patch
-Add macro GICC_IAR_INTID_MASK
-Remove explicit cast in favor of implicit cast
---
 xen/arch/arm/gic-v3-lpi.c         |  2 +-
 xen/arch/arm/gic-v3.c             | 98 ++++++++++++++++---------------
 xen/include/asm-arm/gic.h         |  6 +-
 xen/include/asm-arm/gic_v3_defs.h |  2 +
 4 files changed, 58 insertions(+), 50 deletions(-)

diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
index 869bc97fa1..e1594dd20e 100644
--- a/xen/arch/arm/gic-v3-lpi.c
+++ b/xen/arch/arm/gic-v3-lpi.c
@@ -178,7 +178,7 @@ void gicv3_do_LPI(unsigned int lpi)
     irq_enter();
 
     /* EOI the LPI already. */
-    WRITE_SYSREG32(lpi, ICC_EOIR1_EL1);
+    WRITE_SYSREG(lpi, ICC_EOIR1_EL1);
 
     /* Find out if a guest mapped something to this physical LPI. */
     hlpip = gic_get_host_lpi(lpi);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index ac28013c19..b86f040589 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -246,12 +246,12 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
  */
 static void gicv3_enable_sre(void)
 {
-    uint32_t val;
+    register_t val;
 
-    val = READ_SYSREG32(ICC_SRE_EL2);
+    val = READ_SYSREG(ICC_SRE_EL2);
     val |= GICC_SRE_EL2_SRE;
 
-    WRITE_SYSREG32(val, ICC_SRE_EL2);
+    WRITE_SYSREG(val, ICC_SRE_EL2);
     isb();
 }
 
@@ -315,16 +315,16 @@ static void restore_aprn_regs(const union gic_state_data *d)
     switch ( gicv3.nr_priorities )
     {
     case 7:
-        WRITE_SYSREG32(d->v3.apr0[2], ICH_AP0R2_EL2);
-        WRITE_SYSREG32(d->v3.apr1[2], ICH_AP1R2_EL2);
+        WRITE_SYSREG(d->v3.apr0[2], ICH_AP0R2_EL2);
+        WRITE_SYSREG(d->v3.apr1[2], ICH_AP1R2_EL2);
         /* Fall through */
     case 6:
-        WRITE_SYSREG32(d->v3.apr0[1], ICH_AP0R1_EL2);
-        WRITE_SYSREG32(d->v3.apr1[1], ICH_AP1R1_EL2);
+        WRITE_SYSREG(d->v3.apr0[1], ICH_AP0R1_EL2);
+        WRITE_SYSREG(d->v3.apr1[1], ICH_AP1R1_EL2);
         /* Fall through */
     case 5:
-        WRITE_SYSREG32(d->v3.apr0[0], ICH_AP0R0_EL2);
-        WRITE_SYSREG32(d->v3.apr1[0], ICH_AP1R0_EL2);
+        WRITE_SYSREG(d->v3.apr0[0], ICH_AP0R0_EL2);
+        WRITE_SYSREG(d->v3.apr1[0], ICH_AP1R0_EL2);
         break;
     default:
         BUG();
@@ -338,16 +338,16 @@ static void save_aprn_regs(union gic_state_data *d)
     switch ( gicv3.nr_priorities )
     {
     case 7:
-        d->v3.apr0[2] = READ_SYSREG32(ICH_AP0R2_EL2);
-        d->v3.apr1[2] = READ_SYSREG32(ICH_AP1R2_EL2);
+        d->v3.apr0[2] = READ_SYSREG(ICH_AP0R2_EL2);
+        d->v3.apr1[2] = READ_SYSREG(ICH_AP1R2_EL2);
         /* Fall through */
     case 6:
-        d->v3.apr0[1] = READ_SYSREG32(ICH_AP0R1_EL2);
-        d->v3.apr1[1] = READ_SYSREG32(ICH_AP1R1_EL2);
+        d->v3.apr0[1] = READ_SYSREG(ICH_AP0R1_EL2);
+        d->v3.apr1[1] = READ_SYSREG(ICH_AP1R1_EL2);
         /* Fall through */
     case 5:
-        d->v3.apr0[0] = READ_SYSREG32(ICH_AP0R0_EL2);
-        d->v3.apr1[0] = READ_SYSREG32(ICH_AP1R0_EL2);
+        d->v3.apr0[0] = READ_SYSREG(ICH_AP0R0_EL2);
+        d->v3.apr1[0] = READ_SYSREG(ICH_AP1R0_EL2);
         break;
     default:
         BUG();
@@ -371,15 +371,15 @@ static void gicv3_save_state(struct vcpu *v)
     dsb(sy);
     gicv3_save_lrs(v);
     save_aprn_regs(&v->arch.gic);
-    v->arch.gic.v3.vmcr = READ_SYSREG32(ICH_VMCR_EL2);
-    v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
+    v->arch.gic.v3.vmcr = READ_SYSREG(ICH_VMCR_EL2);
+    v->arch.gic.v3.sre_el1 = READ_SYSREG(ICC_SRE_EL1);
 }
 
 static void gicv3_restore_state(const struct vcpu *v)
 {
-    uint32_t val;
+    register_t val;
 
-    val = READ_SYSREG32(ICC_SRE_EL2);
+    val = READ_SYSREG(ICC_SRE_EL2);
     /*
      * Don't give access to system registers when the guest is using
      * GICv2
@@ -388,7 +388,7 @@ static void gicv3_restore_state(const struct vcpu *v)
         val &= ~GICC_SRE_EL2_ENEL1;
     else
         val |= GICC_SRE_EL2_ENEL1;
-    WRITE_SYSREG32(val, ICC_SRE_EL2);
+    WRITE_SYSREG(val, ICC_SRE_EL2);
 
     /*
      * VFIQEn is RES1 if ICC_SRE_EL1.SRE is 1. This causes a Group0
@@ -398,9 +398,9 @@ static void gicv3_restore_state(const struct vcpu *v)
      * want before starting to mess with the rest of the GIC, and
      * VMCR_EL1 in particular.
      */
-    WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
+    WRITE_SYSREG(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
     isb();
-    WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
+    WRITE_SYSREG(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
     restore_aprn_regs(&v->arch.gic);
     gicv3_restore_lrs(v);
 
@@ -468,24 +468,25 @@ static void gicv3_mask_irq(struct irq_desc *irqd)
 static void gicv3_eoi_irq(struct irq_desc *irqd)
 {
     /* Lower the priority */
-    WRITE_SYSREG32(irqd->irq, ICC_EOIR1_EL1);
+    WRITE_SYSREG(irqd->irq, ICC_EOIR1_EL1);
     isb();
 }
 
 static void gicv3_dir_irq(struct irq_desc *irqd)
 {
     /* Deactivate */
-    WRITE_SYSREG32(irqd->irq, ICC_DIR_EL1);
+    WRITE_SYSREG(irqd->irq, ICC_DIR_EL1);
     isb();
 }
 
 static unsigned int gicv3_read_irq(void)
 {
-    unsigned int irq = READ_SYSREG32(ICC_IAR1_EL1);
+    register_t irq = READ_SYSREG(ICC_IAR1_EL1);
 
     dsb(sy);
 
-    return irq;
+    /* IRQs are encoded using 23bit. */
+    return (irq & GICC_IAR_INTID_MASK);
 }
 
 /*
@@ -857,16 +858,16 @@ static int gicv3_cpu_init(void)
     gicv3_enable_sre();
 
     /* No priority grouping */
-    WRITE_SYSREG32(0, ICC_BPR1_EL1);
+    WRITE_SYSREG(0, ICC_BPR1_EL1);
 
     /* Set priority mask register */
-    WRITE_SYSREG32(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
+    WRITE_SYSREG(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
 
     /* EOI drops priority, DIR deactivates the interrupt (mode 1) */
-    WRITE_SYSREG32(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
+    WRITE_SYSREG(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
 
     /* Enable Group1 interrupts */
-    WRITE_SYSREG32(1, ICC_IGRPEN1_EL1);
+    WRITE_SYSREG(1, ICC_IGRPEN1_EL1);
 
     /* Sync at once at the end of cpu interface configuration */
     isb();
@@ -876,15 +877,15 @@ static int gicv3_cpu_init(void)
 
 static void gicv3_cpu_disable(void)
 {
-    WRITE_SYSREG32(0, ICC_CTLR_EL1);
+    WRITE_SYSREG(0, ICC_CTLR_EL1);
     isb();
 }
 
 static void gicv3_hyp_init(void)
 {
-    uint32_t vtr;
+    register_t vtr;
 
-    vtr = READ_SYSREG32(ICH_VTR_EL2);
+    vtr = READ_SYSREG(ICH_VTR_EL2);
     gicv3_info.nr_lrs  = (vtr & ICH_VTR_NRLRGS) + 1;
     gicv3.nr_priorities = ((vtr >> ICH_VTR_PRIBITS_SHIFT) &
                           ICH_VTR_PRIBITS_MASK) + 1;
@@ -892,8 +893,8 @@ static void gicv3_hyp_init(void)
     if ( !((gicv3.nr_priorities > 4) && (gicv3.nr_priorities < 8)) )
         panic("GICv3: Invalid number of priority bits\n");
 
-    WRITE_SYSREG32(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
-    WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
+    WRITE_SYSREG(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
+    WRITE_SYSREG(GICH_HCR_EN, ICH_HCR_EL2);
 }
 
 /* Set up the per-CPU parts of the GIC for a secondary CPU */
@@ -917,11 +918,11 @@ out:
 
 static void gicv3_hyp_disable(void)
 {
-    uint32_t hcr;
+    register_t hcr;
 
-    hcr = READ_SYSREG32(ICH_HCR_EL2);
+    hcr = READ_SYSREG(ICH_HCR_EL2);
     hcr &= ~GICH_HCR_EN;
-    WRITE_SYSREG32(hcr, ICH_HCR_EL2);
+    WRITE_SYSREG(hcr, ICH_HCR_EL2);
     isb();
 }
 
@@ -1140,39 +1141,44 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
 
 static void gicv3_hcr_status(uint32_t flag, bool status)
 {
-    uint32_t hcr;
+    register_t hcr;
 
-    hcr = READ_SYSREG32(ICH_HCR_EL2);
+    hcr = READ_SYSREG(ICH_HCR_EL2);
     if ( status )
-        WRITE_SYSREG32(hcr | flag, ICH_HCR_EL2);
+        WRITE_SYSREG(hcr | flag, ICH_HCR_EL2);
     else
-        WRITE_SYSREG32(hcr & (~flag), ICH_HCR_EL2);
+        WRITE_SYSREG(hcr & (~flag), ICH_HCR_EL2);
     isb();
 }
 
 static unsigned int gicv3_read_vmcr_priority(void)
 {
-   return ((READ_SYSREG32(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
+   return ((READ_SYSREG(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
             ICH_VMCR_PRIORITY_MASK);
 }
 
 /* Only support reading GRP1 APRn registers */
 static unsigned int gicv3_read_apr(int apr_reg)
 {
+    register_t apr;
+
     switch ( apr_reg )
     {
     case 0:
         ASSERT(gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8);
-        return READ_SYSREG32(ICH_AP1R0_EL2);
+        apr = READ_SYSREG(ICH_AP1R0_EL2);
     case 1:
         ASSERT(gicv3.nr_priorities > 5 && gicv3.nr_priorities < 8);
-        return READ_SYSREG32(ICH_AP1R1_EL2);
+        apr = READ_SYSREG(ICH_AP1R1_EL2);
     case 2:
         ASSERT(gicv3.nr_priorities > 6 && gicv3.nr_priorities < 8);
-        return READ_SYSREG32(ICH_AP1R2_EL2);
+        apr = READ_SYSREG(ICH_AP1R2_EL2);
     default:
         BUG();
     }
+
+    /* Number of priority levels do not exceed 32bit. */
+    return apr;
 }
 
 static bool gicv3_read_pending_state(struct irq_desc *irqd)
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 5069ab4aac..c7f0c343d1 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -171,9 +171,9 @@
  * GICv3 registers that needs to be saved/restored
  */
 struct gic_v3 {
-    uint32_t vmcr, sre_el1;
-    uint32_t apr0[4];
-    uint32_t apr1[4];
+    register_t vmcr, sre_el1;
+    register_t apr0[4];
+    register_t apr1[4];
     uint64_t lr[16];
 };
 #endif
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 5a578e7c11..34ed5f857d 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -45,6 +45,8 @@
 #define GICC_SRE_EL2_DIB             (1UL << 2)
 #define GICC_SRE_EL2_ENEL1           (1UL << 3)
 
+#define GICC_IAR_INTID_MASK          (0xFFFFFF)
+
 /* Additional bits in GICD_TYPER defined by GICv3 */
 #define GICD_TYPE_ID_BITS_SHIFT 19
 #define GICD_TYPE_ID_BITS(r)    ((((r) >> GICD_TYPE_ID_BITS_SHIFT) & 0x1f) + 1)
-- 
2.29.0

[PATCH v2 06/10] arm/p2m: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

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 vtcr to register_t.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/p2m.c   | 8 ++++----
 xen/arch/arm/traps.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ac50312620..d414c4feb9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1973,11 +1973,11 @@ void __init p2m_restrict_ipa_bits(unsigned int ipa_bits)
 }
 
 /* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
-static uint32_t __read_mostly vtcr;
+static register_t __read_mostly vtcr;
 
 static void setup_virt_paging_one(void *data)
 {
-    WRITE_SYSREG32(vtcr, VTCR_EL2);
+    WRITE_SYSREG(vtcr, VTCR_EL2);
 
     /*
      * ARM64_WORKAROUND_AT_SPECULATE: We want to keep the TLBs free from
@@ -2000,7 +2000,7 @@ static void setup_virt_paging_one(void *data)
 void __init setup_virt_paging(void)
 {
     /* Setup Stage 2 address translation */
-    unsigned long val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
+    register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
 
 #ifdef CONFIG_ARM_32
     if ( p2m_ipa_bits < 40 )
@@ -2089,7 +2089,7 @@ void __init setup_virt_paging(void)
            pa_range_info[pa_range].pabits,
            ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
 #endif
-    printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
+    printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
            4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
 
     p2m_vmid_allocator_init();
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ccc0827107..c7acdb2087 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -911,7 +911,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
         show_registers_32(regs, ctxt, guest_mode, v);
 #endif
     }
-    printk("  VTCR_EL2: %08"PRIx32"\n", READ_SYSREG32(VTCR_EL2));
+    printk("  VTCR_EL2: %"PRIregister"\n", READ_SYSREG(VTCR_EL2));
     printk(" VTTBR_EL2: %016"PRIx64"\n", ctxt->vttbr_el2);
     printk("\n");
 
-- 
2.29.0

[PATCH v2 07/10] arm/mm: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

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

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));
     printk("   HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
     printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
     printk("\n");
-- 
2.29.0

[PATCH v2 08/10] arm/page: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

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 accesses to CTR_EL0 to use READ/WRITE_SYSREG.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-arm/page.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 131507a517..c6f9fb0d4e 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -137,10 +137,10 @@ extern size_t dcache_line_bytes;
 
 static inline size_t read_dcache_line_bytes(void)
 {
-    uint32_t ctr;
+    register_t ctr;
 
     /* Read CTR */
-    ctr = READ_SYSREG32(CTR_EL0);
+    ctr = READ_SYSREG(CTR_EL0);
 
     /* Bits 16-19 are the log2 number of words in the cacheline. */
     return (size_t) (4 << ((ctr >> 16) & 0xf));
-- 
2.29.0

[PATCH v2 09/10] arm/time,vtimer: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

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 vtimer structure's member: ctl to register_t.

Add macro CNTFRQ_MASK containing mask for timer clock frequency
field of CNTFRQ_EL0 register.

Modify CNTx_CTL_MASK to return unsigned long instead of
unsigned int as ctl is now of type register_t.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v1:
-Add macro CNTFRQ_MASK
-Modify CNTx_CTL_MASK, CNTx_CTL_ENABLE, CNTx_CTL_PENDING to return ul
---
 xen/arch/arm/time.c             | 28 ++++++++++++++--------------
 xen/arch/arm/vtimer.c           | 10 +++++-----
 xen/include/asm-arm/domain.h    |  2 +-
 xen/include/asm-arm/processor.h |  9 ++++++---
 4 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index b0021c2c69..7dbd363537 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -145,7 +145,7 @@ void __init preinit_xen_time(void)
         preinit_acpi_xen_time();
 
     if ( !cpu_khz )
-        cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
+        cpu_khz = (READ_SYSREG(CNTFRQ_EL0) & CNTFRQ_MASK) / 1000;
 
     res = platform_init_time();
     if ( res )
@@ -205,13 +205,13 @@ int reprogram_timer(s_time_t timeout)
 
     if ( timeout == 0 )
     {
-        WRITE_SYSREG32(0, CNTHP_CTL_EL2);
+        WRITE_SYSREG(0, CNTHP_CTL_EL2);
         return 1;
     }
 
     deadline = ns_to_ticks(timeout) + boot_count;
     WRITE_SYSREG64(deadline, CNTHP_CVAL_EL2);
-    WRITE_SYSREG32(CNTx_CTL_ENABLE, CNTHP_CTL_EL2);
+    WRITE_SYSREG(CNTx_CTL_ENABLE, CNTHP_CTL_EL2);
     isb();
 
     /* No need to check for timers in the past; the Generic Timer fires
@@ -223,23 +223,23 @@ int reprogram_timer(s_time_t timeout)
 static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
     if ( irq == (timer_irq[TIMER_HYP_PPI]) &&
-         READ_SYSREG32(CNTHP_CTL_EL2) & CNTx_CTL_PENDING )
+         READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING )
     {
         perfc_incr(hyp_timer_irqs);
         /* Signal the generic timer code to do its work */
         raise_softirq(TIMER_SOFTIRQ);
         /* Disable the timer to avoid more interrupts */
-        WRITE_SYSREG32(0, CNTHP_CTL_EL2);
+        WRITE_SYSREG(0, CNTHP_CTL_EL2);
     }
 
     if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) &&
-         READ_SYSREG32(CNTP_CTL_EL0) & CNTx_CTL_PENDING )
+         READ_SYSREG(CNTP_CTL_EL0) & CNTx_CTL_PENDING )
     {
         perfc_incr(phys_timer_irqs);
         /* Signal the generic timer code to do its work */
         raise_softirq(TIMER_SOFTIRQ);
         /* Disable the timer to avoid more interrupts */
-        WRITE_SYSREG32(0, CNTP_CTL_EL0);
+        WRITE_SYSREG(0, CNTP_CTL_EL0);
     }
 }
 
@@ -260,8 +260,8 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 
     perfc_incr(virt_timer_irqs);
 
-    current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
-    WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
+    current->arch.virt_timer.ctl = READ_SYSREG(CNTV_CTL_EL0);
+    WRITE_SYSREG(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
     vgic_inject_irq(current->domain, current, current->arch.virt_timer.irq, true);
 }
 
@@ -297,9 +297,9 @@ void init_timer_interrupt(void)
     /* Sensible defaults */
     WRITE_SYSREG64(0, CNTVOFF_EL2);     /* No VM-specific offset */
     /* Do not let the VMs program the physical timer, only read the physical counter */
-    WRITE_SYSREG32(CNTHCTL_EL2_EL1PCTEN, CNTHCTL_EL2);
-    WRITE_SYSREG32(0, CNTP_CTL_EL0);    /* Physical timer disabled */
-    WRITE_SYSREG32(0, CNTHP_CTL_EL2);   /* Hypervisor's timer disabled */
+    WRITE_SYSREG(CNTHCTL_EL2_EL1PCTEN, CNTHCTL_EL2);
+    WRITE_SYSREG(0, CNTP_CTL_EL0);    /* Physical timer disabled */
+    WRITE_SYSREG(0, CNTHP_CTL_EL2);   /* Hypervisor's timer disabled */
     isb();
 
     request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
@@ -320,8 +320,8 @@ void init_timer_interrupt(void)
  */
 static void deinit_timer_interrupt(void)
 {
-    WRITE_SYSREG32(0, CNTP_CTL_EL0);    /* Disable physical timer */
-    WRITE_SYSREG32(0, CNTHP_CTL_EL2);   /* Disable hypervisor's timer */
+    WRITE_SYSREG(0, CNTP_CTL_EL0);    /* Disable physical timer */
+    WRITE_SYSREG(0, CNTHP_CTL_EL2);   /* Disable hypervisor's timer */
     isb();
 
     release_irq(timer_irq[TIMER_HYP_PPI], NULL);
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index c2b27915c6..167fc6127a 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -138,8 +138,8 @@ void virt_timer_save(struct vcpu *v)
 {
     ASSERT(!is_idle_vcpu(v));
 
-    v->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
-    WRITE_SYSREG32(v->arch.virt_timer.ctl & ~CNTx_CTL_ENABLE, CNTV_CTL_EL0);
+    v->arch.virt_timer.ctl = READ_SYSREG(CNTV_CTL_EL0);
+    WRITE_SYSREG(v->arch.virt_timer.ctl & ~CNTx_CTL_ENABLE, CNTV_CTL_EL0);
     v->arch.virt_timer.cval = READ_SYSREG64(CNTV_CVAL_EL0);
     if ( (v->arch.virt_timer.ctl & CNTx_CTL_ENABLE) &&
          !(v->arch.virt_timer.ctl & CNTx_CTL_MASK))
@@ -159,7 +159,7 @@ void virt_timer_restore(struct vcpu *v)
 
     WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2);
     WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0);
-    WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
+    WRITE_SYSREG(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
 }
 
 static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
@@ -347,7 +347,7 @@ bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr)
 }
 
 static void vtimer_update_irq(struct vcpu *v, struct vtimer *vtimer,
-                              uint32_t vtimer_ctl)
+                              register_t vtimer_ctl)
 {
     bool level;
 
@@ -389,7 +389,7 @@ void vtimer_update_irqs(struct vcpu *v)
      * but this requires reworking the arch timer to implement this.
      */
     vtimer_update_irq(v, &v->arch.virt_timer,
-                      READ_SYSREG32(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
+                      READ_SYSREG(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
 
     /* For the physical timer we rely on our emulated state. */
     vtimer_update_irq(v, &v->arch.phys_timer, v->arch.phys_timer.ctl);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 2d4f38c669..c9277b5c6d 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -36,7 +36,7 @@ struct vtimer {
     struct vcpu *v;
     int irq;
     struct timer timer;
-    uint32_t ctl;
+    register_t ctl;
     uint64_t cval;
 };
 
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 5c1768cdec..2058b69447 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -484,9 +484,12 @@ extern register_t __cpu_logical_map[];
 #define CNTKCTL_EL1_EL0PTEN  (1u<<9) /* Expose phys timer registers to EL0 */
 
 /* Timer control registers */
-#define CNTx_CTL_ENABLE   (1u<<0)  /* Enable timer */
-#define CNTx_CTL_MASK     (1u<<1)  /* Mask IRQ */
-#define CNTx_CTL_PENDING  (1u<<2)  /* IRQ pending */
+#define CNTx_CTL_ENABLE   (1ul<<0)  /* Enable timer */
+#define CNTx_CTL_MASK     (1ul<<1)  /* Mask IRQ */
+#define CNTx_CTL_PENDING  (1ul<<2)  /* IRQ pending */
+
+/* Timer frequency mask */
+#define CNTFRQ_MASK       GENMASK(31, 0)
 
 /* Exception Vector offsets */
 /* ... ARM32 */
-- 
2.29.0

[PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

[Michal Orzel]

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.

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>
---
Changes since v1:
-Modify type of cpsr, spsr_el1
-Remove ifdefery in hsr union protecting _res0 members
-Fix formatting of printk calls
---
 xen/arch/arm/arm64/entry.S            |  4 ++--
 xen/arch/arm/arm64/traps.c            |  2 +-
 xen/arch/arm/arm64/vsysreg.c          |  3 ++-
 xen/arch/arm/domain.c                 |  2 +-
 xen/arch/arm/traps.c                  | 30 +++++++++++++++------------
 xen/arch/arm/vcpreg.c                 | 13 ++++++------
 xen/include/asm-arm/arm64/processor.h | 11 +++++-----
 xen/include/asm-arm/hsr.h             | 14 ++++++++++++-
 xen/include/public/arch-arm.h         |  4 ++--
 xen/include/public/vm_event.h         |  4 ++++
 10 files changed, 55 insertions(+), 32 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index ab9a65fc14..fc3811ad0a 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
 
@@ -432,7 +432,7 @@ return_from_trap:
         msr     daifset, #IFLAGS___I_ /* Mask interrupts */
 
         ldr     x21, [sp, #UREGS_PC]            /* load ELR */
-        ldr     w22, [sp, #UREGS_CPSR]          /* load SPSR */
+        ldr     x22, [sp, #UREGS_CPSR]          /* load SPSR */
 
         pop     x0, x1
         pop     x2, x3
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..caf17174b8 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/domain.c b/xen/arch/arm/domain.c
index c021a03c61..74bdbb9082 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -845,7 +845,7 @@ static int is_guest_pv32_psr(uint32_t psr)
 
 
 #ifdef CONFIG_ARM_64
-static int is_guest_pv64_psr(uint32_t psr)
+static int is_guest_pv64_psr(uint64_t psr)
 {
     if ( psr & PSR_MODE_BIT )
         return 0;
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e7384381cc..c8f9773566 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,
@@ -717,7 +717,7 @@ struct reg_ctxt {
     uint64_t vttbr_el2;
 };
 
-static const char *mode_string(uint32_t cpsr)
+static const char *mode_string(register_t cpsr)
 {
     uint32_t mode;
     static const char *mode_strings[] = {
@@ -756,14 +756,16 @@ static void show_registers_32(const struct cpu_user_regs *regs,
 #ifdef CONFIG_ARM_64
     BUG_ON( ! (regs->cpsr & PSR_MODE_BIT) );
     printk("PC:     %08"PRIx32"\n", regs->pc32);
+    printk("CPSR:   %016"PRIx64" MODE:%s\n", regs->cpsr,
+           mode_string(regs->cpsr));
 #else
     printk("PC:     %08"PRIx32, regs->pc);
     if ( !guest_mode )
         printk(" %pS", _p(regs->pc));
     printk("\n");
-#endif
     printk("CPSR:   %08"PRIx32" MODE:%s\n", regs->cpsr,
            mode_string(regs->cpsr));
+#endif
     printk("     R0: %08"PRIx32" R1: %08"PRIx32" R2: %08"PRIx32" R3: %08"PRIx32"\n",
            regs->r0, regs->r1, regs->r2, regs->r3);
     printk("     R4: %08"PRIx32" R5: %08"PRIx32" R6: %08"PRIx32" R7: %08"PRIx32"\n",
@@ -846,7 +848,7 @@ static void show_registers_64(const struct cpu_user_regs *regs,
     {
         printk("SP:     %016"PRIx64"\n", regs->sp);
     }
-    printk("CPSR:   %08"PRIx32" MODE:%s\n", regs->cpsr,
+    printk("CPSR:   %016"PRIx64" MODE:%s\n", regs->cpsr,
            mode_string(regs->cpsr));
     printk("     X0: %016"PRIx64"  X1: %016"PRIx64"  X2: %016"PRIx64"\n",
            regs->x0, regs->x1, regs->x2);
@@ -919,7 +921,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
@@ -1599,7 +1601,7 @@ static const unsigned short cc_map[16] = {
 
 int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
 {
-    unsigned long cpsr, cpsr_cond;
+    register_t cpsr, cpsr_cond;
     int cond;
 
     /*
@@ -1661,7 +1663,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
 
 void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
 {
-    unsigned long itbits, cond, cpsr = regs->cpsr;
+    register_t itbits, cond, cpsr = regs->cpsr;
     bool is_thumb = psr_mode_is_32bit(regs) && (cpsr & PSR_THUMB);
 
     if ( is_thumb && (cpsr & PSR_IT_MASK) )
@@ -2004,13 +2006,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 +2208,7 @@ 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);
         inject_undef_exception(regs, hsr);
     }
@@ -2242,7 +2246,7 @@ 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",
                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..f0cdcc8a54 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",
                      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/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
index 81dfc5e615..0e86079cbb 100644
--- a/xen/include/asm-arm/arm64/processor.h
+++ b/xen/include/asm-arm/arm64/processor.h
@@ -63,18 +63,19 @@ struct cpu_user_regs
 
     /* Return address and mode */
     __DECL_REG(pc,           pc32);             /* ELR_EL2 */
-    uint32_t cpsr;                              /* SPSR_EL2 */
-    uint32_t hsr;                               /* ESR_EL2 */
+    uint64_t cpsr;                              /* SPSR_EL2 */
+    uint64_t hsr;                               /* ESR_EL2 */
+
+    /* Offset of spsr_el1 must be a multiple of 8 */
+    uint64_t pad0;
 
     /* Outer guest frame only from here on... */
 
     union {
-        uint32_t spsr_el1;       /* AArch64 */
+        uint64_t spsr_el1;       /* AArch64 */
         uint32_t spsr_svc;       /* AArch32 */
     };
 
-    uint32_t pad1; /* Doubleword-align the user half of the frame */
-
     /* 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..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;
     };
 
     /* Common to all conditional exception classes (0x0N, except 0x00). */
@@ -30,6 +31,7 @@ union hsr {
         unsigned long ccvalid:1;/* CC Valid */
         unsigned long len:1;   /* Instruction length */
         unsigned long ec:6;    /* Exception Class */
+        unsigned long _res0:32;
     } cond;
 
     struct hsr_wfi_wfe {
@@ -39,6 +41,7 @@ union hsr {
         unsigned long ccvalid:1;/* CC Valid */
         unsigned long len:1;   /* Instruction length */
         unsigned long ec:6;    /* Exception Class */
+        unsigned long _res0:32;
     } wfi_wfe;
 
     /* reg, reg0, reg1 are 4 bits on AArch32, the fifth bit is sbzp. */
@@ -53,6 +56,7 @@ union hsr {
         unsigned long ccvalid:1;/* CC Valid */
         unsigned long len:1;   /* Instruction length */
         unsigned long ec:6;    /* Exception Class */
+        unsigned long _res0:32;
     } cp32; /* HSR_EC_CP15_32, CP14_32, CP10 */
 
     struct hsr_cp64 {
@@ -66,6 +70,7 @@ union hsr {
         unsigned long ccvalid:1;/* CC Valid */
         unsigned long len:1;    /* Instruction length */
         unsigned long ec:6;     /* Exception Class */
+        unsigned long _res0:32;
     } cp64; /* HSR_EC_CP15_64, HSR_EC_CP14_64 */
 
      struct hsr_cp {
@@ -77,6 +82,7 @@ union hsr {
         unsigned long ccvalid:1;/* CC Valid */
         unsigned long len:1;    /* Instruction length */
         unsigned long ec:6;     /* Exception Class */
+        unsigned long _res0:32;
     } cp; /* HSR_EC_CP */
 
     /*
@@ -94,6 +100,7 @@ union hsr {
         unsigned long ccvalid:1;/* CC Valid */
         unsigned long len:1;   /* Instruction length */
         unsigned long ec:6;    /* Exception Class */
+        unsigned long _res0:32;
     } smc32; /* HSR_EC_SMC32 */
 
 #ifdef CONFIG_ARM_64
@@ -108,6 +115,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 +129,7 @@ union hsr {
         unsigned long res2:14;
         unsigned long len:1;   /* Instruction length */
         unsigned long ec:6;    /* Exception Class */
+        unsigned long _res0:32;
     } iabt; /* HSR_EC_INSTR_ABORT_* */
 
     struct hsr_dabt {
@@ -143,6 +152,7 @@ union hsr {
         unsigned long valid:1; /* Syndrome Valid */
         unsigned long len:1;   /* Instruction length */
         unsigned long ec:6;    /* Exception Class */
+        unsigned long _res0:32;
     } dabt; /* HSR_EC_DATA_ABORT_* */
 
     /* Contain the common bits between DABT and IABT */
@@ -156,6 +166,7 @@ union hsr {
         unsigned long pad3:14;  /* Not common */
         unsigned long len:1;    /* Instruction length */
         unsigned long ec:6;     /* Exception Class */
+        unsigned long _res0:32;
     } xabt;
 
 #ifdef CONFIG_ARM_64
@@ -164,6 +175,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
 };
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 */
 
     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
 };
 
 /*
-- 
2.29.0

Re: [PATCH v2 02/10] arm/domain: Get rid of READ/WRITE_SYSREG32

[Julien Grall]

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 register cntkctl to register_t.
> Modify accesses to thumbee registers to use READ/WRITE_SYSREG.
> No need to change type of thumbee registers to register_t as they
> only exist on arm32.

This is a bit ambiguous, I think in this context you mean it is only 
usable by 32-bit domain but should really be only accessed on Armv7 as 
they were restrospectively dropped on Armv8.

> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> Changes since v1:
> -Move modification of ACTLR into seperate patch
> ---
>   xen/arch/arm/domain.c        | 18 +++++++++---------
>   xen/include/asm-arm/domain.h |  2 +-
>   2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index bdd3d3e5b5..621f518b83 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -113,13 +113,13 @@ static void ctxt_switch_from(struct vcpu *p)
>       p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
>   
>       /* Arch timer */
> -    p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
> +    p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
>       virt_timer_save(p);
>   
>       if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
>       {
> -        p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
> -        p->arch.teehbr = READ_SYSREG32(TEEHBR32_EL1);
> +        p->arch.teecr = READ_SYSREG(TEECR32_EL1);
> +        p->arch.teehbr = READ_SYSREG(TEEHBR32_EL1);
>       }
>   
>   #ifdef CONFIG_ARM_32
> @@ -175,7 +175,7 @@ static void ctxt_switch_from(struct vcpu *p)
>   
>   static void ctxt_switch_to(struct vcpu *n)
>   {
> -    uint32_t vpidr;
> +    register_t vpidr;
>   
>       /* When the idle VCPU is running, Xen will always stay in hypervisor
>        * mode. Therefore we don't need to restore the context of an idle VCPU.
> @@ -183,8 +183,8 @@ static void ctxt_switch_to(struct vcpu *n)
>       if ( is_idle_vcpu(n) )
>           return;
>   
> -    vpidr = READ_SYSREG32(MIDR_EL1);
> -    WRITE_SYSREG32(vpidr, VPIDR_EL2);
> +    vpidr = READ_SYSREG(MIDR_EL1);
> +    WRITE_SYSREG(vpidr, VPIDR_EL2);
>       WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
>   
>       /* VGIC */
> @@ -257,8 +257,8 @@ static void ctxt_switch_to(struct vcpu *n)
>   
>       if ( is_32bit_domain(n->domain) && cpu_has_thumbee )
>       {
> -        WRITE_SYSREG32(n->arch.teecr, TEECR32_EL1);
> -        WRITE_SYSREG32(n->arch.teehbr, TEEHBR32_EL1);
> +        WRITE_SYSREG(n->arch.teecr, TEECR32_EL1);
> +        WRITE_SYSREG(n->arch.teehbr, TEEHBR32_EL1);
>       }
>   
>   #ifdef CONFIG_ARM_32
> @@ -274,7 +274,7 @@ static void ctxt_switch_to(struct vcpu *n)
>   
>       /* This is could trigger an hardware interrupt from the virtual
>        * timer. The interrupt needs to be injected into the guest. */
> -    WRITE_SYSREG32(n->arch.cntkctl, CNTKCTL_EL1);
> +    WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
>       virt_timer_restore(n);
>   }
>   
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 0a74df9931..c6b59ee755 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -190,7 +190,7 @@ struct arch_vcpu
>       struct vgic_cpu vgic;
>   
>       /* Timer registers  */
> -    uint32_t cntkctl;
> +    register_t cntkctl;
>   
>       struct vtimer phys_timer;
>       struct vtimer virt_timer;
> 

-- 
Julien Grall

Re: [PATCH v2 03/10] arm: Modify type of actlr to register_t

[Julien Grall]

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.

This is a pretty generic message but doesn't really explain the change 
itself and point out this is a bug (possibly latent on current HW) 
because it is implementation defined. IOW a CPU implementer may already 
have decided to use the top 32-bit without our knowledge.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 04/10] arm/gic: Remove member hcr of structure gic_v3

[Julien Grall]

Hi Michal,

On 27/04/2021 10:35, Michal Orzel wrote:
> ... as it is never used even in the patch introducing it.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
>   xen/include/asm-arm/gic.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ad0f7452d0..5069ab4aac 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -171,7 +171,7 @@
>    * GICv3 registers that needs to be saved/restored
>    */
>   struct gic_v3 {
> -    uint32_t hcr, vmcr, sre_el1;
> +    uint32_t vmcr, sre_el1;
>       uint32_t apr0[4];
>       uint32_t apr1[4];
>       uint64_t lr[16];
> 

-- 
Julien Grall

Re: [PATCH v2 07/10] arm/mm: Get rid of READ/WRITE_SYSREG32

[Julien Grall]

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

Re: [PATCH v2 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32

[Julien Grall]

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 types of following members of struct gic_v3 to register_t:
> -vmcr
> -sre_el1
> -apr0
> -apr1
> 
> Add new macro GICC_IAR_INTID_MASK containing the mask
> for INTID field of ICC_IAR0/1_EL1 register.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> Changes since v1:
> -Remove hcr member of gic_v3 in a seperate patch
> -Add macro GICC_IAR_INTID_MASK

This change needs to be explained in the commit message. Saying 
something like:

"Only the first 23-bits of IAR contains the interrupt number. The rest 
are RES0. Therefore, take the opportunity to mask the bits [23:31] as 
they should be used for an IRQ number (we don't know how the top bits 
will be used).
"

> -Remove explicit cast in favor of implicit cast
> ---
>   xen/arch/arm/gic-v3-lpi.c         |  2 +-
>   xen/arch/arm/gic-v3.c             | 98 ++++++++++++++++---------------
>   xen/include/asm-arm/gic.h         |  6 +-
>   xen/include/asm-arm/gic_v3_defs.h |  2 +
>   4 files changed, 58 insertions(+), 50 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 869bc97fa1..e1594dd20e 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -178,7 +178,7 @@ void gicv3_do_LPI(unsigned int lpi)
>       irq_enter();
>   
>       /* EOI the LPI already. */
> -    WRITE_SYSREG32(lpi, ICC_EOIR1_EL1);
> +    WRITE_SYSREG(lpi, ICC_EOIR1_EL1);
>   
>       /* Find out if a guest mapped something to this physical LPI. */
>       hlpip = gic_get_host_lpi(lpi);
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index ac28013c19..b86f040589 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -246,12 +246,12 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
>    */
>   static void gicv3_enable_sre(void)
>   {
> -    uint32_t val;
> +    register_t val;
>   
> -    val = READ_SYSREG32(ICC_SRE_EL2);
> +    val = READ_SYSREG(ICC_SRE_EL2);
>       val |= GICC_SRE_EL2_SRE;
>   
> -    WRITE_SYSREG32(val, ICC_SRE_EL2);
> +    WRITE_SYSREG(val, ICC_SRE_EL2);
>       isb();
>   }
>   
> @@ -315,16 +315,16 @@ static void restore_aprn_regs(const union gic_state_data *d)
>       switch ( gicv3.nr_priorities )
>       {
>       case 7:
> -        WRITE_SYSREG32(d->v3.apr0[2], ICH_AP0R2_EL2);
> -        WRITE_SYSREG32(d->v3.apr1[2], ICH_AP1R2_EL2);
> +        WRITE_SYSREG(d->v3.apr0[2], ICH_AP0R2_EL2);
> +        WRITE_SYSREG(d->v3.apr1[2], ICH_AP1R2_EL2);
>           /* Fall through */
>       case 6:
> -        WRITE_SYSREG32(d->v3.apr0[1], ICH_AP0R1_EL2);
> -        WRITE_SYSREG32(d->v3.apr1[1], ICH_AP1R1_EL2);
> +        WRITE_SYSREG(d->v3.apr0[1], ICH_AP0R1_EL2);
> +        WRITE_SYSREG(d->v3.apr1[1], ICH_AP1R1_EL2);
>           /* Fall through */
>       case 5:
> -        WRITE_SYSREG32(d->v3.apr0[0], ICH_AP0R0_EL2);
> -        WRITE_SYSREG32(d->v3.apr1[0], ICH_AP1R0_EL2);
> +        WRITE_SYSREG(d->v3.apr0[0], ICH_AP0R0_EL2);
> +        WRITE_SYSREG(d->v3.apr1[0], ICH_AP1R0_EL2);
>           break;
>       default:
>           BUG();
> @@ -338,16 +338,16 @@ static void save_aprn_regs(union gic_state_data *d)
>       switch ( gicv3.nr_priorities )
>       {
>       case 7:
> -        d->v3.apr0[2] = READ_SYSREG32(ICH_AP0R2_EL2);
> -        d->v3.apr1[2] = READ_SYSREG32(ICH_AP1R2_EL2);
> +        d->v3.apr0[2] = READ_SYSREG(ICH_AP0R2_EL2);
> +        d->v3.apr1[2] = READ_SYSREG(ICH_AP1R2_EL2);
>           /* Fall through */
>       case 6:
> -        d->v3.apr0[1] = READ_SYSREG32(ICH_AP0R1_EL2);
> -        d->v3.apr1[1] = READ_SYSREG32(ICH_AP1R1_EL2);
> +        d->v3.apr0[1] = READ_SYSREG(ICH_AP0R1_EL2);
> +        d->v3.apr1[1] = READ_SYSREG(ICH_AP1R1_EL2);
>           /* Fall through */
>       case 5:
> -        d->v3.apr0[0] = READ_SYSREG32(ICH_AP0R0_EL2);
> -        d->v3.apr1[0] = READ_SYSREG32(ICH_AP1R0_EL2);
> +        d->v3.apr0[0] = READ_SYSREG(ICH_AP0R0_EL2);
> +        d->v3.apr1[0] = READ_SYSREG(ICH_AP1R0_EL2);
>           break;
>       default:
>           BUG();
> @@ -371,15 +371,15 @@ static void gicv3_save_state(struct vcpu *v)
>       dsb(sy);
>       gicv3_save_lrs(v);
>       save_aprn_regs(&v->arch.gic);
> -    v->arch.gic.v3.vmcr = READ_SYSREG32(ICH_VMCR_EL2);
> -    v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
> +    v->arch.gic.v3.vmcr = READ_SYSREG(ICH_VMCR_EL2);
> +    v->arch.gic.v3.sre_el1 = READ_SYSREG(ICC_SRE_EL1);
>   }
>   
>   static void gicv3_restore_state(const struct vcpu *v)
>   {
> -    uint32_t val;
> +    register_t val;
>   
> -    val = READ_SYSREG32(ICC_SRE_EL2);
> +    val = READ_SYSREG(ICC_SRE_EL2);
>       /*
>        * Don't give access to system registers when the guest is using
>        * GICv2
> @@ -388,7 +388,7 @@ static void gicv3_restore_state(const struct vcpu *v)
>           val &= ~GICC_SRE_EL2_ENEL1;
>       else
>           val |= GICC_SRE_EL2_ENEL1;
> -    WRITE_SYSREG32(val, ICC_SRE_EL2);
> +    WRITE_SYSREG(val, ICC_SRE_EL2);
>   
>       /*
>        * VFIQEn is RES1 if ICC_SRE_EL1.SRE is 1. This causes a Group0
> @@ -398,9 +398,9 @@ static void gicv3_restore_state(const struct vcpu *v)
>        * want before starting to mess with the rest of the GIC, and
>        * VMCR_EL1 in particular.
>        */
> -    WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
> +    WRITE_SYSREG(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
>       isb();
> -    WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
> +    WRITE_SYSREG(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
>       restore_aprn_regs(&v->arch.gic);
>       gicv3_restore_lrs(v);
>   
> @@ -468,24 +468,25 @@ static void gicv3_mask_irq(struct irq_desc *irqd)
>   static void gicv3_eoi_irq(struct irq_desc *irqd)
>   {
>       /* Lower the priority */
> -    WRITE_SYSREG32(irqd->irq, ICC_EOIR1_EL1);
> +    WRITE_SYSREG(irqd->irq, ICC_EOIR1_EL1);
>       isb();
>   }
>   
>   static void gicv3_dir_irq(struct irq_desc *irqd)
>   {
>       /* Deactivate */
> -    WRITE_SYSREG32(irqd->irq, ICC_DIR_EL1);
> +    WRITE_SYSREG(irqd->irq, ICC_DIR_EL1);
>       isb();
>   }
>   
>   static unsigned int gicv3_read_irq(void)
>   {
> -    unsigned int irq = READ_SYSREG32(ICC_IAR1_EL1);
> +    register_t irq = READ_SYSREG(ICC_IAR1_EL1);
>   
>       dsb(sy);
>   
> -    return irq;
> +    /* IRQs are encoded using 23bit. */
> +    return (irq & GICC_IAR_INTID_MASK);
>   }
>   
>   /*
> @@ -857,16 +858,16 @@ static int gicv3_cpu_init(void)
>       gicv3_enable_sre();
>   
>       /* No priority grouping */
> -    WRITE_SYSREG32(0, ICC_BPR1_EL1);
> +    WRITE_SYSREG(0, ICC_BPR1_EL1);
>   
>       /* Set priority mask register */
> -    WRITE_SYSREG32(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
> +    WRITE_SYSREG(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
>   
>       /* EOI drops priority, DIR deactivates the interrupt (mode 1) */
> -    WRITE_SYSREG32(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
> +    WRITE_SYSREG(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
>   
>       /* Enable Group1 interrupts */
> -    WRITE_SYSREG32(1, ICC_IGRPEN1_EL1);
> +    WRITE_SYSREG(1, ICC_IGRPEN1_EL1);
>   
>       /* Sync at once at the end of cpu interface configuration */
>       isb();
> @@ -876,15 +877,15 @@ static int gicv3_cpu_init(void)
>   
>   static void gicv3_cpu_disable(void)
>   {
> -    WRITE_SYSREG32(0, ICC_CTLR_EL1);
> +    WRITE_SYSREG(0, ICC_CTLR_EL1);
>       isb();
>   }
>   
>   static void gicv3_hyp_init(void)
>   {
> -    uint32_t vtr;
> +    register_t vtr;
>   
> -    vtr = READ_SYSREG32(ICH_VTR_EL2);
> +    vtr = READ_SYSREG(ICH_VTR_EL2);
>       gicv3_info.nr_lrs  = (vtr & ICH_VTR_NRLRGS) + 1;
>       gicv3.nr_priorities = ((vtr >> ICH_VTR_PRIBITS_SHIFT) &
>                             ICH_VTR_PRIBITS_MASK) + 1;
> @@ -892,8 +893,8 @@ static void gicv3_hyp_init(void)
>       if ( !((gicv3.nr_priorities > 4) && (gicv3.nr_priorities < 8)) )
>           panic("GICv3: Invalid number of priority bits\n");
>   
> -    WRITE_SYSREG32(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
> -    WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
> +    WRITE_SYSREG(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
> +    WRITE_SYSREG(GICH_HCR_EN, ICH_HCR_EL2);
>   }
>   
>   /* Set up the per-CPU parts of the GIC for a secondary CPU */
> @@ -917,11 +918,11 @@ out:
>   
>   static void gicv3_hyp_disable(void)
>   {
> -    uint32_t hcr;
> +    register_t hcr;
>   
> -    hcr = READ_SYSREG32(ICH_HCR_EL2);
> +    hcr = READ_SYSREG(ICH_HCR_EL2);
>       hcr &= ~GICH_HCR_EN;
> -    WRITE_SYSREG32(hcr, ICH_HCR_EL2);
> +    WRITE_SYSREG(hcr, ICH_HCR_EL2);
>       isb();
>   }
>   
> @@ -1140,39 +1141,44 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>   
>   static void gicv3_hcr_status(uint32_t flag, bool status)
>   {
> -    uint32_t hcr;
> +    register_t hcr;
>   
> -    hcr = READ_SYSREG32(ICH_HCR_EL2);
> +    hcr = READ_SYSREG(ICH_HCR_EL2);
>       if ( status )
> -        WRITE_SYSREG32(hcr | flag, ICH_HCR_EL2);
> +        WRITE_SYSREG(hcr | flag, ICH_HCR_EL2);
>       else
> -        WRITE_SYSREG32(hcr & (~flag), ICH_HCR_EL2);
> +        WRITE_SYSREG(hcr & (~flag), ICH_HCR_EL2);
>       isb();
>   }
>   
>   static unsigned int gicv3_read_vmcr_priority(void)
>   {
> -   return ((READ_SYSREG32(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
> +   return ((READ_SYSREG(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
>               ICH_VMCR_PRIORITY_MASK);
>   }
>   
>   /* Only support reading GRP1 APRn registers */
>   static unsigned int gicv3_read_apr(int apr_reg)
>   {
> +    register_t apr;
> +
>       switch ( apr_reg )
>       {
>       case 0:
>           ASSERT(gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8);
> -        return READ_SYSREG32(ICH_AP1R0_EL2);
> +        apr = READ_SYSREG(ICH_AP1R0_EL2);
>       case 1:
>           ASSERT(gicv3.nr_priorities > 5 && gicv3.nr_priorities < 8);
> -        return READ_SYSREG32(ICH_AP1R1_EL2);
> +        apr = READ_SYSREG(ICH_AP1R1_EL2);
>       case 2:
>           ASSERT(gicv3.nr_priorities > 6 && gicv3.nr_priorities < 8);
> -        return READ_SYSREG32(ICH_AP1R2_EL2);
> +        apr = READ_SYSREG(ICH_AP1R2_EL2);
>       default:
>           BUG();
>       }
> +
> +    /* Number of priority levels do not exceed 32bit. */
> +    return apr;
>   }
>   
>   static bool gicv3_read_pending_state(struct irq_desc *irqd)
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 5069ab4aac..c7f0c343d1 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -171,9 +171,9 @@
>    * GICv3 registers that needs to be saved/restored
>    */
>   struct gic_v3 {
> -    uint32_t vmcr, sre_el1;
> -    uint32_t apr0[4];
> -    uint32_t apr1[4];
> +    register_t vmcr, sre_el1;
> +    register_t apr0[4];
> +    register_t apr1[4];
>       uint64_t lr[16];
>   };
>   #endif
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 5a578e7c11..34ed5f857d 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -45,6 +45,8 @@
>   #define GICC_SRE_EL2_DIB             (1UL << 2)
>   #define GICC_SRE_EL2_ENEL1           (1UL << 3)
>   
> +#define GICC_IAR_INTID_MASK          (0xFFFFFF)
> +
>   /* Additional bits in GICD_TYPER defined by GICv3 */
>   #define GICD_TYPE_ID_BITS_SHIFT 19
>   #define GICD_TYPE_ID_BITS(r)    ((((r) >> GICD_TYPE_ID_BITS_SHIFT) & 0x1f) + 1)
> 

-- 
Julien Grall

Re: [PATCH v2 09/10] arm/time,vtimer: Get rid of READ/WRITE_SYSREG32

[Julien Grall]

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 vtimer structure's member: ctl to register_t.
> 
> Add macro CNTFRQ_MASK containing mask for timer clock frequency
> field of CNTFRQ_EL0 register.
> 
> Modify CNTx_CTL_MASK to return unsigned long instead of
> unsigned int as ctl is now of type register_t.

You modify all the CTX_CTL_* macros.

This can be fixed on commit.

> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

[Julien Grall]

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

Re: [PATCH v2 02/10] arm/domain: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

Hi Julien,

On 27.04.2021 11:45, Julien Grall wrote:
> 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 register cntkctl to register_t.
>> Modify accesses to thumbee registers to use READ/WRITE_SYSREG.
>> No need to change type of thumbee registers to register_t as they
>> only exist on arm32.
> 
> This is a bit ambiguous, I think in this context you mean it is only usable by 32-bit domain but should really be only accessed on Armv7 as they were restrospectively dropped on Armv8.
> 
Is the following ok for a v3?:
"
...
Modify type of register cntkctl to register_t.

Modify accesses to thumbee registers to use READ/WRITE_SYSREG.
Thumbee registers are only usable by a 32bit domain and in fact
should be only accessed on ARMv7 as they were retrospectively dropped
on ARMv8.
"
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> Changes since v1:
>> -Move modification of ACTLR into seperate patch
>> ---
>>   xen/arch/arm/domain.c        | 18 +++++++++---------
>>   xen/include/asm-arm/domain.h |  2 +-
>>   2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index bdd3d3e5b5..621f518b83 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -113,13 +113,13 @@ static void ctxt_switch_from(struct vcpu *p)
>>       p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
>>         /* Arch timer */
>> -    p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
>> +    p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
>>       virt_timer_save(p);
>>         if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
>>       {
>> -        p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
>> -        p->arch.teehbr = READ_SYSREG32(TEEHBR32_EL1);
>> +        p->arch.teecr = READ_SYSREG(TEECR32_EL1);
>> +        p->arch.teehbr = READ_SYSREG(TEEHBR32_EL1);
>>       }
>>     #ifdef CONFIG_ARM_32
>> @@ -175,7 +175,7 @@ static void ctxt_switch_from(struct vcpu *p)
>>     static void ctxt_switch_to(struct vcpu *n)
>>   {
>> -    uint32_t vpidr;
>> +    register_t vpidr;
>>         /* When the idle VCPU is running, Xen will always stay in hypervisor
>>        * mode. Therefore we don't need to restore the context of an idle VCPU.
>> @@ -183,8 +183,8 @@ static void ctxt_switch_to(struct vcpu *n)
>>       if ( is_idle_vcpu(n) )
>>           return;
>>   -    vpidr = READ_SYSREG32(MIDR_EL1);
>> -    WRITE_SYSREG32(vpidr, VPIDR_EL2);
>> +    vpidr = READ_SYSREG(MIDR_EL1);
>> +    WRITE_SYSREG(vpidr, VPIDR_EL2);
>>       WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
>>         /* VGIC */
>> @@ -257,8 +257,8 @@ static void ctxt_switch_to(struct vcpu *n)
>>         if ( is_32bit_domain(n->domain) && cpu_has_thumbee )
>>       {
>> -        WRITE_SYSREG32(n->arch.teecr, TEECR32_EL1);
>> -        WRITE_SYSREG32(n->arch.teehbr, TEEHBR32_EL1);
>> +        WRITE_SYSREG(n->arch.teecr, TEECR32_EL1);
>> +        WRITE_SYSREG(n->arch.teehbr, TEEHBR32_EL1);
>>       }
>>     #ifdef CONFIG_ARM_32
>> @@ -274,7 +274,7 @@ static void ctxt_switch_to(struct vcpu *n)
>>         /* This is could trigger an hardware interrupt from the virtual
>>        * timer. The interrupt needs to be injected into the guest. */
>> -    WRITE_SYSREG32(n->arch.cntkctl, CNTKCTL_EL1);
>> +    WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
>>       virt_timer_restore(n);
>>   }
>>   diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 0a74df9931..c6b59ee755 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -190,7 +190,7 @@ struct arch_vcpu
>>       struct vgic_cpu vgic;
>>         /* Timer registers  */
>> -    uint32_t cntkctl;
>> +    register_t cntkctl;
>>         struct vtimer phys_timer;
>>       struct vtimer virt_timer;
>>
> 

Re: [PATCH v2 03/10] arm: Modify type of actlr to register_t

[Michal Orzel]

Hi Julien,

On 27.04.2021 11:47, Julien Grall wrote:
> 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.
> 
> This is a pretty generic message but doesn't really explain the change itself and point out this is a bug (possibly latent on current HW) because it is implementation defined. IOW a CPU implementer may already have decided to use the top 32-bit without our knowledge.
> 
> Cheers,
> Is the following ok?
"
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.

ACTLR_EL1 system register bits are implementation defined
which means it is possibly a bug on current HW as the CPU
implementer may already have decided to use the top 32bit.
"

Cheers
Michal

Re: [PATCH v2 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

Hi Julien,

On 27.04.2021 12:02, Julien Grall wrote:
> 
> 
> 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 types of following members of struct gic_v3 to register_t:
>> -vmcr
>> -sre_el1
>> -apr0
>> -apr1
>>
>> Add new macro GICC_IAR_INTID_MASK containing the mask
>> for INTID field of ICC_IAR0/1_EL1 register.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> Changes since v1:
>> -Remove hcr member of gic_v3 in a seperate patch
>> -Add macro GICC_IAR_INTID_MASK
> 
> This change needs to be explained in the commit message. Saying something like:
> 
> "Only the first 23-bits of IAR contains the interrupt number. The rest are RES0. Therefore, take the opportunity to mask the bits [23:31] as they should be used for an IRQ number (we don't know how the top bits will be used).
> "
> 
Ok, will change in v3.
>> -Remove explicit cast in favor of implicit cast
>> ---
>>   xen/arch/arm/gic-v3-lpi.c         |  2 +-
>>   xen/arch/arm/gic-v3.c             | 98 ++++++++++++++++---------------
>>   xen/include/asm-arm/gic.h         |  6 +-
>>   xen/include/asm-arm/gic_v3_defs.h |  2 +
>>   4 files changed, 58 insertions(+), 50 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
>> index 869bc97fa1..e1594dd20e 100644
>> --- a/xen/arch/arm/gic-v3-lpi.c
>> +++ b/xen/arch/arm/gic-v3-lpi.c
>> @@ -178,7 +178,7 @@ void gicv3_do_LPI(unsigned int lpi)
>>       irq_enter();
>>         /* EOI the LPI already. */
>> -    WRITE_SYSREG32(lpi, ICC_EOIR1_EL1);
>> +    WRITE_SYSREG(lpi, ICC_EOIR1_EL1);
>>         /* Find out if a guest mapped something to this physical LPI. */
>>       hlpip = gic_get_host_lpi(lpi);
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index ac28013c19..b86f040589 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -246,12 +246,12 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
>>    */
>>   static void gicv3_enable_sre(void)
>>   {
>> -    uint32_t val;
>> +    register_t val;
>>   -    val = READ_SYSREG32(ICC_SRE_EL2);
>> +    val = READ_SYSREG(ICC_SRE_EL2);
>>       val |= GICC_SRE_EL2_SRE;
>>   -    WRITE_SYSREG32(val, ICC_SRE_EL2);
>> +    WRITE_SYSREG(val, ICC_SRE_EL2);
>>       isb();
>>   }
>>   @@ -315,16 +315,16 @@ static void restore_aprn_regs(const union gic_state_data *d)
>>       switch ( gicv3.nr_priorities )
>>       {
>>       case 7:
>> -        WRITE_SYSREG32(d->v3.apr0[2], ICH_AP0R2_EL2);
>> -        WRITE_SYSREG32(d->v3.apr1[2], ICH_AP1R2_EL2);
>> +        WRITE_SYSREG(d->v3.apr0[2], ICH_AP0R2_EL2);
>> +        WRITE_SYSREG(d->v3.apr1[2], ICH_AP1R2_EL2);
>>           /* Fall through */
>>       case 6:
>> -        WRITE_SYSREG32(d->v3.apr0[1], ICH_AP0R1_EL2);
>> -        WRITE_SYSREG32(d->v3.apr1[1], ICH_AP1R1_EL2);
>> +        WRITE_SYSREG(d->v3.apr0[1], ICH_AP0R1_EL2);
>> +        WRITE_SYSREG(d->v3.apr1[1], ICH_AP1R1_EL2);
>>           /* Fall through */
>>       case 5:
>> -        WRITE_SYSREG32(d->v3.apr0[0], ICH_AP0R0_EL2);
>> -        WRITE_SYSREG32(d->v3.apr1[0], ICH_AP1R0_EL2);
>> +        WRITE_SYSREG(d->v3.apr0[0], ICH_AP0R0_EL2);
>> +        WRITE_SYSREG(d->v3.apr1[0], ICH_AP1R0_EL2);
>>           break;
>>       default:
>>           BUG();
>> @@ -338,16 +338,16 @@ static void save_aprn_regs(union gic_state_data *d)
>>       switch ( gicv3.nr_priorities )
>>       {
>>       case 7:
>> -        d->v3.apr0[2] = READ_SYSREG32(ICH_AP0R2_EL2);
>> -        d->v3.apr1[2] = READ_SYSREG32(ICH_AP1R2_EL2);
>> +        d->v3.apr0[2] = READ_SYSREG(ICH_AP0R2_EL2);
>> +        d->v3.apr1[2] = READ_SYSREG(ICH_AP1R2_EL2);
>>           /* Fall through */
>>       case 6:
>> -        d->v3.apr0[1] = READ_SYSREG32(ICH_AP0R1_EL2);
>> -        d->v3.apr1[1] = READ_SYSREG32(ICH_AP1R1_EL2);
>> +        d->v3.apr0[1] = READ_SYSREG(ICH_AP0R1_EL2);
>> +        d->v3.apr1[1] = READ_SYSREG(ICH_AP1R1_EL2);
>>           /* Fall through */
>>       case 5:
>> -        d->v3.apr0[0] = READ_SYSREG32(ICH_AP0R0_EL2);
>> -        d->v3.apr1[0] = READ_SYSREG32(ICH_AP1R0_EL2);
>> +        d->v3.apr0[0] = READ_SYSREG(ICH_AP0R0_EL2);
>> +        d->v3.apr1[0] = READ_SYSREG(ICH_AP1R0_EL2);
>>           break;
>>       default:
>>           BUG();
>> @@ -371,15 +371,15 @@ static void gicv3_save_state(struct vcpu *v)
>>       dsb(sy);
>>       gicv3_save_lrs(v);
>>       save_aprn_regs(&v->arch.gic);
>> -    v->arch.gic.v3.vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>> -    v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
>> +    v->arch.gic.v3.vmcr = READ_SYSREG(ICH_VMCR_EL2);
>> +    v->arch.gic.v3.sre_el1 = READ_SYSREG(ICC_SRE_EL1);
>>   }
>>     static void gicv3_restore_state(const struct vcpu *v)
>>   {
>> -    uint32_t val;
>> +    register_t val;
>>   -    val = READ_SYSREG32(ICC_SRE_EL2);
>> +    val = READ_SYSREG(ICC_SRE_EL2);
>>       /*
>>        * Don't give access to system registers when the guest is using
>>        * GICv2
>> @@ -388,7 +388,7 @@ static void gicv3_restore_state(const struct vcpu *v)
>>           val &= ~GICC_SRE_EL2_ENEL1;
>>       else
>>           val |= GICC_SRE_EL2_ENEL1;
>> -    WRITE_SYSREG32(val, ICC_SRE_EL2);
>> +    WRITE_SYSREG(val, ICC_SRE_EL2);
>>         /*
>>        * VFIQEn is RES1 if ICC_SRE_EL1.SRE is 1. This causes a Group0
>> @@ -398,9 +398,9 @@ static void gicv3_restore_state(const struct vcpu *v)
>>        * want before starting to mess with the rest of the GIC, and
>>        * VMCR_EL1 in particular.
>>        */
>> -    WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
>> +    WRITE_SYSREG(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
>>       isb();
>> -    WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
>> +    WRITE_SYSREG(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
>>       restore_aprn_regs(&v->arch.gic);
>>       gicv3_restore_lrs(v);
>>   @@ -468,24 +468,25 @@ static void gicv3_mask_irq(struct irq_desc *irqd)
>>   static void gicv3_eoi_irq(struct irq_desc *irqd)
>>   {
>>       /* Lower the priority */
>> -    WRITE_SYSREG32(irqd->irq, ICC_EOIR1_EL1);
>> +    WRITE_SYSREG(irqd->irq, ICC_EOIR1_EL1);
>>       isb();
>>   }
>>     static void gicv3_dir_irq(struct irq_desc *irqd)
>>   {
>>       /* Deactivate */
>> -    WRITE_SYSREG32(irqd->irq, ICC_DIR_EL1);
>> +    WRITE_SYSREG(irqd->irq, ICC_DIR_EL1);
>>       isb();
>>   }
>>     static unsigned int gicv3_read_irq(void)
>>   {
>> -    unsigned int irq = READ_SYSREG32(ICC_IAR1_EL1);
>> +    register_t irq = READ_SYSREG(ICC_IAR1_EL1);
>>         dsb(sy);
>>   -    return irq;
>> +    /* IRQs are encoded using 23bit. */
>> +    return (irq & GICC_IAR_INTID_MASK);
>>   }
>>     /*
>> @@ -857,16 +858,16 @@ static int gicv3_cpu_init(void)
>>       gicv3_enable_sre();
>>         /* No priority grouping */
>> -    WRITE_SYSREG32(0, ICC_BPR1_EL1);
>> +    WRITE_SYSREG(0, ICC_BPR1_EL1);
>>         /* Set priority mask register */
>> -    WRITE_SYSREG32(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
>> +    WRITE_SYSREG(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
>>         /* EOI drops priority, DIR deactivates the interrupt (mode 1) */
>> -    WRITE_SYSREG32(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
>> +    WRITE_SYSREG(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);
>>         /* Enable Group1 interrupts */
>> -    WRITE_SYSREG32(1, ICC_IGRPEN1_EL1);
>> +    WRITE_SYSREG(1, ICC_IGRPEN1_EL1);
>>         /* Sync at once at the end of cpu interface configuration */
>>       isb();
>> @@ -876,15 +877,15 @@ static int gicv3_cpu_init(void)
>>     static void gicv3_cpu_disable(void)
>>   {
>> -    WRITE_SYSREG32(0, ICC_CTLR_EL1);
>> +    WRITE_SYSREG(0, ICC_CTLR_EL1);
>>       isb();
>>   }
>>     static void gicv3_hyp_init(void)
>>   {
>> -    uint32_t vtr;
>> +    register_t vtr;
>>   -    vtr = READ_SYSREG32(ICH_VTR_EL2);
>> +    vtr = READ_SYSREG(ICH_VTR_EL2);
>>       gicv3_info.nr_lrs  = (vtr & ICH_VTR_NRLRGS) + 1;
>>       gicv3.nr_priorities = ((vtr >> ICH_VTR_PRIBITS_SHIFT) &
>>                             ICH_VTR_PRIBITS_MASK) + 1;
>> @@ -892,8 +893,8 @@ static void gicv3_hyp_init(void)
>>       if ( !((gicv3.nr_priorities > 4) && (gicv3.nr_priorities < 8)) )
>>           panic("GICv3: Invalid number of priority bits\n");
>>   -    WRITE_SYSREG32(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
>> -    WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
>> +    WRITE_SYSREG(ICH_VMCR_EOI | ICH_VMCR_VENG1, ICH_VMCR_EL2);
>> +    WRITE_SYSREG(GICH_HCR_EN, ICH_HCR_EL2);
>>   }
>>     /* Set up the per-CPU parts of the GIC for a secondary CPU */
>> @@ -917,11 +918,11 @@ out:
>>     static void gicv3_hyp_disable(void)
>>   {
>> -    uint32_t hcr;
>> +    register_t hcr;
>>   -    hcr = READ_SYSREG32(ICH_HCR_EL2);
>> +    hcr = READ_SYSREG(ICH_HCR_EL2);
>>       hcr &= ~GICH_HCR_EN;
>> -    WRITE_SYSREG32(hcr, ICH_HCR_EL2);
>> +    WRITE_SYSREG(hcr, ICH_HCR_EL2);
>>       isb();
>>   }
>>   @@ -1140,39 +1141,44 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>>     static void gicv3_hcr_status(uint32_t flag, bool status)
>>   {
>> -    uint32_t hcr;
>> +    register_t hcr;
>>   -    hcr = READ_SYSREG32(ICH_HCR_EL2);
>> +    hcr = READ_SYSREG(ICH_HCR_EL2);
>>       if ( status )
>> -        WRITE_SYSREG32(hcr | flag, ICH_HCR_EL2);
>> +        WRITE_SYSREG(hcr | flag, ICH_HCR_EL2);
>>       else
>> -        WRITE_SYSREG32(hcr & (~flag), ICH_HCR_EL2);
>> +        WRITE_SYSREG(hcr & (~flag), ICH_HCR_EL2);
>>       isb();
>>   }
>>     static unsigned int gicv3_read_vmcr_priority(void)
>>   {
>> -   return ((READ_SYSREG32(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
>> +   return ((READ_SYSREG(ICH_VMCR_EL2) >> ICH_VMCR_PRIORITY_SHIFT) &
>>               ICH_VMCR_PRIORITY_MASK);
>>   }
>>     /* Only support reading GRP1 APRn registers */
>>   static unsigned int gicv3_read_apr(int apr_reg)
>>   {
>> +    register_t apr;
>> +
>>       switch ( apr_reg )
>>       {
>>       case 0:
>>           ASSERT(gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8);
>> -        return READ_SYSREG32(ICH_AP1R0_EL2);
>> +        apr = READ_SYSREG(ICH_AP1R0_EL2);
>>       case 1:
>>           ASSERT(gicv3.nr_priorities > 5 && gicv3.nr_priorities < 8);
>> -        return READ_SYSREG32(ICH_AP1R1_EL2);
>> +        apr = READ_SYSREG(ICH_AP1R1_EL2);
>>       case 2:
>>           ASSERT(gicv3.nr_priorities > 6 && gicv3.nr_priorities < 8);
>> -        return READ_SYSREG32(ICH_AP1R2_EL2);
>> +        apr = READ_SYSREG(ICH_AP1R2_EL2);
>>       default:
>>           BUG();
>>       }
>> +
>> +    /* Number of priority levels do not exceed 32bit. */
>> +    return apr;
>>   }
>>     static bool gicv3_read_pending_state(struct irq_desc *irqd)
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 5069ab4aac..c7f0c343d1 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -171,9 +171,9 @@
>>    * GICv3 registers that needs to be saved/restored
>>    */
>>   struct gic_v3 {
>> -    uint32_t vmcr, sre_el1;
>> -    uint32_t apr0[4];
>> -    uint32_t apr1[4];
>> +    register_t vmcr, sre_el1;
>> +    register_t apr0[4];
>> +    register_t apr1[4];
>>       uint64_t lr[16];
>>   };
>>   #endif
>> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
>> index 5a578e7c11..34ed5f857d 100644
>> --- a/xen/include/asm-arm/gic_v3_defs.h
>> +++ b/xen/include/asm-arm/gic_v3_defs.h
>> @@ -45,6 +45,8 @@
>>   #define GICC_SRE_EL2_DIB             (1UL << 2)
>>   #define GICC_SRE_EL2_ENEL1           (1UL << 3)
>>   +#define GICC_IAR_INTID_MASK          (0xFFFFFF)
>> +
>>   /* Additional bits in GICD_TYPER defined by GICv3 */
>>   #define GICD_TYPE_ID_BITS_SHIFT 19
>>   #define GICD_TYPE_ID_BITS(r)    ((((r) >> GICD_TYPE_ID_BITS_SHIFT) & 0x1f) + 1)
>>
> 

Re: [PATCH v2 07/10] arm/mm: Get rid of READ/WRITE_SYSREG32

[Michal Orzel]

Hi Julien,

On 27.04.2021 11:59, Julien Grall wrote:
> 
> 
> 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.
> "
> 
Thank you. I will modify the commit msg according to what you suggested.
> Cheers,
> 

Re: [PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

[Michal Orzel]

Hi Julien,

On 27.04.2021 13:09, Julien Grall wrote:
> 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.
> 
How about:
"
Modify type of hsr, cpsr, spsr_el1 to uint64_t.
Previously we relied on the padding after SPSR_EL1. As we removed the padding, modify the union to be 64bit
so we don't corrupt SPSR_FIQ.
No need to modify the assembly code becuase the accesses were based on 64bit registers as there was a 32bit padding after SPSR_EL1.
"
>>
>> 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.
> 
Ok I'll remove _res0 members. But bits can be of type register_t, right?
>> 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).
> 
Ok so I'll increment XEN_DOMCTL_INTERFACE_VERSION and write in commit msg:
"
Change type of cpsr to uint64_t in the public outside interface "public/arch-arm.h" to allow ABI compatibility between 32bit and 64bit.
Increment XEN_DOMCTL_INTERFACE_VERSION.
"
>>         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.
> 
Ok so I will change type of cpsr here to uint64_t, increment VM_EVENT_INTERFACE_VERSION and write in commit msg:
"
Change type of cpsr to uint64_t in the public outside interface "public/vm_event.h" to allow ABI compatibility between 32bit and 64bit.
Increment VM_EVENT_INTERFACE_VERSION.
"
Ok?
> Cheers,
> 
Cheers,
Michal

Re: [PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

[Tamas K Lengyel]

[-- Attachment #1: Type: text/plain, Size: 5426 bytes --]

On Thu, Apr 29, 2021, 4:53 AM Michal Orzel <michal.orzel@arm.com> wrote:

> Hi Julien,
>
> On 27.04.2021 13:09, Julien Grall wrote:
> > 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.
> >
> How about:
> "
> Modify type of hsr, cpsr, spsr_el1 to uint64_t.
> Previously we relied on the padding after SPSR_EL1. As we removed the
> padding, modify the union to be 64bit
> so we don't corrupt SPSR_FIQ.
> No need to modify the assembly code becuase the accesses were based on
> 64bit registers as there was a 32bit padding after SPSR_EL1.
> "
> >>
> >> 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.
> >
> Ok I'll remove _res0 members. But bits can be of type register_t, right?
> >> 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).
> >
> Ok so I'll increment XEN_DOMCTL_INTERFACE_VERSION and write in commit msg:
> "
> Change type of cpsr to uint64_t in the public outside interface
> "public/arch-arm.h" to allow ABI compatibility between 32bit and 64bit.
> Increment XEN_DOMCTL_INTERFACE_VERSION.
> "
> >>         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.
> >
> Ok so I will change type of cpsr here to uint64_t, increment
> VM_EVENT_INTERFACE_VERSION and write in commit msg:
> "
> Change type of cpsr to uint64_t in the public outside interface
> "public/vm_event.h" to allow ABI compatibility between 32bit and 64bit.
> Increment VM_EVENT_INTERFACE_VERSION.
> "
> Ok?
>

There is no need to bump the interface version for this, you are not
changing the layout or size of the structure since there was already 64bit
space there for cspr for both 32bit and 64bit builds. You are just folding
that padding field into cspr on 32bit builds.

Thanks,
Tamas

>

[-- Attachment #2: Type: text/html, Size: 7112 bytes --]

Re: [PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

[Julien Grall]

On 29/04/2021 11:31, Tamas K Lengyel wrote:
> On Thu, Apr 29, 2021, 4:53 AM Michal Orzel <michal.orzel@arm.com 
> <mailto:michal.orzel@arm.com>> wrote:
> 
>     Hi Julien,
> 
>     On 27.04.2021 13:09, Julien Grall wrote:
>      > 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.
>      >
>     How about:
>     "
>     Modify type of hsr, cpsr, spsr_el1 to uint64_t.
>     Previously we relied on the padding after SPSR_EL1. As we removed
>     the padding, modify the union to be 64bit
>     so we don't corrupt SPSR_FIQ.
>     No need to modify the assembly code becuase the accesses were based
>     on 64bit registers as there was a 32bit padding after SPSR_EL1.
>     "
>      >>
>      >> 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
>     <mailto: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.
>      >
>     Ok I'll remove _res0 members. But bits can be of type register_t, right?
>      >> 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).
>      >
>     Ok so I'll increment XEN_DOMCTL_INTERFACE_VERSION and write in
>     commit msg:
>     "
>     Change type of cpsr to uint64_t in the public outside interface
>     "public/arch-arm.h" to allow ABI compatibility between 32bit and 64bit.
>     Increment XEN_DOMCTL_INTERFACE_VERSION.
>     "
>      >>         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.
>      >
>     Ok so I will change type of cpsr here to uint64_t, increment
>     VM_EVENT_INTERFACE_VERSION and write in commit msg:
>     "
>     Change type of cpsr to uint64_t in the public outside interface
>     "public/vm_event.h" to allow ABI compatibility between 32bit and 64bit.
>     Increment VM_EVENT_INTERFACE_VERSION.
>     "
>     Ok?
> 
> 
> There is no need to bump the interface version for this, you are not 
> changing the layout or size of the structure since there was already 
> 64bit space there for cspr for both 32bit and 64bit builds. You are just 
> folding that padding field into cspr on 32bit builds.

Ah I didn't realize the padding was already there. Although, is it 
always zeroed?

If not, then this is would technically be an ABI breakage if you build a 
vm event tool using public/vm_event.h from Xen 4.16 and use it on Xen 4.15.

Cheers,

-- 
Julien Grall