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

[Eduardo Habkost]

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.

Or, maybe we shouldn't even add a "kvmclock" property at all, and
translate the legacy "kvmclock" flag to kvmclock1|kvmclock2 inside
parse_featurestr()?

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