Re: [PATCH v3 2/4] xen/arm: Add sb instruction support

[Julien Grall <julien@xxxxxxx>]

Hi Bertrand,

On 13/06/2022 13:53, Bertrand Marquis wrote:
> 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
> capability ARM_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@xxxxxxx>
Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

> ---
> Changes in v3:
> - rename ARM64_HAS_SB to ARM_HAS_SB
> - define sb before including per arch macros
> 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     | 19 +++++++++++++++++-
>   xen/arch/arm/setup.c                  |  3 +++
>   xen/arch/arm/smpboot.c                |  1 +
>   5 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index a58965f7b9..62d5e1770a 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 = ARM_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..24c01d2b9d 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 ARM_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..dc791245df 100644
> --- a/xen/arch/arm/include/asm/macros.h
> +++ b/xen/arch/arm/include/asm/macros.h
> @@ -5,13 +5,30 @@
>   # error "This file should only be included in assembly file"
>   #endif
>   
> +#include <asm/alternative.h>
> +
>       /*
>        * Speculative barrier
> -     * XXX: Add support for the 'sb' instruction
>        */
>       .macro sb
> +alternative_if_not ARM_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
>   
>   #if defined (CONFIG_ARM_32)
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 6016471d37..577c54e6fb 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -964,6 +964,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>        */
>       check_local_cpu_errata();
>   
> +    check_local_cpu_features();
> +
>       init_xen_time();
>   
>       gic_init();
> @@ -1033,6 +1035,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 22fede6600..3f62f3a44f 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -395,6 +395,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());
>   
--
Julien Grall