[v3] target/arm: SME prep patches

[PATCH v3 03/15] target/arm: Do not use aarch64_sve_zcr_get_valid_len in reset

Posted by Richard Henderson 3 years, 8 months ago

We don't need to constrain the value set in zcr_el[1],
because it will be done by sve_zcr_len_for_el.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index d2bd74c2ed..0621944167 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -208,8 +208,7 @@ static void arm_cpu_reset(DeviceState *dev)
                                          CPACR_EL1, ZEN, 3);
         /* with reasonable vector length */
         if (cpu_isar_feature(aa64_sve, cpu)) {
-            env->vfp.zcr_el[1] =
-                aarch64_sve_zcr_get_valid_len(cpu, cpu->sve_default_vq - 1);
+            env->vfp.zcr_el[1] = cpu->sve_default_vq - 1;
         }
         /*
          * Enable 48-bit address space (TODO: take reserved_va into account).
-- 
2.34.1

Re: [PATCH v3 03/15] target/arm: Do not use aarch64_sve_zcr_get_valid_len in reset

Posted by Peter Maydell 3 years, 8 months ago

On Fri, 27 May 2022 at 19:07, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We don't need to constrain the value set in zcr_el[1],
> because it will be done by sve_zcr_len_for_el.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index d2bd74c2ed..0621944167 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -208,8 +208,7 @@ static void arm_cpu_reset(DeviceState *dev)
>                                           CPACR_EL1, ZEN, 3);
>          /* with reasonable vector length */
>          if (cpu_isar_feature(aa64_sve, cpu)) {
> -            env->vfp.zcr_el[1] =
> -                aarch64_sve_zcr_get_valid_len(cpu, cpu->sve_default_vq - 1);
> +            env->vfp.zcr_el[1] = cpu->sve_default_vq - 1;
>          }

I'm still not a fan of the zcr_el[] value not actually being
a valid one. I'd rather we constrained it when we write the
value into the field.

thanks
-- PMM

Re: [PATCH v3 03/15] target/arm: Do not use aarch64_sve_zcr_get_valid_len in reset

Posted by Richard Henderson 3 years, 8 months ago

On 5/31/22 05:15, Peter Maydell wrote:
> On Fri, 27 May 2022 at 19:07, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> We don't need to constrain the value set in zcr_el[1],
>> because it will be done by sve_zcr_len_for_el.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/cpu.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index d2bd74c2ed..0621944167 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -208,8 +208,7 @@ static void arm_cpu_reset(DeviceState *dev)
>>                                            CPACR_EL1, ZEN, 3);
>>           /* with reasonable vector length */
>>           if (cpu_isar_feature(aa64_sve, cpu)) {
>> -            env->vfp.zcr_el[1] =
>> -                aarch64_sve_zcr_get_valid_len(cpu, cpu->sve_default_vq - 1);
>> +            env->vfp.zcr_el[1] = cpu->sve_default_vq - 1;
>>           }
> 
> I'm still not a fan of the zcr_el[] value not actually being
> a valid one. I'd rather we constrained it when we write the
> value into the field.

It is an architecturally valid value, exactly like the kernel might write while probing 
for supported vector lengths.  It will result in this, or the next smaller supported 
vector size, being selected.


r~

Re: [PATCH v3 03/15] target/arm: Do not use aarch64_sve_zcr_get_valid_len in reset

Posted by Peter Maydell 3 years, 8 months ago

On Tue, 31 May 2022 at 15:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/31/22 05:15, Peter Maydell wrote:
> > On Fri, 27 May 2022 at 19:07, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> We don't need to constrain the value set in zcr_el[1],
> >> because it will be done by sve_zcr_len_for_el.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   target/arm/cpu.c | 3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> >> index d2bd74c2ed..0621944167 100644
> >> --- a/target/arm/cpu.c
> >> +++ b/target/arm/cpu.c
> >> @@ -208,8 +208,7 @@ static void arm_cpu_reset(DeviceState *dev)
> >>                                            CPACR_EL1, ZEN, 3);
> >>           /* with reasonable vector length */
> >>           if (cpu_isar_feature(aa64_sve, cpu)) {
> >> -            env->vfp.zcr_el[1] =
> >> -                aarch64_sve_zcr_get_valid_len(cpu, cpu->sve_default_vq - 1);
> >> +            env->vfp.zcr_el[1] = cpu->sve_default_vq - 1;
> >>           }
> >
> > I'm still not a fan of the zcr_el[] value not actually being
> > a valid one. I'd rather we constrained it when we write the
> > value into the field.
>
> It is an architecturally valid value, exactly like the kernel might write while probing
> for supported vector lengths.  It will result in this, or the next smaller supported
> vector size, being selected.

Mmm, I guess so (having re-read the ZCR_EL1 docs).

-- PMM