lkml.org 
[lkml]   [2009]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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;
> }


\
 
 \ /
  Last update: 2009-04-09 14:31    [W:0.148 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site