[v3] arm: Fixes and preparatory cleanups for split-accel

[PATCH v3 04/26] target/arm/hvf: Simplify GIC hvf_arch_init_vcpu()

Posted by Philippe Mathieu-Daudé 4 months, 3 weeks ago

Only update the ID_AA64PFR0_EL1 register when a GIC is provided.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/hvf/hvf.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 42258cc2d88..c1ed8b510db 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1057,11 +1057,15 @@ int hvf_arch_init_vcpu(CPUState *cpu)
                               arm_cpu->mp_affinity);
     assert_hvf_ok(ret);
 
-    ret = hv_vcpu_get_sys_reg(cpu->accel->fd, HV_SYS_REG_ID_AA64PFR0_EL1, &pfr);
-    assert_hvf_ok(ret);
-    pfr |= env->gicv3state ? (1 << 24) : 0;
-    ret = hv_vcpu_set_sys_reg(cpu->accel->fd, HV_SYS_REG_ID_AA64PFR0_EL1, pfr);
-    assert_hvf_ok(ret);
+    if (env->gicv3state) {
+        ret = hv_vcpu_get_sys_reg(cpu->accel->fd,
+                                  HV_SYS_REG_ID_AA64PFR0_EL1, &pfr);
+        assert_hvf_ok(ret);
+        pfr = FIELD_DP64(pfr, ID_AA64PFR0, GIC, 1);
+        ret = hv_vcpu_set_sys_reg(cpu->accel->fd,
+                                  HV_SYS_REG_ID_AA64PFR0_EL1, pfr);
+        assert_hvf_ok(ret);
+    }
 
     /* We're limited to underlying hardware caps, override internal versions */
     ret = hv_vcpu_get_sys_reg(cpu->accel->fd, HV_SYS_REG_ID_AA64MMFR0_EL1,
-- 
2.49.0

Re: [PATCH v3 04/26] target/arm/hvf: Simplify GIC hvf_arch_init_vcpu()

Posted by Peter Maydell 4 months, 2 weeks ago

On Mon, 23 Jun 2025 at 13:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Only update the ID_AA64PFR0_EL1 register when a GIC is provided.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/hvf/hvf.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 42258cc2d88..c1ed8b510db 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1057,11 +1057,15 @@ int hvf_arch_init_vcpu(CPUState *cpu)
>                                arm_cpu->mp_affinity);
>      assert_hvf_ok(ret);
>
> -    ret = hv_vcpu_get_sys_reg(cpu->accel->fd, HV_SYS_REG_ID_AA64PFR0_EL1, &pfr);
> -    assert_hvf_ok(ret);
> -    pfr |= env->gicv3state ? (1 << 24) : 0;
> -    ret = hv_vcpu_set_sys_reg(cpu->accel->fd, HV_SYS_REG_ID_AA64PFR0_EL1, pfr);
> -    assert_hvf_ok(ret);
> +    if (env->gicv3state) {
> +        ret = hv_vcpu_get_sys_reg(cpu->accel->fd,
> +                                  HV_SYS_REG_ID_AA64PFR0_EL1, &pfr);
> +        assert_hvf_ok(ret);
> +        pfr = FIELD_DP64(pfr, ID_AA64PFR0, GIC, 1);
> +        ret = hv_vcpu_set_sys_reg(cpu->accel->fd,
> +                                  HV_SYS_REG_ID_AA64PFR0_EL1, pfr);
> +        assert_hvf_ok(ret);
> +    }

This doesn't seem like a simplification to me...

Looking at the code, I suspect what we should really be doing
is setting the GIC field to either 0 or 1 depending on whether
env->gicv3state. Currently if hvf hands us an initial value with
the GIC field set to 1 but we don't have a gicv3state we won't
correctly clear it to 0. i.e. we should change the current
  pfr |= env->gicv3state ? (1 << 24) : 0;
to
  pfr = FIELD_DP64(pfr, ID_AA64PFR0, GIC, env->gicv3state ? 1 : 0);

thanks
-- PMM

Re: [PATCH v3 04/26] target/arm/hvf: Simplify GIC hvf_arch_init_vcpu()

Posted by Philippe Mathieu-Daudé 4 months, 2 weeks ago

On 1/7/25 11:39, Peter Maydell wrote:
> On Mon, 23 Jun 2025 at 13:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Only update the ID_AA64PFR0_EL1 register when a GIC is provided.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/hvf/hvf.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>> index 42258cc2d88..c1ed8b510db 100644
>> --- a/target/arm/hvf/hvf.c
>> +++ b/target/arm/hvf/hvf.c
>> @@ -1057,11 +1057,15 @@ int hvf_arch_init_vcpu(CPUState *cpu)
>>                                 arm_cpu->mp_affinity);
>>       assert_hvf_ok(ret);
>>
>> -    ret = hv_vcpu_get_sys_reg(cpu->accel->fd, HV_SYS_REG_ID_AA64PFR0_EL1, &pfr);
>> -    assert_hvf_ok(ret);
>> -    pfr |= env->gicv3state ? (1 << 24) : 0;
>> -    ret = hv_vcpu_set_sys_reg(cpu->accel->fd, HV_SYS_REG_ID_AA64PFR0_EL1, pfr);
>> -    assert_hvf_ok(ret);
>> +    if (env->gicv3state) {
>> +        ret = hv_vcpu_get_sys_reg(cpu->accel->fd,
>> +                                  HV_SYS_REG_ID_AA64PFR0_EL1, &pfr);
>> +        assert_hvf_ok(ret);
>> +        pfr = FIELD_DP64(pfr, ID_AA64PFR0, GIC, 1);
>> +        ret = hv_vcpu_set_sys_reg(cpu->accel->fd,
>> +                                  HV_SYS_REG_ID_AA64PFR0_EL1, pfr);
>> +        assert_hvf_ok(ret);
>> +    }
> 
> This doesn't seem like a simplification to me...
> 
> Looking at the code, I suspect what we should really be doing
> is setting the GIC field to either 0 or 1 depending on whether
> env->gicv3state. Currently if hvf hands us an initial value with
> the GIC field set to 1 but we don't have a gicv3state we won't
> correctly clear it to 0. i.e. we should change the current
>    pfr |= env->gicv3state ? (1 << 24) : 0;
> to
>    pfr = FIELD_DP64(pfr, ID_AA64PFR0, GIC, env->gicv3state ? 1 : 0);

Good idea.