Re: [PATCH RFC v2 06/10] target/arm: add canonical and no-address tag logic

[Richard Henderson <richard.henderson@linaro.org>]

On 11/17/25 02:40, Gabriel Brookman wrote:
> This feature causes tag checks to compare logical address tags against
> their canonical form rather than against allocation tags. Described in
> the ARM ARM section "Logical Address Tagging".
> 
> Signed-off-by: Gabriel Brookman <brookmangabriel@gmail.com>
> ---
>   target/arm/internals.h      | 40 ++++++++++++++++++++++++++++++++++++++++
>   target/arm/tcg/mte_helper.c | 12 ++++++++++++
>   2 files changed, 52 insertions(+)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 75677945af..5f0bcdaaac 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1633,6 +1633,46 @@ static inline bool tcma_check(uint32_t desc, int bit55, int ptr_tag)
>       return tcma && match;
>   }
>   
> +/* Return whether or not the second nibble of a VA matches bit 55.  */
> +static inline bool tag_is_canonical(int ptr_tag, int bit55)
> +{
> +    return ((ptr_tag + bit55) & 0xf) == 0;
> +}
> +
> +/* Return true if mtx bits mean that the access is canonically checked.  */
> +static inline bool mtx_check(CPUARMState *env, bool bit55)
> +{
> +    /*
> +     * the MTX bits used in EL0 are those used in whichever EL is used
> +     * for the supervisor. The EL that contains the supervisor uses
> +     * bits 60 and 61 (MTX0 and MTX1), while the other ELs that aren't
> +     * used by the supervisor.
> +     */
> +    int el = arm_current_el(env);
> +    if (el == 0) {
> +        if (HCR_E2H & env->cp15.hcr_el2) {
> +            return (1l << (60 + bit55)) & env->cp15.tcr_el[2];
> +        } else {
> +            return (1l << (60 + bit55)) & env->cp15.tcr_el[1];
> +        }
> +    } else if (el == 1) {
> +        if (HCR_E2H & env->cp15.hcr_el2) {
> +            g_assert_not_reached();
> +        } else {
> +            return (1l << (60 + bit55)) & env->cp15.tcr_el[1];
> +        }
> +    } else if (el == 2) {
> +        if (HCR_E2H & env->cp15.hcr_el2) {
> +            return (1l << (60 + bit55)) & env->cp15.tcr_el[2];
> +        } else {
> +            return (1l << 33) & env->cp15.tcr_el[2];
> +        }
> +    } else if (el == 3) {
> +        return (1l << 33) & env->cp15.tcr_el[3];
> +    }
> +    return false;
> +}

Again, you can't touch hcr_el2 directly.
In this case, you want to use regime_tcr, using the mmu_idx value from mte_probe_int.

Use arm_mmu_idx_to_el() instead of arm_current_el(), so that UNPRIV acceses are handled 
correctly.

Using "l" is always wrong; always ll or preferably ull.
Though really defines for

   bit55 ? TCR_MTX1 : TCR_MTX0

wouldn't go amiss.

You seam to be missing a check for FEAT_MTE_NO_ADDRESS_TAGS or FEAT_MTE_CANONICAL_TAGS.

> +
>   /*
>    * For TBI, ideally, we would do nothing.  Proper behaviour on fault is
>    * for the tag to be present in the FAR_ELx register.  But for user-only
> diff --git a/target/arm/tcg/mte_helper.c b/target/arm/tcg/mte_helper.c
> index f9fd6fd408..513ee8d6a1 100644
> --- a/target/arm/tcg/mte_helper.c
> +++ b/target/arm/tcg/mte_helper.c
> @@ -799,6 +799,10 @@ static int mte_probe_int(CPUARMState *env, uint32_t desc, uint64_t ptr,
>           return 1;
>       }
>   
> +    if (mtx_check(env, bit55)) {
> +        return tag_is_canonical(ptr_tag, bit55);
> +    }

This could use a comment like

     If mtx is enabled, then the access is MemTag_CanonicallyTagged,
     otherwise it is MemTag_AllocationTagged.  See AArch64.CheckTag.

> +    if (mtx_check(env, bit55)) {
> +        if (tag_is_canonical(ptr_tag, bit55)) {
> +            goto done;
> +        }
> +        mte_check_fail(env, desc, ptr, ra);
> +    }

Likewise.

In the pseudocode, walkparams.mtx is used in AArch64.VAIsOutOfRange, which is part of 
get_phys_addr_lpae() here:

     /*
      * We determined the region when collecting the parameters, but we
      * have not yet validated that the address is valid for the region.
      * Extract the top bits and verify that they all match select.
      *
      * For aa32, if inputsize == addrsize, then we have selected the
      * region by exclusion in aa32_va_parameters and there is no more
      * validation to do here.
      */
     if (inputsize < addrsize) {
         uint64_t top_bits = sextract64(address, inputsize,
                                            addrsize - inputsize);
         if (-top_bits != param.select) {
             /* The gap between the two regions is a Translation fault */
             goto do_translation_fault;
         }
     }

There is also a use in AArch64.S1DisabledOutput, which would correspond to 
get_phys_addr_disabled(), which is relevant before

             if (extract64(address, pamax, addrtop - pamax + 1) != 0) {
                 fi->type = ARMFault_AddressSize;


r~