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

[Tom Hanson]

On 09/30/2016 05:24 PM, Peter Maydell wrote:
> On 30 September 2016 at 15:46, Tom Hanson  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.  

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

[Tom Hanson]

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

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

[Peter Maydell]

On 30 September 2016 at 15:46, Tom Hanson  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

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

[Tom Hanson]

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.

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

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

[Thomas Hanson]

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