Re: [Qemu-devel] [PATCH v2 12/14] target/arm/kvm: scratch vcpu: Preserve input kvm_vcpu_init features

[Andrew Jones]

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 <address@hidden>
> > ---
> >  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 <address@hidden>
>

Thanks,
drew