This patch is adding sb instruction support when it is supported by a
CPU on arm64.
A new cpu feature capability system is introduced to enable alternative
code using sb instruction when it is supported by the processor. This is
decided based on the isa64 system register value and use a new hardware
capabitility ARM64_HAS_SB.

The sb instruction is encoded using its hexadecimal value to avoid
recursive macro and support old compilers not having support for sb
instruction.

Arm32 instruction support is added but it is not enabled at the moment
due to the lack of hardware supporting it.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
Changes in v2:
- fix commit message
- add comment to explain the extra nop
- add support for arm32 and move macro back to arm generic header
- fix macro comment indentation
- introduce cpu feature system instead of using errata
---
 xen/arch/arm/cpufeature.c             | 28 +++++++++++++++++++++++
 xen/arch/arm/include/asm/cpufeature.h |  6 ++++-
 xen/arch/arm/include/asm/macros.h     | 33 ++++++++++++++++++++-------
 xen/arch/arm/setup.c                  |  3 +++
 xen/arch/arm/smpboot.c                |  1 +
 5 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index a58965f7b9..5d1421dc67 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -26,6 +26,24 @@ DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS);
 
 struct cpuinfo_arm __read_mostly guest_cpuinfo;
 
+#ifdef CONFIG_ARM_64
+static bool has_sb_instruction(const struct arm_cpu_capabilities *entry)
+{
+    return system_cpuinfo.isa64.sb;
+}
+#endif
+
+static const struct arm_cpu_capabilities arm_features[] = {
+#ifdef CONFIG_ARM_64
+    {
+        .desc = "Speculation barrier instruction (SB)",
+        .capability = ARM64_HAS_SB,
+        .matches = has_sb_instruction,
+    },
+#endif
+    {},
+};
+
 void update_cpu_capabilities(const struct arm_cpu_capabilities *caps,
                              const char *info)
 {
@@ -70,6 +88,16 @@ void __init enable_cpu_capabilities(const struct arm_cpu_capabilities *caps)
     }
 }
 
+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/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
index f7368766c0..9649a7afee 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__
 
@@ -78,6 +79,9 @@
 
 extern DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS);
 
+void check_local_cpu_features(void);
+void enable_cpu_features(void);
+
 static inline bool cpus_have_cap(unsigned int num)
 {
     if ( num >= ARM_NCAPS )
diff --git a/xen/arch/arm/include/asm/macros.h b/xen/arch/arm/include/asm/macros.h
index 1aa373760f..33e863d982 100644
--- a/xen/arch/arm/include/asm/macros.h
+++ b/xen/arch/arm/include/asm/macros.h
@@ -5,14 +5,7 @@
 # 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
+#include <asm/alternative.h>
 
 #if defined (CONFIG_ARM_32)
 # include <asm/arm32/macros.h>
@@ -29,4 +22,28 @@
     .endr
     .endm
 
+    /*
+     * Speculative barrier
+     */
+    .macro sb
+alternative_if_not ARM64_HAS_SB
+    dsb nsh
+    isb
+alternative_else
+    /*
+     * SB encoding in hexadecimal to prevent recursive macro.
+     * extra nop is required to keep same number of instructions on both sides
+     * of the alternative.
+     */
+#if defined(CONFIG_ARM_32)
+    .inst 0xf57ff070
+#elif defined(CONFIG_ARM_64)
+    .inst 0xd50330ff
+#else
+#   error "missing sb encoding for ARM variant"
+#endif
+    nop
+alternative_endif
+    .endm
+
 #endif /* __ASM_ARM_MACROS_H */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ea1f5ee3d3..b44494c9a9 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -961,6 +961,8 @@ void __init start_xen(unsigned long boot_phys_offset,
      */
     check_local_cpu_errata();
 
+    check_local_cpu_features();
+
     init_xen_time();
 
     gic_init();
@@ -1030,6 +1032,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 9bb32a301a..fb7cc43a93 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -389,6 +389,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());
 
-- 
2.25.1

Hi Bertrand,

On 31/05/2022 11:43, Bertrand Marquis wrote:
> diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
> index f7368766c0..9649a7afee 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

The feature is for both 32-bit and 64-bit. So I would prefer if it is 
called ARM_HAS_SB.

>   
> -#define ARM_NCAPS           16
> +#define ARM_NCAPS           17
>   
>   #ifndef __ASSEMBLY__
>   
> @@ -78,6 +79,9 @@
>   
>   extern DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS);
>   
> +void check_local_cpu_features(void);
> +void enable_cpu_features(void);
> +
>   static inline bool cpus_have_cap(unsigned int num)
>   {
>       if ( num >= ARM_NCAPS )
> diff --git a/xen/arch/arm/include/asm/macros.h b/xen/arch/arm/include/asm/macros.h
> index 1aa373760f..33e863d982 100644
> --- a/xen/arch/arm/include/asm/macros.h
> +++ b/xen/arch/arm/include/asm/macros.h
> @@ -5,14 +5,7 @@
>   # 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

Looking at the patch bcab2ac84931 "xen/arm64: Place a speculation 
barrier following an ret instruction", the macro was defined before 
including <asm/arm*/macros.h> so 'sb' could be used in macros defined by 
the headers.

I can't remember whether I chose the order because I had a failure on 
some compilers. However, I couldn't find anything in the assembler 
documentation suggesting that a macro A could use B before it is used.

So I would rather avoid to move the macro if there are no strong 
argument for it.

> +#include <asm/alternative.h>
>   
>   #if defined (CONFIG_ARM_32)
>   # include <asm/arm32/macros.h>
> @@ -29,4 +22,28 @@
>       .endr
>       .endm
>   
> +    /*
> +     * Speculative barrier
> +     */
> +    .macro sb
> +alternative_if_not ARM64_HAS_SB
> +    dsb nsh
> +    isb
> +alternative_else
> +    /*
> +     * SB encoding in hexadecimal to prevent recursive macro.
> +     * extra nop is required to keep same number of instructions on both sides
> +     * of the alternative.
> +     */
> +#if defined(CONFIG_ARM_32)
> +    .inst 0xf57ff070
> +#elif defined(CONFIG_ARM_64)
> +    .inst 0xd50330ff
> +#else
> +#   error "missing sb encoding for ARM variant"
> +#endif
> +    nop
> +alternative_endif
> +    .endm
> +
>   #endif /* __ASM_ARM_MACROS_H */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index ea1f5ee3d3..b44494c9a9 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -961,6 +961,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>        */
>       check_local_cpu_errata();
>   
> +    check_local_cpu_features();
> +
>       init_xen_time();
>   
>       gic_init();
> @@ -1030,6 +1032,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 9bb32a301a..fb7cc43a93 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -389,6 +389,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

Hi Julien,

> On 10 Jun 2022, at 19:20, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 31/05/2022 11:43, Bertrand Marquis wrote:
>> diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
>> index f7368766c0..9649a7afee 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
> 
> The feature is for both 32-bit and 64-bit. So I would prefer if it is called ARM_HAS_SB.

Right make sense.

> 
>>  -#define ARM_NCAPS           16
>> +#define ARM_NCAPS           17
>>    #ifndef __ASSEMBLY__
>>  @@ -78,6 +79,9 @@
>>    extern DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS);
>>  +void check_local_cpu_features(void);
>> +void enable_cpu_features(void);
>> +
>>  static inline bool cpus_have_cap(unsigned int num)
>>  {
>>      if ( num >= ARM_NCAPS )
>> diff --git a/xen/arch/arm/include/asm/macros.h b/xen/arch/arm/include/asm/macros.h
>> index 1aa373760f..33e863d982 100644
>> --- a/xen/arch/arm/include/asm/macros.h
>> +++ b/xen/arch/arm/include/asm/macros.h
>> @@ -5,14 +5,7 @@
>>  # 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
> 
> Looking at the patch bcab2ac84931 "xen/arm64: Place a speculation barrier following an ret instruction", the macro was defined before including <asm/arm*/macros.h> so 'sb' could be used in macros defined by the headers.
> 
> I can't remember whether I chose the order because I had a failure on some compilers. However, I couldn't find anything in the assembler documentation suggesting that a macro A could use B before it is used.
> 
> So I would rather avoid to move the macro if there are no strong argument for it.

Sure I will put it back where it was.

Cheers
Bertrand