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

[Andreas Färber]

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.

>   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.

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). 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.

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.

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