diff mbox

[v4,14/18] target-i386: Add "migratable" property to "host" CPU model

Message ID 53751931.4000503@suse.de
State New
Headers show

Commit Message

Andreas Färber May 15, 2014, 7:44 p.m. UTC
Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> This flag will allow the user to choose between two modes:
>  * All flags that can be enabled on the host, even if unmigratable
>    (migratable=no);
>  * All flags that can be enabled on the host, known to QEMU,
>    and migratable (migratable=yes).
> 
> The default is still migratable=false, to keep current behavior, but
> this will be changed to migratable=true by another patch.
> 
> My plan was to support the "migratable" flag on all CPU classes, but
> have the default to "false" on all CPU models except "host". However,
> DeviceClass has no mechanism to allow a child class to have a different
> property default from the parent class yet, so by now only the "host"
> CPU model will support the "migratable" flag.

Just set the new default in the derived type's instance_init?

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu-qom.h |  5 +++++
>  target-i386/cpu.c     | 52 +++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index e9b3d57..016f90d 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -87,6 +87,11 @@ typedef struct X86CPU {
>      bool hyperv_time;
>      bool check_cpuid;
>      bool enforce_cpuid;
> +    /* If set, only migratable flags will be accepted when "enforce" mode is
> +     * used, and only migratable flags will be included in the "host"
> +     * CPU model.
> +     */

This belongs in the documentation comment above:

  */

> +    bool migratable;
>  
>      /* if true the CPUID code directly forward host cache leaves to the guest */
>      bool cache_info_passthrough;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a357056..9c30957 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -338,6 +338,7 @@ typedef struct FeatureWordInfo {
>      uint32_t cpuid_ecx;   /* Input ECX value for CPUID */
>      int cpuid_reg;        /* output register (R_* constant) */
>      uint32_t tcg_features; /* Feature flags supported by TCG */
> +    uint32_t unmigratable_flags; /* Feature flags known to be unmigratable */

(Here it's in .c only, so would not affect generated documentation.)

>  } FeatureWordInfo;
>  
>  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> @@ -461,6 +462,30 @@ void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features)
>      kvm_default_features[w] &= ~features;
>  }
>  
> +/* Returns the set of feature flags that are supported and migratable by
> + * QEMU, for a given FeatureWord
> + */
> +static uint32_t x86_cpu_get_migratable_flags(FeatureWord w)
> +{
> +    uint32_t r = 0;
> +    int i;
> +
> +    FeatureWordInfo *wi = &feature_word_info[w];
> +    for (i = 0; i < 32; i++) {
> +        uint32_t f = 1U << i;
> +        /* If the feature name is unknown, it is not supported by QEMU yet */
> +        if (!wi->feat_names[i]) {
> +            continue;
> +        }
> +        /* Skip features known to QEMU, but explicitly marked as unmigratable */
> +        if (wi->unmigratable_flags & f) {
> +            continue;
> +        }
> +        r |= f;
> +    }
> +    return r;
> +}
> +
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>  {
> @@ -1206,6 +1231,11 @@ static int cpu_x86_fill_model_id(char *str)
>  
>  static X86CPUDefinition host_cpudef;
>  
> +static Property x86_host_cpu_properties[] = {

host_x86_cpu_... for consistency please (specific to abstract).

> +    DEFINE_PROP_BOOL("migratable", X86CPU, migratable, false),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>  /* class_init for the "host" CPU model
>   *
>   * This function may be called before KVM is initialized.
> @@ -1213,6 +1243,7 @@ static X86CPUDefinition host_cpudef;
>  static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);

Ordered by type hierarchy please.

>      uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
>  
>      host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> @@ -1228,12 +1259,14 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
>      xcc->cpu_def = &host_cpudef;
>      host_cpudef.cache_info_passthrough = true;
>  
> +    dc->props = x86_host_cpu_properties;
>      /* level, xlevel, xlevel2, and the feature words are initialized on
>       * instance_init, because they require KVM to be initialized.
>       */

I'll swap these, as [x]level* are logically still xcc.

>  }
>  
> -static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w);
> +static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
> +                                                   bool migratable_only);
>  
>  static void host_x86_cpu_initfn(Object *obj)
>  {
> @@ -1257,7 +1290,7 @@ static void host_x86_cpu_initfn(Object *obj)
>  
>      for (w = 0; w < FEATURE_WORDS; w++) {
>          env->features[w] =
> -            x86_cpu_get_supported_feature_word(w);
> +            x86_cpu_get_supported_feature_word(w, cpu->migratable);
>      }
>      object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
>  }
> @@ -1839,16 +1872,22 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>      return cpu_list;
>  }
>  
> -static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w)
> +static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
> +                                                   bool migratable_only)
>  {
>      FeatureWordInfo *wi = &feature_word_info[w];
> +    uint32_t r;
>      if (kvm_enabled()) {
> -        return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> +        r =  kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>                                                         wi->cpuid_ecx,
>                                                         wi->cpuid_reg);

Reindented.

>      } else {
> -        return wi->tcg_features;
> +        r =  wi->tcg_features;
> +    }
> +    if (migratable_only) {
> +        r &= x86_cpu_get_migratable_flags(w);
>      }
> +    return r;
>  }
>  
>  /* Filters CPU feature words based on host availability of each feature
> @@ -1862,7 +1901,8 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>      int rv = 0;
>  
>      for (w = 0; w < FEATURE_WORDS; w++) {
> -        uint32_t host_feat = x86_cpu_get_supported_feature_word(w);
> +        uint32_t host_feat =
> +            x86_cpu_get_supported_feature_word(w, cpu->migratable);
>          uint32_t requested_features = env->features[w];
>          env->features[w] &= host_feat;
>          cpu->filtered_features[w] = requested_features & ~env->features[w];

Regards,
Andreas

Comments

Eduardo Habkost May 15, 2014, 8:26 p.m. UTC | #1
On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > This flag will allow the user to choose between two modes:
> >  * All flags that can be enabled on the host, even if unmigratable
> >    (migratable=no);
> >  * All flags that can be enabled on the host, known to QEMU,
> >    and migratable (migratable=yes).
> > 
> > The default is still migratable=false, to keep current behavior, but
> > this will be changed to migratable=true by another patch.
> > 
> > My plan was to support the "migratable" flag on all CPU classes, but
> > have the default to "false" on all CPU models except "host". However,
> > DeviceClass has no mechanism to allow a child class to have a different
> > property default from the parent class yet, so by now only the "host"
> > CPU model will support the "migratable" flag.
> 
> Just set the new default in the derived type's instance_init?

That would work. I am still assuming that one day we will allow
management to query for class property defaults without instantiating
objects. But even if we do it, "host" is already an exception (because
the defaults depend on KVM initialization), so in this case it will be
OK.

So, this patch can be dropped because it will be replaced. I will also
implement the other changes you requested for this patch.

Thanks,
Andreas Färber May 15, 2014, 10:12 p.m. UTC | #2
Am 15.05.2014 22:26, schrieb Eduardo Habkost:
> On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
>>> This flag will allow the user to choose between two modes:
>>>  * All flags that can be enabled on the host, even if unmigratable
>>>    (migratable=no);
>>>  * All flags that can be enabled on the host, known to QEMU,
>>>    and migratable (migratable=yes).
>>>
>>> The default is still migratable=false, to keep current behavior, but
>>> this will be changed to migratable=true by another patch.
>>>
>>> My plan was to support the "migratable" flag on all CPU classes, but
>>> have the default to "false" on all CPU models except "host". However,
>>> DeviceClass has no mechanism to allow a child class to have a different
>>> property default from the parent class yet, so by now only the "host"
>>> CPU model will support the "migratable" flag.
>>
>> Just set the new default in the derived type's instance_init?
> 
> That would work. I am still assuming that one day we will allow
> management to query for class property defaults without instantiating
> objects. But even if we do it, "host" is already an exception (because
> the defaults depend on KVM initialization), so in this case it will be
> OK.
> 
> So, this patch can be dropped because it will be replaced. I will also
> implement the other changes you requested for this patch.

Before you make yourself too much work, have a peek at qom-cpu. :)
I should have all except 15 and 18, with some cleanups TBD.

Andreas
Eduardo Habkost May 16, 2014, 4:13 p.m. UTC | #3
On Fri, May 16, 2014 at 12:12:18AM +0200, Andreas Färber wrote:
> Am 15.05.2014 22:26, schrieb Eduardo Habkost:
> > On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
> >> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> >>> This flag will allow the user to choose between two modes:
> >>>  * All flags that can be enabled on the host, even if unmigratable
> >>>    (migratable=no);
> >>>  * All flags that can be enabled on the host, known to QEMU,
> >>>    and migratable (migratable=yes).
> >>>
> >>> The default is still migratable=false, to keep current behavior, but
> >>> this will be changed to migratable=true by another patch.
> >>>
> >>> My plan was to support the "migratable" flag on all CPU classes, but
> >>> have the default to "false" on all CPU models except "host". However,
> >>> DeviceClass has no mechanism to allow a child class to have a different
> >>> property default from the parent class yet, so by now only the "host"
> >>> CPU model will support the "migratable" flag.
> >>
> >> Just set the new default in the derived type's instance_init?
> > 
> > That would work. I am still assuming that one day we will allow
> > management to query for class property defaults without instantiating
> > objects. But even if we do it, "host" is already an exception (because
> > the defaults depend on KVM initialization), so in this case it will be
> > OK.
> > 
> > So, this patch can be dropped because it will be replaced. I will also
> > implement the other changes you requested for this patch.
> 
> Before you make yourself too much work, have a peek at qom-cpu. :)
> I should have all except 15 and 18, with some cleanups TBD.

Thsnk! But I see two problems on current qom-cpu:

 * The "migratable" flag is now not affecting the results of "-cpu host"
   (host_x86_cpu_initfn()), which was the whole point of adding the
   property.
 * Without setting migratable=yes by default, we are going to break
   existing setups after applying 'support "invariant tsc" flag' and
   "block migration and savevm if invariant tsc is exposed" (See
   http://marc.info/?l=qemu-devel&m=139838802220184&w=2 ).
Andreas Färber May 16, 2014, 4:29 p.m. UTC | #4
Am 16.05.2014 18:13, schrieb Eduardo Habkost:
> On Fri, May 16, 2014 at 12:12:18AM +0200, Andreas Färber wrote:
>> Am 15.05.2014 22:26, schrieb Eduardo Habkost:
>>> On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
>>>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
>>>>> This flag will allow the user to choose between two modes:
>>>>>  * All flags that can be enabled on the host, even if unmigratable
>>>>>    (migratable=no);
>>>>>  * All flags that can be enabled on the host, known to QEMU,
>>>>>    and migratable (migratable=yes).
>>>>>
>>>>> The default is still migratable=false, to keep current behavior, but
>>>>> this will be changed to migratable=true by another patch.
>>>>>
>>>>> My plan was to support the "migratable" flag on all CPU classes, but
>>>>> have the default to "false" on all CPU models except "host". However,
>>>>> DeviceClass has no mechanism to allow a child class to have a different
>>>>> property default from the parent class yet, so by now only the "host"
>>>>> CPU model will support the "migratable" flag.
>>>>
>>>> Just set the new default in the derived type's instance_init?
>>>
>>> That would work. I am still assuming that one day we will allow
>>> management to query for class property defaults without instantiating
>>> objects. But even if we do it, "host" is already an exception (because
>>> the defaults depend on KVM initialization), so in this case it will be
>>> OK.
>>>
>>> So, this patch can be dropped because it will be replaced. I will also
>>> implement the other changes you requested for this patch.
>>
>> Before you make yourself too much work, have a peek at qom-cpu. :)
>> I should have all except 15 and 18, with some cleanups TBD.
> 
> Thsnk! But I see two problems on current qom-cpu:
> 
>  * The "migratable" flag is now not affecting the results of "-cpu host"
>    (host_x86_cpu_initfn()), which was the whole point of adding the
>    property.

Where did I break that? Renaming the variable and reordering it with a
comment shouldn't be a functional change... Note that some patches
needed to be applied with patch -p1 due to rebased qom-next, so maybe
there's a mismerge somewhere?

OTOH maybe we should start writing qtests for the CPU? I've been meaning
to write one for cpu-add but didn't get to it yet.

Andreas

>  * Without setting migratable=yes by default, we are going to break
>    existing setups after applying 'support "invariant tsc" flag' and
>    "block migration and savevm if invariant tsc is exposed" (See
>    http://marc.info/?l=qemu-devel&m=139838802220184&w=2 ).
Eduardo Habkost May 16, 2014, 5:18 p.m. UTC | #5
On Fri, May 16, 2014 at 06:29:36PM +0200, Andreas Färber wrote:
> Am 16.05.2014 18:13, schrieb Eduardo Habkost:
> > On Fri, May 16, 2014 at 12:12:18AM +0200, Andreas Färber wrote:
> >> Am 15.05.2014 22:26, schrieb Eduardo Habkost:
> >>> On Thu, May 15, 2014 at 09:44:49PM +0200, Andreas Färber wrote:
> >>>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> >>>>> This flag will allow the user to choose between two modes:
> >>>>>  * All flags that can be enabled on the host, even if unmigratable
> >>>>>    (migratable=no);
> >>>>>  * All flags that can be enabled on the host, known to QEMU,
> >>>>>    and migratable (migratable=yes).
> >>>>>
> >>>>> The default is still migratable=false, to keep current behavior, but
> >>>>> this will be changed to migratable=true by another patch.
> >>>>>
> >>>>> My plan was to support the "migratable" flag on all CPU classes, but
> >>>>> have the default to "false" on all CPU models except "host". However,
> >>>>> DeviceClass has no mechanism to allow a child class to have a different
> >>>>> property default from the parent class yet, so by now only the "host"
> >>>>> CPU model will support the "migratable" flag.
> >>>>
> >>>> Just set the new default in the derived type's instance_init?
> >>>
> >>> That would work. I am still assuming that one day we will allow
> >>> management to query for class property defaults without instantiating
> >>> objects. But even if we do it, "host" is already an exception (because
> >>> the defaults depend on KVM initialization), so in this case it will be
> >>> OK.
> >>>
> >>> So, this patch can be dropped because it will be replaced. I will also
> >>> implement the other changes you requested for this patch.
> >>
> >> Before you make yourself too much work, have a peek at qom-cpu. :)
> >> I should have all except 15 and 18, with some cleanups TBD.
> > 
> > Thsnk! But I see two problems on current qom-cpu:
> > 
> >  * The "migratable" flag is now not affecting the results of "-cpu host"
> >    (host_x86_cpu_initfn()), which was the whole point of adding the
> >    property.
> 
> Where did I break that? Renaming the variable and reordering it with a
> comment shouldn't be a functional change... Note that some patches
> needed to be applied with patch -p1 due to rebased qom-next, so maybe
> there's a mismerge somewhere?

Oh, the problem is not on this specific patch, but the fact that the
patch doesn't make sense without applying "Support '-cpu host' in TCG
mode" first (which changes host_x86_cpu_initfn() to use
x86_cpu_get_supported_feature_word() instead of
kvm_arch_get_supported_cpuid() directly).

> 
> OTOH maybe we should start writing qtests for the CPU? I've been meaning
> to write one for cpu-add but didn't get to it yet.

Main difficulty with writing tests for those host/check/enforce changes
is that the results depend on the GET_SUPPORTED_CPUID values returned by
the kernel. This may be addressed by writing a configurable fake
kvm_arch_get_supported_cpuid().

Another difficulty is that the new code is about handling unknown flags,
and any unknown flag may become a known one in future QEMU versions
(which would then break the test). This may be addressed by adding a
hack to clear an arbitrary item on a *_feature_name[] array during
testing.
diff mbox

Patch

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index e9b3d57..aa63d3c 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -71,6 +71,9 @@  typedef struct X86CPUClass {
 /**
  * X86CPU:
  * @env: #CPUX86State
+ * @migratable: If set, only migratable flags will be accepted when
"enforce"
+ * mode is used, and only migratable flags will be included in the "host"
+ * CPU model.
  *
  * An x86 CPU.