[PATCH 0/2] target/arm: Fix issue 1078

[PATCH 0/2] target/arm: Fix issue 1078

[Richard Henderson]

Nicely summarized by the reporter, but I thought it would be
nicer to pull all of the logic into arm_pamax, rather than
leave it separated.


r~


Richard Henderson (2):
  target/arm: Extend arm_pamax to more than aarch64
  target/arm: Check V7VE as well as LPAE in arm_pamax

 hw/arm/virt.c    | 10 +---------
 target/arm/ptw.c | 26 ++++++++++++++++++++------
 2 files changed, 21 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/2] target/arm: Extend arm_pamax to more than aarch64

[Richard Henderson]

Move the code from hw/arm/virt.c that is supposed
to handle v7 into the one function.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/arm/virt.c    | 10 +---------
 target/arm/ptw.c | 24 ++++++++++++++++--------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 097238faa7..5502aa60c8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2010,15 +2010,7 @@ static void machvirt_init(MachineState *machine)
         cpuobj = object_new(possible_cpus->cpus[0].type);
         armcpu = ARM_CPU(cpuobj);
 
-        if (object_property_get_bool(cpuobj, "aarch64", NULL)) {
-            pa_bits = arm_pamax(armcpu);
-        } else if (arm_feature(&armcpu->env, ARM_FEATURE_LPAE)) {
-            /* v7 with LPAE */
-            pa_bits = 40;
-        } else {
-            /* Anything else */
-            pa_bits = 32;
-        }
+        pa_bits = arm_pamax(armcpu);
 
         object_unref(cpuobj);
 
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 4d97a24808..07f7a21861 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -36,15 +36,23 @@ static const uint8_t pamax_map[] = {
 /* The cpu-specific constant value of PAMax; also used by hw/arm/virt. */
 unsigned int arm_pamax(ARMCPU *cpu)
 {
-    unsigned int parange =
-        FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
+    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+        unsigned int parange =
+            FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
 
-    /*
-     * id_aa64mmfr0 is a read-only register so values outside of the
-     * supported mappings can be considered an implementation error.
-     */
-    assert(parange < ARRAY_SIZE(pamax_map));
-    return pamax_map[parange];
+        /*
+         * id_aa64mmfr0 is a read-only register so values outside of the
+         * supported mappings can be considered an implementation error.
+         */
+        assert(parange < ARRAY_SIZE(pamax_map));
+        return pamax_map[parange];
+    }
+    if (arm_feature(&cpu->env, ARM_FEATURE_LPAE)) {
+        /* v7 with LPAE */
+        return 40;
+    }
+    /* Anything else */
+    return 32;
 }
 
 /*
-- 
2.34.1

[PATCH 2/2] target/arm: Check V7VE as well as LPAE in arm_pamax

[Richard Henderson]

In machvirt_init we create a cpu but do not fully initialize it.
Thus the propagation of V7VE to LPAE has not been done, and we
compute the wrong value for some v7 cpus, e.g. cortex-a15.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1078
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/ptw.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 07f7a21861..da478104f0 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -47,7 +47,13 @@ unsigned int arm_pamax(ARMCPU *cpu)
         assert(parange < ARRAY_SIZE(pamax_map));
         return pamax_map[parange];
     }
-    if (arm_feature(&cpu->env, ARM_FEATURE_LPAE)) {
+
+    /*
+     * In machvirt_init, we call arm_pamax on a cpu that is not fully
+     * initialized, so we can't rely on the propagation done in realize.
+     */
+    if (arm_feature(&cpu->env, ARM_FEATURE_LPAE) ||
+        arm_feature(&cpu->env, ARM_FEATURE_V7VE)) {
         /* v7 with LPAE */
         return 40;
     }
-- 
2.34.1

Re: [PATCH 0/2] target/arm: Fix issue 1078

[He Zhe]

On 6/19/22 08:15, Richard Henderson wrote:
> Nicely summarized by the reporter, but I thought it would be
> nicer to pull all of the logic into arm_pamax, rather than
> leave it separated.

Reported-by: He Zhe <zhe.he@windriver.com>

I ran a quick test. qemu still hangs with these two commits applied.

One fact that might help, qemu can boot up successfully if the change of the
following commit is reverted.
https://gitlab.com/qemu-project/qemu/-/commit/39a1fd25287f5dece59fdf4752491faf59310296
The change had been moved to target/arm/ptw.c.

Zhe

>
>
> r~
>
>
> Richard Henderson (2):
>   target/arm: Extend arm_pamax to more than aarch64
>   target/arm: Check V7VE as well as LPAE in arm_pamax
>
>  hw/arm/virt.c    | 10 +---------
>  target/arm/ptw.c | 26 ++++++++++++++++++++------
>  2 files changed, 21 insertions(+), 15 deletions(-)
>

Re: [PATCH 1/2] target/arm: Extend arm_pamax to more than aarch64

[Peter Maydell]

On Sun, 19 Jun 2022 at 01:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Move the code from hw/arm/virt.c that is supposed
> to handle v7 into the one function.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/arm/virt.c    | 10 +---------
>  target/arm/ptw.c | 24 ++++++++++++++++--------

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/2] target/arm: Check V7VE as well as LPAE in arm_pamax

[Peter Maydell]

On Sun, 19 Jun 2022 at 01:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> In machvirt_init we create a cpu but do not fully initialize it.
> Thus the propagation of V7VE to LPAE has not been done, and we
> compute the wrong value for some v7 cpus, e.g. cortex-a15.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1078
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/ptw.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 07f7a21861..da478104f0 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -47,7 +47,13 @@ unsigned int arm_pamax(ARMCPU *cpu)
>          assert(parange < ARRAY_SIZE(pamax_map));
>          return pamax_map[parange];
>      }
> -    if (arm_feature(&cpu->env, ARM_FEATURE_LPAE)) {
> +
> +    /*
> +     * In machvirt_init, we call arm_pamax on a cpu that is not fully
> +     * initialized, so we can't rely on the propagation done in realize.
> +     */
> +    if (arm_feature(&cpu->env, ARM_FEATURE_LPAE) ||
> +        arm_feature(&cpu->env, ARM_FEATURE_V7VE)) {
>          /* v7 with LPAE */
>          return 40;

I guess this is expedient, so on that basis
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

but as I mentioned in the gitlab issue it's kind of bogus
that the virt board is doing stuff to a non-realized CPU object.

thanks
-- PMM

Re: [PATCH 0/2] target/arm: Fix issue 1078

[Peter Maydell]

On Sun, 19 Jun 2022 at 01:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Nicely summarized by the reporter, but I thought it would be
> nicer to pull all of the logic into arm_pamax, rather than
> leave it separated.

Applied to target-arm.next, thanks.

I think the hang He Zhe reports as still present is an
unrelated issue (I should check the v7 LPAE spec about
block descriptors I guess).

-- PMM

Re: [PATCH 2/2] target/arm: Check V7VE as well as LPAE in arm_pamax

[Richard Henderson]

On 6/24/22 09:27, Peter Maydell wrote:
>> +    /*
>> +     * In machvirt_init, we call arm_pamax on a cpu that is not fully
>> +     * initialized, so we can't rely on the propagation done in realize.
>> +     */
>> +    if (arm_feature(&cpu->env, ARM_FEATURE_LPAE) ||
>> +        arm_feature(&cpu->env, ARM_FEATURE_V7VE)) {
>>           /* v7 with LPAE */
>>           return 40;
> 
> I guess this is expedient, so on that basis
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> but as I mentioned in the gitlab issue it's kind of bogus
> that the virt board is doing stuff to a non-realized CPU object.

My first look suggested that the virt board wasn't even setting all of the cpu properties 
properly, so realization might not help.  I meant to go back again and soend more time, 
but that hasn't happened yet.


r~