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

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

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