From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51260) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpuHq-0005C2-Ei for qemu-devel@nongnu.org; Wed, 06 May 2015 04:02:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YpuHm-0007lm-93 for qemu-devel@nongnu.org; Wed, 06 May 2015 04:02:34 -0400 Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:52065) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpuHl-0007lL-W4 for qemu-devel@nongnu.org; Wed, 06 May 2015 04:02:30 -0400 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 6 May 2015 09:02:28 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id C959F17D8042 for ; Wed, 6 May 2015 09:03:10 +0100 (BST) Received: from d06av06.portsmouth.uk.ibm.com (d06av06.portsmouth.uk.ibm.com [9.149.37.217]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t4682Pjc64946236 for ; Wed, 6 May 2015 08:02:25 GMT Received: from d06av06.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av06.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t462uTgH019296 for ; Tue, 5 May 2015 22:56:30 -0400 Date: Wed, 6 May 2015 10:02:22 +0200 From: Michael Mueller Message-ID: <20150506100222.5f443ca3@bee> In-Reply-To: <20150505143406.GT17796@thinpad.lan.raisama.net> References: <1430146411-34632-1-git-send-email-mimu@linux.vnet.ibm.com> <1430146411-34632-13-git-send-email-mimu@linux.vnet.ibm.com> <20150505143406.GT17796@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 12/17] target-s390x: Add S390 CPU class initialization routines 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 11:34:06 -0300 Eduardo Habkost wrote: > On Mon, Apr 27, 2015 at 04:53:26PM +0200, Michael Mueller wrote: > > This patch provides routines to dynamically update the previously defined > > S390 CPU classes in the current host context. The main function performing > > this process is s390_setup_cpu_classes(). It takes the current host context > > and a facility list mask as parameter to setup the classes accordingly. It > > basically performs the following sub-tasks: > > > > - Update of CPU classes with accelerator specific host and QEMU properties > > - Mark adequate CPU class as default CPU class to be used for CPU model 'host' > > - Invalidate CPU classes not supported by this hosting machine > > - Define machine type aliases to latest GA number of a processor model > > - Define aliases for common CPU model names > > - Set CPU model alias 'host' to default CPU class > > > > Forthermore the patch provides the following routines: > > > > - cpu_desc_avail(), s390 specific stub indicating that list_cpus() can run > > - s390_setup_cpu_aliases(), adds cu model aliases > > - s390_cpu_classes_initialized(), test if CPU classes have been initialized > > - s390_fac_list_mask_by_machine(), returns facility list mask by machine > > - s390_current_fac_list_mask(), returns facility list mask of current machine > > > > Signed-off-by: Michael Mueller > > Acked-by: Christian Borntraeger > > --- > [...] > > +/** > > + * s390_setup_cpu_classes: > > + * @mode: the accelerator mode > > + * @prop: the machine property structure's address > > + * > > + * This function validates the defined cpu classes against the given > > + * machine properties @prop. Only cpu classes that are runnable on the > > + * current host will be set active. In addition the corresponding > > + * cpuid, ibc value and the active set of facilities will be > > + * initialized. Depending on @mode, the function porforms operations > > + * on the current or the temporary accelerator properies. > > + * > > + * Since: 2.4 > > + */ > > +void s390_setup_cpu_classes(S390AccelMode mode, S390MachineProps *prop, > > + uint64_t *fac_list_mask) > > +{ > > Can't you replace the S390AccelMode arguments everywhere with simply an > AccelState pointer? That's the kind of thing that should have been > easier to implement using the accel QOM stuff. Would just make sense in conjunction with an AccelId indexed array in the CPU class but see my concerns below. > > If you still need to save accel-specific data somewhere (like the > is_active, is_host and fac_list arrays), maybe it can be indexed using > the AccelId enum you have introduced, instead of S390AccelMode? I had an AccelId indexed array in a previous version of the patch but dismissed it in favor to this AccelMode index approach for the following reasons: a) There is just one accelerator active and and a second set of values is used for the query-cpu-definitions case. Using the AcceldId index would instantly double the required memory being used for no reason. The size of the second dimension in uint64_t fac_list[ACCEL_MODE_MAX][FAC_LIST_CPU_S390_SIZE_UINT64]; is architecturally allowed to grow up to 2KB. b) The information stored has more dimensions than just the accelerator, it also contains the selected machine (s390-virtio) which is represented by means of qemu_s390_fac_list_mask[] which currently is identical for all machines but that will change as the implementation progresses. So AccelMode (current, tmp) might also not fully express the semantics. Michael > > > + GSList *list; > > + ParmAddrAddrModeMask parm = { > > + .mode = mode, > > + .prop = prop, > > + .mask = fac_list_mask, > > + .host_cc = NULL, > > + }; > > + > > + list = object_class_get_list(TYPE_S390_CPU, false); > > + list = g_slist_sort(list, s390_cpu_class_asc_order_compare); > > + > > + g_slist_foreach(list, (GFunc) s390_update_cpu_class, (gpointer) &parm); > > + g_slist_foreach(list, (GFunc) s390_mark_host_cpu_class, (gpointer) &parm); > > + g_slist_foreach(list, (GFunc) s390_disable_not_supported_cpu_class, &parm); > > + > > + g_slist_free(list); > > +} > > + > [...] >