[Qemu-devel] [PATCH for-4.1] target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

[Qemu-devel] [PATCH for-4.1] target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

[Peter Maydell]

The ARMv5 architecture didn't specify detailed per-feature ID
registers. Now that we're using the MVFR0 register fields to
gate the existence of VFP instructions, we need to set up
the correct values in the cpu->isar structure so that we still
provide an FPU to the guest.

This fixes a regression in the arm926 and arm1026 CPUs, which
are the only ones that both have VFP and are ARMv5 or earlier.
This regression was introduced by the VFP refactoring, and more
specifically by commits 1120827fa182f0e76 and 266bd25c485597c,
which accidentally disabled VFP short-vector support and
double-precision support on these CPUs.

Reported-by: Christophe Lyon <christophe.lyon@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: 1120827fa182f0e
Fixes: 266bd25c485597c
Fixes: https://bugs.launchpad.net/qemu/+bug/1836192
---
I've followed the existing approach we used for ISAR1 here
of just filling in the fields we care about, rather than trying
to set the entire register value.

 target/arm/cpu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index e75a64a25a4..446dd5163dc 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1666,6 +1666,12 @@ static void arm926_initfn(Object *obj)
      * set the field to indicate Jazelle support within QEMU.
      */
     cpu->isar.id_isar1 = FIELD_DP32(cpu->isar.id_isar1, ID_ISAR1, JAZELLE, 1);
+    /*
+     * Similarly, we need to set MVFR0 fields to enable double precision
+     * and short vector support even though ARMv5 doesn't have this register.
+     */
+    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);
+    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1);
 }
 
 static void arm946_initfn(Object *obj)
@@ -1713,6 +1719,12 @@ static void arm1026_initfn(Object *obj)
         };
         define_one_arm_cp_reg(cpu, &ifar);
     }
+    /*
+     * Similarly, we need to set MVFR0 fields to enable double precision
+     * and short vector support even though ARMv5 doesn't have this register.
+     */
+    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);
+    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1);
 }
 
 static void arm1136_r2_initfn(Object *obj)
-- 
2.20.1

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

[Richard Henderson]

On 7/11/19 2:12 PM, Peter Maydell wrote:
> The ARMv5 architecture didn't specify detailed per-feature ID
> registers. Now that we're using the MVFR0 register fields to
> gate the existence of VFP instructions, we need to set up
> the correct values in the cpu->isar structure so that we still
> provide an FPU to the guest.
> 
> This fixes a regression in the arm926 and arm1026 CPUs, which
> are the only ones that both have VFP and are ARMv5 or earlier.
> This regression was introduced by the VFP refactoring, and more
> specifically by commits 1120827fa182f0e76 and 266bd25c485597c,
> which accidentally disabled VFP short-vector support and
> double-precision support on these CPUs.
> 
> Reported-by: Christophe Lyon <christophe.lyon@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 1120827fa182f0e
> Fixes: 266bd25c485597c
> Fixes: https://bugs.launchpad.net/qemu/+bug/1836192
> ---
> I've followed the existing approach we used for ISAR1 here
> of just filling in the fields we care about, rather than trying
> to set the entire register value.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> @@ -1713,6 +1719,12 @@ static void arm1026_initfn(Object *obj)
>          };
>          define_one_arm_cp_reg(cpu, &ifar);
>      }
> +    /*
> +     * Similarly, we need to set MVFR0 fields to enable double precision
> +     * and short vector support even though ARMv5 doesn't have this register.
> +     */
> +    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);
> +    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1);
>  }

I would have placed this immediately after the Jazelle isar setup, so that the
"Similarly" comment had the proper referent.  But, no biggie.


r~

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

[Peter Maydell]

On Thu, 11 Jul 2019 at 13:43, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/11/19 2:12 PM, Peter Maydell wrote:
> > @@ -1713,6 +1719,12 @@ static void arm1026_initfn(Object *obj)
> >          };
> >          define_one_arm_cp_reg(cpu, &ifar);
> >      }
> > +    /*
> > +     * Similarly, we need to set MVFR0 fields to enable double precision
> > +     * and short vector support even though ARMv5 doesn't have this register.
> > +     */
> > +    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);
> > +    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1);
> >  }
>
> I would have placed this immediately after the Jazelle isar setup, so that the
> "Similarly" comment had the proper referent.  But, no biggie.

Yes, I'll move it. I'd misread the chunk of code that sets the custom
IFAR as "ISAR" and assumed it was all of a piece with the Jazelle ISAR
setup.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The ARMv5 architecture didn't specify detailed per-feature ID
> registers. Now that we're using the MVFR0 register fields to
> gate the existence of VFP instructions, we need to set up
> the correct values in the cpu->isar structure so that we still
> provide an FPU to the guest.
>
> This fixes a regression in the arm926 and arm1026 CPUs, which
> are the only ones that both have VFP and are ARMv5 or earlier.
> This regression was introduced by the VFP refactoring, and more
> specifically by commits 1120827fa182f0e76 and 266bd25c485597c,
> which accidentally disabled VFP short-vector support and
> double-precision support on these CPUs.
>
> Reported-by: Christophe Lyon <christophe.lyon@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 1120827fa182f0e
> Fixes: 266bd25c485597c
> Fixes: https://bugs.launchpad.net/qemu/+bug/1836192
> ---
> I've followed the existing approach we used for ISAR1 here
> of just filling in the fields we care about, rather than trying
> to set the entire register value.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Do you think we have caught them all now? If we end up removing the
other ARM_FEATURE_foo flags in favour of isar tests we shall have to be
careful not to re-introduce these sort of bugs.

>
>  target/arm/cpu.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index e75a64a25a4..446dd5163dc 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1666,6 +1666,12 @@ static void arm926_initfn(Object *obj)
>       * set the field to indicate Jazelle support within QEMU.
>       */
>      cpu->isar.id_isar1 = FIELD_DP32(cpu->isar.id_isar1, ID_ISAR1, JAZELLE, 1);
> +    /*
> +     * Similarly, we need to set MVFR0 fields to enable double precision
> +     * and short vector support even though ARMv5 doesn't have this register.
> +     */
> +    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);
> +    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1);
>  }
>
>  static void arm946_initfn(Object *obj)
> @@ -1713,6 +1719,12 @@ static void arm1026_initfn(Object *obj)
>          };
>          define_one_arm_cp_reg(cpu, &ifar);
>      }
> +    /*
> +     * Similarly, we need to set MVFR0 fields to enable double precision
> +     * and short vector support even though ARMv5 doesn't have this register.
> +     */
> +    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);
> +    cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1);
>  }
>
>  static void arm1136_r2_initfn(Object *obj)


--
Alex Bennée

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

[Peter Maydell]

On Thu, 11 Jul 2019 at 14:01, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > The ARMv5 architecture didn't specify detailed per-feature ID
> > registers. Now that we're using the MVFR0 register fields to
> > gate the existence of VFP instructions, we need to set up
> > the correct values in the cpu->isar structure so that we still
> > provide an FPU to the guest.
> >
> > This fixes a regression in the arm926 and arm1026 CPUs, which
> > are the only ones that both have VFP and are ARMv5 or earlier.
> > This regression was introduced by the VFP refactoring, and more
> > specifically by commits 1120827fa182f0e76 and 266bd25c485597c,
> > which accidentally disabled VFP short-vector support and
> > double-precision support on these CPUs.
> >
> > Reported-by: Christophe Lyon <christophe.lyon@linaro.org>
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Fixes: 1120827fa182f0e
> > Fixes: 266bd25c485597c
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1836192
> > ---
> > I've followed the existing approach we used for ISAR1 here
> > of just filling in the fields we care about, rather than trying
> > to set the entire register value.
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> Do you think we have caught them all now? If we end up removing the
> other ARM_FEATURE_foo flags in favour of isar tests we shall have to be
> careful not to re-introduce these sort of bugs.

I checked that these were the only two extra ID-reg checks we
added as part of the VFP conversion, at any rate (ignoring
the checks for features the ARMv5 cores don't have).

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190711121231.3601-1-peter.maydell@linaro.org/



Hi,

This series failed build test on FreeBSD host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
if qemu-system-x86_64 --help >/dev/null 2>&1; then
  QEMU=qemu-system-x86_64
elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then
  QEMU=/usr/libexec/qemu-kvm
else
  exit 1
fi
make vm-build-freebsd J=21 QEMU=$QEMU
exit 0
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190711121231.3601-1-peter.maydell@linaro.org/testing.FreeBSD/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190711121231.3601-1-peter.maydell@linaro.org/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190711121231.3601-1-peter.maydell@linaro.org/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com