[PATCH 0/2] Fix ptrace software breakpoints

[PATCH 0/2] Fix ptrace software breakpoints

[Will Deacon]

When using GDB on a quad-core Cortex-A9 (Versatile Express) board, software
breakpoints do not work if the inferior is scheduled onto a different CPU from
the debugger.

When GDB changes the code of another context via the ptrace POKETEXT mechanism,
the I-cache must be invalidated before the inferior is allowed to resume. If a
copy-on-write is triggered by the copy_to_user_page function, the new page
mappings must be used by the inferior in order to pick up the new instructions.

This patch series addresses this problem by:

(a) Providing a workaround for a known TLB issue on some revisions of the Cortex-A9.
(b) Performing correct I-cache invalidation in the flush_ptrace_access code.

All feedback/comments/tested-bys welcome.

Cc: Rob Clark <rob@ti.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>

Will Deacon (2):
  ARM: errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a
    faulty ASID
  ARM: flush_ptrace_access: invalidate all I-caches

 arch/arm/Kconfig                |   12 ++++++++++++
 arch/arm/include/asm/tlbflush.h |    8 ++++++++
 arch/arm/mm/flush.c             |    4 ++--
 3 files changed, 22 insertions(+), 2 deletions(-)

[PATCH 1/2] ARM: errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a faulty ASID

[Will Deacon]

On versions of the Cortex-A9 prior to r2p0, performing TLB invalidations by
ASID match can result in the incorrect ASID being broadcast to other CPUs.
As a consequence of this, the targetted TLB entries are not invalidated
across the system.

This workaround changes the TLB flushing routines to invalidate entries
regardless of the ASID.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/Kconfig                |   12 ++++++++++++
 arch/arm/include/asm/tlbflush.h |    8 ++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 98922f7..4824fb4 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1027,6 +1027,18 @@ config PL310_ERRATA_588369
 	   is not correctly implemented in PL310 as clean lines are not
 	   invalidated as a result of these operations. Note that this errata
 	   uses Texas Instrument's secure monitor api.
+
+config ARM_ERRATA_720789
+	bool "ARM errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a faulty ASID"
+	depends on CPU_V7 && SMP
+	help
+	  This option enables the workaround for the 720789 Cortex-A9 (prior to
+	  r2p0) erratum. A faulty ASID can be sent to the other CPUs for the
+	  broadcasted CP15 TLB maintenance operations TLBIASIDIS and TLBIMVAIS.
+	  As a consequence of this erratum, some TLB entries which should be
+	  invalidated are not, resulting in an incoherency in the system page
+	  tables. The workaround changes the TLB flushing routines to invalidate
+	  entries regardless of the ASID.
 endmenu
 
 source "arch/arm/common/Kconfig"
diff --git a/arch/arm/include/asm/tlbflush.h b/arch/arm/include/asm/tlbflush.h
index bd863d8..33b546a 100644
--- a/arch/arm/include/asm/tlbflush.h
+++ b/arch/arm/include/asm/tlbflush.h
@@ -378,7 +378,11 @@ static inline void local_flush_tlb_mm(struct mm_struct *mm)
 	if (tlb_flag(TLB_V6_I_ASID))
 		asm("mcr p15, 0, %0, c8, c5, 2" : : "r" (asid) : "cc");
 	if (tlb_flag(TLB_V7_UIS_ASID))
+#ifdef CONFIG_ARM_ERRATA_720789
+		asm("mcr p15, 0, %0, c8, c3, 0" : : "r" (zero) : "cc");
+#else
 		asm("mcr p15, 0, %0, c8, c3, 2" : : "r" (asid) : "cc");
+#endif
 
 	if (tlb_flag(TLB_BTB)) {
 		/* flush the branch target cache */
@@ -424,7 +428,11 @@ local_flush_tlb_page(struct vm_area_struct *vma, unsigned long uaddr)
 	if (tlb_flag(TLB_V6_I_PAGE))
 		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (uaddr) : "cc");
 	if (tlb_flag(TLB_V7_UIS_PAGE))
+#ifdef CONFIG_ARM_ERRATA_720789
+		asm("mcr p15, 0, %0, c8, c3, 3" : : "r" (uaddr & PAGE_MASK) : "cc");
+#else
 		asm("mcr p15, 0, %0, c8, c3, 1" : : "r" (uaddr) : "cc");
+#endif
 
 	if (tlb_flag(TLB_BTB)) {
 		/* flush the branch target cache */
-- 
1.6.3.3

[PATCH 2/2] ARM: flush_ptrace_access: invalidate all I-caches

[Will Deacon]

copy_to_user_page can be used by access_process_vm to write to an
executable page of a process using a mapping acquired by kmap.
For systems with I-cache aliasing, flushing the I-cache using the
Kernel mapping may leave stale data in the I-cache if the user
mapping is of a different colour.

This patch replaces the coherent_kern_range call in flush_ptrace_access
with a D-cache flush followed by a system-wide I-cache invalidation.
This is required on all systems where the size of a way in the I-cache
is larger than PAGE_SIZE.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/mm/flush.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index c6844cb..45896a9 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -120,8 +120,8 @@ void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 
 	/* VIPT non-aliasing cache */
 	if (vma->vm_flags & VM_EXEC) {
-		unsigned long addr = (unsigned long)kaddr;
-		__cpuc_coherent_kern_range(addr, addr + len);
+		__cpuc_flush_dcache_area(kaddr, len);
+		__flush_icache_all();
 #ifdef CONFIG_SMP
 		if (cache_ops_need_broadcast())
 			smp_call_function(flush_ptrace_access_other,
-- 
1.6.3.3

[PATCH 1/2] ARM: errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a faulty ASID

[Shilimkar, Santosh]

Will,
> -----Original Message-----
> From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-arm-
> kernel-bounces at lists.infradead.org] On Behalf Of Will Deacon
> Sent: Thursday, July 15, 2010 9:24 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Will Deacon
> Subject: [PATCH 1/2] ARM: errata: TLBIASIDIS and TLBIMVAIS operations can
> broadcast a faulty ASID
> 
> On versions of the Cortex-A9 prior to r2p0, performing TLB invalidations
> by
> ASID match can result in the incorrect ASID being broadcast to other CPUs.
> As a consequence of this, the targetted TLB entries are not invalidated
> across the system.
> 
> This workaround changes the TLB flushing routines to invalidate entries
> regardless of the ASID.
> 
Just a curious question. How costly is this ?

> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/Kconfig                |   12 ++++++++++++
>  arch/arm/include/asm/tlbflush.h |    8 ++++++++
>  2 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 98922f7..4824fb4 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1027,6 +1027,18 @@ config PL310_ERRATA_588369
>  	   is not correctly implemented in PL310 as clean lines are not
>  	   invalidated as a result of these operations. Note that this
> errata
>  	   uses Texas Instrument's secure monitor api.
> +
> +config ARM_ERRATA_720789
> +	bool "ARM errata: TLBIASIDIS and TLBIMVAIS operations can broadcast
> a faulty ASID"
> +	depends on CPU_V7 && SMP
> +	help
> +	  This option enables the workaround for the 720789 Cortex-A9 (prior
> to
> +	  r2p0) erratum. A faulty ASID can be sent to the other CPUs for the
> +	  broadcasted CP15 TLB maintenance operations TLBIASIDIS and
> TLBIMVAIS.
> +	  As a consequence of this erratum, some TLB entries which should be
> +	  invalidated are not, resulting in an incoherency in the system
> page
> +	  tables. The workaround changes the TLB flushing routines to
> invalidate
> +	  entries regardless of the ASID.
>  endmenu
> 
>  source "arch/arm/common/Kconfig"
> diff --git a/arch/arm/include/asm/tlbflush.h
> b/arch/arm/include/asm/tlbflush.h
> index bd863d8..33b546a 100644
> --- a/arch/arm/include/asm/tlbflush.h
> +++ b/arch/arm/include/asm/tlbflush.h
> @@ -378,7 +378,11 @@ static inline void local_flush_tlb_mm(struct
> mm_struct *mm)
>  	if (tlb_flag(TLB_V6_I_ASID))
>  		asm("mcr p15, 0, %0, c8, c5, 2" : : "r" (asid) : "cc");
>  	if (tlb_flag(TLB_V7_UIS_ASID))
> +#ifdef CONFIG_ARM_ERRATA_720789
> +		asm("mcr p15, 0, %0, c8, c3, 0" : : "r" (zero) : "cc");
> +#else
>  		asm("mcr p15, 0, %0, c8, c3, 2" : : "r" (asid) : "cc");
> +#endif
> 
>  	if (tlb_flag(TLB_BTB)) {
>  		/* flush the branch target cache */
> @@ -424,7 +428,11 @@ local_flush_tlb_page(struct vm_area_struct *vma,
> unsigned long uaddr)
>  	if (tlb_flag(TLB_V6_I_PAGE))
>  		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (uaddr) : "cc");
>  	if (tlb_flag(TLB_V7_UIS_PAGE))
> +#ifdef CONFIG_ARM_ERRATA_720789
> +		asm("mcr p15, 0, %0, c8, c3, 3" : : "r" (uaddr & PAGE_MASK) :
> "cc");
> +#else
>  		asm("mcr p15, 0, %0, c8, c3, 1" : : "r" (uaddr) : "cc");
> +#endif
> 
>  	if (tlb_flag(TLB_BTB)) {
>  		/* flush the branch target cache */
> --
> 1.6.3.3
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] ARM: errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a faulty ASID

[Will Deacon]

Hi Santosh,

> > Subject: [PATCH 1/2] ARM: errata: TLBIASIDIS and TLBIMVAIS operations can
> > broadcast a faulty ASID
> >
> > On versions of the Cortex-A9 prior to r2p0, performing TLB invalidations
> > by
> > ASID match can result in the incorrect ASID being broadcast to other CPUs.
> > As a consequence of this, the targetted TLB entries are not invalidated
> > across the system.
> >
> > This workaround changes the TLB flushing routines to invalidate entries
> > regardless of the ASID.
> >
> Just a curious question. How costly is this ?
 
I think the cost really depends on whether or not the evicted TLB entries
are available in the D-cache. If they are, then a TLB miss occurring because
of this workaround will hit in the data cache and the overhead will be small.
Missing in the D-cache is certainly going to add an overhead, but that depends
on your memory system.

Of course, this workaround should only be enabled on platforms that suffer
from the erratum, where it's worth sacrificing a few cycles in order to get
a working virtual memory system!

Will

[PATCH 2/2] ARM: flush_ptrace_access: invalidate all I-caches

[Russell King - ARM Linux]

On Thu, Jul 15, 2010 at 04:53:58PM +0100, Will Deacon wrote:
> copy_to_user_page can be used by access_process_vm to write to an
> executable page of a process using a mapping acquired by kmap.
> For systems with I-cache aliasing, flushing the I-cache using the
> Kernel mapping may leave stale data in the I-cache if the user
> mapping is of a different colour.
> 
> This patch replaces the coherent_kern_range call in flush_ptrace_access
> with a D-cache flush followed by a system-wide I-cache invalidation.
> This is required on all systems where the size of a way in the I-cache
> is larger than PAGE_SIZE.
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/mm/flush.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> index c6844cb..45896a9 100644
> --- a/arch/arm/mm/flush.c
> +++ b/arch/arm/mm/flush.c
> @@ -120,8 +120,8 @@ void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
>  
>  	/* VIPT non-aliasing cache */
>  	if (vma->vm_flags & VM_EXEC) {
> -		unsigned long addr = (unsigned long)kaddr;
> -		__cpuc_coherent_kern_range(addr, addr + len);
> +		__cpuc_flush_dcache_area(kaddr, len);
> +		__flush_icache_all();

NAK.

If we have aliases in the I-cache, there's probably more places that
need to be fixed - and in any case I think the VIPT aliasing case
should be used in that instance.

This code is for non-aliasing D and I caches, and works as follows.

1. We flush the data out of the D cache, line by line, on at least the
   local CPU (and optionally the other CPUs.)  Since we disable
   preemption, we will own the cache lines.

2. We invalidate the I cache, line by line, on at least local CPU
   (and optionally the other CPUs.)

3. If the I-cache invalidate wasn't broadcast, we flush the entire
   I-cache on the other CPUs.

So, what CPUs report themselves as having VIPT non-aliasing caches but
actually have an aliasing I-cache?

[PATCH 2/2] ARM: flush_ptrace_access: invalidate all I-caches

[Will Deacon]

Hi Russell,

> > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> > index c6844cb..45896a9 100644
> > --- a/arch/arm/mm/flush.c
> > +++ b/arch/arm/mm/flush.c
> > @@ -120,8 +120,8 @@ void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
> >
> >  	/* VIPT non-aliasing cache */
> >  	if (vma->vm_flags & VM_EXEC) {
> > -		unsigned long addr = (unsigned long)kaddr;
> > -		__cpuc_coherent_kern_range(addr, addr + len);
> > +		__cpuc_flush_dcache_area(kaddr, len);
> > +		__flush_icache_all();
> 
> NAK.
>
> If we have aliases in the I-cache, there's probably more places that
> need to be fixed - and in any case I think the VIPT aliasing case
> should be used in that instance.
> 
> This code is for non-aliasing D and I caches, and works as follows.
> 
> 1. We flush the data out of the D cache, line by line, on at least the
>    local CPU (and optionally the other CPUs.)  Since we disable
>    preemption, we will own the cache lines.
> 
> 2. We invalidate the I cache, line by line, on at least local CPU
>    (and optionally the other CPUs.)

(1) and (2) are done in __cpuc_coherent_kern_range. The problem is
that the (2) part doesn't deal with aliases and only invalidates for
the given MVA.
 
> 3. If the I-cache invalidate wasn't broadcast, we flush the entire
>    I-cache on the other CPUs.

Yep.

> So, what CPUs report themselves as having VIPT non-aliasing caches but
> actually have an aliasing I-cache?

As far as I understand, all ARM cores have aliasing I-caches if it's a
VIPT cache with way size greater than the page size. The D-cache has
additional logic for handling aliasing which is what the VIPT non-aliasing
characteristics refer to.

Will

[PATCH 1/2] ARM: errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a faulty ASID

[Shilimkar, Santosh]

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Thursday, July 15, 2010 10:02 PM
> To: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org
> Subject: RE: [PATCH 1/2] ARM: errata: TLBIASIDIS and TLBIMVAIS operations
> can broadcast a faulty ASID
> 
> Hi Santosh,
> 
> > > Subject: [PATCH 1/2] ARM: errata: TLBIASIDIS and TLBIMVAIS operations
> can
> > > broadcast a faulty ASID
> > >
> > > On versions of the Cortex-A9 prior to r2p0, performing TLB
> invalidations
> > > by
> > > ASID match can result in the incorrect ASID being broadcast to other
> CPUs.
> > > As a consequence of this, the targetted TLB entries are not
> invalidated
> > > across the system.
> > >
> > > This workaround changes the TLB flushing routines to invalidate
> entries
> > > regardless of the ASID.
> > >
> > Just a curious question. How costly is this ?
> 
> I think the cost really depends on whether or not the evicted TLB entries
> are available in the D-cache. If they are, then a TLB miss occurring
> because
> of this workaround will hit in the data cache and the overhead will be
> small.
> Missing in the D-cache is certainly going to add an overhead, but that
> depends
> on your memory system.
> 
> Of course, this workaround should only be enabled on platforms that suffer
> from the erratum, where it's worth sacrificing a few cycles in order to
> get
> a working virtual memory system!

Thanks for quick response!!

Regards
Santosh

[PATCH 2/2] ARM: flush_ptrace_access: invalidate all I-caches

[Catalin Marinas]

On Thu, 2010-07-15 at 17:43 +0100, Will Deacon wrote:
> Russell King wrote:
> > So, what CPUs report themselves as having VIPT non-aliasing caches but
> > actually have an aliasing I-cache?
> 
> As far as I understand, all ARM cores have aliasing I-caches if it's a
> VIPT cache with way size greater than the page size. The D-cache has
> additional logic for handling aliasing which is what the VIPT non-aliasing
> characteristics refer to.

Just to confirm what Will said (and I checked this in the past with our
lead architect) - if the CTR reports VIPT I-cache, it is a true VIPT
I-cache with potential aliases if the way size is greater than the page
size (e.g. on vexpress, C-A9 has 32K I-cache and 4 ways).

On the D side, the VIPT on ARMv7 is always non-aliasing (e.g. it behaves
like PIPT).

Anyway, it's not that bad since most places where this matters use
__flush_icache_all(). This includes my patches for cache maintenance via
set_pte_at().

-- 
Catalin

[PATCH 0/2] Fix ptrace software breakpoints

[Rob Clark]

fyi, my gdb test (git://github.com/robclark/gdb-test.git) seems to be passing reliable so far (I haven't seen it fail once yet) with these patches..  without these patches, it was triggering passing the breakpoing 90+% of the time.

I'll keep you posted if I notice any issues, but so far so good!

thx

BR,
-R

On Jul 15, 2010, at 10:53 AM, Will Deacon wrote:

> When using GDB on a quad-core Cortex-A9 (Versatile Express) board, software
> breakpoints do not work if the inferior is scheduled onto a different CPU from
> the debugger.
> 
> When GDB changes the code of another context via the ptrace POKETEXT mechanism,
> the I-cache must be invalidated before the inferior is allowed to resume. If a
> copy-on-write is triggered by the copy_to_user_page function, the new page
> mappings must be used by the inferior in order to pick up the new instructions.
> 
> This patch series addresses this problem by:
> 
> (a) Providing a workaround for a known TLB issue on some revisions of the Cortex-A9.
> (b) Performing correct I-cache invalidation in the flush_ptrace_access code.
> 
> All feedback/comments/tested-bys welcome.
> 
> Cc: Rob Clark <rob@ti.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> 
> Will Deacon (2):
>  ARM: errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a
>    faulty ASID
>  ARM: flush_ptrace_access: invalidate all I-caches
> 
> arch/arm/Kconfig                |   12 ++++++++++++
> arch/arm/include/asm/tlbflush.h |    8 ++++++++
> arch/arm/mm/flush.c             |    4 ++--
> 3 files changed, 22 insertions(+), 2 deletions(-)

[PATCH 2/2] ARM: flush_ptrace_access: invalidate all I-caches

[Will Deacon]

Russell,

> > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> > > index c6844cb..45896a9 100644
> > > --- a/arch/arm/mm/flush.c
> > > +++ b/arch/arm/mm/flush.c
> > > @@ -120,8 +120,8 @@ void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
> > >
> > >  	/* VIPT non-aliasing cache */
> > >  	if (vma->vm_flags & VM_EXEC) {
> > > -		unsigned long addr = (unsigned long)kaddr;
> > > -		__cpuc_coherent_kern_range(addr, addr + len);
> > > +		__cpuc_flush_dcache_area(kaddr, len);
> > > +		__flush_icache_all();
> >
> > NAK.
> >
> > If we have aliases in the I-cache, there's probably more places that
> > need to be fixed - and in any case I think the VIPT aliasing case
> > should be used in that instance.

So I looked into this problem a bit more in order to confirm it's an aliasing
issue.

GDB sets breakpoints inside ld-linux.so so that it can control the loading of
shared libraries and insert any pending breakpoints.

If the inferior has a PID of 3233, we can see the dynamic loader living at
0x40000000:

root at will-lucid-nfs:~/tmp# cat /proc/3233/maps
...
40000000-40016000 r-xp 00000000 00:0b 3493469    /lib/ld-2.11.1.so
40016000-40017000 rw-p 00000000 00:00 0 
4001d000-4001f000 rw-p 00015000 00:0b 3493469    /lib/ld-2.11.1.so

GDB tries to insert a breakpoint on __dl_debug_state, at address 0x40002590.
Some examples of when this works (I hacked flush_ptrace_access to print uaddr
and dst):

[  110.910000] writing to user address 0x40002590 using Kernel mapping 0xbfe6c590
...
[  147.530000] writing to user address 0x40002590 using Kernel mapping 0xbfe7e590

Now, this can fail with a SIGILL:

(gdb) start
Temporary breakpoint 1 at 0x83b4: file test.c, line 7.
Starting program: /root/tmp/test 

Program received signal SIGILL, Illegal instruction.
0x40002590 in ?? () from /lib/ld-linux.so.3

This happens when GDB has removed a breakpoint instruction, but it still exists
in the I-cache because the mapping used to invalidate the relevant way was of a
different colour. In dmesg, we see:

[  150.780000] writing to user address 0x40002590 using Kernel mapping 0xbfe8b590

The I-cache in use is a 4-way 32KB cache, so we need 13 bits to represent an
index into an 8KB way and a byte index into the resulting line. Since the top
bit is not invariant to address translation, we have two colours.

Looking at the GDB examples:

0x40002590 and 0xbfe6c590 have bit 12 == 0, so they are the same colour.
0x40002590 and 0xbfe7e590 have bit 12 == 0, so they are the same colour.
0x40002590 and 0xbfe8b590 have bit 12 == 0 and bit 12 == 1 respectively.

In the last case, the wrong colour is invalidated in the I-cache and a SIGILL
occurs. This problem can be fixed either by invalidating the whole I-cache
(which is what is done in the patch I posted) or we could invalidate the
correct colour, but this would mean implementing something like kmap_coherent
from the MIPS world.

It would be good to get this fixed. Do you still object to my original patch or
can I submit it to the patch system if I get some tested-bys?

Thanks,

Will

[PATCH 0/2] Fix ptrace software breakpoints

[Will Deacon]

Hi Rob,

> fyi, my gdb test (git://github.com/robclark/gdb-test.git) seems to be passing reliable so far (I
> haven't seen it fail once yet) with these patches..  without these patches, it was triggering passing
> the breakpoing 90+% of the time.
> 
> I'll keep you posted if I notice any issues, but so far so good!

Does this mean you haven't seen it fail yet :)? If so, any
chance I could have a tested-by please?

Will

[PATCH 0/2] Fix ptrace software breakpoints

[Rob Clark]

On 07/15/2010 10:53 AM, Will Deacon wrote:
> When using GDB on a quad-core Cortex-A9 (Versatile Express) board, software
> breakpoints do not work if the inferior is scheduled onto a different CPU from
> the debugger.
>
> When GDB changes the code of another context via the ptrace POKETEXT mechanism,
> the I-cache must be invalidated before the inferior is allowed to resume. If a
> copy-on-write is triggered by the copy_to_user_page function, the new page
> mappings must be used by the inferior in order to pick up the new instructions.
>
> This patch series addresses this problem by:
>
> (a) Providing a workaround for a known TLB issue on some revisions of the Cortex-A9.
> (b) Performing correct I-cache invalidation in the flush_ptrace_access code.
>
> All feedback/comments/tested-bys welcome.
>
> Cc: Rob Clark<rob@ti.com>
> Cc: Catalin Marinas<catalin.marinas@arm.com>
>
> Will Deacon (2):
>    ARM: errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a
>      faulty ASID
>    ARM: flush_ptrace_access: invalidate all I-caches
>
>   arch/arm/Kconfig                |   12 ++++++++++++
>   arch/arm/include/asm/tlbflush.h |    8 ++++++++
>   arch/arm/mm/flush.c             |    4 ++--
>   3 files changed, 22 insertions(+), 2 deletions(-)
>    

Tested on omap4430 (cortex-a9 SMP).. with this patchset I have yet to 
see it fail, in regular use nor in the test case I wrote to show the 
problem (git://github.com/robclark/gdb-test.git).  Without these 
patches, I'd see SIGILL and missed breakpoints 90+% of the time in 
regular use of gdb (basically gdb was unusable unless I booted with 
nosmp or disabled cache), and 100% of the time in my gdb-test.

Tested-by: Rob Clark<rob@ti.com>

[PATCH 0/2] Fix ptrace software breakpoints

[Will Deacon]

Hi Rob,

> > Will Deacon (2):
> >    ARM: errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a
> >      faulty ASID
> >    ARM: flush_ptrace_access: invalidate all I-caches
> >
> >   arch/arm/Kconfig                |   12 ++++++++++++
> >   arch/arm/include/asm/tlbflush.h |    8 ++++++++
> >   arch/arm/mm/flush.c             |    4 ++--
> >   3 files changed, 22 insertions(+), 2 deletions(-)
> >
> 
> Tested on omap4430 (cortex-a9 SMP).. with this patchset I have yet to
> see it fail, in regular use nor in the test case I wrote to show the
> problem (git://github.com/robclark/gdb-test.git).  Without these
> patches, I'd see SIGILL and missed breakpoints 90+% of the time in
> regular use of gdb (basically gdb was unusable unless I booted with
> nosmp or disabled cache), and 100% of the time in my gdb-test.
> 
> Tested-by: Rob Clark<rob@ti.com>

Thanks for testing this. I've seen the same results on my Versatile
Express board running Ubuntu Lucid.

Russell - can I submit these to the patch system please or do you have
any outstanding concerns?

Cheers,

Will

 

[PATCH 2/2] ARM: flush_ptrace_access: invalidate all I-caches

[Will Deacon]

Hi Russell,

> Subject: Re: [PATCH 2/2] ARM: flush_ptrace_access: invalidate all I-caches
> 
> On Thu, Jul 15, 2010 at 04:53:58PM +0100, Will Deacon wrote:
> > copy_to_user_page can be used by access_process_vm to write to an
> > executable page of a process using a mapping acquired by kmap.
> > For systems with I-cache aliasing, flushing the I-cache using the
> > Kernel mapping may leave stale data in the I-cache if the user
> > mapping is of a different colour.
> >
> > This patch replaces the coherent_kern_range call in flush_ptrace_access
> > with a D-cache flush followed by a system-wide I-cache invalidation.
> > This is required on all systems where the size of a way in the I-cache
> > is larger than PAGE_SIZE.
> >
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm/mm/flush.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> > index c6844cb..45896a9 100644
> > --- a/arch/arm/mm/flush.c
> > +++ b/arch/arm/mm/flush.c
> > @@ -120,8 +120,8 @@ void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
> >
> >  	/* VIPT non-aliasing cache */
> >  	if (vma->vm_flags & VM_EXEC) {
> > -		unsigned long addr = (unsigned long)kaddr;
> > -		__cpuc_coherent_kern_range(addr, addr + len);
> > +		__cpuc_flush_dcache_area(kaddr, len);
> > +		__flush_icache_all();
> 
> NAK.
> 

How about something like this instead?:

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index c6844cb..6b03043 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -20,10 +20,9 @@
 
 #include "mm.h"
 
-#ifdef CONFIG_CPU_CACHE_VIPT
-
 #define ALIAS_FLUSH_START      0xffff4000
 
+#ifdef CONFIG_CPU_CACHE_VIPT
 static void flush_pfn_alias(unsigned long pfn, unsigned long vaddr)
 {
        unsigned long to = ALIAS_FLUSH_START + (CACHE_COLOUR(vaddr) << PAGE_SHIFT);
@@ -93,6 +92,18 @@ void flush_cache_page(struct vm_area_struct *vma, unsigned long user_addr, unsig
 #define flush_pfn_alias(pfn,vaddr)     do { } while (0)
 #endif
 
+static void flush_icache_alias(unsigned long pfn, unsigned long vaddr, unsigned long len)
+{
+       unsigned long colour = CACHE_COLOUR(vaddr);
+       unsigned long offset = vaddr & (PAGE_SIZE - 1);
+       unsigned long to;
+
+       set_pte_ext(TOP_PTE(ALIAS_FLUSH_START) + colour, pfn_pte(pfn, PAGE_KERNEL), 0);
+       to = ALIAS_FLUSH_START + (colour << PAGE_SHIFT) + offset;
+       flush_tlb_kernel_page(to);
+       flush_icache_range(to, to + len);
+}
+
 #ifdef CONFIG_SMP
 static void flush_ptrace_access_other(void *args)
 {
@@ -120,8 +131,7 @@ void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 
        /* VIPT non-aliasing cache */
        if (vma->vm_flags & VM_EXEC) {
-               unsigned long addr = (unsigned long)kaddr;
-               __cpuc_coherent_kern_range(addr, addr + len);
+               flush_icache_alias(page_to_pfn(page), uaddr, len);
 #ifdef CONFIG_SMP
                if (cache_ops_need_broadcast())
                        smp_call_function(flush_ptrace_access_other,

That way we only use __flush_icache_all for non-aliasing D-caches in
the case that we have to broadcast the flushing using IPI.

Will