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

[Richard Henderson]

On 2/7/19 8:47 AM, Peter Maydell wrote:
> 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.

That's changed since the first rev.  Now ChooseNonExcludedTag just has an
exclude argument, and the caller includes GCR_EL1.Exclude.

>> +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.

All of this has changed since the first rev too.

> 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 for the verbage.


r~