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

[Eduardo Habkost]

On Wed, May 06, 2015 at 10:02:22AM +0200, Michael Mueller wrote:
> 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.

Right. So the data depends on (cpu_model, accel, machine), but we need to cache
the results for very few (just 2) accel+machine combinations at any given
moment. AccelMode makes more sense to me, now.

-- 
Eduardo