Re: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain

[Jason Gunthorpe <jgg@mellanox.com>]

On Wed, Jul 22, 2020 at 10:03:45AM -0700, Dey, Megha wrote:
> Hi Dan,
> 
> On 7/21/2020 9:21 AM, Jason Gunthorpe wrote:
> > On Tue, Jul 21, 2020 at 09:02:35AM -0700, Dave Jiang wrote:
> > > From: Megha Dey <megha.dey@intel.com>
> > > 
> > > When DEV_MSI is enabled, the dev_msi_default_domain is updated to the
> > > base DEV-MSI irq  domain. If interrupt remapping is enabled, we create
> > > a new IR-DEV-MSI irq domain and update the dev_msi_default domain to
> > > the same.
> > > 
> > > For X86, introduce a new irq_alloc_type which will be used by the
> > > interrupt remapping driver.
> > 
> > Why? Shouldn't this by symmetrical with normal MSI? Does MSI do this?
> 
> Since I am introducing the new dev msi domain for the case when IR_REMAP is
> turned on, I have introduced the new type in this patch.
>
> MSI/MSIX have their own irq alloc types which are also only used by the
> intel remapping driver..
>
> > 
> > I would have thought you'd want to switch to this remapping mode as
> > part of vfio or something like current cases.
> 
> Can you let me know what current case you are referring to?

My mistake, I see Intel unconditionally globally enables IR, so this
seems consistent with Intel's MSI

> > > +struct irq_domain *create_remap_dev_msi_irq_domain(struct irq_domain *parent,
> > > +						   const char *name)
> > > +{
> > > +	struct fwnode_handle *fn;
> > > +	struct irq_domain *domain;
> > > +
> > > +	fn = irq_domain_alloc_named_fwnode(name);
> > > +	if (!fn)
> > > +		return NULL;
> > > +
> > > +	domain = msi_create_irq_domain(fn, &dev_msi_ir_domain_info, parent);
> > > +	if (!domain) {
> > > +		pr_warn("failed to initialize irqdomain for IR-DEV-MSI.\n");
> > > +		return ERR_PTR(-ENXIO);
> > > +	}
> > > +
> > > +	irq_domain_update_bus_token(domain, DOMAIN_BUS_PLATFORM_MSI);
> > > +
> > > +	if (!dev_msi_default_domain)
> > > +		dev_msi_default_domain = domain;
> > > +
> > > +	return domain;
> > > +}
> > 
> > What about this code creates a "remap" ? ie why is the function called
> > "create_remap" ?
> 
> Well, this function creates a new domain for the case when IR_REMAP is
> enabled, hence I called it create_remap...

This looks like it just creates a new domain - the thing that makes it
remapping is the caller putting it under the ir_domain - so this code
here in base shouldn't have the word 'remap' in it, this is just
creating a generic domain.

It also kinda looks like create_dev_msi_domain() can just call the
above directly instead of duplicating everything - eg why do we need
two identical dev_msi_ir_controller vs dev_msi_controller just to have
the irq_set_vcpu_affinity difference?

Jason