Re: [Qemu-devel] [PATCH v4 14/18] target-i386: Add "migratable" property to "host" CPU model

[Eduardo Habkost]

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.

-- 
Eduardo