On 19/11/2024 2:39 pm, Roger Pau Monné wrote:
> 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....@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?
Split pruning out into another patch, fold the bracket fix into this one.
~Andrew
