Hi,
> On 9 May 2022, at 12:08, Julien Grall <jul...@xen.org> wrote:
>
> Hi,
>
> On 09/05/2022 11:49, Bertrand Marquis wrote:
>>> On 9 May 2022, at 11:31, Julien Grall <jul...@xen.org> wrote:
>>> On 09/05/2022 11:08, Bertrand Marquis wrote:
>>>>> On 4 May 2022, at 09:06, Julien Grall <jul...@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 <jul...@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
