diff mbox series

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

Message ID 20220527180623.185261-4-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: SME prep patches | expand

Commit Message

Richard Henderson May 27, 2022, 6:06 p.m. UTC
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(-)

Comments

Peter Maydell May 31, 2022, 12:15 p.m. UTC | #1
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
Richard Henderson May 31, 2022, 2:28 p.m. UTC | #2
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~
Peter Maydell May 31, 2022, 2:55 p.m. UTC | #3
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
diff mbox series

Patch

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