| Message ID | 20190621163422.6127-13-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm/kvm: enable SVE in guests | expand |
On 6/21/19 6:34 PM, Andrew Jones wrote: > kvm_arm_create_scratch_host_vcpu() takes a struct kvm_vcpu_init > parameter. Rather than just using it as an output parameter to > pass back the preferred target, use it also as an input parameter, > allowing a caller to pass a selected target if they wish and to > also pass cpu features. If the caller doesn't want to select a > target they can pass -1 for the target which indicates they want > to use the preferred target and have it passed back like before. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target/arm/kvm.c | 20 +++++++++++++++----- > target/arm/kvm32.c | 6 +++++- > target/arm/kvm64.c | 6 +++++- > 3 files changed, 25 insertions(+), 7 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Hi Drew, On 6/21/19 6:34 PM, Andrew Jones wrote: > kvm_arm_create_scratch_host_vcpu() takes a struct kvm_vcpu_init > parameter. Rather than just using it as an output parameter to > pass back the preferred target, use it also as an input parameter, > allowing a caller to pass a selected target if they wish and to > also pass cpu features. If the caller doesn't want to select a > target they can pass -1 for the target which indicates they want > to use the preferred target and have it passed back like before. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target/arm/kvm.c | 20 +++++++++++++++----- > target/arm/kvm32.c | 6 +++++- > target/arm/kvm64.c | 6 +++++- > 3 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index 60645a196d3d..66c0c198604a 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -64,7 +64,7 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > int *fdarray, > struct kvm_vcpu_init *init) > { > - int ret, kvmfd = -1, vmfd = -1, cpufd = -1; > + int ret = 0, kvmfd = -1, vmfd = -1, cpufd = -1; > > kvmfd = qemu_open("/dev/kvm", O_RDWR); > if (kvmfd < 0) { > @@ -84,7 +84,14 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > goto finish; > } > > - ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, init); > + if (init->target == -1) { > + struct kvm_vcpu_init preferred; > + > + ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, &preferred); > + if (!ret) { > + init->target = preferred.target; wouldn't it be safe to copy the whole struct. Kernel code says: /* * For now, we don't return any features. * In future, we might use features to return target * specific features available for the preferred * target type. */ > + } > + } > if (ret >= 0) { > ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init); > if (ret < 0) { > @@ -96,10 +103,12 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > * creating one kind of guest CPU which is its preferred > * CPU type. > */ > + struct kvm_vcpu_init try; > + > while (*cpus_to_try != QEMU_KVM_ARM_TARGET_NONE) { > - init->target = *cpus_to_try++; > - memset(init->features, 0, sizeof(init->features)); > - ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init); > + try.target = *cpus_to_try++; > + memcpy(try.features, init->features, sizeof(init->features)); > + ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, &try); > if (ret >= 0) { > break; > } > @@ -107,6 +116,7 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > if (ret < 0) { > goto err; > } > + init->target = try.target; > } else { > /* Treat a NULL cpus_to_try argument the same as an empty > * list, which means we will fail the call since this must > diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c > index 51f78f722b18..d007f6bd34d7 100644 > --- a/target/arm/kvm32.c > +++ b/target/arm/kvm32.c > @@ -54,7 +54,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > QEMU_KVM_ARM_TARGET_CORTEX_A15, > QEMU_KVM_ARM_TARGET_NONE > }; > - struct kvm_vcpu_init init; > + /* > + * target = -1 informs kvm_arm_create_scratch_host_vcpu() > + * to use the preferred target > + */ > + struct kvm_vcpu_init init = { .target = -1, }; > > if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) { > return false; > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 9fc7f078cf68..2821135a4d0e 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -502,7 +502,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > KVM_ARM_TARGET_CORTEX_A57, > QEMU_KVM_ARM_TARGET_NONE > }; > - struct kvm_vcpu_init init; > + /* > + * target = -1 informs kvm_arm_create_scratch_host_vcpu() > + * to use the preferred target > + */ > + struct kvm_vcpu_init init = { .target = -1, }; > > if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) { > return false; > Besides Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric
On Thu, Jun 27, 2019 at 09:30:57AM +0200, Auger Eric wrote: > Hi Drew, > > On 6/21/19 6:34 PM, Andrew Jones wrote: > > kvm_arm_create_scratch_host_vcpu() takes a struct kvm_vcpu_init > > parameter. Rather than just using it as an output parameter to > > pass back the preferred target, use it also as an input parameter, > > allowing a caller to pass a selected target if they wish and to > > also pass cpu features. If the caller doesn't want to select a > > target they can pass -1 for the target which indicates they want > > to use the preferred target and have it passed back like before. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > target/arm/kvm.c | 20 +++++++++++++++----- > > target/arm/kvm32.c | 6 +++++- > > target/arm/kvm64.c | 6 +++++- > > 3 files changed, 25 insertions(+), 7 deletions(-) > > > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > > index 60645a196d3d..66c0c198604a 100644 > > --- a/target/arm/kvm.c > > +++ b/target/arm/kvm.c > > @@ -64,7 +64,7 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > > int *fdarray, > > struct kvm_vcpu_init *init) > > { > > - int ret, kvmfd = -1, vmfd = -1, cpufd = -1; > > + int ret = 0, kvmfd = -1, vmfd = -1, cpufd = -1; > > > > kvmfd = qemu_open("/dev/kvm", O_RDWR); > > if (kvmfd < 0) { > > @@ -84,7 +84,14 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > > goto finish; > > } > > > > - ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, init); > > + if (init->target == -1) { > > + struct kvm_vcpu_init preferred; > > + > > + ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, &preferred); > > + if (!ret) { > > + init->target = preferred.target; > wouldn't it be safe to copy the whole struct. Kernel code says: > /* > * For now, we don't return any features. > * In future, we might use features to return target > * specific features available for the preferred > * target type. > */ Indeed, we could copy the features into 'preferred', in case KVM starts to do something with them. Based on that KVM comment, though, it looks like we'd need to make other changes as well to QEMU at that time. Some sort of feature matching/filtering code. It probably makes sense to do all that at once, when/if that time comes. > > > + } > > + } > > if (ret >= 0) { > > ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init); > > if (ret < 0) { > > @@ -96,10 +103,12 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > > * creating one kind of guest CPU which is its preferred > > * CPU type. > > */ > > + struct kvm_vcpu_init try; > > + > > while (*cpus_to_try != QEMU_KVM_ARM_TARGET_NONE) { > > - init->target = *cpus_to_try++; > > - memset(init->features, 0, sizeof(init->features)); > > - ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init); > > + try.target = *cpus_to_try++; > > + memcpy(try.features, init->features, sizeof(init->features)); > > + ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, &try); > > if (ret >= 0) { > > break; > > } > > @@ -107,6 +116,7 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > > if (ret < 0) { > > goto err; > > } > > + init->target = try.target; > > } else { > > /* Treat a NULL cpus_to_try argument the same as an empty > > * list, which means we will fail the call since this must > > diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c > > index 51f78f722b18..d007f6bd34d7 100644 > > --- a/target/arm/kvm32.c > > +++ b/target/arm/kvm32.c > > @@ -54,7 +54,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > > QEMU_KVM_ARM_TARGET_CORTEX_A15, > > QEMU_KVM_ARM_TARGET_NONE > > }; > > - struct kvm_vcpu_init init; > > + /* > > + * target = -1 informs kvm_arm_create_scratch_host_vcpu() > > + * to use the preferred target > > + */ > > + struct kvm_vcpu_init init = { .target = -1, }; > > > > if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) { > > return false; > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > > index 9fc7f078cf68..2821135a4d0e 100644 > > --- a/target/arm/kvm64.c > > +++ b/target/arm/kvm64.c > > @@ -502,7 +502,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > > KVM_ARM_TARGET_CORTEX_A57, > > QEMU_KVM_ARM_TARGET_NONE > > }; > > - struct kvm_vcpu_init init; > > + /* > > + * target = -1 informs kvm_arm_create_scratch_host_vcpu() > > + * to use the preferred target > > + */ > > + struct kvm_vcpu_init init = { .target = -1, }; > > > > if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) { > > return false; > > > > Besides > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > Thanks, drew
On Thu, Jun 27, 2019 at 08:30:57AM +0100, Auger Eric wrote: > Hi Drew, > > On 6/21/19 6:34 PM, Andrew Jones wrote: > > kvm_arm_create_scratch_host_vcpu() takes a struct kvm_vcpu_init > > parameter. Rather than just using it as an output parameter to > > pass back the preferred target, use it also as an input parameter, > > allowing a caller to pass a selected target if they wish and to > > also pass cpu features. If the caller doesn't want to select a > > target they can pass -1 for the target which indicates they want > > to use the preferred target and have it passed back like before. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > target/arm/kvm.c | 20 +++++++++++++++----- > > target/arm/kvm32.c | 6 +++++- > > target/arm/kvm64.c | 6 +++++- > > 3 files changed, 25 insertions(+), 7 deletions(-) > > > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > > index 60645a196d3d..66c0c198604a 100644 > > --- a/target/arm/kvm.c > > +++ b/target/arm/kvm.c > > @@ -64,7 +64,7 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > > int *fdarray, > > struct kvm_vcpu_init *init) > > { > > - int ret, kvmfd = -1, vmfd = -1, cpufd = -1; > > + int ret = 0, kvmfd = -1, vmfd = -1, cpufd = -1; > > > > kvmfd = qemu_open("/dev/kvm", O_RDWR); > > if (kvmfd < 0) { > > @@ -84,7 +84,14 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > > goto finish; > > } > > > > - ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, init); > > + if (init->target == -1) { > > + struct kvm_vcpu_init preferred; > > + > > + ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, &preferred); > > + if (!ret) { > > + init->target = preferred.target; > wouldn't it be safe to copy the whole struct. Kernel code says: > /* > * For now, we don't return any features. > * In future, we might use features to return target > * specific features available for the preferred > * target type. > */ Marc or Christoffer should preferably comment on this. I think the spirit of the ABI is that can use the whole return of KVM_ARM_PREFERRED_TARGET as a reasonable template for KVM_VCPU_INIT without it blowing up in your face. I initially tried to report SVE as available through this route, but we decided against it precisely because userspace might be doing the above. [...] Cheers ---Dave
diff --git a/target/arm/kvm.c b/target/arm/kvm.c index 60645a196d3d..66c0c198604a 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -64,7 +64,7 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, int *fdarray, struct kvm_vcpu_init *init) { - int ret, kvmfd = -1, vmfd = -1, cpufd = -1; + int ret = 0, kvmfd = -1, vmfd = -1, cpufd = -1; kvmfd = qemu_open("/dev/kvm", O_RDWR); if (kvmfd < 0) { @@ -84,7 +84,14 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, goto finish; } - ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, init); + if (init->target == -1) { + struct kvm_vcpu_init preferred; + + ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, &preferred); + if (!ret) { + init->target = preferred.target; + } + } if (ret >= 0) { ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init); if (ret < 0) { @@ -96,10 +103,12 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, * creating one kind of guest CPU which is its preferred * CPU type. */ + struct kvm_vcpu_init try; + while (*cpus_to_try != QEMU_KVM_ARM_TARGET_NONE) { - init->target = *cpus_to_try++; - memset(init->features, 0, sizeof(init->features)); - ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init); + try.target = *cpus_to_try++; + memcpy(try.features, init->features, sizeof(init->features)); + ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, &try); if (ret >= 0) { break; } @@ -107,6 +116,7 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, if (ret < 0) { goto err; } + init->target = try.target; } else { /* Treat a NULL cpus_to_try argument the same as an empty * list, which means we will fail the call since this must diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c index 51f78f722b18..d007f6bd34d7 100644 --- a/target/arm/kvm32.c +++ b/target/arm/kvm32.c @@ -54,7 +54,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) QEMU_KVM_ARM_TARGET_CORTEX_A15, QEMU_KVM_ARM_TARGET_NONE }; - struct kvm_vcpu_init init; + /* + * target = -1 informs kvm_arm_create_scratch_host_vcpu() + * to use the preferred target + */ + struct kvm_vcpu_init init = { .target = -1, }; if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) { return false; diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 9fc7f078cf68..2821135a4d0e 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -502,7 +502,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) KVM_ARM_TARGET_CORTEX_A57, QEMU_KVM_ARM_TARGET_NONE }; - struct kvm_vcpu_init init; + /* + * target = -1 informs kvm_arm_create_scratch_host_vcpu() + * to use the preferred target + */ + struct kvm_vcpu_init init = { .target = -1, }; if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) { return false;
kvm_arm_create_scratch_host_vcpu() takes a struct kvm_vcpu_init parameter. Rather than just using it as an output parameter to pass back the preferred target, use it also as an input parameter, allowing a caller to pass a selected target if they wish and to also pass cpu features. If the caller doesn't want to select a target they can pass -1 for the target which indicates they want to use the preferred target and have it passed back like before. Signed-off-by: Andrew Jones <drjones@redhat.com> --- target/arm/kvm.c | 20 +++++++++++++++----- target/arm/kvm32.c | 6 +++++- target/arm/kvm64.c | 6 +++++- 3 files changed, 25 insertions(+), 7 deletions(-)