| Message ID | 1344597756-2890-12-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > target-i386/cpu.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 57 insertions(+), 11 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 7734613..a154e89 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -106,8 +106,8 @@ typedef struct model_features_t { > uint32_t cpuid; > } model_features_t; > > -int check_cpuid = 0; > -int enforce_cpuid = 0; > +bool check_cpuid; > +bool enforce_cpuid; > > void host_cpuid(uint32_t function, uint32_t count, > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > @@ -579,19 +579,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) > * their way to the guest. Note: ft[].check_feat ideally should be > * specified via a guest_def field to suppress report of extraneous flags. > */ > -static int check_features_against_host(x86_def_t *guest_def) > +static int check_features_against_host(X86CPU *cpu) > { > + CPUX86State *env = &cpu->env; > x86_def_t host_def; > uint32_t mask; > int rv, i; > struct model_features_t ft[] = { > - {&guest_def->features, &host_def.features, > + {&env->cpuid_features, &host_def.features, > ~0, feature_name, 0x00000000}, > - {&guest_def->ext_features, &host_def.ext_features, > + {&env->cpuid_ext_features, &host_def.ext_features, > ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001}, > - {&guest_def->ext2_features, &host_def.ext2_features, > + {&env->cpuid_ext2_features, &host_def.ext2_features, > ~PPRO_FEATURES, ext2_feature_name, 0x80000000}, > - {&guest_def->ext3_features, &host_def.ext3_features, > + {&env->cpuid_ext3_features, &host_def.ext3_features, > ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}}; > > cpu_x86_fill_host(&host_def); > @@ -1030,6 +1031,43 @@ static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque, > } > #endif > > +static void x86_cpuid_get_check(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + visit_type_bool(v, &check_cpuid, name, errp); > +} > + > +static void x86_cpuid_set_check(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + bool value; > + > + visit_type_bool(v, &value, name, errp); > + if (error_is_set(errp)) { > + return; > + } > + check_cpuid = value; > +} > + > +static void x86_cpuid_get_enforce(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + visit_type_bool(v, &enforce_cpuid, name, errp); > +} > + > +static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + bool value; > + > + visit_type_bool(v, &value, name, errp); > + if (error_is_set(errp)) { > + return; > + } > + enforce_cpuid = value; > + object_property_set_bool(obj, value, "check", errp); > +} > + > static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp) > { > CPUX86State *env = &cpu->env; > @@ -1225,10 +1263,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, > x86_cpu_def->ext3_features &= ~minus_ext3_features; > x86_cpu_def->kvm_features &= ~minus_kvm_features; > x86_cpu_def->svm_features &= ~minus_svm_features; > - if (check_cpuid) { > - if (check_features_against_host(x86_cpu_def) && enforce_cpuid) > - goto error; > - } > g_free(s); > return 0; > > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp) > env->cpuid_svm_features &= TCG_SVM_FEATURES; > } > > + if (check_cpuid && check_features_against_host(cpu) > + && enforce_cpuid) { > + error_set(errp, QERR_PERMISSION_DENIED); > + return; > + } > + I just noticed that you changed behavior on patch 04/20 and now restore the behavior in this patch: - Before patch 04/20, the feature check was being done after the features were filtered according to the TCG support (meaning a feature not supported by TCG will not trigger enforce/check errors). - After patch 04/20, the check was being done _before_ the features were filtered according to TCG support (meaning a feature not supported by TCG would trigger enforce/check errors). - With this patch, the old behavior is restored. I'm not sure which behavior is better. But we surely shouldn't silently move back and forth between those two modes. IMO, checking _before_ the TCG filtering is better, as it's more predictable. it means having lots of warnings in case too-new CPU models are chosen in TCG model, but that's exactly the point. > #ifndef CONFIG_USER_ONLY > qemu_register_reset(x86_cpu_machine_reset_cb, cpu); > #endif > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj) > object_property_add(obj, "tsc-frequency", "int", > x86_cpuid_get_tsc_freq, > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > + object_property_add(obj, "check", "bool", > + x86_cpuid_get_check, > + x86_cpuid_set_check, NULL, NULL, NULL); > + object_property_add(obj, "enforce", "bool", > + x86_cpuid_get_enforce, > + x86_cpuid_set_enforce, NULL, NULL, NULL); > #if !defined(CONFIG_USER_ONLY) > object_property_add(obj, "hv_spinlocks", "int", > x86_get_hv_spinlocks, > -- > 1.7.11.2 >
On Fri, 10 Aug 2012 12:09:04 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > target-i386/cpu.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 57 insertions(+), 11 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 7734613..a154e89 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -106,8 +106,8 @@ typedef struct model_features_t { > > uint32_t cpuid; > > } model_features_t; > > > > -int check_cpuid = 0; > > -int enforce_cpuid = 0; > > +bool check_cpuid; > > +bool enforce_cpuid; > > > > void host_cpuid(uint32_t function, uint32_t count, > > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > > @@ -579,19 +579,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) > > * their way to the guest. Note: ft[].check_feat ideally should be > > * specified via a guest_def field to suppress report of extraneous flags. > > */ > > -static int check_features_against_host(x86_def_t *guest_def) > > +static int check_features_against_host(X86CPU *cpu) > > { > > + CPUX86State *env = &cpu->env; > > x86_def_t host_def; > > uint32_t mask; > > int rv, i; > > struct model_features_t ft[] = { > > - {&guest_def->features, &host_def.features, > > + {&env->cpuid_features, &host_def.features, > > ~0, feature_name, 0x00000000}, > > - {&guest_def->ext_features, &host_def.ext_features, > > + {&env->cpuid_ext_features, &host_def.ext_features, > > ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001}, > > - {&guest_def->ext2_features, &host_def.ext2_features, > > + {&env->cpuid_ext2_features, &host_def.ext2_features, > > ~PPRO_FEATURES, ext2_feature_name, 0x80000000}, > > - {&guest_def->ext3_features, &host_def.ext3_features, > > + {&env->cpuid_ext3_features, &host_def.ext3_features, > > ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}}; > > > > cpu_x86_fill_host(&host_def); > > @@ -1030,6 +1031,43 @@ static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque, > > } > > #endif > > > > +static void x86_cpuid_get_check(Object *obj, Visitor *v, void *opaque, > > + const char *name, Error **errp) > > +{ > > + visit_type_bool(v, &check_cpuid, name, errp); > > +} > > + > > +static void x86_cpuid_set_check(Object *obj, Visitor *v, void *opaque, > > + const char *name, Error **errp) > > +{ > > + bool value; > > + > > + visit_type_bool(v, &value, name, errp); > > + if (error_is_set(errp)) { > > + return; > > + } > > + check_cpuid = value; > > +} > > + > > +static void x86_cpuid_get_enforce(Object *obj, Visitor *v, void *opaque, > > + const char *name, Error **errp) > > +{ > > + visit_type_bool(v, &enforce_cpuid, name, errp); > > +} > > + > > +static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque, > > + const char *name, Error **errp) > > +{ > > + bool value; > > + > > + visit_type_bool(v, &value, name, errp); > > + if (error_is_set(errp)) { > > + return; > > + } > > + enforce_cpuid = value; > > + object_property_set_bool(obj, value, "check", errp); > > +} > > + > > static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp) > > { > > CPUX86State *env = &cpu->env; > > @@ -1225,10 +1263,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, > > x86_cpu_def->ext3_features &= ~minus_ext3_features; > > x86_cpu_def->kvm_features &= ~minus_kvm_features; > > x86_cpu_def->svm_features &= ~minus_svm_features; > > - if (check_cpuid) { > > - if (check_features_against_host(x86_cpu_def) && enforce_cpuid) > > - goto error; > > - } > > g_free(s); > > return 0; > > > > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp) > > env->cpuid_svm_features &= TCG_SVM_FEATURES; > > } > > > > + if (check_cpuid && check_features_against_host(cpu) > > + && enforce_cpuid) { > > + error_set(errp, QERR_PERMISSION_DENIED); > > + return; > > + } > > + > > I just noticed that you changed behavior on patch 04/20 and now restore > the behavior in this patch: > > - Before patch 04/20, the feature check was being done after the > features were filtered according to the TCG support (meaning a feature > not supported by TCG will not trigger enforce/check errors). before 04/20 check is done before TCG features filtering in cpu_x86_find_by_name() and then later in cpu_x86_register() features are TCG filtered. > - After patch 04/20, the check was being done _before_ the features were > filtered according to TCG support (meaning a feature not supported by > TCG would trigger enforce/check errors). > - With this patch, the old behavior is restored. after 04/20, it is the same as before, i.e realize is called after cpu_x86_register(). it's by mistake that in this patch I've put check after TCG filtering, I'll fix it and do check before it. > > I'm not sure which behavior is better. But we surely shouldn't silently > move back and forth between those two modes. > > IMO, checking _before_ the TCG filtering is better, as it's more > predictable. it means having lots of warnings in case too-new CPU models > are chosen in TCG model, but that's exactly the point. > > > > #ifndef CONFIG_USER_ONLY > > qemu_register_reset(x86_cpu_machine_reset_cb, cpu); > > #endif > > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj) > > object_property_add(obj, "tsc-frequency", "int", > > x86_cpuid_get_tsc_freq, > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > > + object_property_add(obj, "check", "bool", > > + x86_cpuid_get_check, > > + x86_cpuid_set_check, NULL, NULL, NULL); > > + object_property_add(obj, "enforce", "bool", > > + x86_cpuid_get_enforce, > > + x86_cpuid_set_enforce, NULL, NULL, NULL); > > #if !defined(CONFIG_USER_ONLY) > > object_property_add(obj, "hv_spinlocks", "int", > > x86_get_hv_spinlocks, > > -- > > 1.7.11.2 > > > > -- > Eduardo
On Tue, Aug 14, 2012 at 11:18:47PM +0200, Igor Mammedov wrote: > On Fri, 10 Aug 2012 12:09:04 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > target-i386/cpu.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--------- > > > 1 file changed, 57 insertions(+), 11 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 7734613..a154e89 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -106,8 +106,8 @@ typedef struct model_features_t { > > > uint32_t cpuid; > > > } model_features_t; > > > > > > -int check_cpuid = 0; > > > -int enforce_cpuid = 0; > > > +bool check_cpuid; > > > +bool enforce_cpuid; > > > > > > void host_cpuid(uint32_t function, uint32_t count, > > > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > > > @@ -579,19 +579,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) > > > * their way to the guest. Note: ft[].check_feat ideally should be > > > * specified via a guest_def field to suppress report of extraneous flags. > > > */ > > > -static int check_features_against_host(x86_def_t *guest_def) > > > +static int check_features_against_host(X86CPU *cpu) > > > { > > > + CPUX86State *env = &cpu->env; > > > x86_def_t host_def; > > > uint32_t mask; > > > int rv, i; > > > struct model_features_t ft[] = { > > > - {&guest_def->features, &host_def.features, > > > + {&env->cpuid_features, &host_def.features, > > > ~0, feature_name, 0x00000000}, > > > - {&guest_def->ext_features, &host_def.ext_features, > > > + {&env->cpuid_ext_features, &host_def.ext_features, > > > ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001}, > > > - {&guest_def->ext2_features, &host_def.ext2_features, > > > + {&env->cpuid_ext2_features, &host_def.ext2_features, > > > ~PPRO_FEATURES, ext2_feature_name, 0x80000000}, > > > - {&guest_def->ext3_features, &host_def.ext3_features, > > > + {&env->cpuid_ext3_features, &host_def.ext3_features, > > > ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}}; > > > > > > cpu_x86_fill_host(&host_def); > > > @@ -1030,6 +1031,43 @@ static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque, > > > } > > > #endif > > > > > > +static void x86_cpuid_get_check(Object *obj, Visitor *v, void *opaque, > > > + const char *name, Error **errp) > > > +{ > > > + visit_type_bool(v, &check_cpuid, name, errp); > > > +} > > > + > > > +static void x86_cpuid_set_check(Object *obj, Visitor *v, void *opaque, > > > + const char *name, Error **errp) > > > +{ > > > + bool value; > > > + > > > + visit_type_bool(v, &value, name, errp); > > > + if (error_is_set(errp)) { > > > + return; > > > + } > > > + check_cpuid = value; > > > +} > > > + > > > +static void x86_cpuid_get_enforce(Object *obj, Visitor *v, void *opaque, > > > + const char *name, Error **errp) > > > +{ > > > + visit_type_bool(v, &enforce_cpuid, name, errp); > > > +} > > > + > > > +static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque, > > > + const char *name, Error **errp) > > > +{ > > > + bool value; > > > + > > > + visit_type_bool(v, &value, name, errp); > > > + if (error_is_set(errp)) { > > > + return; > > > + } > > > + enforce_cpuid = value; > > > + object_property_set_bool(obj, value, "check", errp); > > > +} > > > + > > > static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp) > > > { > > > CPUX86State *env = &cpu->env; > > > @@ -1225,10 +1263,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, > > > x86_cpu_def->ext3_features &= ~minus_ext3_features; > > > x86_cpu_def->kvm_features &= ~minus_kvm_features; > > > x86_cpu_def->svm_features &= ~minus_svm_features; > > > - if (check_cpuid) { > > > - if (check_features_against_host(x86_cpu_def) && enforce_cpuid) > > > - goto error; > > > - } > > > g_free(s); > > > return 0; > > > > > > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp) > > > env->cpuid_svm_features &= TCG_SVM_FEATURES; > > > } > > > > > > + if (check_cpuid && check_features_against_host(cpu) > > > + && enforce_cpuid) { > > > + error_set(errp, QERR_PERMISSION_DENIED); > > > + return; > > > + } > > > + > > > > I just noticed that you changed behavior on patch 04/20 and now restore > > the behavior in this patch: > > > > - Before patch 04/20, the feature check was being done after the > > features were filtered according to the TCG support (meaning a feature > > not supported by TCG will not trigger enforce/check errors). > before 04/20 check is done before TCG features filtering in > cpu_x86_find_by_name() and then later in cpu_x86_register() features are > TCG filtered. Right, my mistake. The check was done on cpu_x86_find_by_name() (before the filtering), but for some reason I was thinking it was done much later. Nevermind. > > > - After patch 04/20, the check was being done _before_ the features were > > filtered according to TCG support (meaning a feature not supported by > > TCG would trigger enforce/check errors). > > - With this patch, the old behavior is restored. > after 04/20, it is the same as before, i.e realize is called after > cpu_x86_register(). > > it's by mistake that in this patch I've put check after TCG filtering, I'll > fix it and do check before it. Good. I think it's a good idea to make the check before the filtering (and eventually, instead of checking the host CPU features directly, the check on TCG mode should be based on TCG_FEATURES, and the check on KVM mode should be based on GET_SUPPORTED_CPUID, but that's another issue). > > > > I'm not sure which behavior is better. But we surely shouldn't silently > > move back and forth between those two modes. > > > > IMO, checking _before_ the TCG filtering is better, as it's more > > predictable. it means having lots of warnings in case too-new CPU models > > are chosen in TCG model, but that's exactly the point. > > > > > > > #ifndef CONFIG_USER_ONLY > > > qemu_register_reset(x86_cpu_machine_reset_cb, cpu); > > > #endif > > > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj) > > > object_property_add(obj, "tsc-frequency", "int", > > > x86_cpuid_get_tsc_freq, > > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > > > + object_property_add(obj, "check", "bool", > > > + x86_cpuid_get_check, > > > + x86_cpuid_set_check, NULL, NULL, NULL); > > > + object_property_add(obj, "enforce", "bool", > > > + x86_cpuid_get_enforce, > > > + x86_cpuid_set_enforce, NULL, NULL, NULL); > > > #if !defined(CONFIG_USER_ONLY) > > > object_property_add(obj, "hv_spinlocks", "int", > > > x86_get_hv_spinlocks, > > > -- > > > 1.7.11.2 > > > > > > > -- > > Eduardo > > > -- > Regards, > Igor
On Wed, 15 Aug 2012 08:39:54 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Aug 14, 2012 at 11:18:47PM +0200, Igor Mammedov wrote: > > On Fri, 10 Aug 2012 12:09:04 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote: > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > target-i386/cpu.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--------- > > > > 1 file changed, 57 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > index 7734613..a154e89 100644 > > > > --- a/target-i386/cpu.c > > > > +++ b/target-i386/cpu.c > > > > @@ -106,8 +106,8 @@ typedef struct model_features_t { > > > > uint32_t cpuid; > > > > } model_features_t; > > > > > > > > -int check_cpuid = 0; > > > > -int enforce_cpuid = 0; > > > > +bool check_cpuid; > > > > +bool enforce_cpuid; > > > > > > > > void host_cpuid(uint32_t function, uint32_t count, > > > > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > > > > @@ -579,19 +579,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) > > > > * their way to the guest. Note: ft[].check_feat ideally should be > > > > * specified via a guest_def field to suppress report of extraneous flags. > > > > */ > > > > -static int check_features_against_host(x86_def_t *guest_def) > > > > +static int check_features_against_host(X86CPU *cpu) > > > > { > > > > + CPUX86State *env = &cpu->env; > > > > x86_def_t host_def; > > > > uint32_t mask; > > > > int rv, i; > > > > struct model_features_t ft[] = { > > > > - {&guest_def->features, &host_def.features, > > > > + {&env->cpuid_features, &host_def.features, > > > > ~0, feature_name, 0x00000000}, > > > > - {&guest_def->ext_features, &host_def.ext_features, > > > > + {&env->cpuid_ext_features, &host_def.ext_features, > > > > ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001}, > > > > - {&guest_def->ext2_features, &host_def.ext2_features, > > > > + {&env->cpuid_ext2_features, &host_def.ext2_features, > > > > ~PPRO_FEATURES, ext2_feature_name, 0x80000000}, > > > > - {&guest_def->ext3_features, &host_def.ext3_features, > > > > + {&env->cpuid_ext3_features, &host_def.ext3_features, > > > > ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}}; > > > > > > > > cpu_x86_fill_host(&host_def); > > > > @@ -1030,6 +1031,43 @@ static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque, > > > > } > > > > #endif > > > > > > > > +static void x86_cpuid_get_check(Object *obj, Visitor *v, void *opaque, > > > > + const char *name, Error **errp) > > > > +{ > > > > + visit_type_bool(v, &check_cpuid, name, errp); > > > > +} > > > > + > > > > +static void x86_cpuid_set_check(Object *obj, Visitor *v, void *opaque, > > > > + const char *name, Error **errp) > > > > +{ > > > > + bool value; > > > > + > > > > + visit_type_bool(v, &value, name, errp); > > > > + if (error_is_set(errp)) { > > > > + return; > > > > + } > > > > + check_cpuid = value; > > > > +} > > > > + > > > > +static void x86_cpuid_get_enforce(Object *obj, Visitor *v, void *opaque, > > > > + const char *name, Error **errp) > > > > +{ > > > > + visit_type_bool(v, &enforce_cpuid, name, errp); > > > > +} > > > > + > > > > +static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque, > > > > + const char *name, Error **errp) > > > > +{ > > > > + bool value; > > > > + > > > > + visit_type_bool(v, &value, name, errp); > > > > + if (error_is_set(errp)) { > > > > + return; > > > > + } > > > > + enforce_cpuid = value; > > > > + object_property_set_bool(obj, value, "check", errp); > > > > +} > > > > + > > > > static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp) > > > > { > > > > CPUX86State *env = &cpu->env; > > > > @@ -1225,10 +1263,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, > > > > x86_cpu_def->ext3_features &= ~minus_ext3_features; > > > > x86_cpu_def->kvm_features &= ~minus_kvm_features; > > > > x86_cpu_def->svm_features &= ~minus_svm_features; > > > > - if (check_cpuid) { > > > > - if (check_features_against_host(x86_cpu_def) && enforce_cpuid) > > > > - goto error; > > > > - } > > > > g_free(s); > > > > return 0; > > > > > > > > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp) > > > > env->cpuid_svm_features &= TCG_SVM_FEATURES; > > > > } > > > > > > > > + if (check_cpuid && check_features_against_host(cpu) > > > > + && enforce_cpuid) { > > > > + error_set(errp, QERR_PERMISSION_DENIED); > > > > + return; > > > > + } > > > > + > > > > > > I just noticed that you changed behavior on patch 04/20 and now restore > > > the behavior in this patch: > > > > > > - Before patch 04/20, the feature check was being done after the > > > features were filtered according to the TCG support (meaning a feature > > > not supported by TCG will not trigger enforce/check errors). > > before 04/20 check is done before TCG features filtering in > > cpu_x86_find_by_name() and then later in cpu_x86_register() features are > > TCG filtered. > > Right, my mistake. The check was done on cpu_x86_find_by_name() (before > the filtering), but for some reason I was thinking it was done much > later. Nevermind. > > > > > > - After patch 04/20, the check was being done _before_ the features were > > > filtered according to TCG support (meaning a feature not supported by > > > TCG would trigger enforce/check errors). > > > - With this patch, the old behavior is restored. > > after 04/20, it is the same as before, i.e realize is called after > > cpu_x86_register(). > > > > it's by mistake that in this patch I've put check after TCG filtering, I'll > > fix it and do check before it. > > Good. I think it's a good idea to make the check before the filtering > (and eventually, instead of checking the host CPU features directly, the > check on TCG mode should be based on TCG_FEATURES, and the check on KVM > mode should be based on GET_SUPPORTED_CPUID, but that's another issue). If you are not opposed to improving checks later, I'd better re-post this series with original behavior. Improving checks is kind of orthogonal to moving features into properties. > > > > > > > I'm not sure which behavior is better. But we surely shouldn't silently > > > move back and forth between those two modes. > > > > > > IMO, checking _before_ the TCG filtering is better, as it's more > > > predictable. it means having lots of warnings in case too-new CPU models > > > are chosen in TCG model, but that's exactly the point. > > > > > > > > > > #ifndef CONFIG_USER_ONLY > > > > qemu_register_reset(x86_cpu_machine_reset_cb, cpu); > > > > #endif > > > > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj) > > > > object_property_add(obj, "tsc-frequency", "int", > > > > x86_cpuid_get_tsc_freq, > > > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > > > > + object_property_add(obj, "check", "bool", > > > > + x86_cpuid_get_check, > > > > + x86_cpuid_set_check, NULL, NULL, NULL); > > > > + object_property_add(obj, "enforce", "bool", > > > > + x86_cpuid_get_enforce, > > > > + x86_cpuid_set_enforce, NULL, NULL, NULL); > > > > #if !defined(CONFIG_USER_ONLY) > > > > object_property_add(obj, "hv_spinlocks", "int", > > > > x86_get_hv_spinlocks, > > > > -- > > > > 1.7.11.2 > > > > > > > > > > -- > > > Eduardo > > > > > > -- > > Regards, > > Igor > > -- > Eduardo >
On Wed, Aug 15, 2012 at 02:11:43PM +0200, Igor Mammedov wrote: > On Wed, 15 Aug 2012 08:39:54 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Aug 14, 2012 at 11:18:47PM +0200, Igor Mammedov wrote: > > > On Fri, 10 Aug 2012 12:09:04 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote: > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > --- > > > > > target-i386/cpu.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--------- > > > > > 1 file changed, 57 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > index 7734613..a154e89 100644 > > > > > --- a/target-i386/cpu.c > > > > > +++ b/target-i386/cpu.c > > > > > @@ -106,8 +106,8 @@ typedef struct model_features_t { > > > > > uint32_t cpuid; > > > > > } model_features_t; > > > > > > > > > > -int check_cpuid = 0; > > > > > -int enforce_cpuid = 0; > > > > > +bool check_cpuid; > > > > > +bool enforce_cpuid; > > > > > > > > > > void host_cpuid(uint32_t function, uint32_t count, > > > > > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > > > > > @@ -579,19 +579,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) > > > > > * their way to the guest. Note: ft[].check_feat ideally should be > > > > > * specified via a guest_def field to suppress report of extraneous flags. > > > > > */ > > > > > -static int check_features_against_host(x86_def_t *guest_def) > > > > > +static int check_features_against_host(X86CPU *cpu) > > > > > { > > > > > + CPUX86State *env = &cpu->env; > > > > > x86_def_t host_def; > > > > > uint32_t mask; > > > > > int rv, i; > > > > > struct model_features_t ft[] = { > > > > > - {&guest_def->features, &host_def.features, > > > > > + {&env->cpuid_features, &host_def.features, > > > > > ~0, feature_name, 0x00000000}, > > > > > - {&guest_def->ext_features, &host_def.ext_features, > > > > > + {&env->cpuid_ext_features, &host_def.ext_features, > > > > > ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001}, > > > > > - {&guest_def->ext2_features, &host_def.ext2_features, > > > > > + {&env->cpuid_ext2_features, &host_def.ext2_features, > > > > > ~PPRO_FEATURES, ext2_feature_name, 0x80000000}, > > > > > - {&guest_def->ext3_features, &host_def.ext3_features, > > > > > + {&env->cpuid_ext3_features, &host_def.ext3_features, > > > > > ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}}; > > > > > > > > > > cpu_x86_fill_host(&host_def); > > > > > @@ -1030,6 +1031,43 @@ static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque, > > > > > } > > > > > #endif > > > > > > > > > > +static void x86_cpuid_get_check(Object *obj, Visitor *v, void *opaque, > > > > > + const char *name, Error **errp) > > > > > +{ > > > > > + visit_type_bool(v, &check_cpuid, name, errp); > > > > > +} > > > > > + > > > > > +static void x86_cpuid_set_check(Object *obj, Visitor *v, void *opaque, > > > > > + const char *name, Error **errp) > > > > > +{ > > > > > + bool value; > > > > > + > > > > > + visit_type_bool(v, &value, name, errp); > > > > > + if (error_is_set(errp)) { > > > > > + return; > > > > > + } > > > > > + check_cpuid = value; > > > > > +} > > > > > + > > > > > +static void x86_cpuid_get_enforce(Object *obj, Visitor *v, void *opaque, > > > > > + const char *name, Error **errp) > > > > > +{ > > > > > + visit_type_bool(v, &enforce_cpuid, name, errp); > > > > > +} > > > > > + > > > > > +static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque, > > > > > + const char *name, Error **errp) > > > > > +{ > > > > > + bool value; > > > > > + > > > > > + visit_type_bool(v, &value, name, errp); > > > > > + if (error_is_set(errp)) { > > > > > + return; > > > > > + } > > > > > + enforce_cpuid = value; > > > > > + object_property_set_bool(obj, value, "check", errp); > > > > > +} > > > > > + > > > > > static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp) > > > > > { > > > > > CPUX86State *env = &cpu->env; > > > > > @@ -1225,10 +1263,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, > > > > > x86_cpu_def->ext3_features &= ~minus_ext3_features; > > > > > x86_cpu_def->kvm_features &= ~minus_kvm_features; > > > > > x86_cpu_def->svm_features &= ~minus_svm_features; > > > > > - if (check_cpuid) { > > > > > - if (check_features_against_host(x86_cpu_def) && enforce_cpuid) > > > > > - goto error; > > > > > - } > > > > > g_free(s); > > > > > return 0; > > > > > > > > > > @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp) > > > > > env->cpuid_svm_features &= TCG_SVM_FEATURES; > > > > > } > > > > > > > > > > + if (check_cpuid && check_features_against_host(cpu) > > > > > + && enforce_cpuid) { > > > > > + error_set(errp, QERR_PERMISSION_DENIED); > > > > > + return; > > > > > + } > > > > > + > > > > > > > > I just noticed that you changed behavior on patch 04/20 and now restore > > > > the behavior in this patch: > > > > > > > > - Before patch 04/20, the feature check was being done after the > > > > features were filtered according to the TCG support (meaning a feature > > > > not supported by TCG will not trigger enforce/check errors). > > > before 04/20 check is done before TCG features filtering in > > > cpu_x86_find_by_name() and then later in cpu_x86_register() features are > > > TCG filtered. > > > > Right, my mistake. The check was done on cpu_x86_find_by_name() (before > > the filtering), but for some reason I was thinking it was done much > > later. Nevermind. > > > > > > > > > - After patch 04/20, the check was being done _before_ the features were > > > > filtered according to TCG support (meaning a feature not supported by > > > > TCG would trigger enforce/check errors). > > > > - With this patch, the old behavior is restored. > > > after 04/20, it is the same as before, i.e realize is called after > > > cpu_x86_register(). > > > > > > it's by mistake that in this patch I've put check after TCG filtering, I'll > > > fix it and do check before it. > > > > Good. I think it's a good idea to make the check before the filtering > > (and eventually, instead of checking the host CPU features directly, the > > check on TCG mode should be based on TCG_FEATURES, and the check on KVM > > mode should be based on GET_SUPPORTED_CPUID, but that's another issue). > If you are not opposed to improving checks later, I'd better re-post this > series with original behavior. Improving checks is kind of orthogonal to > moving features into properties. Absolutely. It's completely orthogonal, I just mentioned it because it's related to the discussion about the expected '-cpu check' behavior, and to let you know it's on my plans. It's also better to first clean up that code and make it sane (like you are doing), before introducing non-trivial behavior changes. > > > > > > > > > > > I'm not sure which behavior is better. But we surely shouldn't silently > > > > move back and forth between those two modes. > > > > > > > > IMO, checking _before_ the TCG filtering is better, as it's more > > > > predictable. it means having lots of warnings in case too-new CPU models > > > > are chosen in TCG model, but that's exactly the point. > > > > > > > > > > > > > #ifndef CONFIG_USER_ONLY > > > > > qemu_register_reset(x86_cpu_machine_reset_cb, cpu); > > > > > #endif > > > > > @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj) > > > > > object_property_add(obj, "tsc-frequency", "int", > > > > > x86_cpuid_get_tsc_freq, > > > > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > > > > > + object_property_add(obj, "check", "bool", > > > > > + x86_cpuid_get_check, > > > > > + x86_cpuid_set_check, NULL, NULL, NULL); > > > > > + object_property_add(obj, "enforce", "bool", > > > > > + x86_cpuid_get_enforce, > > > > > + x86_cpuid_set_enforce, NULL, NULL, NULL); > > > > > #if !defined(CONFIG_USER_ONLY) > > > > > object_property_add(obj, "hv_spinlocks", "int", > > > > > x86_get_hv_spinlocks, > > > > > -- > > > > > 1.7.11.2 > > > > > > > > > > > > > -- > > > > Eduardo > > > > > > > > > -- > > > Regards, > > > Igor > > > > -- > > Eduardo > > > > > -- > Regards, > Igor
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 7734613..a154e89 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -106,8 +106,8 @@ typedef struct model_features_t { uint32_t cpuid; } model_features_t; -int check_cpuid = 0; -int enforce_cpuid = 0; +bool check_cpuid; +bool enforce_cpuid; void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) @@ -579,19 +579,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) * their way to the guest. Note: ft[].check_feat ideally should be * specified via a guest_def field to suppress report of extraneous flags. */ -static int check_features_against_host(x86_def_t *guest_def) +static int check_features_against_host(X86CPU *cpu) { + CPUX86State *env = &cpu->env; x86_def_t host_def; uint32_t mask; int rv, i; struct model_features_t ft[] = { - {&guest_def->features, &host_def.features, + {&env->cpuid_features, &host_def.features, ~0, feature_name, 0x00000000}, - {&guest_def->ext_features, &host_def.ext_features, + {&env->cpuid_ext_features, &host_def.ext_features, ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001}, - {&guest_def->ext2_features, &host_def.ext2_features, + {&env->cpuid_ext2_features, &host_def.ext2_features, ~PPRO_FEATURES, ext2_feature_name, 0x80000000}, - {&guest_def->ext3_features, &host_def.ext3_features, + {&env->cpuid_ext3_features, &host_def.ext3_features, ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}}; cpu_x86_fill_host(&host_def); @@ -1030,6 +1031,43 @@ static void x86_set_hv_vapic(Object *obj, Visitor *v, void *opaque, } #endif +static void x86_cpuid_get_check(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + visit_type_bool(v, &check_cpuid, name, errp); +} + +static void x86_cpuid_set_check(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + bool value; + + visit_type_bool(v, &value, name, errp); + if (error_is_set(errp)) { + return; + } + check_cpuid = value; +} + +static void x86_cpuid_get_enforce(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + visit_type_bool(v, &enforce_cpuid, name, errp); +} + +static void x86_cpuid_set_enforce(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + bool value; + + visit_type_bool(v, &value, name, errp); + if (error_is_set(errp)) { + return; + } + enforce_cpuid = value; + object_property_set_bool(obj, value, "check", errp); +} + static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp) { CPUX86State *env = &cpu->env; @@ -1225,10 +1263,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, x86_cpu_def->ext3_features &= ~minus_ext3_features; x86_cpu_def->kvm_features &= ~minus_kvm_features; x86_cpu_def->svm_features &= ~minus_svm_features; - if (check_cpuid) { - if (check_features_against_host(x86_cpu_def) && enforce_cpuid) - goto error; - } g_free(s); return 0; @@ -1923,6 +1957,12 @@ void x86_cpu_realize(Object *obj, Error **errp) env->cpuid_svm_features &= TCG_SVM_FEATURES; } + if (check_cpuid && check_features_against_host(cpu) + && enforce_cpuid) { + error_set(errp, QERR_PERMISSION_DENIED); + return; + } + #ifndef CONFIG_USER_ONLY qemu_register_reset(x86_cpu_machine_reset_cb, cpu); #endif @@ -1964,6 +2004,12 @@ static void x86_cpu_initfn(Object *obj) object_property_add(obj, "tsc-frequency", "int", x86_cpuid_get_tsc_freq, x86_cpuid_set_tsc_freq, NULL, NULL, NULL); + object_property_add(obj, "check", "bool", + x86_cpuid_get_check, + x86_cpuid_set_check, NULL, NULL, NULL); + object_property_add(obj, "enforce", "bool", + x86_cpuid_get_enforce, + x86_cpuid_set_enforce, NULL, NULL, NULL); #if !defined(CONFIG_USER_ONLY) object_property_add(obj, "hv_spinlocks", "int", x86_get_hv_spinlocks,
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 11 deletions(-)