Re: [PATCH v4 07/16] [gdbserver/aarch64] sme: Add support for SME

[Luis Machado <luis.machado@arm.com>]

On 8/27/23 02:32, Thiago Jung Bauermann wrote:
> 
> Hello Luis,
> 
> After reviewing the SME2 patches, I spotted this suggestion that
> I didn't notice when I reviewed v3. Sorry about that again.
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> +/* Wrapper for aarch64_za_regs_copy_to_reg_buf, to help copying NT_ARM_ZA
>> +   state from the thread (BUF) to the register cache.  */
>> +
>> +static void
>> +aarch64_za_regs_copy_to_regcache (struct regcache *regcache,
>> +				  ATTRIBUTE_UNUSED const void *buf)
>> +{
>> +  /* BUF is unused here since we collect the data straight from a ptrace
>> +     request, therefore bypassing gdbserver's own call to ptrace.  */
>> +  int tid = lwpid_of (current_thread);
>> +
>> +  gdb::optional<int> za_regnum
>> +    = find_regno_no_throw (regcache->tdesc, "za");
>> +  gdb::optional<int> svg_regnum
>> +    = find_regno_no_throw (regcache->tdesc, "svg");
>> +  gdb::optional<int> svcr_regnum
>> +    = find_regno_no_throw (regcache->tdesc, "svcr");
>> +
>> +  gdb_assert (za_regnum.has_value ());
>> +  gdb_assert (svg_regnum.has_value ());
>> +  gdb_assert (svcr_regnum.has_value ());
> 
> By changing to find_regno() then this gdb_assert() becomes unnecessary.
>

Well spotted. I've changed both of these now to call find_regno.

>> +
>> +  /* Update the register cache.  aarch64_za_regs_copy_to_reg_buf handles
>> +     fetching the NT_ARM_ZA state from thread TID.  */
>> +  aarch64_za_regs_copy_to_reg_buf (tid, regcache, *za_regnum, *svg_regnum,
>> +				   *svcr_regnum);
>> +}
>> +
>> +/* Wrapper for aarch64_za_regs_copy_from_reg_buf, to help copying NT_ARM_ZA
>> +   state from the register cache to the thread (BUF).  */
>> +
>> +static void
>> +aarch64_za_regs_copy_from_regcache (struct regcache *regcache, void *buf)
>> +{
>> +  int tid = lwpid_of (current_thread);
>> +
>> +  gdb::optional<int> za_regnum
>> +    = find_regno_no_throw (regcache->tdesc, "za");
>> +  gdb::optional<int> svg_regnum
>> +    = find_regno_no_throw (regcache->tdesc, "svg");
>> +  gdb::optional<int> svcr_regnum
>> +    = find_regno_no_throw (regcache->tdesc, "svcr");
>> +
>> +  gdb_assert (za_regnum.has_value ());
>> +  gdb_assert (svg_regnum.has_value ());
>> +  gdb_assert (svcr_regnum.has_value ());
> 
> Same comment about find_regno().
> 
>> +
>> +  /* Update the thread NT_ARM_ZA state.  aarch64_za_regs_copy_from_reg_buf
>> +     handles writing the ZA state back to thread TID.  */
>> +  aarch64_za_regs_copy_from_reg_buf (tid, regcache, *za_regnum, *svg_regnum,
>> +				     *svcr_regnum);
>> +
>> +  /* We need to return the expected data in BUF, so copy whatever the kernel
>> +     already has to BUF.  */
>> +
>> +  /* Obtain a dump of ZA from ptrace.  */
>> +  gdb::byte_vector za_state = aarch64_fetch_za_regset (tid);
>> +  memcpy (buf, za_state.data (), za_state.size ());
>> +}
> 

Thanks for the reviews!