* [XEN v3 00/12] Arm: Enable GICv3 for AArch32
@ 2022-11-11 14:17 Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only Ayan Kumar Halder
` (11 more replies)
0 siblings, 12 replies; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
Hi All,
Please find the following patches to enable GICv3 for AArch32.
This is a pre-requisite to support Xen on Cortex-R52 (AArch32-v8R system)
Let me know your thoughts.
The following patches have been reviewed and acked and contain no changes
from v2 :-
1. [XEN v3 05/12] xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host
2. [XEN v3 06/12] xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32
Changes from -
v1 :-
1. Updated in the changelog for each of the patches.
v2 :-
1. Dropped "xen/Arm: GICv3: Move the macros to compute the affnity level to
arm64/arm32". The reason being aff3 does not exist on arm32. And aff0..2 is
the same between arm32, AArch32 and AArch64.
2. Introduce a new patch "xen/Arm: GICv3: Adapt access to VMPIDR register for
AArch32".
3. For the new registers introduced, we have defined the arm32 name and then
an alias.
4. Use 'AArch32' across all the patches.
5. Dropped the 'R-b' and 'Ack' in "[XEN v3 08/12] xen/Arm: GICv3: Define
ICH_AP0R<n> and ICH_AP1R<n> for AArch32".
Ayan Kumar Halder (12):
xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only
xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32
xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32
xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit
host
xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32
xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32
xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32
xen/Arm: GICv3: Define remaining GIC registers for AArch32
xen/Arm: GICv3: Use ULL instead of UL for 64bits
xen/Arm: GICv3: Define macros to read/write 64 bit
xen/Arm: GICv3: Enable GICv3 for AArch32
SUPPORT.md | 7 +
xen/arch/arm/Kconfig | 5 +-
xen/arch/arm/gic-v3.c | 159 +++++++++++++----------
xen/arch/arm/include/asm/arm32/io.h | 9 ++
xen/arch/arm/include/asm/arm32/sysregs.h | 19 +++
xen/arch/arm/include/asm/arm64/sysregs.h | 4 +
xen/arch/arm/include/asm/cpregs.h | 135 +++++++++++++++++++
xen/arch/arm/include/asm/cpufeature.h | 1 +
xen/arch/arm/include/asm/gic_v3_defs.h | 24 ++--
xen/arch/arm/include/asm/vreg.h | 86 +++---------
xen/arch/arm/vgic-v3.c | 18 ++-
11 files changed, 312 insertions(+), 155 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 36+ messages in thread
* [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-17 13:05 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32 Ayan Kumar Halder
` (10 subsequent siblings)
11 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
Sysreg emulation is 64-bit specific, so guard the calls to
vgic_v3_emulate_sysreg() as well as the function itself with
"#ifdef CONFIG_ARM_64".
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from -
v1 - 1. Updated the commit message.
v2 - 1. Updated the commit message (removed the reference to Arm ARM as it is
not required).
xen/arch/arm/vgic-v3.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 015446be17..3f4509dcd3 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1519,6 +1519,7 @@ static bool vgic_v3_emulate_sgi1r(struct cpu_user_regs *regs, uint64_t *r,
}
}
+#ifdef CONFIG_ARM_64
static bool vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
{
struct hsr_sysreg sysreg = hsr.sysreg;
@@ -1539,6 +1540,7 @@ static bool vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
return false;
}
}
+#endif
static bool vgic_v3_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
{
@@ -1562,8 +1564,10 @@ static bool vgic_v3_emulate_reg(struct cpu_user_regs *regs, union hsr hsr)
{
switch (hsr.ec)
{
+#ifdef CONFIG_ARM_64
case HSR_EC_SYSREG:
return vgic_v3_emulate_sysreg(regs, hsr);
+#endif
case HSR_EC_CP15_64:
return vgic_v3_emulate_cp64(regs, hsr);
default:
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-17 13:39 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32 Ayan Kumar Halder
` (9 subsequent siblings)
11 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
Affinity level 3 is not present in AArch32.
Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
Affinity level 3 is not present in Armv7 (ie arm32).
Thus, any access to affinity level 3 needs to be guarded within
"ifdef CONFIG_ARM_64".
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from -
v1 - NA (as it is a new patch)
v2 - NA (as it is a new patch)
xen/arch/arm/gic-v3.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 018fa0dfa0..64a76307dd 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -527,7 +527,10 @@ static void gicv3_set_pending_state(struct irq_desc *irqd, bool pending)
static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
{
uint64_t mpidr = cpu_logical_map(cpu);
- return (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
+ return (
+#ifdef CONFIG_ARM_64
+ MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
+#endif
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
MPIDR_AFFINITY_LEVEL(mpidr, 0));
@@ -720,7 +723,10 @@ static int __init gicv3_populate_rdist(void)
* Convert affinity to a 32bit value that can be matched to GICR_TYPER
* bits [63:32]
*/
- aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
+ aff = (
+#ifdef CONFIG_ARM_64
+ MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
+#endif
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
MPIDR_AFFINITY_LEVEL(mpidr, 0));
@@ -972,7 +978,10 @@ static void gicv3_send_sgi_list(enum gic_sgi sgi, const cpumask_t *cpumask)
* Prepare affinity path of the cluster for which SGI is generated
* along with SGI number
*/
- val = (MPIDR_AFFINITY_LEVEL(cluster_id, 3) << 48 |
+ val = (
+#ifdef CONFIG_ARM_64
+ MPIDR_AFFINITY_LEVEL(cluster_id, 3) << 48 |
+#endif
MPIDR_AFFINITY_LEVEL(cluster_id, 2) << 32 |
sgi << 24 |
MPIDR_AFFINITY_LEVEL(cluster_id, 1) << 16 |
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32 Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-17 13:11 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32 Ayan Kumar Halder
` (8 subsequent siblings)
11 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
In some situations (e.g. GICR_TYPER), the hypervior may need to emulate
64bit registers in AArch32 mode. In such situations, the hypervisor may
need to read/modify the lower or upper 32 bits of the 64 bit register.
In AArch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit
registers.
While we could replace 'unsigned long' by 'uint64_t', it is not entirely clear
whether a 32-bit compiler would not allocate register for the upper 32-bit.
Therefore fold vreg_reg_* helper in the size specific one and use the
appropriate type based on the size requested.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from -
v1 - 1. Remove vreg_reg_extract(), vreg_reg_update(), vreg_reg_setbits() and
vreg_reg_clearbits(). Moved the implementation to vreg_reg##sz##_*.
'mask' and 'val' is now using uint##sz##_t.
v2 - 1. Use 'unsigned int' for 'shift' variable.
2. Updated the commit message.
xen/arch/arm/include/asm/vreg.h | 86 ++++++++-------------------------
1 file changed, 19 insertions(+), 67 deletions(-)
diff --git a/xen/arch/arm/include/asm/vreg.h b/xen/arch/arm/include/asm/vreg.h
index f26a70d024..d92450017b 100644
--- a/xen/arch/arm/include/asm/vreg.h
+++ b/xen/arch/arm/include/asm/vreg.h
@@ -89,106 +89,58 @@ static inline bool vreg_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr
* The check on the size supported by the register has to be done by
* the caller of vreg_regN_*.
*
- * vreg_reg_* should never be called directly. Instead use the vreg_regN_*
- * according to size of the emulated register
- *
* Note that the alignment fault will always be taken in the guest
* (see B3.12.7 DDI0406.b).
*/
-static inline register_t vreg_reg_extract(unsigned long reg,
- unsigned int offset,
- enum dabt_size size)
-{
- reg >>= 8 * offset;
- reg &= VREG_REG_MASK(size);
-
- return reg;
-}
-
-static inline void vreg_reg_update(unsigned long *reg, register_t val,
- unsigned int offset,
- enum dabt_size size)
-{
- unsigned long mask = VREG_REG_MASK(size);
- int shift = offset * 8;
-
- *reg &= ~(mask << shift);
- *reg |= ((unsigned long)val & mask) << shift;
-}
-
-static inline void vreg_reg_setbits(unsigned long *reg, register_t bits,
- unsigned int offset,
- enum dabt_size size)
-{
- unsigned long mask = VREG_REG_MASK(size);
- int shift = offset * 8;
-
- *reg |= ((unsigned long)bits & mask) << shift;
-}
-
-static inline void vreg_reg_clearbits(unsigned long *reg, register_t bits,
- unsigned int offset,
- enum dabt_size size)
-{
- unsigned long mask = VREG_REG_MASK(size);
- int shift = offset * 8;
-
- *reg &= ~(((unsigned long)bits & mask) << shift);
-}
/* N-bit register helpers */
#define VREG_REG_HELPERS(sz, offmask) \
static inline register_t vreg_reg##sz##_extract(uint##sz##_t reg, \
const mmio_info_t *info)\
{ \
- return vreg_reg_extract(reg, info->gpa & (offmask), \
- info->dabt.size); \
+ unsigned int offset = info->gpa & (offmask); \
+ \
+ reg >>= 8 * offset; \
+ reg &= VREG_REG_MASK(info->dabt.size); \
+ \
+ return reg; \
} \
\
static inline void vreg_reg##sz##_update(uint##sz##_t *reg, \
register_t val, \
const mmio_info_t *info) \
{ \
- unsigned long tmp = *reg; \
+ unsigned int offset = info->gpa & (offmask); \
+ uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \
+ unsigned int shift = offset * 8; \
\
- vreg_reg_update(&tmp, val, info->gpa & (offmask), \
- info->dabt.size); \
- \
- *reg = tmp; \
+ *reg &= ~(mask << shift); \
+ *reg |= ((uint##sz##_t)val & mask) << shift; \
} \
\
static inline void vreg_reg##sz##_setbits(uint##sz##_t *reg, \
register_t bits, \
const mmio_info_t *info) \
{ \
- unsigned long tmp = *reg; \
- \
- vreg_reg_setbits(&tmp, bits, info->gpa & (offmask), \
- info->dabt.size); \
+ unsigned int offset = info->gpa & (offmask); \
+ uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \
+ unsigned int shift = offset * 8; \
\
- *reg = tmp; \
+ *reg |= ((uint##sz##_t)bits & mask) << shift; \
} \
\
static inline void vreg_reg##sz##_clearbits(uint##sz##_t *reg, \
register_t bits, \
const mmio_info_t *info) \
{ \
- unsigned long tmp = *reg; \
- \
- vreg_reg_clearbits(&tmp, bits, info->gpa & (offmask), \
- info->dabt.size); \
+ unsigned int offset = info->gpa & (offmask); \
+ uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \
+ unsigned int shift = offset * 8; \
\
- *reg = tmp; \
+ *reg &= ~(((uint##sz##_t)bits & mask) << shift); \
}
-/*
- * 64 bits registers are only supported on platform with 64-bit long.
- * This is also allow us to optimize the 32 bit case by using
- * unsigned long rather than uint64_t
- */
-#if BITS_PER_LONG == 64
VREG_REG_HELPERS(64, 0x7);
-#endif
VREG_REG_HELPERS(32, 0x3);
#undef VREG_REG_HELPERS
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (2 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32 Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-17 13:45 ` Michal Orzel
2022-11-22 20:37 ` Julien Grall
2022-11-11 14:17 ` [XEN v3 05/12] xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host Ayan Kumar Halder
` (7 subsequent siblings)
11 siblings, 2 replies; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
Aff3 does not exist on AArch32.
Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
Aff3 does not exist on Armv7 (ie arm32).
Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have copied it to
'uint64_t vmpidr' to perform the shifts.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-
v1 - Assigned v->arch.vmpidr to "uint64_t vmpdir". Then, we can use
MPIDR_AFFINITY_LEVEL macros to extract the affinity value.
v2 - 1. "MPIDR_AFFINITY_LEVEL(vmpidr, 3)" is contained within
"#ifdef CONFIG_ARM_64".
2. Updated commit message.
xen/arch/arm/vgic-v3.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 3f4509dcd3..a7a935ff57 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -191,12 +191,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
case VREG64(GICR_TYPER):
{
uint64_t typer, aff;
+ uint64_t vmpidr = v->arch.vmpidr;
if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
- aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
+ aff = (
+#ifdef CONFIG_ARM_64
+ MPIDR_AFFINITY_LEVEL(vmpidr, 3) << 56 |
+#endif
+ MPIDR_AFFINITY_LEVEL(vmpidr, 2) << 48 |
+ MPIDR_AFFINITY_LEVEL(vmpidr, 1) << 40 |
+ MPIDR_AFFINITY_LEVEL(vmpidr, 0) << 32);
typer = aff;
/* We use the VCPU ID as the redistributor ID in bits[23:8] */
typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 05/12] xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (3 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32 Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 06/12] xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32 Ayan Kumar Halder
` (6 subsequent siblings)
11 siblings, 0 replies; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
'unsigned long long' is defined as 64 bit across both AArch32 and AArch64.
So, use 'ULL' for 64 bit word instead of UL which is 32 bits for AArch32.
GICR_PENDBASER and GICR_PROPBASER both are 64 bit registers.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes from -
v1 - 1. Extract the bug fix for incorrect bit clearing (GICR_PENDBASER_PTZ)
into a separate patch fix.
https://patchwork.kernel.org/project/xen-devel/patch/20221027185555.46125-1-ayankuma@amd.com/
v2 - No changes.
xen/arch/arm/include/asm/gic_v3_defs.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 728e28d5e5..48a1bc401e 100644
--- a/xen/arch/arm/include/asm/gic_v3_defs.h
+++ b/xen/arch/arm/include/asm/gic_v3_defs.h
@@ -134,15 +134,15 @@
#define GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT 56
#define GICR_PROPBASER_OUTER_CACHEABILITY_MASK \
- (7UL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT)
+ (7ULL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT)
#define GICR_PROPBASER_SHAREABILITY_SHIFT 10
#define GICR_PROPBASER_SHAREABILITY_MASK \
- (3UL << GICR_PROPBASER_SHAREABILITY_SHIFT)
+ (3ULL << GICR_PROPBASER_SHAREABILITY_SHIFT)
#define GICR_PROPBASER_INNER_CACHEABILITY_SHIFT 7
#define GICR_PROPBASER_INNER_CACHEABILITY_MASK \
- (7UL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT)
+ (7ULL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT)
#define GICR_PROPBASER_RES0_MASK \
- (GENMASK(63, 59) | GENMASK(55, 52) | GENMASK(6, 5))
+ (GENMASK_ULL(63, 59) | GENMASK_ULL(55, 52) | GENMASK_ULL(6, 5))
#define GICR_PENDBASER_SHAREABILITY_SHIFT 10
#define GICR_PENDBASER_INNER_CACHEABILITY_SHIFT 7
@@ -152,11 +152,11 @@
#define GICR_PENDBASER_INNER_CACHEABILITY_MASK \
(7UL << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT)
#define GICR_PENDBASER_OUTER_CACHEABILITY_MASK \
- (7UL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT)
-#define GICR_PENDBASER_PTZ BIT(62, UL)
+ (7ULL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT)
+#define GICR_PENDBASER_PTZ BIT(62, ULL)
#define GICR_PENDBASER_RES0_MASK \
- (BIT(63, UL) | GENMASK(61, 59) | GENMASK(55, 52) | \
- GENMASK(15, 12) | GENMASK(6, 0))
+ (BIT(63, ULL) | GENMASK_ULL(61, 59) | GENMASK_ULL(55, 52) | \
+ GENMASK_ULL(15, 12) | GENMASK_ULL(6, 0))
#define DEFAULT_PMR_VALUE 0xff
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 06/12] xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (4 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 05/12] xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 " Ayan Kumar Halder
` (5 subsequent siblings)
11 siblings, 0 replies; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
Refer Arm IHI 0069H ID020922, 12.5.23, ICC_SGI1R is a 64 bit register on
AArch32 systems. Thus, the function needs to change to reflect this.
The reason being 'register_t' is defined as 'u32' on AArch32.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes from :-
v1 - 1. Updated the commit message.
v2 - 1. No changes.
xen/arch/arm/vgic-v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index a7a935ff57..93c8a0ae79 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1479,7 +1479,7 @@ write_reserved:
return 1;
}
-static bool vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
+static bool vgic_v3_to_sgi(struct vcpu *v, uint64_t sgir)
{
int virq;
int irqmode;
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (5 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 06/12] xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32 Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-18 10:13 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32 Ayan Kumar Halder
` (4 subsequent siblings)
11 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
Refer "Arm IHI 0069H ID020922", 12.4.6, Interrupt Controller List Registers
AArch64 System register ICH_LR<n>_EL2 bits [31:0] are architecturally
mapped to AArch32 System register ICH_LR<n>[31:0].
AArch64 System register ICH_LR<n>_EL2 bits [63:32] are architecturally
mapped to AArch32 System register ICH_LRC<n>[31:0].
Defined ICH_LR<0...15>_EL2 and ICH_LRC<0...15>_EL2 for AArch32.
For AArch32, the link register is stored as :-
(((uint64_t) ICH_LRC<0...15>_EL2) << 32) | ICH_LR<0...15>_EL2
Also, ICR_LR macros need to be modified as ULL is 64 bits for AArch32 and
AArch64.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-
v1 - 1. Moved the coproc register definitions to asm/cpregs.h.
2. Use GENMASK(31, 0) to represent 0xFFFFFFFF
3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG().
4. Multi-line macro definitions should be enclosed within ({ }).
v2 - 1. Use WRITE_SYSREG_LR(V, R) to make it consistent with before.
2. Defined the register alias.
3. Style issues.
xen/arch/arm/gic-v3.c | 132 +++++++++++------------
xen/arch/arm/include/asm/arm32/sysregs.h | 19 ++++
xen/arch/arm/include/asm/arm64/sysregs.h | 4 +
xen/arch/arm/include/asm/cpregs.h | 75 +++++++++++++
xen/arch/arm/include/asm/gic_v3_defs.h | 6 +-
5 files changed, 167 insertions(+), 69 deletions(-)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 64a76307dd..4722bb4daf 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -73,37 +73,37 @@ static inline void gicv3_save_lrs(struct vcpu *v)
switch ( gicv3_info.nr_lrs )
{
case 16:
- v->arch.gic.v3.lr[15] = READ_SYSREG(ICH_LR15_EL2);
+ v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
case 15:
- v->arch.gic.v3.lr[14] = READ_SYSREG(ICH_LR14_EL2);
+ v->arch.gic.v3.lr[14] = READ_SYSREG_LR(14);
case 14:
- v->arch.gic.v3.lr[13] = READ_SYSREG(ICH_LR13_EL2);
+ v->arch.gic.v3.lr[13] = READ_SYSREG_LR(13);
case 13:
- v->arch.gic.v3.lr[12] = READ_SYSREG(ICH_LR12_EL2);
+ v->arch.gic.v3.lr[12] = READ_SYSREG_LR(12);
case 12:
- v->arch.gic.v3.lr[11] = READ_SYSREG(ICH_LR11_EL2);
+ v->arch.gic.v3.lr[11] = READ_SYSREG_LR(11);
case 11:
- v->arch.gic.v3.lr[10] = READ_SYSREG(ICH_LR10_EL2);
+ v->arch.gic.v3.lr[10] = READ_SYSREG_LR(10);
case 10:
- v->arch.gic.v3.lr[9] = READ_SYSREG(ICH_LR9_EL2);
+ v->arch.gic.v3.lr[9] = READ_SYSREG_LR(9);
case 9:
- v->arch.gic.v3.lr[8] = READ_SYSREG(ICH_LR8_EL2);
+ v->arch.gic.v3.lr[8] = READ_SYSREG_LR(8);
case 8:
- v->arch.gic.v3.lr[7] = READ_SYSREG(ICH_LR7_EL2);
+ v->arch.gic.v3.lr[7] = READ_SYSREG_LR(7);
case 7:
- v->arch.gic.v3.lr[6] = READ_SYSREG(ICH_LR6_EL2);
+ v->arch.gic.v3.lr[6] = READ_SYSREG_LR(6);
case 6:
- v->arch.gic.v3.lr[5] = READ_SYSREG(ICH_LR5_EL2);
+ v->arch.gic.v3.lr[5] = READ_SYSREG_LR(5);
case 5:
- v->arch.gic.v3.lr[4] = READ_SYSREG(ICH_LR4_EL2);
+ v->arch.gic.v3.lr[4] = READ_SYSREG_LR(4);
case 4:
- v->arch.gic.v3.lr[3] = READ_SYSREG(ICH_LR3_EL2);
+ v->arch.gic.v3.lr[3] = READ_SYSREG_LR(3);
case 3:
- v->arch.gic.v3.lr[2] = READ_SYSREG(ICH_LR2_EL2);
+ v->arch.gic.v3.lr[2] = READ_SYSREG_LR(2);
case 2:
- v->arch.gic.v3.lr[1] = READ_SYSREG(ICH_LR1_EL2);
+ v->arch.gic.v3.lr[1] = READ_SYSREG_LR(1);
case 1:
- v->arch.gic.v3.lr[0] = READ_SYSREG(ICH_LR0_EL2);
+ v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
break;
default:
BUG();
@@ -120,37 +120,37 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
switch ( gicv3_info.nr_lrs )
{
case 16:
- WRITE_SYSREG(v->arch.gic.v3.lr[15], ICH_LR15_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
case 15:
- WRITE_SYSREG(v->arch.gic.v3.lr[14], ICH_LR14_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[14], 14);
case 14:
- WRITE_SYSREG(v->arch.gic.v3.lr[13], ICH_LR13_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[13], 13);
case 13:
- WRITE_SYSREG(v->arch.gic.v3.lr[12], ICH_LR12_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[12], 12);
case 12:
- WRITE_SYSREG(v->arch.gic.v3.lr[11], ICH_LR11_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[11], 11);
case 11:
- WRITE_SYSREG(v->arch.gic.v3.lr[10], ICH_LR10_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[10], 10);
case 10:
- WRITE_SYSREG(v->arch.gic.v3.lr[9], ICH_LR9_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[9], 9);
case 9:
- WRITE_SYSREG(v->arch.gic.v3.lr[8], ICH_LR8_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[8], 8);
case 8:
- WRITE_SYSREG(v->arch.gic.v3.lr[7], ICH_LR7_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[7], 7);
case 7:
- WRITE_SYSREG(v->arch.gic.v3.lr[6], ICH_LR6_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[6], 6);
case 6:
- WRITE_SYSREG(v->arch.gic.v3.lr[5], ICH_LR5_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[5], 5);
case 5:
- WRITE_SYSREG(v->arch.gic.v3.lr[4], ICH_LR4_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[4], 4);
case 4:
- WRITE_SYSREG(v->arch.gic.v3.lr[3], ICH_LR3_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[3], 3);
case 3:
- WRITE_SYSREG(v->arch.gic.v3.lr[2], ICH_LR2_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[2], 2);
case 2:
- WRITE_SYSREG(v->arch.gic.v3.lr[1], ICH_LR1_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[1], 1);
case 1:
- WRITE_SYSREG(v->arch.gic.v3.lr[0], ICH_LR0_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[0], 0);
break;
default:
BUG();
@@ -161,22 +161,22 @@ static uint64_t gicv3_ich_read_lr(int lr)
{
switch ( lr )
{
- case 0: return READ_SYSREG(ICH_LR0_EL2);
- case 1: return READ_SYSREG(ICH_LR1_EL2);
- case 2: return READ_SYSREG(ICH_LR2_EL2);
- case 3: return READ_SYSREG(ICH_LR3_EL2);
- case 4: return READ_SYSREG(ICH_LR4_EL2);
- case 5: return READ_SYSREG(ICH_LR5_EL2);
- case 6: return READ_SYSREG(ICH_LR6_EL2);
- case 7: return READ_SYSREG(ICH_LR7_EL2);
- case 8: return READ_SYSREG(ICH_LR8_EL2);
- case 9: return READ_SYSREG(ICH_LR9_EL2);
- case 10: return READ_SYSREG(ICH_LR10_EL2);
- case 11: return READ_SYSREG(ICH_LR11_EL2);
- case 12: return READ_SYSREG(ICH_LR12_EL2);
- case 13: return READ_SYSREG(ICH_LR13_EL2);
- case 14: return READ_SYSREG(ICH_LR14_EL2);
- case 15: return READ_SYSREG(ICH_LR15_EL2);
+ case 0: return READ_SYSREG_LR(0);
+ case 1: return READ_SYSREG_LR(1);
+ case 2: return READ_SYSREG_LR(2);
+ case 3: return READ_SYSREG_LR(3);
+ case 4: return READ_SYSREG_LR(4);
+ case 5: return READ_SYSREG_LR(5);
+ case 6: return READ_SYSREG_LR(6);
+ case 7: return READ_SYSREG_LR(7);
+ case 8: return READ_SYSREG_LR(8);
+ case 9: return READ_SYSREG_LR(9);
+ case 10: return READ_SYSREG_LR(10);
+ case 11: return READ_SYSREG_LR(11);
+ case 12: return READ_SYSREG_LR(12);
+ case 13: return READ_SYSREG_LR(13);
+ case 14: return READ_SYSREG_LR(14);
+ case 15: return READ_SYSREG_LR(15);
default:
BUG();
}
@@ -187,52 +187,52 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
switch ( lr )
{
case 0:
- WRITE_SYSREG(val, ICH_LR0_EL2);
+ WRITE_SYSREG_LR(val, 0);
break;
case 1:
- WRITE_SYSREG(val, ICH_LR1_EL2);
+ WRITE_SYSREG_LR(val, 1);
break;
case 2:
- WRITE_SYSREG(val, ICH_LR2_EL2);
+ WRITE_SYSREG_LR(val, 2);
break;
case 3:
- WRITE_SYSREG(val, ICH_LR3_EL2);
+ WRITE_SYSREG_LR(val, 3);
break;
case 4:
- WRITE_SYSREG(val, ICH_LR4_EL2);
+ WRITE_SYSREG_LR(val, 4);
break;
case 5:
- WRITE_SYSREG(val, ICH_LR5_EL2);
+ WRITE_SYSREG_LR(val, 5);
break;
case 6:
- WRITE_SYSREG(val, ICH_LR6_EL2);
+ WRITE_SYSREG_LR(val, 6);
break;
case 7:
- WRITE_SYSREG(val, ICH_LR7_EL2);
+ WRITE_SYSREG_LR(val, 7);
break;
case 8:
- WRITE_SYSREG(val, ICH_LR8_EL2);
+ WRITE_SYSREG_LR(val, 8);
break;
case 9:
- WRITE_SYSREG(val, ICH_LR9_EL2);
+ WRITE_SYSREG_LR(val, 9);
break;
case 10:
- WRITE_SYSREG(val, ICH_LR10_EL2);
+ WRITE_SYSREG_LR(val, 10);
break;
case 11:
- WRITE_SYSREG(val, ICH_LR11_EL2);
+ WRITE_SYSREG_LR(val, 11);
break;
case 12:
- WRITE_SYSREG(val, ICH_LR12_EL2);
+ WRITE_SYSREG_LR(val, 12);
break;
case 13:
- WRITE_SYSREG(val, ICH_LR13_EL2);
+ WRITE_SYSREG_LR(val, 13);
break;
case 14:
- WRITE_SYSREG(val, ICH_LR14_EL2);
+ WRITE_SYSREG_LR(val, 14);
break;
case 15:
- WRITE_SYSREG(val, ICH_LR15_EL2);
+ WRITE_SYSREG_LR(val, 15);
break;
default:
return;
@@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
if ( v == current )
{
for ( i = 0; i < gicv3_info.nr_lrs; i++ )
- printk(" HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
+ printk(" HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
}
else
{
for ( i = 0; i < gicv3_info.nr_lrs; i++ )
- printk(" VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
+ printk(" VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
}
}
diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h
index 6841d5de43..31ad7eaefb 100644
--- a/xen/arch/arm/include/asm/arm32/sysregs.h
+++ b/xen/arch/arm/include/asm/arm32/sysregs.h
@@ -62,6 +62,25 @@
#define READ_SYSREG(R...) READ_SYSREG32(R)
#define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R)
+/* Wrappers for accessing interrupt controller list registers. */
+#define ICH_LR_REG(index) ICH_LR ## index ## _EL2
+#define ICH_LRC_REG(index) ICH_LRC ## index ## _EL2
+
+#define READ_SYSREG_LR(index) ({ \
+ uint64_t _val; \
+ uint32_t _lrc = READ_CP32(ICH_LRC_REG(index)); \
+ uint32_t _lr = READ_CP32(ICH_LR_REG(index)); \
+ \
+ _val = ((uint64_t) _lrc << 32) | _lr; \
+ _val; \
+})
+
+#define WRITE_SYSREG_LR(V, index) ({ \
+ uint64_t _val = (V); \
+ WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(index)); \
+ WRITE_CP32(_val >> 32, ICH_LRC_REG(index)); \
+})
+
/* MVFR2 is not defined on ARMv7 */
#define MVFR2_MAYBE_UNDEFINED
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index 54670084c3..b72a6ea3b0 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -472,6 +472,10 @@
#define READ_SYSREG(name) READ_SYSREG64(name)
#define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
+/* Wrappers for accessing interrupt controller list registers. */
+#define ICH_LR_REG(index) ICH_LR ## index ## _EL2
+#define WRITE_SYSREG_LR(V, index) WRITE_SYSREG(V, ICH_LR_REG(index))
+#define READ_SYSREG_LR(index) READ_SYSREG(ICH_LR_REG(index))
#endif /* _ASM_ARM_ARM64_SYSREGS_H */
/*
diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
index 6daf2b1a30..242dabaea8 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -259,6 +259,49 @@
#define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
#define HVBAR p15,4,c12,c0,0 /* Hyp. Vector Base Address Register */
+/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
+#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
+#define __LR0(x) ___CP32(p15, 4, c12, c12, x)
+#define __LR8(x) ___CP32(p15, 4, c12, c13, x)
+
+#define ICH_LR0 __LR0(0)
+#define ICH_LR1 __LR0(1)
+#define ICH_LR2 __LR0(2)
+#define ICH_LR3 __LR0(3)
+#define ICH_LR4 __LR0(4)
+#define ICH_LR5 __LR0(5)
+#define ICH_LR6 __LR0(6)
+#define ICH_LR7 __LR0(7)
+#define ICH_LR8 __LR8(0)
+#define ICH_LR9 __LR8(1)
+#define ICH_LR10 __LR8(2)
+#define ICH_LR11 __LR8(3)
+#define ICH_LR12 __LR8(4)
+#define ICH_LR13 __LR8(5)
+#define ICH_LR14 __LR8(6)
+#define ICH_LR15 __LR8(7)
+
+/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
+#define __LRC0(x) ___CP32(p15, 4, c12, c14, x)
+#define __LRC8(x) ___CP32(p15, 4, c12, c15, x)
+
+#define ICH_LRC0 __LRC0(0)
+#define ICH_LRC1 __LRC0(1)
+#define ICH_LRC2 __LRC0(2)
+#define ICH_LRC3 __LRC0(3)
+#define ICH_LRC4 __LRC0(4)
+#define ICH_LRC5 __LRC0(5)
+#define ICH_LRC6 __LRC0(6)
+#define ICH_LRC7 __LRC0(7)
+#define ICH_LRC8 __LRC8(0)
+#define ICH_LRC9 __LRC8(1)
+#define ICH_LRC10 __LRC8(2)
+#define ICH_LRC11 __LRC8(3)
+#define ICH_LRC12 __LRC8(4)
+#define ICH_LRC13 __LRC8(5)
+#define ICH_LRC14 __LRC8(6)
+#define ICH_LRC15 __LRC8(7)
+
/* CP15 CR13: */
#define FCSEIDR p15,0,c13,c0,0 /* FCSE Process ID Register */
#define CONTEXTIDR p15,0,c13,c0,1 /* Context ID Register */
@@ -317,6 +360,38 @@
#define HCR_EL2 HCR
#define HPFAR_EL2 HPFAR
#define HSTR_EL2 HSTR
+#define ICH_LR0_EL2 ICH_LR0
+#define ICH_LR1_EL2 ICH_LR1
+#define ICH_LR2_EL2 ICH_LR2
+#define ICH_LR3_EL2 ICH_LR3
+#define ICH_LR4_EL2 ICH_LR4
+#define ICH_LR5_EL2 ICH_LR5
+#define ICH_LR6_EL2 ICH_LR6
+#define ICH_LR7_EL2 ICH_LR7
+#define ICH_LR8_EL2 ICH_LR8
+#define ICH_LR9_EL2 ICH_LR9
+#define ICH_LR10_EL2 ICH_LR10
+#define ICH_LR11_EL2 ICH_LR11
+#define ICH_LR12_EL2 ICH_LR12
+#define ICH_LR13_EL2 ICH_LR13
+#define ICH_LR14_EL2 ICH_LR14
+#define ICH_LR15_EL2 ICH_LR15
+#define ICH_LRC0_EL2 ICH_LRC0
+#define ICH_LRC1_EL2 ICH_LRC1
+#define ICH_LRC2_EL2 ICH_LRC2
+#define ICH_LRC3_EL2 ICH_LRC3
+#define ICH_LRC4_EL2 ICH_LRC4
+#define ICH_LRC5_EL2 ICH_LRC5
+#define ICH_LRC6_EL2 ICH_LRC6
+#define ICH_LRC7_EL2 ICH_LRC7
+#define ICH_LRC8_EL2 ICH_LRC8
+#define ICH_LRC9_EL2 ICH_LRC9
+#define ICH_LRC10_EL2 ICH_LRC10
+#define ICH_LRC11_EL2 ICH_LRC11
+#define ICH_LRC12_EL2 ICH_LRC12
+#define ICH_LRC13_EL2 ICH_LRC13
+#define ICH_LRC14_EL2 ICH_LRC14
+#define ICH_LRC15_EL2 ICH_LRC15
#define ID_AFR0_EL1 ID_AFR0
#define ID_DFR0_EL1 ID_DFR0
#define ID_DFR1_EL1 ID_DFR1
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 48a1bc401e..743ebb20fd 100644
--- a/xen/arch/arm/include/asm/gic_v3_defs.h
+++ b/xen/arch/arm/include/asm/gic_v3_defs.h
@@ -185,9 +185,9 @@
#define ICH_LR_HW_SHIFT 61
#define ICH_LR_GRP_MASK 0x1
#define ICH_LR_GRP_SHIFT 60
-#define ICH_LR_MAINTENANCE_IRQ (1UL<<41)
-#define ICH_LR_GRP1 (1UL<<60)
-#define ICH_LR_HW (1UL<<61)
+#define ICH_LR_MAINTENANCE_IRQ (1ULL << 41)
+#define ICH_LR_GRP1 (1ULL << 60)
+#define ICH_LR_HW (1ULL << 61)
#define ICH_VTR_NRLRGS 0x3f
#define ICH_VTR_PRIBITS_MASK 0x7
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (6 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 " Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-18 12:26 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 09/12] xen/Arm: GICv3: Define remaining GIC registers " Ayan Kumar Halder
` (3 subsequent siblings)
11 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
Adapt save_aprn_regs()/restore_aprn_regs() for AArch32.
For which we have defined the following registers:-
1. Interrupt Controller Hyp Active Priorities Group0 Registers 0-3
2. Interrupt Controller Hyp Active Priorities Group1 Registers 0-3
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-
v1 - 1. Moved coproc register definition to asm/cpregs.h.
v2 - 1. Defined register alias.
2. Style issues.
3. Dropped R-b and Ack.
xen/arch/arm/include/asm/cpregs.h | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
index 242dabaea8..5331ec3448 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -259,6 +259,26 @@
#define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
#define HVBAR p15,4,c12,c0,0 /* Hyp. Vector Base Address Register */
+/*
+ * CP15 CR12: Interrupt Controller Hyp Active Priorities Group 0 Registers,
+ * n = 0 - 3
+ */
+#define __AP0Rx(x) ___CP32(p15, 4, c12, c8, x)
+#define ICH_AP0R0 __AP0Rx(0)
+#define ICH_AP0R1 __AP0Rx(1)
+#define ICH_AP0R2 __AP0Rx(2)
+#define ICH_AP0R3 __AP0Rx(3)
+
+/*
+ * CP15 CR12: Interrupt Controller Hyp Active Priorities Group 1 Registers,
+ * n = 0 - 3
+ */
+#define __AP1Rx(x) ___CP32(p15, 4, c12, c9, x)
+#define ICH_AP1R0 __AP1Rx(0)
+#define ICH_AP1R1 __AP1Rx(1)
+#define ICH_AP1R2 __AP1Rx(2)
+#define ICH_AP1R3 __AP1Rx(3)
+
/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
#define __LR0(x) ___CP32(p15, 4, c12, c12, x)
@@ -360,6 +380,14 @@
#define HCR_EL2 HCR
#define HPFAR_EL2 HPFAR
#define HSTR_EL2 HSTR
+#define ICH_AP0R0_EL2 ICH_AP0R0
+#define ICH_AP0R1_EL2 ICH_AP0R1
+#define ICH_AP0R2_EL2 ICH_AP0R2
+#define ICH_AP0R3_EL2 ICH_AP0R3
+#define ICH_AP1R0_EL2 ICH_AP1R0
+#define ICH_AP1R1_EL2 ICH_AP1R1
+#define ICH_AP1R2_EL2 ICH_AP1R2
+#define ICH_AP1R3_EL2 ICH_AP1R3
#define ICH_LR0_EL2 ICH_LR0
#define ICH_LR1_EL2 ICH_LR1
#define ICH_LR2_EL2 ICH_LR2
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 09/12] xen/Arm: GICv3: Define remaining GIC registers for AArch32
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (7 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32 Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-18 12:43 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits Ayan Kumar Halder
` (2 subsequent siblings)
11 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
Define missing assembly aliases for GIC registers on arm32, taking the ones
defined already for arm64 as a base. Aliases are defined according to the
GIC Architecture Specification ARM IHI 0069H.
Defined the following registers:-
1. Interrupt Controller Interrupt Priority Mask Register
2. Interrupt Controller System Register Enable register
3. Interrupt Controller Deactivate Interrupt Register
4. Interrupt Controller End Of Interrupt Register 1
5. Interrupt Controller Interrupt Acknowledge Register 1
6. Interrupt Controller Binary Point Register 1
7. Interrupt Controller Control Register
8. Interrupt Controller Interrupt Group 1 Enable register
9. Interrupt Controller Maintenance Interrupt State Register
10. Interrupt Controller End of Interrupt Status Register
11. Interrupt Controller Empty List Register Status Register
12. Interrupt Controller Virtual Machine Control Register
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-
v1 - 1. Moved coproc regs definition to asm/cpregs.h
v2 - 1. Defined register alias.
2. Style issues.
3. Defined ELSR, MISR, EISR to make it consistent with AArch64.
xen/arch/arm/include/asm/cpregs.h | 32 +++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
index 5331ec3448..0fc606fe99 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -161,6 +161,7 @@
#define DACR p15,0,c3,c0,0 /* Domain Access Control Register */
/* CP15 CR4: */
+#define ICC_PMR p15,0,c4,c6,0 /* Interrupt Priority Mask Register */
/* CP15 CR5: Fault Status Registers */
#define DFSR p15,0,c5,c0,0 /* Data Fault Status Register */
@@ -254,6 +255,8 @@
/* CP15 CR12: */
#define ICC_SGI1R p15,0,c12 /* Interrupt Controller SGI Group 1 */
+#define ICC_DIR p15,0,c12,c11,1 /* Interrupt Controller Deactivate Interrupt Register */
+#define ICC_SRE_L1 p15,0,c12,c12,5 /* Interrupt Controller System Register Enable register */
#define ICC_ASGI1R p15,1,c12 /* Interrupt Controller Alias SGI Group 1 Register */
#define ICC_SGI0R p15,2,c12 /* Interrupt Controller SGI Group 0 */
#define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
@@ -279,6 +282,19 @@
#define ICH_AP1R2 __AP1Rx(2)
#define ICH_AP1R3 __AP1Rx(3)
+#define ICC_IAR1 p15,0,c12,c12,0 /* Interrupt Controller Interrupt Acknowledge Register 1 */
+#define ICC_EOIR1 p15,0,c12,c12,1 /* Interrupt Controller End Of Interrupt Register 1 */
+#define ICC_BPR1 p15,0,c12,c12,3 /* Interrupt Controller Binary Point Register 1 */
+#define ICC_CTLR p15,0,c12,c12,4 /* Interrupt Controller Control Register */
+#define ICC_IGRPEN1 p15,0,c12,c12,7 /* Interrupt Controller Interrupt Group 1 Enable register */
+#define ICC_SRE p15,4,c12,c9,5 /* Interrupt Controller Hyp System Register Enable register */
+#define ICH_HCR p15,4,c12,c11,0 /* Interrupt Controller Hyp Control Register */
+#define ICH_VTR p15,4,c12,c11,1 /* Interrupt Controller VGIC Type Register */
+#define ICH_MISR p15,4,c12,c11,2 /* Interrupt Controller Maintenance Interrupt State Register */
+#define ICH_EISR p15,4,c12,c11,3 /* Interrupt Controller End of Interrupt Status Register */
+#define ICH_ELRSR p15,4,c12,c11,5 /* Interrupt Controller Empty List Register Status Register */
+#define ICH_VMCR p15,4,c12,c11,7 /* Interrupt Controller Virtual Machine Control Register */
+
/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
#define __LR0(x) ___CP32(p15, 4, c12, c12, x)
@@ -380,6 +396,15 @@
#define HCR_EL2 HCR
#define HPFAR_EL2 HPFAR
#define HSTR_EL2 HSTR
+#define ICC_BPR1_EL1 ICC_BPR1
+#define ICC_CTLR_EL1 ICC_CTLR
+#define ICC_DIR_EL1 ICC_DIR
+#define ICC_EOIR1_EL1 ICC_EOIR1
+#define ICC_IGRPEN1_EL1 ICC_IGRPEN1
+#define ICC_PMR_EL1 ICC_PMR
+#define ICC_SGI1R_EL1 ICC_SGI1R
+#define ICC_SRE_EL1 ICC_SRE_L1
+#define ICC_SRE_EL2 ICC_SRE
#define ICH_AP0R0_EL2 ICH_AP0R0
#define ICH_AP0R1_EL2 ICH_AP0R1
#define ICH_AP0R2_EL2 ICH_AP0R2
@@ -388,6 +413,10 @@
#define ICH_AP1R1_EL2 ICH_AP1R1
#define ICH_AP1R2_EL2 ICH_AP1R2
#define ICH_AP1R3_EL2 ICH_AP1R3
+#define ICH_EISR_EL2 ICH_EISR
+#define ICH_ELRSR_EL2 ICH_ELRSR
+#define ICH_HCR_EL2 ICH_HCR
+#define ICC_IAR1_EL1 ICC_IAR1
#define ICH_LR0_EL2 ICH_LR0
#define ICH_LR1_EL2 ICH_LR1
#define ICH_LR2_EL2 ICH_LR2
@@ -420,6 +449,9 @@
#define ICH_LRC13_EL2 ICH_LRC13
#define ICH_LRC14_EL2 ICH_LRC14
#define ICH_LRC15_EL2 ICH_LRC15
+#define ICH_MISR_EL2 ICH_MISR
+#define ICH_VMCR_EL2 ICH_VMCR
+#define ICH_VTR_EL2 ICH_VTR
#define ID_AFR0_EL1 ID_AFR0
#define ID_DFR0_EL1 ID_DFR0
#define ID_DFR1_EL1 ID_DFR1
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (8 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 09/12] xen/Arm: GICv3: Define remaining GIC registers " Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-18 12:58 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32 Ayan Kumar Halder
11 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
"unsigned long long" is defined as 64 bits on AArch64 and AArch32
Thus, one should this instead of "unsigned long" which is 32 bits
on AArch32.
Also use 'PRIx64' instead of 'lx' or 'llx' to print uint64_t.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changed from :-
v1 - 1. Replace PRIu64 with PRIx64 so that the values are printed in hex as
desired.
2. Use ULL in GITS_BASER_RO_MASK as MMIO registers are always unsigned.
v2 - 1. Removed changes to ITS and LPI as they are not supported for AArch32.
xen/arch/arm/gic-v3.c | 4 ++--
xen/arch/arm/include/asm/gic_v3_defs.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 4722bb4daf..6457e7033c 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
if ( v == current )
{
for ( i = 0; i < gicv3_info.nr_lrs; i++ )
- printk(" HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
+ printk(" HW_LR[%d]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
}
else
{
for ( i = 0; i < gicv3_info.nr_lrs; i++ )
- printk(" VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
+ printk(" VCPU_LR[%d]=%" PRIx64 "\n", i, v->arch.gic.v3.lr[i]);
}
}
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 743ebb20fd..227533868f 100644
--- a/xen/arch/arm/include/asm/gic_v3_defs.h
+++ b/xen/arch/arm/include/asm/gic_v3_defs.h
@@ -195,7 +195,7 @@
#define ICH_SGI_IRQMODE_SHIFT 40
#define ICH_SGI_IRQMODE_MASK 0x1
-#define ICH_SGI_TARGET_OTHERS 1UL
+#define ICH_SGI_TARGET_OTHERS 1ULL
#define ICH_SGI_TARGET_LIST 0
#define ICH_SGI_IRQ_SHIFT 24
#define ICH_SGI_IRQ_MASK 0xf
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (9 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-11 16:17 ` Xenia Ragiadakou
2022-11-11 14:17 ` [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32 Ayan Kumar Halder
11 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
On AArch32, ldrd/strd instructions are not atomic when used to access MMIO.
Furthermore, ldrd/strd instructions are not decoded by Arm when running as
a guest to access emulated MMIO region.
Thus, we have defined readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
which in turn calls readl_relaxed()/writel_relaxed() for the lower and upper
32 bits.
As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a non atomic
fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-
v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
2. No need to use le64_to_cpu() as the returned byte order is already in cpu
endianess.
v2 - 1. Replace {read/write}q_relaxed with {read/write}q_relaxed_non_atomic().
xen/arch/arm/gic-v3.c | 12 ++++++++++++
xen/arch/arm/include/asm/arm32/io.h | 9 +++++++++
2 files changed, 21 insertions(+)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 6457e7033c..a5bc549765 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
+#ifdef CONFIG_ARM_32
+ writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i * 8);
+#else
writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
+#endif
}
static int gicv3_enable_redist(void)
@@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
}
do {
+#ifdef CONFIG_ARM_32
+ typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
+#else
typer = readq_relaxed(ptr + GICR_TYPER);
+#endif
if ( (typer >> 32) == aff )
{
@@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
if ( desc->irq >= NR_GIC_LOCAL_IRQS )
+#ifdef CONFIG_ARM_32
+ writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
+#else
writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
+#endif
spin_unlock(&gicv3.lock);
}
diff --git a/xen/arch/arm/include/asm/arm32/io.h b/xen/arch/arm/include/asm/arm32/io.h
index 73a879e9fb..4ddfbea5c2 100644
--- a/xen/arch/arm/include/asm/arm32/io.h
+++ b/xen/arch/arm/include/asm/arm32/io.h
@@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile void __iomem *addr)
__raw_readw(c)); __r; })
#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
__raw_readl(c)); __r; })
+#define readq_relaxed_non_atomic(c) \
+ ({ u64 __r = (((u64)readl_relaxed((c) + 4)) << 32) | \
+ readl_relaxed(c); __r; })
#define writeb_relaxed(v,c) __raw_writeb(v,c)
#define writew_relaxed(v,c) __raw_writew((__force u16) cpu_to_le16(v),c)
#define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c)
+#define writeq_relaxed_non_atomic(v,c) \
+ ({ writel_relaxed((u32)v, c); \
+ writel_relaxed((u32)((v) >> 32), (c) + 4); })
#define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; })
#define readw(c) ({ u16 __v = readw_relaxed(c); __iormb(); __v; })
#define readl(c) ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
+#define readq(c) ({ u64 __v = readq_relaxed_non_atomic(c); \
+ __iormb(); __v; })
#define writeb(v,c) ({ __iowmb(); writeb_relaxed(v,c); })
#define writew(v,c) ({ __iowmb(); writew_relaxed(v,c); })
#define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })
+#define writeq(v,c) ({ __iowmb(); writeq_relaxed_non_atomic(v,c); })
#endif /* _ARM_ARM32_IO_H */
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
` (10 preceding siblings ...)
2022-11-11 14:17 ` [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit Ayan Kumar Halder
@ 2022-11-11 14:17 ` Ayan Kumar Halder
2022-11-23 9:51 ` Michal Orzel
2022-11-25 8:42 ` Julien Grall
11 siblings, 2 replies; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 14:17 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall, burzalodowa,
Ayan Kumar Halder
One can now use GICv3 on AArch32 systems. However, ITS is not supported.
The reason being currently we are trying to validate GICv3 on an AArch32_v8R
system. Refer ARM DDI 0568A.c ID110520, B1.3.1,
"A Generic Interrupt Controller (GIC) implemented with an Armv8-R PE must not
implement LPI support."
By default GICv3 is disabled on AArch32 and enabled on AArch64.
Updated SUPPORT.md to state that GICv3 on Arm32 is not security supported.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changed from :-
v1 - 1. Remove "ARM_64 || ARM_32" as it is always true.
2. Updated SUPPORT.md.
v2 - 1. GICv3 is enabled by default only on ARM_64.
2. Updated SUPPORT.md.
SUPPORT.md | 7 +++++++
xen/arch/arm/Kconfig | 9 +++++----
xen/arch/arm/include/asm/cpufeature.h | 1 +
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/SUPPORT.md b/SUPPORT.md
index ab71464cf6..3f16d83191 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -82,6 +82,13 @@ Extension to the GICv3 interrupt controller to support MSI.
Status: Experimental
+### ARM/GICv3
+
+GICv3 is an interrupt controller specification designed by Arm.
+
+ Status, Arm64: Security supported
+ Status, Arm32: Supported, not security supported
+
## Guest Type
### x86/PV
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 1fe5faf847..b90930955b 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -9,6 +9,7 @@ config ARM_64
select 64BIT
select ARM_EFI
select HAS_FAST_MULTIPLY
+ select GICV3
config ARM
def_bool y
@@ -41,16 +42,16 @@ config ARM_EFI
config GICV3
bool "GICv3 driver"
- depends on ARM_64 && !NEW_VGIC
- default y
+ depends on !NEW_VGIC
+ default n
---help---
Driver for the ARM Generic Interrupt Controller v3.
- If unsure, say Y
+ If unsure, say N
config HAS_ITS
bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
- depends on GICV3 && !NEW_VGIC
+ depends on GICV3 && !NEW_VGIC && !ARM_32
config HVM
def_bool y
diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
index c86a2e7f29..c62cf6293f 100644
--- a/xen/arch/arm/include/asm/cpufeature.h
+++ b/xen/arch/arm/include/asm/cpufeature.h
@@ -33,6 +33,7 @@
#define cpu_has_aarch32 (cpu_has_arm || cpu_has_thumb)
#ifdef CONFIG_ARM_32
+#define cpu_has_gicv3 (boot_cpu_feature32(gic) >= 1)
#define cpu_has_gentimer (boot_cpu_feature32(gentimer) == 1)
/*
* On Armv7, the value 0 is used to indicate that PMUv2 is not
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit
2022-11-11 14:17 ` [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit Ayan Kumar Halder
@ 2022-11-11 16:17 ` Xenia Ragiadakou
2022-11-11 17:37 ` Ayan Kumar Halder
0 siblings, 1 reply; 36+ messages in thread
From: Xenia Ragiadakou @ 2022-11-11 16:17 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall
Hi Ayan,
On 11/11/22 16:17, Ayan Kumar Halder wrote:
> On AArch32, ldrd/strd instructions are not atomic when used to access MMIO.
> Furthermore, ldrd/strd instructions are not decoded by Arm when running as
> a guest to access emulated MMIO region.
> Thus, we have defined readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
> which in turn calls readl_relaxed()/writel_relaxed() for the lower and upper
> 32 bits.
> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a non atomic
> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>
> Changes from :-
> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
> 2. No need to use le64_to_cpu() as the returned byte order is already in cpu
> endianess.
>
> v2 - 1. Replace {read/write}q_relaxed with {read/write}q_relaxed_non_atomic().
>
> xen/arch/arm/gic-v3.c | 12 ++++++++++++
> xen/arch/arm/include/asm/arm32/io.h | 9 +++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 6457e7033c..a5bc549765 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
> affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>
> for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
> +#ifdef CONFIG_ARM_32
> + writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i * 8);
> +#else
> writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
> +#endif
> }
>
> static int gicv3_enable_redist(void)
> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
> }
>
> do {
> +#ifdef CONFIG_ARM_32
> + typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
> +#else
> typer = readq_relaxed(ptr + GICR_TYPER);
> +#endif
>
> if ( (typer >> 32) == aff )
> {
> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>
> if ( desc->irq >= NR_GIC_LOCAL_IRQS )
> +#ifdef CONFIG_ARM_32
> + writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
> +#else
> writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
> +#endif
>
> spin_unlock(&gicv3.lock);
> }
> diff --git a/xen/arch/arm/include/asm/arm32/io.h b/xen/arch/arm/include/asm/arm32/io.h
> index 73a879e9fb..4ddfbea5c2 100644
> --- a/xen/arch/arm/include/asm/arm32/io.h
> +++ b/xen/arch/arm/include/asm/arm32/io.h
> @@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile void __iomem *addr)
> __raw_readw(c)); __r; })
> #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
> __raw_readl(c)); __r; })
> +#define readq_relaxed_non_atomic(c) \
> + ({ u64 __r = (((u64)readl_relaxed((c) + 4)) << 32) | \
> + readl_relaxed(c); __r; })
As Julien pointed out, the expression c will be evaluated twice and if
it produces side effects they will be performed twice.
To prevent this, you can either assign the expression to a local
variable and pass this one to readl_relaxed() or use a static inline
function instead of a macro, for implementing readq_relaxed_non_atomic().
The latter is the MISRA C recommended (not strictly required) approach
according to Dir 4.9 "A function should be used in preference to a
function-like macro where
they are interchangeable".
...
>
> #define writeb_relaxed(v,c) __raw_writeb(v,c)
> #define writew_relaxed(v,c) __raw_writew((__force u16) cpu_to_le16(v),c)
> #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c)
> +#define writeq_relaxed_non_atomic(v,c) \
> + ({ writel_relaxed((u32)v, c); \
> + writel_relaxed((u32)((v) >> 32), (c) + 4); })
... same here.
>
> #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; })
> #define readw(c) ({ u16 __v = readw_relaxed(c); __iormb(); __v; })
> #define readl(c) ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
> +#define readq(c) ({ u64 __v = readq_relaxed_non_atomic(c); \
> + __iormb(); __v; })
I think that, here also, the macro identifier needs to inform that the
access is non-atomic.
...
>
> #define writeb(v,c) ({ __iowmb(); writeb_relaxed(v,c); })
> #define writew(v,c) ({ __iowmb(); writew_relaxed(v,c); })
> #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })
> +#define writeq(v,c) ({ __iowmb(); writeq_relaxed_non_atomic(v,c); })
... same here.
>
> #endif /* _ARM_ARM32_IO_H */
--
Xenia
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit
2022-11-11 16:17 ` Xenia Ragiadakou
@ 2022-11-11 17:37 ` Ayan Kumar Halder
2022-11-11 17:53 ` Julien Grall
0 siblings, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-11 17:37 UTC (permalink / raw)
To: Xenia Ragiadakou, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, michal.orzel, jgrall
On 11/11/2022 16:17, Xenia Ragiadakou wrote:
> Hi Ayan,
Hi Xenia,
>
> On 11/11/22 16:17, Ayan Kumar Halder wrote:
>> On AArch32, ldrd/strd instructions are not atomic when used to access
>> MMIO.
>> Furthermore, ldrd/strd instructions are not decoded by Arm when
>> running as
>> a guest to access emulated MMIO region.
>> Thus, we have defined
>> readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
>> which in turn calls readl_relaxed()/writel_relaxed() for the lower
>> and upper
>> 32 bits.
>> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a
>> non atomic
>> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>
>> Changes from :-
>> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
>> 2. No need to use le64_to_cpu() as the returned byte order is already
>> in cpu
>> endianess.
>>
>> v2 - 1. Replace {read/write}q_relaxed with
>> {read/write}q_relaxed_non_atomic().
>>
>> xen/arch/arm/gic-v3.c | 12 ++++++++++++
>> xen/arch/arm/include/asm/arm32/io.h | 9 +++++++++
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 6457e7033c..a5bc549765 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
>> affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>> for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
>> +#ifdef CONFIG_ARM_32
>> + writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i
>> * 8);
>> +#else
>> writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
>> +#endif
>> }
>> static int gicv3_enable_redist(void)
>> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
>> }
>> do {
>> +#ifdef CONFIG_ARM_32
>> + typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
>> +#else
>> typer = readq_relaxed(ptr + GICR_TYPER);
>> +#endif
>> if ( (typer >> 32) == aff )
>> {
>> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct
>> irq_desc *desc, const cpumask_t *mask)
>> affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>> if ( desc->irq >= NR_GIC_LOCAL_IRQS )
>> +#ifdef CONFIG_ARM_32
>> + writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER +
>> desc->irq * 8));
>> +#else
>> writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq *
>> 8));
>> +#endif
>> spin_unlock(&gicv3.lock);
>> }
>> diff --git a/xen/arch/arm/include/asm/arm32/io.h
>> b/xen/arch/arm/include/asm/arm32/io.h
>> index 73a879e9fb..4ddfbea5c2 100644
>> --- a/xen/arch/arm/include/asm/arm32/io.h
>> +++ b/xen/arch/arm/include/asm/arm32/io.h
>> @@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile void
>> __iomem *addr)
>> __raw_readw(c)); __r; })
>> #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>> __raw_readl(c)); __r; })
>> +#define readq_relaxed_non_atomic(c) \
>> + ({ u64 __r = (((u64)readl_relaxed((c) + 4))
>> << 32) | \
>> + readl_relaxed(c); __r; })
>
> As Julien pointed out, the expression c will be evaluated twice and if
> it produces side effects they will be performed twice.
> To prevent this, you can either assign the expression to a local
> variable and pass this one to readl_relaxed()
Just to make sure I understand you correctly, you are suggesting this :-
#define readq_relaxed_non_atomic(c) \
({ void _iomem *__addr = (c); \
u64 __r = (((u64)readl_relaxed(__addr + 4))
<< 32) | \
readl_relaxed(__addr); __r; })
#define writeq_relaxed_non_atomic(v,c) \
(( u64 __v = (v); \
void _iomem *__addr = (c); \
writel_relaxed((u32)__v, __addr); \
writel_relaxed((u32)((__v) >> 32), (__addr +
4); })
Is this correct understanding ?
> or use a static inline function instead of a macro, for implementing
> readq_relaxed_non_atomic().
> The latter is the MISRA C recommended (not strictly required) approach
> according to Dir 4.9 "A function should be used in preference to a
> function-like macro where
> they are interchangeable".
I have mixed opinion about this.
On one hand, there will be a performance penalty when invoking a
function (compared to macro).
On the other hand {readq/writeq}_relaxed_non_atomic() are called during
init (gicv3 initialization, setting up the interrupt handlers), so the
impact will not be bad.
I am fine with whatever you and any maintainer suggest.
Also now I realize that I had missed another point raised by Julien (a
code comment explaining why ldrd/strd cannot be used) :(.
I will address this in my next version
> ...
>
>> #define writeb_relaxed(v,c) __raw_writeb(v,c)
>> #define writew_relaxed(v,c) __raw_writew((__force u16)
>> cpu_to_le16(v),c)
>> #define writel_relaxed(v,c) __raw_writel((__force u32)
>> cpu_to_le32(v),c)
>> +#define writeq_relaxed_non_atomic(v,c) \
>> + ({ writel_relaxed((u32)v, c); \
>> + writel_relaxed((u32)((v) >> 32),
>> (c) + 4); })
>
> ... same here.
Ack
>
>> #define readb(c) ({ u8 __v = readb_relaxed(c);
>> __iormb(); __v; })
>> #define readw(c) ({ u16 __v = readw_relaxed(c);
>> __iormb(); __v; })
>> #define readl(c) ({ u32 __v = readl_relaxed(c);
>> __iormb(); __v; })
>> +#define readq(c) ({ u64 __v =
>> readq_relaxed_non_atomic(c); \
>> + __iormb(); __v; })
>
> I think that, here also, the macro identifier needs to inform that the
> access is non-atomic.
I will remove this as we will be using readq_relaxed_non_atomic() in the
code.
> ...
>
>> #define writeb(v,c) ({ __iowmb();
>> writeb_relaxed(v,c); })
>> #define writew(v,c) ({ __iowmb(); writew_relaxed(v,c); })
>> #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })
>> +#define writeq(v,c) ({ __iowmb();
>> writeq_relaxed_non_atomic(v,c); })
>
> ... same here.
I will remove this as we will be using writeq_relaxed_non_atomic() in
the code.
- Ayan
>
>> #endif /* _ARM_ARM32_IO_H */
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit
2022-11-11 17:37 ` Ayan Kumar Halder
@ 2022-11-11 17:53 ` Julien Grall
2022-11-28 12:32 ` Ayan Kumar Halder
0 siblings, 1 reply; 36+ messages in thread
From: Julien Grall @ 2022-11-11 17:53 UTC (permalink / raw)
To: Ayan Kumar Halder, Xenia Ragiadakou, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
michal.orzel, jgrall
Hi,
On 11/11/2022 17:37, Ayan Kumar Halder wrote:
>
> On 11/11/2022 16:17, Xenia Ragiadakou wrote:
>> Hi Ayan,
> Hi Xenia,
>>
>> On 11/11/22 16:17, Ayan Kumar Halder wrote:
>>> On AArch32, ldrd/strd instructions are not atomic when used to access
>>> MMIO.
>>> Furthermore, ldrd/strd instructions are not decoded by Arm when
>>> running as
>>> a guest to access emulated MMIO region.
>>> Thus, we have defined
>>> readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
>>> which in turn calls readl_relaxed()/writel_relaxed() for the lower
>>> and upper
>>> 32 bits.
>>> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a
>>> non atomic
>>> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>>
>>> Changes from :-
>>> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
>>> 2. No need to use le64_to_cpu() as the returned byte order is already
>>> in cpu
>>> endianess.
>>>
>>> v2 - 1. Replace {read/write}q_relaxed with
>>> {read/write}q_relaxed_non_atomic().
>>>
>>> xen/arch/arm/gic-v3.c | 12 ++++++++++++
>>> xen/arch/arm/include/asm/arm32/io.h | 9 +++++++++
>>> 2 files changed, 21 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index 6457e7033c..a5bc549765 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
>>> affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>> for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
>>> +#ifdef CONFIG_ARM_32
>>> + writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i
>>> * 8);
>>> +#else
>>> writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
>>> +#endif
I would have been OK if there was one place needed a #ifdef. But 2 is a
bit too much.
Please provide a wrapper writeq_relaxed_non_atomic() for arm64. The
implementation would call writeq(). The same stands for...
>>> }
>>> static int gicv3_enable_redist(void)
>>> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
>>> }
>>> do {
>>> +#ifdef CONFIG_ARM_32
>>> + typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
>>> +#else
>>> typer = readq_relaxed(ptr + GICR_TYPER);
>>> +#endif
... here.
>>> if ( (typer >> 32) == aff )
>>> {
>>> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct
>>> irq_desc *desc, const cpumask_t *mask)
>>> affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>> if ( desc->irq >= NR_GIC_LOCAL_IRQS )
>>> +#ifdef CONFIG_ARM_32
>>> + writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER +
>>> desc->irq * 8));
>>> +#else
>>> writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq *
>>> 8));
>>> +#endif
>>> spin_unlock(&gicv3.lock);
>>> }
>>> diff --git a/xen/arch/arm/include/asm/arm32/io.h
>>> b/xen/arch/arm/include/asm/arm32/io.h
>>> index 73a879e9fb..4ddfbea5c2 100644
>>> --- a/xen/arch/arm/include/asm/arm32/io.h
>>> +++ b/xen/arch/arm/include/asm/arm32/io.h
>>> @@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile void
>>> __iomem *addr)
>>> __raw_readw(c)); __r; })
>>> #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>>> __raw_readl(c)); __r; })
>>> +#define readq_relaxed_non_atomic(c) \
>>> + ({ u64 __r = (((u64)readl_relaxed((c) + 4))
>>> << 32) | \
>>> + readl_relaxed(c); __r; })
>>
>> As Julien pointed out, the expression c will be evaluated twice and if
>> it produces side effects they will be performed twice.
>> To prevent this, you can either assign the expression to a local
>> variable and pass this one to readl_relaxed()
>
> Just to make sure I understand you correctly, you are suggesting this :-
>
> #define readq_relaxed_non_atomic(c) \
>
> ({ void _iomem *__addr = (c); \
>
> u64 __r = (((u64)readl_relaxed(__addr + 4))
> << 32) | \
>
> readl_relaxed(__addr); __r; })
>
> #define writeq_relaxed_non_atomic(v,c) \
>
> (( u64 __v = (v); \
>
> void _iomem *__addr = (c); \
>
> writel_relaxed((u32)__v, __addr); \
>
> writel_relaxed((u32)((__v) >> 32), (__addr +
> 4); })
>
> Is this correct understanding ?
>
>> or use a static inline function instead of a macro, for implementing
>> readq_relaxed_non_atomic().
>> The latter is the MISRA C recommended (not strictly required) approach
>> according to Dir 4.9 "A function should be used in preference to a
>> function-like macro where
>> they are interchangeable".
>
> I have mixed opinion about this.
>
> On one hand, there will be a performance penalty when invoking a
> function (compared to macro).
Most of the compilers are nowadays clever enough to inline small
functions. But we can force the compiler with the attribute always_inline.
>
> On the other hand {readq/writeq}_relaxed_non_atomic() are called during
> init (gicv3 initialization, setting up the interrupt handlers), so the
> impact will not be bad.
>
> I am fine with whatever you and any maintainer suggest.
Project wide, we are trying to use "static inline" whenever it is
possible because it adds a bit more type-safety (the error you made
wouldn't have happened) and the code is clearer (no slash).
So my preference is to use static line.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only
2022-11-11 14:17 ` [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only Ayan Kumar Halder
@ 2022-11-17 13:05 ` Michal Orzel
2022-11-22 20:30 ` Julien Grall
0 siblings, 1 reply; 36+ messages in thread
From: Michal Orzel @ 2022-11-17 13:05 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Sysreg emulation is 64-bit specific, so guard the calls to
> vgic_v3_emulate_sysreg() as well as the function itself with
> "#ifdef CONFIG_ARM_64".
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32
2022-11-11 14:17 ` [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32 Ayan Kumar Halder
@ 2022-11-17 13:11 ` Michal Orzel
2022-11-22 20:34 ` Julien Grall
0 siblings, 1 reply; 36+ messages in thread
From: Michal Orzel @ 2022-11-17 13:11 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> In some situations (e.g. GICR_TYPER), the hypervior may need to emulate
> 64bit registers in AArch32 mode. In such situations, the hypervisor may
> need to read/modify the lower or upper 32 bits of the 64 bit register.
>
> In AArch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit
> registers.
>
> While we could replace 'unsigned long' by 'uint64_t', it is not entirely clear
> whether a 32-bit compiler would not allocate register for the upper 32-bit.
> Therefore fold vreg_reg_* helper in the size specific one and use the
> appropriate type based on the size requested.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
2022-11-11 14:17 ` [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32 Ayan Kumar Halder
@ 2022-11-17 13:39 ` Michal Orzel
2022-11-22 20:31 ` Julien Grall
2022-11-27 13:32 ` Ayan Kumar Halder
0 siblings, 2 replies; 36+ messages in thread
From: Michal Orzel @ 2022-11-17 13:39 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
> Affinity level 3 is not present in AArch32.
> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
> Affinity level 3 is not present in Armv7 (ie arm32).
> Thus, any access to affinity level 3 needs to be guarded within
> "ifdef CONFIG_ARM_64".
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
although, IMO the commit msg does not reflect the change (i.e. you do nothing
related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
Also, I'm not sure why you used VMPIDR and not MPIDR.
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32
2022-11-11 14:17 ` [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32 Ayan Kumar Halder
@ 2022-11-17 13:45 ` Michal Orzel
2022-11-22 20:37 ` Julien Grall
1 sibling, 0 replies; 36+ messages in thread
From: Michal Orzel @ 2022-11-17 13:45 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>
>
> Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
> Aff3 does not exist on AArch32.
> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
> Aff3 does not exist on Armv7 (ie arm32).
>
> Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
s/have been contained within/has been protected with/
> Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have copied it to
s/copied/assigned/
> 'uint64_t vmpidr' to perform the shifts.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32
2022-11-11 14:17 ` [XEN v3 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 " Ayan Kumar Halder
@ 2022-11-18 10:13 ` Michal Orzel
0 siblings, 0 replies; 36+ messages in thread
From: Michal Orzel @ 2022-11-18 10:13 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Refer "Arm IHI 0069H ID020922", 12.4.6, Interrupt Controller List Registers
>
> AArch64 System register ICH_LR<n>_EL2 bits [31:0] are architecturally
> mapped to AArch32 System register ICH_LR<n>[31:0].
> AArch64 System register ICH_LR<n>_EL2 bits [63:32] are architecturally
> mapped to AArch32 System register ICH_LRC<n>[31:0].
>
> Defined ICH_LR<0...15>_EL2 and ICH_LRC<0...15>_EL2 for AArch32.
> For AArch32, the link register is stored as :-
> (((uint64_t) ICH_LRC<0...15>_EL2) << 32) | ICH_LR<0...15>_EL2
>
> Also, ICR_LR macros need to be modified as ULL is 64 bits for AArch32 and
> AArch64.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>
> Changes from :-
> v1 - 1. Moved the coproc register definitions to asm/cpregs.h.
> 2. Use GENMASK(31, 0) to represent 0xFFFFFFFF
> 3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG().
> 4. Multi-line macro definitions should be enclosed within ({ }).
>
> v2 - 1. Use WRITE_SYSREG_LR(V, R) to make it consistent with before.
> 2. Defined the register alias.
> 3. Style issues.
>
> xen/arch/arm/gic-v3.c | 132 +++++++++++------------
> xen/arch/arm/include/asm/arm32/sysregs.h | 19 ++++
> xen/arch/arm/include/asm/arm64/sysregs.h | 4 +
> xen/arch/arm/include/asm/cpregs.h | 75 +++++++++++++
> xen/arch/arm/include/asm/gic_v3_defs.h | 6 +-
> 5 files changed, 167 insertions(+), 69 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 64a76307dd..4722bb4daf 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -73,37 +73,37 @@ static inline void gicv3_save_lrs(struct vcpu *v)
> switch ( gicv3_info.nr_lrs )
> {
> case 16:
> - v->arch.gic.v3.lr[15] = READ_SYSREG(ICH_LR15_EL2);
> + v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
> case 15:
> - v->arch.gic.v3.lr[14] = READ_SYSREG(ICH_LR14_EL2);
> + v->arch.gic.v3.lr[14] = READ_SYSREG_LR(14);
> case 14:
> - v->arch.gic.v3.lr[13] = READ_SYSREG(ICH_LR13_EL2);
> + v->arch.gic.v3.lr[13] = READ_SYSREG_LR(13);
> case 13:
> - v->arch.gic.v3.lr[12] = READ_SYSREG(ICH_LR12_EL2);
> + v->arch.gic.v3.lr[12] = READ_SYSREG_LR(12);
> case 12:
> - v->arch.gic.v3.lr[11] = READ_SYSREG(ICH_LR11_EL2);
> + v->arch.gic.v3.lr[11] = READ_SYSREG_LR(11);
> case 11:
> - v->arch.gic.v3.lr[10] = READ_SYSREG(ICH_LR10_EL2);
> + v->arch.gic.v3.lr[10] = READ_SYSREG_LR(10);
> case 10:
> - v->arch.gic.v3.lr[9] = READ_SYSREG(ICH_LR9_EL2);
> + v->arch.gic.v3.lr[9] = READ_SYSREG_LR(9);
> case 9:
> - v->arch.gic.v3.lr[8] = READ_SYSREG(ICH_LR8_EL2);
> + v->arch.gic.v3.lr[8] = READ_SYSREG_LR(8);
> case 8:
> - v->arch.gic.v3.lr[7] = READ_SYSREG(ICH_LR7_EL2);
> + v->arch.gic.v3.lr[7] = READ_SYSREG_LR(7);
> case 7:
> - v->arch.gic.v3.lr[6] = READ_SYSREG(ICH_LR6_EL2);
> + v->arch.gic.v3.lr[6] = READ_SYSREG_LR(6);
> case 6:
> - v->arch.gic.v3.lr[5] = READ_SYSREG(ICH_LR5_EL2);
> + v->arch.gic.v3.lr[5] = READ_SYSREG_LR(5);
> case 5:
> - v->arch.gic.v3.lr[4] = READ_SYSREG(ICH_LR4_EL2);
> + v->arch.gic.v3.lr[4] = READ_SYSREG_LR(4);
> case 4:
> - v->arch.gic.v3.lr[3] = READ_SYSREG(ICH_LR3_EL2);
> + v->arch.gic.v3.lr[3] = READ_SYSREG_LR(3);
> case 3:
> - v->arch.gic.v3.lr[2] = READ_SYSREG(ICH_LR2_EL2);
> + v->arch.gic.v3.lr[2] = READ_SYSREG_LR(2);
> case 2:
> - v->arch.gic.v3.lr[1] = READ_SYSREG(ICH_LR1_EL2);
> + v->arch.gic.v3.lr[1] = READ_SYSREG_LR(1);
> case 1:
> - v->arch.gic.v3.lr[0] = READ_SYSREG(ICH_LR0_EL2);
> + v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
> break;
> default:
> BUG();
> @@ -120,37 +120,37 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
> switch ( gicv3_info.nr_lrs )
> {
> case 16:
> - WRITE_SYSREG(v->arch.gic.v3.lr[15], ICH_LR15_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
> case 15:
> - WRITE_SYSREG(v->arch.gic.v3.lr[14], ICH_LR14_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[14], 14);
> case 14:
> - WRITE_SYSREG(v->arch.gic.v3.lr[13], ICH_LR13_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[13], 13);
> case 13:
> - WRITE_SYSREG(v->arch.gic.v3.lr[12], ICH_LR12_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[12], 12);
> case 12:
> - WRITE_SYSREG(v->arch.gic.v3.lr[11], ICH_LR11_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[11], 11);
> case 11:
> - WRITE_SYSREG(v->arch.gic.v3.lr[10], ICH_LR10_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[10], 10);
> case 10:
> - WRITE_SYSREG(v->arch.gic.v3.lr[9], ICH_LR9_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[9], 9);
> case 9:
> - WRITE_SYSREG(v->arch.gic.v3.lr[8], ICH_LR8_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[8], 8);
> case 8:
> - WRITE_SYSREG(v->arch.gic.v3.lr[7], ICH_LR7_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[7], 7);
> case 7:
> - WRITE_SYSREG(v->arch.gic.v3.lr[6], ICH_LR6_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[6], 6);
> case 6:
> - WRITE_SYSREG(v->arch.gic.v3.lr[5], ICH_LR5_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[5], 5);
> case 5:
> - WRITE_SYSREG(v->arch.gic.v3.lr[4], ICH_LR4_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[4], 4);
> case 4:
> - WRITE_SYSREG(v->arch.gic.v3.lr[3], ICH_LR3_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[3], 3);
> case 3:
> - WRITE_SYSREG(v->arch.gic.v3.lr[2], ICH_LR2_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[2], 2);
> case 2:
> - WRITE_SYSREG(v->arch.gic.v3.lr[1], ICH_LR1_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[1], 1);
> case 1:
> - WRITE_SYSREG(v->arch.gic.v3.lr[0], ICH_LR0_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[0], 0);
> break;
> default:
> BUG();
> @@ -161,22 +161,22 @@ static uint64_t gicv3_ich_read_lr(int lr)
> {
> switch ( lr )
> {
> - case 0: return READ_SYSREG(ICH_LR0_EL2);
> - case 1: return READ_SYSREG(ICH_LR1_EL2);
> - case 2: return READ_SYSREG(ICH_LR2_EL2);
> - case 3: return READ_SYSREG(ICH_LR3_EL2);
> - case 4: return READ_SYSREG(ICH_LR4_EL2);
> - case 5: return READ_SYSREG(ICH_LR5_EL2);
> - case 6: return READ_SYSREG(ICH_LR6_EL2);
> - case 7: return READ_SYSREG(ICH_LR7_EL2);
> - case 8: return READ_SYSREG(ICH_LR8_EL2);
> - case 9: return READ_SYSREG(ICH_LR9_EL2);
> - case 10: return READ_SYSREG(ICH_LR10_EL2);
> - case 11: return READ_SYSREG(ICH_LR11_EL2);
> - case 12: return READ_SYSREG(ICH_LR12_EL2);
> - case 13: return READ_SYSREG(ICH_LR13_EL2);
> - case 14: return READ_SYSREG(ICH_LR14_EL2);
> - case 15: return READ_SYSREG(ICH_LR15_EL2);
> + case 0: return READ_SYSREG_LR(0);
> + case 1: return READ_SYSREG_LR(1);
> + case 2: return READ_SYSREG_LR(2);
> + case 3: return READ_SYSREG_LR(3);
> + case 4: return READ_SYSREG_LR(4);
> + case 5: return READ_SYSREG_LR(5);
> + case 6: return READ_SYSREG_LR(6);
> + case 7: return READ_SYSREG_LR(7);
> + case 8: return READ_SYSREG_LR(8);
> + case 9: return READ_SYSREG_LR(9);
> + case 10: return READ_SYSREG_LR(10);
> + case 11: return READ_SYSREG_LR(11);
> + case 12: return READ_SYSREG_LR(12);
> + case 13: return READ_SYSREG_LR(13);
> + case 14: return READ_SYSREG_LR(14);
> + case 15: return READ_SYSREG_LR(15);
> default:
> BUG();
> }
> @@ -187,52 +187,52 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
> switch ( lr )
> {
> case 0:
> - WRITE_SYSREG(val, ICH_LR0_EL2);
> + WRITE_SYSREG_LR(val, 0);
> break;
> case 1:
> - WRITE_SYSREG(val, ICH_LR1_EL2);
> + WRITE_SYSREG_LR(val, 1);
> break;
> case 2:
> - WRITE_SYSREG(val, ICH_LR2_EL2);
> + WRITE_SYSREG_LR(val, 2);
> break;
> case 3:
> - WRITE_SYSREG(val, ICH_LR3_EL2);
> + WRITE_SYSREG_LR(val, 3);
> break;
> case 4:
> - WRITE_SYSREG(val, ICH_LR4_EL2);
> + WRITE_SYSREG_LR(val, 4);
> break;
> case 5:
> - WRITE_SYSREG(val, ICH_LR5_EL2);
> + WRITE_SYSREG_LR(val, 5);
> break;
> case 6:
> - WRITE_SYSREG(val, ICH_LR6_EL2);
> + WRITE_SYSREG_LR(val, 6);
> break;
> case 7:
> - WRITE_SYSREG(val, ICH_LR7_EL2);
> + WRITE_SYSREG_LR(val, 7);
> break;
> case 8:
> - WRITE_SYSREG(val, ICH_LR8_EL2);
> + WRITE_SYSREG_LR(val, 8);
> break;
> case 9:
> - WRITE_SYSREG(val, ICH_LR9_EL2);
> + WRITE_SYSREG_LR(val, 9);
> break;
> case 10:
> - WRITE_SYSREG(val, ICH_LR10_EL2);
> + WRITE_SYSREG_LR(val, 10);
> break;
> case 11:
> - WRITE_SYSREG(val, ICH_LR11_EL2);
> + WRITE_SYSREG_LR(val, 11);
> break;
> case 12:
> - WRITE_SYSREG(val, ICH_LR12_EL2);
> + WRITE_SYSREG_LR(val, 12);
> break;
> case 13:
> - WRITE_SYSREG(val, ICH_LR13_EL2);
> + WRITE_SYSREG_LR(val, 13);
> break;
> case 14:
> - WRITE_SYSREG(val, ICH_LR14_EL2);
> + WRITE_SYSREG_LR(val, 14);
> break;
> case 15:
> - WRITE_SYSREG(val, ICH_LR15_EL2);
> + WRITE_SYSREG_LR(val, 15);
> break;
> default:
> return;
> @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
> if ( v == current )
> {
> for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> - printk(" HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
> + printk(" HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
> }
> else
> {
> for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> - printk(" VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
> + printk(" VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
> }
> }
>
> diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h
> index 6841d5de43..31ad7eaefb 100644
> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> @@ -62,6 +62,25 @@
> #define READ_SYSREG(R...) READ_SYSREG32(R)
> #define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R)
>
> +/* Wrappers for accessing interrupt controller list registers. */
> +#define ICH_LR_REG(index) ICH_LR ## index ## _EL2
> +#define ICH_LRC_REG(index) ICH_LRC ## index ## _EL2
> +
> +#define READ_SYSREG_LR(index) ({ \
> + uint64_t _val; \
> + uint32_t _lrc = READ_CP32(ICH_LRC_REG(index)); \
> + uint32_t _lr = READ_CP32(ICH_LR_REG(index)); \
> + \
> + _val = ((uint64_t) _lrc << 32) | _lr; \
> + _val; \
> +})
> +
> +#define WRITE_SYSREG_LR(V, index) ({ \
Didn't we agree on using lower case version?
This upper case V looks a bit odd here...
> + uint64_t _val = (V); \
> + WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(index)); \
> + WRITE_CP32(_val >> 32, ICH_LRC_REG(index)); \
> +})
> +
> /* MVFR2 is not defined on ARMv7 */
> #define MVFR2_MAYBE_UNDEFINED
>
> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
> index 54670084c3..b72a6ea3b0 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -472,6 +472,10 @@
> #define READ_SYSREG(name) READ_SYSREG64(name)
> #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
>
> +/* Wrappers for accessing interrupt controller list registers. */
> +#define ICH_LR_REG(index) ICH_LR ## index ## _EL2
> +#define WRITE_SYSREG_LR(V, index) WRITE_SYSREG(V, ICH_LR_REG(index))
and here.
> +#define READ_SYSREG_LR(index) READ_SYSREG(ICH_LR_REG(index))
Before you added a new code block there was an empty line here.
Could you please add it?
> #endif /* _ASM_ARM_ARM64_SYSREGS_H */
>
> /*
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index 6daf2b1a30..242dabaea8 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -259,6 +259,49 @@
> #define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
> #define HVBAR p15,4,c12,c0,0 /* Hyp. Vector Base Address Register */
Here the value of HVBAR...
>
> +/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
> +#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
> +#define __LR0(x) ___CP32(p15, 4, c12, c12, x)
> +#define __LR8(x) ___CP32(p15, 4, c12, c13, x)
> +
> +#define ICH_LR0 __LR0(0)
> +#define ICH_LR1 __LR0(1)
> +#define ICH_LR2 __LR0(2)
> +#define ICH_LR3 __LR0(3)
> +#define ICH_LR4 __LR0(4)
> +#define ICH_LR5 __LR0(5)
> +#define ICH_LR6 __LR0(6)
> +#define ICH_LR7 __LR0(7)
> +#define ICH_LR8 __LR8(0)
> +#define ICH_LR9 __LR8(1)
> +#define ICH_LR10 __LR8(2)
> +#define ICH_LR11 __LR8(3)
> +#define ICH_LR12 __LR8(4)
> +#define ICH_LR13 __LR8(5)
> +#define ICH_LR14 __LR8(6)
> +#define ICH_LR15 __LR8(7)
> +
> +/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
> +#define __LRC0(x) ___CP32(p15, 4, c12, c14, x)
> +#define __LRC8(x) ___CP32(p15, 4, c12, c15, x)
> +
> +#define ICH_LRC0 __LRC0(0)
> +#define ICH_LRC1 __LRC0(1)
> +#define ICH_LRC2 __LRC0(2)
> +#define ICH_LRC3 __LRC0(3)
> +#define ICH_LRC4 __LRC0(4)
> +#define ICH_LRC5 __LRC0(5)
> +#define ICH_LRC6 __LRC0(6)
> +#define ICH_LRC7 __LRC0(7)
> +#define ICH_LRC8 __LRC8(0)
> +#define ICH_LRC9 __LRC8(1)
> +#define ICH_LRC10 __LRC8(2)
> +#define ICH_LRC11 __LRC8(3)
> +#define ICH_LRC12 __LRC8(4)
> +#define ICH_LRC13 __LRC8(5)
> +#define ICH_LRC14 __LRC8(6)
> +#define ICH_LRC15 __LRC8(7)
> +
> /* CP15 CR13: */
> #define FCSEIDR p15,0,c13,c0,0 /* FCSE Process ID Register */
is aligned to the value of FCSEIDR.
So I'm not sure why you cannot align your code block to it.
Generally a new code block can have its own alignement, but here you did insert
it into something aligned which no longer is after your change.
> #define CONTEXTIDR p15,0,c13,c0,1 /* Context ID Register */
> @@ -317,6 +360,38 @@
> #define HCR_EL2 HCR
> #define HPFAR_EL2 HPFAR
> #define HSTR_EL2 HSTR
> +#define ICH_LR0_EL2 ICH_LR0
> +#define ICH_LR1_EL2 ICH_LR1
> +#define ICH_LR2_EL2 ICH_LR2
> +#define ICH_LR3_EL2 ICH_LR3
> +#define ICH_LR4_EL2 ICH_LR4
> +#define ICH_LR5_EL2 ICH_LR5
> +#define ICH_LR6_EL2 ICH_LR6
> +#define ICH_LR7_EL2 ICH_LR7
> +#define ICH_LR8_EL2 ICH_LR8
> +#define ICH_LR9_EL2 ICH_LR9
> +#define ICH_LR10_EL2 ICH_LR10
> +#define ICH_LR11_EL2 ICH_LR11
> +#define ICH_LR12_EL2 ICH_LR12
> +#define ICH_LR13_EL2 ICH_LR13
> +#define ICH_LR14_EL2 ICH_LR14
> +#define ICH_LR15_EL2 ICH_LR15
> +#define ICH_LRC0_EL2 ICH_LRC0
> +#define ICH_LRC1_EL2 ICH_LRC1
> +#define ICH_LRC2_EL2 ICH_LRC2
> +#define ICH_LRC3_EL2 ICH_LRC3
> +#define ICH_LRC4_EL2 ICH_LRC4
> +#define ICH_LRC5_EL2 ICH_LRC5
> +#define ICH_LRC6_EL2 ICH_LRC6
> +#define ICH_LRC7_EL2 ICH_LRC7
> +#define ICH_LRC8_EL2 ICH_LRC8
> +#define ICH_LRC9_EL2 ICH_LRC9
> +#define ICH_LRC10_EL2 ICH_LRC10
> +#define ICH_LRC11_EL2 ICH_LRC11
> +#define ICH_LRC12_EL2 ICH_LRC12
> +#define ICH_LRC13_EL2 ICH_LRC13
> +#define ICH_LRC14_EL2 ICH_LRC14
> +#define ICH_LRC15_EL2 ICH_LRC15
> #define ID_AFR0_EL1 ID_AFR0
> #define ID_DFR0_EL1 ID_DFR0
> #define ID_DFR1_EL1 ID_DFR1
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 48a1bc401e..743ebb20fd 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -185,9 +185,9 @@
> #define ICH_LR_HW_SHIFT 61
> #define ICH_LR_GRP_MASK 0x1
> #define ICH_LR_GRP_SHIFT 60
> -#define ICH_LR_MAINTENANCE_IRQ (1UL<<41)
> -#define ICH_LR_GRP1 (1UL<<60)
> -#define ICH_LR_HW (1UL<<61)
> +#define ICH_LR_MAINTENANCE_IRQ (1ULL << 41)
> +#define ICH_LR_GRP1 (1ULL << 60)
> +#define ICH_LR_HW (1ULL << 61)
>
> #define ICH_VTR_NRLRGS 0x3f
> #define ICH_VTR_PRIBITS_MASK 0x7
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32
2022-11-11 14:17 ` [XEN v3 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32 Ayan Kumar Halder
@ 2022-11-18 12:26 ` Michal Orzel
0 siblings, 0 replies; 36+ messages in thread
From: Michal Orzel @ 2022-11-18 12:26 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Adapt save_aprn_regs()/restore_aprn_regs() for AArch32.
>
> For which we have defined the following registers:-
> 1. Interrupt Controller Hyp Active Priorities Group0 Registers 0-3
> 2. Interrupt Controller Hyp Active Priorities Group1 Registers 0-3
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>
> Changes from :-
> v1 - 1. Moved coproc register definition to asm/cpregs.h.
>
> v2 - 1. Defined register alias.
> 2. Style issues.
> 3. Dropped R-b and Ack.
>
> xen/arch/arm/include/asm/cpregs.h | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index 242dabaea8..5331ec3448 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -259,6 +259,26 @@
> #define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
> #define HVBAR p15,4,c12,c0,0 /* Hyp. Vector Base Address Register */
>
> +/*
> + * CP15 CR12: Interrupt Controller Hyp Active Priorities Group 0 Registers,
> + * n = 0 - 3
> + */
> +#define __AP0Rx(x) ___CP32(p15, 4, c12, c8, x)
You are using a macro ___CP32 here but it is defined somewhere down the file.
So I think you need to make a change so that the definition appears before use.
> +#define ICH_AP0R0 __AP0Rx(0)
> +#define ICH_AP0R1 __AP0Rx(1)
> +#define ICH_AP0R2 __AP0Rx(2)
> +#define ICH_AP0R3 __AP0Rx(3)
> +
> +/*
> + * CP15 CR12: Interrupt Controller Hyp Active Priorities Group 1 Registers,
> + * n = 0 - 3
> + */
> +#define __AP1Rx(x) ___CP32(p15, 4, c12, c9, x)
> +#define ICH_AP1R0 __AP1Rx(0)
> +#define ICH_AP1R1 __AP1Rx(1)
> +#define ICH_AP1R2 __AP1Rx(2)
> +#define ICH_AP1R3 __AP1Rx(3)
> +
> /* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
> #define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
> #define __LR0(x) ___CP32(p15, 4, c12, c12, x)
> @@ -360,6 +380,14 @@
> #define HCR_EL2 HCR
> #define HPFAR_EL2 HPFAR
> #define HSTR_EL2 HSTR
> +#define ICH_AP0R0_EL2 ICH_AP0R0
> +#define ICH_AP0R1_EL2 ICH_AP0R1
> +#define ICH_AP0R2_EL2 ICH_AP0R2
> +#define ICH_AP0R3_EL2 ICH_AP0R3
> +#define ICH_AP1R0_EL2 ICH_AP1R0
> +#define ICH_AP1R1_EL2 ICH_AP1R1
> +#define ICH_AP1R2_EL2 ICH_AP1R2
> +#define ICH_AP1R3_EL2 ICH_AP1R3
> #define ICH_LR0_EL2 ICH_LR0
> #define ICH_LR1_EL2 ICH_LR1
> #define ICH_LR2_EL2 ICH_LR2
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 09/12] xen/Arm: GICv3: Define remaining GIC registers for AArch32
2022-11-11 14:17 ` [XEN v3 09/12] xen/Arm: GICv3: Define remaining GIC registers " Ayan Kumar Halder
@ 2022-11-18 12:43 ` Michal Orzel
0 siblings, 0 replies; 36+ messages in thread
From: Michal Orzel @ 2022-11-18 12:43 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Define missing assembly aliases for GIC registers on arm32, taking the ones
> defined already for arm64 as a base. Aliases are defined according to the
> GIC Architecture Specification ARM IHI 0069H.
>
> Defined the following registers:-
> 1. Interrupt Controller Interrupt Priority Mask Register
> 2. Interrupt Controller System Register Enable register
> 3. Interrupt Controller Deactivate Interrupt Register
> 4. Interrupt Controller End Of Interrupt Register 1
> 5. Interrupt Controller Interrupt Acknowledge Register 1
> 6. Interrupt Controller Binary Point Register 1
> 7. Interrupt Controller Control Register
> 8. Interrupt Controller Interrupt Group 1 Enable register
> 9. Interrupt Controller Maintenance Interrupt State Register
> 10. Interrupt Controller End of Interrupt Status Register
> 11. Interrupt Controller Empty List Register Status Register
> 12. Interrupt Controller Virtual Machine Control Register
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>
> Changes from :-
> v1 - 1. Moved coproc regs definition to asm/cpregs.h
>
> v2 - 1. Defined register alias.
> 2. Style issues.
> 3. Defined ELSR, MISR, EISR to make it consistent with AArch64.
>
> xen/arch/arm/include/asm/cpregs.h | 32 +++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index 5331ec3448..0fc606fe99 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -161,6 +161,7 @@
> #define DACR p15,0,c3,c0,0 /* Domain Access Control Register */
>
> /* CP15 CR4: */
> +#define ICC_PMR p15,0,c4,c6,0 /* Interrupt Priority Mask Register */
>
> /* CP15 CR5: Fault Status Registers */
> #define DFSR p15,0,c5,c0,0 /* Data Fault Status Register */
> @@ -254,6 +255,8 @@
>
> /* CP15 CR12: */
> #define ICC_SGI1R p15,0,c12 /* Interrupt Controller SGI Group 1 */
> +#define ICC_DIR p15,0,c12,c11,1 /* Interrupt Controller Deactivate Interrupt Register */
> +#define ICC_SRE_L1 p15,0,c12,c12,5 /* Interrupt Controller System Register Enable register */
There is no such register. It should be ICC_SRE.
Also it looks like these are not placed in the correct order. ICC_SRE with its assembly alias should be placed...
> #define ICC_ASGI1R p15,1,c12 /* Interrupt Controller Alias SGI Group 1 Register */
> #define ICC_SGI0R p15,2,c12 /* Interrupt Controller SGI Group 0 */
> #define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
> @@ -279,6 +282,19 @@
> #define ICH_AP1R2 __AP1Rx(2)
> #define ICH_AP1R3 __AP1Rx(3)
>
> +#define ICC_IAR1 p15,0,c12,c12,0 /* Interrupt Controller Interrupt Acknowledge Register 1 */
> +#define ICC_EOIR1 p15,0,c12,c12,1 /* Interrupt Controller End Of Interrupt Register 1 */
> +#define ICC_BPR1 p15,0,c12,c12,3 /* Interrupt Controller Binary Point Register 1 */
> +#define ICC_CTLR p15,0,c12,c12,4 /* Interrupt Controller Control Register */
here...
> +#define ICC_IGRPEN1 p15,0,c12,c12,7 /* Interrupt Controller Interrupt Group 1 Enable register */
> +#define ICC_SRE p15,4,c12,c9,5 /* Interrupt Controller Hyp System Register Enable register */
This one should be ICC_HSRE.
> +#define ICH_HCR p15,4,c12,c11,0 /* Interrupt Controller Hyp Control Register */
> +#define ICH_VTR p15,4,c12,c11,1 /* Interrupt Controller VGIC Type Register */
> +#define ICH_MISR p15,4,c12,c11,2 /* Interrupt Controller Maintenance Interrupt State Register */
> +#define ICH_EISR p15,4,c12,c11,3 /* Interrupt Controller End of Interrupt Status Register */
> +#define ICH_ELRSR p15,4,c12,c11,5 /* Interrupt Controller Empty List Register Status Register */
> +#define ICH_VMCR p15,4,c12,c11,7 /* Interrupt Controller Virtual Machine Control Register */
> +
> /* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
> #define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
> #define __LR0(x) ___CP32(p15, 4, c12, c12, x)
> @@ -380,6 +396,15 @@
> #define HCR_EL2 HCR
> #define HPFAR_EL2 HPFAR
> #define HSTR_EL2 HSTR
> +#define ICC_BPR1_EL1 ICC_BPR1
> +#define ICC_CTLR_EL1 ICC_CTLR
> +#define ICC_DIR_EL1 ICC_DIR
> +#define ICC_EOIR1_EL1 ICC_EOIR1
> +#define ICC_IGRPEN1_EL1 ICC_IGRPEN1
> +#define ICC_PMR_EL1 ICC_PMR
> +#define ICC_SGI1R_EL1 ICC_SGI1R
> +#define ICC_SRE_EL1 ICC_SRE_L1
This should be:
#define ICC_SRE_EL1 ICC_SRE
> +#define ICC_SRE_EL2 ICC_SRE
This should be:
#define ICC_SRE_EL2 ICC_HSRE
> #define ICH_AP0R0_EL2 ICH_AP0R0
> #define ICH_AP0R1_EL2 ICH_AP0R1
> #define ICH_AP0R2_EL2 ICH_AP0R2
> @@ -388,6 +413,10 @@
> #define ICH_AP1R1_EL2 ICH_AP1R1
> #define ICH_AP1R2_EL2 ICH_AP1R2
> #define ICH_AP1R3_EL2 ICH_AP1R3
> +#define ICH_EISR_EL2 ICH_EISR
> +#define ICH_ELRSR_EL2 ICH_ELRSR
> +#define ICH_HCR_EL2 ICH_HCR
> +#define ICC_IAR1_EL1 ICC_IAR1
> #define ICH_LR0_EL2 ICH_LR0
> #define ICH_LR1_EL2 ICH_LR1
> #define ICH_LR2_EL2 ICH_LR2
> @@ -420,6 +449,9 @@
> #define ICH_LRC13_EL2 ICH_LRC13
> #define ICH_LRC14_EL2 ICH_LRC14
> #define ICH_LRC15_EL2 ICH_LRC15
> +#define ICH_MISR_EL2 ICH_MISR
> +#define ICH_VMCR_EL2 ICH_VMCR
> +#define ICH_VTR_EL2 ICH_VTR
> #define ID_AFR0_EL1 ID_AFR0
> #define ID_DFR0_EL1 ID_DFR0
> #define ID_DFR1_EL1 ID_DFR1
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits
2022-11-11 14:17 ` [XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits Ayan Kumar Halder
@ 2022-11-18 12:58 ` Michal Orzel
0 siblings, 0 replies; 36+ messages in thread
From: Michal Orzel @ 2022-11-18 12:58 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> "unsigned long long" is defined as 64 bits on AArch64 and AArch32
> Thus, one should this instead of "unsigned long" which is 32 bits
> on AArch32.
>
> Also use 'PRIx64' instead of 'lx' or 'llx' to print uint64_t.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>
> Changed from :-
> v1 - 1. Replace PRIu64 with PRIx64 so that the values are printed in hex as
> desired.
> 2. Use ULL in GITS_BASER_RO_MASK as MMIO registers are always unsigned.
>
> v2 - 1. Removed changes to ITS and LPI as they are not supported for AArch32.
>
> xen/arch/arm/gic-v3.c | 4 ++--
> xen/arch/arm/include/asm/gic_v3_defs.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 4722bb4daf..6457e7033c 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
> if ( v == current )
> {
> for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> - printk(" HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
> + printk(" HW_LR[%d]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
> }
> else
> {
> for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> - printk(" VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
> + printk(" VCPU_LR[%d]=%" PRIx64 "\n", i, v->arch.gic.v3.lr[i]);
You changed these specifiers to be llx in patch no.7 so such a change (using PRIx64) should
be done in that patch. Generally there is no need for a patch to fix something that you
introduced earlier in the series. It should be done in one step. Having said that...
> }
> }
>
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 743ebb20fd..227533868f 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -195,7 +195,7 @@
>
> #define ICH_SGI_IRQMODE_SHIFT 40
> #define ICH_SGI_IRQMODE_MASK 0x1
> -#define ICH_SGI_TARGET_OTHERS 1UL
> +#define ICH_SGI_TARGET_OTHERS 1ULL
> #define ICH_SGI_TARGET_LIST 0
> #define ICH_SGI_IRQ_SHIFT 24
> #define ICH_SGI_IRQ_MASK 0xf
adding a patch for just this macro is not very useful and you could take the opportunity
to modify it in any of your patches.
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only
2022-11-17 13:05 ` Michal Orzel
@ 2022-11-22 20:30 ` Julien Grall
0 siblings, 0 replies; 36+ messages in thread
From: Julien Grall @ 2022-11-22 20:30 UTC (permalink / raw)
To: Michal Orzel, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
jgrall, burzalodowa
On 17/11/2022 13:05, Michal Orzel wrote:
> Hi Ayan,
>
> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>> Sysreg emulation is 64-bit specific, so guard the calls to
>> vgic_v3_emulate_sysreg() as well as the function itself with
>> "#ifdef CONFIG_ARM_64".
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <julien@xen.org>
Cheers,
>
> ~Michal
--
Julien Grall
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
2022-11-17 13:39 ` Michal Orzel
@ 2022-11-22 20:31 ` Julien Grall
2022-11-23 9:35 ` Michal Orzel
2022-11-27 13:32 ` Ayan Kumar Halder
1 sibling, 1 reply; 36+ messages in thread
From: Julien Grall @ 2022-11-22 20:31 UTC (permalink / raw)
To: Michal Orzel, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
jgrall, burzalodowa
On 17/11/2022 13:39, Michal Orzel wrote:
> Hi Ayan,
>
> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>> Affinity level 3 is not present in AArch32.
>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>> Affinity level 3 is not present in Armv7 (ie arm32).
>> Thus, any access to affinity level 3 needs to be guarded within
>> "ifdef CONFIG_ARM_64".
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>
> although, IMO the commit msg does not reflect the change (i.e. you do nothing
> related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
> Also, I'm not sure why you used VMPIDR and not MPIDR.
+1. Can one of you propose an improved commit message/title?
I would be happy to update the patch on commit if there are nothing else
to update in this series.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32
2022-11-17 13:11 ` Michal Orzel
@ 2022-11-22 20:34 ` Julien Grall
0 siblings, 0 replies; 36+ messages in thread
From: Julien Grall @ 2022-11-22 20:34 UTC (permalink / raw)
To: Michal Orzel, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
jgrall, burzalodowa
Hi Michal,
On 17/11/2022 13:11, Michal Orzel wrote:
> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>> In some situations (e.g. GICR_TYPER), the hypervior may need to emulate
>> 64bit registers in AArch32 mode. In such situations, the hypervisor may
>> need to read/modify the lower or upper 32 bits of the 64 bit register.
>>
>> In AArch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit
>> registers.
>>
>> While we could replace 'unsigned long' by 'uint64_t', it is not entirely clear
>> whether a 32-bit compiler would not allocate register for the upper 32-bit.
>> Therefore fold vreg_reg_* helper in the size specific one and use the
>> appropriate type based on the size requested.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <julien@xen.org>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32
2022-11-11 14:17 ` [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32 Ayan Kumar Halder
2022-11-17 13:45 ` Michal Orzel
@ 2022-11-22 20:37 ` Julien Grall
2022-11-28 9:56 ` Ayan Kumar Halder
1 sibling, 1 reply; 36+ messages in thread
From: Julien Grall @ 2022-11-22 20:37 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
michal.orzel, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 14:17, Ayan Kumar Halder wrote:
> Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
> Aff3 does not exist on AArch32.
> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
> Aff3 does not exist on Armv7 (ie arm32).
>
> Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
> Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have copied it to
> 'uint64_t vmpidr' to perform the shifts.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>
> Changes from :-
> v1 - Assigned v->arch.vmpidr to "uint64_t vmpdir". Then, we can use
> MPIDR_AFFINITY_LEVEL macros to extract the affinity value.
>
> v2 - 1. "MPIDR_AFFINITY_LEVEL(vmpidr, 3)" is contained within
> "#ifdef CONFIG_ARM_64".
> 2. Updated commit message.
>
> xen/arch/arm/vgic-v3.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 3f4509dcd3..a7a935ff57 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -191,12 +191,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
> case VREG64(GICR_TYPER):
> {
> uint64_t typer, aff;
> + uint64_t vmpidr = v->arch.vmpidr;
The type-widening here deserve an in-code comment. Otherwise, it would
be easier for someone to decide to open-code v->arch.vmpidr again.
>
> if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> - aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
> + aff = (
> +#ifdef CONFIG_ARM_64
> + MPIDR_AFFINITY_LEVEL(vmpidr, 3) << 56 |
> +#endif
> + MPIDR_AFFINITY_LEVEL(vmpidr, 2) << 48 |
> + MPIDR_AFFINITY_LEVEL(vmpidr, 1) << 40 |
> + MPIDR_AFFINITY_LEVEL(vmpidr, 0) << 32);
> typer = aff;
> /* We use the VCPU ID as the redistributor ID in bits[23:8] */
> typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
2022-11-22 20:31 ` Julien Grall
@ 2022-11-23 9:35 ` Michal Orzel
2022-11-24 18:50 ` Julien Grall
0 siblings, 1 reply; 36+ messages in thread
From: Michal Orzel @ 2022-11-23 9:35 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
jgrall, burzalodowa
Hi Julien,
On 22/11/2022 21:31, Julien Grall wrote:
>
>
> On 17/11/2022 13:39, Michal Orzel wrote:
>> Hi Ayan,
>>
>> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>>> Affinity level 3 is not present in AArch32.
>>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>>> Affinity level 3 is not present in Armv7 (ie arm32).
>>> Thus, any access to affinity level 3 needs to be guarded within
>>> "ifdef CONFIG_ARM_64".
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> although, IMO the commit msg does not reflect the change (i.e. you do nothing
>> related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
>> Also, I'm not sure why you used VMPIDR and not MPIDR.
>
> +1. Can one of you propose an improved commit message/title?
Title: "Do not calculate affinity level 3 for AArch32"
I think the commit message can stay as it is.
>
> I would be happy to update the patch on commit if there are nothing else
> to update in this series.
The series requires re-spinning anyway so no need to update the patch on commit.
>
> Cheers,
>
> --
> Julien Grall
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32
2022-11-11 14:17 ` [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32 Ayan Kumar Halder
@ 2022-11-23 9:51 ` Michal Orzel
2022-11-25 8:42 ` Julien Grall
1 sibling, 0 replies; 36+ messages in thread
From: Michal Orzel @ 2022-11-23 9:51 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
Hi Ayan,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> One can now use GICv3 on AArch32 systems. However, ITS is not supported.
> The reason being currently we are trying to validate GICv3 on an AArch32_v8R
> system. Refer ARM DDI 0568A.c ID110520, B1.3.1,
> "A Generic Interrupt Controller (GIC) implemented with an Armv8-R PE must not
> implement LPI support."
>
> By default GICv3 is disabled on AArch32 and enabled on AArch64.
>
> Updated SUPPORT.md to state that GICv3 on Arm32 is not security supported.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
with one remark...
> ---
>
> Changed from :-
> v1 - 1. Remove "ARM_64 || ARM_32" as it is always true.
> 2. Updated SUPPORT.md.
>
> v2 - 1. GICv3 is enabled by default only on ARM_64.
> 2. Updated SUPPORT.md.
>
> SUPPORT.md | 7 +++++++
> xen/arch/arm/Kconfig | 9 +++++----
> xen/arch/arm/include/asm/cpufeature.h | 1 +
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/SUPPORT.md b/SUPPORT.md
> index ab71464cf6..3f16d83191 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -82,6 +82,13 @@ Extension to the GICv3 interrupt controller to support MSI.
>
> Status: Experimental
>
> +### ARM/GICv3
... For me this section should go before the section ARM/GICv3 ITS.
The reason being ITS is an extension and your section being generic
should be listed before listing extension.
~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
2022-11-23 9:35 ` Michal Orzel
@ 2022-11-24 18:50 ` Julien Grall
0 siblings, 0 replies; 36+ messages in thread
From: Julien Grall @ 2022-11-24 18:50 UTC (permalink / raw)
To: Michal Orzel, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
jgrall, burzalodowa
On 23/11/2022 10:35, Michal Orzel wrote:
> Hi Julien,
>
> On 22/11/2022 21:31, Julien Grall wrote:
>>
>>
>> On 17/11/2022 13:39, Michal Orzel wrote:
>>> Hi Ayan,
>>>
>>> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>>>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>>>> Affinity level 3 is not present in AArch32.
>>>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>>>> Affinity level 3 is not present in Armv7 (ie arm32).
>>>> Thus, any access to affinity level 3 needs to be guarded within
>>>> "ifdef CONFIG_ARM_64".
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>
>>> although, IMO the commit msg does not reflect the change (i.e. you do nothing
>>> related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
>>> Also, I'm not sure why you used VMPIDR and not MPIDR.
>>
>> +1. Can one of you propose an improved commit message/title?
> Title: "Do not calculate affinity level 3 for AArch32"
Sounds good to me.
>
> I think the commit message can stay as it is.
>>
>> I would be happy to update the patch on commit if there are nothing else
>> to update in this series.
> The series requires re-spinning anyway so no need to update the patch on commit.
Fine with that.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32
2022-11-11 14:17 ` [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32 Ayan Kumar Halder
2022-11-23 9:51 ` Michal Orzel
@ 2022-11-25 8:42 ` Julien Grall
1 sibling, 0 replies; 36+ messages in thread
From: Julien Grall @ 2022-11-25 8:42 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
michal.orzel, jgrall, burzalodowa
Hi,
On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> One can now use GICv3 on AArch32 systems. However, ITS is not supported.
> The reason being currently we are trying to validate GICv3 on an AArch32_v8R
> system. Refer ARM DDI 0568A.c ID110520, B1.3.1,
> "A Generic Interrupt Controller (GIC) implemented with an Armv8-R PE must not
> implement LPI support."
>
> By default GICv3 is disabled on AArch32 and enabled on AArch64.
>
> Updated SUPPORT.md to state that GICv3 on Arm32 is not security supported.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>
> Changed from :-
> v1 - 1. Remove "ARM_64 || ARM_32" as it is always true.
> 2. Updated SUPPORT.md.
>
> v2 - 1. GICv3 is enabled by default only on ARM_64.
> 2. Updated SUPPORT.md.
>
> SUPPORT.md | 7 +++++++
> xen/arch/arm/Kconfig | 9 +++++----
> xen/arch/arm/include/asm/cpufeature.h | 1 +
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/SUPPORT.md b/SUPPORT.md
> index ab71464cf6..3f16d83191 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -82,6 +82,13 @@ Extension to the GICv3 interrupt controller to support MSI.
>
> Status: Experimental
>
> +### ARM/GICv3
> +
> +GICv3 is an interrupt controller specification designed by Arm.
> +
> + Status, Arm64: Security supported
> + Status, Arm32: Supported, not security supported
> +
> ## Guest Type
>
> ### x86/PV
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 1fe5faf847..b90930955b 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -9,6 +9,7 @@ config ARM_64
> select 64BIT
> select ARM_EFI
> select HAS_FAST_MULTIPLY
> + select GICV3
AFAIU "select" will force the GICV3 to be enabled. IOW, it would not be
possible to disable it for Arm64 anymore. You want to remove the
"select" and ...
>
> config ARM
> def_bool y
> @@ -41,16 +42,16 @@ config ARM_EFI
>
> config GICV3
> bool "GICv3 driver"
> - depends on ARM_64 && !NEW_VGIC
> - default y
> + depends on !NEW_VGIC
> + default n
... replace the "default n" with:
default n if ARM_32
default y if ARM_64
> ---help---
>
> Driver for the ARM Generic Interrupt Controller v3.
> - If unsure, say Y
> + If unsure, say N
That's technically incorrect. For Arm64, we still want the user to
select it by default. So I think we need to differentiate 32-bit vs
64-bit for the "If unsure" part.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
2022-11-17 13:39 ` Michal Orzel
2022-11-22 20:31 ` Julien Grall
@ 2022-11-27 13:32 ` Ayan Kumar Halder
2022-11-28 9:21 ` Julien Grall
1 sibling, 1 reply; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-27 13:32 UTC (permalink / raw)
To: Michal Orzel, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jgrall, burzalodowa
On 17/11/2022 13:39, Michal Orzel wrote:
> Hi Ayan,
Hi Michal,
>
> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>> Affinity level 3 is not present in AArch32.
>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>> Affinity level 3 is not present in Armv7 (ie arm32).
>> Thus, any access to affinity level 3 needs to be guarded within
>> "ifdef CONFIG_ARM_64".
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>
> although, IMO the commit msg does not reflect the change (i.e. you do nothing
> related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
> Also, I'm not sure why you used VMPIDR and not MPIDR.
Actually MPIDR in EL2 is known as VMPIDR. So I used this name.
ReferARM DDI 0487I.aID081822, D17-6354,D17.2.152, VMPIDR_EL2,
Virtualization Multiprocessor ID Register
- Ayan
>
> ~Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
2022-11-27 13:32 ` Ayan Kumar Halder
@ 2022-11-28 9:21 ` Julien Grall
0 siblings, 0 replies; 36+ messages in thread
From: Julien Grall @ 2022-11-28 9:21 UTC (permalink / raw)
To: Ayan Kumar Halder, Michal Orzel, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
jgrall, burzalodowa
On 27/11/2022 14:32, Ayan Kumar Halder wrote:
>
> On 17/11/2022 13:39, Michal Orzel wrote:
>> Hi Ayan,
> Hi Michal,
>>
>> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>>> Affinity level 3 is not present in AArch32.
>>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>>> Affinity level 3 is not present in Armv7 (ie arm32).
>>> Thus, any access to affinity level 3 needs to be guarded within
>>> "ifdef CONFIG_ARM_64".
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> although, IMO the commit msg does not reflect the change (i.e. you do
>> nothing
>> related to accessing MPIDR, but instead you are just not taking the
>> Aff3 into account for AArch32).
>> Also, I'm not sure why you used VMPIDR and not MPIDR.
>
> Actually MPIDR in EL2 is known as VMPIDR. So I used this name.
Quoting the Arm Arm: The VMPIDR, holds the value of the Virtualization
Multiprocessor ID. This is the value returned by Non-secure EL1 reads of
MPIDR.
The code you are touching is looking a the pCPU information. Therefore,
the correct name of the register is MPIDR.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32
2022-11-22 20:37 ` Julien Grall
@ 2022-11-28 9:56 ` Ayan Kumar Halder
0 siblings, 0 replies; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-28 9:56 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
michal.orzel, jgrall, burzalodowa
On 22/11/2022 20:37, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
I need a clarification.
>
> On 11/11/2022 14:17, Ayan Kumar Halder wrote:
>> Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
>> Aff3 does not exist on AArch32.
>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
>> Aff3 does not exist on Armv7 (ie arm32).
>>
>> Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
>> Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have
>> copied it to
>> 'uint64_t vmpidr' to perform the shifts.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>
>> Changes from :-
>> v1 - Assigned v->arch.vmpidr to "uint64_t vmpdir". Then, we can use
>> MPIDR_AFFINITY_LEVEL macros to extract the affinity value.
>>
>> v2 - 1. "MPIDR_AFFINITY_LEVEL(vmpidr, 3)" is contained within
>> "#ifdef CONFIG_ARM_64".
>> 2. Updated commit message.
>>
>> xen/arch/arm/vgic-v3.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 3f4509dcd3..a7a935ff57 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -191,12 +191,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct
>> vcpu *v, mmio_info_t *info,
>> case VREG64(GICR_TYPER):
>> {
>> uint64_t typer, aff;
>> + uint64_t vmpidr = v->arch.vmpidr;
>
> The type-widening here deserve an in-code comment. Otherwise, it would
> be easier for someone to decide to open-code v->arch.vmpidr again.
Does this comment look fine ?
/*
* This is to enable shifts greater than 32 bits which would have
* otherwise caused overflow (as v->arch.vmpidr is 32 bit on
AArch32).
*/
uint64_t vmpidr = v->arch.vmpidr;
- Ayan
>
>> if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
>> - aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
>> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
>> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
>> - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
>> + aff = (
>> +#ifdef CONFIG_ARM_64
>> + MPIDR_AFFINITY_LEVEL(vmpidr, 3) << 56 |
>> +#endif
>> + MPIDR_AFFINITY_LEVEL(vmpidr, 2) << 48 |
>> + MPIDR_AFFINITY_LEVEL(vmpidr, 1) << 40 |
>> + MPIDR_AFFINITY_LEVEL(vmpidr, 0) << 32);
>> typer = aff;
>> /* We use the VCPU ID as the redistributor ID in bits[23:8] */
>> typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
>
> Cheers,
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit
2022-11-11 17:53 ` Julien Grall
@ 2022-11-28 12:32 ` Ayan Kumar Halder
0 siblings, 0 replies; 36+ messages in thread
From: Ayan Kumar Halder @ 2022-11-28 12:32 UTC (permalink / raw)
To: Julien Grall, Xenia Ragiadakou, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
michal.orzel, jgrall
On 11/11/2022 17:53, Julien Grall wrote:
> Hi,
Hi Julien,
I need a clarification.
>
> On 11/11/2022 17:37, Ayan Kumar Halder wrote:
>>
>> On 11/11/2022 16:17, Xenia Ragiadakou wrote:
>>> Hi Ayan,
>> Hi Xenia,
>>>
>>> On 11/11/22 16:17, Ayan Kumar Halder wrote:
>>>> On AArch32, ldrd/strd instructions are not atomic when used to
>>>> access MMIO.
>>>> Furthermore, ldrd/strd instructions are not decoded by Arm when
>>>> running as
>>>> a guest to access emulated MMIO region.
>>>> Thus, we have defined
>>>> readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
>>>> which in turn calls readl_relaxed()/writel_relaxed() for the lower
>>>> and upper
>>>> 32 bits.
>>>> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a
>>>> non atomic
>>>> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>>
>>>> Changes from :-
>>>> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
>>>> 2. No need to use le64_to_cpu() as the returned byte order is
>>>> already in cpu
>>>> endianess.
>>>>
>>>> v2 - 1. Replace {read/write}q_relaxed with
>>>> {read/write}q_relaxed_non_atomic().
>>>>
>>>> xen/arch/arm/gic-v3.c | 12 ++++++++++++
>>>> xen/arch/arm/include/asm/arm32/io.h | 9 +++++++++
>>>> 2 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>>> index 6457e7033c..a5bc549765 100644
>>>> --- a/xen/arch/arm/gic-v3.c
>>>> +++ b/xen/arch/arm/gic-v3.c
>>>> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
>>>> affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>>> for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
>>>> +#ifdef CONFIG_ARM_32
>>>> + writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER +
>>>> i * 8);
>>>> +#else
>>>> writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
>>>> +#endif
>
> I would have been OK if there was one place needed a #ifdef. But 2 is
> a bit too much.
>
> Please provide a wrapper writeq_relaxed_non_atomic() for arm64. The
> implementation would call writeq(). The same stands for...
For arm64, why shouldn't we invoke {read/write}q_relaxed() for
{read/write}q_relaxed_non_atomic() ?
As I understand, this will be less expensive than invoking
readq()/writeq() (as it will introduce memory barriers).
Also, the original code was invoking {read/write}q_relaxed() for arm64.
Please let me know if I am missing something ?
- Ayan
>
>>>> }
>>>> static int gicv3_enable_redist(void)
>>>> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
>>>> }
>>>> do {
>>>> +#ifdef CONFIG_ARM_32
>>>> + typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
>>>> +#else
>>>> typer = readq_relaxed(ptr + GICR_TYPER);
>>>> +#endif
>
> ... here.
>
>>>> if ( (typer >> 32) == aff )
>>>> {
>>>> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct
>>>> irq_desc *desc, const cpumask_t *mask)
>>>> affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>>> if ( desc->irq >= NR_GIC_LOCAL_IRQS )
>>>> +#ifdef CONFIG_ARM_32
>>>> + writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER +
>>>> desc->irq * 8));
>>>> +#else
>>>> writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq
>>>> * 8));
>>>> +#endif
>>>> spin_unlock(&gicv3.lock);
>>>> }
>>>> diff --git a/xen/arch/arm/include/asm/arm32/io.h
>>>> b/xen/arch/arm/include/asm/arm32/io.h
>>>> index 73a879e9fb..4ddfbea5c2 100644
>>>> --- a/xen/arch/arm/include/asm/arm32/io.h
>>>> +++ b/xen/arch/arm/include/asm/arm32/io.h
>>>> @@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile
>>>> void __iomem *addr)
>>>> __raw_readw(c)); __r; })
>>>> #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>>>> __raw_readl(c)); __r; })
>>>> +#define readq_relaxed_non_atomic(c) \
>>>> + ({ u64 __r = (((u64)readl_relaxed((c) +
>>>> 4)) << 32) | \
>>>> + readl_relaxed(c); __r; })
>>>
>>> As Julien pointed out, the expression c will be evaluated twice and
>>> if it produces side effects they will be performed twice.
>>> To prevent this, you can either assign the expression to a local
>>> variable and pass this one to readl_relaxed()
>>
>> Just to make sure I understand you correctly, you are suggesting this :-
>>
>> #define readq_relaxed_non_atomic(c) \
>>
>> ({ void _iomem *__addr = (c); \
>>
>> u64 __r = (((u64)readl_relaxed(__addr +
>> 4)) << 32) | \
>>
>> readl_relaxed(__addr); __r; })
>>
>> #define writeq_relaxed_non_atomic(v,c) \
>>
>> (( u64 __v = (v); \
>>
>> void _iomem *__addr = (c); \
>>
>> writel_relaxed((u32)__v, __addr); \
>>
>> writel_relaxed((u32)((__v) >> 32), (__addr
>> + 4); })
>
>
>>
>> Is this correct understanding ?
>>
>>> or use a static inline function instead of a macro, for implementing
>>> readq_relaxed_non_atomic().
>>> The latter is the MISRA C recommended (not strictly required)
>>> approach according to Dir 4.9 "A function should be used in
>>> preference to a function-like macro where
>>> they are interchangeable".
>>
>> I have mixed opinion about this.
>>
>> On one hand, there will be a performance penalty when invoking a
>> function (compared to macro).
>
> Most of the compilers are nowadays clever enough to inline small
> functions. But we can force the compiler with the attribute
> always_inline.
>
>>
>> On the other hand {readq/writeq}_relaxed_non_atomic() are called
>> during init (gicv3 initialization, setting up the interrupt
>> handlers), so the impact will not be bad.
>>
>> I am fine with whatever you and any maintainer suggest.
>
> Project wide, we are trying to use "static inline" whenever it is
> possible because it adds a bit more type-safety (the error you made
> wouldn't have happened) and the code is clearer (no slash).
>
> So my preference is to use static line.
>
> Cheers,
>
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2022-11-28 12:33 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-11 14:17 [XEN v3 00/12] Arm: Enable GICv3 for AArch32 Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only Ayan Kumar Halder
2022-11-17 13:05 ` Michal Orzel
2022-11-22 20:30 ` Julien Grall
2022-11-11 14:17 ` [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32 Ayan Kumar Halder
2022-11-17 13:39 ` Michal Orzel
2022-11-22 20:31 ` Julien Grall
2022-11-23 9:35 ` Michal Orzel
2022-11-24 18:50 ` Julien Grall
2022-11-27 13:32 ` Ayan Kumar Halder
2022-11-28 9:21 ` Julien Grall
2022-11-11 14:17 ` [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32 Ayan Kumar Halder
2022-11-17 13:11 ` Michal Orzel
2022-11-22 20:34 ` Julien Grall
2022-11-11 14:17 ` [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32 Ayan Kumar Halder
2022-11-17 13:45 ` Michal Orzel
2022-11-22 20:37 ` Julien Grall
2022-11-28 9:56 ` Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 05/12] xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 06/12] xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32 Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 " Ayan Kumar Halder
2022-11-18 10:13 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32 Ayan Kumar Halder
2022-11-18 12:26 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 09/12] xen/Arm: GICv3: Define remaining GIC registers " Ayan Kumar Halder
2022-11-18 12:43 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits Ayan Kumar Halder
2022-11-18 12:58 ` Michal Orzel
2022-11-11 14:17 ` [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit Ayan Kumar Halder
2022-11-11 16:17 ` Xenia Ragiadakou
2022-11-11 17:37 ` Ayan Kumar Halder
2022-11-11 17:53 ` Julien Grall
2022-11-28 12:32 ` Ayan Kumar Halder
2022-11-11 14:17 ` [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32 Ayan Kumar Halder
2022-11-23 9:51 ` Michal Orzel
2022-11-25 8:42 ` 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.