diff mbox

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

Message ID 1474047287-145701-4-git-send-email-thomas.hanson@linaro.org
State New
Headers show

Commit Message

Tom Hanson Sept. 16, 2016, 5:34 p.m. UTC 
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.

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

Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>

---
 target-arm/translate-a64.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

-- 
1.9.1

Comments

Peter Maydell Sept. 30, 2016, 1:27 a.m. UTC | #1 
On 16 September 2016 at 10:34, Thomas Hanson <thomas.hanson@linaro.org> 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 <thomas.hanson@linaro.org>

> ---

>  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
Tom Hanson Sept. 30, 2016, 10:46 p.m. UTC | #2 
On 09/29/2016 07:27 PM, Peter Maydell wrote:
...
>> 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.


But, in an earlier discussion which we had about the desire to use QEMU to test potential new ARM-based architectures with large address spaces I suggested that these changes be made now.  You said that the changes shouldn't be made because:
    where there is no supported guest CPU that could use
    that code, the code shouldn't be there because it's untested
    and untestable
Isn't that the same thing I said above?

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


As part of that same conversation you suggested adding these comments rather than making the changes:
    If we can assert, or failing that have a comment in the place
    that would be modified anyway for 56 bit addresses then that
    ought to catch the future case I think.
Peter Maydell Sept. 30, 2016, 11:24 p.m. UTC | #3 
On 30 September 2016 at 15:46, Tom Hanson <thomas.hanson@linaro.org> wrote:
> On 09/29/2016 07:27 PM, Peter Maydell wrote:

> ...

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

>

> But, in an earlier discussion which we had about the desire to use QEMU

> to test potential new ARM-based architectures with large address spaces

> I suggested that these changes be made now.  You said that the changes

> shouldn't be made because:

>     where there is no supported guest CPU that could use

>     that code, the code shouldn't be there because it's untested

>     and untestable

> Isn't that the same thing I said above?


That's a general statement of principle about what I think we
should or shouldn't write code for in QEMU. In this particular case,
it's true, but the reason it's true isn't just that we don't
currently have any 56 bit-VA CPUs implemented, but because such
a CPU is not permitted by the architecture. That's a stronger
statement and I think it's worth making.

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

>

> As part of that same conversation you suggested adding these

> comments rather than making the changes:

>     If we can assert, or failing that have a comment in the place

>     that would be modified anyway for 56 bit addresses then that

>     ought to catch the future case I think.


Yes, I still think this. What does it have to do with adding
"SVC", "HVC", etc comments to the switch cases? Those have
nothing to do with tagged addresses or 56 bit VAs, and should
not be in this patch (though I don't object to them inherently).

thanks
-- PMM
Tom Hanson Oct. 3, 2016, 5:01 p.m. UTC | #4 
On 09/30/2016 05:24 PM, Peter Maydell wrote:
>>>> 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.

>>

>> As part of that same conversation you suggested adding these

>> comments rather than making the changes:

>>     If we can assert, or failing that have a comment in the place

>>     that would be modified anyway for 56 bit addresses then that

>>     ought to catch the future case I think.

> 

> Yes, I still think this. What does it have to do with adding

> "SVC", "HVC", etc comments to the switch cases? Those have

> nothing to do with tagged addresses or 56 bit VAs, and should

> not be in this patch (though I don't object to them inherently).

> 

> thanks

> -- PMM


Sorry, moving too fast and didn't look at which comments you were referring to.  I'll drop them.

-Tom
Tom Hanson Oct. 3, 2016, 6:26 p.m. UTC | #5 
On 09/30/2016 05:24 PM, Peter Maydell wrote:
> On 30 September 2016 at 15:46, Tom Hanson <thomas.hanson@linaro.org> wrote:

>> On 09/29/2016 07:27 PM, Peter Maydell wrote:

>> ...

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

>>

>> But, in an earlier discussion which we had about the desire to use QEMU

>> to test potential new ARM-based architectures with large address spaces

>> I suggested that these changes be made now.  You said that the changes

>> shouldn't be made because:

>>     where there is no supported guest CPU that could use

>>     that code, the code shouldn't be there because it's untested

>>     and untestable

>> Isn't that the same thing I said above?

> 

> That's a general statement of principle about what I think we

> should or shouldn't write code for in QEMU. In this particular case,

> it's true, but the reason it's true isn't just that we don't

> currently have any 56 bit-VA CPUs implemented, but because such

> a CPU is not permitted by the architecture. That's a stronger

> statement and I think it's worth making.

> 


Per the current spec (and v2) that's true.  But the intent was to enable testing of "new ARM-based architectures with large address spaces."  Vendors and OEMs may have difficulty in determining whether to ask for / push for / support a future, larger address space in the absence of a platform which is capable of emulating the future architecture.
diff mbox

Patch

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
+     * 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;