diff mbox series

[v3,2/5] target/arm: Fill in ARMISARegisters for kvm64

Message ID 20181108175246.13416-3-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: KVM vs ARMISARegisters | expand

Commit Message

Richard Henderson Nov. 8, 2018, 5:52 p.m. UTC 
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/kvm64.c | 90 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 88 insertions(+), 2 deletions(-)

-- 
2.17.2

Comments

Peter Maydell Nov. 12, 2018, 11:37 a.m. UTC | #1 
On 8 November 2018 at 17:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---


>      /* Old kernels may not know about the PREFERRED_TARGET ioctl: however

>       * we know these will only support creating one kind of guest CPU,

>       * which is its preferred CPU type. Fortunately these old kernels

> @@ -474,8 +497,71 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)

>      ahcf->target = init.target;

>      ahcf->dtb_compatible = "arm,arm-v8";

>

> +    err = read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,

> +                         ARM64_SYS_REG(3, 0, 0, 4, 0));

> +    if (unlikely(err < 0)) {

> +        /*

> +         * Before v4.15, the kernel only exposed a limited number of system

> +         * registers, not including any of the interesting AArch64 ID regs.

> +         * For the most part we could leave these fields as zero with minimal

> +         * effect, since this does not affect the values seen by the guest.


These older kernels do implement reading of id_isar0 through
id_isar5, though -- we could read and use those values rather than
leaving them zero.

> +         *

> +         * However, it could cause problems down the line for QEMU,

> +         * so provide a minimal v8.0 default.

> +         *

> +         * ??? Could read MIDR and use knowledge from cpu64.c.

> +         * ??? Could map a page of memory into our temp guest and

> +         *     run the tiniest of hand-crafted kernels to extract

> +         *     the values seen by the guest.

> +         * ??? Either of these sounds like too much effort just

> +         *     to work around running a modern host kernel.

> +         */

> +        ahcf->isar.id_aa64pfr0 = 0x00000011; /* EL1&0, AArch64 only */

> +        err = 0;


Doesn't this code path leave everything except id_aa64pfr0 as
zero, thus leaving us with the "could cause problems down the
line" situation ?

thanks
-- PMM
Richard Henderson Nov. 12, 2018, 2:09 p.m. UTC | #2 
On 11/12/18 12:37 PM, Peter Maydell wrote:
> On 8 November 2018 at 17:52, Richard Henderson

> <richard.henderson@linaro.org> wrote:

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

>> ---

> 

>>      /* Old kernels may not know about the PREFERRED_TARGET ioctl: however

>>       * we know these will only support creating one kind of guest CPU,

>>       * which is its preferred CPU type. Fortunately these old kernels

>> @@ -474,8 +497,71 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)

>>      ahcf->target = init.target;

>>      ahcf->dtb_compatible = "arm,arm-v8";

>>

>> +    err = read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,

>> +                         ARM64_SYS_REG(3, 0, 0, 4, 0));

>> +    if (unlikely(err < 0)) {

>> +        /*

>> +         * Before v4.15, the kernel only exposed a limited number of system

>> +         * registers, not including any of the interesting AArch64 ID regs.

>> +         * For the most part we could leave these fields as zero with minimal

>> +         * effect, since this does not affect the values seen by the guest.

> 

> These older kernels do implement reading of id_isar0 through

> id_isar5, though -- we could read and use those values rather than

> leaving them zero.


But without id_aa64pfr0, we don't know if id_isar0 et al have UNKNOWN values
due to aa32 mode not being present.  E.g. run a 4.8 kernel on a thunderx1 cpu.

>> +        ahcf->isar.id_aa64pfr0 = 0x00000011; /* EL1&0, AArch64 only */

>> +        err = 0;

> 

> Doesn't this code path leave everything except id_aa64pfr0 as

> zero, thus leaving us with the "could cause problems down the

> line" situation ?


The other id_aa64* register fields are all extensions to v8.0, so they should
be zero.  I am of course assuming that AdvSIMD is present, otherwise qemu
itself probably have failed before now.


r~
Peter Maydell Nov. 12, 2018, 2:34 p.m. UTC | #3 
On 12 November 2018 at 14:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 11/12/18 12:37 PM, Peter Maydell wrote:

>> On 8 November 2018 at 17:52, Richard Henderson

>> <richard.henderson@linaro.org> wrote:

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

>>> ---

>>

>>>      /* Old kernels may not know about the PREFERRED_TARGET ioctl: however

>>>       * we know these will only support creating one kind of guest CPU,

>>>       * which is its preferred CPU type. Fortunately these old kernels

>>> @@ -474,8 +497,71 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)

>>>      ahcf->target = init.target;

>>>      ahcf->dtb_compatible = "arm,arm-v8";

>>>

>>> +    err = read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,

>>> +                         ARM64_SYS_REG(3, 0, 0, 4, 0));

>>> +    if (unlikely(err < 0)) {

>>> +        /*

>>> +         * Before v4.15, the kernel only exposed a limited number of system

>>> +         * registers, not including any of the interesting AArch64 ID regs.

>>> +         * For the most part we could leave these fields as zero with minimal

>>> +         * effect, since this does not affect the values seen by the guest.

>>

>> These older kernels do implement reading of id_isar0 through

>> id_isar5, though -- we could read and use those values rather than

>> leaving them zero.

>

> But without id_aa64pfr0, we don't know if id_isar0 et al have UNKNOWN values

> due to aa32 mode not being present.  E.g. run a 4.8 kernel on a thunderx1 cpu.


Good point, which makes things kind of awkward.

>>> +        ahcf->isar.id_aa64pfr0 = 0x00000011; /* EL1&0, AArch64 only */

>>> +        err = 0;

>>

>> Doesn't this code path leave everything except id_aa64pfr0 as

>> zero, thus leaving us with the "could cause problems down the

>> line" situation ?

>

> The other id_aa64* register fields are all extensions to v8.0, so they should

> be zero.  I am of course assuming that AdvSIMD is present, otherwise qemu

> itself probably have failed before now.


You don't set the id_isar* to say "AdvSIMD present" (or "VFP present"), though.
So if we in a later commit change target/arm/machine.c:vfp_needed() to
check an id_isar* field instead of ARM_FEATURE_VFP then we'll break
migration for these old kernel versions. That was the kind of thing I was
hoping we could avoid having to remember for the future...

thanks
-- PMM
Richard Henderson Nov. 12, 2018, 4 p.m. UTC | #4 
On 11/12/18 3:34 PM, Peter Maydell wrote:
>> The other id_aa64* register fields are all extensions to v8.0, so they should

>> be zero.  I am of course assuming that AdvSIMD is present, otherwise qemu

>> itself probably have failed before now.

> 

> You don't set the id_isar* to say "AdvSIMD present" (or "VFP present"), though.


Those two are almost unique in that they are signed fields.  So 0 indicates
present and -1 indicates not present (and 1 indicates fp16 support).


r~
diff mbox series

Patch

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 5de8ff0ac5..e1128b94b2 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -443,17 +443,40 @@  static inline void unset_feature(uint64_t *features, int feature)
     *features &= ~(1ULL << feature);
 }
 
+static int read_sys_reg32(int fd, uint32_t *pret, uint64_t id)
+{
+    uint64_t ret;
+    struct kvm_one_reg idreg = { .id = id, .addr = (uintptr_t)&ret };
+    int err;
+
+    assert((id & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U64);
+    err = ioctl(fd, KVM_GET_ONE_REG, &idreg);
+    if (err < 0) {
+        return -1;
+    }
+    *pret = ret;
+    return 0;
+}
+
+static int read_sys_reg64(int fd, uint64_t *pret, uint64_t id)
+{
+    struct kvm_one_reg idreg = { .id = id, .addr = (uintptr_t)pret };
+
+    assert((id & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U64);
+    return ioctl(fd, KVM_GET_ONE_REG, &idreg);
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
      * fill out the ARMHostCPUClass fields accordingly. To do this
      * we have to create a scratch VM, create a single CPU inside it,
      * and then query that CPU for the relevant ID registers.
-     * For AArch64 we currently don't care about ID registers at
-     * all; we just want to know the CPU type.
      */
     int fdarray[3];
     uint64_t features = 0;
+    int err;
+
     /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
      * we know these will only support creating one kind of guest CPU,
      * which is its preferred CPU type. Fortunately these old kernels
@@ -474,8 +497,71 @@  bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     ahcf->target = init.target;
     ahcf->dtb_compatible = "arm,arm-v8";
 
+    err = read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,
+                         ARM64_SYS_REG(3, 0, 0, 4, 0));
+    if (unlikely(err < 0)) {
+        /*
+         * Before v4.15, the kernel only exposed a limited number of system
+         * registers, not including any of the interesting AArch64 ID regs.
+         * For the most part we could leave these fields as zero with minimal
+         * effect, since this does not affect the values seen by the guest.
+         *
+         * However, it could cause problems down the line for QEMU,
+         * so provide a minimal v8.0 default.
+         *
+         * ??? Could read MIDR and use knowledge from cpu64.c.
+         * ??? Could map a page of memory into our temp guest and
+         *     run the tiniest of hand-crafted kernels to extract
+         *     the values seen by the guest.
+         * ??? Either of these sounds like too much effort just
+         *     to work around running a modern host kernel.
+         */
+        ahcf->isar.id_aa64pfr0 = 0x00000011; /* EL1&0, AArch64 only */
+        err = 0;
+    } else {
+        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr1,
+                              ARM64_SYS_REG(3, 0, 0, 4, 1));
+        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64isar0,
+                              ARM64_SYS_REG(3, 0, 0, 6, 0));
+        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64isar1,
+                              ARM64_SYS_REG(3, 0, 0, 6, 1));
+
+        /*
+         * Note that if AArch32 support is not present in the host,
+         * the AArch32 sysregs are present to be read, but will
+         * return UNKNOWN values.  This is neither better nor worse
+         * than skipping the reads and leaving 0, as we must avoid
+         * considering the values in every case.
+         */
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar0,
+                              ARM64_SYS_REG(3, 0, 0, 2, 0));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar1,
+                              ARM64_SYS_REG(3, 0, 0, 2, 1));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar2,
+                              ARM64_SYS_REG(3, 0, 0, 2, 2));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar3,
+                              ARM64_SYS_REG(3, 0, 0, 2, 3));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar4,
+                              ARM64_SYS_REG(3, 0, 0, 2, 4));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar5,
+                              ARM64_SYS_REG(3, 0, 0, 2, 5));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6,
+                              ARM64_SYS_REG(3, 0, 0, 2, 7));
+
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr0,
+                              ARM64_SYS_REG(3, 0, 0, 3, 0));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr1,
+                              ARM64_SYS_REG(3, 0, 0, 3, 1));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr2,
+                              ARM64_SYS_REG(3, 0, 0, 3, 2));
+    }
+
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
 
+    if (err < 0) {
+        return false;
+    }
+
    /* We can assume any KVM supporting CPU is at least a v8
      * with VFPv4+Neon; this in turn implies most of the other
      * feature bits.