Re: [PATCH v6 8/8] PCI: hv: Turn on the host bridge probing on ARM64

[Lorenzo Pieralisi]

On Mon, Aug 09, 2021 at 10:38:48PM +0800, Boqun Feng wrote:
> On Tue, Aug 03, 2021 at 06:14:51PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Jul 27, 2021 at 02:06:57AM +0800, Boqun Feng 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@xxxxxxxxx>
> > > ---
> > >  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;
> > 
> > This is ugly. HV does not need pci_config_window at all right
> > (other than arm64 pcibios_root_bridge_prepare()) ?
> > 
> 
> Right.
> 
> > The issue is that in HV you have to have *some* sysdata != NULL, it is
> > just some data to retrieve the hv_pcibus_device.
> > 
> > Mmaybe we can rework ARM64 ACPI code to store the acpi_device in struct
> > pci_host_bridge->private instead of retrieving it from pci_config_window
> > so that we decouple HV from the ARM64 back-end.
> > 
> > HV would just set struct pci_host_bridge->private == NULL.
> > 
> 
> Works for me, but please note that pci_sysdata is an x86-specific
> structure, so we still need to define a fake pci_sysdata inside
> pci-hyperv.c, like:
> 
> 	#ifndef CONFIG_X86
> 	struct pci_sysdata { };
> 	#end
> 
> > I need to think about this a bit, I don't think it should block
> > this series though but it would be nicer.
> 
> After a quick look into the code, seems that what we need to do is to
> add an additional parameter for acpi_pci_root_create() and introduce a
> slightly different version of pci_create_root_bus(). A question is:
> should we only do this for ARM64, or should we also do this for
> other acpi_pci_root_create() users (x86 and ia64)? Another question
> comes to my mind is, while we are at it, is there anything else that we
> want to move from sysdata to ->private? These questions are out of scope
> of this patchset, I think. Maybe it's better that we address them in the
> future, and I can send out separate RFC patches to start the discussion.
> Does that sound like a plan to you?

Yes it does and we can start from ARM64 - what I really don't like
is the arch/arm64 dependency with the HV controller driver as I
described, being forced to have a struct pci_config_window in the
driver is not really nice or clean IMO.

Not that I expect any other PCI host bridge driver with ACPI coming
anytime soon but even if it is not within set (that we can merge) I'd
like to see the decoupling rework done asap, let me put it this way.

Thanks,
Lorenzo

> Regards,
> Boqun
> 
> > 
> > Lorenzo
> > 
> > > +#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);
> > > -- 
> > > 2.32.0
> > >