Hi Julien, > On 3 May 2022, at 19:47, Julien Grall <jul...@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
