Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 21 Dec 2012 11:50:26 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Dec 21, 2012 at 01:56:56AM +0100, Igor Mammedov wrote:
> [...]
> > > All above said, I am not strongly against your approach, but I believe
> > > we could try to make the feature string parsing code generic and
> > > reusable (with target-specific properties or callbacks), instead of
> > > having to write a new generic feature string parsing function.
> > > 
> > > Anyway, the above is just bikeshedding to me, and your approach is an
> > > improvement, already, even if we try to make the parser more generic
> > > later.
> > >
> > feature string parser in this series isn't meant to be generic, it's just
> > a simplification of the current function and splitting parsing
> > features & setting properties into separate steps. kind of what you did with
> > cpu_name+feature_string split.
> > 
> > It's not clear to me what you mean under legacy properties, could you
> > throw in a hack using as example xlevel feature to demonstrate what you mean,
> > please?
> 
> I will try to show it below, but note that we can try to do that _after_
> we introduce the true/final/clean properties that won't have those
> hacks, and after changing the current parse_featurestr() implementation
> to contain the compatibility hacks (that's the work you are already
> doing).
> 
> I was thinking about using the "legacy-%s" feature inside
> qdev_prop_parse(), this way:
> 
> /* Generic featurestr parsing function */
> void generic_parse_featurestr(CPUClass *cc, const char *featurestr)
> {
>     opts = g_strsplit(featurestr, ",")
>     for (each opt in opts) {
>         if (opt[0] == '+') {
>             dict[opt+1] = 'true';
>         } else if (opt[0] == '
>             dict[opt+1] = 'false ;
>         } else if (opt contains '=) {
>             key, value = g_strsplit(opt, "=");
>             dict[key] = value;
>         }
>     }
It's only sparc and i386 targets that use +- format for features.
I'd rather avoid exposing +- format parsing to other targets.
BTW it's a bit more complex for i386 target due to f-* name conversion of
features into properties.

We should work towards deprecation of this format and not introducing it to
other targets. When all this legacy stuff is deprecated we should have only
foo=xxx format left as the other devices, then there won't be need in
dedicated parser.

 

>     for (each key in dict) {
>         /* qdev_prop_set_globals() uses qdev_prop_parse(), that already
>          * checks for a "legacy-<name>" property.
>          */
>         qdev_add_one_global(key, dict[key]);
>     }
> }
>
In addition, having per target converter makes deprecation easier and possible
to do on per target basis instead of fixing everything at once.

> 
> /* target-specific handling of legacy values for the -cpu option */
> void cpu_set_legacy_xlevel(X86CPU *cpu, Visitor *v, ...)
> {
>     int64_t i;
>     visit_type_int(v, &i, ...);
>     if (i < 0x80000000)
>         i += 0x80000000;
>     object_property_set_int(cpu, i, "xlevel");
> }
> 
> void cpu_set_legacy_foobar(...)
> {
>     ...
> }
> 
> void cpu_x86_class_init(CPUClass *cc, ...)
> {
>     ...
>     qdev_class_add_property("legacy-xlevel", cpu_set_legacy_xlevel, NULL, ...); /* only a setter, no getter */
>     qdev_class_add_property("legacy-foobar", cpu_set_legacy_foobar, NULL, ...); /* only a setter, no getter */
> }
> 
> 
> Note:
> 
> - We don't have a qdev_class_add_property() function, so we have to add
>   the legacy property to the ->props array;
> - The props array is based on name/offset/type, so we would need to
>   define (for example) a "PropertyInfo cpu_prop_xlevel" struct;
>   - On the other hand, setting a ->parse function on PropertyInfo gives
>     us the legacy-* property registration for free when registering
>     "xlevel" (see device_initfn()/qdev_property_add_legacy())
>   - But I would prefer a qdev_class_add_property() interface, as it
>     would be much simpler...
> 
> So, in the end it could be too complex due to the interfaces provided by
> qdev.
Concentrating legacy conversion to one function /especially if function
already exists/ and gradually srinking it to NOP looks a bit simpler 
than introducing new legacy-* code and/or qdev interfaces.

> 
> > 
> > > But the questions below about global properties seem important/interesting:
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > >         // 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.
> > > 
> > > I don't see the difference between storing a cpu_model string and
> > > storing a dictionary to be used later. It would just be a
> > > micro-optimization for the code that creates new CPUs.
> > Dictionary is an implementation detail, a temporary list of properties that
> > would be applied to global properties and then list destroyed. It is there now
> > because:
> >  1. it's just a convenient data structure to implement current weird +-foo
> >     semantic
> >  2. a necessary intermediate step to hold list of properties because
> >     properties/features is not yet converted into static properties. Properties
> >     could/should be pushed directly into global properties if possible (i.e.
> >     when property is static), then there won't be any need in intermediate
> >     property list at all.
> > 
> > There is not compelling reason to use dictionary in generic code, it could be
> > removed or be hidden inside current featurestr parser.
> 
> I wasn't taking into account that the dictionary would be simply used to
> feed the global property list, so nevermind.
> 
> > 
> > > 
> > > 
> > > > 
> > > > Presently all created CPUs use the same cpu_model string so parsing it
> > > > only once instead of N times would be already an improvement.
> > > 
> > > I don't believe we would want to make the code more complex just to
> > > optimize string parsing that is run a few times during QEMU
> > > initialization. That would be unnecessary optimization.
> > If we move generalized parsing out into vl.c & *-user/main.c option parsing
> > code, it would simplify every target. I would be better to do option parsing
> > at the time when options are parsed in a single place and let boards/targets to
> > concentrate on creating cpus (without caring about options at all, not really
> > their job).
> 
> Even if we did it the "inefficient" way (parsing it multiple times), it
> could be moved to generic CPU creation code.
> 
> Anyway, my assumption about "making the code more complex" doesn't apply
> if the dictionary is used only internally and we use global properties.
> 
> 
> > 
> > > 
> > > > 
> > > > Pushing common features for type into global properties also makes sense,
> > > > isn't global properties made specifically for this?
> > > 
> > > Yes, but do we want to make "-cpu" affect every single
> > > -device/device_add call for CPUs, or only the ones created on
> > > initialization? That's the main question, I believe.
> > If user overrides default cpu model features with -cpu option it would be only
> > logical for hot-plugged cpus to use that overrides as well, instead of creating
> > cpu that would be different (it may confuse guests, to the point of crashing).
> > IMHO, It would be surprising for user to get a different cpu on hot-plug than
> > already existing ones, in most cases.
> > 
> > And if user likes to shoot in its own foot, it will be possible to override
> > global features by adding extra foo=xxx to a specific device_add/-device
> > command.
> 
> This is a good argument, and I am now convinced that translating "-cpu"
> to global properties would be a good idea.  :)
> 
> 
> > > > 
> > > > 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.
> > > 
> > > CPU hotplug is an interesting case: if thinking only about the CPUs
> > > created from the command-line the "-global vs -device" question wouldn't
> > > make such a big difference. But in the case of device_add for CPU
> > > hotplug, it would be very different. But I am not sure which option is
> > > less surprising for users.
> > > 
> > > What others think? Andreas? Should "-cpu" affect only the CPUs created
> > > on initialization, or all CPUs created by -device/device_add for the
> > > lifetime of the QEMU process?  (in other words, should the options to
> > > "-cpu" be translated to "-device" arguments, or to "-global" arguments?)
> > > 
> > > (BTW about the device_add example above: I believe we will want the
> > > hotplug interface to be based on a "cpu_index" parameter/property, to
> > > abstract out the complexity of the APIC ID calculation)
> > there was other opinion about cpu_index
> > http://permalink.gmane.org/gmane.comp.emulators.qemu/151626
> >  
> > However if board will allocate sockets (Anthony suggested to try links for
> > this) then user won't have to calculate ID, instead of user will have to
> > enumerate existing cpu links and use IDs embedded in it for managing
> > cpus.
> 
> I don't care if we use "IDs", "links", a "cpu_index" property, socket
> objects, socket IDs, or whatever. I just strongly recommend we don't
> force users/management-tools to calculate the APIC ID themselves.
> 
> We could even kill the cpu_index field (as suggested at the URL above).
> But to allow the APIC ID to be calculated, we need to let the CPU know
> where it is "located" in the system (its
> cpu_index/socket_index/whatever), and the CPU topology of the system,
> somehow.
> 
> (I believe asked multiple times why devices can't know their parents by
> design [so they can't just ask the sockets/motherboard for the
> topology], and I never got an answer...)
Can we create core_id/package_id/node_id properties for CPU and use them when
creating/hot-plugging CPUs for APICID calculation?


> 
> 
> [...]
> > > > 
> > > > 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?
> > > 
> > > I see no issue, except that it was not what I was expecting at first. I
> > > just don't know which one is less surprising for users (including
> > > libvirt).
> > users shouldn't be affected, -cpu will continue to works the same way until we
> > start deprecating legacy behavior.
> 
> I mean: users will be affected by our decision once they start using
> device_add for CPU hotplug (then both approaches would have different
> results).
One more reason to make them global /i.e. to have the same behavior/

> 
> -- 
> Eduardo


-- 
Regards,
  Igor