[PATCH 0/3] Spectre BHB follow up

[PATCH 0/3] Spectre BHB follow up

[Bertrand Marquis]

Following up the handling of Spectre BHB on Arm (XSA-398), this serie
contain several changes which were not needed in the XSA patches but
should be done in Xen:
- Sync sysregs and cpuinfo with latest version of Linux (5.18-rc3)
- Advertise both workaround 1 and 3 if we apply workaround 3 as it
  handle both cases. This will keep the same behaviour for guests which
  are not updated to support workaround 3.
- Add sb instruction support. Some newer generations of CPU
  (Neoverse-N2) do support the instruction so add support for it in Xen.

Bertrand Marquis (3):
  xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3
  xen/arm: Advertise workaround 1 if we apply 3
  xen/arm: Add sb instruction support

 xen/arch/arm/arm64/cpufeature.c          | 18 +++++-
 xen/arch/arm/cpuerrata.c                 | 14 +++++
 xen/arch/arm/include/asm/arm32/macros.h  |  8 +++
 xen/arch/arm/include/asm/arm64/macros.h  | 18 ++++++
 xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----
 xen/arch/arm/include/asm/cpufeature.h    | 17 ++++--
 xen/arch/arm/include/asm/macros.h        |  9 ---
 xen/arch/arm/vsmc.c                      | 11 +++-
 8 files changed, 141 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Bertrand Marquis]

Sync arm64 sysreg bit shift definitions with status of Linux kernel as
of 5.18-rc3 version (linux commit b2d229d4ddb1).
Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
sanitization of ISAR2 registers.
Complete AA64ISAR2 and AA64MMFR1 with more fields.
While there add a comment for MMFR bitfields as for other registers in
the cpuinfo structure definition.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/arm64/cpufeature.c          | 18 +++++-
 xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----
 xen/arch/arm/include/asm/cpufeature.h    | 14 ++++-
 3 files changed, 91 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
index 6e5d30dc7b..d9039d37b2 100644
--- a/xen/arch/arm/arm64/cpufeature.c
+++ b/xen/arch/arm/arm64/cpufeature.c
@@ -143,6 +143,16 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
 	ARM64_FTR_END,
 };
 
+static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64ISAR2_CLEARBHB_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
+		       FTR_STRICT, FTR_EXACT, ID_AA64ISAR2_APA3_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
+		       FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR2_GPA3_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64ISAR2_RPRES_SHIFT, 4, 0),
+	ARM64_FTR_END,
+};
+
 static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_CSV3_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_CSV2_SHIFT, 4, 0),
@@ -158,8 +168,8 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
 	S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL3_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY),
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_ELx_64BIT_ONLY),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_ELx_64BIT_ONLY),
 	ARM64_FTR_END,
 };
 
@@ -197,7 +207,7 @@ static const struct arm64_ftr_bits ftr_id_aa64zfr0[] = {
 };
 
 static const struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_ECV_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_ECV_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_FGT_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_EXS_SHIFT, 4, 0),
 	/*
@@ -243,6 +253,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
 };
 
 static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
+	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_AFP_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_ETS_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_TWED_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_XNX_SHIFT, 4, 0),
@@ -588,6 +599,7 @@ void update_system_features(const struct cpuinfo_arm *new)
 
 	SANITIZE_ID_REG(isa64, 0, aa64isar0);
 	SANITIZE_ID_REG(isa64, 1, aa64isar1);
+	SANITIZE_ID_REG(isa64, 2, aa64isar2);
 
 	SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
 
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index eac08ed33f..54670084c3 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -144,6 +144,30 @@
 
 /* id_aa64isar2 */
 #define ID_AA64ISAR2_CLEARBHB_SHIFT 28
+#define ID_AA64ISAR2_APA3_SHIFT     12
+#define ID_AA64ISAR2_GPA3_SHIFT     8
+#define ID_AA64ISAR2_RPRES_SHIFT    4
+#define ID_AA64ISAR2_WFXT_SHIFT     0
+
+#define ID_AA64ISAR2_RPRES_8BIT     0x0
+#define ID_AA64ISAR2_RPRES_12BIT    0x1
+/*
+ * Value 0x1 has been removed from the architecture, and is
+ * reserved, but has not yet been removed from the ARM ARM
+ * as of ARM DDI 0487G.b.
+ */
+#define ID_AA64ISAR2_WFXT_NI        0x0
+#define ID_AA64ISAR2_WFXT_SUPPORTED 0x2
+
+#define ID_AA64ISAR2_APA3_NI                  0x0
+#define ID_AA64ISAR2_APA3_ARCHITECTED         0x1
+#define ID_AA64ISAR2_APA3_ARCH_EPAC           0x2
+#define ID_AA64ISAR2_APA3_ARCH_EPAC2          0x3
+#define ID_AA64ISAR2_APA3_ARCH_EPAC2_FPAC     0x4
+#define ID_AA64ISAR2_APA3_ARCH_EPAC2_FPAC_CMB 0x5
+
+#define ID_AA64ISAR2_GPA3_NI             0x0
+#define ID_AA64ISAR2_GPA3_ARCHITECTED    0x1
 
 /* id_aa64pfr0 */
 #define ID_AA64PFR0_CSV3_SHIFT       60
@@ -165,14 +189,13 @@
 #define ID_AA64PFR0_AMU              0x1
 #define ID_AA64PFR0_SVE              0x1
 #define ID_AA64PFR0_RAS_V1           0x1
+#define ID_AA64PFR0_RAS_V1P1         0x2
 #define ID_AA64PFR0_FP_NI            0xf
 #define ID_AA64PFR0_FP_SUPPORTED     0x0
 #define ID_AA64PFR0_ASIMD_NI         0xf
 #define ID_AA64PFR0_ASIMD_SUPPORTED  0x0
-#define ID_AA64PFR0_EL1_64BIT_ONLY   0x1
-#define ID_AA64PFR0_EL1_32BIT_64BIT  0x2
-#define ID_AA64PFR0_EL0_64BIT_ONLY   0x1
-#define ID_AA64PFR0_EL0_32BIT_64BIT  0x2
+#define ID_AA64PFR0_ELx_64BIT_ONLY   0x1
+#define ID_AA64PFR0_ELx_32BIT_64BIT  0x2
 
 /* id_aa64pfr1 */
 #define ID_AA64PFR1_MPAMFRAC_SHIFT   16
@@ -189,6 +212,7 @@
 #define ID_AA64PFR1_MTE_NI           0x0
 #define ID_AA64PFR1_MTE_EL0          0x1
 #define ID_AA64PFR1_MTE              0x2
+#define ID_AA64PFR1_MTE_ASYMM        0x3
 
 /* id_aa64zfr0 */
 #define ID_AA64ZFR0_F64MM_SHIFT      56
@@ -228,17 +252,37 @@
 #define ID_AA64MMFR0_ASID_SHIFT      4
 #define ID_AA64MMFR0_PARANGE_SHIFT   0
 
-#define ID_AA64MMFR0_TGRAN4_NI         0xf
-#define ID_AA64MMFR0_TGRAN4_SUPPORTED  0x0
-#define ID_AA64MMFR0_TGRAN64_NI        0xf
-#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
-#define ID_AA64MMFR0_TGRAN16_NI        0x0
-#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
+#define ID_AA64MMFR0_ASID_8          0x0
+#define ID_AA64MMFR0_ASID_16         0x2
+
+#define ID_AA64MMFR0_TGRAN4_NI             0xf
+#define ID_AA64MMFR0_TGRAN4_SUPPORTED_MIN  0x0
+#define ID_AA64MMFR0_TGRAN4_SUPPORTED_MAX  0x7
+#define ID_AA64MMFR0_TGRAN64_NI            0xf
+#define ID_AA64MMFR0_TGRAN64_SUPPORTED_MIN 0x0
+#define ID_AA64MMFR0_TGRAN64_SUPPORTED_MAX 0x7
+#define ID_AA64MMFR0_TGRAN16_NI            0x0
+#define ID_AA64MMFR0_TGRAN16_SUPPORTED_MIN 0x1
+#define ID_AA64MMFR0_TGRAN16_SUPPORTED_MAX 0xf
+
+#define ID_AA64MMFR0_PARANGE_32        0x0
+#define ID_AA64MMFR0_PARANGE_36        0x1
+#define ID_AA64MMFR0_PARANGE_40        0x2
+#define ID_AA64MMFR0_PARANGE_42        0x3
+#define ID_AA64MMFR0_PARANGE_44        0x4
 #define ID_AA64MMFR0_PARANGE_48        0x5
 #define ID_AA64MMFR0_PARANGE_52        0x6
 
+#define ARM64_MIN_PARANGE_BITS     32
+
+#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_DEFAULT 0x0
+#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_NONE    0x1
+#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_MIN     0x2
+#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_MAX     0x7
+
 /* id_aa64mmfr1 */
 #define ID_AA64MMFR1_ECBHB_SHIFT     60
+#define ID_AA64MMFR1_AFP_SHIFT       44
 #define ID_AA64MMFR1_ETS_SHIFT       36
 #define ID_AA64MMFR1_TWED_SHIFT      32
 #define ID_AA64MMFR1_XNX_SHIFT       28
@@ -271,6 +315,9 @@
 #define ID_AA64MMFR2_CNP_SHIFT       0
 
 /* id_aa64dfr0 */
+#define ID_AA64DFR0_MTPMU_SHIFT      48
+#define ID_AA64DFR0_TRBE_SHIFT       44
+#define ID_AA64DFR0_TRACE_FILT_SHIFT 40
 #define ID_AA64DFR0_DOUBLELOCK_SHIFT 36
 #define ID_AA64DFR0_PMSVER_SHIFT     32
 #define ID_AA64DFR0_CTX_CMPS_SHIFT   28
@@ -284,11 +331,18 @@
 #define ID_AA64DFR0_PMUVER_8_1       0x4
 #define ID_AA64DFR0_PMUVER_8_4       0x5
 #define ID_AA64DFR0_PMUVER_8_5       0x6
+#define ID_AA64DFR0_PMUVER_8_7       0x7
 #define ID_AA64DFR0_PMUVER_IMP_DEF   0xf
 
+#define ID_AA64DFR0_PMSVER_8_2      0x1
+#define ID_AA64DFR0_PMSVER_8_3      0x2
+
 #define ID_DFR0_PERFMON_SHIFT        24
 
-#define ID_DFR0_PERFMON_8_1          0x4
+#define ID_DFR0_PERFMON_8_0         0x3
+#define ID_DFR0_PERFMON_8_1         0x4
+#define ID_DFR0_PERFMON_8_4         0x5
+#define ID_DFR0_PERFMON_8_5         0x6
 
 #define ID_ISAR4_SWP_FRAC_SHIFT        28
 #define ID_ISAR4_PSR_M_SHIFT           24
diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
index f7368766c0..4719de47f3 100644
--- a/xen/arch/arm/include/asm/cpufeature.h
+++ b/xen/arch/arm/include/asm/cpufeature.h
@@ -230,6 +230,7 @@ struct cpuinfo_arm {
     union {
         register_t bits[3];
         struct {
+            /* MMFR0 */
             unsigned long pa_range:4;
             unsigned long asid_bits:4;
             unsigned long bigend:4;
@@ -240,16 +241,21 @@ struct cpuinfo_arm {
             unsigned long tgranule_4K:4;
             unsigned long __res0:32;
 
+            /* MMFR1 */
             unsigned long hafdbs:4;
             unsigned long vmid_bits:4;
             unsigned long vh:4;
             unsigned long hpds:4;
             unsigned long lo:4;
             unsigned long pan:4;
-            unsigned long __res1:8;
-            unsigned long __res2:28;
+            unsigned long specsei:4;
+            unsigned long xnx:4;
+            unsigned long twed:4;
+            unsigned long ets:4;
+            unsigned long __res1:20;
             unsigned long ecbhb:4;
 
+            /* MMFR2 */
             unsigned long __res3:64;
         };
     } mm64;
@@ -293,7 +299,9 @@ struct cpuinfo_arm {
             unsigned long __res2:8;
 
             /* ISAR2 */
-            unsigned long __res3:28;
+            unsigned long wfxt:4;
+            unsigned long rpres:4;
+            unsigned long __res3:20;
             unsigned long clearbhb:4;
 
             unsigned long __res4:32;
-- 
2.25.1

[PATCH 2/3] xen/arm: Advertise workaround 1 if we apply 3

[Bertrand Marquis]

SMCC_WORKAROUND_3 is handling both Spectre v2 and spectre BHB.
So when a guest is asking if we support workaround 1, tell yes if we
apply workaround 3 on exception entry as it handles it.

This will allow guests not supporting Spectre BHB but impacted by
spectre v2 to still handle it correctly.
The modified behaviour is coherent with what the Linux kernel does in
KVM for guests.

While there use ARM_SMCCC_SUCCESS instead of 0 for the return code value
for workaround detection to be coherent with Workaround 2 handling.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/vsmc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index b633ff2fe8..676740ef15 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -104,8 +104,13 @@ static bool handle_arch(struct cpu_user_regs *regs)
         switch ( arch_func_id )
         {
         case ARM_SMCCC_ARCH_WORKAROUND_1_FID:
-            if ( cpus_have_cap(ARM_HARDEN_BRANCH_PREDICTOR) )
-                ret = 0;
+            /*
+             * Workaround 3 is also mitigating spectre v2 so advertise that we
+             * support Workaround 1 if we do Workaround 3 on exception entry.
+             */
+            if ( cpus_have_cap(ARM_HARDEN_BRANCH_PREDICTOR) ||
+                 cpus_have_cap(ARM_WORKAROUND_BHB_SMCC_3) )
+                ret = ARM_SMCCC_SUCCESS;
             break;
         case ARM_SMCCC_ARCH_WORKAROUND_2_FID:
             switch ( get_ssbd_state() )
@@ -126,7 +131,7 @@ static bool handle_arch(struct cpu_user_regs *regs)
             break;
         case ARM_SMCCC_ARCH_WORKAROUND_3_FID:
             if ( cpus_have_cap(ARM_WORKAROUND_BHB_SMCC_3) )
-                ret = 0;
+                ret = ARM_SMCCC_SUCCESS;
             break;
         }
 
-- 
2.25.1

[PATCH 3/3] xen/arm: Add sb instruction support

[Bertrand Marquis]

This patch is adding sb instruction support when it is supported by a
CPU on arm64.
To achieve this, the "sb" macro is moved to sub-arch macros.h so that we
can use sb instruction when available through alternative on arm64 and
keep the current behaviour on arm32.
A new cpuerrata capability is introduced to enable the alternative
code for sb when the support is detected using isa64 coprocessor
register.
The sb instruction is encoded using its hexadecimal value.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/cpuerrata.c                | 14 ++++++++++++++
 xen/arch/arm/include/asm/arm32/macros.h |  8 ++++++++
 xen/arch/arm/include/asm/arm64/macros.h | 18 ++++++++++++++++++
 xen/arch/arm/include/asm/cpufeature.h   |  3 ++-
 xen/arch/arm/include/asm/macros.h       |  9 ---------
 5 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index ae649d16ef..e744abe800 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -458,6 +458,13 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
 }
 #endif
 
+#ifdef CONFIG_ARM_64
+static bool has_sb_instruction(const struct arm_cpu_capabilities *entry)
+{
+    return system_cpuinfo.isa64.sb;
+}
+#endif
+
 #define MIDR_RANGE(model, min, max)     \
     .matches = is_affected_midr_range,  \
     .midr_model = model,                \
@@ -674,6 +681,13 @@ static const struct arm_cpu_capabilities arm_errata[] = {
         .capability = ARM64_WORKAROUND_AT_SPECULATE,
         MIDR_ALL_VERSIONS(MIDR_CORTEX_A55),
     },
+#ifdef CONFIG_ARM_64
+    {
+        .desc = "Speculation barrier (SB)",
+        .capability = ARM64_HAS_SB,
+        .matches = has_sb_instruction,
+    },
+#endif
     {},
 };
 
diff --git a/xen/arch/arm/include/asm/arm32/macros.h b/xen/arch/arm/include/asm/arm32/macros.h
index a4e20aa520..cf0ddad52b 100644
--- a/xen/arch/arm/include/asm/arm32/macros.h
+++ b/xen/arch/arm/include/asm/arm32/macros.h
@@ -5,4 +5,12 @@
         mov     pc, lr
     .endm
 
+    /*
+     * Speculative barrier
+     */
+    .macro sb
+    dsb nsh
+    isb
+    .endm
+
 #endif /* __ASM_ARM_ARM32_MACROS_H */
diff --git a/xen/arch/arm/include/asm/arm64/macros.h b/xen/arch/arm/include/asm/arm64/macros.h
index 140e223b4c..e639cec400 100644
--- a/xen/arch/arm/include/asm/arm64/macros.h
+++ b/xen/arch/arm/include/asm/arm64/macros.h
@@ -1,6 +1,24 @@
 #ifndef __ASM_ARM_ARM64_MACROS_H
 #define __ASM_ARM_ARM64_MACROS_H
 
+#include <asm/alternative.h>
+
+    /*
+     * Speculative barrier
+     */
+    .macro sb
+alternative_if_not ARM64_HAS_SB
+    dsb nsh
+    isb
+alternative_else
+/*
+ * SB encoding as given in chapter C6.2.264 of ARM ARM (DDI 0487H.a).
+ */
+    .inst 0xd50330ff
+    nop
+alternative_endif
+    .endm
+
     /*
      * @dst: Result of get_cpu_info()
      */
diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
index 4719de47f3..9370805900 100644
--- a/xen/arch/arm/include/asm/cpufeature.h
+++ b/xen/arch/arm/include/asm/cpufeature.h
@@ -67,8 +67,9 @@
 #define ARM_WORKAROUND_BHB_LOOP_24 13
 #define ARM_WORKAROUND_BHB_LOOP_32 14
 #define ARM_WORKAROUND_BHB_SMCC_3 15
+#define ARM64_HAS_SB 16
 
-#define ARM_NCAPS           16
+#define ARM_NCAPS           17
 
 #ifndef __ASSEMBLY__
 
diff --git a/xen/arch/arm/include/asm/macros.h b/xen/arch/arm/include/asm/macros.h
index 1aa373760f..91ea3505e4 100644
--- a/xen/arch/arm/include/asm/macros.h
+++ b/xen/arch/arm/include/asm/macros.h
@@ -5,15 +5,6 @@
 # error "This file should only be included in assembly file"
 #endif
 
-    /*
-     * Speculative barrier
-     * XXX: Add support for the 'sb' instruction
-     */
-    .macro sb
-    dsb nsh
-    isb
-    .endm
-
 #if defined (CONFIG_ARM_32)
 # include <asm/arm32/macros.h>
 #elif defined(CONFIG_ARM_64)
-- 
2.25.1

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Julien Grall]

Hi Bertrand,

On 03/05/2022 10:38, Bertrand Marquis wrote:
> Sync arm64 sysreg bit shift definitions with status of Linux kernel as
> of 5.18-rc3 version (linux commit b2d229d4ddb1).
> Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
> sanitization of ISAR2 registers.
Please outline which specific commits you are actually backported. This 
would help to know what changed, why and also keep track of the autorships.

When possible, the changes should be separated to match each Linux 
commit we backport.

> Complete AA64ISAR2 and AA64MMFR1 with more fields.
> While there add a comment for MMFR bitfields as for other registers in
> the cpuinfo structure definition.

AFAICT, this patch is doing 3 different things that are somewhat related:
   - Sync cpufeature.c
   - Update the headers with unused defines
   - Complete the structure cpufeature.h

All those changes seem to be independent, so I think they should be done 
separately. This would help to keep the authorship right (your code vs 
Linux code).

> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
>   xen/arch/arm/arm64/cpufeature.c          | 18 +++++-
>   xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----
>   xen/arch/arm/include/asm/cpufeature.h    | 14 ++++-
>   3 files changed, 91 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
> index 6e5d30dc7b..d9039d37b2 100644
> --- a/xen/arch/arm/arm64/cpufeature.c
> +++ b/xen/arch/arm/arm64/cpufeature.c
> @@ -143,6 +143,16 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>   	ARM64_FTR_END,
>   };
>   
> +static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64ISAR2_CLEARBHB_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
> +		       FTR_STRICT, FTR_EXACT, ID_AA64ISAR2_APA3_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
So we are using CONFIG_ARM64_PTR_AUTH. But this is not defined in 
Kconfig. I realize there are more in cpufeature.c (somehow I didn't spot 
during preview), but I don't think this is right to define CONFIG_* 
without an associated entry in Kconfig.

In one hand, I think it would be odd to add an entry in Kconfig because 
Xen wouldn't properly work if selected. On the other hand, it is useful 
if when we will implement pointer authentification.

So maybe we should just add the Kconfig entry with a comment explaning 
why they are not selected. Any thoughts?

Cheers,

-- 
Julien Grall

Re: [PATCH 2/3] xen/arm: Advertise workaround 1 if we apply 3

[Julien Grall]

Hi Bertrand,

On 03/05/2022 10:38, Bertrand Marquis wrote:
> SMCC_WORKAROUND_3 is handling both Spectre v2 and spectre BHB.
> So when a guest is asking if we support workaround 1, tell yes if we
> apply workaround 3 on exception entry as it handles it.
> 
> This will allow guests not supporting Spectre BHB but impacted by
> spectre v2 to still handle it correctly.
> The modified behaviour is coherent with what the Linux kernel does in
> KVM for guests.
> 
> While there use ARM_SMCCC_SUCCESS instead of 0 for the return code value
> for workaround detection to be coherent with Workaround 2 handling.
> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>

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

I think we should also consider for backport.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Julien Grall]

Hi Bertrand,

On 03/05/2022 10:38, Bertrand Marquis wrote:
> This patch is adding sb instruction support when it is supported by a
> CPU on arm64.
> To achieve this, the "sb" macro is moved to sub-arch macros.h so that we
> can use sb instruction when available through alternative on arm64 and
> keep the current behaviour on arm32.

SB is also supported on Arm32. So I would prefer to introduce the 
encoding right now and avoid duplicating the .macro sb.

> A new cpuerrata capability is introduced to enable the alternative

'sb' is definitely not an erratum. Errata are for stuff that are meant 
to be specific to one (or multiple) CPU and they are not part of the 
architecture.

This is the first time we introduce a feature in Xen. So we need to add 
a new array in cpufeature.c that will cover 'SB' for now. In future we 
could add feature like pointer auth, LSE atomics...

> code for sb when the support is detected using isa64 coprocessor

s/coprocessor/system/

> register.
> The sb instruction is encoded using its hexadecimal value.

This is necessary to avoid recursive macro, right?

> diff --git a/xen/arch/arm/include/asm/arm64/macros.h b/xen/arch/arm/include/asm/arm64/macros.h
> index 140e223b4c..e639cec400 100644
> --- a/xen/arch/arm/include/asm/arm64/macros.h
> +++ b/xen/arch/arm/include/asm/arm64/macros.h
> @@ -1,6 +1,24 @@
>   #ifndef __ASM_ARM_ARM64_MACROS_H
>   #define __ASM_ARM_ARM64_MACROS_H
>   
> +#include <asm/alternative.h>
> +
> +    /*
> +     * Speculative barrier
> +     */
> +    .macro sb
> +alternative_if_not ARM64_HAS_SB
> +    dsb nsh
> +    isb
> +alternative_else
> +/*
> + * SB encoding as given in chapter C6.2.264 of ARM ARM (DDI 0487H.a).
> + */

NIT: Please align the comment with ".inst" below. I also don't think it 
is necessary to mention the spec here. The instruction encoding is not 
going to change.

> +    .inst 0xd50330ff
> +    nop

Why do we need the NOP?

> +alternative_endif
> +    .endm
> +
>       /*
>        * @dst: Result of get_cpu_info()
>        */
> diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
> index 4719de47f3..9370805900 100644
> --- a/xen/arch/arm/include/asm/cpufeature.h
> +++ b/xen/arch/arm/include/asm/cpufeature.h
> @@ -67,8 +67,9 @@
>   #define ARM_WORKAROUND_BHB_LOOP_24 13
>   #define ARM_WORKAROUND_BHB_LOOP_32 14
>   #define ARM_WORKAROUND_BHB_SMCC_3 15
> +#define ARM64_HAS_SB 16
>   
> -#define ARM_NCAPS           16
> +#define ARM_NCAPS           17
>   
>   #ifndef __ASSEMBLY__
>   
> diff --git a/xen/arch/arm/include/asm/macros.h b/xen/arch/arm/include/asm/macros.h
> index 1aa373760f..91ea3505e4 100644
> --- a/xen/arch/arm/include/asm/macros.h
> +++ b/xen/arch/arm/include/asm/macros.h
> @@ -5,15 +5,6 @@
>   # error "This file should only be included in assembly file"
>   #endif
>   
> -    /*
> -     * Speculative barrier
> -     * XXX: Add support for the 'sb' instruction
> -     */
> -    .macro sb
> -    dsb nsh
> -    isb
> -    .endm
> -
>   #if defined (CONFIG_ARM_32)
>   # include <asm/arm32/macros.h>
>   #elif defined(CONFIG_ARM_64)

Cheers,

-- 
Julien Grall

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Bertrand Marquis]

Hi Julien,

> On 3 May 2022, at 19:47, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 03/05/2022 10:38, Bertrand Marquis wrote:
>> This patch is adding sb instruction support when it is supported by a
>> CPU on arm64.
>> To achieve this, the "sb" macro is moved to sub-arch macros.h so that we
>> can use sb instruction when available through alternative on arm64 and
>> keep the current behaviour on arm32.
> 
> SB is also supported on Arm32. So I would prefer to introduce the encoding right now and avoid duplicating the .macro sb.

I will look into that.

> 
>> A new cpuerrata capability is introduced to enable the alternative
> 
> 'sb' is definitely not an erratum. Errata are for stuff that are meant to be specific to one (or multiple) CPU and they are not part of the architecture.
> 
> This is the first time we introduce a feature in Xen. So we need to add a new array in cpufeature.c that will cover 'SB' for now. In future we could add feature like pointer auth, LSE atomics...

I am not quite sure why you would want to do that.

Using the sb instruction is definitely something to do to solve erratas, if a CPU is not impacted by those erratas, why would you want to use this ?

> 
>> code for sb when the support is detected using isa64 coprocessor
> 
> s/coprocessor/system/

Ack

> 
>> register.
>> The sb instruction is encoded using its hexadecimal value.
> 
> This is necessary to avoid recursive macro, right?

This is necessary for several reasons:
- support compilers not supporting sb instructions (need encoding)
- handle the alternative code (we do not want to repeat this everywhere)
- avoid recursive macro

> 
>> diff --git a/xen/arch/arm/include/asm/arm64/macros.h b/xen/arch/arm/include/asm/arm64/macros.h
>> index 140e223b4c..e639cec400 100644
>> --- a/xen/arch/arm/include/asm/arm64/macros.h
>> +++ b/xen/arch/arm/include/asm/arm64/macros.h
>> @@ -1,6 +1,24 @@
>>  #ifndef __ASM_ARM_ARM64_MACROS_H
>>  #define __ASM_ARM_ARM64_MACROS_H
>>  +#include <asm/alternative.h>
>> +
>> +    /*
>> +     * Speculative barrier
>> +     */
>> +    .macro sb
>> +alternative_if_not ARM64_HAS_SB
>> +    dsb nsh
>> +    isb
>> +alternative_else
>> +/*
>> + * SB encoding as given in chapter C6.2.264 of ARM ARM (DDI 0487H.a).
>> + */
> 
> NIT: Please align the comment with ".inst" below. I also don't think it is necessary to mention the spec here. The instruction encoding is not going to change.
Ack

> 
>> +    .inst 0xd50330ff
>> +    nop
> 
> Why do we need the NOP?

Alternative requires both sides to have the same size hence the nop to have 2 instructions as in the if.

> 
>> +alternative_endif
>> +    .endm
>> +
>>      /*
>>       * @dst: Result of get_cpu_info()
>>       */
>> diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
>> index 4719de47f3..9370805900 100644
>> --- a/xen/arch/arm/include/asm/cpufeature.h
>> +++ b/xen/arch/arm/include/asm/cpufeature.h
>> @@ -67,8 +67,9 @@
>>  #define ARM_WORKAROUND_BHB_LOOP_24 13
>>  #define ARM_WORKAROUND_BHB_LOOP_32 14
>>  #define ARM_WORKAROUND_BHB_SMCC_3 15
>> +#define ARM64_HAS_SB 16
>>  -#define ARM_NCAPS           16
>> +#define ARM_NCAPS           17
>>    #ifndef __ASSEMBLY__
>>  diff --git a/xen/arch/arm/include/asm/macros.h b/xen/arch/arm/include/asm/macros.h
>> index 1aa373760f..91ea3505e4 100644
>> --- a/xen/arch/arm/include/asm/macros.h
>> +++ b/xen/arch/arm/include/asm/macros.h
>> @@ -5,15 +5,6 @@
>>  # error "This file should only be included in assembly file"
>>  #endif
>>  -    /*
>> -     * Speculative barrier
>> -     * XXX: Add support for the 'sb' instruction
>> -     */
>> -    .macro sb
>> -    dsb nsh
>> -    isb
>> -    .endm
>> -
>>  #if defined (CONFIG_ARM_32)
>>  # include <asm/arm32/macros.h>
>>  #elif defined(CONFIG_ARM_64)
> 
> Cheers,

Thanks for the review

Cheers
Bertrand

Re: [PATCH 2/3] xen/arm: Advertise workaround 1 if we apply 3

[Bertrand Marquis]

Hi Julien,

> On 3 May 2022, at 19:17, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 03/05/2022 10:38, Bertrand Marquis wrote:
>> SMCC_WORKAROUND_3 is handling both Spectre v2 and spectre BHB.
>> So when a guest is asking if we support workaround 1, tell yes if we
>> apply workaround 3 on exception entry as it handles it.
>> This will allow guests not supporting Spectre BHB but impacted by
>> spectre v2 to still handle it correctly.
>> The modified behaviour is coherent with what the Linux kernel does in
>> KVM for guests.
>> While there use ARM_SMCCC_SUCCESS instead of 0 for the return code value
>> for workaround detection to be coherent with Workaround 2 handling.
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks

> 
> I think we should also consider for backport.

Agree.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Bertrand Marquis]

Hi Julien,

> On 3 May 2022, at 19:08, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 03/05/2022 10:38, Bertrand Marquis wrote:
>> Sync arm64 sysreg bit shift definitions with status of Linux kernel as
>> of 5.18-rc3 version (linux commit b2d229d4ddb1).
>> Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
>> sanitization of ISAR2 registers.
> Please outline which specific commits you are actually backported. This would help to know what changed, why and also keep track of the autorships.
> 
> When possible, the changes should be separated to match each Linux commit we backport.

As those are exactly identical to the linux tree, one can easily use git blame on the linux source tree to find those information if it is needed.

I checked a bit and this is not something that was required before (for example when the cpufeature was introduced).

> 
>> Complete AA64ISAR2 and AA64MMFR1 with more fields.
>> While there add a comment for MMFR bitfields as for other registers in
>> the cpuinfo structure definition.
> 
> AFAICT, this patch is doing 3 different things that are somewhat related:
> - Sync cpufeature.c
> - Update the headers with unused defines
> - Complete the structure cpufeature.h
> 
> All those changes seem to be independent, so I think they should be done separately. This would help to keep the authorship right (your code vs Linux code).

This and the previous request to split using linux commit will actually end up in 10 patches or more.

In the current, the change can easily be checked doing a diff with the mentioned Linux version, so I am not really thrilled to make it more complex.

Please confirm that all this is really what you want.

> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> xen/arch/arm/arm64/cpufeature.c | 18 +++++-
>> xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----
>> xen/arch/arm/include/asm/cpufeature.h | 14 ++++-
>> 3 files changed, 91 insertions(+), 17 deletions(-)
>> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
>> index 6e5d30dc7b..d9039d37b2 100644
>> --- a/xen/arch/arm/arm64/cpufeature.c
>> +++ b/xen/arch/arm/arm64/cpufeature.c
>> @@ -143,6 +143,16 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>> 	ARM64_FTR_END,
>> };
>> +static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64ISAR2_CLEARBHB_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>> +		 FTR_STRICT, FTR_EXACT, ID_AA64ISAR2_APA3_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
> So we are using CONFIG_ARM64_PTR_AUTH. But this is not defined in Kconfig. I realize there are more in cpufeature.c (somehow I didn't spot during preview), but I don't think this is right to define CONFIG_* without an associated entry in Kconfig.
> 
> In one hand, I think it would be odd to add an entry in Kconfig because Xen wouldn't properly work if selected. On the other hand, it is useful if when we will implement pointer authentification.
> 
> So maybe we should just add the Kconfig entry with a comment explaning why they are not selected. Any thoughts?

This is really right and a very good catch.

I think it would make sense to introduce those in Kconfig in order to keep the code equivalent to Linux.

So I would suggest here to add hidden entries like this:

ARM64_PTR_AUTH
	def_bool n
	depends on ARM64
        help
          Pointer authentication support.
          This feature is not supported by Xen.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Julien Grall]

On 04/05/2022 08:24, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 3 May 2022, at 19:47, Julien Grall <julien@xen.org> wrote:
>>> A new cpuerrata capability is introduced to enable the alternative
>>
>> 'sb' is definitely not an erratum. Errata are for stuff that are meant to be specific to one (or multiple) CPU and they are not part of the architecture.
>>
>> This is the first time we introduce a feature in Xen. So we need to add a new array in cpufeature.c that will cover 'SB' for now. In future we could add feature like pointer auth, LSE atomics...
> 
> I am not quite sure why you would want to do that.
> 
> Using the sb instruction is definitely something to do to solve erratas, if a CPU is not impacted by those erratas, why would you want to use this ?

I agree that SB is used to solve errata but the instruction itself is 
not a workaround (it may be part of it though). Instead, this is a more 
efficient way to prevent speculation and will replace dsb/isb.

Speculation is never going to disappear from processor. So, in the 
future, there might be valid reason for us to say "We don't want the 
processor to speculate". This would mean using SB.

>>> +    .inst 0xd50330ff
>>> +    nop
>>
>> Why do we need the NOP?
> 
> Alternative requires both sides to have the same size hence the nop to have 2 instructions as in the if.

A few years ago we backported a patch from Linux to automatically add 
nop. However, looking at the code, this would not handle this

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Julien Grall]

On 04/05/2022 08:39, Bertrand Marquis wrote:
> Hi Julien,
Hi,

>> On 3 May 2022, at 19:08, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Bertrand,
>>
>> On 03/05/2022 10:38, Bertrand Marquis wrote:
>>> Sync arm64 sysreg bit shift definitions with status of Linux kernel as
>>> of 5.18-rc3 version (linux commit b2d229d4ddb1).
>>> Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
>>> sanitization of ISAR2 registers.
>> Please outline which specific commits you are actually backported. This would help to know what changed, why and also keep track of the autorships.
>>
>> When possible, the changes should be separated to match each Linux commit we backport.
> 
> As those are exactly identical to the linux tree, one can easily use git blame on the linux source tree to find those information if it is needed

Well... that's possible at the cost of everyone going through Linux to 
understand why the changes were made. This is not very scalable.

> 
> I checked a bit and this is not something that was required before (for example when the cpufeature was introduced).

If we import the full file, then we will generally don't log all the 
commits. However, for smaller changes, we will always mention the commit 
backported. There are several examples on the ML:

  - 0435784cc75d ("xen/arm: smmuv1: Intelligent SMR allocation")
  - 9c432b876bf5 ("x86/mwait-idle: add SPR support")

We also recently introduced a tag "Origin:" to keep track of which 
commit was backported. If you want to understand the rationale, you can 
read this long thread:

https://lore.kernel.org/xen-devel/0ed245fa-58a7-a5f6-b82e-48f9ed0b6970@suse.com/

> 
>>
>>> Complete AA64ISAR2 and AA64MMFR1 with more fields.
>>> While there add a comment for MMFR bitfields as for other registers in
>>> the cpuinfo structure definition.
>>
>> AFAICT, this patch is doing 3 different things that are somewhat related:
>> - Sync cpufeature.c
>> - Update the headers with unused defines
>> - Complete the structure cpufeature.h
>>
>> All those changes seem to be independent, so I think they should be done separately. This would help to keep the authorship right (your code vs Linux code).
> 
> This and the previous request to split using linux commit will actually end up in 10 patches or more.

I think we need to differentiate the two request. The previous request 
is about logging which commits you backported. I would be open to have 
all of them in one patch so long we account the authors/tags properly.

For this request, this is mostly about avoid to mix multiple things 
together. Your commit message describes 3 distinct parts and therefore 
they should be split.

>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>> ---
>>> xen/arch/arm/arm64/cpufeature.c | 18 +++++-
>>> xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----
>>> xen/arch/arm/include/asm/cpufeature.h | 14 ++++-
>>> 3 files changed, 91 insertions(+), 17 deletions(-)
>>> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
>>> index 6e5d30dc7b..d9039d37b2 100644
>>> --- a/xen/arch/arm/arm64/cpufeature.c
>>> +++ b/xen/arch/arm/arm64/cpufeature.c
>>> @@ -143,6 +143,16 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>>> 	ARM64_FTR_END,
>>> };
>>> +static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64ISAR2_CLEARBHB_SHIFT, 4, 0),
>>> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>>> +		 FTR_STRICT, FTR_EXACT, ID_AA64ISAR2_APA3_SHIFT, 4, 0),
>>> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>> So we are using CONFIG_ARM64_PTR_AUTH. But this is not defined in Kconfig. I realize there are more in cpufeature.c (somehow I didn't spot during preview), but I don't think this is right to define CONFIG_* without an associated entry in Kconfig.
>>
>> In one hand, I think it would be odd to add an entry in Kconfig because Xen wouldn't properly work if selected. On the other hand, it is useful if when we will implement pointer authentification.
>>
>> So maybe we should just add the Kconfig entry with a comment explaning why they are not selected. Any thoughts?
> 
> This is really right and a very good catch.
> 
> I think it would make sense to introduce those in Kconfig in order to keep the code equivalent to Linux.
> 
> So I would suggest here to add hidden entries like this:
> 
> ARM64_PTR_AUTH
> 	def_bool n
> 	depends on ARM64
>          help
>            Pointer authentication support.
>            This feature is not supported by Xen.

I am OK with that.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Bertrand Marquis]

Hi Julien,

> On 4 May 2022, at 09:20, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 04/05/2022 08:39, Bertrand Marquis wrote:
>> Hi Julien,
> Hi,
> 
>>> On 3 May 2022, at 19:08, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On 03/05/2022 10:38, Bertrand Marquis wrote:
>>>> Sync arm64 sysreg bit shift definitions with status of Linux kernel as
>>>> of 5.18-rc3 version (linux commit b2d229d4ddb1).
>>>> Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
>>>> sanitization of ISAR2 registers.
>>> Please outline which specific commits you are actually backported. This would help to know what changed, why and also keep track of the autorships.
>>> 
>>> When possible, the changes should be separated to match each Linux commit we backport.
>> As those are exactly identical to the linux tree, one can easily use git blame on the linux source tree to find those information if it is needed
> 
> Well... that's possible at the cost of everyone going through Linux to understand why the changes were made. This is not very scalable.
> 
>> I checked a bit and this is not something that was required before (for example when the cpufeature was introduced).
> 
> If we import the full file, then we will generally don't log all the commits. However, for smaller changes, we will always mention the commit backported. There are several examples on the ML:
> 
> - 0435784cc75d ("xen/arm: smmuv1: Intelligent SMR allocation")
> - 9c432b876bf5 ("x86/mwait-idle: add SPR support")
> 
> We also recently introduced a tag "Origin:" to keep track of which commit was backported. If you want to understand the rationale, you can read this long thread:
> 
> https://lore.kernel.org/xen-devel/0ed245fa-58a7-a5f6-b82e-48f9ed0b6970@suse.com/

Do I understand right that it is ok for you if I push one patch mentioning all the commits done in Linux corresponding to the changes (instead of one patch per commit) ?

> 
>>> 
>>>> Complete AA64ISAR2 and AA64MMFR1 with more fields.
>>>> While there add a comment for MMFR bitfields as for other registers in
>>>> the cpuinfo structure definition.
>>> 
>>> AFAICT, this patch is doing 3 different things that are somewhat related:
>>> - Sync cpufeature.c
>>> - Update the headers with unused defines
>>> - Complete the structure cpufeature.h
>>> 
>>> All those changes seem to be independent, so I think they should be done separately. This would help to keep the authorship right (your code vs Linux code).
>> This and the previous request to split using linux commit will actually end up in 10 patches or more.
> 
> I think we need to differentiate the two request. The previous request is about logging which commits you backported. I would be open to have all of them in one patch so long we account the authors/tags properly.
> 
> For this request, this is mostly about avoid to mix multiple things together. Your commit message describes 3 distinct parts and therefore they should be split.

So 3 patches if you confirm the previous point.
I am ok with that

> 
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>> ---
>>>> xen/arch/arm/arm64/cpufeature.c | 18 +++++-
>>>> xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----
>>>> xen/arch/arm/include/asm/cpufeature.h | 14 ++++-
>>>> 3 files changed, 91 insertions(+), 17 deletions(-)
>>>> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
>>>> index 6e5d30dc7b..d9039d37b2 100644
>>>> --- a/xen/arch/arm/arm64/cpufeature.c
>>>> +++ b/xen/arch/arm/arm64/cpufeature.c
>>>> @@ -143,6 +143,16 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>>>> 	ARM64_FTR_END,
>>>> };
>>>> +static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64ISAR2_CLEARBHB_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>>>> +		 FTR_STRICT, FTR_EXACT, ID_AA64ISAR2_APA3_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>>> So we are using CONFIG_ARM64_PTR_AUTH. But this is not defined in Kconfig. I realize there are more in cpufeature.c (somehow I didn't spot during preview), but I don't think this is right to define CONFIG_* without an associated entry in Kconfig.
>>> 
>>> In one hand, I think it would be odd to add an entry in Kconfig because Xen wouldn't properly work if selected. On the other hand, it is useful if when we will implement pointer authentification.
>>> 
>>> So maybe we should just add the Kconfig entry with a comment explaning why they are not selected. Any thoughts?
>> This is really right and a very good catch.
>> I think it would make sense to introduce those in Kconfig in order to keep the code equivalent to Linux.
>> So I would suggest here to add hidden entries like this:
>> ARM64_PTR_AUTH
>> 	def_bool n
>> 	depends on ARM64
>> help
>> Pointer authentication support.
>> This feature is not supported by Xen.
> 
> I am OK with that.

Ok, there are currently 4 CONFIG_ not defined so I will add a patch for those in my serie.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Julien Grall]

Hi Bertrand,

On 04/05/2022 10:49, Bertrand Marquis wrote:
>> On 4 May 2022, at 09:20, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 04/05/2022 08:39, Bertrand Marquis wrote:
>>> Hi Julien,
>> Hi,
>>
>>>> On 3 May 2022, at 19:08, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi Bertrand,
>>>>
>>>> On 03/05/2022 10:38, Bertrand Marquis wrote:
>>>>> Sync arm64 sysreg bit shift definitions with status of Linux kernel as
>>>>> of 5.18-rc3 version (linux commit b2d229d4ddb1).
>>>>> Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
>>>>> sanitization of ISAR2 registers.
>>>> Please outline which specific commits you are actually backported. This would help to know what changed, why and also keep track of the autorships.
>>>>
>>>> When possible, the changes should be separated to match each Linux commit we backport.
>>> As those are exactly identical to the linux tree, one can easily use git blame on the linux source tree to find those information if it is needed
>>
>> Well... that's possible at the cost of everyone going through Linux to understand why the changes were made. This is not very scalable.
>>
>>> I checked a bit and this is not something that was required before (for example when the cpufeature was introduced).
>>
>> If we import the full file, then we will generally don't log all the commits. However, for smaller changes, we will always mention the commit backported. There are several examples on the ML:
>>
>> - 0435784cc75d ("xen/arm: smmuv1: Intelligent SMR allocation")
>> - 9c432b876bf5 ("x86/mwait-idle: add SPR support")
>>
>> We also recently introduced a tag "Origin:" to keep track of which commit was backported. If you want to understand the rationale, you can read this long thread:
>>
>> https://lore.kernel.org/xen-devel/0ed245fa-58a7-a5f6-b82e-48f9ed0b6970@suse.com/
> 
> Do I understand right that it is ok for you if I push one patch mentioning all the commits done in Linux corresponding to the changes (instead of one patch per commit) ?

For this case yes.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/3] xen/arm: Advertise workaround 1 if we apply 3

[Julien Grall]

On 04/05/2022 08:25, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

>> On 3 May 2022, at 19:17, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Bertrand,
>>
>> On 03/05/2022 10:38, Bertrand Marquis wrote:
>>> SMCC_WORKAROUND_3 is handling both Spectre v2 and spectre BHB.
>>> So when a guest is asking if we support workaround 1, tell yes if we
>>> apply workaround 3 on exception entry as it handles it.
>>> This will allow guests not supporting Spectre BHB but impacted by
>>> spectre v2 to still handle it correctly.
>>> The modified behaviour is coherent with what the Linux kernel does in
>>> KVM for guests.
>>> While there use ARM_SMCCC_SUCCESS instead of 0 for the return code value
>>> for workaround detection to be coherent with Workaround 2 handling.
>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Thanks
> 
>>
>> I think we should also consider for backport.
> 
> Agree.

I have committed this patch and added to my list of backport candidate.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Julien Grall]

Hi,

On 04/05/2022 09:06, Julien Grall wrote:
> 
> 
> On 04/05/2022 08:24, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>
>>> On 3 May 2022, at 19:47, Julien Grall <julien@xen.org> wrote:
>>>> A new cpuerrata capability is introduced to enable the alternative
>>>
>>> 'sb' is definitely not an erratum. Errata are for stuff that are 
>>> meant to be specific to one (or multiple) CPU and they are not part 
>>> of the architecture.
>>>
>>> This is the first time we introduce a feature in Xen. So we need to 
>>> add a new array in cpufeature.c that will cover 'SB' for now. In 
>>> future we could add feature like pointer auth, LSE atomics...
>>
>> I am not quite sure why you would want to do that.
>>
>> Using the sb instruction is definitely something to do to solve 
>> erratas, if a CPU is not impacted by those erratas, why would you want 
>> to use this ?
> 
> I agree that SB is used to solve errata but the instruction itself is 
> not a workaround (it may be part of it though). Instead, this is a more 
> efficient way to prevent speculation and will replace dsb/isb.
> 
> Speculation is never going to disappear from processor. So, in the 
> future, there might be valid reason for us to say "We don't want the 
> processor to speculate". This would mean using SB.
> 
>>>> +    .inst 0xd50330ff
>>>> +    nop
>>>
>>> Why do we need the NOP?
>>
>> Alternative requires both sides to have the same size hence the nop to 
>> have 2 instructions as in the if.
> 
> A few years ago we backported a patch from Linux to automatically add 
> nop. However, looking at the code, this would not handle this

Hmmm... Going through my e-mail again. I realized my sentence has not 
been finished. What I was meant to write that because the code is not 
handling it, then adding the extra nop is fine.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Bertrand Marquis]

Hi Julien,

> On 4 May 2022, at 09:06, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 04/05/2022 08:24, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 3 May 2022, at 19:47, Julien Grall <julien@xen.org> wrote:
>>>> A new cpuerrata capability is introduced to enable the alternative
>>> 
>>> 'sb' is definitely not an erratum. Errata are for stuff that are meant to be specific to one (or multiple) CPU and they are not part of the architecture.
>>> 
>>> This is the first time we introduce a feature in Xen. So we need to add a new array in cpufeature.c that will cover 'SB' for now. In future we could add feature like pointer auth, LSE atomics...
>> I am not quite sure why you would want to do that.
>> Using the sb instruction is definitely something to do to solve erratas, if a CPU is not impacted by those erratas, why would you want to use this ?
> 
> I agree that SB is used to solve errata but the instruction itself is not a workaround (it may be part of it though). Instead, this is a more efficient way to prevent speculation and will replace dsb/isb.
> 
> Speculation is never going to disappear from processor. So, in the future, there might be valid reason for us to say "We don't want the processor to speculate". This would mean using SB.

If the need arise then we will check depending on that how we can support it but in the current status as there is no use case I don’t think we need that.

Cheers
Bertrand

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Julien Grall]

On 09/05/2022 11:08, Bertrand Marquis wrote:
> Hi Julien,

Hi,

>> On 4 May 2022, at 09:06, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 04/05/2022 08:24, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi Bertrand,
>>
>>>> On 3 May 2022, at 19:47, Julien Grall <julien@xen.org> wrote:
>>>>> A new cpuerrata capability is introduced to enable the alternative
>>>>
>>>> 'sb' is definitely not an erratum. Errata are for stuff that are meant to be specific to one (or multiple) CPU and they are not part of the architecture.
>>>>
>>>> This is the first time we introduce a feature in Xen. So we need to add a new array in cpufeature.c that will cover 'SB' for now. In future we could add feature like pointer auth, LSE atomics...
>>> I am not quite sure why you would want to do that.
>>> Using the sb instruction is definitely something to do to solve erratas, if a CPU is not impacted by those erratas, why would you want to use this ?
>>
>> I agree that SB is used to solve errata but the instruction itself is not a workaround (it may be part of it though). Instead, this is a more efficient way to prevent speculation and will replace dsb/isb.
>>
>> Speculation is never going to disappear from processor. So, in the future, there might be valid reason for us to say "We don't want the processor to speculate". This would mean using SB.
> 
> If the need arise then we will check depending on that how we can support it but in the current status as there is no use case I don’t think we need that.

It is not clear how I should read this answer... If you add SB in 
cpuerrata.c, then a user will start to see message like:

"enabled workaround for Speculation Barrier".

Which is completely bogus. Replacing "dsb; isb" with "sb" is mostly an 
optimization and none of the current use will end up to be 
architecturaly executed.

I appreciate this is more work to add cpufeature.c. However, AFAIK, 
there are no rush to get this optimization in (see why above) and muddy 
(even temporarily) the logs.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Bertrand Marquis]

Hi Julien,,

> On 9 May 2022, at 11:31, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 09/05/2022 11:08, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi,
> 
>>> On 4 May 2022, at 09:06, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 04/05/2022 08:24, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>>> On 3 May 2022, at 19:47, Julien Grall <julien@xen.org> wrote:
>>>>>> A new cpuerrata capability is introduced to enable the alternative
>>>>> 
>>>>> 'sb' is definitely not an erratum. Errata are for stuff that are meant to be specific to one (or multiple) CPU and they are not part of the architecture.
>>>>> 
>>>>> This is the first time we introduce a feature in Xen. So we need to add a new array in cpufeature.c that will cover 'SB' for now. In future we could add feature like pointer auth, LSE atomics...
>>>> I am not quite sure why you would want to do that.
>>>> Using the sb instruction is definitely something to do to solve erratas, if a CPU is not impacted by those erratas, why would you want to use this ?
>>> 
>>> I agree that SB is used to solve errata but the instruction itself is not a workaround (it may be part of it though). Instead, this is a more efficient way to prevent speculation and will replace dsb/isb.
>>> 
>>> Speculation is never going to disappear from processor. So, in the future, there might be valid reason for us to say "We don't want the processor to speculate". This would mean using SB.
>> If the need arise then we will check depending on that how we can support it but in the current status as there is no use case I don’t think we need that.
> 
> It is not clear how I should read this answer... If you add SB in cpuerrata.c, then a user will start to see message like:
> 
> "enabled workaround for Speculation Barrier".
> 
> Which is completely bogus. Replacing "dsb; isb" with "sb" is mostly an optimization and none of the current use will end up to be architecturaly executed.

So ultimately something like this is what you are looking for ?

diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index e744abe800..7c3e5141a6 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -681,9 +681,12 @@ static const struct arm_cpu_capabilities arm_errata[] = {
         .capability = ARM64_WORKAROUND_AT_SPECULATE,
         MIDR_ALL_VERSIONS(MIDR_CORTEX_A55),
     },
+};
+
+static const struct arm_cpu_capabilities arm_features[] = {
 #ifdef CONFIG_ARM_64
     {
-        .desc = "Speculation barrier (SB)",
+        .desc = "Speculation barrier instruction (SB)",
         .capability = ARM64_HAS_SB,
         .matches = has_sb_instruction,
     },
@@ -694,6 +697,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
 void check_local_cpu_errata(void)
 {
     update_cpu_capabilities(arm_errata, "enabled workaround for");
+    update_cpu_capabilities(arm_features, "enabled support for");
 }

> 
> I appreciate this is more work to add cpufeature.c. However, AFAIK, there are no rush to get this optimization in (see why above) and muddy (even temporarily) the logs.

The upper I am ok to do but if we want to design something new to handle possible features and move this to cpufeature, we will need to step back and check more possible use cases and how we want to handle them.

This is the part which I do not want to handle in this serie.
Point here is to enable the use of the proper instruction when possible on new processors (namely Neoverse N2 at the moment).

Is doing it like this (maybe with a TODO to say that this should be moved to cpufeature) ok for you ?

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Julien Grall]

Hi,

On 09/05/2022 11:49, Bertrand Marquis wrote:
>> On 9 May 2022, at 11:31, Julien Grall <julien@xen.org> wrote:
>> On 09/05/2022 11:08, Bertrand Marquis wrote:
>>>> On 4 May 2022, at 09:06, Julien Grall <julien@xen.org> wrote:
>>>>
>>>>
>>>>
>>>> On 04/05/2022 08:24, Bertrand Marquis wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi Bertrand,
>>>>
>>>>>> On 3 May 2022, at 19:47, Julien Grall <julien@xen.org> wrote:
>>>>>>> A new cpuerrata capability is introduced to enable the alternative
>>>>>>
>>>>>> 'sb' is definitely not an erratum. Errata are for stuff that are meant to be specific to one (or multiple) CPU and they are not part of the architecture.
>>>>>>
>>>>>> This is the first time we introduce a feature in Xen. So we need to add a new array in cpufeature.c that will cover 'SB' for now. In future we could add feature like pointer auth, LSE atomics...
>>>>> I am not quite sure why you would want to do that.
>>>>> Using the sb instruction is definitely something to do to solve erratas, if a CPU is not impacted by those erratas, why would you want to use this ?
>>>>
>>>> I agree that SB is used to solve errata but the instruction itself is not a workaround (it may be part of it though). Instead, this is a more efficient way to prevent speculation and will replace dsb/isb.
>>>>
>>>> Speculation is never going to disappear from processor. So, in the future, there might be valid reason for us to say "We don't want the processor to speculate". This would mean using SB.
>>> If the need arise then we will check depending on that how we can support it but in the current status as there is no use case I don’t think we need that.
>>
>> It is not clear how I should read this answer... If you add SB in cpuerrata.c, then a user will start to see message like:
>>
>> "enabled workaround for Speculation Barrier".
>>
>> Which is completely bogus. Replacing "dsb; isb" with "sb" is mostly an optimization and none of the current use will end up to be architecturaly executed.
> 
> So ultimately something like this is what you are looking for ?
> 
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index e744abe800..7c3e5141a6 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -681,9 +681,12 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>           .capability = ARM64_WORKAROUND_AT_SPECULATE,
>           MIDR_ALL_VERSIONS(MIDR_CORTEX_A55),
>       },
> +};
> +
> +static const struct arm_cpu_capabilities arm_features[] = {
>   #ifdef CONFIG_ARM_64
>       {
> -        .desc = "Speculation barrier (SB)",
> +        .desc = "Speculation barrier instruction (SB)",
>           .capability = ARM64_HAS_SB,
>           .matches = has_sb_instruction,
>       },
> @@ -694,6 +697,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>   void check_local_cpu_errata(void)
>   {
>       update_cpu_capabilities(arm_errata, "enabled workaround for");
> +    update_cpu_capabilities(arm_features, "enabled support for");
>   }
What I am looking for is two separate arrays: one for workaround and the 
other for features. Something like (untested):

diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index a58965f7b9bf..54c10751dba8 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -70,6 +70,20 @@ void __init enable_cpu_capabilities(const struct 
arm_cpu_capabilities *caps)
      }
  }

+static const struct arm_cpu_capabilities arm_features[] = {
+    /* XXX: Add SB */
+    {},
+};
+
+void check_local_cpu_features(void)
+{
+    update_cpu_capabilities(arm_features, "enabled support for");
+}
+
+void __init enable_cpu_features(void)
+{
+    enable_cpu_capabilities(arm_features);
+}
+
  /*
   * Run through the enabled capabilities and enable() them on the 
calling CPU.
   * If enabling of any capability fails the error is returned. After 
enabling a
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d5d0792ed48a..c2cd442844df 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -951,6 +951,7 @@ void __init start_xen(unsigned long boot_phys_offset,
       * (called from smp_init_cpus()).
       */
      check_local_cpu_errata();
+    check_local_cpu_features();

      init_xen_time();

@@ -1021,6 +1022,7 @@ void __init start_xen(unsigned long boot_phys_offset,
       */
      apply_alternatives_all();
      enable_errata_workarounds();
+    enable_cpu_features();

      /* Create initial domain 0. */
      if ( !is_dom0less_mode() )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 7bfd0a73a7d2..d6b8c598df98 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -383,6 +383,7 @@ void start_secondary(void)
      local_abort_enable();

      check_local_cpu_errata();
+    check_local_cpu_features();

      printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());

Cheers,

-- 
Julien Grall

Re: [PATCH 3/3] xen/arm: Add sb instruction support

[Bertrand Marquis]

Hi,

> On 9 May 2022, at 12:08, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 09/05/2022 11:49, Bertrand Marquis wrote:
>>> On 9 May 2022, at 11:31, Julien Grall <julien@xen.org> wrote:
>>> On 09/05/2022 11:08, Bertrand Marquis wrote:
>>>>> On 4 May 2022, at 09:06, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 04/05/2022 08:24, Bertrand Marquis wrote:
>>>>>> Hi Julien,
>>>>> 
>>>>> Hi Bertrand,
>>>>> 
>>>>>>> On 3 May 2022, at 19:47, Julien Grall <julien@xen.org> wrote:
>>>>>>>> A new cpuerrata capability is introduced to enable the alternative
>>>>>>> 
>>>>>>> 'sb' is definitely not an erratum. Errata are for stuff that are meant to be specific to one (or multiple) CPU and they are not part of the architecture.
>>>>>>> 
>>>>>>> This is the first time we introduce a feature in Xen. So we need to add a new array in cpufeature.c that will cover 'SB' for now. In future we could add feature like pointer auth, LSE atomics...
>>>>>> I am not quite sure why you would want to do that.
>>>>>> Using the sb instruction is definitely something to do to solve erratas, if a CPU is not impacted by those erratas, why would you want to use this ?
>>>>> 
>>>>> I agree that SB is used to solve errata but the instruction itself is not a workaround (it may be part of it though). Instead, this is a more efficient way to prevent speculation and will replace dsb/isb.
>>>>> 
>>>>> Speculation is never going to disappear from processor. So, in the future, there might be valid reason for us to say "We don't want the processor to speculate". This would mean using SB.
>>>> If the need arise then we will check depending on that how we can support it but in the current status as there is no use case I don’t think we need that.
>>> 
>>> It is not clear how I should read this answer... If you add SB in cpuerrata.c, then a user will start to see message like:
>>> 
>>> "enabled workaround for Speculation Barrier".
>>> 
>>> Which is completely bogus. Replacing "dsb; isb" with "sb" is mostly an optimization and none of the current use will end up to be architecturaly executed.
>> So ultimately something like this is what you are looking for ?
>> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
>> index e744abe800..7c3e5141a6 100644
>> --- a/xen/arch/arm/cpuerrata.c
>> +++ b/xen/arch/arm/cpuerrata.c
>> @@ -681,9 +681,12 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>>          .capability = ARM64_WORKAROUND_AT_SPECULATE,
>>          MIDR_ALL_VERSIONS(MIDR_CORTEX_A55),
>>      },
>> +};
>> +
>> +static const struct arm_cpu_capabilities arm_features[] = {
>>  #ifdef CONFIG_ARM_64
>>      {
>> -        .desc = "Speculation barrier (SB)",
>> +        .desc = "Speculation barrier instruction (SB)",
>>          .capability = ARM64_HAS_SB,
>>          .matches = has_sb_instruction,
>>      },
>> @@ -694,6 +697,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>>  void check_local_cpu_errata(void)
>>  {
>>      update_cpu_capabilities(arm_errata, "enabled workaround for");
>> +    update_cpu_capabilities(arm_features, "enabled support for");
>>  }
> What I am looking for is two separate arrays: one for workaround and the other for features. Something like (untested):
> 
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index a58965f7b9bf..54c10751dba8 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -70,6 +70,20 @@ void __init enable_cpu_capabilities(const struct arm_cpu_capabilities *caps)
>     }
> }
> 
> +static const struct arm_cpu_capabilities arm_features[] = {
> +    /* XXX: Add SB */
> +    {},
> +};
> +
> +void check_local_cpu_features(void)
> +{
> +    update_cpu_capabilities(arm_features, "enabled support for");
> +}
> +
> +void __init enable_cpu_features(void)
> +{
> +    enable_cpu_capabilities(arm_features);
> +}
> +
> /*
>  * Run through the enabled capabilities and enable() them on the calling CPU.
>  * If enabling of any capability fails the error is returned. After enabling a
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index d5d0792ed48a..c2cd442844df 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -951,6 +951,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>      * (called from smp_init_cpus()).
>      */
>     check_local_cpu_errata();
> +    check_local_cpu_features();
> 
>     init_xen_time();
> 
> @@ -1021,6 +1022,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>      */
>     apply_alternatives_all();
>     enable_errata_workarounds();
> +    enable_cpu_features();
> 
>     /* Create initial domain 0. */
>     if ( !is_dom0less_mode() )
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 7bfd0a73a7d2..d6b8c598df98 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -383,6 +383,7 @@ void start_secondary(void)
>     local_abort_enable();
> 
>     check_local_cpu_errata();
> +    check_local_cpu_features();
> 
>     printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());

Thanks for the code, I get the idea and will do that.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Stefano Stabellini]

On Wed, 4 May 2022, Julien Grall wrote:
> > Do I understand right that it is ok for you if I push one patch mentioning
> > all the commits done in Linux corresponding to the changes (instead of one
> > patch per commit) ?
> 
> For this case yes.

I managed to do a review of the patch by doing a diff of the relevant
portion of Xen cpufeature.c with Linux cpufeature.c (from commit
b2d229d4ddb1), and the relevant portion of Xen sysregs.h with Linux
sysregs.h (diff -E -b -u).

Everything checks out.

In my opinion, this patch should be split in 2 patches: the changes to
cpufeature.c and sysregs.c that come from the Linux sources; and the
updates to cpufeature.h that do not. If you do that you can add my
reviewed-by to the first patch with the changes from Linux.

The list of individual commit IDs would be nice, but thanksfully the two
source files are still "diffable" so in my opinion are not required.

I have a couple of comments on the changes to cpufeature.h (the ones not
from Linux) which I'll reply directly to the patch.

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Stefano Stabellini]

On Tue, 3 May 2022, Bertrand Marquis wrote:
> Sync arm64 sysreg bit shift definitions with status of Linux kernel as
> of 5.18-rc3 version (linux commit b2d229d4ddb1).
> Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
> sanitization of ISAR2 registers.
> Complete AA64ISAR2 and AA64MMFR1 with more fields.
> While there add a comment for MMFR bitfields as for other registers in
> the cpuinfo structure definition.
> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
>  xen/arch/arm/arm64/cpufeature.c          | 18 +++++-
>  xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----

Linux cpufeature.c has a couple more structures compared to Xen. So I
would add the word "existing" in the commit message:

"Sync existing ID registers sanitization with the status of Linux
5.18-rc3 and add sanitization of ISAR2 registers."

A couple of comments about the cpufeature.h changes below.


>  xen/arch/arm/include/asm/cpufeature.h    | 14 ++++-
>  3 files changed, 91 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
> index 6e5d30dc7b..d9039d37b2 100644
> --- a/xen/arch/arm/arm64/cpufeature.c
> +++ b/xen/arch/arm/arm64/cpufeature.c
> @@ -143,6 +143,16 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>  	ARM64_FTR_END,
>  };
>  
> +static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64ISAR2_CLEARBHB_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
> +		       FTR_STRICT, FTR_EXACT, ID_AA64ISAR2_APA3_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
> +		       FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR2_GPA3_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64ISAR2_RPRES_SHIFT, 4, 0),
> +	ARM64_FTR_END,
> +};
> +
>  static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_CSV3_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_CSV2_SHIFT, 4, 0),
> @@ -158,8 +168,8 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
>  	S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL3_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY),
> -	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_ELx_64BIT_ONLY),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_ELx_64BIT_ONLY),
>  	ARM64_FTR_END,
>  };
>  
> @@ -197,7 +207,7 @@ static const struct arm64_ftr_bits ftr_id_aa64zfr0[] = {
>  };
>  
>  static const struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
> -	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_ECV_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_ECV_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_FGT_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_EXS_SHIFT, 4, 0),
>  	/*
> @@ -243,6 +253,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
>  };
>  
>  static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
> +	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_AFP_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_ETS_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_TWED_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_XNX_SHIFT, 4, 0),
> @@ -588,6 +599,7 @@ void update_system_features(const struct cpuinfo_arm *new)
>  
>  	SANITIZE_ID_REG(isa64, 0, aa64isar0);
>  	SANITIZE_ID_REG(isa64, 1, aa64isar1);
> +	SANITIZE_ID_REG(isa64, 2, aa64isar2);
>  
>  	SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
>  
> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
> index eac08ed33f..54670084c3 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -144,6 +144,30 @@
>  
>  /* id_aa64isar2 */
>  #define ID_AA64ISAR2_CLEARBHB_SHIFT 28
> +#define ID_AA64ISAR2_APA3_SHIFT     12
> +#define ID_AA64ISAR2_GPA3_SHIFT     8
> +#define ID_AA64ISAR2_RPRES_SHIFT    4
> +#define ID_AA64ISAR2_WFXT_SHIFT     0
> +
> +#define ID_AA64ISAR2_RPRES_8BIT     0x0
> +#define ID_AA64ISAR2_RPRES_12BIT    0x1
> +/*
> + * Value 0x1 has been removed from the architecture, and is
> + * reserved, but has not yet been removed from the ARM ARM
> + * as of ARM DDI 0487G.b.
> + */
> +#define ID_AA64ISAR2_WFXT_NI        0x0
> +#define ID_AA64ISAR2_WFXT_SUPPORTED 0x2
> +
> +#define ID_AA64ISAR2_APA3_NI                  0x0
> +#define ID_AA64ISAR2_APA3_ARCHITECTED         0x1
> +#define ID_AA64ISAR2_APA3_ARCH_EPAC           0x2
> +#define ID_AA64ISAR2_APA3_ARCH_EPAC2          0x3
> +#define ID_AA64ISAR2_APA3_ARCH_EPAC2_FPAC     0x4
> +#define ID_AA64ISAR2_APA3_ARCH_EPAC2_FPAC_CMB 0x5
> +
> +#define ID_AA64ISAR2_GPA3_NI             0x0
> +#define ID_AA64ISAR2_GPA3_ARCHITECTED    0x1
>  
>  /* id_aa64pfr0 */
>  #define ID_AA64PFR0_CSV3_SHIFT       60
> @@ -165,14 +189,13 @@
>  #define ID_AA64PFR0_AMU              0x1
>  #define ID_AA64PFR0_SVE              0x1
>  #define ID_AA64PFR0_RAS_V1           0x1
> +#define ID_AA64PFR0_RAS_V1P1         0x2
>  #define ID_AA64PFR0_FP_NI            0xf
>  #define ID_AA64PFR0_FP_SUPPORTED     0x0
>  #define ID_AA64PFR0_ASIMD_NI         0xf
>  #define ID_AA64PFR0_ASIMD_SUPPORTED  0x0
> -#define ID_AA64PFR0_EL1_64BIT_ONLY   0x1
> -#define ID_AA64PFR0_EL1_32BIT_64BIT  0x2
> -#define ID_AA64PFR0_EL0_64BIT_ONLY   0x1
> -#define ID_AA64PFR0_EL0_32BIT_64BIT  0x2
> +#define ID_AA64PFR0_ELx_64BIT_ONLY   0x1
> +#define ID_AA64PFR0_ELx_32BIT_64BIT  0x2
>  
>  /* id_aa64pfr1 */
>  #define ID_AA64PFR1_MPAMFRAC_SHIFT   16
> @@ -189,6 +212,7 @@
>  #define ID_AA64PFR1_MTE_NI           0x0
>  #define ID_AA64PFR1_MTE_EL0          0x1
>  #define ID_AA64PFR1_MTE              0x2
> +#define ID_AA64PFR1_MTE_ASYMM        0x3
>  
>  /* id_aa64zfr0 */
>  #define ID_AA64ZFR0_F64MM_SHIFT      56
> @@ -228,17 +252,37 @@
>  #define ID_AA64MMFR0_ASID_SHIFT      4
>  #define ID_AA64MMFR0_PARANGE_SHIFT   0
>  
> -#define ID_AA64MMFR0_TGRAN4_NI         0xf
> -#define ID_AA64MMFR0_TGRAN4_SUPPORTED  0x0
> -#define ID_AA64MMFR0_TGRAN64_NI        0xf
> -#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
> -#define ID_AA64MMFR0_TGRAN16_NI        0x0
> -#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
> +#define ID_AA64MMFR0_ASID_8          0x0
> +#define ID_AA64MMFR0_ASID_16         0x2
> +
> +#define ID_AA64MMFR0_TGRAN4_NI             0xf
> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED_MIN  0x0
> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED_MAX  0x7
> +#define ID_AA64MMFR0_TGRAN64_NI            0xf
> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED_MIN 0x0
> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED_MAX 0x7
> +#define ID_AA64MMFR0_TGRAN16_NI            0x0
> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED_MIN 0x1
> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED_MAX 0xf
> +
> +#define ID_AA64MMFR0_PARANGE_32        0x0
> +#define ID_AA64MMFR0_PARANGE_36        0x1
> +#define ID_AA64MMFR0_PARANGE_40        0x2
> +#define ID_AA64MMFR0_PARANGE_42        0x3
> +#define ID_AA64MMFR0_PARANGE_44        0x4
>  #define ID_AA64MMFR0_PARANGE_48        0x5
>  #define ID_AA64MMFR0_PARANGE_52        0x6
>  
> +#define ARM64_MIN_PARANGE_BITS     32
> +
> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_DEFAULT 0x0
> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_NONE    0x1
> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_MIN     0x2
> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_MAX     0x7
> +
>  /* id_aa64mmfr1 */
>  #define ID_AA64MMFR1_ECBHB_SHIFT     60
> +#define ID_AA64MMFR1_AFP_SHIFT       44
>  #define ID_AA64MMFR1_ETS_SHIFT       36
>  #define ID_AA64MMFR1_TWED_SHIFT      32
>  #define ID_AA64MMFR1_XNX_SHIFT       28
> @@ -271,6 +315,9 @@
>  #define ID_AA64MMFR2_CNP_SHIFT       0
>  
>  /* id_aa64dfr0 */
> +#define ID_AA64DFR0_MTPMU_SHIFT      48
> +#define ID_AA64DFR0_TRBE_SHIFT       44
> +#define ID_AA64DFR0_TRACE_FILT_SHIFT 40
>  #define ID_AA64DFR0_DOUBLELOCK_SHIFT 36
>  #define ID_AA64DFR0_PMSVER_SHIFT     32
>  #define ID_AA64DFR0_CTX_CMPS_SHIFT   28
> @@ -284,11 +331,18 @@
>  #define ID_AA64DFR0_PMUVER_8_1       0x4
>  #define ID_AA64DFR0_PMUVER_8_4       0x5
>  #define ID_AA64DFR0_PMUVER_8_5       0x6
> +#define ID_AA64DFR0_PMUVER_8_7       0x7
>  #define ID_AA64DFR0_PMUVER_IMP_DEF   0xf
>  
> +#define ID_AA64DFR0_PMSVER_8_2      0x1
> +#define ID_AA64DFR0_PMSVER_8_3      0x2
> +
>  #define ID_DFR0_PERFMON_SHIFT        24
>  
> -#define ID_DFR0_PERFMON_8_1          0x4
> +#define ID_DFR0_PERFMON_8_0         0x3
> +#define ID_DFR0_PERFMON_8_1         0x4
> +#define ID_DFR0_PERFMON_8_4         0x5
> +#define ID_DFR0_PERFMON_8_5         0x6
>  
>  #define ID_ISAR4_SWP_FRAC_SHIFT        28
>  #define ID_ISAR4_PSR_M_SHIFT           24
> diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
> index f7368766c0..4719de47f3 100644
> --- a/xen/arch/arm/include/asm/cpufeature.h
> +++ b/xen/arch/arm/include/asm/cpufeature.h
> @@ -230,6 +230,7 @@ struct cpuinfo_arm {
>      union {
>          register_t bits[3];
>          struct {
> +            /* MMFR0 */
>              unsigned long pa_range:4;
>              unsigned long asid_bits:4;
>              unsigned long bigend:4;
> @@ -240,16 +241,21 @@ struct cpuinfo_arm {
>              unsigned long tgranule_4K:4;
>              unsigned long __res0:32;
>  
> +            /* MMFR1 */
>              unsigned long hafdbs:4;
>              unsigned long vmid_bits:4;
>              unsigned long vh:4;
>              unsigned long hpds:4;
>              unsigned long lo:4;
>              unsigned long pan:4;
> -            unsigned long __res1:8;
> -            unsigned long __res2:28;
> +            unsigned long specsei:4;
> +            unsigned long xnx:4;
> +            unsigned long twed:4;
> +            unsigned long ets:4;
> +            unsigned long __res1:20;

We might as well complete the fields by also adding hcx, afp, ntlbpa,
tidcp1, cmow. What do you think?


>              unsigned long ecbhb:4;
>  
> +            /* MMFR2 */
>              unsigned long __res3:64;
>          };
>      } mm64;
> @@ -293,7 +299,9 @@ struct cpuinfo_arm {
>              unsigned long __res2:8;
>  
>              /* ISAR2 */
> -            unsigned long __res3:28;
> +            unsigned long wfxt:4;
> +            unsigned long rpres:4;
> +            unsigned long __res3:20;

Also here we can add gpa3, apa3, mops, bc, and pac_frac?


>              unsigned long clearbhb:4;
>  
>              unsigned long __res4:32;

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Bertrand Marquis]

> On 10 May 2022, at 03:03, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 4 May 2022, Julien Grall wrote:
>>> Do I understand right that it is ok for you if I push one patch mentioning
>>> all the commits done in Linux corresponding to the changes (instead of one
>>> patch per commit) ?
>> 
>> For this case yes.
> 
> I managed to do a review of the patch by doing a diff of the relevant
> portion of Xen cpufeature.c with Linux cpufeature.c (from commit
> b2d229d4ddb1), and the relevant portion of Xen sysregs.h with Linux
> sysregs.h (diff -E -b -u).
> 
> Everything checks out.
> 
> In my opinion, this patch should be split in 2 patches: the changes to
> cpufeature.c and sysregs.c that come from the Linux sources; and the
> updates to cpufeature.h that do not. If you do that you can add my
> reviewed-by to the first patch with the changes from Linux.
> 
> The list of individual commit IDs would be nice, but thanksfully the two
> source files are still "diffable" so in my opinion are not required.

I agree with that.

Julien: Do you agree if I just put the changes to cpufeature.h in a separate patch ?

I started to list the commit IDs corresponding to the changes in Linux and this would
end up with 5 or more which I do not think would be that useful as the diff can be easily
done as Stefano mentioned.

> 
> I have a couple of comments on the changes to cpufeature.h (the ones not
> from Linux) which I'll reply directly to the patch.

I will answer to them.

Cheers
Bertrand

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Bertrand Marquis]

Hi Stefano,

> On 10 May 2022, at 03:04, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 3 May 2022, Bertrand Marquis wrote:
>> Sync arm64 sysreg bit shift definitions with status of Linux kernel as
>> of 5.18-rc3 version (linux commit b2d229d4ddb1).
>> Sync ID registers sanitization with the status of Linux 5.18-rc3 and add
>> sanitization of ISAR2 registers.
>> Complete AA64ISAR2 and AA64MMFR1 with more fields.
>> While there add a comment for MMFR bitfields as for other registers in
>> the cpuinfo structure definition.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> xen/arch/arm/arm64/cpufeature.c | 18 +++++-
>> xen/arch/arm/include/asm/arm64/sysregs.h | 76 ++++++++++++++++++++----
> 
> Linux cpufeature.c has a couple more structures compared to Xen. So I
> would add the word "existing" in the commit message:
> 
> "Sync existing ID registers sanitization with the status of Linux
> 5.18-rc3 and add sanitization of ISAR2 registers."

Ok will do.

> 
> A couple of comments about the cpufeature.h changes below.
> 
> 
>> xen/arch/arm/include/asm/cpufeature.h | 14 ++++-
>> 3 files changed, 91 insertions(+), 17 deletions(-)
>> 
>> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
>> index 6e5d30dc7b..d9039d37b2 100644
>> --- a/xen/arch/arm/arm64/cpufeature.c
>> +++ b/xen/arch/arm/arm64/cpufeature.c
>> @@ -143,6 +143,16 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>> 	ARM64_FTR_END,
>> };
>> 
>> +static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64ISAR2_CLEARBHB_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>> +		 FTR_STRICT, FTR_EXACT, ID_AA64ISAR2_APA3_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>> +		 FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR2_GPA3_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64ISAR2_RPRES_SHIFT, 4, 0),
>> +	ARM64_FTR_END,
>> +};
>> +
>> static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_CSV3_SHIFT, 4, 0),
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_CSV2_SHIFT, 4, 0),
>> @@ -158,8 +168,8 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
>> 	S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI),
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL3_SHIFT, 4, 0),
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0),
>> -	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY),
>> -	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_ELx_64BIT_ONLY),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_ELx_64BIT_ONLY),
>> 	ARM64_FTR_END,
>> };
>> 
>> @@ -197,7 +207,7 @@ static const struct arm64_ftr_bits ftr_id_aa64zfr0[] = {
>> };
>> 
>> static const struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
>> -	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_ECV_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_ECV_SHIFT, 4, 0),
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_FGT_SHIFT, 4, 0),
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_EXS_SHIFT, 4, 0),
>> 	/*
>> @@ -243,6 +253,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
>> };
>> 
>> static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
>> +	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_AFP_SHIFT, 4, 0),
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_ETS_SHIFT, 4, 0),
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_TWED_SHIFT, 4, 0),
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR1_XNX_SHIFT, 4, 0),
>> @@ -588,6 +599,7 @@ void update_system_features(const struct cpuinfo_arm *new)
>> 
>> 	SANITIZE_ID_REG(isa64, 0, aa64isar0);
>> 	SANITIZE_ID_REG(isa64, 1, aa64isar1);
>> +	SANITIZE_ID_REG(isa64, 2, aa64isar2);
>> 
>> 	SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
>> 
>> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
>> index eac08ed33f..54670084c3 100644
>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>> @@ -144,6 +144,30 @@
>> 
>> /* id_aa64isar2 */
>> #define ID_AA64ISAR2_CLEARBHB_SHIFT 28
>> +#define ID_AA64ISAR2_APA3_SHIFT 12
>> +#define ID_AA64ISAR2_GPA3_SHIFT 8
>> +#define ID_AA64ISAR2_RPRES_SHIFT 4
>> +#define ID_AA64ISAR2_WFXT_SHIFT 0
>> +
>> +#define ID_AA64ISAR2_RPRES_8BIT 0x0
>> +#define ID_AA64ISAR2_RPRES_12BIT 0x1
>> +/*
>> + * Value 0x1 has been removed from the architecture, and is
>> + * reserved, but has not yet been removed from the ARM ARM
>> + * as of ARM DDI 0487G.b.
>> + */
>> +#define ID_AA64ISAR2_WFXT_NI 0x0
>> +#define ID_AA64ISAR2_WFXT_SUPPORTED 0x2
>> +
>> +#define ID_AA64ISAR2_APA3_NI 0x0
>> +#define ID_AA64ISAR2_APA3_ARCHITECTED 0x1
>> +#define ID_AA64ISAR2_APA3_ARCH_EPAC 0x2
>> +#define ID_AA64ISAR2_APA3_ARCH_EPAC2 0x3
>> +#define ID_AA64ISAR2_APA3_ARCH_EPAC2_FPAC 0x4
>> +#define ID_AA64ISAR2_APA3_ARCH_EPAC2_FPAC_CMB 0x5
>> +
>> +#define ID_AA64ISAR2_GPA3_NI 0x0
>> +#define ID_AA64ISAR2_GPA3_ARCHITECTED 0x1
>> 
>> /* id_aa64pfr0 */
>> #define ID_AA64PFR0_CSV3_SHIFT 60
>> @@ -165,14 +189,13 @@
>> #define ID_AA64PFR0_AMU 0x1
>> #define ID_AA64PFR0_SVE 0x1
>> #define ID_AA64PFR0_RAS_V1 0x1
>> +#define ID_AA64PFR0_RAS_V1P1 0x2
>> #define ID_AA64PFR0_FP_NI 0xf
>> #define ID_AA64PFR0_FP_SUPPORTED 0x0
>> #define ID_AA64PFR0_ASIMD_NI 0xf
>> #define ID_AA64PFR0_ASIMD_SUPPORTED 0x0
>> -#define ID_AA64PFR0_EL1_64BIT_ONLY 0x1
>> -#define ID_AA64PFR0_EL1_32BIT_64BIT 0x2
>> -#define ID_AA64PFR0_EL0_64BIT_ONLY 0x1
>> -#define ID_AA64PFR0_EL0_32BIT_64BIT 0x2
>> +#define ID_AA64PFR0_ELx_64BIT_ONLY 0x1
>> +#define ID_AA64PFR0_ELx_32BIT_64BIT 0x2
>> 
>> /* id_aa64pfr1 */
>> #define ID_AA64PFR1_MPAMFRAC_SHIFT 16
>> @@ -189,6 +212,7 @@
>> #define ID_AA64PFR1_MTE_NI 0x0
>> #define ID_AA64PFR1_MTE_EL0 0x1
>> #define ID_AA64PFR1_MTE 0x2
>> +#define ID_AA64PFR1_MTE_ASYMM 0x3
>> 
>> /* id_aa64zfr0 */
>> #define ID_AA64ZFR0_F64MM_SHIFT 56
>> @@ -228,17 +252,37 @@
>> #define ID_AA64MMFR0_ASID_SHIFT 4
>> #define ID_AA64MMFR0_PARANGE_SHIFT 0
>> 
>> -#define ID_AA64MMFR0_TGRAN4_NI 0xf
>> -#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
>> -#define ID_AA64MMFR0_TGRAN64_NI 0xf
>> -#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
>> -#define ID_AA64MMFR0_TGRAN16_NI 0x0
>> -#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
>> +#define ID_AA64MMFR0_ASID_8 0x0
>> +#define ID_AA64MMFR0_ASID_16 0x2
>> +
>> +#define ID_AA64MMFR0_TGRAN4_NI 0xf
>> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED_MIN 0x0
>> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED_MAX 0x7
>> +#define ID_AA64MMFR0_TGRAN64_NI 0xf
>> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED_MIN 0x0
>> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED_MAX 0x7
>> +#define ID_AA64MMFR0_TGRAN16_NI 0x0
>> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED_MIN 0x1
>> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED_MAX 0xf
>> +
>> +#define ID_AA64MMFR0_PARANGE_32 0x0
>> +#define ID_AA64MMFR0_PARANGE_36 0x1
>> +#define ID_AA64MMFR0_PARANGE_40 0x2
>> +#define ID_AA64MMFR0_PARANGE_42 0x3
>> +#define ID_AA64MMFR0_PARANGE_44 0x4
>> #define ID_AA64MMFR0_PARANGE_48 0x5
>> #define ID_AA64MMFR0_PARANGE_52 0x6
>> 
>> +#define ARM64_MIN_PARANGE_BITS 32
>> +
>> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_DEFAULT 0x0
>> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_NONE 0x1
>> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_MIN 0x2
>> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_MAX 0x7
>> +
>> /* id_aa64mmfr1 */
>> #define ID_AA64MMFR1_ECBHB_SHIFT 60
>> +#define ID_AA64MMFR1_AFP_SHIFT 44
>> #define ID_AA64MMFR1_ETS_SHIFT 36
>> #define ID_AA64MMFR1_TWED_SHIFT 32
>> #define ID_AA64MMFR1_XNX_SHIFT 28
>> @@ -271,6 +315,9 @@
>> #define ID_AA64MMFR2_CNP_SHIFT 0
>> 
>> /* id_aa64dfr0 */
>> +#define ID_AA64DFR0_MTPMU_SHIFT 48
>> +#define ID_AA64DFR0_TRBE_SHIFT 44
>> +#define ID_AA64DFR0_TRACE_FILT_SHIFT 40
>> #define ID_AA64DFR0_DOUBLELOCK_SHIFT 36
>> #define ID_AA64DFR0_PMSVER_SHIFT 32
>> #define ID_AA64DFR0_CTX_CMPS_SHIFT 28
>> @@ -284,11 +331,18 @@
>> #define ID_AA64DFR0_PMUVER_8_1 0x4
>> #define ID_AA64DFR0_PMUVER_8_4 0x5
>> #define ID_AA64DFR0_PMUVER_8_5 0x6
>> +#define ID_AA64DFR0_PMUVER_8_7 0x7
>> #define ID_AA64DFR0_PMUVER_IMP_DEF 0xf
>> 
>> +#define ID_AA64DFR0_PMSVER_8_2 0x1
>> +#define ID_AA64DFR0_PMSVER_8_3 0x2
>> +
>> #define ID_DFR0_PERFMON_SHIFT 24
>> 
>> -#define ID_DFR0_PERFMON_8_1 0x4
>> +#define ID_DFR0_PERFMON_8_0 0x3
>> +#define ID_DFR0_PERFMON_8_1 0x4
>> +#define ID_DFR0_PERFMON_8_4 0x5
>> +#define ID_DFR0_PERFMON_8_5 0x6
>> 
>> #define ID_ISAR4_SWP_FRAC_SHIFT 28
>> #define ID_ISAR4_PSR_M_SHIFT 24
>> diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
>> index f7368766c0..4719de47f3 100644
>> --- a/xen/arch/arm/include/asm/cpufeature.h
>> +++ b/xen/arch/arm/include/asm/cpufeature.h
>> @@ -230,6 +230,7 @@ struct cpuinfo_arm {
>> union {
>> register_t bits[3];
>> struct {
>> + /* MMFR0 */
>> unsigned long pa_range:4;
>> unsigned long asid_bits:4;
>> unsigned long bigend:4;
>> @@ -240,16 +241,21 @@ struct cpuinfo_arm {
>> unsigned long tgranule_4K:4;
>> unsigned long __res0:32;
>> 
>> + /* MMFR1 */
>> unsigned long hafdbs:4;
>> unsigned long vmid_bits:4;
>> unsigned long vh:4;
>> unsigned long hpds:4;
>> unsigned long lo:4;
>> unsigned long pan:4;
>> - unsigned long __res1:8;
>> - unsigned long __res2:28;
>> + unsigned long specsei:4;
>> + unsigned long xnx:4;
>> + unsigned long twed:4;
>> + unsigned long ets:4;
>> + unsigned long __res1:20;
> 
> We might as well complete the fields by also adding hcx, afp, ntlbpa,
> tidcp1, cmow. What do you think?

Ok.

> 
> 
>> unsigned long ecbhb:4;
>> 
>> + /* MMFR2 */
>> unsigned long __res3:64;
>> };
>> } mm64;
>> @@ -293,7 +299,9 @@ struct cpuinfo_arm {
>> unsigned long __res2:8;
>> 
>> /* ISAR2 */
>> - unsigned long __res3:28;
>> + unsigned long wfxt:4;
>> + unsigned long rpres:4;
>> + unsigned long __res3:20;
> 
> Also here we can add gpa3, apa3, mops, bc, and pac_frac?

Ok.

Cheers
Bertrand

> 
> 
>> unsigned long clearbhb:4;
>> 
>> unsigned long __res4:32;

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Julien Grall]

Hi,

On 11/05/2022 15:41, Bertrand Marquis wrote:
> 
> 
>> On 10 May 2022, at 03:03, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> On Wed, 4 May 2022, Julien Grall wrote:
>>>> Do I understand right that it is ok for you if I push one patch mentioning
>>>> all the commits done in Linux corresponding to the changes (instead of one
>>>> patch per commit) ?
>>>
>>> For this case yes.
>>
>> I managed to do a review of the patch by doing a diff of the relevant
>> portion of Xen cpufeature.c with Linux cpufeature.c (from commit
>> b2d229d4ddb1), and the relevant portion of Xen sysregs.h with Linux
>> sysregs.h (diff -E -b -u).
>>
>> Everything checks out.
>>
>> In my opinion, this patch should be split in 2 patches: the changes to
>> cpufeature.c and sysregs.c that come from the Linux sources; and the
>> updates to cpufeature.h that do not. If you do that you can add my
>> reviewed-by to the first patch with the changes from Linux.
>>
>> The list of individual commit IDs would be nice, but thanksfully the two
>> source files are still "diffable" so in my opinion are not required.
> 
> I agree with that.
> 
> Julien: Do you agree if I just put the changes to cpufeature.h in a separate patch ?
> 
> I started to list the commit IDs corresponding to the changes in Linux and this would
> end up with 5 or more which I do not think would be that useful as the diff can be easily
> done as Stefano mentioned.

It looks like there are some confusion why I asked the list of commit. 
For this case, this is not about diffing the code (it is easy to do and 
I have already done that). It is more about authorship and where the 
patches come from.

Technically, speaking you only copied the code from Linux and therefore 
you are not the author of some of the changes.

For such case, our general process is:
   1) Backport the commit as-is (i.e the Author is the original Author)
   2) Add the tag Origin (recently introduced)
   3) Add your signed-off-by

I understand the patch is already written, so I was OK if you simply 
list of the commits with the authors/tags for this time.

If both Stefano and you agree to not keep the authorships, then I will 
not stand against it. However, I will not get involved in committing and 
adding my ack.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Bertrand Marquis]

Hi Julien,

> On 11 May 2022, at 16:20, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 11/05/2022 15:41, Bertrand Marquis wrote:
>>> On 10 May 2022, at 03:03, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Wed, 4 May 2022, Julien Grall wrote:
>>>>> Do I understand right that it is ok for you if I push one patch mentioning
>>>>> all the commits done in Linux corresponding to the changes (instead of one
>>>>> patch per commit) ?
>>>> 
>>>> For this case yes.
>>> 
>>> I managed to do a review of the patch by doing a diff of the relevant
>>> portion of Xen cpufeature.c with Linux cpufeature.c (from commit
>>> b2d229d4ddb1), and the relevant portion of Xen sysregs.h with Linux
>>> sysregs.h (diff -E -b -u).
>>> 
>>> Everything checks out.
>>> 
>>> In my opinion, this patch should be split in 2 patches: the changes to
>>> cpufeature.c and sysregs.c that come from the Linux sources; and the
>>> updates to cpufeature.h that do not. If you do that you can add my
>>> reviewed-by to the first patch with the changes from Linux.
>>> 
>>> The list of individual commit IDs would be nice, but thanksfully the two
>>> source files are still "diffable" so in my opinion are not required.
>> I agree with that.
>> Julien: Do you agree if I just put the changes to cpufeature.h in a separate patch ?
>> I started to list the commit IDs corresponding to the changes in Linux and this would
>> end up with 5 or more which I do not think would be that useful as the diff can be easily
>> done as Stefano mentioned.
> 
> It looks like there are some confusion why I asked the list of commit. For this case, this is not about diffing the code (it is easy to do and I have already done that). It is more about authorship and where the patches come from.

This is clear from the commit message as I give the commit in Linux used so the history can be easily found from that.
I am a bit lost on the authorship part ...

> 
> Technically, speaking you only copied the code from Linux and therefore you are not the author of some of the changes.
> 
> For such case, our general process is:

Could you tell me where this process is described ?

> 1) Backport the commit as-is (i.e the Author is the original Author)
> 2) Add the tag Origin (recently introduced)
> 3) Add your signed-off-by

So following this theory, if we import a file from Linux we should list all the people who contributed to it since it was created ?

> 
> I understand the patch is already written, so I was OK if you simply list of the commits with the authors/tags for this time.

I would like to understand where this requirement is coming from.

@George: is there some kind of legal reason for something like that ?

> 
> If both Stefano and you agree to not keep the authorships, then I will not stand against it. However, I will not get involved in committing and adding my ack.

I want first to clear up this process and understand why you are requesting this to know how I should do anything like that in the future.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Julien Grall]

Hi Bertrand,

On 11/05/2022 16:40, Bertrand Marquis wrote:
>> On 11 May 2022, at 16:20, Julien Grall <julien@xen.org> wrote:
>> Technically, speaking you only copied the code from Linux and therefore you are not the author of some of the changes.
>>
>> For such case, our general process is:
> 
> Could you tell me where this process is described ?

The closest description I could find is:

https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/process/sending-patches.pandoc;h=7ff7826c992b68804b41cc4a8605329f7a10e44c;hb=HEAD#l75

> 
>> 1) Backport the commit as-is (i.e the Author is the original Author)
>> 2) Add the tag Origin (recently introduced)
>> 3) Add your signed-off-by
> 
> So following this theory, if we import a file from Linux we should list all the people who contributed to it since it was created ?

Technically yes.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Bertrand Marquis]

Hi,

> On 11 May 2022, at 16:47, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 11/05/2022 16:40, Bertrand Marquis wrote:
>>> On 11 May 2022, at 16:20, Julien Grall <julien@xen.org> wrote:
>>> Technically, speaking you only copied the code from Linux and therefore you are not the author of some of the changes.
>>> 
>>> For such case, our general process is:
>> Could you tell me where this process is described ?
> 
> The closest description I could find is:
> 
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/process/sending-patches.pandoc;h=7ff7826c992b68804b41cc4a8605329f7a10e44c;hb=HEAD#l75

This state to give the origin which I can easily do for my patch.
But it does not state that we need to list all the authors from all changes in the origin.

Are you only requesting to modify my patch to add one origin line with the SHA or to actually list all authors of all patches for all changes between the previous state in Xen and the state after my patch ?

> 
>>> 1) Backport the commit as-is (i.e the Author is the original Author)
>>> 2) Add the tag Origin (recently introduced)
>>> 3) Add your signed-off-by
>> So following this theory, if we import a file from Linux we should list all the people who contributed to it since it was created ?
> 
> Technically yes.

But this does not apply here as I am not back porting a patch but syncing a file.
The origin as said upper should be enough to get this information.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Julien Grall]

On 11/05/2022 17:01, Bertrand Marquis wrote:
> Hi,
> 
>> On 11 May 2022, at 16:47, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Bertrand,
>>
>> On 11/05/2022 16:40, Bertrand Marquis wrote:
>>>> On 11 May 2022, at 16:20, Julien Grall <julien@xen.org> wrote:
>>>> Technically, speaking you only copied the code from Linux and therefore you are not the author of some of the changes.
>>>>
>>>> For such case, our general process is:
>>> Could you tell me where this process is described ?
>>
>> The closest description I could find is:
>>
>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/process/sending-patches.pandoc;h=7ff7826c992b68804b41cc4a8605329f7a10e44c;hb=HEAD#l75
> 
> This state to give the origin which I can easily do for my patch.
> But it does not state that we need to list all the authors from all changes in the origin.
Hmmmm.... I guess we are reading the following statement differently:

All tags **above** the `Origin:` tag are from the original patch (which
should all be kept), while tags **after** `Origin:` are related to the
normal Xen patch process as described here.

> 
> Are you only requesting to modify my patch to add one origin line with the SHA or to actually list all authors of all patches for all changes between the previous state in Xen and the state after my patch ?
> 
>>
>>>> 1) Backport the commit as-is (i.e the Author is the original Author)
>>>> 2) Add the tag Origin (recently introduced)
>>>> 3) Add your signed-off-by
>>> So following this theory, if we import a file from Linux we should list all the people who contributed to it since it was created ?
>>
>> Technically yes.
> 
> But this does not apply here as I am not back porting a patch but syncing a file.

I am not sure why you want to differentiate the two. In the latter it is 
actually easy to find the authors (you are syncing after all!):

   git log lastbaseline...newbaseline myfile.c

The result could be pasted in the commit message. As the code is meant 
to follow Linux, then all the commits in the list should be valid for Xen.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Stefano Stabellini]

On Wed, 11 May 2022, Bertrand Marquis wrote:
> > I understand the patch is already written, so I was OK if you simply list of the commits with the authors/tags for this time.
> 
> I would like to understand where this requirement is coming from.
> 
> @George: is there some kind of legal reason for something like that ?

I am not George but I'll answer the legal question. Our "legal" document
is the DCO:

https://developercertificate.org/

This falls under case (b):

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

So from the legal point of view only your Signed-off-by line is
required.

I remember this well because I was confused about this a few years ago
in another case of taking code from Linux.


> > If both Stefano and you agree to not keep the authorships, then I will
> > not stand against it. However, I will not get involved in
> > committing and adding my ack.

I am fine either way. My only request is to mention the Linux commit-id
that Bertrand used as a base and Bertrand has already done that.


> I want first to clear up this process and understand why you are
> requesting this to know how I should do anything like that in the
> future.

It looks like our process docs are not very good on this point and might
benefit from a clarification. I hope you are volunteering :-)

Origin is defined as "it specifies the source of the patch" but it
doesn't say what actually is considered a "source".

I suggest to distinguish between the case where commits are ported
individually from the case where code is copied over (like when we
introduced SMMUv3.) If commits are copied individually, I think we
probably want an Origin tag for each of them and the source is the
original commit-id. If the code is copied from Linux (like the SMMUv3
case) then we probably only want to request a single Origin tag (or a
new tag?) with the base Linux version (5.18-rc3) rather than the
commit-id being backported. In that case the source would be the
repository baseline.

Cheers,

Stefano

Re: [PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3

[Bertrand Marquis]

Hi Stefano,

> On 11 May 2022, at 21:06, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 11 May 2022, Bertrand Marquis wrote:
>>> I understand the patch is already written, so I was OK if you simply list of the commits with the authors/tags for this time.
>> 
>> I would like to understand where this requirement is coming from.
>> 
>> @George: is there some kind of legal reason for something like that ?
> 
> I am not George but I'll answer the legal question. Our "legal" document
> is the DCO:

I agree with your analysis but I still think this is an area where we would
need the confirmation from George.

> 
> https://developercertificate.org/
> 
> This falls under case (b):
> 
> (b) The contribution is based upon previous work that, to the best
>    of my knowledge, is covered under an appropriate open source
>    license and I have the right under that license to submit that
>    work with modifications, whether created in whole or in part
>    by me, under the same open source license (unless I am
>    permitted to submit under a different license), as indicated
>    in the file; or
> 
> So from the legal point of view only your Signed-off-by line is
> required.
> 
> I remember this well because I was confused about this a few years ago
> in another case of taking code from Linux.
> 
> 
>>> If both Stefano and you agree to not keep the authorships, then I will
>>> not stand against it. However, I will not get involved in
>>> committing and adding my ack.
> 
> I am fine either way. My only request is to mention the Linux commit-id
> that Bertrand used as a base and Bertrand has already done that.

Ok

> 
> 
>> I want first to clear up this process and understand why you are
>> requesting this to know how I should do anything like that in the
>> future.
> 
> It looks like our process docs are not very good on this point and might
> benefit from a clarification. I hope you are volunteering :-)
> 
> Origin is defined as "it specifies the source of the patch" but it
> doesn't say what actually is considered a "source".
> 
> I suggest to distinguish between the case where commits are ported
> individually from the case where code is copied over (like when we
> introduced SMMUv3.) If commits are copied individually, I think we
> probably want an Origin tag for each of them and the source is the
> original commit-id. If the code is copied from Linux (like the SMMUv3
> case) then we probably only want to request a single Origin tag (or a
> new tag?) with the base Linux version (5.18-rc3) rather than the
> commit-id being backported. In that case the source would be the
> repository baseline.

Before defining our own way to do that maybe we should check how
others are handling those cases to not reinvent the wheel.

Anybody has a suggestion of an other open source project we could
check which could have the same kind of “needs” ?

Cheers
Bertrand

> 
> Cheers,
> 
> Stefano