Re: [git pull] x86 updates

[Ingo Molnar <mingo@elte.hu>]

* Andi Kleen <andi@firstfloor.org> wrote:

> Ingo Molnar <mingo@elte.hu> writes:
> >
> > Thomas Gleixner (1):
> >       x86: EFI: fix use of unitialized variable and the cache logic
> 
> Your honor, I would like to register a differing opinion...

As i mentioned it (in the portion of my email that you clipped), there 
are other pending CPA patches in x86.git:

> > [there are more CPA cleanups still brewing but none of that is 
> > urgent.]

We didnt plan to push these secondary alias fixes out yet with 
yesterday's push, and i disagree with you about their urgency.

> I submitted that fix originally in a different form, but it got 
> finally stripped down to this. However in my opinion the full version 
> of the patch is still needed, otherwise EFI still does the mapping 
> wrong.

well, not really. Firtly, and most importantly, can you see any failures 
on any real boxes?

I'd be surprised if you could see any failures, because secondary 
aliases have little practical relevance today. Getting the _primary_ 
alias right is the most important task of CPA and -git does that all 
correctly. (we'd be seeing crashes left and right in -rc1 if it didnt)

Secondary virtual alias discovery indeed has an ugly-looking but 
fortunately harmless bug in -git (fixed in x86.git#mm).

The fix is rather straightforward, but we want to test it some more. 
We'll push the fix probably before -rc2 though, in the next few days.

> On 64bit cpa will still change cache attributes on random low pages 
> and cause illegal caching attribute aliases on both 32bit and 64bit.

Well, this is moot with latest x86.git, but you are still wrong.

Firstly, what we do currently incorrectly (and which is fixed in 
x86.git) is that we do a __pa() on a vmalloc/ioremap-range address on 
64-bit in secondary alias discovery. But as i showed it to you days ago 
in my reply, while it will yield a "random" looking address, but that 
address is a guaranteed-high physical address in the higher than 
0x410000000000 (i.e. 65TB+) physical range, so we'll just return 
harmlessly from the filter function. So yes, it's very much ugly and 
unintended and we've been working on fixing it (and have fixed it).

Secondly, your suggestion that inconsistent caching attribute aliases 
are not permitted is incorrect. They are very much not allowed under 
PAT, and they are "nice to avoid" even on non-PAT (early CPU iterations 
sometimes have erratas in this area), but we always had them in one way 
or another - permanently and temporarily as well. What Linux still 
relies on fundamentally for cache attributes are the MTRR attributes.

Anyway, please take a look at today's x86.git#mm:

  git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git mm

it should handle secondary aliases correctly - but we are still working 
on some other cleanups to make it complete.

If anyone proves us wrong (with a specific example - what is mapped 
where, and what bad effect does it have, or if it's available a 
crashlog, etc.) we'll certainly reconsider.

	Ingo