2006-09-26 09:28:06

by OGAWA Hirofumi

[permalink] [raw]
Subject: [PATCH] arch/i386/pci/mmconfig.c tlb flush fix

Hi,

We use the fixmap for accessing pci config space in pci_mmcfg_read/write().
The problem is in pci_exp_set_dev_base(). It is caching a last
accessed address to avoid calling set_fixmap_nocache() whenever
pci_mmcfg_read/write() is used.

static inline void pci_exp_set_dev_base(int bus, int devfn)
{
u32 dev_base = base | (bus << 20) | (devfn << 12);
if (dev_base != mmcfg_last_accessed_device) {
mmcfg_last_accessed_device = dev_base;
set_fixmap_nocache(FIX_PCIE_MCFG, dev_base);
}
}

cpu0 cpu1
---------------------------------------------------------------------------
pci_mmcfg_read("device-A")
pci_exp_set_dev_base()
set_fixmap_nocache()
pci_mmcfg_read("device-B")
pci_exp_set_dev_base()
set_fixmap_nocache()
pci_mmcfg_read("device-B")
pci_exp_set_dev_base()
/* doesn't flush tlb */

But if cpus accessed the above order, the second pci_mmcfg_read() on
cpu0 doesn't flush the TLB, because "mmcfg_last_accessed_device" is
device-B. So, second pci_mmcfg_read() on cpu0 accesses a device-A via
a previous TLB cache.

This patches fixes this situation by adds "mmcfg_last_accessed_cpu" check.

Please apply.


Signed-off-by: OGAWA Hirofumi <[email protected]>
---

arch/i386/pci/mmconfig.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff -puN arch/i386/pci/mmconfig.c~mmconfig-tlb-race-fix arch/i386/pci/mmconfig.c
--- linux-2.6/arch/i386/pci/mmconfig.c~mmconfig-tlb-race-fix 2006-09-26 11:35:54.000000000 +0900
+++ linux-2.6-hirofumi/arch/i386/pci/mmconfig.c 2006-09-26 11:36:54.000000000 +0900
@@ -26,6 +26,7 @@

/* The base address of the last MMCONFIG device accessed */
static u32 mmcfg_last_accessed_device;
+static int mmcfg_last_accessed_cpu;

static DECLARE_BITMAP(fallback_slots, MAX_CHECK_BUS*32);

@@ -70,8 +71,11 @@ static u32 get_base_addr(unsigned int se
static inline void pci_exp_set_dev_base(unsigned int base, int bus, int devfn)
{
u32 dev_base = base | (bus << 20) | (devfn << 12);
- if (dev_base != mmcfg_last_accessed_device) {
+ int cpu = smp_processor_id();
+ if (dev_base != mmcfg_last_accessed_device ||
+ cpu != mmcfg_last_accessed_cpu) {
mmcfg_last_accessed_device = dev_base;
+ mmcfg_last_accessed_cpu = cpu;
set_fixmap_nocache(FIX_PCIE_MCFG, dev_base);
}
}
_

--
OGAWA Hirofumi <[email protected]>


2006-09-26 12:22:46

by Andi Kleen

[permalink] [raw]
Subject: Re: [PATCH] arch/i386/pci/mmconfig.c tlb flush fix

OGAWA Hirofumi <[email protected]> writes:

> Hi,
>
> We use the fixmap for accessing pci config space in pci_mmcfg_read/write().
> The problem is in pci_exp_set_dev_base(). It is caching a last
> accessed address to avoid calling set_fixmap_nocache() whenever
> pci_mmcfg_read/write() is used.


Good catch. I already had another report of mmconfig corruption on i386,
but didn't have time to look at it yet.

Will be definitely stable material once it hit mainline.

-Andi

2006-09-26 15:45:19

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] arch/i386/pci/mmconfig.c tlb flush fix

Andi Kleen <[email protected]> writes:

> OGAWA Hirofumi <[email protected]> writes:
>>
>> We use the fixmap for accessing pci config space in pci_mmcfg_read/write().
>> The problem is in pci_exp_set_dev_base(). It is caching a last
>> accessed address to avoid calling set_fixmap_nocache() whenever
>> pci_mmcfg_read/write() is used.
>
>
> Good catch. I already had another report of mmconfig corruption on i386,
> but didn't have time to look at it yet.
>
> Will be definitely stable material once it hit mainline.

Indeed.

If I found this in mainline, should I send something like a reminder
email with patch to you, Greg?
--
OGAWA Hirofumi <[email protected]>

2006-09-26 15:47:22

by Andi Kleen

[permalink] [raw]
Subject: Re: [PATCH] arch/i386/pci/mmconfig.c tlb flush fix

On Tuesday 26 September 2006 17:45, OGAWA Hirofumi wrote:
> Andi Kleen <[email protected]> writes:
>
> > OGAWA Hirofumi <[email protected]> writes:
> >>
> >> We use the fixmap for accessing pci config space in pci_mmcfg_read/write().
> >> The problem is in pci_exp_set_dev_base(). It is caching a last
> >> accessed address to avoid calling set_fixmap_nocache() whenever
> >> pci_mmcfg_read/write() is used.
> >
> >
> > Good catch. I already had another report of mmconfig corruption on i386,
> > but didn't have time to look at it yet.
> >
> > Will be definitely stable material once it hit mainline.
>
> Indeed.
>
> If I found this in mainline, should I send something like a reminder
> email with patch to you, Greg?

I have it already queued for both.

-Andi

2006-09-26 16:31:26

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] arch/i386/pci/mmconfig.c tlb flush fix

Andi Kleen <[email protected]> writes:

> On Tuesday 26 September 2006 17:45, OGAWA Hirofumi wrote:
>> Andi Kleen <[email protected]> writes:
>>
>> > OGAWA Hirofumi <[email protected]> writes:
>> >>
>> >> We use the fixmap for accessing pci config space in pci_mmcfg_read/write().
>> >> The problem is in pci_exp_set_dev_base(). It is caching a last
>> >> accessed address to avoid calling set_fixmap_nocache() whenever
>> >> pci_mmcfg_read/write() is used.
>> >
>> >
>> > Good catch. I already had another report of mmconfig corruption on i386,
>> > but didn't have time to look at it yet.
>> >
>> > Will be definitely stable material once it hit mainline.
>>
>> Indeed.
>>
>> If I found this in mainline, should I send something like a reminder
>> email with patch to you, Greg?
>
> I have it already queued for both.

Oh, thanks.
--
OGAWA Hirofumi <[email protected]>