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~
