Re: [PATCH v1] PowerPC: Add support for Dense Math registers (RFC02653)

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

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?

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.


>@@ -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.


>+  <!-- 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" ?


>@@ -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.


>+  /* 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?


>+    /* 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.


>+  /* 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?


Bye,
Ulrich