Re: [XEN PATCH v3 1/7] x86/msi: address violation of MISRA C Rule 20.7 and coding style

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-04-02 17:05, Jan Beulich wrote:
> On 29.03.2024 10:11, 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.
> >
> > No functional change.
> >
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > ---
> > Changes in v2:
> > - Make the style change more consistent
> > ---
> >  xen/arch/x86/include/asm/msi.h | 49 
> > +++++++++++++++++-----------------
> >  1 file changed, 25 insertions(+), 24 deletions(-)
> >
> > diff --git a/xen/arch/x86/include/asm/msi.h 
> > b/xen/arch/x86/include/asm/msi.h
> > index 997ccb87be0c..bd110c357ce4 100644
> > --- 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)
> >  #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))
> > +#define msi_disable(control)         ({ (control) &= 
> > ~PCI_MSI_FLAGS_ENABLE })
> >  #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))
> > -#define msi_enable(control, num) multi_msi_enable(control, num); \
> > -       control |= PCI_MSI_FLAGS_ENABLE
> > -
> > -#define msix_control_reg(base)         (base + PCI_MSIX_FLAGS)
> > -#define msix_table_offset_reg(base)    (base + PCI_MSIX_TABLE)
> > -#define msix_pba_offset_reg(base)      (base + PCI_MSIX_PBA)
> > -#define msix_enable(control)           control |= PCI_MSIX_FLAGS_ENABLE
> > -#define msix_disable(control)          control &= ~PCI_MSIX_FLAGS_ENABLE
> > -#define msix_table_size(control) 	((control & 
> > PCI_MSIX_FLAGS_QSIZE)+1)
> > -#define msix_unmask(address)           (address & ~PCI_MSIX_VECTOR_BITMASK)
> > -#define msix_mask(address)             (address | PCI_MSIX_VECTOR_BITMASK)
> > +    ({ (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))
> > +#define msi_enable(control, num)     ({ multi_msi_enable(control, 
> > num); \
> > +                                        (control) |= 
> > PCI_MSI_FLAGS_ENABLE })
> Neither this nor ...
>
> > +#define msix_control_reg(base)       ((base) + PCI_MSIX_FLAGS)
> > +#define msix_table_offset_reg(base)  ((base) + PCI_MSIX_TABLE)
> > +#define msix_pba_offset_reg(base)    ((base) + PCI_MSIX_PBA)
> > +#define msix_enable(control)         ({ (control) |= 
> > PCI_MSIX_FLAGS_ENABLE })
> > +#define msix_disable(control)        ({ (control) &= 
> > ~PCI_MSIX_FLAGS_ENABLE })
> ... these would compile afaict, if  they were used.
>
> Once again - before fiddling with these we need to settle on which of 
> these
> we want to keep (and then also use, rather than open-coding), and which 
> to
> drop (instead of massaging).
>
> Jan
Ok, we can drop this patch from this for now.

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