[PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Alex Williamson]

Prior to 1f82de10 we always initialized the upper 32bits of the
prefetchable memory window, regardless of the address range used.
Now we only touch it for a >32bit address, which means the upper32
registers remain whatever the BIOS initialized them too.

It's valid for the BIOS to set the upper32 base/limit to
0xffffffff/0x00000000, which makes us program prefetchable ranges
like 0xffffffffabc00000 - 0x00000000abc00000

Revert the chunk of 1f82de10 that made this conditional so we always
write the upper32 registers and remove now unused pref_mem64 variable.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

v2:
  Remove pref_mem64 as it's now unused.

 drivers/pci/setup-bus.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index cb1a027..dd58c6a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -142,7 +142,6 @@ static void pci_setup_bridge(struct pci_bus *bus)
 	struct pci_dev *bridge = bus->self;
 	struct pci_bus_region region;
 	u32 l, bu, lu, io_upper16;
-	int pref_mem64;
 
 	if (pci_is_enabled(bridge))
 		return;
@@ -198,7 +197,6 @@ static void pci_setup_bridge(struct pci_bus *bus)
 	pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, 0);
 
 	/* Set up PREF base/limit. */
-	pref_mem64 = 0;
 	bu = lu = 0;
 	pcibios_resource_to_bus(bridge, &region, bus->resource[2]);
 	if (bus->resource[2]->flags & IORESOURCE_PREFETCH) {
@@ -206,7 +204,6 @@ static void pci_setup_bridge(struct pci_bus *bus)
 		l = (region.start >> 16) & 0xfff0;
 		l |= region.end & 0xfff00000;
 		if (bus->resource[2]->flags & IORESOURCE_MEM_64) {
-			pref_mem64 = 1;
 			bu = upper_32_bits(region.start);
 			lu = upper_32_bits(region.end);
 			width = 16;
@@ -221,11 +218,9 @@ static void pci_setup_bridge(struct pci_bus *bus)
 	}
 	pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, l);
 
-	if (pref_mem64) {
-		/* Set the upper 32 bits of PREF base & limit. */
-		pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
-		pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
-	}
+	/* Set the upper 32 bits of PREF base & limit. */
+	pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
+	pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
 
 	pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, bus->bridge_ctl);
 }

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Grant Grundler]

On Mon, Nov 30, 2009 at 02:51:44PM -0700, Alex Williamson wrote:
> Prior to 1f82de10 we always initialized the upper 32bits of the
> prefetchable memory window, regardless of the address range used.
> Now we only touch it for a >32bit address, which means the upper32
> registers remain whatever the BIOS initialized them too.
> 
> It's valid for the BIOS to set the upper32 base/limit to
> 0xffffffff/0x00000000, which makes us program prefetchable ranges
> like 0xffffffffabc00000 - 0x00000000abc00000
> 
> Revert the chunk of 1f82de10 that made this conditional so we always
> write the upper32 registers and remove now unused pref_mem64 variable.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

Reviewed-by: Grant Grundler <grundler@parisc-linux.org>

thanks,
grant

> ---
> 
> v2:
>   Remove pref_mem64 as it's now unused.
> 
>  drivers/pci/setup-bus.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index cb1a027..dd58c6a 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -142,7 +142,6 @@ static void pci_setup_bridge(struct pci_bus *bus)
>  	struct pci_dev *bridge = bus->self;
>  	struct pci_bus_region region;
>  	u32 l, bu, lu, io_upper16;
> -	int pref_mem64;
>  
>  	if (pci_is_enabled(bridge))
>  		return;
> @@ -198,7 +197,6 @@ static void pci_setup_bridge(struct pci_bus *bus)
>  	pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, 0);
>  
>  	/* Set up PREF base/limit. */
> -	pref_mem64 = 0;
>  	bu = lu = 0;
>  	pcibios_resource_to_bus(bridge, &region, bus->resource[2]);
>  	if (bus->resource[2]->flags & IORESOURCE_PREFETCH) {
> @@ -206,7 +204,6 @@ static void pci_setup_bridge(struct pci_bus *bus)
>  		l = (region.start >> 16) & 0xfff0;
>  		l |= region.end & 0xfff00000;
>  		if (bus->resource[2]->flags & IORESOURCE_MEM_64) {
> -			pref_mem64 = 1;
>  			bu = upper_32_bits(region.start);
>  			lu = upper_32_bits(region.end);
>  			width = 16;
> @@ -221,11 +218,9 @@ static void pci_setup_bridge(struct pci_bus *bus)
>  	}
>  	pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, l);
>  
> -	if (pref_mem64) {
> -		/* Set the upper 32 bits of PREF base & limit. */
> -		pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
> -		pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
> -	}
> +	/* Set the upper 32 bits of PREF base & limit. */
> +	pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
> +	pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
>  
>  	pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, bus->bridge_ctl);
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Grant Grundler]

On Mon, Nov 30, 2009 at 05:03:32PM -0700, Grant Grundler wrote:
> On Mon, Nov 30, 2009 at 02:51:44PM -0700, Alex Williamson wrote:
> > Prior to 1f82de10 we always initialized the upper 32bits of the
> > prefetchable memory window, regardless of the address range used.
> > Now we only touch it for a >32bit address, which means the upper32
> > registers remain whatever the BIOS initialized them too.
> > 
> > It's valid for the BIOS to set the upper32 base/limit to
> > 0xffffffff/0x00000000, which makes us program prefetchable ranges
> > like 0xffffffffabc00000 - 0x00000000abc00000
> > 
> > Revert the chunk of 1f82de10 that made this conditional so we always
> > write the upper32 registers and remove now unused pref_mem64 variable.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> 
> Reviewed-by: Grant Grundler <grundler@parisc-linux.org>

NAK this - I messed up. Yinghai is correct. Something else is going on.

It might be perfectly OK to read 0xffffffffabc00000 if the bridge
isn't using the upper32 Prefetchable register. Maybe the problem is
some code is reading the upper32 value without checking that it's valid?

grant

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Yinghai Lu]

Grant Grundler wrote:
> On Mon, Nov 30, 2009 at 05:03:32PM -0700, Grant Grundler wrote:
>> On Mon, Nov 30, 2009 at 02:51:44PM -0700, Alex Williamson wrote:
>>> Prior to 1f82de10 we always initialized the upper 32bits of the
>>> prefetchable memory window, regardless of the address range used.
>>> Now we only touch it for a >32bit address, which means the upper32
>>> registers remain whatever the BIOS initialized them too.
>>>
>>> It's valid for the BIOS to set the upper32 base/limit to
>>> 0xffffffff/0x00000000, which makes us program prefetchable ranges
>>> like 0xffffffffabc00000 - 0x00000000abc00000
>>>
>>> Revert the chunk of 1f82de10 that made this conditional so we always
>>> write the upper32 registers and remove now unused pref_mem64 variable.
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
>> Reviewed-by: Grant Grundler <grundler@parisc-linux.org>
> 
> NAK this - I messed up. Yinghai is correct. Something else is going on.
> 
> It might be perfectly OK to read 0xffffffffabc00000 if the bridge
> isn't using the upper32 Prefetchable register. Maybe the problem is
> some code is reading the upper32 value without checking that it's valid?

maybe Alex is using old lspci?

YH

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Alex Williamson]

On Mon, 2009-11-30 at 17:19 -0700, Grant Grundler wrote:
> On Mon, Nov 30, 2009 at 05:03:32PM -0700, Grant Grundler wrote:
> > On Mon, Nov 30, 2009 at 02:51:44PM -0700, Alex Williamson wrote:
> > > Prior to 1f82de10 we always initialized the upper 32bits of the
> > > prefetchable memory window, regardless of the address range used.
> > > Now we only touch it for a >32bit address, which means the upper32
> > > registers remain whatever the BIOS initialized them too.
> > > 
> > > It's valid for the BIOS to set the upper32 base/limit to
> > > 0xffffffff/0x00000000, which makes us program prefetchable ranges
> > > like 0xffffffffabc00000 - 0x00000000abc00000
> > > 
> > > Revert the chunk of 1f82de10 that made this conditional so we always
> > > write the upper32 registers and remove now unused pref_mem64 variable.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> > 
> > Reviewed-by: Grant Grundler <grundler@parisc-linux.org>
> 
> NAK this - I messed up. Yinghai is correct. Something else is going on.
> 
> It might be perfectly OK to read 0xffffffffabc00000 if the bridge
> isn't using the upper32 Prefetchable register. Maybe the problem is
> some code is reading the upper32 value without checking that it's valid?

Apologies for not threading the v2 patch into the original thread.  The
prefetchable base register does support the upper32 bits and it does
work correctly.  However per the pci-to-pci bridge spec, a little lower
on page 47, devices only supporting 32bit prefetchable ranges are to
implement the upper32 registers as read-only registers that return zero.
In the example above, -1 in the upper32 base simply means that base >
limit, which disables the range.

Further investigation shows that the MEM_64 resource flag is setup for
this range based on hardware capabilities, but then it gets removed in
pbus_size_mem() because we want to use the range to map a 32bit option
ROM.  This leaves us entering pci_setup_bridge() with -1 in the upper32
base and the MEM_64 flag clear, so we never touch the upper32 base
register.  I think this patch is still a simple, safe solution.  Thanks,

Alex

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Grant Grundler]

On Mon, Nov 30, 2009 at 09:10:54PM -0700, Alex Williamson wrote:
> On Mon, 2009-11-30 at 17:19 -0700, Grant Grundler wrote:
> > On Mon, Nov 30, 2009 at 05:03:32PM -0700, Grant Grundler wrote:
> > > On Mon, Nov 30, 2009 at 02:51:44PM -0700, Alex Williamson wrote:
> > > > Prior to 1f82de10 we always initialized the upper 32bits of the
> > > > prefetchable memory window, regardless of the address range used.
> > > > Now we only touch it for a >32bit address, which means the upper32
> > > > registers remain whatever the BIOS initialized them too.
> > > > 
> > > > It's valid for the BIOS to set the upper32 base/limit to
> > > > 0xffffffff/0x00000000, which makes us program prefetchable ranges
> > > > like 0xffffffffabc00000 - 0x00000000abc00000
> > > > 
> > > > Revert the chunk of 1f82de10 that made this conditional so we always
> > > > write the upper32 registers and remove now unused pref_mem64 variable.
> > > > 
> > > > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> > > 
> > > Reviewed-by: Grant Grundler <grundler@parisc-linux.org>
> > 
> > NAK this - I messed up. Yinghai is correct. Something else is going on.
> > 
> > It might be perfectly OK to read 0xffffffffabc00000 if the bridge
> > isn't using the upper32 Prefetchable register. Maybe the problem is
> > some code is reading the upper32 value without checking that it's valid?
> 
> Apologies for not threading the v2 patch into the original thread.  The
> prefetchable base register does support the upper32 bits and it does
> work correctly.  However per the pci-to-pci bridge spec, a little lower
> on page 47, devices only supporting 32bit prefetchable ranges are to
> implement the upper32 registers as read-only registers that return zero.
> In the example above, -1 in the upper32 base simply means that base >
> limit, which disables the range.
> 
> Further investigation shows that the MEM_64 resource flag is setup for
> this range based on hardware capabilities, but then it gets removed in
> pbus_size_mem() because we want to use the range to map a 32bit option
> ROM.  This leaves us entering pci_setup_bridge() with -1 in the upper32
> base and the MEM_64 flag clear, so we never touch the upper32 base
> register.  I think this patch is still a simple, safe solution.  Thanks,

Yup - after reading the PCI-PCI spec a 3rd time. I have to agree.
Alex, sorry for the flip flopping. Pre-2.6.30 code was clearly working.
Please add:
  Reviewed-by: Grant Grundler <grundler@parisc-linux.org>

I assumed Yinghai's objection was based on a specific problem he had
seen with writing upper32 register. Bjorn asked the right question.
If there isn't a specific problem, I'd prefer AW's simpler patch.

I'm also thinking the resource allocation design which uses resource
flags to indicate resources assigned (e.g a resource is 32-bit) rather
than HW attributes is broken. We should be able to allocate 32-bit Option
ROM into a 64-bit prefetchable MMIO window that is programmed with upper32
as zeros without changing the resource type. The resource allocation
code only be looking at Resource "Type" when (re)programming
window registers. The rest of the time (programming BARs) should be
able to just test "if it fits".
 
cheers,
grant

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Yinghai Lu]

Grant Grundler wrote:
> On Mon, Nov 30, 2009 at 09:10:54PM -0700, Alex Williamson wrote:
>> On Mon, 2009-11-30 at 17:19 -0700, Grant Grundler wrote:
>>> On Mon, Nov 30, 2009 at 05:03:32PM -0700, Grant Grundler wrote:
>>>> On Mon, Nov 30, 2009 at 02:51:44PM -0700, Alex Williamson wrote:
>>>>> Prior to 1f82de10 we always initialized the upper 32bits of the
>>>>> prefetchable memory window, regardless of the address range used.
>>>>> Now we only touch it for a >32bit address, which means the upper32
>>>>> registers remain whatever the BIOS initialized them too.
>>>>>
>>>>> It's valid for the BIOS to set the upper32 base/limit to
>>>>> 0xffffffff/0x00000000, which makes us program prefetchable ranges
>>>>> like 0xffffffffabc00000 - 0x00000000abc00000
>>>>>
>>>>> Revert the chunk of 1f82de10 that made this conditional so we always
>>>>> write the upper32 registers and remove now unused pref_mem64 variable.
>>>>>
>>>>> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
>>>> Reviewed-by: Grant Grundler <grundler@parisc-linux.org>
>>> NAK this - I messed up. Yinghai is correct. Something else is going on.
>>>
>>> It might be perfectly OK to read 0xffffffffabc00000 if the bridge
>>> isn't using the upper32 Prefetchable register. Maybe the problem is
>>> some code is reading the upper32 value without checking that it's valid?
>> Apologies for not threading the v2 patch into the original thread.  The
>> prefetchable base register does support the upper32 bits and it does
>> work correctly.  However per the pci-to-pci bridge spec, a little lower
>> on page 47, devices only supporting 32bit prefetchable ranges are to
>> implement the upper32 registers as read-only registers that return zero.
>> In the example above, -1 in the upper32 base simply means that base >
>> limit, which disables the range.
>>
>> Further investigation shows that the MEM_64 resource flag is setup for
>> this range based on hardware capabilities, but then it gets removed in
>> pbus_size_mem() because we want to use the range to map a 32bit option
>> ROM.  This leaves us entering pci_setup_bridge() with -1 in the upper32
>> base and the MEM_64 flag clear, so we never touch the upper32 base
>> register.  I think this patch is still a simple, safe solution.  Thanks,
> 
> Yup - after reading the PCI-PCI spec a 3rd time. I have to agree.
> Alex, sorry for the flip flopping. Pre-2.6.30 code was clearly working.
> Please add:
>   Reviewed-by: Grant Grundler <grundler@parisc-linux.org>
> 
> I assumed Yinghai's objection was based on a specific problem he had
> seen with writing upper32 register. Bjorn asked the right question.
> If there isn't a specific problem, I'd prefer AW's simpler patch.

we just should not touch that register if the HW only support 32bit pref mmio.

> 
> I'm also thinking the resource allocation design which uses resource
> flags to indicate resources assigned (e.g a resource is 32-bit) rather
> than HW attributes is broken. We should be able to allocate 32-bit Option
> ROM into a 64-bit prefetchable MMIO window that is programmed with upper32
> as zeros without changing the resource type. The resource allocation
> code only be looking at Resource "Type" when (re)programming
> window registers. The rest of the time (programming BARs) should be
> able to just test "if it fits".

IORESOURCE_MEM_64 is the flags that the resource could be assigned to >4g range.
so it is NOT assigned resource ...

YH

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Grant Grundler]

On Tue, Dec 01, 2009 at 12:40:03PM -0800, Yinghai Lu wrote:
...
> > I assumed Yinghai's objection was based on a specific problem he had
> > seen with writing upper32 register. Bjorn asked the right question.
> > If there isn't a specific problem, I'd prefer AW's simpler patch.
> 
> we just should not touch that register if the HW only support 32bit pref mmio.

Why not?

I agree the PCI-PCI spec defines how to determine if a PCI Bridge supports
64-bit Pref MMIO (using upper32 - or not). But spec also doesn't prohibit
writing to a read-only register.  Writing this Read-Only register so far
hasn't caused any problems.

> > I'm also thinking the resource allocation design which uses resource
> > flags to indicate resources assigned (e.g a resource is 32-bit) rather
> > than HW attributes is broken. We should be able to allocate 32-bit Option
> > ROM into a 64-bit prefetchable MMIO window that is programmed with upper32
> > as zeros without changing the resource type. The resource allocation
> > code only be looking at Resource "Type" when (re)programming
> > window registers. The rest of the time (programming BARs) should be
> > able to just test "if it fits".
> 
> IORESOURCE_MEM_64 is the flags that the resource could be assigned to >4g range.
> so it is NOT assigned resource ...

*shrug* I don't have time to work on the broader issue.

thanks,
grant

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Yinghai Lu]

Grant Grundler wrote:
> On Tue, Dec 01, 2009 at 12:40:03PM -0800, Yinghai Lu wrote:
> ...
>>> I assumed Yinghai's objection was based on a specific problem he had
>>> seen with writing upper32 register. Bjorn asked the right question.
>>> If there isn't a specific problem, I'd prefer AW's simpler patch.
>> we just should not touch that register if the HW only support 32bit pref mmio.
> 
> Why not?
> 
> I agree the PCI-PCI spec defines how to determine if a PCI Bridge supports
> 64-bit Pref MMIO (using upper32 - or not). But spec also doesn't prohibit
> writing to a read-only register.  Writing this Read-Only register so far
> hasn't caused any problems.

if we can find out that is 32bit mmio pref, why waste cycle to write value to them?

YH

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Jesse Barnes]

On Mon, 30 Nov 2009 14:51:44 -0700
Alex Williamson <alex.williamson@hp.com> wrote:

> Prior to 1f82de10 we always initialized the upper 32bits of the
> prefetchable memory window, regardless of the address range used.
> Now we only touch it for a >32bit address, which means the upper32
> registers remain whatever the BIOS initialized them too.
> 
> It's valid for the BIOS to set the upper32 base/limit to
> 0xffffffff/0x00000000, which makes us program prefetchable ranges
> like 0xffffffffabc00000 - 0x00000000abc00000
> 
> Revert the chunk of 1f82de10 that made this conditional so we always
> write the upper32 registers and remove now unused pref_mem64 variable.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
> 

Applied this one, though it looks like your diff had some context that
wasn't in my tree.  Please double check that I fixed it up correctly.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH v2] PCI: Always set prefetchable base/limit upper32 registers

[Alex Williamson]

On Fri, 2009-12-04 at 16:01 -0800, Jesse Barnes wrote:
> On Mon, 30 Nov 2009 14:51:44 -0700
> Alex Williamson <alex.williamson@hp.com> wrote:
> 
> > Prior to 1f82de10 we always initialized the upper 32bits of the
> > prefetchable memory window, regardless of the address range used.
> > Now we only touch it for a >32bit address, which means the upper32
> > registers remain whatever the BIOS initialized them too.
> > 
> > It's valid for the BIOS to set the upper32 base/limit to
> > 0xffffffff/0x00000000, which makes us program prefetchable ranges
> > like 0xffffffffabc00000 - 0x00000000abc00000
> > 
> > Revert the chunk of 1f82de10 that made this conditional so we always
> > write the upper32 registers and remove now unused pref_mem64 variable.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> > ---
> > 
> 
> Applied this one, though it looks like your diff had some context that
> wasn't in my tree.  Please double check that I fixed it up correctly.

Yep, looks fine.  Thanks Jesse.

Alex