Message ID | 53751931.4000503@suse.de |
---|---|
State | New |
Headers | show |
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,
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
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 ).
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 ).
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 --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.