Messages in this thread | | | Date | Thu, 9 Apr 2009 14:27:20 +0200 | From | Joerg Roedel <> | Subject | Re: [PATCH] Enable GART-IOMMU only after setting up protection methods |
| |
On Wed, Apr 08, 2009 at 04:08:55PM -0500, Mark Langsdorf wrote: > The current code to set up the GART as an IOMMU enables GART > translations before it removes the aperture from the kernel > memory map, sets the GART PTEs to UC, sets up the guard and > scratch pages, or does a wbinvd(). This leaves the possibility > of cache aliasing open and can cause system crashes.
This patch looks broken. I don't think that there are any aliasing problems in the existing code. Can you elaborate were do you see these problems? Is there any existing bug report for this?
> Re-order the code and add tlbflush so as to enable the > GART translations only after all safeguards are in place and > the tlb has been flushed. > > AMD has tested this patch and seen no adverse effects. > > Signed-off-by: Mark Langsdorf <mark.langsdorf@amd.com> > > diff -r 0d1744c7acc7 arch/x86/kernel/pci-gart_64.c > --- a/arch/x86/kernel/pci-gart_64.c Fri Mar 27 16:47:28 2009 -0500 > +++ b/arch/x86/kernel/pci-gart_64.c Mon Mar 30 15:05:47 2009 -0500 > @@ -38,6 +38,7 @@ > #include <asm/swiotlb.h> > #include <asm/dma.h> > #include <asm/k8.h> > +#include <asm/tlbflush.h> > > static unsigned long iommu_bus_base; /* GART remapping area (physical) */ > static unsigned long iommu_size; /* size of remapping area bytes */ > @@ -682,9 +683,9 @@ > if (set_memory_uc((unsigned long)gatt, gatt_size >> PAGE_SHIFT)) > panic("Could not set GART PTEs to uncacheable pages"); > > + wbinvd(); > + > agp_gatt_table = gatt; > - > - enable_gart_translations();
To enable the GART here has the advantage that all other CPUs are not yet up. Since the GATT has only non-present entries there is no cache aliasing problem here. If you enable it later you need to flush the cache on _all_ CPUs and not just the CPU where the GART initialization code runs.
> > error = sysdev_class_register(&gart_sysdev_class); > if (!error) > @@ -855,6 +856,11 @@ > for (i = EMERGENCY_PAGES; i < iommu_pages; i++) > iommu_gatt_base[i] = gart_unmapped_entry; > > + wbinvd();
Why a wbinvd() here? A few lines above is already one.
> + flush_tlb_all();
Same with TLB flush. The set_memory_np() function does that for us.
> + > + enable_gart_translations(); > +
If we change the enablement logic like done in this patch you need to invalidate the caches on _all_ cpus. I don't think this is necessary if we just enable all GARTs when only the boot cpu is up.
> flush_gart(); > dma_ops = &gart_dma_ops; > }
| |