qemu-devel
[Top][All Lists]
Advanced

[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: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs
Date: Fri, 21 Dec 2012 11:50:26 -0200
User-agent: Mutt/1.5.21 (2010-09-15)

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;
        }
    }
    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]);
    }
}


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


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


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

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]