Re: [Qemu-devel] [PATCH v6 15/17] target-s390x: Extend arch specific QMP command query-cpu-definitions

[Michael Mueller <mimu@linux.vnet.ibm.com>]

On Tue, 5 May 2015 15:40:34 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Apr 27, 2015 at 04:53:29PM +0200, Michael Mueller wrote:
> [...]
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 4d75ff0..94fede5 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -276,12 +276,59 @@ static int cpu_model_set(KVMState *s, uint64_t attr, void *addr)
> >      return rc;
> >  }
> >  
> > -static int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
> 
> This seems to duplicate lots of the existing KVM code. (See additional
> comment below about possible ways to avoid it).

some :-)

> 
> > +static int get_machine_props_fallback(S390MachineProps *prop)
> > +{
> > +    struct kvm_device_attr dev_attr;
> > +    int rc, kvmfd = -1, vmfd = -1;
> > +
> > +    rc  = qemu_open("/dev/kvm", O_RDWR);
> > +    if (rc < 0) {
> > +        goto out_err;
> > +    }
> > +    kvmfd = rc;
> > +
> > +    rc = ioctl(kvmfd, KVM_CREATE_VM, 0);
> > +    if (rc < 0) {
> > +        goto out_err;
> > +    }
> > +    vmfd = rc;
> > +
> > +    rc = ioctl(vmfd, KVM_CHECK_EXTENSION, KVM_CAP_VM_ATTRIBUTES);
> > +    if (rc < 0) {
> > +        rc = -ENOSYS;
> > +        goto out_err;
> > +    }
> > +
> > +    dev_attr.group = KVM_S390_VM_CPU_MODEL;
> > +    dev_attr.attr = KVM_S390_VM_CPU_MACHINE;
> > +    rc = ioctl(vmfd, KVM_HAS_DEVICE_ATTR, &dev_attr);
> > +    if (rc < 0) {
> > +        rc = -EFAULT;
> > +        goto out_err;
> > +    }
> > +
> > +    dev_attr.addr = (uint64_t) prop;
> > +    rc = ioctl(vmfd, KVM_GET_DEVICE_ATTR, &dev_attr);
> > +
> > +out_err:
> > +    if (vmfd >= 0) {
> > +        close(vmfd);
> > +    }
> > +    if (kvmfd >= 0) {
> > +        close(kvmfd);
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> > +int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
> >  {
> >      int rc = -EFAULT;
> >  
> >      if (s) {
> >          rc = cpu_model_get(s, KVM_S390_VM_CPU_MACHINE, prop);
> > +    } else {
> > +        rc = get_machine_props_fallback(prop);
> >      }
> 
> The comments below are just suggestions, not something which should
> block the patch, in my opinion:
> 
> First, if s is always NULL inside arch_query_cpu_definitions(), and is
> always non-NULL inside kvm_setup_cpu_classes(), why don't you just call
> keep the original kvm_s390_get_machine_props() function, and call and
> get_machine_props_fallback() inside arch_query_cpu_definitions()?

My reason for pulling both paths through the same internal interface call is
to have just single call for the same purpose.

> 
> The only thing common to both cases is the tracing point, but if we are
> running two completely different code paths I assume it would be a good
> thing to have a different tracing point for
> get_machine_props_fallback().
> 
> 
> Second, you shouldn't even need to duplicate code in
> get_machine_props_fallback() if you are able to create an accel object
> and do just basic initialization so that cpu_model_get() works.
> Allowing accel objects to be created on the fly was one of the main
> purposes of the accel QOM work.
> 
> For example, if we do something like this:
>   https://github.com/ehabkost/qemu-hacks/commit/36a250e34c5fd0d43a25271f5bc9b04681fdd56a [1]
>   https://github.com/ehabkost/qemu-hacks/commits/work/accel-open-func

I had a look at your qemu-hacks before writing the _fallback() routine
but did not wanted to base on some not yet published code. Once your part goes
upstream my intend is to provide a cleanup patch... And I was missing the
KVM_CREATE_VM actually.

> 
> Then the code could look like this:
> 
> accel.c:
> 
> /* configure_accelerator() would be changed to reuse this function: */
> AccelState *accel_create(const char *accel_name)
> {
>     AccelClass *acc = accel_find(accel_name);
>     /*TODO: error handling, checking acc->available() */
>     return ACCEL(object_new(object_class_get_name(OBJECT_CLASS(acc))));
> }
> 
> /* Do basic accel initialization without affecting global QEMU state */
> /* accel_init_machine() would be changed to reuse this function: */
> void accel_open(AccelState *s, Error **errp)
> {
>     object_property_set_bool(OBJECT(s), true, "open", errp);
> }
> 
> target-s390/kvm.c:
> 
> /* Using a different function name would be interesting, as it would be
>  * the main arch_query_cpu_definitions() code path, not a fallback.
>  */
> int get_machine_props_fallback(S390MachineProps *prop)
> {
>     int r;
>     AccelState *ac = accel_create("kvm");
>     /*TODO: error handling */
>     accel_open(ac, &err);
>     r = cpu_model_get(ac, prop);
>     object_unref(OBJECT(ac));
>     return r;
> }
> 
> 
> [1] I only moved the /dev/kvm opening to the open method, but maybe the
>     whole code up to KVM_CREATE_VM and capabitlity checking could be
>     moved.

Yes as mentioned above.

>     (But I don't know how to handle kvm_type, as it is currently
>     provided by MachineClass. Maybe kvm_type() belongs to CPUClass
>     instead of MachineClass?)
> 
> >      trace_kvm_get_machine_props(rc, prop->cpuid, prop->ibc);
> >  
> > -- 
> > 1.8.3.1
> > 
>