* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Joakim Tjernlund @ 2009-10-04 20:38 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <1254688002.7122.28.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 04/10/2009 22:26:42:
> On Sun, 2009-10-04 at 10:35 +0200, Joakim Tjernlund wrote:
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 03/10/2009 12:57:28:
> > >
> > > On Sat, 2009-10-03 at 11:24 +0200, Joakim Tjernlund wrote:
> > > >
> > > > So yes, there is a missing _tlbil_va() missing for 8xx somewhere
> > > > but there is something more too.
> > > > Maybe your new filter functions and my
> > > > powerpc, 8xx: DTLB Error must check for more errors.
> > > > will do the trick?
> > >
> > > Well, if we can't tell between a load and a store on a TLB miss, then
> > > we should probably let it create an unpopulated entry in all cases,
> > > so that we do take a proper DSI/ISI the second time around, which
> > > would then tell us where we come from...
> >
> > While looking closer on 8xx TLB handling I noticed we were taking
> > an extra TLB Error when making a page dirty. Tracked it down to:
> > static inline pte_t pte_mkdirty(pte_t pte) {
> > pte_val(pte) |= _PAGE_DIRTY; return pte; }
> > pte_mkdirty does not set HWWRITE thus forcing a new
> > TLB error to clear it. Adding HWWRITE to pte_mkdirty fixes the
> > problem.
>
> We should just get rid of HWWRITE like I did for 44x and BookE.
I am trying :)
>
> > Looking at (especially pte_mkclean):
> > static inline pte_t pte_wrprotect(pte_t pte) {
> > pte_val(pte) &= ~(_PAGE_RW | _PAGE_HWWRITE); return pte; }
> > static inline pte_t pte_mkclean(pte_t pte) {
> > pte_val(pte) &= ~(_PAGE_DIRTY | _PAGE_HWWRITE); return pte; }
> >
> > it looks like a mistake not to include HWWRITE in pte_mkdirty(),
> > what do you think?
>
> Maybe yes. No big deal right now, we have more important problems
> to fix no ? :-)
The missing invalidate you guys need to work out. I have no clue
where to put it. If we are really lucky, getting rid of HWWRITE
might help :)
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Benjamin Herrenschmidt @ 2009-10-04 20:28 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <OF7C895670.A7F16A1A-ONC1257645.00620A4D-C1257645.006ED40F@transmode.se>
> I have managed to update the TLB code to make proper use of dirty and accessed states.
> Advantages are:
> - I/D TLB Miss never needs to write to the linux pte, saving a few cycles
That's good, that leaves us with only 40x to fix now. Also we can remove
atomic updates of PTEs for all non-hash. It's pointless on those CPUs
anyway.
> - Accessed is only set by I/D TLB Error, should be a plus when SWAP is used.
No need for that neither.
ISI/DSI shouldn't touch the PTE. They should just fall back to C code
which takes care of it all.l
> - _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
> and there will be no extra DTLB Error to actually set the changed bit
> when a page has been made dirty.
> - Proper RO/RW mapping of user space.
>
> Cons:
> - 4 more insn in TLB Miss handlers, but the since the linux pte isn't
> written it should still be a win.
>
> However, I did this on my 2.4 tree but I can port it to 2.6 if you guys
> can test it for me.
Why don't you use and test 2.6 ? :-)
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Benjamin Herrenschmidt @ 2009-10-04 20:26 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <OF1CF373A5.3E3C833B-ONC1257645.002E46F1-C1257645.002F3070@transmode.se>
On Sun, 2009-10-04 at 10:35 +0200, Joakim Tjernlund wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 03/10/2009 12:57:28:
> >
> > On Sat, 2009-10-03 at 11:24 +0200, Joakim Tjernlund wrote:
> > >
> > > So yes, there is a missing _tlbil_va() missing for 8xx somewhere
> > > but there is something more too.
> > > Maybe your new filter functions and my
> > > powerpc, 8xx: DTLB Error must check for more errors.
> > > will do the trick?
> >
> > Well, if we can't tell between a load and a store on a TLB miss, then
> > we should probably let it create an unpopulated entry in all cases,
> > so that we do take a proper DSI/ISI the second time around, which
> > would then tell us where we come from...
>
> While looking closer on 8xx TLB handling I noticed we were taking
> an extra TLB Error when making a page dirty. Tracked it down to:
> static inline pte_t pte_mkdirty(pte_t pte) {
> pte_val(pte) |= _PAGE_DIRTY; return pte; }
> pte_mkdirty does not set HWWRITE thus forcing a new
> TLB error to clear it. Adding HWWRITE to pte_mkdirty fixes the
> problem.
We should just get rid of HWWRITE like I did for 44x and BookE.
> Looking at (especially pte_mkclean):
> static inline pte_t pte_wrprotect(pte_t pte) {
> pte_val(pte) &= ~(_PAGE_RW | _PAGE_HWWRITE); return pte; }
> static inline pte_t pte_mkclean(pte_t pte) {
> pte_val(pte) &= ~(_PAGE_DIRTY | _PAGE_HWWRITE); return pte; }
>
> it looks like a mistake not to include HWWRITE in pte_mkdirty(),
> what do you think?
Maybe yes. No big deal right now, we have more important problems
to fix no ? :-)
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Joakim Tjernlund @ 2009-10-04 20:10 UTC (permalink / raw)
To: Scott Wood, Rex Feany; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <20091002214949.GA20514@b07421-ec1.am.freescale.net>
Scott Wood <scottwood@freescale.com> wrote on 02/10/2009 23:49:49:
>
> On Thu, Oct 01, 2009 at 08:35:59AM +1000, Benjamin Herrenschmidt wrote:
> > >From what I can see, the TLB miss code will check _PAGE_PRESENT, and
> > when not set, it will -still- insert something into the TLB (unlike
> > all other CPU types that go straight to data access faults from there).
> >
> > So we end up populating with those unpopulated entries that will then
> > cause us to take a DSI (or ISI) instead of a TLB miss the next time
> > around ... and so we need to remove them once we do that, no ? IE. Once
> > we have actually put a valid PTE in.
> >
> > At least that's my understanding and why I believe we should always have
> > tlbil_va() in set_pte() and ptep_set_access_flags(), which boils down
> > in putting it into the 2 "filter" functions in the new code.
> >
> > Well.. actually, the ptep_set_access_flags() will already do a
> > flush_tlb_page_nohash() when the PTE is changed. So I suppose all we
> > really need here would be in set_pte_filter(), to do the same if we are
> > writing a PTE that has _PAGE_PRESENT, at least on 8xx.
> >
> > But just unconditionally doing a tlbil_va() in both the filter functions
> > shouldn't harm and also fix the problem, though Rex seems to indicate
> > that is not the case.
>
> Adding a tlbil_va to do_page_fault makes the problem go away for me (on
> top of your "merge" branch) -- none of the other changes in this thread
> do (assuming I didn't miss any). FWIW, when it gets stuck on a fault,
> DSISR is 0xc0000000, and handle_mm_fault returns zero.
Scott and Rex
I have managed to update the TLB code to make proper use of dirty and accessed states.
Advantages are:
- I/D TLB Miss never needs to write to the linux pte, saving a few cycles
- Accessed is only set by I/D TLB Error, should be a plus when SWAP is used.
- _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
and there will be no extra DTLB Error to actually set the changed bit
when a page has been made dirty.
- Proper RO/RW mapping of user space.
Cons:
- 4 more insn in TLB Miss handlers, but the since the linux pte isn't
written it should still be a win.
However, I did this on my 2.4 tree but I can port it to 2.6 if you guys
can test it for me.
Jocke
^ permalink raw reply
* Re: [patch 1/3] powerpc: Move 64bit VDSO to improve context switch performance
From: Andreas Schwab @ 2009-10-04 12:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Anton Blanchard
In-Reply-To: <1254606807.7122.23.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> Maybe a better fix is to force alignment in the kernel by requesting
> size + 64k - 4k and aligning it.
Sure you are right.
Andreas.
>From b9441a3d2148d439e2730def3222a7b70dccc432 Mon Sep 17 00:00:00 2001
From: Andreas Schwab <schwab@linux-m68k.org>
Date: Sun, 4 Oct 2009 14:29:04 +0200
Subject: [PATCH] powerpc: align vDSO base address
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
---
arch/powerpc/kernel/vdso.c | 11 ++++++++++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index 94e2df3..137dc22 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -50,6 +50,9 @@
/* Max supported size for symbol names */
#define MAX_SYMNAME 64
+/* The alignment of the vDSO */
+#define VDSO_ALIGNMENT (1 << 16)
+
extern char vdso32_start, vdso32_end;
static void *vdso32_kbase = &vdso32_start;
static unsigned int vdso32_pages;
@@ -231,15 +234,21 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
* pick a base address for the vDSO in process space. We try to put it
* at vdso_base which is the "natural" base for it, but we might fail
* and end up putting it elsewhere.
+ * Add enough to the size so that the result can be aligned.
*/
down_write(&mm->mmap_sem);
vdso_base = get_unmapped_area(NULL, vdso_base,
- vdso_pages << PAGE_SHIFT, 0, 0);
+ (vdso_pages << PAGE_SHIFT) +
+ ((VDSO_ALIGNMENT - 1) & PAGE_MASK),
+ 0, 0);
if (IS_ERR_VALUE(vdso_base)) {
rc = vdso_base;
goto fail_mmapsem;
}
+ /* Add required alignment. */
+ vdso_base = ALIGN(vdso_base, VDSO_ALIGNMENT);
+
/*
* Put vDSO base into mm struct. We need to do this before calling
* install_special_mapping or the perf counter mmap tracking code
--
1.6.4.4
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply related
* Re: [PATCH] sound: Don't assume i2c device probing always succeeds
From: Jean Delvare @ 2009-10-04 9:35 UTC (permalink / raw)
To: Takashi Iwai
Cc: linuxppc-dev, Johannes Berg, Tim Shepard, alsa-devel,
Jaroslav Kysela
In-Reply-To: <s5hiqezk3lg.wl%tiwai@suse.de>
Hi Takashi,
On Thu, 01 Oct 2009 08:52:59 +0200, Takashi Iwai wrote:
> At Wed, 30 Sep 2009 18:55:05 +0200,
> Jean Delvare wrote:
> >
> > On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
> > > Yes, indeed I prefer NULL check because the user can know the error
> > > at the right place. I share your concern about the code addition,
> > > though :)
> > >
> > > I already made a patch below, but it's totally untested.
> > > It'd be helpful if someone can do review and build-test it.
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > ---
> > > diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
> > > index f0ebc97..0f810c8 100644
> > > --- a/sound/aoa/codecs/tas.c
> > > +++ b/sound/aoa/codecs/tas.c
> > > @@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
> > > client = i2c_new_device(adapter, &info);
> > > if (!client)
> > > return -ENODEV;
> > > + if (!client->driver) {
> > > + i2c_unregister_device(client);
> > > + return -ENODEV;
> > > + }
> > >
> > > /*
> > > * Let i2c-core delete that device on driver removal.
> > > diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
> > > index 835fa19..473c5a6 100644
> > > --- a/sound/ppc/keywest.c
> > > +++ b/sound/ppc/keywest.c
> > > @@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
> > > strlcpy(info.type, "keywest", I2C_NAME_SIZE);
> > > info.addr = keywest_ctx->addr;
> > > keywest_ctx->client = i2c_new_device(adapter, &info);
> > > + if (!keywest_ctx->client)
> > > + return -ENODEV;
> > > + if (!keywest_ctx->client->driver) {
> > > + i2c_unregister_device(keywest_ctx->client);
> > > + keywest_ctx->client = NULL;
> > > + return -ENODEV;
> > > + }
> > >
> > > /*
> > > * Let i2c-core delete that device on driver removal.
> >
> > This looks good to me. Please add the following comment before the
> > client->driver check in both drivers:
> >
> > /*
> > * We know the driver is already loaded, so the device should be
> > * already bound. If not it means binding failed, and then there
> > * is no point in keeping the device instantiated.
> > */
> >
> > Otherwise it's a little difficult to understand why the check is there.
>
> Fair enough. I applied the patch with the comment now.
> Thanks!
I see this is upstream now. While the keywest fix was essentially
theoretical, the tas one addresses a crash which really could happen,
so I think it would be worth sending to stable for 2.6.31. What do you
think? Will you take care, or do you want me to?
Thanks,
--
Jean Delvare
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Joakim Tjernlund @ 2009-10-04 8:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <1254567448.7122.8.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 03/10/2009 12:57:28:
>
> On Sat, 2009-10-03 at 11:24 +0200, Joakim Tjernlund wrote:
> >
> > So yes, there is a missing _tlbil_va() missing for 8xx somewhere
> > but there is something more too.
> > Maybe your new filter functions and my
> > powerpc, 8xx: DTLB Error must check for more errors.
> > will do the trick?
>
> Well, if we can't tell between a load and a store on a TLB miss, then
> we should probably let it create an unpopulated entry in all cases,
> so that we do take a proper DSI/ISI the second time around, which
> would then tell us where we come from...
While looking closer on 8xx TLB handling I noticed we were taking
an extra TLB Error when making a page dirty. Tracked it down to:
static inline pte_t pte_mkdirty(pte_t pte) {
pte_val(pte) |= _PAGE_DIRTY; return pte; }
pte_mkdirty does not set HWWRITE thus forcing a new
TLB error to clear it. Adding HWWRITE to pte_mkdirty fixes the
problem.
Looking at (especially pte_mkclean):
static inline pte_t pte_wrprotect(pte_t pte) {
pte_val(pte) &= ~(_PAGE_RW | _PAGE_HWWRITE); return pte; }
static inline pte_t pte_mkclean(pte_t pte) {
pte_val(pte) &= ~(_PAGE_DIRTY | _PAGE_HWWRITE); return pte; }
it looks like a mistake not to include HWWRITE in pte_mkdirty(),
what do you think?
Jocke
^ permalink raw reply
* Re: [patch 1/3] powerpc: Move 64bit VDSO to improve context switch performance
From: Benjamin Herrenschmidt @ 2009-10-03 21:53 UTC (permalink / raw)
To: Andreas Schwab; +Cc: linuxppc-dev, Anton Blanchard
In-Reply-To: <m2ocoo1qfi.fsf@igel.home>
On Sat, 2009-10-03 at 16:51 +0200, Andreas Schwab wrote:
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
> > Andreas Schwab <schwab@linux-m68k.org> writes:
> >
> >> Anton Blanchard <anton@samba.org> writes:
> >>
> >>> On 64bit applications the VDSO is the only thing in segment 0. Since the VDSO
> >>> is position independent we can remove the hint and let get_unmapped_area pick
> >>> an area.
> >>
> >> This breaks gdb. The section table in the VDSO image when mapped into
> >> the process no longer contains meaningful values, and gdb rejects it.
> >
> > The problem is that the load segment requires 64k alignment, but the
> > page allocater of course only provides PAGE_SIZE alignment, causing the
> > image to be unaligned in memory.
>
> Here is a patch. The disadvantage is that now gdb complains that the
> vdso does not contain section headers, because they are no longer
> covered by the load segment.
Maybe a better fix is to force alignment in the kernel by requesting
size + 64k - 4k and aligning it.
Ben.
> Andreas.
> ---
> >From 003c323c753d8717e58220c9560329882bcfa2c2 Mon Sep 17 00:00:00 2001
> From: Andreas Schwab <schwab@linux-m68k.org>
> Date: Sat, 3 Oct 2009 16:46:45 +0200
> Subject: [PATCH] powerpc: fix unaligned load segment in vdso64
>
> Since the page allocator can only guarantee PAGE_SIZE alignment the load
> segment in the vdso object cannot claim more than that alignment. Use 4k
> alignment which is the minimum possible page size.
>
> Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
> ---
> arch/powerpc/kernel/vdso64/Makefile | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
> index 79da65d..d4be303 100644
> --- a/arch/powerpc/kernel/vdso64/Makefile
> +++ b/arch/powerpc/kernel/vdso64/Makefile
> @@ -10,7 +10,9 @@ obj-vdso64 := $(addprefix $(obj)/, $(obj-vdso64))
> GCOV_PROFILE := n
>
> EXTRA_CFLAGS := -shared -fno-common -fno-builtin
> +# Force maximum page size of 4k so that it works with any page size
> EXTRA_CFLAGS += -nostdlib -Wl,-soname=linux-vdso64.so.1 \
> + -Wl,-z,max-page-size=4096 \
> $(call cc-ldoption, -Wl$(comma)--hash-style=sysv)
> EXTRA_AFLAGS := -D__VDSO64__ -s
>
> --
> 1.6.4.4
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/5200: make BestComm gen_bd microcode exchangeable
From: Albrecht Dreß @ 2009-10-03 15:09 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linuxppc-dev
In-Reply-To: <20091003144026.GA27519@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 857 bytes --]
Am 03.10.09 16:40 schrieb(en) Wolfram Sang:
> Sorry, I hardly know anything about the microcode. From what I know,
> it shouldn't be much fun due to various bugs in the Bestcomm engine.
Ummm. That's not encouraging! :-/
> Hey! No need for insults! ;)
Sorry, that wasn't my intention, but it's a MUA I *know* it doesn't
understand it... most OSS clients do (like Balsa, Thunderbird, kmail,
etc.).
> > Or is '3676 in general not acceptable for patches?
>
> Maybe. Could you try without next time, and we will see if it works
> better?
Actually, it's in Documentation/email-clients.txt - '3676 must not be
used. Sorry, should have read that before! (However, messages
*without* folding *and* with lines > 78 characters violate RFC2822...)
> Yup, since -rc1. (git log --grep=mtd-ram)
Thanks!
Cheers, Albrecht.
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [patch 1/3] powerpc: Move 64bit VDSO to improve context switch performance
From: Andreas Schwab @ 2009-10-03 14:51 UTC (permalink / raw)
To: Anton Blanchard; +Cc: linuxppc-dev
In-Reply-To: <m28wfsef79.fsf@igel.home>
Andreas Schwab <schwab@linux-m68k.org> writes:
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Anton Blanchard <anton@samba.org> writes:
>>
>>> On 64bit applications the VDSO is the only thing in segment 0. Since the VDSO
>>> is position independent we can remove the hint and let get_unmapped_area pick
>>> an area.
>>
>> This breaks gdb. The section table in the VDSO image when mapped into
>> the process no longer contains meaningful values, and gdb rejects it.
>
> The problem is that the load segment requires 64k alignment, but the
> page allocater of course only provides PAGE_SIZE alignment, causing the
> image to be unaligned in memory.
Here is a patch. The disadvantage is that now gdb complains that the
vdso does not contain section headers, because they are no longer
covered by the load segment.
Andreas.
---
>From 003c323c753d8717e58220c9560329882bcfa2c2 Mon Sep 17 00:00:00 2001
From: Andreas Schwab <schwab@linux-m68k.org>
Date: Sat, 3 Oct 2009 16:46:45 +0200
Subject: [PATCH] powerpc: fix unaligned load segment in vdso64
Since the page allocator can only guarantee PAGE_SIZE alignment the load
segment in the vdso object cannot claim more than that alignment. Use 4k
alignment which is the minimum possible page size.
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
---
arch/powerpc/kernel/vdso64/Makefile | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 79da65d..d4be303 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -10,7 +10,9 @@ obj-vdso64 := $(addprefix $(obj)/, $(obj-vdso64))
GCOV_PROFILE := n
EXTRA_CFLAGS := -shared -fno-common -fno-builtin
+# Force maximum page size of 4k so that it works with any page size
EXTRA_CFLAGS += -nostdlib -Wl,-soname=linux-vdso64.so.1 \
+ -Wl,-z,max-page-size=4096 \
$(call cc-ldoption, -Wl$(comma)--hash-style=sysv)
EXTRA_AFLAGS := -D__VDSO64__ -s
--
1.6.4.4
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply related
* Re: [PATCH] powerpc/5200: make BestComm gen_bd microcode exchangeable
From: Wolfram Sang @ 2009-10-03 14:40 UTC (permalink / raw)
To: Albrecht Dreß; +Cc: linuxppc-dev
In-Reply-To: <1254577821.3331.0@antares>
[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]
> doesn't give any support, so it's all trial and error and error and
> error... If you have any idea, pointers would be appreciated!
Sorry, I hardly know anything about the microcode. From what I know, it
shouldn't be much fun due to various bugs in the Bestcomm engine.
> I don't think so - the message body is a "format=flowed" (RFC 3676)
> text/plain part. Maybe your MUA doesn't display that format correctly
> (Outlook?)?
Hey! No need for insults! ;)
Have to check if my MUA (see headers) supports it. The archives seem to have
split it as well:
http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-October/076236.html
I recall a similar problem, where some archives had the patch correct and some
didn't.
> Or is '3676 in general not acceptable for patches?
Maybe. Could you try without next time, and we will see if it works better?
> P.S.: Did your mtd-ram/physmap_of patch you submitted a while ago make
> it into the stock kernel? I have some pending extra work (16-bit LPB
> RAM access) building on top of it...
Yup, since -rc1. (git log --grep=mtd-ram)
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [patch 1/3] powerpc: Move 64bit VDSO to improve context switch performance
From: Andreas Schwab @ 2009-10-03 14:15 UTC (permalink / raw)
To: Anton Blanchard; +Cc: linuxppc-dev
In-Reply-To: <m2ocophale.fsf__34527.2158309401$1254511503$gmane$org@igel.home>
Andreas Schwab <schwab@linux-m68k.org> writes:
> Anton Blanchard <anton@samba.org> writes:
>
>> On 64bit applications the VDSO is the only thing in segment 0. Since the VDSO
>> is position independent we can remove the hint and let get_unmapped_area pick
>> an area.
>
> This breaks gdb. The section table in the VDSO image when mapped into
> the process no longer contains meaningful values, and gdb rejects it.
The problem is that the load segment requires 64k alignment, but the
page allocater of course only provides PAGE_SIZE alignment, causing the
image to be unaligned in memory.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH] powerpc/5200: make BestComm gen_bd microcode exchangeable
From: Albrecht Dreß @ 2009-10-03 13:50 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linuxppc-dev
In-Reply-To: <20091003094417.GA24206@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]
Hi Wolfram:
Am 03.10.09 11:44 schrieb(en) Wolfram Sang:
> you wrote your own microcode? :)
I modified the bcom_gen_bd_rx_task for a LPB peripheral as to perform
Endianess swapping during the transfer (works meanwhile :-).
Modifying the standard kernel code for testing seemed to be the wrong
approach for me, so I wrote that patch...
If possible, I would also like to have a crc32 calculation performed by
BestComm, but unfortunately there is no documentation and Freescale
doesn't give any support, so it's all trial and error and error and
error... If you have any idea, pointers would be appreciated!
> approach looks ok to me in general, but this patch is line-wrapped.
I don't think so - the message body is a "format=flowed" (RFC 3676)
text/plain part. Maybe your MUA doesn't display that format correctly
(Outlook?)? Or is '3676 in general not acceptable for patches?
[snip]
> spaces instead of tabs
[snip]
> Two empty lines.
Thanks - will submit a fixed patch next week, stay tuned...
Cheers, Albrecht.
P.S.: Did your mtd-ram/physmap_of patch you submitted a while ago make
it into the stock kernel? I have some pending extra work (16-bit LPB
RAM access) building on top of it...
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Joakim Tjernlund @ 2009-10-03 11:47 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <1254567448.7122.8.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 03/10/2009 12:57:28:
> On Sat, 2009-10-03 at 11:24 +0200, Joakim Tjernlund wrote:
> >
> > So yes, there is a missing _tlbil_va() missing for 8xx somewhere
> > but there is something more too.
> > Maybe your new filter functions and my
> > powerpc, 8xx: DTLB Error must check for more errors.
> > will do the trick?
>
> Well, if we can't tell between a load and a store on a TLB miss, then
> we should probably let it create an unpopulated entry in all cases,
> so that we do take a proper DSI/ISI the second time around, which
> would then tell us where we come from...
Not quite sure what you mean here?
Always branch to DSI at TLB Miss and create a unpopulated
entry? That does not feel right.
The current method is better. The only odd thing is
the null pmd entry and what to load into RPN. I am not convinced
that it is a problem but I would like to see a generic impl. of
a null pmd and use that instead. A 8xx impl. of that
looks like this(tested on 2.4, works fine)
lwz r11, 0(r10) /* Get the level 1 entry */
rlwinm. r10, r11,0,0,19 /* Extract page descriptor page address */
bne+ 4f /* If zero, use a null pmd */
lis r11, null_pmd_dir@h
ori r11, r11, null_pmd_dir@l
4: tophys(r11,r11)
...
.data
.globl null_pmd_dir
null_pmd_dir:
.space 4096
If the pmd_none() and friends are updated to test for a null_pmd_dir
we can loose the test above and save 4 insn :)
Anyhow did you post a patch(can't find one) about your suggested
filter functions for 8xx? I sure would like to see one :)
^ permalink raw reply
* Re: [PATCH] powerpc: fix segment mapping in vdso32
From: Benjamin Herrenschmidt @ 2009-10-03 11:22 UTC (permalink / raw)
To: Andreas Schwab; +Cc: linuxppc-dev
In-Reply-To: <m2bpkoomlw.fsf@whitebox.home>
On Sat, 2009-10-03 at 11:25 +0200, Andreas Schwab wrote:
> Due to missing segment assignments the .text section was put in the NOTES
> segment (and marked as NOTE section), and the .got was put in the DYNAMIC
> segment.
Ouch, good catch ! Thanks.
Cheers,
Ben.
> Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
> ---
> arch/powerpc/kernel/vdso32/vdso32.lds.S | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/vdso32/vdso32.lds.S b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> index 904ef13..0546bcd 100644
> --- a/arch/powerpc/kernel/vdso32/vdso32.lds.S
> +++ b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> @@ -25,7 +25,7 @@ SECTIONS
> . = ALIGN(16);
> .text : {
> *(.text .stub .text.* .gnu.linkonce.t.* __ftr_alt_*)
> - }
> + } :text
> PROVIDE(__etext = .);
> PROVIDE(_etext = .);
> PROVIDE(etext = .);
> @@ -56,7 +56,7 @@ SECTIONS
> .fixup : { *(.fixup) }
>
> .dynamic : { *(.dynamic) } :text :dynamic
> - .got : { *(.got) }
> + .got : { *(.got) } :text
> .plt : { *(.plt) }
>
> _end = .;
> --
> 1.6.4.4
>
^ permalink raw reply
* Re: Is volatile always verboten for FSL QE structures?
From: Benjamin Herrenschmidt @ 2009-10-03 11:20 UTC (permalink / raw)
To: Simon Richter; +Cc: linuxppc-dev
In-Reply-To: <20091003095537.GA15992@honey.hogyros.de>
> Making the target of foo volatile properly rechecks the condition on
> each iteration.
>
> OTOH my PPC box runs fine, so I'm probably missing something obvious.
Probably because the IO accessors do -both- volatile casts and
add the barriers :-)
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Benjamin Herrenschmidt @ 2009-10-03 10:57 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <OF0216D5A2.8B77DD30-ONC1257644.0031FDC4-C1257644.0033A760@transmode.se>
On Sat, 2009-10-03 at 11:24 +0200, Joakim Tjernlund wrote:
>
> So yes, there is a missing _tlbil_va() missing for 8xx somewhere
> but there is something more too.
> Maybe your new filter functions and my
> powerpc, 8xx: DTLB Error must check for more errors.
> will do the trick?
Well, if we can't tell between a load and a store on a TLB miss, then
we should probably let it create an unpopulated entry in all cases,
so that we do take a proper DSI/ISI the second time around, which
would then tell us where we come from...
Ben.
^ permalink raw reply
* Re: Is volatile always verboten for FSL QE structures?
From: Simon Richter @ 2009-10-03 9:55 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20091002200848.06be4c5a@xilun.lan.proformatique.com>
Hi,
> > >> 'volatile' just doesn't really do what you think it should do. The
> > >> PowerPC architecture is too complicated w.r.t. ordering of reads and
> > >> writes. In other words, you can't trust it.
It's not sufficient on PowerPC.
It might be necessary, depending on the compiler's mood for moving stuff
out of loops.
Consider:
| unsigned int *foo = (unsigned int *)0x12345678;
| void bar(void) { while(*foo != 0) asm("eieio"); }
gcc 4.3.4 with -O3 compiles this to
00000000 <bar>:
0: 3d 20 00 00 lis r9,0
2: R_PPC_ADDR16_HA foo
4: 81 69 00 00 lwz r11,0(r9)
6: R_PPC_ADDR16_LO foo
8: 80 0b 00 00 lwz r0,0(r11)
c: 2f 80 00 00 cmpwi cr7,r0,0
10: 4d 9e 00 20 beqlr cr7
14: 7c 00 06 ac eieio
18: 7c 00 06 ac eieio
1c: 4b ff ff f8 b 14 <bar+0x14>
Making the target of foo volatile properly rechecks the condition on
each iteration.
OTOH my PPC box runs fine, so I'm probably missing something obvious.
Simon
^ permalink raw reply
* Re: [PATCH] powerpc/5200: make BestComm gen_bd microcode exchangeable
From: Wolfram Sang @ 2009-10-03 9:44 UTC (permalink / raw)
To: Albrecht Dreß; +Cc: linuxppc-dev
In-Reply-To: <1254426938.3252.1@antares>
[-- Attachment #1: Type: text/plain, Size: 3948 bytes --]
Hi Albrecht,
you wrote your own microcode? :)
approach looks ok to me in general, but this patch is line-wrapped.
On Thu, Oct 01, 2009 at 09:55:38PM +0200, Albrecht Dreß wrote:
> This patch adds a method for defining different microcodes than the
> pe-defined ones for the MPC52xx processor's BestComm General Buffer
pre-defined
> Descriptor (gen_db) tasks. The default microcode is still the one from
> bcom_gen_bd_[rt]x_task, but it can be replaced by calling
> bcom_gen_bd_set_microcode() which is more efficient than explicitly
> loading it via bcom_load_image() after each bcom_gen_bd_[rt]x_reset().
>
> Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de>
>
>
> ---
>
>
> diff -uprN -X linux-2.6.30.3/Documentation/dontdiff
> linux-2.6.30.3.orig/arch/powerpc/sysdev/bestcomm/gen_bd.c
> linux-2.6.30.3/arch/powerpc/sysdev/bestcomm/gen_bd.c
> --- linux-2.6.30.3.orig/arch/powerpc/sysdev/bestcomm/gen_bd.c 2009-07-24
> 23:47:51.000000000 +0200
> +++ linux-2.6.30.3/arch/powerpc/sysdev/bestcomm/gen_bd.c 2009-10-01
> 14:26:33.000000000 +0200
> @@ -78,6 +78,7 @@ struct bcom_gen_bd_priv {
> int initiator;
> int ipr;
> int maxbufsize;
> + u32 *microcode;
spaces instead of tabs
> };
>
>
> @@ -104,6 +105,7 @@ bcom_gen_bd_rx_init(int queue_len, phys_
> priv->initiator = initiator;
> priv->ipr = ipr;
> priv->maxbufsize = maxbufsize;
> + priv->microcode = bcom_gen_bd_rx_task;
>
> if (bcom_gen_bd_rx_reset(tsk)) {
> bcom_task_free(tsk);
> @@ -128,7 +130,7 @@ bcom_gen_bd_rx_reset(struct bcom_task *t
> var = (struct bcom_gen_bd_rx_var *) bcom_task_var(tsk->tasknum);
> inc = (struct bcom_gen_bd_rx_inc *) bcom_task_inc(tsk->tasknum);
>
> - if (bcom_load_image(tsk->tasknum, bcom_gen_bd_rx_task))
> + if (bcom_load_image(tsk->tasknum, priv->microcode))
> return -1;
>
> var->enable = bcom_eng->regs_base +
> @@ -188,6 +190,7 @@ bcom_gen_bd_tx_init(int queue_len, phys_
> priv->fifo = fifo;
> priv->initiator = initiator;
> priv->ipr = ipr;
> + priv->microcode = bcom_gen_bd_tx_task;
>
> if (bcom_gen_bd_tx_reset(tsk)) {
> bcom_task_free(tsk);
> @@ -212,7 +215,7 @@ bcom_gen_bd_tx_reset(struct bcom_task *t
> var = (struct bcom_gen_bd_tx_var *) bcom_task_var(tsk->tasknum);
> inc = (struct bcom_gen_bd_tx_inc *) bcom_task_inc(tsk->tasknum);
>
> - if (bcom_load_image(tsk->tasknum, bcom_gen_bd_tx_task))
> + if (bcom_load_image(tsk->tasknum, priv->microcode))
> return -1;
>
> var->enable = bcom_eng->regs_base +
> @@ -253,6 +256,16 @@ bcom_gen_bd_tx_release(struct bcom_task
> }
> EXPORT_SYMBOL_GPL(bcom_gen_bd_tx_release);
>
> +void
> +bcom_gen_bd_set_microcode(struct bcom_task *tsk, u32 *microcode)
> +{
> + struct bcom_gen_bd_priv *priv = tsk->priv;
> +
> + priv->microcode = microcode;
> +}
> +EXPORT_SYMBOL_GPL(bcom_gen_bd_set_microcode);
> +
> +
Two empty lines.
> /*
> ---------------------------------------------------------------------
> * PSC support code
> */
> diff -uprN -X linux-2.6.30.3/Documentation/dontdiff
> linux-2.6.30.3.orig/arch/powerpc/sysdev/bestcomm/gen_bd.h
> linux-2.6.30.3/arch/powerpc/sysdev/bestcomm/gen_bd.h
> --- linux-2.6.30.3.orig/arch/powerpc/sysdev/bestcomm/gen_bd.h 2009-07-24
> 23:47:51.000000000 +0200
> +++ linux-2.6.30.3/arch/powerpc/sysdev/bestcomm/gen_bd.h 2009-10-01
> 14:26:50.000000000 +0200
> @@ -43,6 +43,9 @@ bcom_gen_bd_tx_reset(struct bcom_task *t
> extern void
> bcom_gen_bd_tx_release(struct bcom_task *tsk);
>
> +extern void
> +bcom_gen_bd_set_microcode(struct bcom_task *tsk, u32 *microcode);
> +
>
> /* PSC support utility wrappers */
> struct bcom_task * bcom_psc_gen_bd_rx_init(unsigned psc_num, int
> queue_len,
>
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Joakim Tjernlund @ 2009-10-03 9:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <1254558678.7122.7.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 03/10/2009 10:31:18:
>
> On Sat, 2009-10-03 at 10:05 +0200, Joakim Tjernlund wrote:
> > Cannot shake the feeling that it this snip of code that causes it
> > lwz r11, 0(r10) /* Get the level 1 entry */
> > rlwinm. r10, r11,0,0,19 /* Extract page descriptor page
> > address */
> > beq 2f /* If zero, don't try to find a pte */
> > Perhaps we can do something better? I still feel that we need to
> > force a TLB Error as the TLBMiss does not set DSISR so we have no way
> > of
> > knowing if it is an load or store.
>
> Can't we manufacture a DSISR and branch to the right function ?
Not if we want know if it is a load or store. There is no info to manufacture
a DSISR from. The best we can do here is try getting the RPN physical page
number correct. Perhaps something like this will do:
/* Copy 20 msb from MD_EPN to r20 to get the correct page
* number. Do not rely on DAR since the dcxx instructions fails
* to update DAR when they cause a DTLB Miss
*/
mfspr r21, MD_EPN
li r20, 0x0
rlwimi r20, r21, 0, 0, 19
Then go back and set the RPN accordingly.
The 8xx is different as as it will force a TLB error every time
it needs to deal with a page fault.
I suspect adding
if (!ret)
_tlbil_va(address);
in do_page_fault() will do the trick too.
So yes, there is a missing _tlbil_va() missing for 8xx somewhere
but there is something more too.
Maybe your new filter functions and my
powerpc, 8xx: DTLB Error must check for more errors.
will do the trick?
Jocke
^ permalink raw reply
* [PATCH] powerpc: fix segment mapping in vdso32
From: Andreas Schwab @ 2009-10-03 9:25 UTC (permalink / raw)
To: linuxppc-dev
Due to missing segment assignments the .text section was put in the NOTES
segment (and marked as NOTE section), and the .got was put in the DYNAMIC
segment.
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
---
arch/powerpc/kernel/vdso32/vdso32.lds.S | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/vdso32/vdso32.lds.S b/arch/powerpc/kernel/vdso32/vdso32.lds.S
index 904ef13..0546bcd 100644
--- a/arch/powerpc/kernel/vdso32/vdso32.lds.S
+++ b/arch/powerpc/kernel/vdso32/vdso32.lds.S
@@ -25,7 +25,7 @@ SECTIONS
. = ALIGN(16);
.text : {
*(.text .stub .text.* .gnu.linkonce.t.* __ftr_alt_*)
- }
+ } :text
PROVIDE(__etext = .);
PROVIDE(_etext = .);
PROVIDE(etext = .);
@@ -56,7 +56,7 @@ SECTIONS
.fixup : { *(.fixup) }
.dynamic : { *(.dynamic) } :text :dynamic
- .got : { *(.got) }
+ .got : { *(.got) } :text
.plt : { *(.plt) }
_end = .;
--
1.6.4.4
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply related
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Benjamin Herrenschmidt @ 2009-10-03 8:31 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <OF4175DD8E.796930EB-ONC1257644.002A7135-C1257644.002C7929@transmode.se>
On Sat, 2009-10-03 at 10:05 +0200, Joakim Tjernlund wrote:
> Cannot shake the feeling that it this snip of code that causes it
> lwz r11, 0(r10) /* Get the level 1 entry */
> rlwinm. r10, r11,0,0,19 /* Extract page descriptor page
> address */
> beq 2f /* If zero, don't try to find a pte */
> Perhaps we can do something better? I still feel that we need to
> force a TLB Error as the TLBMiss does not set DSISR so we have no way
> of
> knowing if it is an load or store.
Can't we manufacture a DSISR and branch to the right function ?
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Joakim Tjernlund @ 2009-10-03 8:05 UTC (permalink / raw)
To: Scott Wood; +Cc: Rex Feany, linuxppc-dev@ozlabs.org
In-Reply-To: <20091002214949.GA20514@b07421-ec1.am.freescale.net>
Scott Wood <scottwood@freescale.com> wrote on 02/10/2009 23:49:49:
>
> On Thu, Oct 01, 2009 at 08:35:59AM +1000, Benjamin Herrenschmidt wrote:
> > >From what I can see, the TLB miss code will check _PAGE_PRESENT, and
> > when not set, it will -still- insert something into the TLB (unlike
> > all other CPU types that go straight to data access faults from there).
> >
> > So we end up populating with those unpopulated entries that will then
> > cause us to take a DSI (or ISI) instead of a TLB miss the next time
> > around ... and so we need to remove them once we do that, no ? IE. Once
> > we have actually put a valid PTE in.
> >
> > At least that's my understanding and why I believe we should always have
> > tlbil_va() in set_pte() and ptep_set_access_flags(), which boils down
> > in putting it into the 2 "filter" functions in the new code.
> >
> > Well.. actually, the ptep_set_access_flags() will already do a
> > flush_tlb_page_nohash() when the PTE is changed. So I suppose all we
> > really need here would be in set_pte_filter(), to do the same if we are
> > writing a PTE that has _PAGE_PRESENT, at least on 8xx.
> >
> > But just unconditionally doing a tlbil_va() in both the filter functions
> > shouldn't harm and also fix the problem, though Rex seems to indicate
> > that is not the case.
>
> Adding a tlbil_va to do_page_fault makes the problem go away for me (on
> top of your "merge" branch) -- none of the other changes in this thread
> do (assuming I didn't miss any). FWIW, when it gets stuck on a fault,
> DSISR is 0xc0000000, and handle_mm_fault returns zero.
OK, that is a no translation error for a load (assuming trap is 0x400)
Do you know what insn this is? I am adding a patch last in this mail
for catching dcbX insn in do_page_fault()
You need the patch I posted yesterday too:
>From c5f1a70561730b8a443f7081fbd9c5b023147043 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Fri, 2 Oct 2009 14:59:21 +0200
Subject: [PATCH] powerpc, 8xx: DTLB Error must check for more errors.
DataTLBError currently does:
if ((err & 0x02000000) == 0)
DSI();
This won't handle a store with no valid translation.
Change this to
if ((err & 0x48000000) != 0)
DSI();
that is, branch to DSI if either !permission or
!translation.
---
arch/powerpc/kernel/head_8xx.S | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 52ff8c5..118bb05 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -472,8 +472,8 @@ DataTLBError:
/* First, make sure this was a store operation.
*/
mfspr r10, SPRN_DSISR
- andis. r11, r10, 0x0200 /* If set, indicates store op */
- beq 2f
+ andis. r11, r10, 0x4800 /* no translation, no permission. */
+ bne 2f /* branch if either is set */
/* The EA of a data TLB miss is automatically stored in the MD_EPN
* register. The EA of a data TLB error is automatically stored in
--
1.6.4.4
Cannot shake the feeling that it this snip of code that causes it
lwz r11, 0(r10) /* Get the level 1 entry */
rlwinm. r10, r11,0,0,19 /* Extract page descriptor page address */
beq 2f /* If zero, don't try to find a pte */
Perhaps we can do something better? I still feel that we need to
force a TLB Error as the TLBMiss does not set DSISR so we have no way of
knowing if it is an load or store.
Jocke
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 7699394..c33c6de 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -139,6 +139,88 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
#else
is_write = error_code & ESR_DST;
#endif /* CONFIG_4xx || CONFIG_BOOKE */
+#if 1 /* defined(CONFIG_8xx)*/
+#define DEBUG_DCBX
+/*
+ Work around DTLB Miss/Error, as these do not update
+ DAR for dcbf, dcbi, dcbst, dcbz and icbi instructions
+ This relies on every exception tagging DAR with 0xf0
+ before returning (rfi)
+ DAR is passed as 'address' to this function.
+ */
+ {
+ unsigned long ra, rb, dar, insn;
+#ifdef DEBUG_DCBX
+ const char *istr = NULL;
+
+ insn = *((unsigned long *)regs->nip);
+ if (((insn >> (31-5)) & 0x3f) == 31) {
+ if (((insn >> 1) & 0x3ff) == 1014) /* dcbz ? 0x3f6 */
+ istr = "dcbz";
+ if (((insn >> 1) & 0x3ff) == 86) /* dcbf ? 0x56 */
+ istr = "dcbf";
+ if (((insn >> 1) & 0x3ff) == 470) /* dcbi ? 0x1d6 */
+ istr = "dcbi";
+ if (((insn >> 1) & 0x3ff) == 54) /* dcbst ? 0x36 */
+ istr = "dcbst";
+ if (((insn >> 1) & 0x3ff) == 982) /* icbi ? 0x3d6 */
+ istr = "icbi";
+ if (istr) {
+ ra = (insn >> (31-15)) & 0x1f; /* Reg RA */
+ rb = (insn >> (31-20)) & 0x1f; /* Reg RB */
+ dar = regs->gpr[rb];
+ if (ra)
+ dar += regs->gpr[ra];
+ if (dar != address && address != 0x00f0 && trap == 0x300)
+ printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar);
+ if (!strcmp(istr, "dcbst") && is_write) {
+ printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n",
+ ra, rb, dar);
+ is_write = 0;
+ }
+
+ if (trap == 0x300 && address != dar) {
+ __asm__ ("mtdar %0" : : "r" (dar));
+ return 0;
+ }
+ }
+ }
+#endif
+ if (address == 0x00f0 && trap == 0x300) {
+ pte_t *ptep;
+
+ /* This is from a dcbX or icbi insn gone bad, these
+ * insn do not set DAR so we have to do it here instead */
+ insn = *((unsigned long *)regs->nip);
+
+ ra = (insn >> (31-15)) & 0x1f; /* Reg RA */
+ rb = (insn >> (31-20)) & 0x1f; /* Reg RB */
+ dar = regs->gpr[rb];
+ if (ra)
+ dar += regs->gpr[ra];
+ /* Set DAR to correct address for the DTLB Miss/Error handler
+ * to redo the TLB exception. This time with correct address */
+ __asm__ ("mtdar %0" : : "r" (dar));
+#ifdef DEBUG_DCBX
+ printk(KERN_CRIT "trap:%x address:%lx, dar:%lx,err:%lx %s\n",
+ trap, address, dar, error_code, istr);
+#endif
+ address = dar;
+#if 1
+ if (is_write && get_pteptr(mm, dar, &ptep, NULL)) {
+ pte_t my_pte = *ptep;
+
+ if (pte_present(my_pte) && pte_write(my_pte)) {
+ pte_val(my_pte) |= _PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE;
+ set_pte_at(mm, dar, ptep, my_pte);
+ }
+ }
+#else
+ return 0;
+#endif
+ }
+ }
+#endif
if (notify_page_fault(regs))
return 0;
^ permalink raw reply related
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Benjamin Herrenschmidt @ 2009-10-02 22:04 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <20091002214949.GA20514@b07421-ec1.am.freescale.net>
On Fri, 2009-10-02 at 16:49 -0500, Scott Wood wrote:
> Adding a tlbil_va to do_page_fault makes the problem go away for me (on
> top of your "merge" branch) -- none of the other changes in this thread
> do (assuming I didn't miss any). FWIW, when it gets stuck on a fault,
> DSISR is 0xc0000000, and handle_mm_fault returns zero.
But in that case, it should hit ptep_set_access_flags() (via
handle_mm_fault) and from there call tlbil_va (provided we add a call to
it in the relevant filter function which I proposed initially), no ?
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
From: Scott Wood @ 2009-10-02 21:49 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <1254350159.5699.21.camel@pasglop>
On Thu, Oct 01, 2009 at 08:35:59AM +1000, Benjamin Herrenschmidt wrote:
> >From what I can see, the TLB miss code will check _PAGE_PRESENT, and
> when not set, it will -still- insert something into the TLB (unlike
> all other CPU types that go straight to data access faults from there).
>
> So we end up populating with those unpopulated entries that will then
> cause us to take a DSI (or ISI) instead of a TLB miss the next time
> around ... and so we need to remove them once we do that, no ? IE. Once
> we have actually put a valid PTE in.
>
> At least that's my understanding and why I believe we should always have
> tlbil_va() in set_pte() and ptep_set_access_flags(), which boils down
> in putting it into the 2 "filter" functions in the new code.
>
> Well.. actually, the ptep_set_access_flags() will already do a
> flush_tlb_page_nohash() when the PTE is changed. So I suppose all we
> really need here would be in set_pte_filter(), to do the same if we are
> writing a PTE that has _PAGE_PRESENT, at least on 8xx.
>
> But just unconditionally doing a tlbil_va() in both the filter functions
> shouldn't harm and also fix the problem, though Rex seems to indicate
> that is not the case.
Adding a tlbil_va to do_page_fault makes the problem go away for me (on
top of your "merge" branch) -- none of the other changes in this thread
do (assuming I didn't miss any). FWIW, when it gets stuck on a fault,
DSISR is 0xc0000000, and handle_mm_fault returns zero.
-Scott
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox