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

[Julien Grall <julien@xen.org>]

On 19/04/2021 12:16, Jan Beulich wrote:
> 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. 

As I pointed out before, Arm doesn't use the IOMMU to setup the MSIs. So 
the naming and using an IOMMU callback is definitely wrong for Arm.

> 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.
AFAIK, this helper is only called by x86 specific code and it will not 
be used as-is by Arm.

I can't tell for other arch (e.g RISCv, PowerPC). However... we can take 
the decision to move the code back to common back when it is necessary.

For the time being, I think move this code in x86 is a lot better than 
#ifdef or keep the code in common code.

Cheers,

-- 
Julien Grall