| Message ID | 20210720134429.511541-9-boqun.feng@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: hv: Support host bridge probing on ARM64 | expand |
On Tue, 20 Jul 2021 14:44:29 +0100, Boqun Feng <boqun.feng@gmail.com> wrote: > > Now we have everything we need, just provide a proper sysdata type for > the bus to use on ARM64 and everything else works. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > drivers/pci/controller/pci-hyperv.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index e6276aaa4659..62dbe98d1fe1 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -40,6 +40,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/pci.h> > +#include <linux/pci-ecam.h> > #include <linux/delay.h> > #include <linux/semaphore.h> > #include <linux/irqdomain.h> > @@ -448,7 +449,11 @@ enum hv_pcibus_state { > }; > > struct hv_pcibus_device { > +#ifdef CONFIG_X86 > struct pci_sysdata sysdata; > +#elif defined(CONFIG_ARM64) > + struct pci_config_window sysdata; > +#endif Am I the only one who find this rather odd? Nothing ever populates this data structure on arm64, and its only purpose seems to serve as an anchor to retrieve the hbus via container_of(). If that's indeed the case, I'd rather see an arch-specific to_hbus() helper that uses another (preexisting) field as the anchor for arm64. Thanks, M.
On Tue, Jul 20, 2021 at 03:38:26PM +0100, Marc Zyngier wrote: > On Tue, 20 Jul 2021 14:44:29 +0100, > Boqun Feng <boqun.feng@gmail.com> wrote: > > > > Now we have everything we need, just provide a proper sysdata type for > > the bus to use on ARM64 and everything else works. > > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > --- > > drivers/pci/controller/pci-hyperv.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > > index e6276aaa4659..62dbe98d1fe1 100644 > > --- a/drivers/pci/controller/pci-hyperv.c > > +++ b/drivers/pci/controller/pci-hyperv.c > > @@ -40,6 +40,7 @@ > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/pci.h> > > +#include <linux/pci-ecam.h> > > #include <linux/delay.h> > > #include <linux/semaphore.h> > > #include <linux/irqdomain.h> > > @@ -448,7 +449,11 @@ enum hv_pcibus_state { > > }; > > > > struct hv_pcibus_device { > > +#ifdef CONFIG_X86 > > struct pci_sysdata sysdata; > > +#elif defined(CONFIG_ARM64) > > + struct pci_config_window sysdata; > > +#endif > > Am I the only one who find this rather odd? Nothing ever populates > this data structure on arm64, and its only purpose seems to serve as > an anchor to retrieve the hbus via container_of(). > This field will also be used as the ->sysdata of pci_bus and pci_host_bridge, and some of the PCI core code touches. Although I made this field as all zeroed and make sure PCI core can handle (patch #4). > If that's indeed the case, I'd rather see an arch-specific to_hbus() > helper that uses another (preexisting) field as the anchor for arm64. > I did a quick look, but I didn't find another field works: the field needs to be placed inside hv_pcibus_device and the address can be retrieved via pci_bus. I'm open to any suggestion in case that I missed something. Regards, Boqun > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
On Tue, 20 Jul 2021 15:59:41 +0100, Boqun Feng <boqun.feng@gmail.com> wrote: > > On Tue, Jul 20, 2021 at 03:38:26PM +0100, Marc Zyngier wrote: > > On Tue, 20 Jul 2021 14:44:29 +0100, > > Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > Now we have everything we need, just provide a proper sysdata type for > > > the bus to use on ARM64 and everything else works. > > > > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > > --- > > > drivers/pci/controller/pci-hyperv.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > > > index e6276aaa4659..62dbe98d1fe1 100644 > > > --- a/drivers/pci/controller/pci-hyperv.c > > > +++ b/drivers/pci/controller/pci-hyperv.c > > > @@ -40,6 +40,7 @@ > > > #include <linux/kernel.h> > > > #include <linux/module.h> > > > #include <linux/pci.h> > > > +#include <linux/pci-ecam.h> > > > #include <linux/delay.h> > > > #include <linux/semaphore.h> > > > #include <linux/irqdomain.h> > > > @@ -448,7 +449,11 @@ enum hv_pcibus_state { > > > }; > > > > > > struct hv_pcibus_device { > > > +#ifdef CONFIG_X86 > > > struct pci_sysdata sysdata; > > > +#elif defined(CONFIG_ARM64) > > > + struct pci_config_window sysdata; > > > +#endif > > > > Am I the only one who find this rather odd? Nothing ever populates > > this data structure on arm64, and its only purpose seems to serve as > > an anchor to retrieve the hbus via container_of(). > > > > This field will also be used as the ->sysdata of pci_bus and > pci_host_bridge, and some of the PCI core code touches. Although I made > this field as all zeroed and make sure PCI core can handle (patch #4). Huh, I see. I missed this particular nugget. This is so convoluted... > > If that's indeed the case, I'd rather see an arch-specific to_hbus() > > helper that uses another (preexisting) field as the anchor for arm64. > > > > I did a quick look, but I didn't find another field works: the field > needs to be placed inside hv_pcibus_device and the address can be > retrieved via pci_bus. I'm open to any suggestion in case that I missed > something. No, the above pretty much kills my suggestion. Thanks for the explanation, M.
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index e6276aaa4659..62dbe98d1fe1 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -40,6 +40,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/pci.h> +#include <linux/pci-ecam.h> #include <linux/delay.h> #include <linux/semaphore.h> #include <linux/irqdomain.h> @@ -448,7 +449,11 @@ enum hv_pcibus_state { }; struct hv_pcibus_device { +#ifdef CONFIG_X86 struct pci_sysdata sysdata; +#elif defined(CONFIG_ARM64) + struct pci_config_window sysdata; +#endif struct pci_host_bridge *bridge; struct fwnode_handle *fwnode; /* Protocol version negotiated with the host */ @@ -3075,7 +3080,9 @@ static int hv_pci_probe(struct hv_device *hdev, dom_req, dom); hbus->bridge->domain_nr = dom; +#ifdef CONFIG_X86 hbus->sysdata.domain = dom; +#endif hbus->hdev = hdev; INIT_LIST_HEAD(&hbus->children);
Now we have everything we need, just provide a proper sysdata type for the bus to use on ARM64 and everything else works. Signed-off-by: Boqun Feng <boqun.feng@gmail.com> --- drivers/pci/controller/pci-hyperv.c | 7 +++++++ 1 file changed, 7 insertions(+)