| Message ID | 20181024113709.16599-3-richard.henderson@linaro.org |
|---|---|
| State | New |
| 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/kvm64.c | 63 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 61 insertions(+), 2 deletions(-) > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 5de8ff0ac5..6ed80eadc2 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -443,17 +443,41 @@ 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; > + } > + assert(ret <= UINT32_MAX); In AArch64, there isn't really any such thing as a 32 bit sysreg. Where the Arm ARM refers to 32 bit system registers this is just a shorthand for "64-bit register where the top 32 bits are currently RES0". This assert() would result in QEMU crashing if we ever ran it on a host for some hypothetical future architecture which assigned meanings to some of the high bits. (Granted, this is unlikely for the specific case of the ID registers which track the AArch32 ID register values.) I think I would prefer it if we expanded the id_isar* fields in the ARMISARegisters struct to uint64_t. If you dislike that, I think we should make this code fail a bit more gracefully in the presence of an unexpected extension into the high bits of these registers. Or just ignore the high bits, since we're effectively trusting that future architecture versions use a compatible meaning for these registers anyway. > + *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); > +} > thanks -- PMM
On 10/29/18 2:58 PM, Peter Maydell wrote: > I think I would prefer it if we expanded the id_isar* fields > in the ARMISARegisters struct to uint64_t. If you dislike > that, I think we should make this code fail a bit more gracefully > in the presence of an unexpected extension into the high bits > of these registers. Or just ignore the high bits, since we're > effectively trusting that future architecture versions use > a compatible meaning for these registers anyway. Given these options, I'd prefer to just ignore the high bits. I'm fairly comfortable trusting that the architecture gods won't mess up and assign values in there. Otherwise they would have already expanded instead of adding ID_ISAR6. r~
On 29 October 2018 at 16:03, Richard Henderson <richard.henderson@linaro.org> wrote: > On 10/29/18 2:58 PM, Peter Maydell wrote: >> I think I would prefer it if we expanded the id_isar* fields >> in the ARMISARegisters struct to uint64_t. If you dislike >> that, I think we should make this code fail a bit more gracefully >> in the presence of an unexpected extension into the high bits >> of these registers. Or just ignore the high bits, since we're >> effectively trusting that future architecture versions use >> a compatible meaning for these registers anyway. > > Given these options, I'd prefer to just ignore the high bits. > I'm fairly comfortable trusting that the architecture gods > won't mess up and assign values in there. Otherwise they > would have already expanded instead of adding ID_ISAR6. OK with me. I think we could also add a comment noting that if the host CPU doesn't have AArch32 then the AArch32 sysregs still exist to be read, but they return UNKNOWN values. thanks -- PMM
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 5de8ff0ac5..6ed80eadc2 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -443,17 +443,41 @@ 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; + } + assert(ret <= UINT32_MAX); + *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 = 0; + /* 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 +498,43 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) ahcf->target = init.target; ahcf->dtb_compatible = "arm,arm-v8"; + 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)); + + 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)); + err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0, + ARM64_SYS_REG(3, 0, 0, 4, 0)); + err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr1, + ARM64_SYS_REG(3, 0, 0, 4, 1)); + 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.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/kvm64.c | 63 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 2 deletions(-)