Re: [Qemu-devel] [RFC 11/20] target-i386: convert 'check' and 'enforce' features into properties

[Eduardo Habkost]

On Wed, Aug 15, 2012 at 02:11:43PM +0200, Igor Mammedov wrote:
> On Wed, 15 Aug 2012 08:39:54 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden> wrote:
> > > 
> > > > On Fri, Aug 10, 2012 at 01:22:27PM +0200, Igor Mammedov wrote:
> > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > ---
> > > > >  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

-- 
Eduardo