Re: [Qemu-devel] [PATCH qom-cpu 00/17] x86 CPU cleanup, part 3

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 15 Jan 2013 05:16:52 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 11.01.2013 03:10, schrieb Igor Mammedov:
> > Igor Mammedov (17):
> >   target-i386: move setting defaults out of cpu_x86_parse_featurestr()
> >   target-i386: cpu_x86_register() consolidate freeing resources
> >   target-i386: move kvm_check_features_against_host() check to realize
> >     time
> 
> Thanks, I've applied 1-3 to qom-cpu:
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu
> 
> I really like patch 3, which moves more logic into our realizefn! When
> the QOM realize series gets applied, I have a queue prepared already to
> change today's signatures and to hook up to the infrastructure and to
> make this uniform across targets.
> 
> >   target-i386: add x86_cpu_vendor_words2str()
> 
> Patch 4 v3 looks okay too, but it mentions "next patch" and doesn't
> provide any value on its own:
> 
> >   target-i386: replace uint32_t vendor fields by vendor string in
> >     x86_def_t
> 
> I raised some (non-)issues on patch 5 that I would like to give others a
> chance to review rather than taking the lonely decision of putting this
> in a PULL tonight; a new idea there would be a union { uint32_t
> words[3]; char str[12]; } to keep both options open while avoiding the
> string -> 3x uint32_t -> string mess that patch 5 rightfully attempts to
> clean up. Once consensus is reached and it is still needed, I would
> prefer to squash patch 4 as justification, being a rather trivial code
> movement.
I'll try it.

> 
> >   target-i386: remove vendor_override field from CPUX86State
> >   target-i386: prepare cpu_x86_parse_featurestr() to return a set of
> >     key,value property pairs
> 
> Didn't get around to fully reviewing the remainder of the series yet.
Remainder of series is gradual conversion of currently existing properties
to foo,val pairs that set later by x86_cpu_set_props(). And following
static properties series will convert non property-fied features into
properties following the same pattern.

> 
> However I am still unclear and skeptic towards this patch using a QDict
> to set properties on the CPU. I was told this was for -feat and +feat
> backwards compatibility but the properties it is being used for are not
> features but regular properties that I would like to always set to get
> errors from each property rather than papering over, e.g.,
> "...,tsc_freq=invalid,tsc_freq=1G,..." (IIUC).
-feat and +feat is the only reason[1] to use dictionary as temporary storage
foo=val values, It could be hidden inside of cpu_x86_parse_featurestr()
and QList could be returned instead of it. That would achieve processing
of tsc_freq=invalid,tsc_freq=1G in sequence when setting properties and
failing on the first invalid tsc_freq or another similar case.

> By operating on the
> X86CPU, the intermediate x86_def_t can be spared too, as shown in my
> subclasses patch. Maybe I'm overlooking something, needs calm review
> from my side and some trial-and-error.

But main point of patch 7 is to separate parsing logic of command line
features (some of them legacy naming) from setting properties on actual
CPU instance.
Later when sub-classes are implemented and all features are converted into
static properties, x86_cpu_set_props() would be merged into
cpu_x86_parse_featurestr() and set global properties instead of operating
on CPU instance directly. After that cpu_x86_parse_featurestr() could be
called only once to set global properties for a given CPU type instead of
parsing the same featurestr for every CPU.

I could merge x86_cpu_set_props() into cpu_x86_parse_featurestr() and pass
cpu instance inside it temporally but I'd insist on first normalizing
featurestr into list of property pairs and then applying them to CPU.
It makes conversion to global properties trivial.

> 
> As a reminder, I have been pushing for converting to subclasses early
> because that is a prerequisite for doing some CPUClass-level changes
> across targets to clean up the way CPUs get created wrt cpu_init() and
> cpu_copy(). Feature properties are, as far as I have seen and heard, an
> x86-only project that could thus well be done on top of that common
> infrastructure IMO.
properties is x86-only project for now, but they make conversion to
sub-classes simpler and actually make x86 sub-classes meaningful.

We could push hard for properties and sub-classes to make in 1.4.


> 
> Regards,
> Andreas
> 
> >   target-i386: set custom 'vendor' without intermediate x86_def_t
> >   target-i386: print deprecated warning if xlevel < 0x80000000
> >   target-i386: set custom 'xlevel' without intermediate x86_def_t
> >   target-i386: set custom 'level' without intermediate x86_def_t
> >   target-i386: set custom 'model-id' without intermediate x86_def_t
> >   target-i386: set custom 'stepping' without intermediate x86_def_t
> >   target-i386: set custom 'model' without intermediate x86_def_t
> >   target-i386: set custom 'family' without intermediate x86_def_t
> >   target-i386: set custom 'tsc-frequency' without intermediate
> >     x86_def_t
> >   target-i386: remove setting tsc-frequency from x86_def_t
> > 
> >  target-i386/cpu.c |  314 ++++++++++++++++++++++-------------------------------
> >  target-i386/cpu.h |    7 +-
> >  2 files changed, 131 insertions(+), 190 deletions(-)
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Regards,
  Igor