Re: [Qemu-devel] [PATCH v3 0/4] target-arm: Handle tagged addresses when loading PC

[Peter Maydell]

On 12 October 2016 at 20:50, Thomas Hanson <address@hidden> wrote:
> If tagged addresses are enabled, then addresses being loaded into the
> PC must be cleaned up by overwriting the tag bits with either all 0's
> or all 1's as specified in the ARM ARM spec.  The decision process is
> dependent on whether the code will be running in EL0/1 or in EL2/3 and
> is controlled by a combination of Top Byte Ignored (TBI) bits in the
> TCR and the value of bit 55 in the address being loaded.
>
> TBI values are extracted from the appropriate TCR and made available
> to TCG code generation routines by inserting them into the TB flags
> field and then transferring them to DisasContext structure in
> gen_intermediate_code_a64().
>
> New function gen_a64_set_pc_var() encapsulates the logic required to
> determine whether clean up of the tag byte is required and then
> generating the code to correctly load the PC. New function
> gen_a64_set_pc_reg() accepts a register number and then calls
> gen_a64_set_pc_var().
>
> In addition to those instruction which can directly load a tagged
> address into the PC, there are others which increment or add a value to
> the PC.  If 56 bit addressing is used, these instructions can cause an
> arithmetic roll-over into the tag bits.  The ARM ARM specification for
> handling tagged addresses requires that these cases also be addressed
> by cleaning up the tag field.  This work has been deferred because
> there is currently no CPU model available for testing with 56 bit
> addresses.

Thanks for these patches. I have applied 1, 2 and 4 to target-arm.next,
with the following minor changes:
 * in patch 1:
   + drop unnecessary 'extern' keyword on function prototypes
   + provide "always returns 0" inline versions of arm_regime_tbi0()
     and arm_regime_tbi1() [the patch doesn't build for linux-user
     without these]
 * in patch 2:
   + remove the unnecessary gen_a64_set_pc_reg() wrapper, and
     rename gen_a64_set_pc_var() to get_a64_set_pc()
   + fix a stray misindented line (not sure why checkpatch misses
     it, but it's not infallible...)

Regarding patch 3: I realized while reading it that we actually
have all the info we need to correctly fix up the tag bit at
translate time (since we know the PC, the offset, the TBI bits
and the current EL). So I'm happy that we just write some code
now to do this (what I was unhappy about before was generating
code that didn't need to do anything while addresses are less
than 56 bits, but slightly unnecessary work at translate time
is much less worrying). So I think that rather than adding
comments we could have a function like:

/* Return the address of the target of a relative branch
 * to pc + offset, accounting for possible overflow into
 * the tag bits if necessary.
 * @offset should be the offset from the branch instruction;
 * this function takes care of subtracting 4 to account for
 * s->pc being ahead of the instruction being translated.
 */
static uint64_t rel_branch_target(DisasContext *s, int64_t offset)
{
   /* You can either just have the comment here about doing
    * something if virtual addresses are 56 bits, or you can
    * actually do the necessary fixup; your choice.
    */
   return s->pc - 4 + offset;
}

and then use it in the 3 places you currently have a comment.
(Note that we can also hide that irritating -4 in here.)

Sorry for not realising earlier that we could fix stuff up
at translate time.

thanks
-- PMM