[2/4] gdbstub: Add support for MTE in user mode

This commit implements the stubs to handle the qIsAddressTagged,
qMemTag, and QMemTag GDB packets, allowing all GDB 'memory-tag'
subcommands to work with QEMU gdbstub on aarch64 user mode. It also
implements the get/set function for the special GDB MTE register
'tag_ctl', used to control the MTE fault type at runtime.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 configs/targets/aarch64-linux-user.mak |   2 +-
 target/arm/cpu.c                       |   1 +
 target/arm/gdbstub.c                   | 321 +++++++++++++++++++++++++
 target/arm/internals.h                 |   2 +
 4 files changed, 325 insertions(+), 1 deletion(-)

Hi Gustavo,

On 15/5/24 19:31, Gustavo Romero wrote:
> This commit implements the stubs to handle the qIsAddressTagged,
> qMemTag, and QMemTag GDB packets, allowing all GDB 'memory-tag'
> subcommands to work with QEMU gdbstub on aarch64 user mode. It also
> implements the get/set function for the special GDB MTE register
> 'tag_ctl', used to control the MTE fault type at runtime.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>   configs/targets/aarch64-linux-user.mak |   2 +-
>   target/arm/cpu.c                       |   1 +
>   target/arm/gdbstub.c                   | 321 +++++++++++++++++++++++++
>   target/arm/internals.h                 |   2 +
>   4 files changed, 325 insertions(+), 1 deletion(-)


> +void arm_cpu_register_gdb_commands(ARMCPU *cpu)
> +{
> +    GArray *gdb_gen_query_table_arm =
> +        g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
> +    GArray *gdb_gen_set_table_arm =
> +        g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
> +    GString *supported_features = g_string_new(NULL);
> +
> +#ifdef TARGET_AARCH64
> +#ifdef CONFIG_USER_ONLY
> +    /* MTE */
> +    if (isar_feature_aa64_mte(&cpu->isar)) {

Can we keep this code generic (not guarded by #ifdef'ry)? We
are protected by this isar_feature_aa64_mte() call to register
the MTE feature.

> +        g_string_append(supported_features, ";memory-tagging+");
> +
> +        add_packet_handler(gdb_gen_query_table_arm, qMemTags);
> +        add_packet_handler(gdb_gen_query_table_arm, qIsAddressTagged);
> +
> +        add_packet_handler(gdb_gen_set_table_arm, QMemTags);
> +    }
> +#endif
> +#endif
> +
> +    /* Set arch-specific handlers for 'q' commands. */
> +    if (gdb_gen_query_table_arm->len) {
> +        set_gdb_gen_query_table_arch(&g_array_index(gdb_gen_query_table_arm,
> +                                                    GdbCmdParseEntry, 0),
> +                                                    gdb_gen_query_table_arm->len);
> +    }
> +
> +    /* Set arch-specific handlers for 'Q' commands. */
> +    if (gdb_gen_set_table_arm->len) {
> +        set_gdb_gen_set_table_arch(&g_array_index(gdb_gen_set_table_arm,
> +                                   GdbCmdParseEntry, 0),
> +                                   gdb_gen_set_table_arm->len);
> +    }
> +
> +    /* Set arch-specific qSupported feature. */
> +    if (supported_features->len) {
> +        set_query_supported_arch(supported_features->str);
> +    }
> +}

On 5/15/24 19:31, Gustavo Romero wrote:
> +static int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    assert(reg == 0);
> +
> +    /* Sanitize TCF0 bits. */
> +    *buf &= 0x03;
> +
> +    if (!isar_feature_aa64_mte3(&cpu->isar) && *buf == 3) {

cpu_isar_feature(aa64_mte3, cpu)


> +        /*
> +         * If FEAT_MTE3 is not implemented, the value 0b11 is reserved, hence
> +         * ignore setting it.
> +         */
> +        return 0;

That said, we always implement the mte3 behaviour, so perhaps drop this entirely?

> +    }
> +
> +    /*
> +     * 'tag_ctl' register is actually a "pseudo-register" provided by GDB to
> +     * expose options that can be controlled at runtime and has the same effect
> +     * of prctl() with option PR_SET_TAGGED_ADDR_CTRL,
> +     * i.e. prctl(PR_SET_TAGGED_ADDR_CTRL, tcf, 0, 0, 0), hence it controls
> +     * the effect of Tag Check Faults (TCF) due to Loads and Stores in EL0.
> +     */
> +    env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, *buf);
> +
> +    return 1;
> +}
> +
> +static void handle_q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
> +{
> +    uint64_t addr = get_param(params, 0)->val_ull;
> +    uint64_t len = get_param(params, 1)->val_ul;
> +    int type = get_param(params, 2)->val_ul;
> +
> +    uint64_t clean_addr;
> +    uint8_t *tags;
> +    int granules_index;
> +    int granule_index;
> +    uint8_t addr_tag;
> +
> +    g_autoptr(GString) str_buf = g_string_new(NULL);
> +
> +    /*
> +     * GDB does not query tags for a memory range on remote targets, so that's
> +     * not supported either by gdbstub.
> +     */
> +    if (len != 1) {
> +        gdb_put_packet("E02");
> +    }
> +
> +    /* GDB never queries a tag different from an allocation tag (type 1). */
> +    if (type != 1) {
> +        gdb_put_packet("E02");
> +    }
> +
> +    /* Remove any non-addressing bits. */
> +    clean_addr = useronly_clean_ptr(addr);
> +
> +    /*
> +     * Get pointer to all tags in the page where the address is. Note that tags
> +     * are packed, so there are 2 tags packed in one byte.
> +     */
> +    tags = page_get_target_data(clean_addr);

While you expect gdb will have called isaddresstagged first, you can't guarantee that:
you should verify that the address is valid and tagged first.

> +static void handle_q_isaddresstagged(GArray *params, G_GNUC_UNUSED void *user_ctx)
> +{
> +    uint64_t addr = get_param(params, 0)->val_ull;
> +
> +    uint64_t clean_addr;
> +    int mflags;
> +
> +    g_autoptr(GString) str_buf = g_string_new(NULL);
> +
> +    /* Remove any non-addressing bits. */
> +    clean_addr = useronly_clean_ptr(addr);
> +
> +    mflags = page_get_flags(clean_addr);
> +    if (mflags & PAGE_ANON && mflags & PAGE_MTE) {
> +        /* Address is tagged. */
> +        g_string_printf(str_buf, "%.2x", 1 /* true */);
> +    } else {
> +        /* Address is not tagged. */
> +        g_string_printf(str_buf, "%.2x", 0 /* false */);
> +    }
> +
> +    gdb_put_packet(str_buf->str);

Overkill with GString?

     const char *result = (mflags & PAGE_ANON && mflags & PAGE_MTE ? "1" : "0");
     gdb_put_packet(result);

?

> +}
> +
> +static void handle_Q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
> +{
> +    uint64_t addr = get_param(params, 0)->val_ull;
> +    uint64_t len = get_param(params, 1)->val_ul;
> +    int type = get_param(params, 2)->val_ul;
> +    char const *new_tags = get_param(params, 3)->data;
> +
> +    uint64_t clean_addr;
> +    int last_addr_index;
> +
> +    uint64_t start_addr_page;
> +    uint64_t end_addr_page;
> +
> +    uint32_t first_tag_index;
> +    uint32_t last_tag_index;
> +
> +    uint8_t *tags; /* Pointer to the current tags in a page. */
> +    int num_new_tags;
> +
> +    g_autoptr(GString) str_buf = g_string_new(NULL);
> +
> +    /*
> +     * Only the allocation tag (type 1) can be set at the stub side.
> +     */
> +    if (type != 1) {
> +        gdb_put_packet("E02");
> +        return;
> +    }
> +
> +    /*
> +     * 'len' is always >= 1 and refers to the size of the memory range about to
> +     * have its tags updated. However, it's convenient to obtain the index for
> +     * the last byte of the memory range for page boundary checks and for
> +     * obtaining the indexes for the tags in the page.
> +     */
> +    last_addr_index = len - 1;
> +
> +    /* Remove any non-addressing bits. */
> +    clean_addr = useronly_clean_ptr(addr);
> +
> +    start_addr_page = extract64(clean_addr, TARGET_PAGE_BITS,
> +                                64 - TARGET_PAGE_BITS);
> +    end_addr_page = extract64(clean_addr + last_addr_index, TARGET_PAGE_BITS,
> +                              64 - TARGET_PAGE_BITS);
> +
> +    /*
> +     * Check if memory range is within page boundaries.
> +     */
> +    if (start_addr_page != end_addr_page) {
> +        gdb_put_packet("E03");
> +        return;
> +    }
> +
> +    /*
> +     * Get pointer to all tags in the page where the address is. Note that here
> +     * tags are packed, so there are 2 tags packed in one byte.
> +     */
> +    tags = page_get_target_data(clean_addr);

Likewise.

> +
> +    /* Tag indices below refer to unpacked tags. */
> +    first_tag_index = extract32(clean_addr, LOG2_TAG_GRANULE,
> +                                TARGET_PAGE_BITS - LOG2_TAG_GRANULE);
> +    last_tag_index = extract32(clean_addr + last_addr_index, LOG2_TAG_GRANULE,
> +                               TARGET_PAGE_BITS - LOG2_TAG_GRANULE);
> +
> +    /*
> +     * GDB sends 2 hex digits per tag number, i.e. tags are not represented in
> +     * a packed way.
> +     */
> +    num_new_tags = strlen(new_tags) / 2;
> +
> +    /*
> +     * If the number of tags provided is greater than the number of tags
> +     * in the provided memory range, the exceeding tags are ignored. If the
> +     * number of tags is less than the number of tags in the provided memory
> +     * range, then the provided tags are used as a repeating pattern to fill
> +     * the tags in the provided memory range.
> +     */
> +    for (int i = first_tag_index, j = 0; i <= last_tag_index; i++, j++) {
> +        int new_tag_value;
> +        int packed_granules_index;
> +        int nibble_index;
> +
> +        sscanf(new_tags + 2 * (j % num_new_tags), "%2x", &new_tag_value);

Overkill?
While gdb may send 2 digits, only 0[0-9a-fA-F] is valid.


> +        /*
> +         * Find packed tag index from unpacked tag index. There are two tags
> +         * packed in one packed index. One tag per nibble.
> +         */
> +        packed_granules_index = i / 2;
> +        /* Find nibble index in the packed tag from unpacked tag index. */
> +        nibble_index = i % 2;
> +
> +        if (nibble_index == 0) { /* Update low nibble */
> +            *(tags + packed_granules_index) &= 0xF0;
> +            *(tags + packed_granules_index) |= (new_tag_value & 0x0F);
> +        } else { /* Update high nibble */
> +            *(tags + packed_granules_index) &= 0x0F;
> +            *(tags + packed_granules_index) |= ((new_tag_value & 0x0F) << 4);
> +        }

How many tags will gdb typically send with this?
If 1 or 2, it might be worth using memset.
If even, it might be worth pre-computing and using memcpy.


r~

On 5/15/24 19:31, Gustavo Romero wrote:
> +    /* Remove any non-addressing bits. */
> +    clean_addr = useronly_clean_ptr(addr);
> +
> +    /*
> +     * Get pointer to all tags in the page where the address is. Note that tags
> +     * are packed, so there are 2 tags packed in one byte.
> +     */
> +    tags = page_get_target_data(clean_addr);
> +
> +    /*
> +     * Tags are per granule (16 bytes). 2 tags (4 bits each) are kept in a
> +     * single byte for compactness, so first a page tag index for 2 packed
> +     * granule tags (1 byte) is found, and then an index for a single granule
> +     * tag (nibble) is found, and finally the address tag is obtained.
> +     */
> +    granules_index = extract32(clean_addr, LOG2_TAG_GRANULE + 1,
> +                               TARGET_PAGE_BITS - LOG2_TAG_GRANULE - 1);
> +    granule_index = extract32(clean_addr, LOG2_TAG_GRANULE, 1);
> +
> +    addr_tag = *(tags + granules_index);
> +    /* Extract tag from the right nibble. */
> +    if (granule_index == 0) {
> +        addr_tag &= 0xF;
> +    } else {
> +        addr_tag >>= 4;
> +    }
> +

I think I would prefer the body of all three of these gdb commands to be split out into 
separate functions.  I think they should use use allocation_tag_mem_probe, load_tag1, 
store_tag1 from mte_helper.c.  I am undecided as to whether the gdb helpers should be 
placed in mte_helper.c, or if the existing mte_helper.c functions should be exported.


r~

Gustavo Romero <gustavo.romero@linaro.org> writes:

> This commit implements the stubs to handle the qIsAddressTagged,
> qMemTag, and QMemTag GDB packets, allowing all GDB 'memory-tag'
> subcommands to work with QEMU gdbstub on aarch64 user mode. It also
> implements the get/set function for the special GDB MTE register
> 'tag_ctl', used to control the MTE fault type at runtime.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  configs/targets/aarch64-linux-user.mak |   2 +-
>  target/arm/cpu.c                       |   1 +
>  target/arm/gdbstub.c                   | 321 +++++++++++++++++++++++++
>  target/arm/internals.h                 |   2 +
>  4 files changed, 325 insertions(+), 1 deletion(-)
>
> diff --git a/configs/targets/aarch64-linux-user.mak b/configs/targets/aarch64-linux-user.mak
> index ba8bc5fe3f..8f0ed21d76 100644
> --- a/configs/targets/aarch64-linux-user.mak
> +++ b/configs/targets/aarch64-linux-user.mak
> @@ -1,6 +1,6 @@
>  TARGET_ARCH=aarch64
>  TARGET_BASE_ARCH=arm
> -TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/aarch64-pauth.xml
> +TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/aarch64-pauth.xml gdb-xml/aarch64-mte.xml

Ahh there it is, this got missed from the commit