| Message ID | 1fe7602b48cabb7710025f6b4e32e9b99a1faacd.1697123806.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 10.1 | expand |
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
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(-) Oh, also - there's no S-o-b here. Jan
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
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(-) > > Oh, also - there's no S-o-b here. > > Jan Ah, good catch.
On 16.10.2023 18:36, Nicola Vetrini wrote: > On 16/10/2023 17:42, Jan Beulich wrote: >> On 12.10.2023 17:28, Nicola Vetrini wrote: >>> --- 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? No. But I assumed such a function would take an enum parameter. Jan
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))) #define IO_APIC_ID(idx) (mp_ioapics[idx].mpc_apicid)