[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featur
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs |
Date: |
Thu, 20 Dec 2012 21:22:49 +0100 |
On Thu, 20 Dec 2012 12:10:25 -0200
Eduardo Habkost <address@hidden> wrote:
It's a log list of answers. please look through them all to the end.
> On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote:
> > On Wed, 19 Dec 2012 14:54:30 -0200
> > Eduardo Habkost <address@hidden> wrote:
> >
> > > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote:
> > > > It prepares for converting "+feature,-feature,feature=foo,feature"
> > > > into a set of key,value property pairs that will be applied to CPU by
> > > > cpu_x86_set_props().
> > > >
> > > > Each feature handled by cpu_x86_parse_featurestr() will be converted
> > > > into foo,val pair and a corresponding property setter by following
> > > > patches.
> > > >
> > > > Signed-off-by: Igor Mammedov <address@hidden>
> > >
> > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU
> > > object as parameter and set the properties directly? Or you can see some
> > > use case where saving the property-setting dictionary for later use will
> > > be useful?
> >
> > I plan to use cpu_x86_parse_featurestr() + list of properties later when
> > we have properties and subclasses in place. Then it would be possible to
> > transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of
> > global properties.
>
> Is it really worth it to use global properties when you can simply set
> the properties at the code that handles the "-cpu" string (and already
> has to create the CPU object)?
>
> (more comments about globals are below)
>
>
> >
> > In the end the option handler for -cpu XXX,... could be changed into:
> >
> > cpu_opt_parser() {
> > // hides legacy target specific ugliness
> > target_XXX_cpu_compat_parser_callback() {
> > cpu_class_name = get_class_name(optval)
> >
> > // dumb compatibility parser, which converts old format into
> > // canonical form feature,val property list
> > prop_list = parse_featurestr(optval)
>
> I'm expecting it to look different: instead of a target-specific parsing
> function, you just need target-specific legacy properties on the CPU
> object, that would be set by the (generic) featurestr parsing code if
> present.
>
> (e.g. "legacy-xlevel" could be used for the legacy "1 becomes
> 0x80000001" behavior, and "xlevel" would be the more sane xlevel
> property without any magic).
Introducing 'legacy-xlevel' doesn't solve problem, because with new name
compatibility will be broken and user has to fix qemu invocation anyway.
The same effect could be achieved if qemu exits with invalid value error and
forces user to fix value on qemu invocation without introducing new
properties.
An idea of creating legacy properties would be a negative for following
reasons:
- new legacy properties with new names could break current users, and force
them to fix their code
+ converting old feature names into the same property names with same
behavior without adding new ones. But sometimes old behavior is wrong or
hard to convert into property, so not good for cpu model eventually.
- it burdens CPU object with properties that do almost the same as its
'correct' counterpart but not quite so. That complicates API of CPU
object a lot.
- more properties lead to more code, more invariants, resulting in
more complex behavior.
- increased maintenance (fixing/rewriting/testing/name it)
- maintenance will multiply by target amount that has legacy features that
do not map 1:1 into properties
Legacy format converter allows to mitigate or reduce above mention problems:
+ keeps CPU API simpler, new interfaces /QMP .../ see only CPU API and
expected not to behave in legacy way.
+ targets provide simple converter from command line format (could be
painlessly deprecated in future)
+ allows to centralize point of conversion in one place, using utilities
that target provides if it cares.
+ 'using global properties' and 'cpu_name=>class_name conversion in
converter' opens easy road to removing CPUxxxState.cpu_model_str and as
result simplifying CPUxxxState, copy_cpu() and cpu_xxx_init()
+ cpu_init() would be easier to unify/simplify and could be reduced to
object_new() + set instance specific properties() + realize()
- practically every QOMified CPU that would use unified cpu_init, should
provide as minimum converter cpu_name=>class_name, but it could be fixed
gradually per target.
- there will be ugly converter callback for a while, but it's temporary,
after it's deprecation users would be expected to use
foo-cpu-class,foo=xxx format
In summary I think it would be better to keep CPU object as simple as possible
with a stable API(a property set) for the every interface and convert legacy
command-line format into this API.
It will keep legacy code contained inside one function, which eventually
should disappear as legacy features are deprecated.
>
>
> >
> > // with classes and global properties we could get rid of the
> > field // cpu_model_str in CPUxxxState
> > return prop_list, cpu_class_name
> > }
> >
> > foreach (prop_list)
> > add_global_prop(cpu_class_name,prop,val)
> >
> > // could be later transformed to property of board object and
> > // we could get rid of one more global var
> > cpu_model = cpu_class_name
>
> The cpu_model string will be immediately used to create the CPU object
> just after this code runs. So do we really need to do the parsing before
> creating the CPU object and use global properties for that? It looks
> like unnecessary added complexity.
It might be used immediately or might be not in case of hot-plug or
copy_cpu() or depending where converter is called. Ideally there won't be
cpu_model string at all.
Presently all created CPUs use the same cpu_model string so parsing it
only once instead of N times would be already an improvement.
Pushing common features for type into global properties also makes sense,
isn't global properties made specifically for this?
If common features are pushed in global properties, cpu hot-plug could look
like:
device_add driver=cpu_x86_foo_class,[apicid|id]=xxx
and all common features that user has overridden on command line would
applied to a new cpu without extra work from user.
>
> > }
> >
> > >
> > > (This will probably require calling cpudef_2_x86_cpu() before
> > > cpu_x86_parse_featurestr(), and changing the existing
> > > cpu_x86_parse_featurestr() code to change the cpu object, not a
> > > x86_def_t struct, but I think this will simplify the logic a lot)
> > You cannot set/uset +-feat directly on CPU without breaking current
> > behavior where -feat overrides all +feat no matter in what order they are
> > specified. That's why dictionary is used to easily maintain "if(not feat)
> > ignore" logic and avoid duplication. Pls, look at commit
> > https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3
> > that converts +-feat into canonical feat=on/off form.
>
> Well, you can make feature parsing code could simply save the
> +feat/-feat results internally, and set/unset the properties directly at
> the CPU in the end. You can even use a dictionary internally, the point
> is that you don't need to expose the dictionary-based interface to the
> outside if not necessary (making the API much simpler, IMO).
dictionary outside of cpu_x86_parse_featurestr() is temporary,
until all features is converted into static properties, after that it could
be moved inside cpu_x86_parse_featurestr(), which would push custom properties
into global properties list and be called only once.
Additionally it would be possible to remove introduced here
cpu_x86_set_props(), simplifying cpu_x86_(init|create)().
I'd like to keep legacy handling in one compact place and as much as possible
separated from CPU object itself.
>
> >
> > And if features are applied directly to CPU, it would require to another
> > rewrite if global properties are to be used. Which IMHO should be
> > eventually done since -cpu ... looks like global parameters for a
> > specific cpu type.
>
> If we really are going to use global properties for the featurestring
> result, we will need to parse the featurestring before creating the CPU
> object, then you are correct.
>
> I'm questioning the need to use global properties for the
> featurestring parsing, if we can simply set the properties after
> creating the CPU object. I expect "-cpu" to be easily translatable to
> "-device" options, not to "-global" options.
>
> In other words, I expect this:
>
> -cpu foo,+xxx,-yyy
>
> to be translated in the future into something like:
>
> -device foo-x86-cpu,xxx=on,yyy=off
>
> and not to:
>
> -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu
>
> In other words, I disagree about "-cpu ..." looking like global
> parameters for a specific CPU type.
I can't agree with you here, on the contrary second option makes more sense
for me.
But both ways are lead to the same result and both ways will be possible to
use.
However if you look at current -cpu cpu_name,foo=xxx,... it is template for
all CPUs of type cpu_name, qemu is creating. That's maps perfectly into
global properties for a specific type.
If you are concerned with command line length the for single/several cpus
case then syntax "-device foo-cpu,xxx=on,yyy=off -device foo-cpu,.."
will be shorter, but with many cpus syntax:
-global foo-cpu.yyy=off -device foo-cpu ...
will be more compact.
And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage
of global properties as well.
I think "-cpu foo-cpu,foo=x -smp n -numa node,cpus=..." could be
translated into something like:
-global foo-cpu.foo=x
-device foo-cpu,id=a,node=nodeA
-device foo-cpu,id=b,node=nodeB
...
or
-device foo-cpu,id=f(cpuN,nodeN)
...
where common options are pushed into global properties and only instance
specific ones are specified per instance.
Do you see any issues ahead that global properties would introduce?
> >
[snip]
> > > > --
> > > > 1.7.1
> > > >
> > > >
> > >
> > > --
> > > Eduardo
> >
> >
> > --
> > Regards,
> > Igor
>
- [Qemu-devel] [PATCH 14/20] target-i386: set custom 'vendor' without intermediate x86_def_t, (continued)
- [Qemu-devel] [PATCH 14/20] target-i386: set custom 'vendor' without intermediate x86_def_t, Igor Mammedov, 2012/12/17
- [Qemu-devel] [PATCH 09/20] target-i386: move kvm_check_features_against_host() check to realize time, Igor Mammedov, 2012/12/17
- [Qemu-devel] [PATCH 13/20] target-i386: convert [cpuid_]vendor_override to bool, Igor Mammedov, 2012/12/17
- [Qemu-devel] [PATCH 06/20] target-i386: move out CPU features initialization in separate func, Igor Mammedov, 2012/12/17
- [Qemu-devel] [PATCH 05/20] target-i386: move setting defaults out of cpu_x86_parse_featurestr(), Igor Mammedov, 2012/12/17
- [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Igor Mammedov, 2012/12/17
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Eduardo Habkost, 2012/12/19
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Igor Mammedov, 2012/12/19
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Eduardo Habkost, 2012/12/20
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs,
Igor Mammedov <=
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Eduardo Habkost, 2012/12/20
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Igor Mammedov, 2012/12/20
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Eduardo Habkost, 2012/12/21
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Igor Mammedov, 2012/12/27
- Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs, Eduardo Habkost, 2012/12/27
[Qemu-devel] [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor(), Igor Mammedov, 2012/12/17