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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 9 May 2022, at 12:08, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 09/05/2022 11:49, Bertrand Marquis wrote:
>>> On 9 May 2022, at 11:31, Julien Grall <julien@xxxxxxx> wrote:
>>> On 09/05/2022 11:08, Bertrand Marquis wrote:
>>>>> On 4 May 2022, at 09:06, Julien Grall <julien@xxxxxxx> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 04/05/2022 08:24, Bertrand Marquis wrote:
>>>>>> Hi Julien,
>>>>> 
>>>>> Hi Bertrand,
>>>>> 
>>>>>>> On 3 May 2022, at 19:47, Julien Grall <julien@xxxxxxx> 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