| Message ID | 20200824142934.20850-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Implement fp16 for AArch32 VFP | expand |
On 8/24/20 7:29 AM, Peter Maydell wrote: > The aa32_fp16_arith feature check function currently looks at the > AArch64 ID_AA64PFR0 register. This is (as the comment notes) not > correct. The bogus check was put in mostly to allow testing of the > fp16 variants of the VCMLA instructions and it was something of > a mistake that we allowed them to exist in master. > > Switch the feature check function to testing VMFR1.FPHP, which is > what it ought to be. > > This will remove emulation of the VCMLA and VCADD insns from > AArch32 code running on an AArch64 '-cpu max' using system emulation. > (They were never enabled for aarch32 linux-user and system-emulation.) > Since we weren't advertising their existence via the AArch32 ID > register, well-behaved guests wouldn't have been using them anyway. > > Once we have implemented all the AArch32 support for the FP16 extension > we will advertise it in the MVFR1 ID register field, which will reenable > these insns along with all the others. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I don't expect that any guests will have been using these insns, > but in any case the fp16 work will be all done before the next > QEMU release and the insns re-enabled... > --- > target/arm/cpu.h | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) Cc qemu-stable, for the bug fix? Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, 25 Aug 2020 at 19:06, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 8/24/20 7:29 AM, Peter Maydell wrote: > > The aa32_fp16_arith feature check function currently looks at the > > AArch64 ID_AA64PFR0 register. This is (as the comment notes) not > > correct. The bogus check was put in mostly to allow testing of the > > fp16 variants of the VCMLA instructions and it was something of > > a mistake that we allowed them to exist in master. > > > > Switch the feature check function to testing VMFR1.FPHP, which is > > what it ought to be. > > > > This will remove emulation of the VCMLA and VCADD insns from > > AArch32 code running on an AArch64 '-cpu max' using system emulation. > > (They were never enabled for aarch32 linux-user and system-emulation.) > > Since we weren't advertising their existence via the AArch32 ID > > register, well-behaved guests wouldn't have been using them anyway. > > > > Once we have implemented all the AArch32 support for the FP16 extension > > we will advertise it in the MVFR1 ID register field, which will reenable > > these insns along with all the others. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > I don't expect that any guests will have been using these insns, > > but in any case the fp16 work will be all done before the next > > QEMU release and the insns re-enabled... > > --- > > target/arm/cpu.h | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > Cc qemu-stable, for the bug fix? > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> I'd rather not put this in -stable, because it is removing insns that previously worked. (In master the insns will also be removed, of course, but there they will come back again once the fp16 VFP and Neon patchset is all done and we can set the MVFR1 value correctly.) thanks -- PMM
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index ac857bdc2c1..a1c7d8ebae5 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -3519,12 +3519,7 @@ static inline bool isar_feature_aa32_predinv(const ARMISARegisters *id) static inline bool isar_feature_aa32_fp16_arith(const ARMISARegisters *id) { - /* - * This is a placeholder for use by VCMA until the rest of - * the ARMv8.2-FP16 extension is implemented for aa32 mode. - * At which point we can properly set and check MVFR1.FPHP. - */ - return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1; + return FIELD_EX32(id->mvfr1, MVFR1, FPHP) >= 3; } static inline bool isar_feature_aa32_vfp_simd(const ARMISARegisters *id)
The aa32_fp16_arith feature check function currently looks at the AArch64 ID_AA64PFR0 register. This is (as the comment notes) not correct. The bogus check was put in mostly to allow testing of the fp16 variants of the VCMLA instructions and it was something of a mistake that we allowed them to exist in master. Switch the feature check function to testing VMFR1.FPHP, which is what it ought to be. This will remove emulation of the VCMLA and VCADD insns from AArch32 code running on an AArch64 '-cpu max' using system emulation. (They were never enabled for aarch32 linux-user and system-emulation.) Since we weren't advertising their existence via the AArch32 ID register, well-behaved guests wouldn't have been using them anyway. Once we have implemented all the AArch32 support for the FP16 extension we will advertise it in the MVFR1 ID register field, which will reenable these insns along with all the others. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I don't expect that any guests will have been using these insns, but in any case the fp16 work will be all done before the next QEMU release and the insns re-enabled... --- target/arm/cpu.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) -- 2.20.1