On Tue, 5 May 2015 11:34:06 -0300 Eduardo Habkost <ehabk...@redhat.com> 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 <m...@linux.vnet.ibm.com> > > Acked-by: Christian Borntraeger <borntrae...@de.ibm.com> > > --- > [...] > > +/** > > + * 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); > > +} > > + > [...] >