Re: [Qemu-devel] [PATCH 06/17] target/arm: Implement the IRG instruction

[Peter Maydell]

On Mon, 14 Jan 2019 at 01:11, Richard Henderson
<address@hidden> wrote:
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/helper-a64.h    |  1 +
>  target/arm/mte_helper.c    | 55 ++++++++++++++++++++++++++++++++++++++
>  target/arm/translate-a64.c |  7 +++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
> index fa4c371a47..7a6051fdab 100644
> --- a/target/arm/helper-a64.h
> +++ b/target/arm/helper-a64.h
> @@ -104,3 +104,4 @@ DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, 
> i64)
>  DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
>
>  DEF_HELPER_FLAGS_2(mte_check, TCG_CALL_NO_WG, i64, env, i64)
> +DEF_HELPER_FLAGS_3(irg, TCG_CALL_NO_RWG, i64, env, i64, i64)
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> index 6f4bc0aa04..1878393fc4 100644
> --- a/target/arm/mte_helper.c
> +++ b/target/arm/mte_helper.c
> @@ -36,6 +36,48 @@ static int allocation_tag_from_addr(uint64_t ptr)
>      return (extract64(ptr, 56, 4) + extract64(ptr, 55, 1)) & 15;
>  }
>
> +/* Like ChooseNonExcludedTag, except that GCR_EL1 is already in.  */

I don't understand this comment -- neither the pseudocode
function nor this code refer to GCR_EL1.

> +static int choose_nonexcluded_tag(int tag, int offset, uint16_t exclude)
> +{
> +    if (exclude != 0xffff) {
> +        int i;
> +        for (i = 0; i < offset; ++i) {
> +            do {
> +                tag = (tag + 1) & 15;
> +            } while (exclude & (1 << tag));
> +        }
> +    }
> +    return tag;

This doesn't seem to do the same thing as ChooseNonExcludedTag()
for the offset==0 case, or for the exclude == 0xffff case.

> +}
> +
> +static int choose_random_nonexcluded_tag(CPUARMState *env, uint16_t exclude)
> +{
> +    /* Ignore GCR_EL1.RRND.  Always produce deterministic results.  */

This comment is trying to say that our IMPDEF choice for GCR_EL1.RRND==1
is "behave the same as if RRND==0", right? I think that would be
clearer as a comment at the callsite, because if you're following
along with the pseudocode you expect the IRG code to do an
"if RRND == 1 then { choose_random_nonexcluded_tag(); } else { ... }".

thanks
-- PMM