[PATCH] 8xx: avoid icbi misbehaviour in __flush_dcache_icache_phys

[PATCH] 8xx: avoid icbi misbehaviour in __flush_dcache_icache_phys

[Marcelo Tosatti]

Anton,

Can you please test the following which is equivalent to your=20
suggest changes, the only difference being the location, contained=20
inside flush_dcache_icache_page(). After confirmation that it works
we can send up to the kernel maintainers.

We should still try to understand why this is happening, possibly=20
matching it to a published CPU errata bug, or inform Freescale
otherwise.

I've written that entry down in the 8xx TODO list.

Thanks!

-------------

On 8xx, in the case where a pagefault happens for a process who's not
the owner of the vma in question (ptrace for instance), the flush=20
operation is performed via the physical address.

Unfortunately, that results in a strange, unexplainable "icbi"=20
instruction fault, most likely due to a CPU bug (see oops below).

Avoid that by flushing the page via its kernel virtual address.=20

Oops: kernel access of bad area, sig: 11 [#2]
NIP: C000543C LR: C000B060 SP: C0F35DF0 REGS: c0f35d40 TRAP: 0300 Not tai=
nted
MSR: 00009022 EE: 1 PR: 0 FP: 0 ME: 1 IR/DR: 10
DAR: 00000010, DSISR: C2000000
TASK =3D c0ea8430[761] 'gdbserver' THREAD: c0f34000
Last syscall: 26
GPR00: 00009022 C0F35DF0 C0EA8430 00F59000 00000100 FFFFFFFF 00F58000
00000001
GPR08: C021DAEF C0270000 00009032 C0270000 22044024 10025428 01000800
00000001
GPR16: 007FFF3F 00000001 00000000 7FBC6AC0 00F61022 00000001 C0839300
C01E0000
GPR24: 00CD0889 C082F568 3000AC18 C02A7A00 C0EA15C8 00F588A9 C02ACB00
C02ACB00
NIP [c000543c] __flush_dcache_icache_phys+0x38/0x54
LR [c000b060] flush_dcache_icache_page+0x20/0x30
Call trace:
[c000b154] update_mmu_cache+0x7c/0xa4
[c005ae98] do_wp_page+0x460/0x5ec
[c005c8a0] handle_mm_fault+0x7cc/0x91c
[c005ccec] get_user_pages+0x2fc/0x65c
[c0027104] access_process_vm+0x9c/0x1d4
[c00076e0] sys_ptrace+0x240/0x4a4
[c0002bd0] ret_from_syscall+0x0/0x44

From: Anton W=F6llert <a.woellert@gmail.com>=20
Signed-off-by: Marcelo Tosatti <marcelo.tosatti@cyclades.com>

--- a/arch/ppc/mm/init.c.orig	2005-07-20 03:05:44.000000000 -0300
+++ b/arch/ppc/mm/init.c	2005-07-20 03:05:46.000000000 -0300
@@ -577,6 +577,9 @@
 #ifdef CONFIG_BOOKE
 	__flush_dcache_icache(kmap(page));
 	kunmap(page);
+#elif CONFIG_8xx
+	/* On 8xx there is no need to kmap since highmem is not supported */
+	__flush_dcache_icache(page_address(page));=20
 #else
 	__flush_dcache_icache_phys(page_to_pfn(page) << PAGE_SHIFT);
 #endif

Re: [PATCH] 8xx: avoid icbi misbehaviour in __flush_dcache_icache_phys

[Anton Wöllert]

Marcelo Tosatti wrote:
> Can you please test the following which is equivalent to your 
> suggest changes, the only difference being the location, contained 
> inside flush_dcache_icache_page(). After confirmation that it works
> we can send up to the kernel maintainers.

of course, i can do that :).

> We should still try to understand why this is happening, possibly 
> matching it to a published CPU errata bug, or inform Freescale
> otherwise.

i think so too. as far as i understand it, the thing with the previous 
__flush_dcache_icache&tlbie patch was that __flush_dcache_icache should 
try to call dcbst on every address on a cache-line-boundary that could 
reference the page to give the cpu a hint, so that the next write-access 
to this page will be more effective, right? but while trying to do the 
dcbst, the TLB is looked up for the addresses for a faster translation. 
but here the old TLB-Entry exists, that says invalid Page or something 
like that, right? So it should be invalidated before, right?
	Question, normally the dcbst should cause a TLB-Error-Exception, that 
than should do reload the TLB-Entry automatically, doesn't this work 
because of the dcb*-errata (btw. where can i found that?). And the 
__flush_dcache_icache should do a dcbst 4096/16=256 times, but there are 
just 64 cache-lines à 16Byte (1Kb DCache), so shouldn't be 3/4 of that 
routine redundant? and also, as it seems to me, the whole dcache will be 
*flushed* or better dcbst'd trough this routine. is that right?
please correct me!

so, now the __flush_dcache_icache_phys(). this is just called, when the 
memory-mapping of the vma is not same as the one of that from the 
current process. this should be the case with ptrace. couldn't now be 
done the same as above? maybe no because the address is the virtual 
address as seen from the ptraced process? and so that will *flush* the 
pages from the ptraced process (that doesn't exists yet?, because they 
were just created for the tracing process?).
so the pte, that was created for the tracing process is used. from that, 
the physical address is calculated (how is this done, whats the magic 
about the pfn-stuff etc, i didn't really understand that :( ). now 
__flush_dcache_icache_phys is used, because the [d|i]cache uses physical 
address tags and so the data-mmu could be turned off to flush these entries.
so to understand the things further, i have to understand which 
addresses are used with dcbst and icbi. there is 
page=pfn_to_page(pte_pfn(pte)) and (page_to_pfn(page) << PAGE_SHIFT). 
but what are these values or where do they point. any hints to the 
pfn&pte stuff? unfortunately i didn't have much time the last days to 
understand this more deeply...

> 
> On 8xx, in the case where a pagefault happens for a process who's not
> the owner of the vma in question (ptrace for instance), the flush 
> operation is performed via the physical address.
> 
> Unfortunately, that results in a strange, unexplainable "icbi" 
> instruction fault, most likely due to a CPU bug (see oops below).

where do you know, that "icbi" is the bad?

Anton

Re: [PATCH] 8xx: avoid icbi misbehaviour in __flush_dcache_icache_phys

[Marcelo Tosatti]

Hi Anton,

On Thu, Jul 21, 2005 at 01:34:53AM +0200, Anton W=F6llert wrote:
> Marcelo Tosatti wrote:
> >Can you please test the following which is equivalent to your=20
> >suggest changes, the only difference being the location, contained=20
> >inside flush_dcache_icache_page(). After confirmation that it works
> >we can send up to the kernel maintainers.
>=20
> of course, i can do that :).
>=20
> >We should still try to understand why this is happening, possibly=20
> >matching it to a published CPU errata bug, or inform Freescale
> >otherwise.
>=20
> i think so too. as far as i understand it, the thing with the previous=20
> __flush_dcache_icache&tlbie patch was that __flush_dcache_icache should=
=20
> try to call dcbst on every address on a cache-line-boundary that could=20
> reference the page to give the cpu a hint, so that the next write-acces=
s=20
> to this page will be more effective, right? but while trying to do the=20
> dcbst, the TLB is looked up for the addresses for a faster translation.=
=20
> but here the old TLB-Entry exists, that says invalid Page or something=20
> like that, right? So it should be invalidated before, right?

Actually the dcbst instruction flushes data from cache to main memory.
Quoting MPC860UM.pdf:

Data Cache Block Store (dcbst)

The effective address is computed, translated, and checked for protection
violations as defined in the PowerPC architecture. This instruction is=20
treated as a load with respect to address translation and memory protecti=
on.
If the address hits in the cache and the cache block is in the
unmodified-valid state, no action is taken. If the address hits in the ca=
che
and the cache block is in the modified-valid state, the modified block is=
=20
written back to memory and the cache block is placed in the unmodified-va=
lid
state.

> 	Question, normally the dcbst should cause a TLB-Error-Exception,=20
> 	that than should do reload the TLB-Entry automatically, doesn't this w=
ork=20
> because of the dcb*-errata (btw. where can i found that?).=20

I dont think that this one bug which the TLB invalidate worksaround, name=
ly=20
dcbst misbehaviour with invalid (zeroed) TLB, is covered by Freescale
errata (I'm looking at Errata revision A.2).

> And the __flush_dcache_icache should do a dcbst 4096/16=3D256 times, bu=
t there are=20
> just 64 cache-lines =E0 16Byte (1Kb DCache), so shouldn't be 3/4 of tha=
t=20
> routine redundant? and also, as it seems to me, the whole dcache will b=
e=20
> *flushed* or better dcbst'd trough this routine. is that right?
> please correct me!

See the dcbst description above, the cache line entry is only flushed if
the address hits in cache _and_ is the modified state.

> so, now the __flush_dcache_icache_phys(). this is just called, when the=
=20
> memory-mapping of the vma is not same as the one of that from the=20
> current process. this should be the case with ptrace. couldn't now be=20
> done the same as above? maybe no because the address is the virtual=20
> address as seen from the ptraced process? and so that will *flush* the=20
> pages from the ptraced process (that doesn't exists yet?, because they=20
> were just created for the tracing process?).=20

Since the 8xx cache is physically indexed and tagged, you can flush memor=
y
by either its virtual address or its physical address - virtual accesses=20
need to perform v->p translation with help from the MMU.

On the ptrace case the kernel must do a physical flush because the ptrace=
ing=20
process does not have an equivalent virtual->physical mapping in its own=20
address space. ie. equivalent virtual addresses from the ptracing and=20
ptrace'd processes are obviously not physically equivalent.=20

The change you proposed flushes the page via its kernel virtual map:=20
page_address(page), which is similar to doing a physical flush. Only=20
difference being that on 8xx "icbi" goes nuts when using the physical=20
flush, as you reported.

> so the pte, that was created for the tracing process is used. from that=
,=20
> the physical address is calculated (how is this done, whats the magic=20
> about the pfn-stuff etc, i didn't really understand that :( ).

Actually the page being faulted in by the ptrace'ing process is a page
of the _ptraced_ process, that is, one process is faulting in pages=20
of another process.

> now  __flush_dcache_icache_phys is used, because the [d|i]cache uses ph=
ysical=20
> address tags and so the data-mmu could be turned off to flush these ent=
ries.
> so to understand the things further, i have to understand which=20
> addresses are used with dcbst and icbi. there is=20
> page=3Dpfn_to_page(pte_pfn(pte)) and (page_to_pfn(page) << PAGE_SHIFT).=
=20
> but what are these values or where do they point. any hints to the=20
> pfn&pte stuff? unfortunately i didn't have much time the last days to=20
> understand this more deeply...=20

PFN =3D Page Frame Number. This value is the page identifier by page size=
 units.=20

page_to_pfn(page) << PAGE_SHIFT means:

Get the pfn of the page and multiply it by page size ("<< PAGE_SHIFT").=20

Returns address of page in bytes, not in page units.

> >On 8xx, in the case where a pagefault happens for a process who's not
> >the owner of the vma in question (ptrace for instance), the flush=20
> >operation is performed via the physical address.
> >
> >Unfortunately, that results in a strange, unexplainable "icbi"=20
> >instruction fault, most likely due to a CPU bug (see oops below).
>=20
> where do you know, that "icbi" is the bad?

What instruction resides at __flush_dcache_icache_phys+0x38 ?=20

>From what I could see that is "icbi".