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

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:33:04PM +0100, Igor Mammedov wrote:
> On Fri, 21 Dec 2012 11:50:26 -0200
> Eduardo Habkost <address@hidden> 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.

OK, that would be a good reason to have a separate parser function.

[...]
> > > > > 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?

That's how I think we have to do it, but It would be interesting to not
require the caller to keep track of all those IDs and explicitly set
them on the device_add arguments. I believe this is similar to the
allocation of addresses in many device busses, so there may be an
existing solution for this that I don't know about.


> > [...]
> > > > > 
> > > > > 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/

True.

(That said, maybe we could even save the CPU model name and topology in
global properties so we could just use "device_add cpu-package" [with no
arguments] and have everything magically set [including the CPU model]
depending on the globals?)

-- 
Eduardo