Re: [PATCH v3 1/7] xen/arm: Add ID registers and complete cpuinfo

Thu, 10 Dec 2020 07:45:48 -0800 

Hi Bertrand,

On 10/12/2020 15:14, Bertrand Marquis wrote:

Hi Julien,


On 9 Dec 2020, at 23:03, Julien Grall <jul...@xen.org> 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.marq...@arm.com>
---
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

Reply via email to