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