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

[Abhay Kandpal <abhay@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 5779 bytes --]

Hi Ulrich,

Thanks very much for the review. Responses below.

On 25/09/26 17:35, Ulrich Weigand wrote:
> 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

[-- Attachment #2: Type: text/html, Size: 9508 bytes --]