Re: [XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2024-02-29 17:34, Jan Beulich wrote:
> On 29.02.2024 16:27, Nicola Vetrini wrote:
>> --- a/xen/arch/arm/cpuerrata.c
>> +++ b/xen/arch/arm/cpuerrata.c
>> @@ -462,8 +462,8 @@ static bool has_ssbd_mitigation(const struct 
>> arm_cpu_capabilities *entry)
>>  #define MIDR_RANGE(model, min, max)     \
>>      .matches = is_affected_midr_range,  \
>>      .midr_model = model,                \
>> -    .midr_range_min = min,              \
>> -    .midr_range_max = max
>> +    .midr_range_min = (min),            \
>> +    .midr_range_max = (max)
> 
> Why min and max, but not model?
> 

All the constants in the full expansions are parenthesized via 
MIDR_CPU_MODEL, so it doesn't trigger any violation right now, but for 
consistency I'd better put parentheses there as well.

>> --- a/xen/arch/arm/include/asm/smccc.h
>> +++ b/xen/arch/arm/include/asm/smccc.h
>> @@ -122,7 +122,7 @@ struct arm_smccc_res {
>>  #define __constraint_read_7 __constraint_read_6, "r" (r7)
>> 
>>  #define __declare_arg_0(a0, res)                            \
>> -    struct arm_smccc_res    *___res = res;                  \
>> +    struct arm_smccc_res    *___res = (res);                \
>>      register unsigned long  r0 ASM_REG(0) = (uint32_t)a0;   \
> 
> Why res but not a0?
> 

Seems like it's never used in a non-compliant way, but you do have a 
point. Here and also below, to keep it consistent. I didn't look at all 
the violations yet, so I may have missed some. I did want to show a few 
patches also to gather opinions on what may/may not be accepted.

>> --- a/xen/arch/arm/include/asm/vgic-emul.h
>> +++ b/xen/arch/arm/include/asm/vgic-emul.h
>> @@ -6,11 +6,11 @@
>>   * a range of registers
>>   */
>> 
>> -#define VREG32(reg) reg ... reg + 3
>> -#define VREG64(reg) reg ... reg + 7
>> +#define VREG32(reg) (reg) ... (reg) + 3
>> +#define VREG64(reg) (reg) ... (reg) + 7
> 
> #define VREG32(reg) (reg) ... ((reg) + 3)
> #define VREG64(reg) (reg) ... ((reg) + 7)
> 
> ?
> 

The outer parentheses are not required, but I can add them if the 
maintainers share your view.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)