| Message ID | 20181024113709.16599-5-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: KVM vs ARMISARegisters | expand |
On 24 October 2018 at 12:37, Richard Henderson <richard.henderson@linaro.org> wrote: > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/kvm32.c | 33 ++++++++++++++++++++++++++++----- > 1 file changed, 28 insertions(+), 5 deletions(-) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > + /* > + * FIXME: There is not yet a way to read MVFR2. > + * Fortunately there is not yet anything in there that affects migration. > + */ We should bring that up with the KVM folks (cc'd)... Presumably KVM_REG_ARM_VFP_MVFR2 should be 0x1005 and the code in the kernel for handling this bit of the get/set-one-reg ioctls needs to have code for it (including returning 0 for v7 CPUs, where this VMRS register encoding was UNPREDICTABLE). thanks -- PMM
On 29 October 2018 at 15:13, Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 October 2018 at 12:37, Richard Henderson > <richard.henderson@linaro.org> wrote: >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/arm/kvm32.c | 33 ++++++++++++++++++++++++++++----- >> 1 file changed, 28 insertions(+), 5 deletions(-) > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > >> + /* >> + * FIXME: There is not yet a way to read MVFR2. >> + * Fortunately there is not yet anything in there that affects migration. >> + */ > > We should bring that up with the KVM folks (cc'd)... > > Presumably KVM_REG_ARM_VFP_MVFR2 should be 0x1005 and the code > in the kernel for handling this bit of the get/set-one-reg > ioctls needs to have code for it (including returning 0 > for v7 CPUs, where this VMRS register encoding was UNPREDICTABLE). There's an argument for "fail the ioctl on v7 CPUs" rather than "return 0"; I don't think I have a strong opinion, since in practice userspace needs to handle the "doesn't exist" case when it's running on an older kernel anyway. thanks -- PMM
On 29/10/18 15:21, Peter Maydell wrote: > On 29 October 2018 at 15:13, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 24 October 2018 at 12:37, Richard Henderson >> <richard.henderson@linaro.org> wrote: >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> target/arm/kvm32.c | 33 ++++++++++++++++++++++++++++----- >>> 1 file changed, 28 insertions(+), 5 deletions(-) >> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> >>> + /* >>> + * FIXME: There is not yet a way to read MVFR2. >>> + * Fortunately there is not yet anything in there that affects migration. >>> + */ >> >> We should bring that up with the KVM folks (cc'd)... >> >> Presumably KVM_REG_ARM_VFP_MVFR2 should be 0x1005 and the code >> in the kernel for handling this bit of the get/set-one-reg >> ioctls needs to have code for it (including returning 0 >> for v7 CPUs, where this VMRS register encoding was UNPREDICTABLE). > > There's an argument for "fail the ioctl on v7 CPUs" rather than > "return 0"; I don't think I have a strong opinion, since in > practice userspace needs to handle the "doesn't exist" case > when it's running on an older kernel anyway. My temptation would be not to expose it at all when running on a v7 core, and return an error rather than zero. The other issue is that we currently don't support running 32bit KVM on any ARMv8 platform, as we strictly check the CPUs we want to run on (A7 and A15). I remember seeing patches that would allow any host core to be used (similar to what we have on the 64bit side), but that never made it in the tree. If there is interest, I can revive this effort. Thanks, M. -- Jazz is not dead. It just smells funny...
On 29 October 2018 at 15:40, Marc Zyngier <marc.zyngier@arm.com> wrote: > My temptation would be not to expose it at all when running on a v7 > core, and return an error rather than zero. > > The other issue is that we currently don't support running 32bit KVM on > any ARMv8 platform, as we strictly check the CPUs we want to run on (A7 > and A15). I remember seeing patches that would allow any host core to be > used (similar to what we have on the 64bit side), but that never made it > in the tree. Ah, that's convenient in some ways. It means we can define the API to be "for v7, no register available via the ONE_REG API; for v8, always present", provided we add the constant and the support before we turn on any actual v8 CPUs for 32-bit KVM (avoiding the awkward case of "v8 but kernel doesn't expose MVFR2"). I don't think we particularly care about the 32-bit-kvm-on-v8 part, but it would be good to nail down this wrinkle so we don't forget about it, maybe ? thanks -- PMM
On 29/10/18 15:48, Peter Maydell wrote: > On 29 October 2018 at 15:40, Marc Zyngier <marc.zyngier@arm.com> wrote: >> My temptation would be not to expose it at all when running on a v7 >> core, and return an error rather than zero. >> >> The other issue is that we currently don't support running 32bit KVM on >> any ARMv8 platform, as we strictly check the CPUs we want to run on (A7 >> and A15). I remember seeing patches that would allow any host core to be >> used (similar to what we have on the 64bit side), but that never made it >> in the tree. > > Ah, that's convenient in some ways. It means we can define the > API to be "for v7, no register available via the ONE_REG API; > for v8, always present", provided we add the constant and the > support before we turn on any actual v8 CPUs for 32-bit KVM > (avoiding the awkward case of "v8 but kernel doesn't expose > MVFR2"). > > I don't think we particularly care about the 32-bit-kvm-on-v8 > part, but it would be good to nail down this wrinkle so we don't > forget about it, maybe ? Absolutely. I'll write something up. Thanks, M. -- Jazz is not dead. It just smells funny...
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c index da08f7aab8..f23cc77d9e 100644 --- a/target/arm/kvm32.c +++ b/target/arm/kvm32.c @@ -44,7 +44,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) * and then query that CPU for the relevant ID registers. */ int i, err = 0, fdarray[3]; - uint32_t midr, id_pfr0, mvfr1; + uint32_t midr, id_pfr0; uint64_t features = 0; /* Old kernels may not know about the PREFERRED_TARGET ioctl: however @@ -71,9 +71,32 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) err |= read_sys_reg32(fdarray[2], &midr, ARM_CP15_REG32(0, 0, 0, 0)); err |= read_sys_reg32(fdarray[2], &id_pfr0, ARM_CP15_REG32(0, 0, 1, 0)); - err |= read_sys_reg32(fdarray[2], &mvfr1, + + err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar0, + ARM_CP15_REG32(0, 0, 2, 0)); + err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar1, + ARM_CP15_REG32(0, 0, 2, 1)); + err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar2, + ARM_CP15_REG32(0, 0, 2, 2)); + err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar3, + ARM_CP15_REG32(0, 0, 2, 3)); + err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar4, + ARM_CP15_REG32(0, 0, 2, 4)); + err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar5, + ARM_CP15_REG32(0, 0, 2, 5)); + err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6, + ARM_CP15_REG32(0, 0, 2, 7)); + + err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr0, + KVM_REG_ARM | KVM_REG_SIZE_U32 | + KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR0); + err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr1, KVM_REG_ARM | KVM_REG_SIZE_U32 | KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1); + /* + * FIXME: There is not yet a way to read MVFR2. + * Fortunately there is not yet anything in there that affects migration. + */ kvm_arm_destroy_scratch_host_vcpu(fdarray); @@ -95,13 +118,13 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) if (extract32(id_pfr0, 12, 4) == 1) { set_feature(&features, ARM_FEATURE_THUMB2EE); } - if (extract32(mvfr1, 20, 4) == 1) { + if (extract32(ahcf->isar.mvfr1, 20, 4) == 1) { set_feature(&features, ARM_FEATURE_VFP_FP16); } - if (extract32(mvfr1, 12, 4) == 1) { + if (extract32(ahcf->isar.mvfr1, 12, 4) == 1) { set_feature(&features, ARM_FEATURE_NEON); } - if (extract32(mvfr1, 28, 4) == 1) { + if (extract32(ahcf->isar.mvfr1, 28, 4) == 1) { /* FMAC support implies VFPv4 */ set_feature(&features, ARM_FEATURE_VFP4); }
Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/kvm32.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) -- 2.17.2