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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 16/10/2023 17:42, Jan Beulich wrote:
> 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 don't think so: the violation is in the argument to the macro (i.e., 
the fact that
idx is unsigned and FIX_IO_APIC_BASE_0 is a constant from a named enum). 
Do you see a way in
which a MISRA rule is violated if __fix_to_virt becomes a function with 
an unsigned int argument?
> 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().
I don't know. It certainly would help from a violation count 
perspective, though that's more of a code
style decision in my opinion.

> 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.
Sure

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