RE: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain
Hi Thomas,
> -Original Message-
> From: Thomas Gleixner
> Sent: Wednesday, July 22, 2020 1:45 PM
> To: Jiang, Dave ; vk...@kernel.org; Dey, Megha
> ; m...@kernel.org; bhelg...@google.com;
> raf...@kernel.org; gre...@linuxfoundation.org; h...@zytor.com;
> alex.william...@redhat.com; Pan, Jacob jun ; Raj,
> Ashok ; j...@mellanox.com; Liu, Yi L
> ;
> Lu, Baolu ; Tian, Kevin ; Kumar,
> Sanjay K ; Luck, Tony ; Lin,
> Jing ; Williams, Dan J ;
> kwankh...@nvidia.com; eric.au...@redhat.com; pa...@mellanox.com;
> j...@mellanox.com; raf...@kernel.org; Hansen, Dave
> ; netan...@mellanox.com; shah...@mellanox.com;
> yan.y.z...@linux.intel.com; pbonz...@redhat.com; Ortiz, Samuel
> ; Hossain, Mona
> Cc: dmaeng...@vger.kernel.org; linux-kernel@vger.kernel.org;
> x...@kernel.org; linux-...@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain
>
> Dave Jiang writes:
> > From: Megha Dey
> >
> > 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
>
> s/we//
ok
>
> > 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.
> >
> > Reviewed-by: Dan Williams
> > Signed-off-by: Megha Dey
> > Signed-off-by: Dave Jiang
> > ---
> > arch/x86/include/asm/hw_irq.h |1 +
> > arch/x86/kernel/apic/msi.c | 12 ++
> > drivers/base/dev-msi.c | 66
> > +++
> > drivers/iommu/intel/irq_remapping.c | 11 +-
> > include/linux/intel-iommu.h |1 +
> > include/linux/irqdomain.h | 11 ++
> > include/linux/msi.h |3 ++
>
> Why is this mixing generic code, x86 core code and intel specific driver code?
> This is new functionality so:
>
> 1) Provide the infrastructure
> 2) Add support to architecture specific parts
> 3) Enable it
Ok, I will try to adhere to the layering next time around..
>
> > +
> > +#ifdef CONFIG_DEV_MSI
> > +int dev_msi_prepare(struct irq_domain *domain, struct device *dev,
> > + int nvec, msi_alloc_info_t *arg) {
> > + memset(arg, 0, sizeof(*arg));
> > +
> > + arg->type = X86_IRQ_ALLOC_TYPE_DEV_MSI;
> > +
> > + return 0;
> > +}
> > +#endif
>
> What is this? Tons of new lines for taking up more space and not a single
> comment.
Hmm, I will add a comment..
>
> > -static int dev_msi_prepare(struct irq_domain *domain, struct device
> > *dev,
> > +int __weak dev_msi_prepare(struct irq_domain *domain, struct device
> > +*dev,
> >int nvec, msi_alloc_info_t *arg) {
> > memset(arg, 0, sizeof(*arg));
>
> Oh well. So every architecure which needs to override this and I assume all
> which are eventually going to support it need to do the memset() in their
> override.
>
>memset(arg,,,);
>arch_dev_msi_prepare();
>
>
Per you suggestion, I have introduced arch_dev_msi_prepare which returns 0 by
default unless
overridden by arch code in the next patch set.
> > - dev_msi_default_domain = msi_create_irq_domain(fn,
> _msi_domain_info, parent);
> > + /*
> > +* This initcall may come after remap code is initialized. Ensure that
> > +* dev_msi_default domain is updated correctly.
>
> What? No, this is a disgusting hack. Get your ordering straight, that's not
> rocket
> science.
>
Hmm yeah, actually I realized we don't really need to have 2 new IRQ domains
for dev-msi
(with and without interrupt remapping enabled). Hence all this will go away in
the next round
of patches.
> > +#ifdef CONFIG_IRQ_REMAP
>
> IRQ_REMAP is x86 specific. Is this file x86 only or intended to be for
> general use?
> If it's x86 only, then this should be clearly documented. If not, then these
> x86'isms have no place here.
True, I will take care of this in the next patch set.
>
> > +struct irq_domain *create_remap_dev_msi_irq_domain(struct irq_domain
> *parent,
> > + const char *name)
>
> So we have msi_create_irq_domain() and this is about dev_msi, right? So can
> you please stick with a consistent naming scheme?
sure
>
> > +{
> > + struct fwnode_handle *fn;
> > + struct irq_domain *domain;
> > +
> > + fn = irq_domain_alloc_named_fwnode(name);
> > + if (!f
Re: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain
Dave Jiang writes:
> From: Megha Dey
>
> 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
s/we//
> 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.
>
> Reviewed-by: Dan Williams
> Signed-off-by: Megha Dey
> Signed-off-by: Dave Jiang
> ---
> arch/x86/include/asm/hw_irq.h |1 +
> arch/x86/kernel/apic/msi.c | 12 ++
> drivers/base/dev-msi.c | 66
> +++
> drivers/iommu/intel/irq_remapping.c | 11 +-
> include/linux/intel-iommu.h |1 +
> include/linux/irqdomain.h | 11 ++
> include/linux/msi.h |3 ++
Why is this mixing generic code, x86 core code and intel specific driver
code? This is new functionality so:
1) Provide the infrastructure
2) Add support to architecture specific parts
3) Enable it
> +
> +#ifdef CONFIG_DEV_MSI
> +int dev_msi_prepare(struct irq_domain *domain, struct device *dev,
> +int nvec, msi_alloc_info_t *arg)
> +{
> + memset(arg, 0, sizeof(*arg));
> +
> + arg->type = X86_IRQ_ALLOC_TYPE_DEV_MSI;
> +
> + return 0;
> +}
> +#endif
What is this? Tons of new lines for taking up more space and not a
single comment.
> -static int dev_msi_prepare(struct irq_domain *domain, struct device *dev,
> +int __weak dev_msi_prepare(struct irq_domain *domain, struct device *dev,
> int nvec, msi_alloc_info_t *arg)
> {
> memset(arg, 0, sizeof(*arg));
Oh well. So every architecure which needs to override this and I assume
all which are eventually going to support it need to do the memset() in
their override.
memset(arg,,,);
arch_dev_msi_prepare();
> - dev_msi_default_domain = msi_create_irq_domain(fn,
> _msi_domain_info, parent);
> + /*
> + * This initcall may come after remap code is initialized. Ensure that
> + * dev_msi_default domain is updated correctly.
What? No, this is a disgusting hack. Get your ordering straight, that's
not rocket science.
> +#ifdef CONFIG_IRQ_REMAP
IRQ_REMAP is x86 specific. Is this file x86 only or intended to be for
general use? If it's x86 only, then this should be clearly
documented. If not, then these x86'isms have no place here.
> +struct irq_domain *create_remap_dev_msi_irq_domain(struct irq_domain *parent,
> +const char *name)
So we have msi_create_irq_domain() and this is about dev_msi, right? So
can you please stick with a consistent naming scheme?
> +{
> + 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, _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;
Can this be called several times? If so, then this lacks a comment. If
not, then this condition is useless.
Thanks,
tglx
Re: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain
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
> > >
> > > 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, _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
Re: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain
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
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, _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
Re: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain
On Tue, Jul 21, 2020 at 09:02:35AM -0700, Dave Jiang wrote:
> From: Megha Dey
>
> 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?
I would have thought you'd want to switch to this remapping mode as
part of vfio or something like current cases.
> +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, _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" ?
> 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?
Jason
