Re: [Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses

[Peter Maydell]

On 16 September 2016 at 10:34, Thomas Hanson <address@hidden> wrote:
> Certain instructions which can not directly load a tagged address value
> may trigger a corner case when the address size is 56 bits.  This is
> because incrementing or offsetting from the current PC can cause an
> arithetic roll-over into the tag bits.  Per the ARM ARM spec, these cases
> should also be addressed by cleaning up the tag field.
>
> This work was not done at this time since the changes could not be tested
> with current CPU models.  Comments have been added to flag the locations
> where this will need to be fixed once a model is available.

This is *not* why we haven't done this work. We haven't done it
because the maximum virtual address size permitted by the
architecture is less than 56 bits, and so this is a "can't happen"
situation.

> 3 comments added in same file to identify cases in a switch.

This should be a separate patch, because it is unrelated to the
tagged address stuff.

> Signed-off-by: Thomas Hanson <address@hidden>
> ---
>  target-arm/translate-a64.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 4d6f951..8810180 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1205,6 +1205,9 @@ static inline AArch64DecodeFn *lookup_disas_fn(const 
> AArch64DecodeTable *table,
>   */
>  static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
>  {
> +    /*If/when address size is 56 bits, this could overflow into address tag

Missing space before "If".

> +     * byte, and that byte should be fixed per ARM ARM spec.
> +     */
>      uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
>
>      if (insn & (1U << 31)) {
> @@ -1232,6 +1235,9 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t 
> insn)
>      sf = extract32(insn, 31, 1);
>      op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
>      rt = extract32(insn, 0, 5);
> +    /*If/when address size is 56 bits, this could overflow into address tag
> +     * byte, and that byte should be fixed per ARM ARM spec.
> +     */
>      addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
>
>      tcg_cmp = read_cpu_reg(s, rt, sf);
> @@ -1260,6 +1266,9 @@ static void disas_test_b_imm(DisasContext *s, uint32_t 
> insn)
>
>      bit_pos = (extract32(insn, 31, 1) << 5) | extract32(insn, 19, 5);
>      op = extract32(insn, 24, 1); /* 0: TBZ; 1: TBNZ */
> +    /*If/when address size is 56 bits, this could overflow into address tag
> +     * byte, and that byte should be fixed per ARM ARM spec.
> +     */
>      addr = s->pc + sextract32(insn, 5, 14) * 4 - 4;
>      rt = extract32(insn, 0, 5);
>
> @@ -1289,6 +1298,9 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t 
> insn)
>          unallocated_encoding(s);
>          return;
>      }
> +    /*If/when address size is 56 bits, this could overflow into address tag
> +     * byte, and that byte should be fixed per ARM ARM spec.
> +     */
>      addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
>      cond = extract32(insn, 0, 4);
>
> @@ -1636,12 +1648,12 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>           * instruction works properly.
>           */
>          switch (op2_ll) {
> -        case 1:
> +        case 1:                                                     /* SVC */
>              gen_ss_advance(s);
>              gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
>                                 default_exception_el(s));
>              break;
> -        case 2:
> +        case 2:                                                     /* HVC */
>              if (s->current_el == 0) {
>                  unallocated_encoding(s);
>                  break;
> @@ -1654,7 +1666,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>              gen_ss_advance(s);
>              gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
>              break;
> -        case 3:
> +        case 3:                                                     /* SMC */
>              if (s->current_el == 0) {
>                  unallocated_encoding(s);
>                  break;
> --
> 1.9.1

thanks
-- PMM