[11/13] PCI: Add has_mem64 for struct host_bridge

Commit Message

Yinghai Lu April 21, 2017, 5:04 a.m. UTC

Add has_mem64 for struct host_bridge, on root bus that does not support
mmio64 above 4g, will not set that.

We will use that info next two following patches:
1. Don't treat non-pref mmio64 as pref mmio, so will not put
   it under bridge's pref range when rescan the devices
2. will keep pref mmio64 and pref mmio32 under bridge pref bar.

Signed-off-by: Yinghai Lu <[e-mail]>
Tested-by: Khalid Aziz <[e-mail]>
---
 drivers/pci/probe.c | 7 +++++++
 include/linux/pci.h | 1 +
 2 files changed, 8 insertions(+)

Comments

Bjorn Helgaas May 4, 2017, 11:04 p.m. UTC | #1

[+cc Christian]

On Thu, Apr 20, 2017 at 10:04:58PM -0700, Yinghai Lu wrote:
> Add has_mem64 for struct host_bridge, on root bus that does not support
> mmio64 above 4g, will not set that.
> 
> We will use that info next two following patches:
> 1. Don't treat non-pref mmio64 as pref mmio, so will not put
>    it under bridge's pref range when rescan the devices
> 2. will keep pref mmio64 and pref mmio32 under bridge pref bar.
> 
> Signed-off-by: Yinghai Lu <[e-mail]>
> Tested-by: Khalid Aziz <[e-mail]>
> ---
>  drivers/pci/probe.c | 7 +++++++
>  include/linux/pci.h | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 676b55f..8f439e0 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -818,6 +818,13 @@ int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  			addr[0] = '\0';
>  
>  		dev_info(&bus->dev, "root bus resource %pR%s\n", res, addr);
> +
> +		if (resource_type(res) == IORESOURCE_MEM) {
> +			if ((res->end - offset) > 0xffffffff)
> +				bridge->has_mem64 = 1;

This part makes sense -- if any part of the window extends above 4G,
only a 64-bit BAR can use the part above 4G.

> +			if ((res->start - offset) > 0xffffffff)
> +				res->flags |= IORESOURCE_MEM_64;

But I don't understand this part.  You only set IORESOURCE_MEM_64 if
the *start* is above 4G?  If the window started at 2GB and ended at
6GB, you wouldn't set IORESOURCE_MEM_64?

And I don't understand where this IORESOURCE_MEM_64 in res->flags is
used.  It seems like the only possible place is this test added by the
last patch:

  int pci_bus_alloc_resource(bus, res, ...)
  {
    if (res->flags & mmio64) {

but this patch is setting IORESOURCE_MEM_64 in the host bridge window,
i.e., the bus resource, and pci_bus_alloc_resource() is testing the
flags of the resource we're allocating *from* the bus resource.

> +		}

I *think* this will be broken by the current implementation of
Christian's patch to enable a 64-bit host bridge window:

  https://lkml.kernel.org/r/[e-mail]

because pci_register_host_bridge() runs before we scan the bus, and
Christian's patch adds a quirk that runs when we enumerate the AMD
host bridge device.

If we apply this and Christian's patch, I think we could end up with
a host bridge window above 4G, but with bridge->has_mem64 not set.

>  	}
>  
>  	down_write(&pci_bus_sem);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index b14dd94..a3693ef 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -436,6 +436,7 @@ struct pci_host_bridge {
>  	void *release_data;
>  	struct msi_controller *msi;
>  	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
> +	unsigned int has_mem64:1;
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
>  			const struct resource *res,
> -- 
> 2.9.3
>

Christian König May 8, 2017, 8:54 a.m. UTC | #2

Am 05.05.2017 um 01:04 schrieb Bjorn Helgaas:
> [+cc Christian]
Thanks for that.

> [SNIP]
> I *think* this will be broken by the current implementation of
> Christian's patch to enable a 64-bit host bridge window:
>
>    https://lkml.kernel.org/r/[e-mail]
>
> because pci_register_host_bridge() runs before we scan the bus, and
> Christian's patch adds a quirk that runs when we enumerate the AMD
> host bridge device.
>
> If we apply this and Christian's patch, I think we could end up with
> a host bridge window above 4G, but with bridge->has_mem64 not set.
Yes, indeed. I can adjust my patch, but I would prefer not to do so.

I don't completely understand the background of this change, but from 
what I know how the BIOS (at least on X86) allocates resources it 
doesn't sounds correct to me.

Maybe we just need a Sparc specific quirk here instead of changing the 
common logic?

Regards,
Christian.

Bjorn Helgaas May 8, 2017, 1:25 p.m. UTC | #3

On Mon, May 08, 2017 at 10:54:55AM +0200, Christian König wrote:
> Am 05.05.2017 um 01:04 schrieb Bjorn Helgaas:
> >I *think* this will be broken by the current implementation of
> >Christian's patch to enable a 64-bit host bridge window:
> >
> >   https://lkml.kernel.org/r/[e-mail]
> >
> >because pci_register_host_bridge() runs before we scan the bus, and
> >Christian's patch adds a quirk that runs when we enumerate the AMD
> >host bridge device.
> >
> >If we apply this and Christian's patch, I think we could end up with
> >a host bridge window above 4G, but with bridge->has_mem64 not set.
> Yes, indeed. I can adjust my patch, but I would prefer not to do so.
> 
> I don't completely understand the background of this change, but
> from what I know how the BIOS (at least on X86) allocates resources
> it doesn't sounds correct to me.
> 
> Maybe we just need a Sparc specific quirk here instead of changing
> the common logic?

There's nothing in Yinghai's patch that's conceptually Sparc-specific,
so I would prefer not to artificially tie it to Sparc.

One possibility would be to compute has_mem64 when we need it instead
of caching it.

Christian König May 9, 2017, 11:38 a.m. UTC | #4

Am 08.05.2017 um 15:25 schrieb Bjorn Helgaas:
> On Mon, May 08, 2017 at 10:54:55AM +0200, Christian König wrote:
>> Am 05.05.2017 um 01:04 schrieb Bjorn Helgaas:
>>> I *think* this will be broken by the current implementation of
>>> Christian's patch to enable a 64-bit host bridge window:
>>>
>>>    https://lkml.kernel.org/r/[e-mail]
>>>
>>> because pci_register_host_bridge() runs before we scan the bus, and
>>> Christian's patch adds a quirk that runs when we enumerate the AMD
>>> host bridge device.
>>>
>>> If we apply this and Christian's patch, I think we could end up with
>>> a host bridge window above 4G, but with bridge->has_mem64 not set.
>> Yes, indeed. I can adjust my patch, but I would prefer not to do so.
>>
>> I don't completely understand the background of this change, but
>> from what I know how the BIOS (at least on X86) allocates resources
>> it doesn't sounds correct to me.
>>
>> Maybe we just need a Sparc specific quirk here instead of changing
>> the common logic?
> There's nothing in Yinghai's patch that's conceptually Sparc-specific,
> so I would prefer not to artificially tie it to Sparc.

That's possible, I would need to take a closer look on them which I 
currently don't have time for.

It was more of a gut feeling considering that the current allocation 
code already looks rather complex.

> One possibility would be to compute has_mem64 when we need it instead
> of caching it.

Sounds like a good idea to me as well. I mean the code using this isn't 
time critical, isn't it?

And scanning the parent resources if a 64bit window can be found 
shouldn't be much overhead.

Christian.

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 676b55f..8f439e0 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -818,6 +818,13 @@  int pci_register_host_bridge(struct pci_host_bridge *bridge)
 			addr[0] = '\0';
 
 		dev_info(&bus->dev, "root bus resource %pR%s\n", res, addr);
+
+		if (resource_type(res) == IORESOURCE_MEM) {
+			if ((res->end - offset) > 0xffffffff)
+				bridge->has_mem64 = 1;
+			if ((res->start - offset) > 0xffffffff)
+				res->flags |= IORESOURCE_MEM_64;
+		}
 	}
 
 	down_write(&pci_bus_sem);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b14dd94..a3693ef 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -436,6 +436,7 @@  struct pci_host_bridge {
 	void *release_data;
 	struct msi_controller *msi;
 	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
+	unsigned int has_mem64:1;
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,