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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>

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