From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42583) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yq1I6-0000eM-Ld for qemu-devel@nongnu.org; Wed, 06 May 2015 11:31:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yq1I3-0001QH-Fw for qemu-devel@nongnu.org; Wed, 06 May 2015 11:31:18 -0400 Received: from e06smtp10.uk.ibm.com ([195.75.94.106]:49077) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yq1I3-0001Q3-7h for qemu-devel@nongnu.org; Wed, 06 May 2015 11:31:15 -0400 Received: from /spool/local by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 6 May 2015 16:31:13 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 578F7219004D for ; Wed, 6 May 2015 16:30:53 +0100 (BST) Received: from d06av12.portsmouth.uk.ibm.com (d06av12.portsmouth.uk.ibm.com [9.149.37.247]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t46FVBUI4718998 for ; Wed, 6 May 2015 15:31:11 GMT Received: from d06av12.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av12.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t46FV9Je016962 for ; Wed, 6 May 2015 09:31:11 -0600 Date: Wed, 6 May 2015 17:31:06 +0200 From: Michael Mueller Message-ID: <20150506173106.7bf7262f@bee> In-Reply-To: <20150505184034.GW17796@thinpad.lan.raisama.net> References: <1430146411-34632-1-git-send-email-mimu@linux.vnet.ibm.com> <1430146411-34632-16-git-send-email-mimu@linux.vnet.ibm.com> <20150505184034.GW17796@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 15/17] target-s390x: Extend arch specific QMP command query-cpu-definitions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Cornelia Huck , Gleb Natapov , qemu-devel@nongnu.org, Alexander Graf , Christian Borntraeger , "Jason J. Herne" , Daniel Hansel , Paolo Bonzini , Andreas Faerber , Richard Henderson On Tue, 5 May 2015 15:40:34 -0300 Eduardo Habkost 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 > > >