LinuxLists.cc - [PATCH] PCI: Always set prefetchable base/limit upper32 registers

[permalink] [raw]

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

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.
>
> Signed-off-by: Alex Williamson <[email protected]>
> ---
>
> drivers/pci/setup-bus.c | 8 +++-----
> 1 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index cb1a027..127d759 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -221,11 +221,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);
> }

when pref_mem64=0 it means that pref register is only 32 bit, we should not touch upper 32bits.

YH

2009-11-30 21:42:42

[permalink] [raw]

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

On Mon, Nov 30, 2009 at 02:22:53PM -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.
>
> Signed-off-by: Alex Williamson <[email protected]>
> ---
>
> drivers/pci/setup-bus.c | 8 +++-----
> 1 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index cb1a027..127d759 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -221,11 +221,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);
> - }

Why not remove all references to pref_mem64?
This was the only use AFAICT.

thanks,
grant

> + /* 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2009-11-30 21:43:09

[permalink] [raw]

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

On Mon, 2009-11-30 at 13:36 -0800, Yinghai Lu wrote:
> 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.
> >
> > Signed-off-by: Alex Williamson <[email protected]>
> > ---
> >
> > drivers/pci/setup-bus.c | 8 +++-----
> > 1 files changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index cb1a027..127d759 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -221,11 +221,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);
> > }
>
> when pref_mem64=0 it means that pref register is only 32 bit, we should not touch upper 32bits.

Did you read my patch description? In that case bu = lu = 0 and we
clear whatever state the BIOS left for those registers.

Alex

2009-11-30 21:44:05

[permalink] [raw]

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

On Mon, 2009-11-30 at 14:42 -0700, Grant Grundler wrote:
> On Mon, Nov 30, 2009 at 02:22:53PM -0700, Alex Williamson wrote:
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index cb1a027..127d759 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -221,11 +221,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);
> > - }
>
> Why not remove all references to pref_mem64?
> This was the only use AFAICT.

Ah, good point, I'll send a v2. Thanks Grant.

Alex

2009-11-30 21:52:51

[permalink] [raw]

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

Alex Williamson wrote:
> On Mon, 2009-11-30 at 13:36 -0800, Yinghai Lu wrote:
>> 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.
>>>
>>> Signed-off-by: Alex Williamson <[email protected]>
>>> ---
>>>
>>> drivers/pci/setup-bus.c | 8 +++-----
>>> 1 files changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>> index cb1a027..127d759 100644
>>> --- a/drivers/pci/setup-bus.c
>>> +++ b/drivers/pci/setup-bus.c
>>> @@ -221,11 +221,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);
>>> }
>> when pref_mem64=0 it means that pref register is only 32 bit, we should not touch upper 32bits.
>
> Did you read my patch description? In that case bu = lu = 0 and we
> clear whatever state the BIOS left for those registers.

if HW state that reg is only 32bit pref, why should we care about the upper 32bit?

looks like that your bridge device need quirk to clear that so called upper 32bit for it.

YH

2009-11-30 22:01:42

[permalink] [raw]

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

On Mon, 2009-11-30 at 13:52 -0800, Yinghai Lu wrote:
> Alex Williamson wrote:
> > On Mon, 2009-11-30 at 13:36 -0800, Yinghai Lu wrote:
> >> Alex Williamson wrote:
> >>> - 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);
> >>> }
> >> when pref_mem64=0 it means that pref register is only 32 bit, we should not touch upper 32bits.
> >
> > Did you read my patch description? In that case bu = lu = 0 and we
> > clear whatever state the BIOS left for those registers.
>
> if HW state that reg is only 32bit pref, why should we care about the upper 32bit?

In my case, we're programming a 32bit prefetchable range into a 64bit
capable bridge. We can't just assume the upper 32bits are already zero.

> looks like that your bridge device need quirk to clear that so called upper 32bit for it.

I don't believe the PCI spec dictates whether the upper 32bit base
should be 0 or -1, so it's purely a BIOS initialization choice and Linux
should properly handle both. If the hardware only supports 32bit
prefetchable windows, the hardware will drop the write, just as it did
for every 2.6 kernel before 1f82de10. Thanks,

Alex

2009-11-30 22:13:11

[permalink] [raw]

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

Alex Williamson wrote:
> On Mon, 2009-11-30 at 13:52 -0800, Yinghai Lu wrote:
>> Alex Williamson wrote:
>>> On Mon, 2009-11-30 at 13:36 -0800, Yinghai Lu wrote:
>>>> Alex Williamson wrote:
>>>>> - 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);
>>>>> }
>>>> when pref_mem64=0 it means that pref register is only 32 bit, we should not touch upper 32bits.
>>> Did you read my patch description? In that case bu = lu = 0 and we
>>> clear whatever state the BIOS left for those registers.
>> if HW state that reg is only 32bit pref, why should we care about the upper 32bit?
>
> In my case, we're programming a 32bit prefetchable range into a 64bit
> capable bridge. We can't just assume the upper 32bits are already zero.
>
>> looks like that your bridge device need quirk to clear that so called upper 32bit for it.
>
> I don't believe the PCI spec dictates whether the upper 32bit base
> should be 0 or -1, so it's purely a BIOS initialization choice and Linux
> should properly handle both. If the hardware only supports 32bit
> prefetchable windows, the hardware will drop the write, just as it did
> for every 2.6 kernel before 1f82de10. Thanks,

current code:

#define PCI_PREF_RANGE_TYPE_MASK 0x0fUL
#define PCI_PREF_RANGE_TYPE_32 0x00
#define PCI_PREF_RANGE_TYPE_64 0x01
#define PCI_PREF_RANGE_MASK (~0x0fUL)

if the HW state the pref mmio is 64bit, we will touch upper 32bit. otherwise we will not touch it.

please post your boot log to show the problem.

YH

2009-11-30 22:19:44

[permalink] [raw]

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

On Mon, 2009-11-30 at 14:12 -0800, Yinghai Lu wrote:
> Alex Williamson wrote:
> > I don't believe the PCI spec dictates whether the upper 32bit base
> > should be 0 or -1, so it's purely a BIOS initialization choice and Linux
> > should properly handle both. If the hardware only supports 32bit
> > prefetchable windows, the hardware will drop the write, just as it did
> > for every 2.6 kernel before 1f82de10. Thanks,
>
> current code:
>
> #define PCI_PREF_RANGE_TYPE_MASK 0x0fUL
> #define PCI_PREF_RANGE_TYPE_32 0x00
> #define PCI_PREF_RANGE_TYPE_64 0x01
> #define PCI_PREF_RANGE_MASK (~0x0fUL)
>
> if the HW state the pref mmio is 64bit, we will touch upper 32bit. otherwise we will not touch it.

Really, where? Please paste the code that writes to
PCI_PREF_BASE_UPPER32 in the case of hardware supporting a 64bit
prefetchable window. I only see this happening if we are assigning it
to an IORESOURCE_MEM_64 resources.

Alex

2009-11-30 23:33:04

[permalink] [raw]

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

Alex Williamson wrote:
> On Mon, 2009-11-30 at 14:12 -0800, Yinghai Lu wrote:
>> Alex Williamson wrote:
>>> I don't believe the PCI spec dictates whether the upper 32bit base
>>> should be 0 or -1, so it's purely a BIOS initialization choice and Linux
>>> should properly handle both. If the hardware only supports 32bit
>>> prefetchable windows, the hardware will drop the write, just as it did
>>> for every 2.6 kernel before 1f82de10. Thanks,
>> current code:
>>
>> #define PCI_PREF_RANGE_TYPE_MASK 0x0fUL
>> #define PCI_PREF_RANGE_TYPE_32 0x00
>> #define PCI_PREF_RANGE_TYPE_64 0x01
>> #define PCI_PREF_RANGE_MASK (~0x0fUL)
>>
>> if the HW state the pref mmio is 64bit, we will touch upper 32bit. otherwise we will not touch it.
>
> Really, where? Please paste the code that writes to
> PCI_PREF_BASE_UPPER32 in the case of hardware supporting a 64bit
> prefetchable window. I only see this happening if we are assigning it
> to an IORESOURCE_MEM_64 resources.

IORESOURCE_MEM_64 get set when PCI_PREF_RANGE_TYPE_64 is set.

in probe.c::pci_read_bridge_bases()
and
setup-bus.c::pci_bridge_check_ranges()

YH

2009-11-30 23:53:53

[permalink] [raw]

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

On Mon, 2009-11-30 at 15:32 -0800, Yinghai Lu wrote:
> Alex Williamson wrote:
> > On Mon, 2009-11-30 at 14:12 -0800, Yinghai Lu wrote:
> >> Alex Williamson wrote:
> >>> I don't believe the PCI spec dictates whether the upper 32bit base
> >>> should be 0 or -1, so it's purely a BIOS initialization choice and Linux
> >>> should properly handle both. If the hardware only supports 32bit
> >>> prefetchable windows, the hardware will drop the write, just as it did
> >>> for every 2.6 kernel before 1f82de10. Thanks,
> >> current code:
> >>
> >> #define PCI_PREF_RANGE_TYPE_MASK 0x0fUL
> >> #define PCI_PREF_RANGE_TYPE_32 0x00
> >> #define PCI_PREF_RANGE_TYPE_64 0x01
> >> #define PCI_PREF_RANGE_MASK (~0x0fUL)
> >>
> >> if the HW state the pref mmio is 64bit, we will touch upper 32bit. otherwise we will not touch it.
> >
> > Really, where? Please paste the code that writes to
> > PCI_PREF_BASE_UPPER32 in the case of hardware supporting a 64bit
> > prefetchable window. I only see this happening if we are assigning it
> > to an IORESOURCE_MEM_64 resources.
>
> IORESOURCE_MEM_64 get set when PCI_PREF_RANGE_TYPE_64 is set.
>
> in probe.c::pci_read_bridge_bases()

Ah, I think I see where you're going. We only set IORESOURCE_MEM_64 if
base <= limit, ie. the BIOS has programmed the prefetchable range. This
is not a requirement by the PCI spec. In my case the BIOS has left base
> limit, just as Linux would do if it disabled the range, so we never
set this flag.

> setup-bus.c::pci_bridge_check_ranges()

This is only checking that the upper 32bits is actually implemented,
should we have already set the IORESOURCE_MEM_64 from the function
above, which we haven't.

So, in my case I have a 64bit capable prefetchable range, that the BIOS
has not programmed and is not required to program. We assign it to a
32bit window, and never touch the UPPER32 registers.

Alex

2009-11-30 23:58:37

[permalink] [raw]

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

On Mon, Nov 30, 2009 at 01:52:03PM -0800, Yinghai Lu wrote:
> if HW state that reg is only 32bit pref,
> why should we care about the upper 32bit?

pref_mem64 describes the resource, not the HW.

All the PCI specs define Type 1 configuration space with Prefetchable Memory
register as 64-bit. Even if the Type 1 (PCI Bridge) HW ignores this register.
See "7.5.3. Type 1 Configuration Space Header" of PCI EXPRESS BASE
SPECIFICATION, REV. 1.1.

> looks like that your bridge device need quirk to clear that so called
> upper 32bit for it.

Hrm...I don't think this is a quirk. PCI spec defines 64-bit PrefMem
window register regardless of what type of resource we program into it.

hth,
grant

2009-12-01 00:01:18

[permalink] [raw]

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

Alex Williamson wrote:
> On Mon, 2009-11-30 at 15:32 -0800, Yinghai Lu wrote:
>> Alex Williamson wrote:
>>> On Mon, 2009-11-30 at 14:12 -0800, Yinghai Lu wrote:
>>>> Alex Williamson wrote:
>>>>> I don't believe the PCI spec dictates whether the upper 32bit base
>>>>> should be 0 or -1, so it's purely a BIOS initialization choice and Linux
>>>>> should properly handle both. If the hardware only supports 32bit
>>>>> prefetchable windows, the hardware will drop the write, just as it did
>>>>> for every 2.6 kernel before 1f82de10. Thanks,
>>>> current code:
>>>>
>>>> #define PCI_PREF_RANGE_TYPE_MASK 0x0fUL
>>>> #define PCI_PREF_RANGE_TYPE_32 0x00
>>>> #define PCI_PREF_RANGE_TYPE_64 0x01
>>>> #define PCI_PREF_RANGE_MASK (~0x0fUL)
>>>>
>>>> if the HW state the pref mmio is 64bit, we will touch upper 32bit. otherwise we will not touch it.
>>> Really, where? Please paste the code that writes to
>>> PCI_PREF_BASE_UPPER32 in the case of hardware supporting a 64bit
>>> prefetchable window. I only see this happening if we are assigning it
>>> to an IORESOURCE_MEM_64 resources.
>> IORESOURCE_MEM_64 get set when PCI_PREF_RANGE_TYPE_64 is set.
>>
>> in probe.c::pci_read_bridge_bases()
>
> Ah, I think I see where you're going. We only set IORESOURCE_MEM_64 if
> base <= limit, ie. the BIOS has programmed the prefetchable range. This
> is not a requirement by the PCI spec. In my case the BIOS has left base
>> limit, just as Linux would do if it disabled the range, so we never
> set this flag.
>
>> setup-bus.c::pci_bridge_check_ranges()
>
> This is only checking that the upper 32bits is actually implemented,
> should we have already set the IORESOURCE_MEM_64 from the function
> above, which we haven't.
>
> So, in my case I have a 64bit capable prefetchable range, that the BIOS
> has not programmed and is not required to program. We assign it to a
> 32bit window, and never touch the UPPER32 registers.

no.

before assign range to that resource.
pci_bridge_check_ranges is called, it will check those two bit to make sure that is set correcly

if (pmem) {
b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64)
b_res[2].flags |= IORESOURCE_MEM_64;
}

/* double check if bridge does support 64 bit pref */
if (b_res[2].flags & IORESOURCE_MEM_64) {
u32 mem_base_hi, tmp;
pci_read_config_dword(bridge, PCI_PREF_BASE_UPPER32,
&mem_base_hi);
pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
0xffffffff);
pci_read_config_dword(bridge, PCI_PREF_BASE_UPPER32, &tmp);
if (!tmp)
b_res[2].flags &= ~IORESOURCE_MEM_64;
pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
mem_base_hi);
}

so even with 32 bit window in 64bit BAR, that upper32 bit still get cleared.

YH

2009-12-01 00:00:47

[permalink] [raw]

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

On Mon, Nov 30, 2009 at 03:32:13PM -0800, Yinghai Lu wrote:
> IORESOURCE_MEM_64 get set when PCI_PREF_RANGE_TYPE_64 is set.

PCI_PREF_RANGE_TYPE_64 is when we read BARs. It doesn't indicate
anything about PCI Bridge Window registers AFAIK.

hth,
grant

2009-12-01 00:10:10

[permalink] [raw]

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

Grant Grundler wrote:
> On Mon, Nov 30, 2009 at 03:32:13PM -0800, Yinghai Lu wrote:
>> IORESOURCE_MEM_64 get set when PCI_PREF_RANGE_TYPE_64 is set.
>
> PCI_PREF_RANGE_TYPE_64 is when we read BARs. It doesn't indicate
> anything about PCI Bridge Window registers AFAIK.
>

please double check pci-to-pci bridge 1.2, in page 47.

YH

2009-12-01 00:15:43

[permalink] [raw]

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

On Mon, Nov 30, 2009 at 04:09:19PM -0800, Yinghai Lu wrote:
> Grant Grundler wrote:
> > On Mon, Nov 30, 2009 at 03:32:13PM -0800, Yinghai Lu wrote:
> >> IORESOURCE_MEM_64 get set when PCI_PREF_RANGE_TYPE_64 is set.
> >
> > PCI_PREF_RANGE_TYPE_64 is when we read BARs. It doesn't indicate
> > anything about PCI Bridge Window registers AFAIK.
> >
>
> please double check pci-to-pci bridge 1.2, in page 47.

My apologies. You are correct:
The bottom 4 bits of both the Prefetchable Memory Base and
Prefetchable Memory Limit registers are read-only, contain
the same value, and encode whether or not the bridge supports
64-bit addresses. If these four bits have the value 0h, then
the bridge supports only 32 bit addresses. If these four bits
have the value 01h, then the bridge supports 64-bit addresses
and the Prefetchable Base Upper 32 Bits and Prefetchable Limit
Upper 32 Bits registers hold the rest of the 64-bit prefetchable
base and limit addresses respectively.

thanks,
grant

2009-12-01 00:23:24

[permalink] [raw]

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

2009-12-01 01:56:04

[permalink] [raw]

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

On Mon, 2009-11-30 at 16:00 -0800, Yinghai Lu wrote:
> Alex Williamson wrote:
>
> > Ah, I think I see where you're going. We only set IORESOURCE_MEM_64 if
> > base <= limit, ie. the BIOS has programmed the prefetchable range. This
> > is not a requirement by the PCI spec. In my case the BIOS has left base
> >> limit, just as Linux would do if it disabled the range, so we never
> > set this flag.
> >
> >> setup-bus.c::pci_bridge_check_ranges()
> >
> > This is only checking that the upper 32bits is actually implemented,
> > should we have already set the IORESOURCE_MEM_64 from the function
> > above, which we haven't.
> >
> > So, in my case I have a 64bit capable prefetchable range, that the BIOS
> > has not programmed and is not required to program. We assign it to a
> > 32bit window, and never touch the UPPER32 registers.
>
> no.
>
> before assign range to that resource.
> pci_bridge_check_ranges is called, it will check those two bit to make sure that is set correcly
>
> if (pmem) {
> b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
> if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64)
> b_res[2].flags |= IORESOURCE_MEM_64;
> }

Ok, sorry I missed this. Yes, this is getting called, but when we get
back to pci_setup_bridge() that flag is missing IORESOURCE_MEM_64.
Perhaps these are different resources? I'm still tracing the code to
find out what happened to that flag.

Also, I'm running 64bit(x86_64), and if lspci is wrong, then so is
setpci. I don't think there's an "ignore upper32" anywhere, so the
result of 0xffffffffabc00000 - 0x00000000abc00000 is that base > limit
thus the range is disabled at the bridge and the ROM resource we
assigned into the window behind the bridge is inaccessible.

Alex

2009-12-01 02:27:31

[permalink] [raw]

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

Alex Williamson wrote:
> On Mon, 2009-11-30 at 16:00 -0800, Yinghai Lu wrote:
>> Alex Williamson wrote:
>>
>>> Ah, I think I see where you're going. We only set IORESOURCE_MEM_64 if
>>> base <= limit, ie. the BIOS has programmed the prefetchable range. This
>>> is not a requirement by the PCI spec. In my case the BIOS has left base
>>>> limit, just as Linux would do if it disabled the range, so we never
>>> set this flag.
>>>
>>>> setup-bus.c::pci_bridge_check_ranges()
>>> This is only checking that the upper 32bits is actually implemented,
>>> should we have already set the IORESOURCE_MEM_64 from the function
>>> above, which we haven't.
>>>
>>> So, in my case I have a 64bit capable prefetchable range, that the BIOS
>>> has not programmed and is not required to program. We assign it to a
>>> 32bit window, and never touch the UPPER32 registers.
>> no.
>>
>> before assign range to that resource.
>> pci_bridge_check_ranges is called, it will check those two bit to make sure that is set correcly
>>
>> if (pmem) {
>> b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
>> if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64)
>> b_res[2].flags |= IORESOURCE_MEM_64;
>> }
>
> Ok, sorry I missed this. Yes, this is getting called, but when we get
> back to pci_setup_bridge() that flag is missing IORESOURCE_MEM_64.
> Perhaps these are different resources? I'm still tracing the code to
> find out what happened to that flag.
>
> Also, I'm running 64bit(x86_64), and if lspci is wrong, then so is
> setpci. I don't think there's an "ignore upper32" anywhere, so the
> result of 0xffffffffabc00000 - 0x00000000abc00000 is that base > limit
> thus the range is disabled at the bridge and the ROM resource we
> assigned into the window behind the bridge is inaccessible.

can you check

---
drivers/pci/setup-bus.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -397,10 +397,17 @@ static void pci_bridge_check_ranges(stru
pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
0xffffffff);
pci_read_config_dword(bridge, PCI_PREF_BASE_UPPER32, &tmp);
- if (!tmp)
+ if (!tmp) {
b_res[2].flags &= ~IORESOURCE_MEM_64;
- pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
- mem_base_hi);
+ dev_info(&bridge->dev, "%pR MEM_64 clearred\n", &b_res[2]);
+ /* not sure if we can clear it */
+ pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
+ 0);
+ pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32,
+ 0);
+ } else
+ pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
+ mem_base_hi);
}
}

2009-12-01 02:51:37

[permalink] [raw]

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

Yinghai Lu wrote:
> Alex Williamson wrote:
>> On Mon, 2009-11-30 at 16:00 -0800, Yinghai Lu wrote:
>>> Alex Williamson wrote:
>>>
>>>> Ah, I think I see where you're going. We only set IORESOURCE_MEM_64 if
>>>> base <= limit, ie. the BIOS has programmed the prefetchable range. This
>>>> is not a requirement by the PCI spec. In my case the BIOS has left base
>>>>> limit, just as Linux would do if it disabled the range, so we never
>>>> set this flag.
>>>>
>>>>> setup-bus.c::pci_bridge_check_ranges()
>>>> This is only checking that the upper 32bits is actually implemented,
>>>> should we have already set the IORESOURCE_MEM_64 from the function
>>>> above, which we haven't.
>>>>
>>>> So, in my case I have a 64bit capable prefetchable range, that the BIOS
>>>> has not programmed and is not required to program. We assign it to a
>>>> 32bit window, and never touch the UPPER32 registers.
>>> no.
>>>
>>> before assign range to that resource.
>>> pci_bridge_check_ranges is called, it will check those two bit to make sure that is set correcly
>>>
>>> if (pmem) {
>>> b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
>>> if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64)
>>> b_res[2].flags |= IORESOURCE_MEM_64;
>>> }
>> Ok, sorry I missed this. Yes, this is getting called, but when we get
>> back to pci_setup_bridge() that flag is missing IORESOURCE_MEM_64.
>> Perhaps these are different resources? I'm still tracing the code to
>> find out what happened to that flag.
>>
>> Also, I'm running 64bit(x86_64), and if lspci is wrong, then so is
>> setpci. I don't think there's an "ignore upper32" anywhere, so the
>> result of 0xffffffffabc00000 - 0x00000000abc00000 is that base > limit
>> thus the range is disabled at the bridge and the ROM resource we
>> assigned into the window behind the bridge is inaccessible.
>
> can you check
>
> ---
> drivers/pci/setup-bus.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> Index: linux-2.6/drivers/pci/setup-bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-bus.c
> +++ linux-2.6/drivers/pci/setup-bus.c
> @@ -397,10 +397,17 @@ static void pci_bridge_check_ranges(stru
> pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
> 0xffffffff);
> pci_read_config_dword(bridge, PCI_PREF_BASE_UPPER32, &tmp);
> - if (!tmp)
> + if (!tmp) {
> b_res[2].flags &= ~IORESOURCE_MEM_64;
> - pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
> - mem_base_hi);
> + dev_info(&bridge->dev, "%pR MEM_64 clearred\n", &b_res[2]);
> + /* not sure if we can clear it */
> + pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
> + 0);
> + pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32,
> + 0);
> + } else
> + pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
> + mem_base_hi);
> }
> }
>
>

or

---
drivers/pci/setup-bus.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -289,7 +289,6 @@ static void pci_setup_bridge_mmio_pref(s
struct resource *res;
struct pci_bus_region region;
u32 l, bu, lu;
- int pref_mem64;

/* Clear out the upper 32 bits of PREF limit.
If PCI_PREF_BASE_UPPER32 was non-zero, this temporarily
@@ -297,7 +296,6 @@ static void pci_setup_bridge_mmio_pref(s
pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, 0);

/* Set up PREF base/limit. */
- pref_mem64 = 0;
bu = lu = 0;
res = bus->resource[2];
pcibios_resource_to_bus(bridge, &region, res);
@@ -305,7 +303,6 @@ static void pci_setup_bridge_mmio_pref(s
l = (region.start >> 16) & 0xfff0;
l |= region.end & 0xfff00000;
if (res->flags & IORESOURCE_MEM_64) {
- pref_mem64 = 1;
bu = upper_32_bits(region.start);
lu = upper_32_bits(region.end);
}
@@ -317,7 +314,7 @@ static void pci_setup_bridge_mmio_pref(s
}
pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, l);

- if (pref_mem64) {
+ if (res->flags & PCI_PREF_RANGE_TYPE_64) {
/* 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);
@@ -385,8 +382,10 @@ static void pci_bridge_check_ranges(stru
}
if (pmem) {
b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
- if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64)
+ if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64) {
b_res[2].flags |= IORESOURCE_MEM_64;
+ b_res[2].flags |= PCI_PREF_RANGE_TYPE_64;
+ }
}

/* double check if bridge does support 64 bit pref */
@@ -397,8 +396,10 @@ static void pci_bridge_check_ranges(stru
pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
0xffffffff);
pci_read_config_dword(bridge, PCI_PREF_BASE_UPPER32, &tmp);
- if (!tmp)
+ if (!tmp) {
b_res[2].flags &= ~IORESOURCE_MEM_64;
+ dev_info(&bridge->dev, "%pR MEM_64 cleared\n", &b_res[2]);
+ }
pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
mem_base_hi);
}

2009-12-01 03:23:23

[permalink] [raw]

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

On Mon, 2009-11-30 at 18:50 -0800, Yinghai Lu wrote:
> Yinghai Lu wrote:
> > Alex Williamson wrote:
> >> Ok, sorry I missed this. Yes, this is getting called, but when we get
> >> back to pci_setup_bridge() that flag is missing IORESOURCE_MEM_64.
> >> Perhaps these are different resources? I'm still tracing the code to
> >> find out what happened to that flag.
> >>
> >> Also, I'm running 64bit(x86_64), and if lspci is wrong, then so is
> >> setpci. I don't think there's an "ignore upper32" anywhere, so the
> >> result of 0xffffffffabc00000 - 0x00000000abc00000 is that base > limit
> >> thus the range is disabled at the bridge and the ROM resource we
> >> assigned into the window behind the bridge is inaccessible.
> >
> > can you check
[snip]

The upper32 base register works as advertised, that's not where we clear
the MEM_64 flag. I tracked that down to pbus_size_mem(). So, we have a
MEM_64 capable prefetchable base, but we want to use it to map a 32bit
resource behind the bridge (a ROM in this case), so we drop the MEM_64
flag, causing us to hit pci_setup_bridge() with the flag clear and thus
not touching UPPER32. I think your second patch would also solve this
since it separates the desired resource size from the register size.
However, it seems much more simple to unconditionally write the upper32
registers as was done for all 2.6 kernels up to 2.6.30. Thanks,

Alex

2009-12-01 06:36:19

[permalink] [raw]

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

Alex Williamson wrote:
> On Mon, 2009-11-30 at 18:50 -0800, Yinghai Lu wrote:
>> Yinghai Lu wrote:
>>> Alex Williamson wrote:
>>>> Ok, sorry I missed this. Yes, this is getting called, but when we get
>>>> back to pci_setup_bridge() that flag is missing IORESOURCE_MEM_64.
>>>> Perhaps these are different resources? I'm still tracing the code to
>>>> find out what happened to that flag.
>>>>
>>>> Also, I'm running 64bit(x86_64), and if lspci is wrong, then so is
>>>> setpci. I don't think there's an "ignore upper32" anywhere, so the
>>>> result of 0xffffffffabc00000 - 0x00000000abc00000 is that base > limit
>>>> thus the range is disabled at the bridge and the ROM resource we
>>>> assigned into the window behind the bridge is inaccessible.
>>> can you check
> [snip]
>
> The upper32 base register works as advertised, that's not where we clear
> the MEM_64 flag. I tracked that down to pbus_size_mem(). So, we have a
> MEM_64 capable prefetchable base, but we want to use it to map a 32bit
> resource behind the bridge (a ROM in this case), so we drop the MEM_64
> flag, causing us to hit pci_setup_bridge() with the flag clear and thus
> not touching UPPER32. I think your second patch would also solve this
> since it separates the desired resource size from the register size.
> However, it seems much more simple to unconditionally write the upper32
> registers as was done for all 2.6 kernels up to 2.6.30. Thanks,

if the bridge self does not support 64bit pref mmio, we should not touch
upper32 reg.

will update the my second patch with your analysis

YH

2009-12-01 06:55:53

[permalink] [raw]

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

On Mon, 2009-11-30 at 22:35 -0800, Yinghai Lu wrote:
> Alex Williamson wrote:
> >
> > The upper32 base register works as advertised, that's not where we clear
> > the MEM_64 flag. I tracked that down to pbus_size_mem(). So, we have a
> > MEM_64 capable prefetchable base, but we want to use it to map a 32bit
> > resource behind the bridge (a ROM in this case), so we drop the MEM_64
> > flag, causing us to hit pci_setup_bridge() with the flag clear and thus
> > not touching UPPER32. I think your second patch would also solve this
> > since it separates the desired resource size from the register size.
> > However, it seems much more simple to unconditionally write the upper32
> > registers as was done for all 2.6 kernels up to 2.6.30. Thanks,
>
> if the bridge self does not support 64bit pref mmio, we should not touch
> upper32 reg.

Does touching it actually cause any problems? The spec states:

If the Prefetchable Memory Base and Prefetchable Memory Limit
registers indicate support for 32-bit addressing, then the
Prefetchable Base Upper 32 Bits and Prefetchable Limit Upper 32
Bits registers are both implemented as read-only registers that
return zero when read.

So even if the bridge only supports 32bit prefetchable, the upper32
registers are still present and writes will be dropped. This is the way
the code worked for a long, long time. I'm wondering why we need to
make it more complicated.

Alex

2009-12-01 07:04:58

[permalink] [raw]

Subject: [PATCH] pci: fix bridge 64bit flag setting

Alex found one system that one pci bridge pref mmio 64 is not set correctly.
aka, the upper32 base/limit is not cleaned.
he found that bridge is supporting 64 bit pref mmio, but device under that
does not support that. so that IORESOURCE_MEM_64 get cleared in pbus_size_mem()

the fix will be:
make pci_bridge_check_ranges() to store the PCI_PREF_RANGE_TYPE_64 in addition
to IORESOURCE_MEM_64. just like pci_read_bridge_bases()
and later will use that bit in pci_setup_bridge_mmio_pref instead of
IORESOURCE_MEM_64.
also avoid touching upper32 regs if the bridge does not support 64bit pref.

Reported-by: Alex Williamson <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>

---
drivers/pci/setup-bus.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -289,7 +289,6 @@ static void pci_setup_bridge_mmio_pref(s
struct resource *res;
struct pci_bus_region region;
u32 l, bu, lu;
- int pref_mem64;

/* Clear out the upper 32 bits of PREF limit.
If PCI_PREF_BASE_UPPER32 was non-zero, this temporarily
@@ -297,7 +296,6 @@ static void pci_setup_bridge_mmio_pref(s
pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, 0);

/* Set up PREF base/limit. */
- pref_mem64 = 0;
bu = lu = 0;
res = bus->resource[2];
pcibios_resource_to_bus(bridge, &region, res);
@@ -305,7 +303,6 @@ static void pci_setup_bridge_mmio_pref(s
l = (region.start >> 16) & 0xfff0;
l |= region.end & 0xfff00000;
if (res->flags & IORESOURCE_MEM_64) {
- pref_mem64 = 1;
bu = upper_32_bits(region.start);
lu = upper_32_bits(region.end);
}
@@ -317,7 +314,7 @@ static void pci_setup_bridge_mmio_pref(s
}
pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, l);

- if (pref_mem64) {
+ if (res->flags & PCI_PREF_RANGE_TYPE_64) {
/* 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);
@@ -385,8 +382,10 @@ static void pci_bridge_check_ranges(stru
}
if (pmem) {
b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
- if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64)
+ if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64) {
b_res[2].flags |= IORESOURCE_MEM_64;
+ b_res[2].flags |= PCI_PREF_RANGE_TYPE_64;
+ }
}

/* double check if bridge does support 64 bit pref */

2009-12-01 15:38:54

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH] pci: fix bridge 64bit flag setting

On Tuesday 01 December 2009 12:03:57 am Yinghai Lu wrote:
>
> Alex found one system that one pci bridge pref mmio 64 is not set correctly.
> aka, the upper32 base/limit is not cleaned.
> he found that bridge is supporting 64 bit pref mmio, but device under that
> does not support that. so that IORESOURCE_MEM_64 get cleared in pbus_size_mem()

I think it's wrong that pbus_size_mem() fiddles with IORESOURCE_MEM_64
in bus resources based on where BARs of devices on that bus live. That
feels fragile.

The question of whether the bridge supports 64-bit apertures is
strictly a hardware property of the bridge. It has nothing to do
with where we place downstream devices.

Is there really a problem with writing to PCI_PREF_BASE_UPPER32
unconditionally? As Alex pointed out, per 3.2.5.10 of the bridge
spec, the UPPER32 registers are read-only if only 32-bit apertures
are supported. If you mentioned a problem with doing this
unconditionally, I missed it.

The only place we test IORESOURCE_MEM_64 for a bus resource is when
we're programming PCI_PREF_BASE_UPPER32. If we think it's important
to program it conditionally, why don't we skip IORESOURCE_MEM_64
altogether, and just look at the bits in PCI_PREF_MEMORY_BASE directly?
E.g., something like this:

pci_read_config_dword(bridge, PCI_PREF_MEMORY_BASE, &l);
if ((l & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64) {
pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
}

Then we don't have to maintain flags at all, and it's easy to verify
that the code corresponds to the spec.

Bjorn

> the fix will be:
> make pci_bridge_check_ranges() to store the PCI_PREF_RANGE_TYPE_64 in addition
> to IORESOURCE_MEM_64. just like pci_read_bridge_bases()
> and later will use that bit in pci_setup_bridge_mmio_pref instead of
> IORESOURCE_MEM_64.
> also avoid touching upper32 regs if the bridge does not support 64bit pref.
>
> Reported-by: Alex Williamson <[email protected]>
> Signed-off-by: Yinghai Lu <[email protected]>
>
> ---
> drivers/pci/setup-bus.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> Index: linux-2.6/drivers/pci/setup-bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-bus.c
> +++ linux-2.6/drivers/pci/setup-bus.c
> @@ -289,7 +289,6 @@ static void pci_setup_bridge_mmio_pref(s
> struct resource *res;
> struct pci_bus_region region;
> u32 l, bu, lu;
> - int pref_mem64;
>
> /* Clear out the upper 32 bits of PREF limit.
> If PCI_PREF_BASE_UPPER32 was non-zero, this temporarily
> @@ -297,7 +296,6 @@ static void pci_setup_bridge_mmio_pref(s
> pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, 0);
>
> /* Set up PREF base/limit. */
> - pref_mem64 = 0;
> bu = lu = 0;
> res = bus->resource[2];
> pcibios_resource_to_bus(bridge, &region, res);
> @@ -305,7 +303,6 @@ static void pci_setup_bridge_mmio_pref(s
> l = (region.start >> 16) & 0xfff0;
> l |= region.end & 0xfff00000;
> if (res->flags & IORESOURCE_MEM_64) {
> - pref_mem64 = 1;
> bu = upper_32_bits(region.start);
> lu = upper_32_bits(region.end);
> }
> @@ -317,7 +314,7 @@ static void pci_setup_bridge_mmio_pref(s
> }
> pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, l);
>
> - if (pref_mem64) {
> + if (res->flags & PCI_PREF_RANGE_TYPE_64) {
> /* 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);
> @@ -385,8 +382,10 @@ static void pci_bridge_check_ranges(stru
> }
> if (pmem) {
> b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
> - if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64)
> + if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64) {
> b_res[2].flags |= IORESOURCE_MEM_64;
> + b_res[2].flags |= PCI_PREF_RANGE_TYPE_64;
> + }
> }
>
> /* double check if bridge does support 64 bit pref */
>

2009-12-01 18:29:28

[permalink] [raw]

Subject: Re: [PATCH] pci: fix bridge 64bit flag setting

Bjorn Helgaas wrote:
> On Tuesday 01 December 2009 12:03:57 am Yinghai Lu wrote:
>> Alex found one system that one pci bridge pref mmio 64 is not set correctly.
>> aka, the upper32 base/limit is not cleaned.
>> he found that bridge is supporting 64 bit pref mmio, but device under that
>> does not support that. so that IORESOURCE_MEM_64 get cleared in pbus_size_mem()
>
> I think it's wrong that pbus_size_mem() fiddles with IORESOURCE_MEM_64
> in bus resources based on where BARs of devices on that bus live. That
> feels fragile.

yes. need more clean up.

>
> The question of whether the bridge supports 64-bit apertures is
> strictly a hardware property of the bridge. It has nothing to do
> with where we place downstream devices.
>
> Is there really a problem with writing to PCI_PREF_BASE_UPPER32
> unconditionally? As Alex pointed out, per 3.2.5.10 of the bridge
> spec, the UPPER32 registers are read-only if only 32-bit apertures
> are supported. If you mentioned a problem with doing this
> unconditionally, I missed it.

remember that some x2apic registers say they are reserved, and read them could cause GP error.

and for pci devices, if it is read-only, for good design, then write it should be ok.
but if there is some design problem in devices, and they could say those are read-only.
why kernel write value to it?

>
> The only place we test IORESOURCE_MEM_64 for a bus resource is when
> we're programming PCI_PREF_BASE_UPPER32. If we think it's important
> to program it conditionally, why don't we skip IORESOURCE_MEM_64

IORESOURCE_MEM_64 is used to control if we can use MMIO > 4g.

> altogether, and just look at the bits in PCI_PREF_MEMORY_BASE directly?
> E.g., something like this:
>
> pci_read_config_dword(bridge, PCI_PREF_MEMORY_BASE, &l);
> if ((l & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64) {
> pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
> pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
> }
>
> Then we don't have to maintain flags at all, and it's easy to verify
> that the code corresponds to the spec.

that will have several extra read, and also we already store that bit in pci_read_bridge_bases()

but forget to set that in pci_check_bus_range()

YH

YH

2009-12-01 19:16:27