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....@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. 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. 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. > --- > 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'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). ~Andrew
