From: Marc Zyngier <maz@kernel.org> To: Bjorn Helgaas <helgaas@kernel.org>, Boqun Feng <boqun.feng@gmail.com> Cc: Bjorn Helgaas <bhelgaas@google.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, "K. Y. Srinivasan" <kys@microsoft.com>, Haiyang Zhang <haiyangz@microsoft.com>, Stephen Hemminger <sthemmin@microsoft.com>, Wei Liu <wei.liu@kernel.org>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Rob Herring <robh@kernel.org>, Clint Sbisa <csbisa@amazon.com>, Ard Biesheuvel <ardb@kernel.org>, Sunil Muthuswamy <sunilmut@microsoft.com>, Arnd Bergmann <arnd@arndb.de> Subject: Re: [RFC 1/2] arm64: PCI: Allow use arch-specific pci sysdata Date: Sat, 20 Mar 2021 12:54:37 +0000 [thread overview] Message-ID: <87tup6gf3m.wl-maz@kernel.org> (raw) In-Reply-To: <20210319211246.GA250618@bjorn-Precision-5520> Thanks Bjorn for looping me in. On Fri, 19 Mar 2021 21:12:46 +0000, Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Arnd (author of 37d6a0a6f470 ("PCI: Add > pci_register_host_bridge() interface"), which I think would make my > idea below possible), Marc (IRQ domains maintainer)] > > On Sat, Mar 20, 2021 at 12:19:55AM +0800, Boqun Feng wrote: > > Currently, if an architecture selects CONFIG_PCI_DOMAINS_GENERIC, the > > ->sysdata in bus and bridge will be treated as struct pci_config_window, > > which is created by generic ECAM using the data from acpi. > > It might be a mistake that we put the struct pci_config_window > pointer, which is really arch-independent, in the ->sysdata element, > which normally contains a pointer to arch- or host bridge-dependent > data. > > > However, for a virtualized PCI bus, there might be no enough data in of > > or acpi table to create a pci_config_window. This is similar to the case > > where CONFIG_PCI_DOMAINS_GENERIC=n, IOW, architectures use their own > > structure for sysdata, so no apci table lookup is required. > > > > In order to enable Hyper-V's virtual PCI (which doesn't have acpi table > > entry for PCI) on ARM64 (which selects CONFIG_PCI_DOMAINS_GENERIC), we > > introduce arch-specific pci sysdata (similar to the one for x86) for > > ARM64, and allow the core PCI code to detect the type of sysdata at the > > runtime. The latter is achieved by adding a pci_ops::use_arch_sysdata > > field. > > > > Originally-by: Sunil Muthuswamy <sunilmut@microsoft.com> > > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> > > --- > > arch/arm64/include/asm/pci.h | 29 +++++++++++++++++++++++++++++ > > arch/arm64/kernel/pci.c | 15 ++++++++++++--- > > include/linux/pci.h | 3 +++ > > 3 files changed, 44 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h > > index b33ca260e3c9..dade061a0658 100644 > > --- a/arch/arm64/include/asm/pci.h > > +++ b/arch/arm64/include/asm/pci.h > > @@ -22,6 +22,16 @@ > > > > extern int isa_dma_bridge_buggy; > > > > +struct pci_sysdata { > > + int domain; /* PCI domain */ > > + int node; /* NUMA Node */ > > +#ifdef CONFIG_ACPI > > + struct acpi_device *companion; /* ACPI companion device */ > > +#endif > > +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN > > + void *fwnode; /* IRQ domain for MSI assignment */ Why isn't this more strongly typed? pci_host_bridge_msi_domain() definitely expects this to be the real thing. And the comment is wrong. [...] > > +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN > > +static inline void *_pci_root_bus_fwnode(struct pci_bus *bus) > > +{ > > + struct pci_sysdata *sd = bus->sysdata; > > + > > + if (bus->ops->use_arch_sysdata) > > + return sd->fwnode; > > + > > + /* > > + * bus->sysdata is not struct pci_sysdata, fwnode should be able to > > + * be queried from of/acpi. > > + */ > > + return NULL; > > +} > > +#define pci_root_bus_fwnode _pci_root_bus_fwnode > > Ugh. pci_root_bus_fwnode() is another callback to find the > irq_domain. Only one call, from pci_host_bridge_msi_domain(), which > itself is only called from pci_set_bus_msi_domain(). This feels like > another case where we could simplify things by having the host bridge > driver figure out the irq_domain explicitly when it creates the > pci_host_bridge. It seems like that's where we have the most > information about how to find the irq_domain. Urgh. This is a perfect copy paste of the x86 horror, warts and all. I can't say I'm thrilled (another way to say "Gawd, Noes! Never!"). One thing I am sure of is that I do not want to add more custom indirection to build the MSI topology. We barely got rid of the msi_controller structure, and this is the same thing by another name. Probably worse, actually. In this case, I don't see the point in going via a fwnode indirection given that there is no firmware tables the first place. As for finding the irq domain from the host bridge, that's not doable in most cases on arm64, as it is pretty likely that the host bridge knows nothing about MSIs when they are implemented in the GIC (see my recent msi_controller removal series that has a few patches about that). Having an optional callback to host bridges to obtain the MSI domain may be possible in some cases though (there might be a chicken/egg problem for some drivers though...). Thanks, M. -- Without deviation from the norm, progress is not possible.
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Bjorn Helgaas <helgaas@kernel.org>, Boqun Feng <boqun.feng@gmail.com> Cc: Bjorn Helgaas <bhelgaas@google.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, "K. Y. Srinivasan" <kys@microsoft.com>, Haiyang Zhang <haiyangz@microsoft.com>, Stephen Hemminger <sthemmin@microsoft.com>, Wei Liu <wei.liu@kernel.org>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Rob Herring <robh@kernel.org>, Clint Sbisa <csbisa@amazon.com>, Ard Biesheuvel <ardb@kernel.org>, Sunil Muthuswamy <sunilmut@microsoft.com>, Arnd Bergmann <arnd@arndb.de> Subject: Re: [RFC 1/2] arm64: PCI: Allow use arch-specific pci sysdata Date: Sat, 20 Mar 2021 12:54:37 +0000 [thread overview] Message-ID: <87tup6gf3m.wl-maz@kernel.org> (raw) In-Reply-To: <20210319211246.GA250618@bjorn-Precision-5520> Thanks Bjorn for looping me in. On Fri, 19 Mar 2021 21:12:46 +0000, Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Arnd (author of 37d6a0a6f470 ("PCI: Add > pci_register_host_bridge() interface"), which I think would make my > idea below possible), Marc (IRQ domains maintainer)] > > On Sat, Mar 20, 2021 at 12:19:55AM +0800, Boqun Feng wrote: > > Currently, if an architecture selects CONFIG_PCI_DOMAINS_GENERIC, the > > ->sysdata in bus and bridge will be treated as struct pci_config_window, > > which is created by generic ECAM using the data from acpi. > > It might be a mistake that we put the struct pci_config_window > pointer, which is really arch-independent, in the ->sysdata element, > which normally contains a pointer to arch- or host bridge-dependent > data. > > > However, for a virtualized PCI bus, there might be no enough data in of > > or acpi table to create a pci_config_window. This is similar to the case > > where CONFIG_PCI_DOMAINS_GENERIC=n, IOW, architectures use their own > > structure for sysdata, so no apci table lookup is required. > > > > In order to enable Hyper-V's virtual PCI (which doesn't have acpi table > > entry for PCI) on ARM64 (which selects CONFIG_PCI_DOMAINS_GENERIC), we > > introduce arch-specific pci sysdata (similar to the one for x86) for > > ARM64, and allow the core PCI code to detect the type of sysdata at the > > runtime. The latter is achieved by adding a pci_ops::use_arch_sysdata > > field. > > > > Originally-by: Sunil Muthuswamy <sunilmut@microsoft.com> > > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> > > --- > > arch/arm64/include/asm/pci.h | 29 +++++++++++++++++++++++++++++ > > arch/arm64/kernel/pci.c | 15 ++++++++++++--- > > include/linux/pci.h | 3 +++ > > 3 files changed, 44 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h > > index b33ca260e3c9..dade061a0658 100644 > > --- a/arch/arm64/include/asm/pci.h > > +++ b/arch/arm64/include/asm/pci.h > > @@ -22,6 +22,16 @@ > > > > extern int isa_dma_bridge_buggy; > > > > +struct pci_sysdata { > > + int domain; /* PCI domain */ > > + int node; /* NUMA Node */ > > +#ifdef CONFIG_ACPI > > + struct acpi_device *companion; /* ACPI companion device */ > > +#endif > > +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN > > + void *fwnode; /* IRQ domain for MSI assignment */ Why isn't this more strongly typed? pci_host_bridge_msi_domain() definitely expects this to be the real thing. And the comment is wrong. [...] > > +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN > > +static inline void *_pci_root_bus_fwnode(struct pci_bus *bus) > > +{ > > + struct pci_sysdata *sd = bus->sysdata; > > + > > + if (bus->ops->use_arch_sysdata) > > + return sd->fwnode; > > + > > + /* > > + * bus->sysdata is not struct pci_sysdata, fwnode should be able to > > + * be queried from of/acpi. > > + */ > > + return NULL; > > +} > > +#define pci_root_bus_fwnode _pci_root_bus_fwnode > > Ugh. pci_root_bus_fwnode() is another callback to find the > irq_domain. Only one call, from pci_host_bridge_msi_domain(), which > itself is only called from pci_set_bus_msi_domain(). This feels like > another case where we could simplify things by having the host bridge > driver figure out the irq_domain explicitly when it creates the > pci_host_bridge. It seems like that's where we have the most > information about how to find the irq_domain. Urgh. This is a perfect copy paste of the x86 horror, warts and all. I can't say I'm thrilled (another way to say "Gawd, Noes! Never!"). One thing I am sure of is that I do not want to add more custom indirection to build the MSI topology. We barely got rid of the msi_controller structure, and this is the same thing by another name. Probably worse, actually. In this case, I don't see the point in going via a fwnode indirection given that there is no firmware tables the first place. As for finding the irq domain from the host bridge, that's not doable in most cases on arm64, as it is pretty likely that the host bridge knows nothing about MSIs when they are implemented in the GIC (see my recent msi_controller removal series that has a few patches about that). Having an optional callback to host bridges to obtain the MSI domain may be possible in some cases though (there might be a chicken/egg problem for some drivers though...). Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-20 12:55 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-19 16:19 [RFC 0/2] PCI: Introduce pci_ops::use_arch_sysdata Boqun Feng 2021-03-19 16:19 ` Boqun Feng 2021-03-19 16:19 ` [RFC 1/2] arm64: PCI: Allow use arch-specific pci sysdata Boqun Feng 2021-03-19 16:19 ` Boqun Feng 2021-03-19 21:12 ` Bjorn Helgaas 2021-03-19 21:12 ` Bjorn Helgaas 2021-03-20 12:54 ` Marc Zyngier [this message] 2021-03-20 12:54 ` Marc Zyngier 2021-03-20 13:03 ` Arnd Bergmann 2021-03-20 13:03 ` Arnd Bergmann 2021-03-20 13:23 ` Marc Zyngier 2021-03-20 13:23 ` Marc Zyngier 2021-03-20 14:24 ` Arnd Bergmann 2021-03-20 14:24 ` Arnd Bergmann 2021-03-20 17:14 ` Marc Zyngier 2021-03-20 17:14 ` Marc Zyngier 2021-03-20 12:54 ` Arnd Bergmann 2021-03-20 12:54 ` Arnd Bergmann 2021-03-20 16:09 ` Arnd Bergmann 2021-03-20 16:09 ` Arnd Bergmann 2021-03-29 14:32 ` Boqun Feng 2021-03-29 14:32 ` Boqun Feng 2021-03-29 14:43 ` Arnd Bergmann 2021-03-29 14:43 ` Arnd Bergmann 2021-03-20 12:52 ` Arnd Bergmann 2021-03-20 12:52 ` Arnd Bergmann 2021-03-19 16:19 ` [RFC 2/2] PCI: hv: Tell PCI core arch-specific sysdata is used Boqun Feng 2021-03-19 16:19 ` Boqun Feng 2021-03-19 19:04 ` [RFC 0/2] PCI: Introduce pci_ops::use_arch_sysdata Bjorn Helgaas 2021-03-19 19:04 ` Bjorn Helgaas
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=87tup6gf3m.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=ardb@kernel.org \ --cc=arnd@arndb.de \ --cc=bhelgaas@google.com \ --cc=boqun.feng@gmail.com \ --cc=catalin.marinas@arm.com \ --cc=csbisa@amazon.com \ --cc=haiyangz@microsoft.com \ --cc=helgaas@kernel.org \ --cc=kys@microsoft.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-hyperv@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=robh@kernel.org \ --cc=sthemmin@microsoft.com \ --cc=sunilmut@microsoft.com \ --cc=wei.liu@kernel.org \ --cc=will@kernel.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.