Re: [PATCH v5 04/22] target/arm: Add helper_mte_check{1,2,3}

From: Richard Henderson <richard.henderson@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm <qemu-arm@nongnu.org>, QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH v5 04/22] target/arm: Add helper_mte_check{1,2,3}
Date: Tue, 3 Dec 2019 08:14:26 -0800	[thread overview]
Message-ID: <120a00d4-a33d-6263-c6d4-4671449d7dab@linaro.org> (raw)
In-Reply-To: <CAFEAcA_WOMbFzFQghakLxxuCTc2b3p6eb18nS5KSrSj+oW-Ckg@mail.gmail.com>

Oh, to finish up on the replies...

On 12/3/19 1:42 PM, Peter Maydell wrote:
>> +    ptr_tag = allocation_tag_from_addr(dirty_ptr);
>> +    if (ptr_tag == 0) {
>> +        ARMVAParameters p = aa64_va_parameters(env, dirty_ptr, stage1, true);
>> +        if (p.tcma) {
>> +            return clean_ptr;
>> +        }
>> +    }
> 
> I don't think this logic gets the "regime has two address ranges"
> case correct. For a two-address-range translation regime (where
> TCR_ELx has TCMA0 and TCMA1 bits, rather than just a single TCMA bit),
> then the 'select' argument to this function needs to be involved,
> because we should do a tag-unchecked access if:
>  * addr[59:55]==0b00000 (ie select == 0 and ptr_tag == 0)
>    and TCR_ELx.TCMA0 is set
>  * addr[59:55]==0b11111 (ie select == 1 and ptr_tag == 0xf)
>    and TCR_ELx.TCMA1 is set
> (the pseudocode for this is in AArch64.AccessTagIsChecked(),
> and the TCR_EL1.TCMA[01] bit definitions agree; the text in
> D6.8.1 appears to be confused.)

It used to be correct.

That was the lovely bit about physical vs logical tags.  Add 1 bit bit 55, let
the carry ripple up, so that the physical tag check for TCMA was always against 0.

That seems to be broken now in the final spec.

>> +        el = arm_current_el(env);
>> +        regime_el = (el ? el : 1);   /* TODO: ARMv8.1-VHE EL2&0 regime */
> 
> We could write this as "regime_el(env, stage1)" if that function
> wasn't local to helper.c, right ?

Yes.

>> +            /* noreturn; fall through to assert anyway */
> 
> hopefully this fallthrough comment syntax doesn't confuse any
> of our compilers/static analyzers...

It shouldn't...

>> +            /* Tag check fail causes asynchronous flag set.  */
>> +            env->cp15.tfsr_el[regime_el] |= 1 << select;
> 
> Won't this incorrectly accumulate tagfails for EL0 into
> TFSR_EL1 rather than TFSRE0_EL1 ? I think you want "[el]".

Yep.

>> +            /* Case 3: Reserved. */
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "Tag check failure with SCTLR_EL%d.TCF "
>> +                          "set to reserved value %d\n", regime_el, tcf);
> 
> Technically this message is going to be wrong for the
> case of el==0 (where it's SCTLR_EL1.TCF0, not .TCF, that's
> been mis-set).

Yep.

r~