Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2021 10:40, Roger Pau Monné wrote:
> On Mon, Apr 19, 2021 at 07:16:52AM +0000, Rahul Singh wrote:
>> Thanks you everyone for reviewing the code. I will summarise what I have 
>> understood from all the comments 
>> and what I will be doing for the next version of the patch.  Please let me 
>> know your view on this.
>>
>> 1. Create a separate non-arch specific file "msi-intercept.c"  for the below 
>> newly introduced function and 
>>     compile that file if CONFIG_PCI_MSI_INTERCEPT is 
>> enabled.CONFIG_PCI_MSI_INTERCEPT  will  be 
>>     enabled for x86 by default.

Everything up to here wants to be separate from ...

>> Also Mention in the commit message that these function will be needed for 
>> Xen to 
>>     support MSI interrupt within XEN.
>>
>>      pdev_msi_initi(..)
>>      pdev_msi_deiniti(..)

... this (if all of these functions really are needed beyond the
purpose of intercepting MSI accesses).

> I would drop the last 'i' from both function names above, as we use
> init/deinit in the rest of the code base.

+1

>>      pdev_dump_msi(..),
>>      pdev_msix_assign(..)
>>
>> 2. Create separate patch for iommu_update_ire_from_msi() related code. There 
>> are two suggestion please help me which one to choose. 
>>  
>>      - Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and 
>> also move the hook from iommu_ops under CONFIG_X86.
> 
> I would go for this one.

Strictly speaking this isn't x86-specific and hence shouldn't move there.
It merely depends on whether full MSI support is wanted by an arch. I'd
therefore guard the declaration by an #ifdef (if needed at all - have a
declaration without implementation isn't really that problematic). For
the definition question is going to be whether you introduce another new
file for the pdev_*() functions above. If not, #ifdef may again be better
than moving to an x86-specific file.

>>       - Implement a more generic function "arch_register_msi()"). This could 
>> call iommu_update_ire_from_msi() on x86 and the 
>>         ITS related code once implemented on Arm. Introduce the new Kconfig 
>> CONFIG_HAS_IOMMU_INTERRUPT_REMAP for this option.
> 
> I think it's best to introduce this hook when you actually have to
> implement the Arm version of it.

Plus arch_register_msi() sounds like a one-time operation, whereas (iirc)
iommu_update_ire_from_msi() may get called many times during the lifetime
of a single MSI interrupt.

Jan