From: Marc Zyngier <maz@kernel.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Dimitri Sivanich <sivanich@hpe.com>,
linux-hyperv@vger.kernel.org, Steve Wahl <steve.wahl@hpe.com>,
linux-pci@vger.kernel.org, "K. Y. Srinivasan" <kys@microsoft.com>,
Dan Williams <dan.j.williams@intel.com>,
Wei Liu <wei.liu@kernel.org>,
Stephen Hemminger <sthemmin@microsoft.com>,
Baolu Lu <baolu.lu@intel.com>,
x86@kernel.org, Jason Gunthorpe <jgg@mellanox.com>,
Megha Dey <megha.dey@intel.com>,
xen-devel@lists.xenproject.org, Kevin Tian <kevin.tian@intel.com>,
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Alex Williamson <alex.williamson@redhat.com>,
Stefano Stabellini <sstabellini@kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>,
Dave Jiang <dave.jiang@intel.com>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
Jon Derrick <jonathan.derrick@intel.com>,
Juergen Gross <jgross@suse.com>, Russ Anderson <rja@hpe.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
LKML <linux-kernel@vger.kernel.org>,
iommu@lists.linux-foundation.org,
Jacob Pan <jacob.jun.pan@intel.com>,
"Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [patch V2 04/46] genirq/chip: Use the first chip in irq_chip_compose_msi_msg()
Date: Wed, 26 Aug 2020 20:50:28 +0100 [thread overview]
Message-ID: <87a6yh2nln.wl-maz@kernel.org> (raw)
In-Reply-To: <20200826112331.047917603@linutronix.de>
On Wed, 26 Aug 2020 12:16:32 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
>
> The documentation of irq_chip_compose_msi_msg() claims that with
> hierarchical irq domains the first chip in the hierarchy which has an
> irq_compose_msi_msg() callback is chosen. But the code just keeps
> iterating after it finds a chip with a compose callback.
>
> The x86 HPET MSI implementation relies on that behaviour, but that does not
> make it more correct.
>
> The message should always be composed at the domain which manages the
> underlying resource (e.g. APIC or remap table) because that domain knows
> about the required layout of the message.
>
> On X86 the following hierarchies exist:
>
> 1) vector -------- PCI/MSI
> 2) vector -- IR -- PCI/MSI
>
> The vector domain has a different message format than the IR (remapping)
> domain. So obviously the PCI/MSI domain can't compose the message without
> having knowledge about the parent domain, which is exactly the opposite of
> what hierarchical domains want to achieve.
>
> X86 actually has two different PCI/MSI chips where #1 has a compose
> callback and #2 does not. #2 delegates the composition to the remap domain
> where it belongs, but #1 does it at the PCI/MSI level.
>
> For the upcoming device MSI support it's necessary to change this and just
> let the first domain which can compose the message take care of it. That
> way the top level chip does not have to worry about it and the device MSI
> code does not need special knowledge about topologies. It just sets the
> compose callback to NULL and lets the hierarchy pick the first chip which
> has one.
>
> Due to that the attempt to move the compose callback from the direct
> delivery PCI/MSI domain to the vector domain made the system fail to boot
> with interrupt remapping enabled because in the remapping case
> irq_chip_compose_msi_msg() keeps iterating and choses the compose callback
> of the vector domain which obviously creates the wrong format for the remap
> table.
>
> Break out of the loop when the first irq chip with a compose callback is
> found and fixup the HPET code temporarily. That workaround will be removed
> once the direct delivery compose callback is moved to the place where it
> belongs in the vector domain.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V2: New patch. Note, that this might break other stuff which relies on the
> current behaviour, but the hierarchy composition of DT based chips is
> really hard to follow.
Grepping around, I don't think there is any occurrence of two irqchips
providing irq_compose_msi() that can share a hierarchy on any real
system, so we should be fine. Famous last words.
> ---
> arch/x86/kernel/apic/msi.c | 7 +++++--
> kernel/irq/chip.c | 12 +++++++++---
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> --- a/arch/x86/kernel/apic/msi.c
> +++ b/arch/x86/kernel/apic/msi.c
> @@ -479,10 +479,13 @@ struct irq_domain *hpet_create_irq_domai
> info.type = X86_IRQ_ALLOC_TYPE_HPET;
> info.hpet_id = hpet_id;
> parent = irq_remapping_get_ir_irq_domain(&info);
> - if (parent == NULL)
> + if (parent == NULL) {
> parent = x86_vector_domain;
> - else
> + } else {
> hpet_msi_controller.name = "IR-HPET-MSI";
> + /* Temporary fix: Will go away */
> + hpet_msi_controller.irq_compose_msi_msg = NULL;
> + }
>
> fn = irq_domain_alloc_named_id_fwnode(hpet_msi_controller.name,
> hpet_id);
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -1544,10 +1544,16 @@ int irq_chip_compose_msi_msg(struct irq_
> struct irq_data *pos = NULL;
>
> #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> - for (; data; data = data->parent_data)
> -#endif
> - if (data->chip && data->chip->irq_compose_msi_msg)
> + for (; data; data = data->parent_data) {
> + if (data->chip && data->chip->irq_compose_msi_msg) {
> pos = data;
> + break;
> + }
> + }
> +#else
> + if (data->chip && data->chip->irq_compose_msi_msg)
> + pos = data;
> +#endif
> if (!pos)
> return -ENOSYS;
>
>
>
Is it just me, or is this last change more complex than it ought to be?
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 857f5f4c8098..25e18b73699c 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1544,7 +1544,7 @@ int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
struct irq_data *pos = NULL;
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
- for (; data; data = data->parent_data)
+ for (; data && !pos; data = data->parent_data)
#endif
if (data->chip && data->chip->irq_compose_msi_msg)
pos = data;
Though the for loop in a #ifdef in admittedly an acquired taste...
Reviewed-by: Marc Zyngier <maz@kernel.org>
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2020-08-26 19:50 UTC|newest]
Thread overview: 121+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-26 11:16 [patch V2 00/46] x86, PCI, XEN, genirq ...: Prepare for device MSI Thomas Gleixner
2020-08-26 11:16 ` [patch V2 01/46] iommu/amd: Prevent NULL pointer dereference Thomas Gleixner
2020-08-27 14:57 ` Joerg Roedel
2020-08-26 11:16 ` [patch V2 02/46] x86/init: Remove unused init ops Thomas Gleixner
2020-08-26 11:16 ` [patch V2 03/46] PCI: vmd: Dont abuse vector irqomain as parent Thomas Gleixner
2020-08-26 11:16 ` [patch V2 04/46] genirq/chip: Use the first chip in irq_chip_compose_msi_msg() Thomas Gleixner
2020-08-26 19:50 ` Marc Zyngier [this message]
2020-08-26 21:19 ` Thomas Gleixner
2020-08-26 21:32 ` Marc Zyngier
2020-08-26 11:16 ` [patch V2 05/46] x86/msi: Move compose message callback where it belongs Thomas Gleixner
2020-08-26 11:16 ` [patch V2 06/46] x86/msi: Remove pointless vcpu_affinity callback Thomas Gleixner
2020-08-26 11:16 ` [patch V2 07/46] x86/irq: Rename X86_IRQ_ALLOC_TYPE_MSI* to reflect PCI dependency Thomas Gleixner
2020-08-26 11:16 ` [patch V2 08/46] x86/irq: Add allocation type for parent domain retrieval Thomas Gleixner
2020-08-26 11:16 ` [patch V2 09/46] iommu/vt-d: Consolidate irq domain getter Thomas Gleixner
2020-08-26 11:16 ` [patch V2 10/46] iommu/amd: " Thomas Gleixner
2020-08-26 11:16 ` [patch V2 11/46] iommu/irq_remapping: Consolidate irq domain lookup Thomas Gleixner
2020-08-26 11:16 ` [patch V2 12/46] x86/irq: Prepare consolidation of irq_alloc_info Thomas Gleixner
2020-08-26 11:16 ` [patch V2 13/46] x86/msi: Consolidate HPET allocation Thomas Gleixner
2020-08-26 11:16 ` [patch V2 14/46] x86/ioapic: Consolidate IOAPIC allocation Thomas Gleixner
2020-09-08 13:35 ` Wei Liu
2020-08-26 11:16 ` [patch V2 15/46] x86/irq: Consolidate DMAR irq allocation Thomas Gleixner
2020-08-26 16:50 ` Dey, Megha
2020-08-26 18:32 ` Thomas Gleixner
2020-08-26 20:50 ` Thomas Gleixner
2020-08-28 0:12 ` Dey, Megha
2020-08-26 11:16 ` [patch V2 16/46] x86/irq: Consolidate UV domain allocation Thomas Gleixner
2020-08-26 11:16 ` [patch V2 17/46] PCI/MSI: Rework pci_msi_domain_calc_hwirq() Thomas Gleixner
2020-08-26 20:24 ` Marc Zyngier
2020-08-26 11:16 ` [patch V2 18/46] x86/msi: Consolidate MSI allocation Thomas Gleixner
2020-09-08 13:36 ` Wei Liu
2020-08-26 11:16 ` [patch V2 19/46] x86/msi: Use generic MSI domain ops Thomas Gleixner
2020-08-26 20:21 ` Marc Zyngier
2020-08-26 20:43 ` Thomas Gleixner
2020-08-26 11:16 ` [patch V2 20/46] x86/irq: Move apic_post_init() invocation to one place Thomas Gleixner
2020-08-26 11:16 ` [patch V2 21/46] x86/pci: Reducde #ifdeffery in PCI init code Thomas Gleixner
2020-08-26 11:16 ` [patch V2 22/46] x86/irq: Initialize PCI/MSI domain at PCI init time Thomas Gleixner
2020-08-26 11:16 ` [patch V2 23/46] irqdomain/msi: Provide DOMAIN_BUS_VMD_MSI Thomas Gleixner
2020-08-26 20:42 ` Marc Zyngier
2020-08-26 20:57 ` Derrick, Jonathan
2020-08-26 11:16 ` [patch V2 24/46] PCI: vmd: Mark VMD irqdomain with DOMAIN_BUS_VMD_MSI Thomas Gleixner
2020-08-26 20:47 ` Marc Zyngier
2020-08-31 14:39 ` Jason Gunthorpe
2020-09-30 12:45 ` Derrick, Jonathan
2020-09-30 12:57 ` Jason Gunthorpe
2020-09-30 13:08 ` Derrick, Jonathan
2020-09-30 18:47 ` Jason Gunthorpe
2020-08-26 11:16 ` [patch V2 25/46] PCI/MSI: Provide pci_dev_has_special_msi_domain() helper Thomas Gleixner
2020-08-26 11:16 ` [patch V2 26/46] x86/xen: Make xen_msi_init() static and rename it to xen_hvm_msi_init() Thomas Gleixner
2020-08-26 11:16 ` [patch V2 27/46] x86/xen: Rework MSI teardown Thomas Gleixner
2020-08-27 7:46 ` Jürgen Groß
2020-08-26 11:16 ` [patch V2 28/46] x86/xen: Consolidate XEN-MSI init Thomas Gleixner
2020-08-27 7:47 ` Jürgen Groß
2020-08-26 11:16 ` [patch V2 29/46] irqdomain/msi: Allow to override msi_domain_alloc/free_irqs() Thomas Gleixner
2020-08-26 19:06 ` Marc Zyngier
2020-08-26 19:47 ` Thomas Gleixner
2020-08-26 21:33 ` Marc Zyngier
2020-08-28 0:24 ` Dey, Megha
2020-08-26 11:16 ` [patch V2 30/46] x86/xen: Wrap XEN MSI management into irqdomain Thomas Gleixner
2020-08-26 11:16 ` [patch V2 31/46] iommm/vt-d: Store irq domain in struct device Thomas Gleixner
2020-08-26 11:17 ` [patch V2 32/46] iommm/amd: " Thomas Gleixner
2020-08-26 11:17 ` [patch V2 33/46] x86/pci: Set default irq domain in pcibios_add_device() Thomas Gleixner
2020-08-26 11:17 ` [patch V2 34/46] PCI/MSI: Make arch_.*_msi_irq[s] fallbacks selectable Thomas Gleixner
2020-08-26 15:53 ` Thomas Gleixner
2020-08-26 21:14 ` Marc Zyngier
2020-08-26 21:27 ` Thomas Gleixner
2020-08-27 18:20 ` Bjorn Helgaas
2020-08-28 11:21 ` Lorenzo Pieralisi
2020-08-28 12:19 ` Jason Gunthorpe
2020-08-28 12:47 ` Marc Zyngier
2020-08-28 12:54 ` Jason Gunthorpe
2020-08-28 13:52 ` Marc Zyngier
2020-08-28 18:29 ` Thomas Gleixner
2020-09-25 13:54 ` Qian Cai
2020-09-26 12:38 ` Vasily Gorbik
2020-09-28 10:11 ` Thomas Gleixner
2020-08-26 11:17 ` [patch V2 35/46] x86/irq: Cleanup the arch_*_msi_irqs() leftovers Thomas Gleixner
2020-08-26 11:17 ` [patch V2 36/46] x86/irq: Make most MSI ops XEN private Thomas Gleixner
2020-08-26 11:17 ` [patch V2 37/46] iommu/vt-d: Remove domain search for PCI/MSI[X] Thomas Gleixner
2020-08-26 11:17 ` [patch V2 38/46] iommu/amd: Remove domain search for PCI/MSI Thomas Gleixner
2020-08-26 11:17 ` [patch V2 39/46] x86/irq: Add DEV_MSI allocation type Thomas Gleixner
2020-08-26 11:17 ` [patch V2 40/46] x86/msi: Rename and rework pci_msi_prepare() to cover non-PCI MSI Thomas Gleixner
2020-08-26 11:17 ` [patch V2 41/46] platform-msi: Provide default irq_chip:: Ack Thomas Gleixner
2020-08-26 21:25 ` Marc Zyngier
2020-08-26 11:17 ` [patch V2 42/46] genirq/proc: Take buslock on affinity write Thomas Gleixner
2020-08-26 11:17 ` [patch V2 43/46] genirq/msi: Provide and use msi_domain_set_default_info_flags() Thomas Gleixner
2020-08-27 8:17 ` Marc Zyngier
2020-08-28 18:42 ` Thomas Gleixner
2020-08-26 11:17 ` [patch V2 44/46] platform-msi: Add device MSI infrastructure Thomas Gleixner
2020-08-26 11:17 ` [patch V2 45/46] irqdomain/msi: Provide msi_alloc/free_store() callbacks Thomas Gleixner
2020-08-26 11:17 ` [patch V2 46/46] irqchip: Add IMS (Interrupt Message Storm) driver - NOT FOR MERGING Thomas Gleixner
2020-08-31 14:45 ` Jason Gunthorpe
2020-08-28 11:41 ` [patch V2 00/46] x86, PCI, XEN, genirq ...: Prepare for device MSI Joerg Roedel
2020-08-31 0:51 ` Lu Baolu
2020-08-31 7:10 ` Thomas Gleixner
2020-08-31 7:29 ` Lu Baolu
2020-09-01 9:06 ` Boqun Feng
2020-09-03 16:35 ` Raj, Ashok
2020-09-03 18:12 ` Thomas Gleixner
2020-09-08 3:39 ` Russ Anderson
2020-09-25 15:29 ` Qian Cai
2020-09-25 15:49 ` Peter Zijlstra
2020-09-25 23:14 ` Thomas Gleixner
2020-09-27 8:46 ` [PATCH] x86/apic/msi: Unbreak DMAR and HPET MSI Thomas Gleixner
2020-09-29 23:03 ` [patch V2 00/46] x86, PCI, XEN, genirq ...: Prepare for device MSI Dey, Megha
2020-09-30 6:41 ` Thomas Gleixner
2020-09-30 11:43 ` Jason Gunthorpe
2020-09-30 15:20 ` Thomas Gleixner
2020-09-30 17:25 ` Dey, Megha
2020-09-30 18:11 ` Thomas Gleixner
2020-11-12 12:55 ` REGRESSION: " Jason Gunthorpe
2020-11-12 14:15 ` Thomas Gleixner
2020-11-12 15:18 ` Thomas Gleixner
2020-11-12 19:15 ` iommu/vt-d: Cure VF irqdomain hickup Thomas Gleixner
2020-11-12 21:34 ` Thomas Gleixner
2020-11-13 9:19 ` Marc Zyngier
2020-11-13 13:52 ` Thomas Gleixner
2020-11-13 7:20 ` Lu Baolu
2020-11-16 9:47 ` Geert Uytterhoeven
2020-11-16 12:50 ` Thomas Gleixner
2020-11-16 12:50 ` Lu Baolu
2020-11-16 23:22 ` Jason Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a6yh2nln.wl-maz@kernel.org \
--to=maz@kernel.org \
--cc=alex.williamson@redhat.com \
--cc=baolu.lu@intel.com \
--cc=bhelgaas@google.com \
--cc=boris.ostrovsky@oracle.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=haiyangz@microsoft.com \
--cc=iommu@lists.linux-foundation.org \
--cc=jacob.jun.pan@intel.com \
--cc=jgg@mellanox.com \
--cc=jgross@suse.com \
--cc=jonathan.derrick@intel.com \
--cc=kevin.tian@intel.com \
--cc=konrad.wilk@oracle.com \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=megha.dey@intel.com \
--cc=rafael@kernel.org \
--cc=rja@hpe.com \
--cc=sivanich@hpe.com \
--cc=sstabellini@kernel.org \
--cc=steve.wahl@hpe.com \
--cc=sthemmin@microsoft.com \
--cc=tglx@linutronix.de \
--cc=wei.liu@kernel.org \
--cc=x86@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).