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

[Dey, Megha]

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@xxxxxxxxx>
> >
> > 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?
> > +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...

> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index 1da97f905720..7098ba566bcd 100644
> > +++ b/include/linux/msi.h
> > @@ -378,6 +378,9 @@ void *platform_msi_get_host_data(struct irq_domain *domain);
> >   void platform_msi_write_msg(struct irq_data *data, struct msi_msg *msg);
> >   void platform_msi_unmask_irq(struct irq_data *data);
> >   void platform_msi_mask_irq(struct irq_data *data);
> > +
> > +int dev_msi_prepare(struct irq_domain *domain, struct device *dev,
> > +                           int nvec, msi_alloc_info_t *arg);
>
> I wonder if this should use the popular #ifdef dev_msi_prepare scheme
> instead of a weak symbol?

Ok, I will look into the #ifdef option.
> Jason