Re: [XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-03-26 16:13, Jan Beulich wrote:
> On 26.03.2024 15:30, Nicola Vetrini wrote:
> > On 2024-03-26 11:05, Jan Beulich wrote:
> > > On 22.03.2024 17:01, Nicola Vetrini wrote:
> > > > MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> > > > of macro parameters shall be enclosed in parentheses". Therefore, 
> > > > some
> > > > macro definitions should gain additional parentheses to ensure that
> > > > all
> > > > current and future users will be safe with respect to expansions 
> > > > that
> > > > can possibly alter the semantics of the passed-in macro parameter.
> > > >
> > > > While at it, the style of these macros has been somewhat uniformed.
> > > Hmm, yes, but they then still leave more to be desired. I guess I can
> > > see
> > > though why you don't want to e.g. ...
> > >
> > > > --- a/xen/arch/x86/include/asm/msi.h
> > > > +++ b/xen/arch/x86/include/asm/msi.h
> > > > @@ -147,33 +147,34 @@ int msi_free_irq(struct msi_desc *entry);
> > > >   */
> > > >  #define NR_HP_RESERVED_VECTORS         20
> > > >
> > > > -#define msi_control_reg(base)          (base + PCI_MSI_FLAGS)
> > > > -#define msi_lower_address_reg(base)    (base + PCI_MSI_ADDRESS_LO)
> > > > -#define msi_upper_address_reg(base)    (base + PCI_MSI_ADDRESS_HI)
> > > > -#define msi_data_reg(base, is64bit)    \
> > > > -       ( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
> > > > -#define msi_mask_bits_reg(base, is64bit) \
> > > > -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : 
> > > > base+PCI_MSI_MASK_BIT-4)
> > > > +#define msi_control_reg(base)        ((base) + PCI_MSI_FLAGS)
> > > > +#define msi_lower_address_reg(base)  ((base) + PCI_MSI_ADDRESS_LO)
> > > > +#define msi_upper_address_reg(base)  ((base) + PCI_MSI_ADDRESS_HI)
> > > > +#define msi_data_reg(base, is64bit) \
> > > > +    (((is64bit) == 1) ? (base) + PCI_MSI_DATA_64 : (base) +
> > > > PCI_MSI_DATA_32)
> > > > +#define msi_mask_bits_reg(base, is64bit)                \
> > > > +    (((is64bit) == 1) ? (base) + PCI_MSI_MASK_BIT       \
> > > > +                      : (base) + PCI_MSI_MASK_BIT - 4)
> > > ... drop the bogus == 1 in these two, making them similar to ...
> > >
> > > >  #define msi_pending_bits_reg(base, is64bit) \
> > > > -       ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> > > > -#define msi_disable(control)           control &= ~PCI_MSI_FLAGS_ENABLE
> > > > +    ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> > > ... this.
> > >
> > > > +#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE
> > > Doesn't this need an outer pair of parentheses, too?
> > Not necessarily.
> And use of msi_disable() in another expression would then likely not do
> what's expected?
Actually I just noticed that some of these macros are never used 
(msi_disable being one of them), as far as I can tell.
> > I'm in favour of a consistent style to be converted in.
> > This also applies below.
> I'm all for consistency; I just don't know what you want to be 
> consistent
> with, here.
I would propose adding parentheses around assignments to control, so 
that all macros are consistently parenthesized.
> > > >  #define multi_msi_capable(control) \
> > > > -       (1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> > > > +    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
> > > >  #define multi_msi_enable(control, num) \
> > > > -       control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> > > > -#define is_64bit_address(control)	(!!(control & 
> > > > PCI_MSI_FLAGS_64BIT))
> > > > -#define is_mask_bit_support(control)   (!!(control &
> > > > PCI_MSI_FLAGS_MASKBIT))
> > > > +    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> > > And this, together with dropping the bogus semicolon?
> > I'll drop the semicolon.
> >
> > > There also look to be cases where MASK_EXTR() / MASK_INSR() would 
> > > want
> > > using,
> > > in favor of using open-coded numbers.
> > Yes, perhaps. However, the risk that I make some mistakes in doing so
> > are quite high, though.
> Right, hence how I started my earlier reply. Question is - do we want 
> to
> go just half the way here, or would we better tidy things all in one 
> go?
> In the latter case I could see about getting to that (whether to take
> your patch as basis or instead do it from scratch isn't quite clear to
> me at this point).
>
> Jan
How about I revise this patch with parentheses added where needed, as 
suggested earlier, and then you can submit a further cleanup patch to 
remove e.g. the open coding?
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)