Re: [PATCH 3/4] target/arm: Support reading ZA[] from gdbstub

[Luis Machado]

On 6/27/23 14:07, Peter Maydell wrote:
> On Thu, 22 Jun 2023 at 16:12, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Mirror the existing support for SVE.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
>
>> @@ -247,6 +247,61 @@ int aarch64_gdb_set_pauth_reg(CPUARMState *env, uint8_t 
>> *buf, int reg)
>>      return 0;
>>  }
>>
>> +static int max_svq(ARMCPU *cpu)
>> +{
>> +    return 32 - clz32(cpu->sme_vq.map);
>> +}
>> +
>> +int aarch64_gdb_get_za_reg(CPUARMState *env, GByteArray *buf, int reg)
>> +{
>> +    ARMCPU *cpu = env_archcpu(env);
>> +    int max_vq = max_svq(cpu);
>> +    int cur_vq = EX_TBFLAG_A64(env->hflags, SVL) + 1;
>> +    int i;
>> +
>> +    if (reg >= max_vq * 16) {
>> +        return 0;
>> +    }
>> +
>> +    /* If ZA is unset, or reg out of range, the contents are zero. */
>> +    if (FIELD_EX64(env->svcr, SVCR, ZA) && reg < cur_vq * 16) {
>> +        for (i = 0; i < cur_vq; i++) {
>> +            gdb_get_reg128(buf, env->zarray[reg].d[i * 2 + 1],
>> +                           env->zarray[reg].d[i * 2]);
>> +        }
>> +    } else {
>> +        cur_vq = 0;
>> +    }
>> +
>> +    for (i = cur_vq; i < max_vq; i++) {
>> +        gdb_get_reg128(buf, 0, 0);
>> +    }
>> +
>> +    return max_vq * 16;
>> +}
>> +
>> +int aarch64_gdb_set_za_reg(CPUARMState *env, uint8_t *buf, int reg)
>> +{
>> +    ARMCPU *cpu = env_archcpu(env);
>> +    uint64_t *p = (uint64_t *) buf;
>> +    int max_vq = max_svq(cpu);
>> +    int cur_vq = EX_TBFLAG_A64(env->hflags, SVL) + 1;
>> +    int i;
>> +
>> +    if (reg >= max_vq * 16) {
>> +        return 0;
>> +    }
>> +
>> +    /* If ZA is unset, or reg out of range, the contents are zero. */
>> +    if (FIELD_EX64(env->svcr, SVCR, ZA) && reg < cur_vq * 16) {
>> +        for (i = 0; i < cur_vq; i++) {
>> +            env->zarray[reg].d[i * 2 + 1] = *p++;
>> +            env->zarray[reg].d[i * 2 + 0] = *p++;
>
> This looks like it won't do the right thing on a big-endian
> system. (And the existing SVE code also looks wrong.)
> The gdb_get_reg*() functions handle endianness conversion
> from the gdb data buffer; there are no equivalent gdb_set_reg*()
> functions so you have to do the byte-swapping yourself.
> (This is pretty bug-prone so maybe we should design a better
> API here :-))
>
> Compare aarch64_gdb_get/set_fpu_reg() where a gdb_get_reg128()
> is matched with a pair of ldq_le_p() and so on.
>
>> +        }
>> +    }
>> +    return max_vq * 16;
>> +}
>> +
>>  static void output_vector_union_type(GString *s, int reg_width,
>>                                       const char *name)
>>  {
>> @@ -379,3 +434,36 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int 
>> orig_base_reg)
>>      info->num = base_reg - orig_base_reg;
>>      return info->num;
>>  }
>> +
>> +/*
>> + * Generate the xml for SME, with matrix size set to the maximum
>> + * for the cpu.  Returns the number of registers generated.
>> + */
>> +int arm_gen_dynamic_zareg_xml(CPUState *cs, int base_reg)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    GString *s = g_string_new(NULL);
>> +    int vq = max_svq(cpu);
>> +    int row_count = vq * 16;
>> +    int row_width = vq * 128;
>> +    int i;
>> +
>> +    g_string_printf(s, "<?xml version=\"1.0\"?>");
>> +    g_string_append_printf(s, "<!DOCTYPE target SYSTEM 
>> \"gdb-target.dtd\">");
>> +    g_string_append_printf(s, "<feature name=\"org.qemu.gdb.aarch64.za\">");

Thanks for cc-ing me in the thread Peter.

>
> The patches on the GDB end are still under review, but they
> use the feature name org.gnu.gdb.aarch64.sme:
>
> https://inbox.sourceware.org/gdb-patches/20230519102508.14020-18-luis.machado@arm.com/
> We should follow that (and only commit our end when the GDB
> spec for the XML layout is finalized.
>
> Luis kindly gave me a dump of some example XML to save us
> from trying to parse it out of the patch:
>
>   <feature name="org.gnu.gdb.aarch64.sme">
>     <flags id="svcr_flags" size="8">
>       <field name="SM" start="0" end="0" type="bool"/>
>       <field name="ZA" start="1" end="1" type="bool"/>
>     </flags>
>     <vector id="sme_bv" type="uint8" count="32"/>
>     <vector id="sme_bvv" type="sme_bv" count="32"/>

Just to clarify, for convenience I've defined ZA as a 2-dimensional array of 
bytes. That way gdb can do things like:

(gdb) p $za
$1 = {{0 <repeats 32 times>} <repeats 32 times>}

Or you can access a particular row or col as needed.

Here SVL is 32 bytes. So the final size of ZA is 1024 (8192 bits).

GDB will also take care of providing the numerous pseudo-registers that 
read/write to portions of ZA.

>     <reg name="svg" bitsize="64" type="int" regnum="91"/>

SVG is just like VG in SVE, but for SME. It is SVL / 8.

>     <reg name="svcr" bitsize="64" type="svcr_flags" regnum="92"/>

SVCR tracks the SM and ZA bits, which QEMU must provide. I haven't decided if 
we want to make that read-only or read/write. I'm tempted to make it read-only.

I haven't done any testing of bare metal ZA support yet. Please let me know 
what you see.

>     <reg name="za" bitsize="8192" type="sme_bvv" regnum="93"/>
>   </feature>
>
>> +
>> +    output_vector_union_type(s, row_width, "zav");
>> +
>> +    for (i = 0; i < row_count; i++) {
>> +        g_string_append_printf(s,
>> +                               "<reg name=\"za%d\" bitsize=\"%d\""
>> +                               " regnum=\"%d\" type=\"zav\"/>",
>> +                               i, row_width, base_reg + i);
>> +    }
>> +
>> +    g_string_append_printf(s, "</feature>");
>> +
>> +    cpu->dyn_zareg_xml.num = row_count;
>> +    cpu->dyn_zareg_xml.desc = g_string_free(s, false);
>> +    return row_count;
>> +}
>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.