Re: [Qemu-devel] [PATCH v6 12/17] target-s390x: Add S390 CPU class initialization routines

[Michael Mueller]

On Tue, 5 May 2015 11:34:06 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden>
> > Acked-by: Christian Borntraeger <address@hidden>
> > ---
> [...]
> > +/**
> > + * 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);
> > +}
> > +
> [...]
>