Hi Ulrich,

Thanks very much for the review. Responses below.

Abhay Kandpal <abhay@linux.ibm.com> wrote:

A new target description (powerpc-dmr-vsx32l/64l) is introduced for
systems where DMR registers are available. The existing isa207 target
description is left unchanged so that systems without DMR support are
not affected. DMR availability is detected at runtime by probing the
kernel with PTRACE_GETDMREGS. On kernels without DMR support the
registers are gracefully skipped and show as unavailable.

This looks good in general.  One question about naming: most recent
features have names that identify the base Power ISA level (isa205,
isa207, etc.).   Should we follow this precedent for the new target
description name?

The existing names encode the base ISA level, but DMR is a later ISA
feature layered on an isa207-derived description, so isa207-dmr would 
misstate the ISA level. A few options:

1. powerpc-isa207-dmr-vsx64l - names the base feature set the description extends
2. powerpc-isa32-dmr-vsx64l - names DMR's own ISA level (ISA 3.2)
3. powerpc-dmr-vsx64l - as posted, no ISA level

A few other comments below.

   if (features.wordsize == 8)
     {
+      /* HTM was disabled on POWER9 and later hardware following the
+	 transactional-memory erratum, so it does not coexist with the
+	 Dense Math facility introduced on later processors.  The htm
+	 and has_dmr cases below are therefore mutually exclusive in
+	 practice.  */
       if (features.vsx)
 	tdesc = (features.htm ? tdesc_powerpc_isa207_htm_vsx64l
+		 : features.has_dmr ? tdesc_powerpc_dmr_vsx64l

Even so, it would be preferable to add the newest feature first.

Will do. The comment then becomes unnecessary.

@@ -58,6 +59,7 @@ struct ppc_linux_features
   bool ppr_dscr;
   bool isa207;
   bool htm;
+  bool has_dmr;

For this struct, precedent is not to use the "has_" prefix.

Will rename to dmr.

+  <!-- Define one 1024-bit vector composed of eight 128-bit lanes -->
+  <vector id="uint1024" type="uint128" count="8"/>

"uint1024" is a surprising name for a vector type.  I would have
expected something like "v8uint128" ?

Agreed. I'll rename it and regenerate the description files.

@@ -159,6 +159,11 @@
 #define NT_PPC_TM_CDSCR 0x10f
 #endif
 
+#ifndef PTRACE_GETDMREGS
+#define PTRACE_GETDMREGS 0x1f
+#define PTRACE_SETDMREGS 0x20
+#endif

Please move this block higher up, together with the
other PTRACE_ defines in this file.

That said, it would have been preferable to use the
regset mechanism like for all other recently added
registers, rather than a completely new PTRACE_ call.
I see in the gdbserver you actually do that!  So please
use the same method in gdb itself as well.

The dedicated calls were used on the native side because
that is what the kernel team pointed tooling at for DMR,
but I agree the inconsistency between gdb and gdbserver 
isn't justified. In the kernel, PTRACE_GETDMREGS is a thin
wrapper around copy_regset_to_user() on the same regset that
NT_PPC_DMR reaches, and I've confirmed PTRACE_GETREGSET with
NT_PPC_DMR works on the enabled kernel.

So in v2 I'll convert the native side to the regset mechanism
throughout: fetch_regset/store_regset with NT_PPC_DMR in
fetch_register, fetch_ppc_registers, store_register and
store_ppc_registers, and a regset-based availability check in
read_description. That removes the need for the 
PTRACE_GETDMREGS/PTRACE_SETDMREGS defines, so I'll drop the 
block rather than move it - unless something still needs them,
in which case I'll relocate it as you suggest.

+  /* Check whether the kernel supports Dense Math registers.  Native
+     GDB accesses DMR through the dedicated PTRACE_GETDMREGS

request.  */

+  {
+    gdb_byte buf[PPC_LINUX_SIZEOF_DMRREGSET];
+    if (ptrace (PTRACE_GETDMREGS, tid, 0, buf) >= 0)
+      features.has_dmr = true;
+  }

Again it would be preferable to use a regset check instead.

Is there a HWCAP bit we should check in addition, to indicate that
the kernel and hypervisor support context-swapping these registers?

Yes - PPC_FEATURE2_DMF (0x00008000, Dense Math Facility). I'll add that
check. The kernel does save and restore these registers across context 
switches. The hwcap bit and the DMR ptrace support are currently in 
separate kernel branches here, so I'm assembling a tree with both before
validating v2.

+    /* Dense Math registers.  */
+    int have_dmr = 0;
+    /* Register number of dmr0, or -1 if DMR is not available.  Set
+       from PPC_DMR0_REGNUM when the target provides the dmr feature.
+       Provided for register-offset computations (e.g. core file and
+       pseudo-register handling) added in follow-up DMR work.  */
+    int ppc_dmr0_regnum = 0;

It seems odd to have *both* a have_ flag and a regnum.
Usually, where we have regnum field, all tests are written
as "if (tdep->ppc_dmr0_regnum != -1)" instead.

Agreed. I'll drop have_dmr and test tdep->ppc_dmr0_regnum != -1 instead.

+  /* Check whether the kernel supports Dense Math registers.  Native
+     GDB accesses DMR through the dedicated PTRACE_GETDMREGS

request.  */

+  {
+    gdb_byte buf[PPC_LINUX_SIZEOF_DMRREGSET];
+    if (ptrace (PTRACE_GETDMREGS, tid, 0, buf) >= 0)
+      features.has_dmr = true;
+  }

Why this mix of new PTRACE_ and regsets?

Covered above - I'll move the native side to regsets so there's no mix.

Regards,
Abhay

Bye,
Ulrich