Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1636080 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 11/13] PCI: Add has_mem64 for struct host_bridge Bjorn Helgaas <helgaas@kernel.org> - 2017-05-05 01:10 +0200
    Re: [PATCH 11/13] PCI: Add has_mem64 for struct host_bridge Christian König <deathsimple@vodafone.de> - 2017-05-08 11:00 +0200
      Re: [PATCH 11/13] PCI: Add has_mem64 for struct host_bridge Bjorn Helgaas <helgaas@kernel.org> - 2017-05-08 15:30 +0200
        Re: [PATCH 11/13] PCI: Add has_mem64 for struct host_bridge Christian König <deathsimple@vodafone.de> - 2017-05-09 13:50 +0200

#1636080 — Re: [PATCH 11/13] PCI: Add has_mem64 for struct host_bridge

[+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 <yinghai@kernel.org>
> Tested-by: Khalid Aziz <khalid.aziz@oracle.com>
> ---
>  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/1493890270-1188-5-git-send-email-deathsimple@vodafone.de

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
> 

[toc] | [next] | [standalone]

#1637314

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/1493890270-1188-5-git-send-email-deathsimple@vodafone.de
>
> 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.

[toc] | [prev] | [next] | [standalone]

#1637443

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/1493890270-1188-5-git-send-email-deathsimple@vodafone.de
> >
> >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.

[toc] | [prev] | [next] | [standalone]

#1638057

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/1493890270-1188-5-git-send-email-deathsimple@vodafone.de
>>>
>>> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web