On Tue, 19 Feb 2013 16:03:04 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Mon, Feb 11, 2013 at 05:35:11PM +0100, Igor Mammedov wrote:
> > * Define static properties for cpuid feature bits
> > * property names of CPUID features are changed to have "f-" prefix,
> > so that it would be easy to distinguish them from other properties.
> >
> > * Convert [+-]cpuid_features to a set(QDict) of key, value pairs, where
> > +feat => (f-feat, on)
> > -feat => (f-feat, off)
> > [+-] unknown feat => (feat, on/off) - it's expected to be rejected
> > by property setter later
> > QDict is used as convinience sturcture to keep -foo semantic.
> > Once all +-foo are parsed, collected features are applied to CPU
> > instance.
> >
> > * Cleanup unused anymore:
> > add_flagname_to_bitmaps(), lookup_feature(), altcmp(), sstrcmp(),
> >
> > * Rename lowlevel 'kvmclock' into 'f-kvmclock1' and introduce
> > legacy composite property 'f-kvmclock' that sets both 'f-kvmclock1'
> > and 'f-kvmclock2' feature bits to mantain legacy kvmclock behavior
> >
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > ---
> > v4:
> > - major patch reordering, rebasing on current qom-cpu-next
> > - merged patches: "define static properties..." and "set [+-]feature..."
> > - merge in "make 'f-kvmclock' compatible..." to aviod behavior breakage
> > - use register name in feature macros, so that if we rename cpuid_*
> > fields,
> > it wouldn't involve mass rename in features array.
> > - when converting feature names into property names, replace '_' with
> > '-',
> > Requested-By: Don Slutz <d...@cloudswitch.com>,
> > mgs-id: <5085d4aa.1060...@cloudswitch.com>
> >
> > v3:
> > - new static properties after rebase:
> > - add features "f-rdseed" & "f-adx"
> > - add features added by c8acc380
> > - add features f-kvm_steal_tm and f-kvmclock_stable
> > - ext4 features f-xstore, f-xstore-en, f-xcrypt, f-xcrypt-en,
> > f-ace2, f-ace2-en, f-phe, f-phe-en, f-pmm, f-pmm-en
> >
> > - f-hypervisor set default to false as for the rest of features
> > - define helper FEAT for declaring CPUID feature properties to
> > make shorter lines (<80 characters)
> >
> > v2:
> > - use X86CPU as a type to count of offset correctly, because env field
> > isn't starting at CPUstate beginning, but located after it.
> > ---
> > target-i386/cpu.c | 348
> > +++++++++++++++++++++++++++++++++++++----------------
> > 1 files changed, 242 insertions(+), 106 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index fcfe8ec..2a5a5b5 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -656,6 +656,65 @@ PropertyInfo qdev_prop_enforce = {
> > .defval = _defval
> > \
> > }
> >
> > +static void x86_cpu_get_kvmclock(Object *obj, Visitor *v, void *opaque,
> > + const char *name, Error **errp)
> > +{
> > + X86CPU *cpu = X86_CPU(obj);
> > + bool value = cpu->env.cpuid_kvm_features;
> > + value = (value & KVM_FEATURE_CLOCKSOURCE) &&
> > + (value & KVM_FEATURE_CLOCKSOURCE2);
> > + visit_type_bool(v, &value, name, errp);
> > +}
> > +
> > +static void x86_cpu_set_kvmclock(Object *obj, Visitor *v, void *opaque,
> > + const char *name, Error **errp)
> > +{
> > + X86CPU *cpu = X86_CPU(obj);
> > + bool value;
> > + visit_type_bool(v, &value, name, errp);
> > + if (value == true) {
> > + cpu->env.cpuid_kvm_features |= KVM_FEATURE_CLOCKSOURCE |
> > + KVM_FEATURE_CLOCKSOURCE2;
> > + } else {
> > + cpu->env.cpuid_kvm_features &= ~(KVM_FEATURE_CLOCKSOURCE |
> > + KVM_FEATURE_CLOCKSOURCE2);
> > + }
> > +}
> > +
> > +PropertyInfo qdev_prop_kvmclock = {
> > + .name = "boolean",
> > + .get = x86_cpu_get_kvmclock,
> > + .set = x86_cpu_set_kvmclock,
> > +};
> > +#define DEFINE_PROP_KVMCLOCK(_n) {
> > \
> > + .name = _n,
> > \
> > + .info = &qdev_prop_kvmclock
> > \
> > +}
>
> Instead of the complexity of a new PropertyInfo struct, I would rather
> have a "enable_both_kvmclock_bits" field in X86CPU, that would be
> handled on x86_cpu_realize() and set the other feature bits. Then we
> could have a plain struct-field property with no special PropertyInfo
> struct.
No extra fields pls, unless we have to.
>
> Or, maybe we shouldn't even add a "kvmclock" property at all, and
> translate the legacy "kvmclock" flag to kvmclock1|kvmclock2 inside
> parse_featurestr()?
That's what I was proposing a while back. Lets do it this way.
>
> Except for that, patch looks good overall, but I still need to review
> the feature name list to make sure it matches exactly what have in the
> feature name arrays (I plan to review that later).
>
> --
> Eduardo
--
Regards,
Igor
