Re: [PATCH v1] target/i386: kvm: Block migration when enfore_cpuid is set to false

[Peter Xu]

On Thu, Jul 04, 2024 at 03:10:27PM +0000, Wang, Wei W wrote:
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c index
> > > 4c2e6f3a71..7db4fe4ead 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -8258,7 +8258,7 @@ static Property x86_cpu_properties[] = {
> > >      DEFINE_PROP_UINT32("hv-version-id-snumber", X86CPU,
> > > hyperv_ver_id_sn, 0),
> > >
> > >      DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
> > > -    DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> > > +    DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, true),
> > 
> > I assume in many cases people can still properly migrate when the hosts are
> > similar or identical, so maybe we at least want the old machine types keep
> > working (by introducing a machine compat property)?
> 
> You meant keeping "enforce_cpuid=false" for old machine types (e.g. before 
> 9.1)?
> This will make them non-migratable with this patch, but they were migratable 
> (by
> default) as "migratable" wasn't enforced by "enforce_cpuid". Should we keep 
> them
> being migratable by default (e.g. enforce_cpuid=true) as well?

Ah, this is trickier than I thought..

The issue is if we make them silently switch to enforce_cpuid=true on old
machines, there's chance they start to fail boot, am I right?

    if (cpu->enforce_cpuid && x86_cpu_have_filtered_features(cpu)) {
        error_setg(&local_err,
                   accel_uses_host_cpuid() ?
                       "Host doesn't support requested features" :
                       "TCG doesn't support requested features");
        goto out;
    }

I suppose we still need to keep all the old worlds running all fine without
breaking them when people do an QEMU upgrade.  It needs to work both on
booting fine, and on allowing to migrate.

So maybe we actually need two things?

  - One patch introduce forbit_migration_if_cpuid_mismatch property, when
    set, block migration if not enforced, otherwise it should still allow
    migration even if enforce_cpud=fales.  It should default to on, but off
    on old machines.

  - One patch change default value of enforce_cpuid to on, but turn it off
    on old machines.

Does that look right?

Thanks,

-- 
Peter Xu