Re: [Qemu-devel] [PATCH 9/9] target-i386: set [+-]feature using static properties

[Igor Mammedov]

On Tue, 19 Feb 2013 16:03:04 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden>
> > ---
> >  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 <address@hidden>,
> >          mgs-id: <address@hidden>
> > 
> >  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