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

[Julien Grall <julien@xxxxxxx>]

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