[PATCH v1] [powerpc] Remove 512 bytes region limit if using 2nd DAWR.

[PATCH v1] [powerpc] Remove 512 bytes region limit if using 2nd DAWR.

[Rogerio Alves via Gdb-patches]

Power 10 introduces the 2nd DAWR (second watchpoint) and also removed
a restriction that limit the watch region to 512 bytes.

2020-11-08  Rogerio A. Cardoso  <rcardoso@linux.ibm.com>

gdb/

* ppc-linux-nat.c: (PPC_DEBUG_FEATURE_DATA_BP_ARCH_31): New define.
(region_ok_for_hw_watchpoint): Check if 2nd DAWR is avaliable before set
region.

---
 gdb/ppc-linux-nat.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 7131134c10..643400eac5 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -138,6 +138,11 @@ struct ppc_hw_breakpoint
 #define PPC_DEBUG_FEATURE_DATA_BP_DAWR	0x10
 #endif /* PPC_DEBUG_FEATURE_DATA_BP_DAWR */
 
+/* Feature defined on Linux kernel v5.1: Second watchpoint support.  */
+#ifndef PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
+#define PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 0x20
+#endif /* PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 */
+
 /* The version of the PowerPC HWDEBUG kernel interface that we will use, if
    available.  */
 #define PPC_DEBUG_CURRENT_VERSION 1
@@ -2111,7 +2116,6 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
       int region_size;
       const struct ppc_debug_info &hwdebug_info = (m_dreg_interface
 						   .hwdebug_info ());
-
       /* Embedded DAC-based processors, like the PowerPC 440 have ranged
 	 watchpoints and can watch any access within an arbitrary memory
 	 region. This is useful to watch arrays and structs, for instance.  It
@@ -2121,9 +2125,11 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
 	  && linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
 	return 2;
       /* Check if the processor provides DAWR interface.  */
-      if (hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_DAWR)
+      if (hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_DAWR
+	  && !(hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_ARCH_31))
 	/* DAWR interface allows to watch up to 512 byte wide ranges which
-	   can't cross a 512 byte boundary.  */
+	   can't cross a 512 byte boundary unless it uses a second DARW
+	   ISA 3.1 (P10) which has no such restriction.  */
 	region_size = 512;
       else
 	region_size = hwdebug_info.data_bp_alignment;
-- 
2.17.1

Re: [PATCH v1] [powerpc] Remove 512 bytes region limit if using 2nd DAWR.

[Ulrich Weigand via Gdb-patches]

On Sun, Nov 08, 2020 at 11:12:04PM -0300, Rogerio Alves wrote:
> Power 10 introduces the 2nd DAWR (second watchpoint) and also removed
> a restriction that limit the watch region to 512 bytes.
> 
> 2020-11-08  Rogerio A. Cardoso  <rcardoso@linux.ibm.com>
> 
> gdb/
> 
> * ppc-linux-nat.c: (PPC_DEBUG_FEATURE_DATA_BP_ARCH_31): New define.
> (region_ok_for_hw_watchpoint): Check if 2nd DAWR is avaliable before set
> region.

This doesn't look correct, the patch not only removes the alignment
requirement but actually removes support for 512 bytes ranges itself!

I think for P10 we need to continue to allow watched ranges up to
512 bytes in size, but they only need to be 8 byte aligned now and
may cross a 512-byte boundary.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH v1] [powerpc] Remove 512 bytes region limit if using 2nd DAWR.

[Rogerio Alves via Gdb-patches]

On 11/9/20 7:24 AM, Ulrich Weigand wrote:
> On Sun, Nov 08, 2020 at 11:12:04PM -0300, Rogerio Alves wrote:
>> Power 10 introduces the 2nd DAWR (second watchpoint) and also removed
>> a restriction that limit the watch region to 512 bytes.
>>
>> 2020-11-08  Rogerio A. Cardoso  <rcardoso@linux.ibm.com>
>>
>> gdb/
>>
>> * ppc-linux-nat.c: (PPC_DEBUG_FEATURE_DATA_BP_ARCH_31): New define.
>> (region_ok_for_hw_watchpoint): Check if 2nd DAWR is avaliable before set
>> region.
> 
> This doesn't look correct, the patch not only removes the alignment
> requirement but actually removes support for 512 bytes ranges itself!
> 

The patch doesn't remove the support. If we are using the second 
watchpoint (PPC_DEBUG_FEATURE_DATA_BP_ARCH_31) we don't limit the 
region_size to 512 (region_size = 512) instead we use 
hwdebug_info.data_bp_alignment right? Isn't region_size that create a 
512-byte bondary?

> I think for P10 we need to continue to allow watched ranges up to
> 512 bytes in size, but they only need to be 8 byte aligned now and
> may cross a 512-byte boundary.
> 

Since the else case is region_size = hwdebug_info.data_bp_alignment;
I guess this is what it do unless I am missing something here.
> Bye,
> Ulrich
> 

Re: [PATCH v1] [powerpc] Remove 512 bytes region limit if using 2nd DAWR.

[Ulrich Weigand via Gdb-patches]

On Mon, Nov 09, 2020 at 10:08:09AM -0300, Rogerio Alves wrote:
> On 11/9/20 7:24 AM, Ulrich Weigand wrote:
> > On Sun, Nov 08, 2020 at 11:12:04PM -0300, Rogerio Alves wrote:
> >> Power 10 introduces the 2nd DAWR (second watchpoint) and also removed
> >> a restriction that limit the watch region to 512 bytes.
> >>
> >> 2020-11-08  Rogerio A. Cardoso  <rcardoso@linux.ibm.com>
> >>
> >> gdb/
> >>
> >> * ppc-linux-nat.c: (PPC_DEBUG_FEATURE_DATA_BP_ARCH_31): New define.
> >> (region_ok_for_hw_watchpoint): Check if 2nd DAWR is avaliable before set
> >> region.
> > 
> > This doesn't look correct, the patch not only removes the alignment
> > requirement but actually removes support for 512 bytes ranges itself!
> > 
> 
> The patch doesn't remove the support. If we are using the second 
> watchpoint (PPC_DEBUG_FEATURE_DATA_BP_ARCH_31) we don't limit the 
> region_size to 512 (region_size = 512) instead we use 
> hwdebug_info.data_bp_alignment right? Isn't region_size that create a 
> 512-byte bondary?

hwdebug_info.data_bp_alignment is the required *alignment*, which
remains 8 for Power10.

I think the problem is that until now, the maximum size was always
equal to the required alignment (on Power8: alignment 8, size 8;
on Power9: alignment 512, size 512), but on Power10 they are now
different: alignment 8, size 512.

> > I think for P10 we need to continue to allow watched ranges up to
> > 512 bytes in size, but they only need to be 8 byte aligned now and
> > may cross a 512-byte boundary.
> > 
> 
> Since the else case is region_size = hwdebug_info.data_bp_alignment;
> I guess this is what it do unless I am missing something here.

We'll probably have to use two different variables for size and
alignment now, update the test accordingly, and set the values as
above.

Given the size and alignment value, and given a range start address,
the maximum allowable end can be found by rounding the start address
*down* to the required alignment, and then adding the maximum size.

For example, if we have a start address of 0x1234, then the maximum
end address on P9 is 0x1400, but on P10 it would be 0x1430.

(With your patch as-is, it would reset to the P8 values, where the
maximum end address is only 0x1238.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com