All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32
@ 2021-05-05  7:42 Michal Orzel
  2021-05-05  7:42 ` [PATCH v3 01/10] arm64/vfp: " Michal Orzel
                   ` (10 more replies)
  0 siblings, 11 replies; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:42 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Tamas K Lengyel, Alexandru Isaila, Petre Pircalabu,
	bertrand.marquis, wei.chen

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
  xen/arm: Always access SCTLR_EL2 using READ/WRITE_SYSREG()
  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             |  2 +-
 xen/include/asm-arm/page.h            |  4 +-
 xen/include/asm-arm/processor.h       |  5 +-
 xen/include/public/arch-arm.h         |  4 +-
 xen/include/public/domctl.h           |  2 +-
 xen/include/public/vm_event.h         |  3 +-
 24 files changed, 153 insertions(+), 136 deletions(-)

-- 
2.29.0



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 01/10] arm64/vfp: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
@ 2021-05-05  7:42 ` Michal Orzel
  2021-05-05  7:43 ` [PATCH v3 02/10] arm/domain: " Michal Orzel
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:42 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen, Julien Grall

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



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 02/10] arm/domain: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
  2021-05-05  7:42 ` [PATCH v3 01/10] arm64/vfp: " Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05 18:03   ` Julien Grall
  2021-05-05  7:43 ` [PATCH v3 03/10] arm: Modify type of actlr to register_t Michal Orzel
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen

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.
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 v2:
-Modify the commit message
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



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 03/10] arm: Modify type of actlr to register_t
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
  2021-05-05  7:42 ` [PATCH v3 01/10] arm64/vfp: " Michal Orzel
  2021-05-05  7:43 ` [PATCH v3 02/10] arm/domain: " Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05 18:04   ` Julien Grall
  2021-05-05  7:43 ` [PATCH v3 04/10] arm/gic: Remove member hcr of structure gic_v3 Michal Orzel
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen

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 latent bug on current HW as the CPU
implementer may already have decided to use the top 32bit.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v2:
-Modify the commit message
---
 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



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 04/10] arm/gic: Remove member hcr of structure gic_v3
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (2 preceding siblings ...)
  2021-05-05  7:43 ` [PATCH v3 03/10] arm: Modify type of actlr to register_t Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05  7:43 ` [PATCH v3 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen, Julien Grall

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



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (3 preceding siblings ...)
  2021-05-05  7:43 ` [PATCH v3 04/10] arm/gic: Remove member hcr of structure gic_v3 Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05 18:06   ` Julien Grall
  2021-05-05  7:43 ` [PATCH v3 06/10] arm/p2m: " Michal Orzel
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen

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

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v2:
-Modify the commit message
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



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 06/10] arm/p2m: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (4 preceding siblings ...)
  2021-05-05  7:43 ` [PATCH v3 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32 Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05  7:43 ` [PATCH v3 07/10] xen/arm: Always access SCTLR_EL2 using READ/WRITE_SYSREG() Michal Orzel
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen, Julien Grall

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



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 07/10] xen/arm: Always access SCTLR_EL2 using READ/WRITE_SYSREG()
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (5 preceding siblings ...)
  2021-05-05  7:43 ` [PATCH v3 06/10] arm/p2m: " Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05 18:07   ` Julien Grall
  2021-05-05  7:43 ` [PATCH v3 08/10] arm/page: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen

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.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v2:
-Modify the commit message
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



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 08/10] arm/page: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (6 preceding siblings ...)
  2021-05-05  7:43 ` [PATCH v3 07/10] xen/arm: Always access SCTLR_EL2 using READ/WRITE_SYSREG() Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05  7:43 ` [PATCH v3 09/10] arm/time,vtimer: " Michal Orzel
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen, Julien Grall

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



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 09/10] arm/time,vtimer: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (7 preceding siblings ...)
  2021-05-05  7:43 ` [PATCH v3 08/10] arm/page: Get rid of READ/WRITE_SYSREG32 Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05  7:43 ` [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t Michal Orzel
  2021-05-10 17:19 ` [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Julien Grall
  10 siblings, 0 replies; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	bertrand.marquis, wei.chen, Julien Grall

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_* macros 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>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 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 |  5 ++++-
 4 files changed, 24 insertions(+), 21 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..2577e9a244 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -485,9 +485,12 @@ extern register_t __cpu_logical_map[];
 
 /* Timer control registers */
 #define CNTx_CTL_ENABLE   (1u<<0)  /* Enable timer */
-#define CNTx_CTL_MASK     (1u<<1)  /* Mask IRQ */
+#define CNTx_CTL_MASK     (1ul<<1)  /* Mask IRQ */
 #define CNTx_CTL_PENDING  (1u<<2)  /* IRQ pending */
 
+/* Timer frequency mask */
+#define CNTFRQ_MASK       GENMASK(31, 0)
+
 /* Exception Vector offsets */
 /* ... ARM32 */
 #define VECTOR32_RST  0
-- 
2.29.0



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (8 preceding siblings ...)
  2021-05-05  7:43 ` [PATCH v3 09/10] arm/time,vtimer: " Michal Orzel
@ 2021-05-05  7:43 ` Michal Orzel
  2021-05-05  8:00   ` Jan Beulich
  2021-06-07 13:31   ` Julien Grall
  2021-05-10 17:19 ` [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Julien Grall
  10 siblings, 2 replies; 31+ messages in thread
From: Michal Orzel @ 2021-05-05  7:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Tamas K Lengyel, Alexandru Isaila, Petre Pircalabu,
	bertrand.marquis, wei.chen

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

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.

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.

Change type of cpsr to uint64_t in the public outside interface
"public/vm_event.h" to allow ABI compatibility between 32bit and 64bit.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v2:
-Remove _res0 members from structures inside hsr union
-Update commit message
-Modify type of cpsr to uint64_t in public/arch-arm.h
-Increment XEN_DOMCTL_INTERFACE_VERSION
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             |  2 +-
 xen/include/public/arch-arm.h         |  4 ++--
 xen/include/public/domctl.h           |  2 +-
 xen/include/public/vm_event.h         |  3 +--
 11 files changed, 41 insertions(+), 35 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..9b91b28c48 100644
--- a/xen/include/asm-arm/hsr.h
+++ b/xen/include/asm-arm/hsr.h
@@ -16,7 +16,7 @@ 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 */
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 713fd65317..64a2ca30da 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 */
+    uint64_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/domctl.h b/xen/include/public/domctl.h
index 4dbf107785..d576bfabd6 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -38,7 +38,7 @@
 #include "hvm/save.h"
 #include "memory.h"
 
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 36135ba4f1..bb003d21d0 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -266,8 +266,7 @@ struct vm_event_regs_arm {
     uint64_t ttbr1;
     uint64_t ttbcr;
     uint64_t pc;
-    uint32_t cpsr;
-    uint32_t _pad;
+    uint64_t cpsr;
 };
 
 /*
-- 
2.29.0



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-05  7:43 ` [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t Michal Orzel
@ 2021-05-05  8:00   ` Jan Beulich
  2021-05-05 11:49     ` Tamas K Lengyel
  2021-05-11  6:37     ` Michal Orzel
  2021-06-07 13:31   ` Julien Grall
  1 sibling, 2 replies; 31+ messages in thread
From: Jan Beulich @ 2021-05-05  8:00 UTC (permalink / raw)
  To: Michal Orzel
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
	Tamas K Lengyel, Alexandru Isaila, Petre Pircalabu,
	bertrand.marquis, wei.chen, xen-devel

On 05.05.2021 09:43, Michal Orzel wrote:
> --- 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 */
> +    uint64_t cpsr;                              /* SPSR_EL2 */
>  
>      union {
> -        uint32_t spsr_el1;       /* AArch64 */
> +        uint64_t spsr_el1;       /* AArch64 */
>          uint32_t spsr_svc;       /* AArch32 */
>      };

This change affects, besides domctl, also default_initialise_vcpu(),
which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
only allows two unrelated VCPUOP_* to pass, but then I don't
understand why arch_initialise_vcpu() doesn't simply return e.g.
-EOPNOTSUPP. Hence I suspect I'm missing something.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014

So this is to cover for the struct vcpu_guest_core_regs change.

> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -266,8 +266,7 @@ struct vm_event_regs_arm {
>      uint64_t ttbr1;
>      uint64_t ttbcr;
>      uint64_t pc;
> -    uint32_t cpsr;
> -    uint32_t _pad;
> +    uint64_t cpsr;
>  };

Then I wonder why this isn't accompanied by a similar bump of
VM_EVENT_INTERFACE_VERSION. I don't see you drop any checking /
filling of the _pad field, so existing callers may pass garbage
there, and new callers need to be prevented from looking at the
upper half when running on an older hypervisor.

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-05  8:00   ` Jan Beulich
@ 2021-05-05 11:49     ` Tamas K Lengyel
  2021-05-11  6:37     ` Michal Orzel
  1 sibling, 0 replies; 31+ messages in thread
From: Tamas K Lengyel @ 2021-05-05 11:49 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Michal Orzel, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Wei Liu, Alexandru Isaila, Petre Pircalabu, bertrand.marquis,
	wei.chen, Xen-devel

On Wed, May 5, 2021 at 4:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.05.2021 09:43, Michal Orzel wrote:
> > --- 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 */
> > +    uint64_t cpsr;                              /* SPSR_EL2 */
> >
> >      union {
> > -        uint32_t spsr_el1;       /* AArch64 */
> > +        uint64_t spsr_el1;       /* AArch64 */
> >          uint32_t spsr_svc;       /* AArch32 */
> >      };
>
> This change affects, besides domctl, also default_initialise_vcpu(),
> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
> only allows two unrelated VCPUOP_* to pass, but then I don't
> understand why arch_initialise_vcpu() doesn't simply return e.g.
> -EOPNOTSUPP. Hence I suspect I'm missing something.
>
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -38,7 +38,7 @@
> >  #include "hvm/save.h"
> >  #include "memory.h"
> >
> > -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
> > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
>
> So this is to cover for the struct vcpu_guest_core_regs change.
>
> > --- a/xen/include/public/vm_event.h
> > +++ b/xen/include/public/vm_event.h
> > @@ -266,8 +266,7 @@ struct vm_event_regs_arm {
> >      uint64_t ttbr1;
> >      uint64_t ttbcr;
> >      uint64_t pc;
> > -    uint32_t cpsr;
> > -    uint32_t _pad;
> > +    uint64_t cpsr;
> >  };
>
> Then I wonder why this isn't accompanied by a similar bump of
> VM_EVENT_INTERFACE_VERSION. I don't see you drop any checking /
> filling of the _pad field, so existing callers may pass garbage
> there, and new callers need to be prevented from looking at the
> upper half when running on an older hypervisor.

No, there is no need to bump the vm event interface version here. They
are folding the _pad into the cpsr and the structure is always zero
initialized. There is never "garbage" in the _pad field. As such there
is no change to the structure layout or anyone using it with a header
compiled on an older version. I asked them not to bump the version for
this change.

Thanks,
Tamas


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 02/10] arm/domain: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:43 ` [PATCH v3 02/10] arm/domain: " Michal Orzel
@ 2021-05-05 18:03   ` Julien Grall
  2021-05-06  6:13     ` Michal Orzel
  0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2021-05-05 18:03 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis, wei.chen

Hi Michal,

On 05/05/2021 08:43, 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.
> 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.

Sorry for not replying on v2. How about:

"
Thumbee registers are only usable by a 32-bit domain and therefore we 
can just store the bottom 32-bit (IOW there is no type change). In fact, 
this could technically be restricted to Armv7 HW (the support was 
dropped retrospectively in Armv8) but leave it as-is for now.
"

If you are happy with it, I will do it on commit.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 03/10] arm: Modify type of actlr to register_t
  2021-05-05  7:43 ` [PATCH v3 03/10] arm: Modify type of actlr to register_t Michal Orzel
@ 2021-05-05 18:04   ` Julien Grall
  0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2021-05-05 18:04 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis, wei.chen

Hi Michal,

On 05/05/2021 08:43, 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.
> 
> ACTLR_EL1 system register bits are implementation defined
> which means it is possibly a latent bug on current HW as the CPU
> implementer may already have decided to use the top 32bit.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

This patch will want to be backported.

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

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:43 ` [PATCH v3 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32 Michal Orzel
@ 2021-05-05 18:06   ` Julien Grall
  0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2021-05-05 18:06 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis, wei.chen

Hi Michal,

On 05/05/2021 08:43, 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 as 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).
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

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

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 07/10] xen/arm: Always access SCTLR_EL2 using READ/WRITE_SYSREG()
  2021-05-05  7:43 ` [PATCH v3 07/10] xen/arm: Always access SCTLR_EL2 using READ/WRITE_SYSREG() Michal Orzel
@ 2021-05-05 18:07   ` Julien Grall
  0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2021-05-05 18:07 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis, wei.chen

Hi Michal,

On 05/05/2021 08:43, Michal Orzel wrote:
> 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.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

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

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 02/10] arm/domain: Get rid of READ/WRITE_SYSREG32
  2021-05-05 18:03   ` Julien Grall
@ 2021-05-06  6:13     ` Michal Orzel
  2021-05-10 17:02       ` Julien Grall
  0 siblings, 1 reply; 31+ messages in thread
From: Michal Orzel @ 2021-05-06  6:13 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis, wei.chen

Hi Julien,

On 05.05.2021 20:03, Julien Grall wrote:
> Hi Michal,
> 
> On 05/05/2021 08:43, 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.
>> 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.
> 
> Sorry for not replying on v2. How about:
> 
> "
> Thumbee registers are only usable by a 32-bit domain and therefore we can just store the bottom 32-bit (IOW there is no type change). In fact, this could technically be restricted to Armv7 HW (the support was dropped retrospectively in Armv8) but leave it as-is for now.
> "
> 
> If you are happy with it, I will do it on commit.
> 
I am happy with it. Please ack and change it on commit.
Thanks.
> Cheers,
> 

Cheers,
Michal


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 02/10] arm/domain: Get rid of READ/WRITE_SYSREG32
  2021-05-06  6:13     ` Michal Orzel
@ 2021-05-10 17:02       ` Julien Grall
  0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2021-05-10 17:02 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, bertrand.marquis, wei.chen



On 06/05/2021 07:13, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 05.05.2021 20:03, Julien Grall wrote:
>> Hi Michal,
>>
>> On 05/05/2021 08:43, 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.
>>> 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.
>>
>> Sorry for not replying on v2. How about:
>>
>> "
>> Thumbee registers are only usable by a 32-bit domain and therefore we can just store the bottom 32-bit (IOW there is no type change). In fact, this could technically be restricted to Armv7 HW (the support was dropped retrospectively in Armv8) but leave it as-is for now.
>> "
>>
>> If you are happy with it, I will do it on commit.
>>
> I am happy with it. Please ack and change it on commit.

Thanks.

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

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32
  2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
                   ` (9 preceding siblings ...)
  2021-05-05  7:43 ` [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t Michal Orzel
@ 2021-05-10 17:19 ` Julien Grall
  10 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2021-05-10 17:19 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Tamas K Lengyel, Alexandru Isaila, Petre Pircalabu,
	bertrand.marquis, wei.chen

Hi Michal,

On 05/05/2021 08:42, Michal Orzel wrote:
> 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
>    xen/arm: Always access SCTLR_EL2 using READ/WRITE_SYSREG()
>    arm/page: Get rid of READ/WRITE_SYSREG32
>    arm/time,vtimer: Get rid of READ/WRITE_SYSREG32

I have merged the first 9 patches. It looks like...

>    arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

... this one needs an answer from you at least.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-05  8:00   ` Jan Beulich
  2021-05-05 11:49     ` Tamas K Lengyel
@ 2021-05-11  6:37     ` Michal Orzel
  2021-05-12 17:59       ` Julien Grall
  1 sibling, 1 reply; 31+ messages in thread
From: Michal Orzel @ 2021-05-11  6:37 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
	Tamas K Lengyel, Alexandru Isaila, Petre Pircalabu,
	bertrand.marquis, wei.chen, xen-devel

Hi Jan,

On 05.05.2021 10:00, Jan Beulich wrote:
> On 05.05.2021 09:43, Michal Orzel wrote:
>> --- 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 */
>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>  
>>      union {
>> -        uint32_t spsr_el1;       /* AArch64 */
>> +        uint64_t spsr_el1;       /* AArch64 */
>>          uint32_t spsr_svc;       /* AArch32 */
>>      };
> 
> This change affects, besides domctl, also default_initialise_vcpu(),
> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
> only allows two unrelated VCPUOP_* to pass, but then I don't
> understand why arch_initialise_vcpu() doesn't simply return e.g.
> -EOPNOTSUPP. Hence I suspect I'm missing something.
> 
I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
arch_initialise_vcpu being called in case of VCPUOP_initialise
has no sense as VCPUOP_initialise is not supported on arm.
It makes sense that it should return -EOPNOTSUPP.
However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
-EINVAL. So arch_initialise_vcpu for arm will not be called.
Do you think that changing this behaviour so that arch_initialise_vcpu returns
-EOPNOTSUPP should be part of this patch?

>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -38,7 +38,7 @@
>>  #include "hvm/save.h"
>>  #include "memory.h"
>>  
>> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
>> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
> 
> So this is to cover for the struct vcpu_guest_core_regs change.
> 
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -266,8 +266,7 @@ struct vm_event_regs_arm {
>>      uint64_t ttbr1;
>>      uint64_t ttbcr;
>>      uint64_t pc;
>> -    uint32_t cpsr;
>> -    uint32_t _pad;
>> +    uint64_t cpsr;
>>  };
> 
> Then I wonder why this isn't accompanied by a similar bump of
> VM_EVENT_INTERFACE_VERSION. I don't see you drop any checking /
> filling of the _pad field, so existing callers may pass garbage
> there, and new callers need to be prevented from looking at the
> upper half when running on an older hypervisor.
> 
> Jan
> 
Cheers,
Michal


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-11  6:37     ` Michal Orzel
@ 2021-05-12 17:59       ` Julien Grall
  2021-05-12 18:14         ` Andrew Cooper
  2021-05-17  7:01         ` Jan Beulich
  0 siblings, 2 replies; 31+ messages in thread
From: Julien Grall @ 2021-05-12 17:59 UTC (permalink / raw)
  To: Michal Orzel, Jan Beulich
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Tamas K Lengyel,
	Alexandru Isaila, Petre Pircalabu, bertrand.marquis, wei.chen,
	xen-devel

Hi,

On 11/05/2021 07:37, Michal Orzel wrote:
> On 05.05.2021 10:00, Jan Beulich wrote:
>> On 05.05.2021 09:43, Michal Orzel wrote:
>>> --- 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 */
>>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>>   
>>>       union {
>>> -        uint32_t spsr_el1;       /* AArch64 */
>>> +        uint64_t spsr_el1;       /* AArch64 */
>>>           uint32_t spsr_svc;       /* AArch32 */
>>>       };
>>
>> This change affects, besides domctl, also default_initialise_vcpu(),
>> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
>> only allows two unrelated VCPUOP_* to pass, but then I don't
>> understand why arch_initialise_vcpu() doesn't simply return e.g.
>> -EOPNOTSUPP. Hence I suspect I'm missing something.

I think it is just an overlooked when reviewing the following commit:

commit 192df6f9122ddebc21d0a632c10da3453aeee1c2
Author: Roger Pau Monné <roger.pau@citrix.com>
Date:   Tue Dec 15 14:12:32 2015 +0100

     x86: allow HVM guests to use hypercalls to bring up vCPUs

     Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down,
     VCPUOP_is_up, VCPUOP_get_physid and VCPUOP_send_nmi hypercalls from HVM
     guests.

     This patch introduces a new structure (vcpu_hvm_context) that 
should be used
     in conjuction with the VCPUOP_initialise hypercall in order to 
initialize
     vCPUs for HVM guests.

     Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
     Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
     Reviewed-by: Jan Beulich <jbeulich@suse.com>
     Acked-by: Ian Campbell <ian.campbell@citrix.com>

On Arm, the structure vcpu_guest_context is not exposed outside of Xen 
and the tools. Interestingly vcpu_guest_core_regs is but it should only 
be used within vcpu_guest_context.

So as this is not used by stable ABI, it is fine to break it.

>>
> I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
> arch_initialise_vcpu being called in case of VCPUOP_initialise
> has no sense as VCPUOP_initialise is not supported on arm.
> It makes sense that it should return -EOPNOTSUPP.
> However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
> -EINVAL. So arch_initialise_vcpu for arm will not be called.
> Do you think that changing this behaviour so that arch_initialise_vcpu returns
> -EOPNOTSUPP should be part of this patch?

I think this change is unrelated. So it should be handled in a follow-up 
patch.

If you are taking care of this, would you mind to also look to move 
struct vcpu_guest_core_regs within the #if defined(__XEN__) || 
defined(__XEN_TOOLS__)?

I will attempt to do a proper review of this patch by the end of the week.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-12 17:59       ` Julien Grall
@ 2021-05-12 18:14         ` Andrew Cooper
  2021-05-17  7:01         ` Jan Beulich
  1 sibling, 0 replies; 31+ messages in thread
From: Andrew Cooper @ 2021-05-12 18:14 UTC (permalink / raw)
  To: Julien Grall, Michal Orzel, Jan Beulich
  Cc: Stefano Stabellini, Volodymyr Babchuk, George Dunlap,
	Ian Jackson, Wei Liu, Tamas K Lengyel, Alexandru Isaila,
	Petre Pircalabu, bertrand.marquis, wei.chen, xen-devel

On 12/05/2021 18:59, Julien Grall wrote:
> Hi,
>
> On 11/05/2021 07:37, Michal Orzel wrote:
>> On 05.05.2021 10:00, Jan Beulich wrote:
>>> On 05.05.2021 09:43, Michal Orzel wrote:
>>>> --- 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 */
>>>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>>>         union {
>>>> -        uint32_t spsr_el1;       /* AArch64 */
>>>> +        uint64_t spsr_el1;       /* AArch64 */
>>>>           uint32_t spsr_svc;       /* AArch32 */
>>>>       };
>>>
>>> This change affects, besides domctl, also default_initialise_vcpu(),
>>> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
>>> only allows two unrelated VCPUOP_* to pass, but then I don't
>>> understand why arch_initialise_vcpu() doesn't simply return e.g.
>>> -EOPNOTSUPP. Hence I suspect I'm missing something.
>
> I think it is just an overlooked when reviewing the following commit:
>
> commit 192df6f9122ddebc21d0a632c10da3453aeee1c2
> Author: Roger Pau Monné <roger.pau@citrix.com>
> Date:   Tue Dec 15 14:12:32 2015 +0100
>
>     x86: allow HVM guests to use hypercalls to bring up vCPUs
>
>     Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down,
>     VCPUOP_is_up, VCPUOP_get_physid and VCPUOP_send_nmi hypercalls
> from HVM
>     guests.
>
>     This patch introduces a new structure (vcpu_hvm_context) that
> should be used
>     in conjuction with the VCPUOP_initialise hypercall in order to
> initialize
>     vCPUs for HVM guests.
>
>     Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>     Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>     Reviewed-by: Jan Beulich <jbeulich@suse.com>
>     Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> On Arm, the structure vcpu_guest_context is not exposed outside of Xen
> and the tools. Interestingly vcpu_guest_core_regs is but it should
> only be used within vcpu_guest_context.
>
> So as this is not used by stable ABI, it is fine to break it.
>
>>>
>> I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
>> arch_initialise_vcpu being called in case of VCPUOP_initialise
>> has no sense as VCPUOP_initialise is not supported on arm.
>> It makes sense that it should return -EOPNOTSUPP.
>> However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
>> -EINVAL. So arch_initialise_vcpu for arm will not be called.
>> Do you think that changing this behaviour so that
>> arch_initialise_vcpu returns
>> -EOPNOTSUPP should be part of this patch?
>
> I think this change is unrelated. So it should be handled in a
> follow-up patch.
>
> If you are taking care of this, would you mind to also look to move
> struct vcpu_guest_core_regs within the #if defined(__XEN__) ||
> defined(__XEN_TOOLS__)?

+1.  Fairly sure this is the conclusion of a discussion a year or so
back where I noted the same peculiarity, and tried to untangle the mess
which is the common vs arch specific code.  (which is still outstanding,
and I don't immediately recall why.)

~Andrew


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-12 17:59       ` Julien Grall
  2021-05-12 18:14         ` Andrew Cooper
@ 2021-05-17  7:01         ` Jan Beulich
  2021-05-17 16:03           ` Julien Grall
  1 sibling, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2021-05-17  7:01 UTC (permalink / raw)
  To: Julien Grall, Michal Orzel
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Tamas K Lengyel,
	Alexandru Isaila, Petre Pircalabu, bertrand.marquis, wei.chen,
	xen-devel

On 12.05.2021 19:59, Julien Grall wrote:
> Hi,
> 
> On 11/05/2021 07:37, Michal Orzel wrote:
>> On 05.05.2021 10:00, Jan Beulich wrote:
>>> On 05.05.2021 09:43, Michal Orzel wrote:
>>>> --- 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 */
>>>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>>>   
>>>>       union {
>>>> -        uint32_t spsr_el1;       /* AArch64 */
>>>> +        uint64_t spsr_el1;       /* AArch64 */
>>>>           uint32_t spsr_svc;       /* AArch32 */
>>>>       };
>>>
>>> This change affects, besides domctl, also default_initialise_vcpu(),
>>> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
>>> only allows two unrelated VCPUOP_* to pass, but then I don't
>>> understand why arch_initialise_vcpu() doesn't simply return e.g.
>>> -EOPNOTSUPP. Hence I suspect I'm missing something.
> 
> I think it is just an overlooked when reviewing the following commit:
> 
> commit 192df6f9122ddebc21d0a632c10da3453aeee1c2
> Author: Roger Pau Monné <roger.pau@citrix.com>
> Date:   Tue Dec 15 14:12:32 2015 +0100
> 
>      x86: allow HVM guests to use hypercalls to bring up vCPUs
> 
>      Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down,
>      VCPUOP_is_up, VCPUOP_get_physid and VCPUOP_send_nmi hypercalls from HVM
>      guests.
> 
>      This patch introduces a new structure (vcpu_hvm_context) that 
> should be used
>      in conjuction with the VCPUOP_initialise hypercall in order to 
> initialize
>      vCPUs for HVM guests.
> 
>      Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>      Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>      Reviewed-by: Jan Beulich <jbeulich@suse.com>
>      Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> On Arm, the structure vcpu_guest_context is not exposed outside of Xen 
> and the tools. Interestingly vcpu_guest_core_regs is but it should only 
> be used within vcpu_guest_context.
> 
> So as this is not used by stable ABI, it is fine to break it.
> 
>>>
>> I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
>> arch_initialise_vcpu being called in case of VCPUOP_initialise
>> has no sense as VCPUOP_initialise is not supported on arm.
>> It makes sense that it should return -EOPNOTSUPP.
>> However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
>> -EINVAL. So arch_initialise_vcpu for arm will not be called.
>> Do you think that changing this behaviour so that arch_initialise_vcpu returns
>> -EOPNOTSUPP should be part of this patch?
> 
> I think this change is unrelated. So it should be handled in a follow-up 
> patch.

My only difference in viewing this is that I'd say the adjustment
would better be a prereq patch to this one, such that the one here
ends up being more obviously correct. Also, if the function is
indeed not meant to be reachable, besides making it return
-EOPNOTSUPP (or alike) it should probably also have
ASSERT_UNREACHABLE() added.

Jan

> If you are taking care of this, would you mind to also look to move 
> struct vcpu_guest_core_regs within the #if defined(__XEN__) || 
> defined(__XEN_TOOLS__)?
> 
> I will attempt to do a proper review of this patch by the end of the week.
> 
> Cheers,
> 



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-17  7:01         ` Jan Beulich
@ 2021-05-17 16:03           ` Julien Grall
  2021-05-21  6:33             ` Michal Orzel
  0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2021-05-17 16:03 UTC (permalink / raw)
  To: Jan Beulich, Michal Orzel
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Tamas K Lengyel,
	Alexandru Isaila, Petre Pircalabu, bertrand.marquis, wei.chen,
	xen-devel

Hi Jan,

On 17/05/2021 08:01, Jan Beulich wrote:
> On 12.05.2021 19:59, Julien Grall wrote:
>> Hi,
>>
>> On 11/05/2021 07:37, Michal Orzel wrote:
>>> On 05.05.2021 10:00, Jan Beulich wrote:
>>>> On 05.05.2021 09:43, Michal Orzel wrote:
>>>>> --- 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 */
>>>>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>>>>    
>>>>>        union {
>>>>> -        uint32_t spsr_el1;       /* AArch64 */
>>>>> +        uint64_t spsr_el1;       /* AArch64 */
>>>>>            uint32_t spsr_svc;       /* AArch32 */
>>>>>        };
>>>>
>>>> This change affects, besides domctl, also default_initialise_vcpu(),
>>>> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
>>>> only allows two unrelated VCPUOP_* to pass, but then I don't
>>>> understand why arch_initialise_vcpu() doesn't simply return e.g.
>>>> -EOPNOTSUPP. Hence I suspect I'm missing something.
>>
>> I think it is just an overlooked when reviewing the following commit:
>>
>> commit 192df6f9122ddebc21d0a632c10da3453aeee1c2
>> Author: Roger Pau Monné <roger.pau@citrix.com>
>> Date:   Tue Dec 15 14:12:32 2015 +0100
>>
>>       x86: allow HVM guests to use hypercalls to bring up vCPUs
>>
>>       Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down,
>>       VCPUOP_is_up, VCPUOP_get_physid and VCPUOP_send_nmi hypercalls from HVM
>>       guests.
>>
>>       This patch introduces a new structure (vcpu_hvm_context) that
>> should be used
>>       in conjuction with the VCPUOP_initialise hypercall in order to
>> initialize
>>       vCPUs for HVM guests.
>>
>>       Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>       Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>       Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>       Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>
>> On Arm, the structure vcpu_guest_context is not exposed outside of Xen
>> and the tools. Interestingly vcpu_guest_core_regs is but it should only
>> be used within vcpu_guest_context.
>>
>> So as this is not used by stable ABI, it is fine to break it.
>>
>>>>
>>> I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
>>> arch_initialise_vcpu being called in case of VCPUOP_initialise
>>> has no sense as VCPUOP_initialise is not supported on arm.
>>> It makes sense that it should return -EOPNOTSUPP.
>>> However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
>>> -EINVAL. So arch_initialise_vcpu for arm will not be called.
>>> Do you think that changing this behaviour so that arch_initialise_vcpu returns
>>> -EOPNOTSUPP should be part of this patch?
>>
>> I think this change is unrelated. So it should be handled in a follow-up
>> patch.
> 
> My only difference in viewing this is that I'd say the adjustment
> would better be a prereq patch to this one, such that the one here
> ends up being more obviously correct.

The function is already not reachable so I felt it was unfair to require 
the clean-up for merging this code.

> Also, if the function is
> indeed not meant to be reachable, besides making it return
> -EOPNOTSUPP (or alike) it should probably also have
> ASSERT_UNREACHABLE() added.

+1 on the idea.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-17 16:03           ` Julien Grall
@ 2021-05-21  6:33             ` Michal Orzel
  2021-05-21  7:07               ` Jan Beulich
  0 siblings, 1 reply; 31+ messages in thread
From: Michal Orzel @ 2021-05-21  6:33 UTC (permalink / raw)
  To: Julien Grall, Jan Beulich
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Tamas K Lengyel,
	Alexandru Isaila, Petre Pircalabu, bertrand.marquis, wei.chen,
	xen-devel

Hi guys,

On 17.05.2021 18:03, Julien Grall wrote:
> Hi Jan,
> 
> On 17/05/2021 08:01, Jan Beulich wrote:
>> On 12.05.2021 19:59, Julien Grall wrote:
>>> Hi,
>>>
>>> On 11/05/2021 07:37, Michal Orzel wrote:
>>>> On 05.05.2021 10:00, Jan Beulich wrote:
>>>>> On 05.05.2021 09:43, Michal Orzel wrote:
>>>>>> --- 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 */
>>>>>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>>>>>           union {
>>>>>> -        uint32_t spsr_el1;       /* AArch64 */
>>>>>> +        uint64_t spsr_el1;       /* AArch64 */
>>>>>>            uint32_t spsr_svc;       /* AArch32 */
>>>>>>        };
>>>>>
>>>>> This change affects, besides domctl, also default_initialise_vcpu(),
>>>>> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
>>>>> only allows two unrelated VCPUOP_* to pass, but then I don't
>>>>> understand why arch_initialise_vcpu() doesn't simply return e.g.
>>>>> -EOPNOTSUPP. Hence I suspect I'm missing something.
>>>
>>> I think it is just an overlooked when reviewing the following commit:
>>>
>>> commit 192df6f9122ddebc21d0a632c10da3453aeee1c2
>>> Author: Roger Pau Monné <roger.pau@citrix.com>
>>> Date:   Tue Dec 15 14:12:32 2015 +0100
>>>
>>>       x86: allow HVM guests to use hypercalls to bring up vCPUs
>>>
>>>       Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down,
>>>       VCPUOP_is_up, VCPUOP_get_physid and VCPUOP_send_nmi hypercalls from HVM
>>>       guests.
>>>
>>>       This patch introduces a new structure (vcpu_hvm_context) that
>>> should be used
>>>       in conjuction with the VCPUOP_initialise hypercall in order to
>>> initialize
>>>       vCPUs for HVM guests.
>>>
>>>       Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>       Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>       Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>       Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> On Arm, the structure vcpu_guest_context is not exposed outside of Xen
>>> and the tools. Interestingly vcpu_guest_core_regs is but it should only
>>> be used within vcpu_guest_context.
>>>
>>> So as this is not used by stable ABI, it is fine to break it.
>>>
>>>>>
>>>> I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
>>>> arch_initialise_vcpu being called in case of VCPUOP_initialise
>>>> has no sense as VCPUOP_initialise is not supported on arm.
>>>> It makes sense that it should return -EOPNOTSUPP.
>>>> However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
>>>> -EINVAL. So arch_initialise_vcpu for arm will not be called.
>>>> Do you think that changing this behaviour so that arch_initialise_vcpu returns
>>>> -EOPNOTSUPP should be part of this patch?
>>>
>>> I think this change is unrelated. So it should be handled in a follow-up
>>> patch.
>>
>> My only difference in viewing this is that I'd say the adjustment
>> would better be a prereq patch to this one, such that the one here
>> ends up being more obviously correct.
> 
> The function is already not reachable so I felt it was unfair to require the clean-up for merging this code.
> 
>> Also, if the function is
>> indeed not meant to be reachable, besides making it return
>> -EOPNOTSUPP (or alike) it should probably also have
>> ASSERT_UNREACHABLE() added.
> 
> +1 on the idea.
> 
> Cheers,
> 
FWICS, all the discussion is about creating the next patch fixing the VCPUOP_initialise function.
Is there anything left to do in this patch or are you going to ack it?

Cheers,
Michal


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-21  6:33             ` Michal Orzel
@ 2021-05-21  7:07               ` Jan Beulich
  2021-06-07 13:16                 ` Michal Orzel
  0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2021-05-21  7:07 UTC (permalink / raw)
  To: Michal Orzel
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Tamas K Lengyel,
	Alexandru Isaila, Petre Pircalabu, bertrand.marquis, wei.chen,
	xen-devel, Julien Grall

On 21.05.2021 08:33, Michal Orzel wrote:
> On 17.05.2021 18:03, Julien Grall wrote:
>> On 17/05/2021 08:01, Jan Beulich wrote:
>>> On 12.05.2021 19:59, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 11/05/2021 07:37, Michal Orzel wrote:
>>>>> On 05.05.2021 10:00, Jan Beulich wrote:
>>>>>> On 05.05.2021 09:43, Michal Orzel wrote:
>>>>>>> --- 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 */
>>>>>>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>>>>>>           union {
>>>>>>> -        uint32_t spsr_el1;       /* AArch64 */
>>>>>>> +        uint64_t spsr_el1;       /* AArch64 */
>>>>>>>            uint32_t spsr_svc;       /* AArch32 */
>>>>>>>        };
>>>>>>
>>>>>> This change affects, besides domctl, also default_initialise_vcpu(),
>>>>>> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
>>>>>> only allows two unrelated VCPUOP_* to pass, but then I don't
>>>>>> understand why arch_initialise_vcpu() doesn't simply return e.g.
>>>>>> -EOPNOTSUPP. Hence I suspect I'm missing something.
>>>>
>>>> I think it is just an overlooked when reviewing the following commit:
>>>>
>>>> commit 192df6f9122ddebc21d0a632c10da3453aeee1c2
>>>> Author: Roger Pau Monné <roger.pau@citrix.com>
>>>> Date:   Tue Dec 15 14:12:32 2015 +0100
>>>>
>>>>       x86: allow HVM guests to use hypercalls to bring up vCPUs
>>>>
>>>>       Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down,
>>>>       VCPUOP_is_up, VCPUOP_get_physid and VCPUOP_send_nmi hypercalls from HVM
>>>>       guests.
>>>>
>>>>       This patch introduces a new structure (vcpu_hvm_context) that
>>>> should be used
>>>>       in conjuction with the VCPUOP_initialise hypercall in order to
>>>> initialize
>>>>       vCPUs for HVM guests.
>>>>
>>>>       Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>       Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>       Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>       Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>>
>>>> On Arm, the structure vcpu_guest_context is not exposed outside of Xen
>>>> and the tools. Interestingly vcpu_guest_core_regs is but it should only
>>>> be used within vcpu_guest_context.
>>>>
>>>> So as this is not used by stable ABI, it is fine to break it.
>>>>
>>>>>>
>>>>> I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
>>>>> arch_initialise_vcpu being called in case of VCPUOP_initialise
>>>>> has no sense as VCPUOP_initialise is not supported on arm.
>>>>> It makes sense that it should return -EOPNOTSUPP.
>>>>> However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
>>>>> -EINVAL. So arch_initialise_vcpu for arm will not be called.
>>>>> Do you think that changing this behaviour so that arch_initialise_vcpu returns
>>>>> -EOPNOTSUPP should be part of this patch?
>>>>
>>>> I think this change is unrelated. So it should be handled in a follow-up
>>>> patch.
>>>
>>> My only difference in viewing this is that I'd say the adjustment
>>> would better be a prereq patch to this one, such that the one here
>>> ends up being more obviously correct.
>>
>> The function is already not reachable so I felt it was unfair to require the clean-up for merging this code.
>>
>>> Also, if the function is
>>> indeed not meant to be reachable, besides making it return
>>> -EOPNOTSUPP (or alike) it should probably also have
>>> ASSERT_UNREACHABLE() added.
>>
>> +1 on the idea.
>>
>> Cheers,
>>
> FWICS, all the discussion is about creating the next patch fixing the VCPUOP_initialise function.
> Is there anything left to do in this patch or are you going to ack it?

Afaic I'd find it quite helpful if that other patch was a prereq to this
one, making more obvious that the change here is not going to break
anything. But it's Arm stuff, so Arm folks get the final say anyway.

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-21  7:07               ` Jan Beulich
@ 2021-06-07 13:16                 ` Michal Orzel
  0 siblings, 0 replies; 31+ messages in thread
From: Michal Orzel @ 2021-06-07 13:16 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Tamas K Lengyel,
	Alexandru Isaila, Petre Pircalabu, bertrand.marquis, wei.chen,
	xen-devel, Julien Grall



On 21.05.2021 09:07, Jan Beulich wrote:
> On 21.05.2021 08:33, Michal Orzel wrote:
>> On 17.05.2021 18:03, Julien Grall wrote:
>>> On 17/05/2021 08:01, Jan Beulich wrote:
>>>> On 12.05.2021 19:59, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 11/05/2021 07:37, Michal Orzel wrote:
>>>>>> On 05.05.2021 10:00, Jan Beulich wrote:
>>>>>>> On 05.05.2021 09:43, Michal Orzel wrote:
>>>>>>>> --- 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 */
>>>>>>>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>>>>>>>           union {
>>>>>>>> -        uint32_t spsr_el1;       /* AArch64 */
>>>>>>>> +        uint64_t spsr_el1;       /* AArch64 */
>>>>>>>>            uint32_t spsr_svc;       /* AArch32 */
>>>>>>>>        };
>>>>>>>
>>>>>>> This change affects, besides domctl, also default_initialise_vcpu(),
>>>>>>> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
>>>>>>> only allows two unrelated VCPUOP_* to pass, but then I don't
>>>>>>> understand why arch_initialise_vcpu() doesn't simply return e.g.
>>>>>>> -EOPNOTSUPP. Hence I suspect I'm missing something.
>>>>>
>>>>> I think it is just an overlooked when reviewing the following commit:
>>>>>
>>>>> commit 192df6f9122ddebc21d0a632c10da3453aeee1c2
>>>>> Author: Roger Pau Monné <roger.pau@citrix.com>
>>>>> Date:   Tue Dec 15 14:12:32 2015 +0100
>>>>>
>>>>>       x86: allow HVM guests to use hypercalls to bring up vCPUs
>>>>>
>>>>>       Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down,
>>>>>       VCPUOP_is_up, VCPUOP_get_physid and VCPUOP_send_nmi hypercalls from HVM
>>>>>       guests.
>>>>>
>>>>>       This patch introduces a new structure (vcpu_hvm_context) that
>>>>> should be used
>>>>>       in conjuction with the VCPUOP_initialise hypercall in order to
>>>>> initialize
>>>>>       vCPUs for HVM guests.
>>>>>
>>>>>       Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>       Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>       Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>>       Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>>>
>>>>> On Arm, the structure vcpu_guest_context is not exposed outside of Xen
>>>>> and the tools. Interestingly vcpu_guest_core_regs is but it should only
>>>>> be used within vcpu_guest_context.
>>>>>
>>>>> So as this is not used by stable ABI, it is fine to break it.
>>>>>
>>>>>>>
>>>>>> I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
>>>>>> arch_initialise_vcpu being called in case of VCPUOP_initialise
>>>>>> has no sense as VCPUOP_initialise is not supported on arm.
>>>>>> It makes sense that it should return -EOPNOTSUPP.
>>>>>> However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
>>>>>> -EINVAL. So arch_initialise_vcpu for arm will not be called.
>>>>>> Do you think that changing this behaviour so that arch_initialise_vcpu returns
>>>>>> -EOPNOTSUPP should be part of this patch?
>>>>>
>>>>> I think this change is unrelated. So it should be handled in a follow-up
>>>>> patch.
>>>>
>>>> My only difference in viewing this is that I'd say the adjustment
>>>> would better be a prereq patch to this one, such that the one here
>>>> ends up being more obviously correct.
>>>
>>> The function is already not reachable so I felt it was unfair to require the clean-up for merging this code.
>>>
>>>> Also, if the function is
>>>> indeed not meant to be reachable, besides making it return
>>>> -EOPNOTSUPP (or alike) it should probably also have
>>>> ASSERT_UNREACHABLE() added.
>>>
>>> +1 on the idea.
>>>
>>> Cheers,
>>>
>> FWICS, all the discussion is about creating the next patch fixing the VCPUOP_initialise function.
>> Is there anything left to do in this patch or are you going to ack it?
> 
> Afaic I'd find it quite helpful if that other patch was a prereq to this
> one, making more obvious that the change here is not going to break
> anything. But it's Arm stuff, so Arm folks get the final say anyway.
This change is not going to break anything as the new patch is going to mainly add ASSERT_UNREACHABLE into VCPUOP_initialise which means it'll be a clean-up patch.
Also the problem was not introduced by this patch so I think it should be merged.
> 
> Jan
> 

So what is the final say from Arm folks :) ?



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-05-05  7:43 ` [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t Michal Orzel
  2021-05-05  8:00   ` Jan Beulich
@ 2021-06-07 13:31   ` Julien Grall
  2021-07-01  8:19     ` Michal Orzel
  1 sibling, 1 reply; 31+ messages in thread
From: Julien Grall @ 2021-06-07 13:31 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Tamas K Lengyel, Alexandru Isaila, Petre Pircalabu,
	bertrand.marquis, wei.chen

Hi,

On 05/05/2021 08:43, 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.
> 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

s/becuase/because/

> registers as there was a 32bit padding after SPSR_EL1.
> 
> 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.
> 
> 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.
> 
> Change type of cpsr to uint64_t in the public outside interface
> "public/vm_event.h" to allow ABI compatibility between 32bit and 64bit.
[...]

> 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));
Why do you now need to duplicate this line? Can't we use PRIregister you 
did everywhere else a register is printed?

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

[...]

> 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 */

I am guessing you are saying it should be 8-byte aligned, right? If so, 
the field before is a 64-bit value, therefore the offset should already 
be a multiple of 8. Did I miss anything?

> +    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..9b91b28c48 100644
> --- a/xen/include/asm-arm/hsr.h
> +++ b/xen/include/asm-arm/hsr.h
> @@ -16,7 +16,7 @@ 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 */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 713fd65317..64a2ca30da 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 */
> +    uint64_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/domctl.h b/xen/include/public/domctl.h
> index 4dbf107785..d576bfabd6 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>   #include "hvm/save.h"
>   #include "memory.h"
>   
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
>   
>   /*
>    * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 36135ba4f1..bb003d21d0 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -266,8 +266,7 @@ struct vm_event_regs_arm {
>       uint64_t ttbr1;
>       uint64_t ttbcr;
>       uint64_t pc;
> -    uint32_t cpsr;
> -    uint32_t _pad;
> +    uint64_t cpsr;
>   };
>   
>   /*
> 

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-06-07 13:31   ` Julien Grall
@ 2021-07-01  8:19     ` Michal Orzel
  2021-07-03 14:42       ` Julien Grall
  0 siblings, 1 reply; 31+ messages in thread
From: Michal Orzel @ 2021-07-01  8:19 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Tamas K Lengyel, Alexandru Isaila, Petre Pircalabu,
	bertrand.marquis, wei.chen

Hi Julien,

On 07.06.2021 15:31, Julien Grall wrote:
> Hi,
> 
> On 05/05/2021 08:43, 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.
>> 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
> 
> s/becuase/because/
> 
Will do.
>> registers as there was a 32bit padding after SPSR_EL1.
>>
>> 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.
>>
>> 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.
>>
>> Change type of cpsr to uint64_t in the public outside interface
>> "public/vm_event.h" to allow ABI compatibility between 32bit and 64bit.
> [...]
> 
>> 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));
> Why do you now need to duplicate this line? Can't we use PRIregister you did everywhere else a register is printed?
> 
Indeed we do not need. It was needed at the beginning but I do not remember why. I will fix it.
>>   #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
> 
> [...]
> 
>> 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 */
> 
> I am guessing you are saying it should be 8-byte aligned, right? If so, the field before is a 64-bit value, therefore the offset should already be a multiple of 8. Did I miss anything?
> 
Well, instead of 8 I should write 16.
We have 2 bug_on checks:
1) BUILD_BUG_ON((sizeof (struct cpu_user_regs)) & 0xf);
2) BUILD_BUG_ON((offsetof(struct cpu_user_regs, spsr_el1)) & 0xf);

Total size of cpu_user_regs without padding is 344 which does not pass the first check.
With the padding it is 352 and it passes the first check.

The offset without padding is 280 which does not pass the second check.
The offset with padding is 288 which passes the second check.

So I should write:
"/* Offset of spsr_el1 must be a multiple of 16 bytes */"

Is it ok for you?
If yes, I will push v3.
>> +    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..9b91b28c48 100644
>> --- a/xen/include/asm-arm/hsr.h
>> +++ b/xen/include/asm-arm/hsr.h
>> @@ -16,7 +16,7 @@ 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 */
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 713fd65317..64a2ca30da 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 */
>> +    uint64_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/domctl.h b/xen/include/public/domctl.h
>> index 4dbf107785..d576bfabd6 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -38,7 +38,7 @@
>>   #include "hvm/save.h"
>>   #include "memory.h"
>>   -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
>> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
>>     /*
>>    * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
>> index 36135ba4f1..bb003d21d0 100644
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -266,8 +266,7 @@ struct vm_event_regs_arm {
>>       uint64_t ttbr1;
>>       uint64_t ttbcr;
>>       uint64_t pc;
>> -    uint32_t cpsr;
>> -    uint32_t _pad;
>> +    uint64_t cpsr;
>>   };
>>     /*
>>
> 

Cheers,
Michal


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t
  2021-07-01  8:19     ` Michal Orzel
@ 2021-07-03 14:42       ` Julien Grall
  0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2021-07-03 14:42 UTC (permalink / raw)
  To: Michal Orzel, xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Tamas K Lengyel, Alexandru Isaila, Petre Pircalabu,
	bertrand.marquis, wei.chen



On 01/07/2021 09:19, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 07.06.2021 15:31, Julien Grall wrote:
>> Hi,
>>
>> On 05/05/2021 08:43, 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.
>>> 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
>>
>> s/becuase/because/
>>
> Will do.
>>> registers as there was a 32bit padding after SPSR_EL1.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> Change type of cpsr to uint64_t in the public outside interface
>>> "public/vm_event.h" to allow ABI compatibility between 32bit and 64bit.
>> [...]
>>
>>> 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));
>> Why do you now need to duplicate this line? Can't we use PRIregister you did everywhere else a register is printed?
>>
> Indeed we do not need. It was needed at the beginning but I do not remember why. I will fix it.
>>>    #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
>>
>> [...]
>>
>>> 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 */
>>
>> I am guessing you are saying it should be 8-byte aligned, right? If so, the field before is a 64-bit value, therefore the offset should already be a multiple of 8. Did I miss anything?
>>
> Well, instead of 8 I should write 16.
> We have 2 bug_on checks:
> 1) BUILD_BUG_ON((sizeof (struct cpu_user_regs)) & 0xf);
> 2) BUILD_BUG_ON((offsetof(struct cpu_user_regs, spsr_el1)) & 0xf);
> 
> Total size of cpu_user_regs without padding is 344 which does not pass the first check.
> With the padding it is 352 and it passes the first check.
> 
> The offset without padding is 280 which does not pass the second check.
> The offset with padding is 288 which passes the second check.

Ah yes, I forgot that the stack should always be 16-byte aligned.

> 
> So I should write:
> "/* Offset of spsr_el1 must be a multiple of 16 bytes */"
I would suggest the following comment:

/* The kernel frame should be 16-byte aligned. */

So this would make clear that the 16-byte requirement is for the frame 
itself.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2021-07-03 14:42 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05  7:42 [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Michal Orzel
2021-05-05  7:42 ` [PATCH v3 01/10] arm64/vfp: " Michal Orzel
2021-05-05  7:43 ` [PATCH v3 02/10] arm/domain: " Michal Orzel
2021-05-05 18:03   ` Julien Grall
2021-05-06  6:13     ` Michal Orzel
2021-05-10 17:02       ` Julien Grall
2021-05-05  7:43 ` [PATCH v3 03/10] arm: Modify type of actlr to register_t Michal Orzel
2021-05-05 18:04   ` Julien Grall
2021-05-05  7:43 ` [PATCH v3 04/10] arm/gic: Remove member hcr of structure gic_v3 Michal Orzel
2021-05-05  7:43 ` [PATCH v3 05/10] arm/gic: Get rid of READ/WRITE_SYSREG32 Michal Orzel
2021-05-05 18:06   ` Julien Grall
2021-05-05  7:43 ` [PATCH v3 06/10] arm/p2m: " Michal Orzel
2021-05-05  7:43 ` [PATCH v3 07/10] xen/arm: Always access SCTLR_EL2 using READ/WRITE_SYSREG() Michal Orzel
2021-05-05 18:07   ` Julien Grall
2021-05-05  7:43 ` [PATCH v3 08/10] arm/page: Get rid of READ/WRITE_SYSREG32 Michal Orzel
2021-05-05  7:43 ` [PATCH v3 09/10] arm/time,vtimer: " Michal Orzel
2021-05-05  7:43 ` [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t Michal Orzel
2021-05-05  8:00   ` Jan Beulich
2021-05-05 11:49     ` Tamas K Lengyel
2021-05-11  6:37     ` Michal Orzel
2021-05-12 17:59       ` Julien Grall
2021-05-12 18:14         ` Andrew Cooper
2021-05-17  7:01         ` Jan Beulich
2021-05-17 16:03           ` Julien Grall
2021-05-21  6:33             ` Michal Orzel
2021-05-21  7:07               ` Jan Beulich
2021-06-07 13:16                 ` Michal Orzel
2021-06-07 13:31   ` Julien Grall
2021-07-01  8:19     ` Michal Orzel
2021-07-03 14:42       ` Julien Grall
2021-05-10 17:19 ` [PATCH v3 00/10] arm64: Get rid of READ/WRITE_SYSREG32 Julien Grall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.