| Message ID | 24fd2364294345f103cb13bdab2ad0b706681071.1651570561.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Spectre BHB follow up | expand |
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,
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
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,
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,
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
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,
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
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,
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
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)
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(-)