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