Re: [XEN PATCH][for-next v2 5/8] x86/io_apic: address violation of MISRA C:2012 Rule 10.1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.10.2023 17:28, Nicola Vetrini wrote:
> The definition of IO_APIC_BASE contains a sum of an essentially enum
> value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all
> instances, is unsigned, therefore the former is cast to unsigned, so that
> the operands are of the same essential type.
> 
> No functional change.
> ---
>  xen/arch/x86/include/asm/io_apic.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/io_apic.h 
> b/xen/arch/x86/include/asm/io_apic.h
> index a7e4c9e146de..a0fc50d601fe 100644
> --- a/xen/arch/x86/include/asm/io_apic.h
> +++ b/xen/arch/x86/include/asm/io_apic.h
> @@ -14,9 +14,10 @@
>   * Copyright (C) 1997, 1998, 1999, 2000 Ingo Molnar
>   */
>  
> -#define IO_APIC_BASE(idx)                                               \
> -    ((volatile uint32_t *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + (idx))    \
> -                           + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))
> +#define IO_APIC_BASE(idx)                                     \
> +    ((volatile uint32_t *)                                    \
> +     (__fix_to_virt((unsigned int)FIX_IO_APIC_BASE_0 + (idx)) \
> +      + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))

Let's assume that down the road we want to make __fix_to_virt() an inline
function (which perhaps it really ought to be already): Won't this trade
one violation for another then? I wonder whether the better course of
action wouldn't be to switch the two enums to be "anonymous", even if that
means adjusting __set_fixmap() and __set_fixmap_x().

As an aside, since you touch the entire construct, it would be nice if the
+ was also moved to the end of the earlier line.

Jan