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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Rahul Singh <Rahul.Singh@arm.com>
Cc: Julien Grall <julien@xen.org>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	Paul Durrant <paul@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	Daniel De Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code
Date: Mon, 19 Apr 2021 10:40:36 +0200	[thread overview]
Message-ID: <YH1CBP8At7lVoHCz@Air-de-Roger> (raw)
In-Reply-To: <AD2BC1CD-74AF-48FF-8B42-2853C0E7A7BE@arm.com>

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. 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(..)

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

> 	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.

> 
>       - 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.

Thanks, Roger.