Re: [PATCH 2/4] x86/msi: fix Misra Rule 20.7 in msi.h

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Nov 19, 2024 at 02:21:35PM +0000, Andrew Cooper wrote:
> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> > Prune unused macros and adjust the remaining ones to parenthesize macro
> > arguments.
> >
> > No functional change intended.
> >
> > Singed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> It's a little early for carol season, isn't it?
> 
> It would help to identify which macros are being dropped, because the
> diff is far from simple to read.
> 
> AFAICT, its:
> 
>   msi_disable()
>   msi_enable()
>   msix_enable()
>   msix_disable()
>   msix_unmask()
>   msix_mask()
> 
> Splitting this change does make a marginal improvement in the diff, and
> a substantial improvement in `git diff --color-word`'s ability to review
> this change.

Hm, yes, it would likely be easier to parse, I just went on a spree to
clean it up.

> You've also introduced uses of MASK_EXTR() and MASK_INSR(), which at
> least ought to be noted in the commit message.  Technically I think it's
> a bugfix for multi_msi_enable(), because I think it now won't overflow
> the 3-bit field if an overly large num is passed in.

Hm, I've become used to MASK_{EXTR,INSR}(), so the change felt natural
since I was already adjusting the code.

> 
> Bloat-o-meter reports:
> 
> add/remove: 0/0 grow/shrink: 3/1 up/down: 15/-61 (-46)
> Function                                     old     new   delta
> set_iommu_interrupt_handler                  366     373      +7
> write_msi_msg                                348     352      +4
> init_msi                                     574     578      +4
> pci_enable_msi                              1084    1023     -61
> 
> 
> Taking the first example, that's caused by swapping this:
> 
> > iommu->msi.msi.mpos = ( ((!!(control & 0x80)) == 1) ?
> > iommu->msi.msi_attrib.pos+16 : iommu->msi.msi_attrib.pos+16 -4);
> 
> for this:
> 
> > iommu->msi.msi.mpos = ((iommu->msi.msi_attrib.pos) + 16 -
> > (((!!((control) & 0x80))) ? 0 : 4));
> 
> and code generation changing from a CMOV to straight-line arithmetic.
> 
> In write_msi_msg(), we actually drop a conditional branch and replace it
> with straight-line arithmetic.
> 
> init_msi() gets a substantial restructuring, but it looks like two
> branches are dropped.
> 
> pci_enable_msi() has the biggest change, but doesn't obviously reduce
> the number of branches.  There is clearly less register setup around
> existing branches, so my best guess is that the new macro forms are more
> amenable to common-sub-expression-elimination.
> 
> 
> Either way, it's all minor.  Staring at the diff for long enough, I'm
> pretty sure it's all good.

Thanks.

> > ---
> >  xen/arch/x86/include/asm/msi.h | 35 ++++++++++++++--------------------
> >  1 file changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> > index 748bc3cd6d8b..49a576383288 100644
> > --- a/xen/arch/x86/include/asm/msi.h
> > +++ b/xen/arch/x86/include/asm/msi.h
> > @@ -147,33 +147,26 @@ 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_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 )
> > +	((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
> >  #define msi_mask_bits_reg(base, is64bit) \
> > -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
> > +	((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 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
> >  #define multi_msi_capable(control) \
> > -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> > +	(1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
> >  #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) |= MASK_INSR(fls(num) - 1, 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)
> 
> You need to retain the outermost brackets for other MISRA reasons.

I was borderline on dropping those braces, as I was expecting Misra to
require them.

> I'm happy to fix up on commit, even splitting the patch (seeing as I've
> already done the split in order to review the rest).

Fine, by split I think you mean the pruning of unused macros vs the
fixing of the parentheses?

Thanks, Roger.