[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 1/7] xen/arm: Add ID registers and complete cpuinfo
Hi Bertrand,
On 10/12/2020 15:14, Bertrand Marquis wrote:
Hi Julien,
On 9 Dec 2020, at 23:03, Julien Grall <julien@xxxxxxx> wrote:
Hi Bertrand,
On 09/12/2020 16:30, Bertrand Marquis wrote:
Add definition and entries in cpuinfo for ID registers introduced in
newer Arm Architecture reference manual:
- ID_PFR2: processor feature register 2
- ID_DFR1: debug feature register 1
- ID_MMFR4 and ID_MMFR5: Memory model feature registers 4 and 5
- ID_ISA6: ISA Feature register 6
Add more bitfield definitions in PFR fields of cpuinfo.
Add MVFR2 register definition for aarch32.
Add mvfr values in cpuinfo.
Add some registers definition for arm64 in sysregs as some are not
always know by compilers.
Initialize the new values added in cpuinfo in identify_cpu during init.
Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
---
Changes in V2:
Fix dbg32 table size and add proper initialisation of the second entry
of the table by reading ID_DFR1 register.
Changes in V3:
Fix typo in commit title
Add MVFR2 definition and handling on aarch32 and remove specific case
for mvfr field in cpuinfo (now the same on arm64 and arm32).
Add MMFR4 definition if not known by the compiler.
---
xen/arch/arm/cpufeature.c | 18 ++++++++++
xen/include/asm-arm/arm64/sysregs.h | 28 +++++++++++++++
xen/include/asm-arm/cpregs.h | 12 +++++++
xen/include/asm-arm/cpufeature.h | 56 ++++++++++++++++++++++++-----
4 files changed, 105 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index 44126dbf07..bc7ee5ac95 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -114,15 +114,20 @@ void identify_cpu(struct cpuinfo_arm *c)
c->mm64.bits[0] = READ_SYSREG64(ID_AA64MMFR0_EL1);
c->mm64.bits[1] = READ_SYSREG64(ID_AA64MMFR1_EL1);
+ c->mm64.bits[2] = READ_SYSREG64(ID_AA64MMFR2_EL1);
c->isa64.bits[0] = READ_SYSREG64(ID_AA64ISAR0_EL1);
c->isa64.bits[1] = READ_SYSREG64(ID_AA64ISAR1_EL1);
+
+ c->zfr64.bits[0] = READ_SYSREG64(ID_AA64ZFR0_EL1);
#endif
c->pfr32.bits[0] = READ_SYSREG32(ID_PFR0_EL1);
c->pfr32.bits[1] = READ_SYSREG32(ID_PFR1_EL1);
+ c->pfr32.bits[2] = READ_SYSREG32(ID_PFR2_EL1);
c->dbg32.bits[0] = READ_SYSREG32(ID_DFR0_EL1);
+ c->dbg32.bits[1] = READ_SYSREG32(ID_DFR1_EL1);
c->aux32.bits[0] = READ_SYSREG32(ID_AFR0_EL1);
@@ -130,6 +135,8 @@ void identify_cpu(struct cpuinfo_arm *c)
c->mm32.bits[1] = READ_SYSREG32(ID_MMFR1_EL1);
c->mm32.bits[2] = READ_SYSREG32(ID_MMFR2_EL1);
c->mm32.bits[3] = READ_SYSREG32(ID_MMFR3_EL1);
+ c->mm32.bits[4] = READ_SYSREG32(ID_MMFR4_EL1);
+ c->mm32.bits[5] = READ_SYSREG32(ID_MMFR5_EL1);
Please don't introduce any more use of READ_SYSREG32(), they are wrong on Armv8
because system registers are always 64-bit.
I followed the existing implementation but ...
c->isa32.bits[0] = READ_SYSREG32(ID_ISAR0_EL1);
c->isa32.bits[1] = READ_SYSREG32(ID_ISAR1_EL1);
@@ -137,6 +144,17 @@ void identify_cpu(struct cpuinfo_arm *c)
c->isa32.bits[3] = READ_SYSREG32(ID_ISAR3_EL1);
c->isa32.bits[4] = READ_SYSREG32(ID_ISAR4_EL1);
c->isa32.bits[5] = READ_SYSREG32(ID_ISAR5_EL1);
+ c->isa32.bits[6] = READ_SYSREG32(ID_ISAR6_EL1);
+
+#ifdef CONFIG_ARM_64
+ c->mvfr.bits[0] = READ_SYSREG64(MVFR0_EL1);
+ c->mvfr.bits[1] = READ_SYSREG64(MVFR1_EL1);
+ c->mvfr.bits[2] = READ_SYSREG64(MVFR2_EL1);
+#else
+ c->mvfr.bits[0] = READ_CP32(MVFR0);
+ c->mvfr.bits[1] = READ_CP32(MVFR1);
+ c->mvfr.bits[2] = READ_CP32(MVFR2);
+#endif
READ_SYSREG() will do the job to either use READ_SYSREG64() or READ_CP32()
depending on the arch used.
.. I can modify the ones I added and the existing ones to user READ_SYSREG
instead.
Please confirm if you want me to do that.
Yes, please use READ_SYSREG() for the new ones. We can convert the
others separately.
}
/*
diff --git a/xen/include/asm-arm/arm64/sysregs.h
b/xen/include/asm-arm/arm64/sysregs.h
index c60029d38f..077fd95fb7 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -57,6 +57,34 @@
#define ICH_AP1R2_EL2 __AP1Rx_EL2(2)
#define ICH_AP1R3_EL2 __AP1Rx_EL2(3)
+/*
+ * Define ID coprocessor registers if they are not
+ * already defined by the compiler.
+ *
+ * Values picked from linux kernel
+ */
+#ifndef ID_AA64MMFR2_EL1
I am a bit puzzled how this meant to work. Will the libc/compiler headers
define ID_AA64MMFR2_EL1?
I tested this and if the compiler has a definition for the register, I am not
entering the ifndef.
So there is no header defining this but if the compiler has the definition for
this the ifndef will
not be entered.
Good to hear :).
+#define ID_AA64MMFR2_EL1 S3_0_C0_C7_2
+#endif
+#ifndef ID_PFR2_EL1
+#define ID_PFR2_EL1 S3_0_C0_C3_4
+#endif
+#ifndef ID_MMFR4_EL1
+#define ID_MMFR4_EL1 S3_0_C0_C2_6
+#endif
+#ifndef ID_MMFR5_EL1
+#define ID_MMFR5_EL1 S3_0_C0_C3_6
+#endif
+#ifndef ID_ISAR6_EL1
+#define ID_ISAR6_EL1 S3_0_C0_C2_7
+#endif
+#ifndef ID_AA64ZFR0_EL1
+#define ID_AA64ZFR0_EL1 S3_0_C0_C4_4
+#endif
+#ifndef ID_DFR1_EL1
+#define ID_DFR1_EL1 S3_0_C0_C3_5
+#endif
+
/* Access to system registers */
#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name))
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 8fd344146e..2690ddeb7a 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -63,6 +63,8 @@
#define FPSID p10,7,c0,c0,0 /* Floating-Point System ID Register
*/
#define FPSCR p10,7,c1,c0,0 /* Floating-Point Status and Control
Register */
#define MVFR0 p10,7,c7,c0,0 /* Media and VFP Feature Register 0 */
+#define MVFR1 p10,7,c6,c0,0 /* Media and VFP Feature Register 1 */
+#define MVFR2 p10,7,c5,c0,0 /* Media and VFP Feature Register 2 */
#define FPEXC p10,7,c8,c0,0 /* Floating-Point Exception Control
Register */
#define FPINST p10,7,c9,c0,0 /* Floating-Point Instruction
Register */
#define FPINST2 p10,7,c10,c0,0 /* Floating-point Instruction
Register 2 */
@@ -108,18 +110,23 @@
#define MPIDR p15,0,c0,c0,5 /* Multiprocessor Affinity Register */
#define ID_PFR0 p15,0,c0,c1,0 /* Processor Feature Register 0 */
#define ID_PFR1 p15,0,c0,c1,1 /* Processor Feature Register 1 */
+#define ID_PFR2 p15,0,c0,c3,4 /* Processor Feature Register 2 */
#define ID_DFR0 p15,0,c0,c1,2 /* Debug Feature Register 0 */
+#define ID_DFR1 p15,0,c0,c3,5 /* Debug Feature Register 1 */
#define ID_AFR0 p15,0,c0,c1,3 /* Auxiliary Feature Register 0 */
#define ID_MMFR0 p15,0,c0,c1,4 /* Memory Model Feature Register 0 */
#define ID_MMFR1 p15,0,c0,c1,5 /* Memory Model Feature Register 1 */
#define ID_MMFR2 p15,0,c0,c1,6 /* Memory Model Feature Register 2 */
#define ID_MMFR3 p15,0,c0,c1,7 /* Memory Model Feature Register 3 */
+#define ID_MMFR4 p15,0,c0,c2,6 /* Memory Model Feature Register 4 */
+#define ID_MMFR5 p15,0,c0,c3,6 /* Memory Model Feature Register 5 */
#define ID_ISAR0 p15,0,c0,c2,0 /* ISA Feature Register 0 */
#define ID_ISAR1 p15,0,c0,c2,1 /* ISA Feature Register 1 */
#define ID_ISAR2 p15,0,c0,c2,2 /* ISA Feature Register 2 */
#define ID_ISAR3 p15,0,c0,c2,3 /* ISA Feature Register 3 */
#define ID_ISAR4 p15,0,c0,c2,4 /* ISA Feature Register 4 */
#define ID_ISAR5 p15,0,c0,c2,5 /* ISA Feature Register 5 */
+#define ID_ISAR6 p15,0,c0,c2,7 /* ISA Feature Register 6 */
#define CCSIDR p15,1,c0,c0,0 /* Cache Size ID Registers */
#define CLIDR p15,1,c0,c0,1 /* Cache Level ID Register */
#define CSSELR p15,2,c0,c0,0 /* Cache Size Selection Register */
@@ -312,18 +319,23 @@
#define HSTR_EL2 HSTR
#define ID_AFR0_EL1 ID_AFR0
#define ID_DFR0_EL1 ID_DFR0
+#define ID_DFR1_EL1 ID_DFR1
#define ID_ISAR0_EL1 ID_ISAR0
#define ID_ISAR1_EL1 ID_ISAR1
#define ID_ISAR2_EL1 ID_ISAR2
#define ID_ISAR3_EL1 ID_ISAR3
#define ID_ISAR4_EL1 ID_ISAR4
#define ID_ISAR5_EL1 ID_ISAR5
+#define ID_ISAR6_EL1 ID_ISAR6
#define ID_MMFR0_EL1 ID_MMFR0
#define ID_MMFR1_EL1 ID_MMFR1
#define ID_MMFR2_EL1 ID_MMFR2
#define ID_MMFR3_EL1 ID_MMFR3
+#define ID_MMFR4_EL1 ID_MMFR4
+#define ID_MMFR5_EL1 ID_MMFR5
#define ID_PFR0_EL1 ID_PFR0
#define ID_PFR1_EL1 ID_PFR1
+#define ID_PFR2_EL1 ID_PFR2
#define IFSR32_EL2 IFSR
#define MDCR_EL2 HDCR
#define MIDR_EL1 MIDR
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index c7b5052992..6cf83d775b 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -148,6 +148,7 @@ struct cpuinfo_arm {
union {
uint64_t bits[2];
struct {
+ /* PFR0 */
unsigned long el0:4;
unsigned long el1:4;
unsigned long el2:4;
@@ -155,9 +156,23 @@ struct cpuinfo_arm {
unsigned long fp:4; /* Floating Point */
unsigned long simd:4; /* Advanced SIMD */
unsigned long gic:4; /* GIC support */
- unsigned long __res0:28;
+ unsigned long ras:4;
+ unsigned long sve:4;
+ unsigned long sel2:4;
+ unsigned long mpam:4;
+ unsigned long amu:4;
+ unsigned long dit:4;
+ unsigned long __res0:4;
unsigned long csv2:4;
- unsigned long __res1:4;
+ unsigned long cvs3:4;
+
+ /* PFR1 */
+ unsigned long bt:4;
+ unsigned long ssbs:4;
+ unsigned long mte:4;
+ unsigned long ras_frac:4;
+ unsigned long mpam_frac:4;
+ unsigned long __res1:44;
};
} pfr64;
@@ -170,7 +185,7 @@ struct cpuinfo_arm {
} aux64;
union {
- uint64_t bits[2];
+ uint64_t bits[3];
struct {
unsigned long pa_range:4;
unsigned long asid_bits:4;
@@ -190,6 +205,8 @@ struct cpuinfo_arm {
unsigned long pan:4;
unsigned long __res1:8;
unsigned long __res2:32;
+
+ unsigned long __res3:64;
};
} mm64;
@@ -197,6 +214,10 @@ struct cpuinfo_arm {
uint64_t bits[2];
} isa64;
+ struct {
+ uint64_t bits[1];
+ } zfr64;
+
#endif
/*
@@ -204,25 +225,38 @@ struct cpuinfo_arm {
* when running in 32-bit mode.
*/
union {
- uint32_t bits[2];
+ uint32_t bits[3];
struct {
+ /* PFR0 */
unsigned long arm:4;
unsigned long thumb:4;
unsigned long jazelle:4;
unsigned long thumbee:4;
- unsigned long __res0:16;
+ unsigned long csv2:4;
+ unsigned long amu:4;
+ unsigned long dit:4;
+ unsigned long ras:4;
+ /* PFR1 */
unsigned long progmodel:4;
unsigned long security:4;
unsigned long mprofile:4;
unsigned long virt:4;
unsigned long gentimer:4;
- unsigned long __res1:12;
+ unsigned long sec_frac:4;
+ unsigned long virt_frac:4;
+ unsigned long gic:4;
+
+ /* PFR2 */
+ unsigned long csv3:4;
+ unsigned long ssbs:4;
+ unsigned long ras_frac:4;
+ unsigned long __res2:20;
};
} pfr32;
struct {
- uint32_t bits[1];
+ uint32_t bits[2];
} dbg32;
struct {
@@ -230,12 +264,16 @@ struct cpuinfo_arm {
} aux32;
struct {
- uint32_t bits[4];
+ uint32_t bits[6];
} mm32;
struct {
- uint32_t bits[6];
+ uint32_t bits[7];
} isa32;
+
+ struct {
+ uint64_t bits[3];
Shouldn't this be register_t?
I followed the scheme of the rest of the structure which
is always using uint64_t or uint32_t for bits definitions.
Right, but I am sure you will not be surprised if I tell you this is
buggy ;). The historical reason is, IIRC, the original spec of Armv8.0
described them as 32-bit registers.
The spec was updated a while ago to clarify they are 64-bit when running
in AArch64. But a majority of them still have the top 32-bit RES0
(thankfully!).
Why should I use register_t type for this one ?
Because the value is 32-bit on AArch32 and 64-bit for AArch64. I am ok
to use 64-bit still for AArch32, but it sounds a bit of a waste of memory.
What I care the most here is we use 64-bit for the new registers on
AArch64. Otherwise, we are going to soon discover that a bit above 32
was allocated and not detected by Xen. I don't want to be the one doing
the debugging!
Admittly, this is not a new issue. However, the more offending code we
had the more it will be difficul to get Xen to be fully compliant with
the Armv8 spec.
Cheers,
--
Julien Grall
|
