On 03/12/2022 18:35, Julien Grall wrote:
Hi,
Hi Julien,
On 29/11/2022 14:33, Michal Orzel wrote:
@@ -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]=%" PRIx64 "\n", i,
gicv3_ich_read_lr(i));
1. We do not usually add spaces between " and PRIx.
I don't have a strong preference on this one.
ok, I will then keep it as it is.
}
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]=%" PRIx64 "\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..22871999af 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..4638999514 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -472,6 +472,11 @@
#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..b85e811f51 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -8,6 +8,8 @@
* support 32-bit guests.
*/
+#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn,
crm, opc2
2. As you are using ___CP32 much later in this file, it could be
moved...
__CP32() is already defined in arm32/sysregs.h which includes
cpregs.h. We should not define __CP32() twice and the only reason the
compiler doesn't complain is because the definition is the same
The definition is different :-
In xen/arch/arm/include/asm/arm32/sysregs.h
"#define __CP32(r, coproc, opc1, crn, crm, opc2) coproc, opc1, r, crn,
crm, opc2" (Note:- it takes 6 arguments)
And what I have defined in xen/arch/arm/include/asm/cpregs.h:-
#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm,
opc2 (It takes 5 arguments, also note it has 3 underscores before CP32).
So one of the two needs to be dropped. Also, I think __CP32(),
__CP64(), CP32() and CP64() should be defined together because they
are all related.
However...
+
#define __HSR_CPREG_c0 0
#define __HSR_CPREG_c1 1
#define __HSR_CPREG_c2 2
@@ -259,6 +261,48 @@
#define VBAR p15,0,c12,c0,0 /* Vector Base Address
Register */
#define HVBAR p15,4,c12,c0,0 /* Hyp. Vector Base
Address Register */
...here, before first use. The remark I gave in v3 was that the
definition should occur before use,
but it does not mean placing the macro at the top of the file.
+/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
+#define __LR0(x) ___CP32(p15, 4, c12, c12, x)
+#define __LR8(x) ___CP32(p15, 4, c12, c13, x)
... I don't understand why you need to use __CP32 here and everywhere
else in this header. In fact I have replaced in my tree all the
__CP32(foo) with foo and it still compiles.
I think you mean ___CP32 here (3 underscores).
So can you explain why they are necessary?
Actually, they are not necessary. I will remove ___CP32 definition in
the v5 patch.
Sorry, I was blindly influenced by the definition of __CP32. :( But,
there it was needed.
Would you also review 8/11, 9/11, 10/11 and 10/11 before I re-spin a new
series ? They have been reviewed by Michal (with some minor comments
which I can address in v5).
- Ayan
Cheers,
