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

[Julien Grall <julien@xxxxxxx>]

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