LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andi Kleen <ak@suse.de>
To: Ingo Molnar <mingo@elte.hu>
Cc: tglx@linutronix.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] [2/8] CPA: Flush the caches when setting pages not present
Date: Mon, 11 Feb 2008 13:26:31 +0100	[thread overview]
Message-ID: <200802111326.31515.ak@suse.de> (raw)
In-Reply-To: <20080211110025.GA15373@elte.hu>

On Monday 11 February 2008 12:00:25 Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > e.g. the AMD64 pci-gart code sets pages not present to prevent 
> > potential cache coherency problems.  When doing this it is probably 
> > safer to flush the caches too. So consider clearing of the present bit 
> > as a cache flush indicator.
> 
> uhm, but why?  "Probably safer" is not the right kind of technical  
> argument ;-)

It is safer in my opinion, but I have not seen a concrete bug triggered by
it so I qualified it.

Also it is quite common to qualify technical arguments this way BTW.
 
> The PCI-GART quirk which we use on some systems unmaps pages _not_ 
> because of coherency problems (any cache coherency problems would be 
> triggerable before we unmap them anyway),

The GART is unmapped to avoid a cache coherency problem. AGP 
normally requires cache flushes and changing to uncached to map  
pages into the GART. Otherwise you could have cache incoherence
between the original page and the remapped page.
Since it would be very expensive to call cpa on each pci_map_* 
to turn the pages uncached the  IOMMU part of the GART is unmapped instead 
so that the CPU only ever sees the original memory; never the remapped
part.

> but due to _page aliasing_  
> problems: these pages might be in our general e820 map so we must 
> disable them.
> 
> ( in any case, the CPA code does not deal with cache attribute coherency 
>   - that is topic of the upcoming PAT feature. We still largely rely on 
>   MTRRs to get cache coherency right. )

If it wouldn't deal with cache coherency surely it wouldn't need to flush
caches ...

I started pageattr.c originally to deal with a concrete cache coherency
bugs (AGP cache corruptions between GART and original page on Athlon K7).
This is so that when a page is remapped in the GART it is changed in the 
direct mapping to uncached. Without that there was a reproducible cache corruption
on some systems which was very hard to track down and debug.

Even with all your changes it still does that.

All the other uses like DEBUG_RODATA or DEBUG_PAGEALLOC only came later.

 
> furthermore, your patch does not even do what you claim it does, because
> it is an elaborate NOP on most modern CPUs:
> 
> > +static inline int cache_attr(pgprot_t set, pgprot_t clr)
> >  {
> > -	return pgprot_val(attr) &
> > +	/*
> > +	 * Clearing pages is usually done for cache cohereny reasons
> > +	 * (except for pagealloc debug, but that doesn't call this anyways)
> > +	 * It's safer to flush the caches in this case too.
> > +	 */
> > +	if (pgprot_val(clr) & _PAGE_PRESENT)
> > +		return 1;
> 
> as clflush does not work on not present pages...

Hmm, that's true. It needs to force WBINVD for this case. 
I'll revise. Thanks.

-Andi

  reply	other threads:[~2008-02-11 12:28 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-11  9:34 [PATCH] [0/8] Various kernel mapping bug fixes Andi Kleen
2008-02-11  9:34 ` [PATCH] [1/8] CPA: Fix gbpages support in try_preserve_lage_page Andi Kleen
2008-02-11  9:45   ` Thomas Gleixner
2008-02-11 10:12     ` Ingo Molnar
2008-02-11 11:01       ` Andi Kleen
2008-02-11  9:34 ` [PATCH] [2/8] CPA: Flush the caches when setting pages not present Andi Kleen
2008-02-11 11:00   ` Ingo Molnar
2008-02-11 12:26     ` Andi Kleen [this message]
2008-02-11  9:34 ` [PATCH] [3/8] CPA: Test the correct mapping alias on x86-64 Andi Kleen
2008-02-11 11:49   ` Ingo Molnar
2008-02-11  9:34 ` [PATCH] [4/8] CPA: Fix set_memory_x for ioremap Andi Kleen
2008-02-11 12:27   ` Ingo Molnar
2008-02-11 12:45     ` Andi Kleen
2008-02-11  9:34 ` [PATCH] [5/8] Fix logic error in 64bit memory hotadd Andi Kleen
2008-02-11 12:46   ` Ingo Molnar
2008-02-11 13:05     ` Andi Kleen
2008-02-11 13:33       ` Ingo Molnar
2008-02-11 13:44         ` Andi Kleen
2008-02-12 10:35       ` Yasunori Goto
2008-02-12 11:20         ` Andi Kleen
2008-02-11  9:34 ` [PATCH] [6/8] Account overlapped mappings in end_pfn_map Andi Kleen
2008-02-11 13:08   ` Ingo Molnar
2008-02-11 13:27     ` Andi Kleen
2008-02-11 13:55       ` Ingo Molnar
2008-02-11 14:16       ` Peter Zijlstra
2008-02-11 14:24         ` Andi Kleen
2008-02-11 14:41           ` Sam Ravnborg
2008-02-11 15:12   ` Arjan van de Ven
2008-02-11  9:34 ` [PATCH] [7/8] Implement true end_pfn_mapped for 32bit Andi Kleen
2008-02-12 19:39   ` Thomas Gleixner
2008-02-12 19:49     ` Andi Kleen
2008-02-12 20:25       ` Thomas Gleixner
2008-02-11  9:34 ` [PATCH] [8/8] RFC: Fix some EFI problems Andi Kleen
2008-02-12 20:04   ` Thomas Gleixner
2008-02-12 20:23     ` Andi Kleen
2008-02-12 20:48       ` Thomas Gleixner
2008-02-13 11:05         ` Andi Kleen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200802111326.31515.ak@suse.de \
    --to=ak@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH] [2/8] CPA: Flush the caches when setting pages not present' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).