| Message ID | 20240703144912.130988-1-wei.w.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] target/i386: kvm: Block migration when enfore_cpuid is set to false | expand |
On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > When enforce_cpuid is set to false, the guest is launched with a filtered > set of features, meaning that unsupported features by the host are removed > from the guest's vCPU model. This could cause issues for live migration. > For example, a guest on the source is running with features A and B. If > the destination host does not support feature B, the stub guest can still > be launched on the destination with feature A only if enforce_cpuid=false. > Live migration can start in this case, though it may fail later when the > states of feature B are put to the destination side. This failure occurs > in the late stage (i.e., stop© phase) of the migration flow, where the > source guest has already been paused. Tests show that in such cases the > source guest does not recover, and the destination is unable to resume to > run. > > Make "enfore_cpuid=true" a hard requirement for a guest to be migratable, > and change the default value of "enforce_cpuid" to true, making the guest > vCPUs migratable by default. If the destination stub guest has inconsistent > CPUIDs (i.e., destination host cannot support the features defined by the > guest's vCPU model), it fails to boot (with enfore_cpuid=true by default), > thereby preventing migration from occuring. If enfore_cpuid=false is > explicitly added for the guest, the guest is deemed as non-migratable > (via the migration blocker), so the above issue won't occur as the guest > won't be migrated. > > Tested-by: Lei Wang <lei4.wang@intel.com> > Signed-off-by: Wei Wang <wei.w.wang@intel.com> [Copy Jiri and Dan for libvirt-side implications] > --- > target/i386/cpu.c | 2 +- > target/i386/kvm/kvm.c | 25 +++++++++++++++---------- > 2 files changed, 16 insertions(+), 11 deletions(-) > > 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)? > DEFINE_PROP_BOOL("x-force-features", X86CPU, force_features, false), > DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), > DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0), > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index dd8b0f3313..aee717c1cf 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -1741,7 +1741,7 @@ static int hyperv_init_vcpu(X86CPU *cpu) > return 0; > } > > -static Error *invtsc_mig_blocker; > +static Error *cpu_mig_blocker; > > #define KVM_MAX_CPUID_ENTRIES 100 > > @@ -2012,6 +2012,15 @@ full: > abort(); > } > > +static bool kvm_vcpu_need_block_migration(X86CPU *cpu) > +{ > + CPUX86State *env = &cpu->env; > + > + return !cpu->enforce_cpuid || > + (!env->user_tsc_khz && (env->features[FEAT_8000_0007_EDX] & > + CPUID_APM_INVTSC)); > +} Nit: maybe it's nice this returns a "const char*" with detailed reasons to be put into the error_setg(), so it dumps the same as before for the invtsc blocker. Thanks, > + > int kvm_arch_init_vcpu(CPUState *cs) > { > struct { > @@ -2248,18 +2257,14 @@ int kvm_arch_init_vcpu(CPUState *cs) > has_msr_mcg_ext_ctl = has_msr_feature_control = true; > } > > - if (!env->user_tsc_khz) { > - if ((env->features[FEAT_8000_0007_EDX] & CPUID_APM_INVTSC) && > - invtsc_mig_blocker == NULL) { > - error_setg(&invtsc_mig_blocker, > - "State blocked by non-migratable CPU device" > - " (invtsc flag)"); > - r = migrate_add_blocker(&invtsc_mig_blocker, &local_err); > + if (!cpu_mig_blocker && kvm_vcpu_need_block_migration(cpu)) { > + error_setg(&cpu_mig_blocker, > + "State blocked by non-migratable CPU device"); > + r = migrate_add_blocker(&cpu_mig_blocker, &local_err); > if (r < 0) { > error_report_err(local_err); > return r; > } > - } > } > > if (cpu->vmware_cpuid_freq > @@ -2312,7 +2317,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > return 0; > > fail: > - migrate_del_blocker(&invtsc_mig_blocker); > + migrate_del_blocker(&cpu_mig_blocker); > > return r; > } > -- > 2.27.0 > >
On Thursday, July 4, 2024 2:04 AM, Peter Xu wrote: > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > When enforce_cpuid is set to false, the guest is launched with a > > filtered set of features, meaning that unsupported features by the > > host are removed from the guest's vCPU model. This could cause issues for > live migration. > > For example, a guest on the source is running with features A and B. > > If the destination host does not support feature B, the stub guest can > > still be launched on the destination with feature A only if > enforce_cpuid=false. > > Live migration can start in this case, though it may fail later when > > the states of feature B are put to the destination side. This failure > > occurs in the late stage (i.e., stop© phase) of the migration > > flow, where the source guest has already been paused. Tests show that > > in such cases the source guest does not recover, and the destination > > is unable to resume to run. > > > > Make "enfore_cpuid=true" a hard requirement for a guest to be > > migratable, and change the default value of "enforce_cpuid" to true, > > making the guest vCPUs migratable by default. If the destination stub > > guest has inconsistent CPUIDs (i.e., destination host cannot support > > the features defined by the guest's vCPU model), it fails to boot > > (with enfore_cpuid=true by default), thereby preventing migration from > > occuring. If enfore_cpuid=false is explicitly added for the guest, the > > guest is deemed as non-migratable (via the migration blocker), so the > > above issue won't occur as the guest won't be migrated. > > > > Tested-by: Lei Wang <lei4.wang@intel.com> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > > [Copy Jiri and Dan for libvirt-side implications] Thanks! > > > --- > > target/i386/cpu.c | 2 +- > > target/i386/kvm/kvm.c | 25 +++++++++++++++---------- > > 2 files changed, 16 insertions(+), 11 deletions(-) > > > > 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? > > > DEFINE_PROP_BOOL("x-force-features", X86CPU, force_features, false), > > DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), > > DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0), diff --git > > a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index > > dd8b0f3313..aee717c1cf 100644 > > --- a/target/i386/kvm/kvm.c > > +++ b/target/i386/kvm/kvm.c > > @@ -1741,7 +1741,7 @@ static int hyperv_init_vcpu(X86CPU *cpu) > > return 0; > > } > > > > -static Error *invtsc_mig_blocker; > > +static Error *cpu_mig_blocker; > > > > #define KVM_MAX_CPUID_ENTRIES 100 > > > > @@ -2012,6 +2012,15 @@ full: > > abort(); > > } > > > > +static bool kvm_vcpu_need_block_migration(X86CPU *cpu) { > > + CPUX86State *env = &cpu->env; > > + > > + return !cpu->enforce_cpuid || > > + (!env->user_tsc_khz && (env->features[FEAT_8000_0007_EDX] & > > + CPUID_APM_INVTSC)); } > > Nit: maybe it's nice this returns a "const char*" with detailed reasons to be put > into the error_setg(), so it dumps the same as before for the invtsc blocker. Sounds good. I'll check how to incorporate such info into the logs.
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,
On Thursday, July 4, 2024 11:59 PM, Peter Xu wrote: > 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? Right for newly launched guests, regardless of whether they are new or old machine types, they will fail to boot when the host cannot afford the features for the configured vCPU models. This is expected, and actually part of the intentions of this patch. When there is a need to boot a guest with reduced features, users need to explicitly add "enforce_cpuid=false", which marks the new booted guest as non-migratable, or a _better_ way, to identify the unsupported features from the host first, and then get it booted with "-cpu CpuModel,-A,-B", this can make it migratable with those known reduced features, and the destination guest is required to use the same QEMU commands (as usual) to reduce the same set of features as the source and get a enforced check by "enforce_cpuid". For live update of QEMU for existing running guests (as you mentioned below), the impact is only on the running guests that have had features reduced from vCPU models (at the time of their original launch). For this case, the recommended way to update them to the new QEMU is also to explicitly identify the reduced features and update them with "-cpu CpuModel,-A,-B". The rationale behind this is that the features reduced from the guest needs to be explicitly determined and controllable. In terms of live migration, the destination is ensured to have the same set of reduced features as the source side. > > 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? I think this can work. Not sure what you would think about the above explanations. If agree, then probably we don’t need to add the extra complexity. Also, the above two things seem to impede the upgrade for guests with older machine types to incorporate this enforcement. I think the primary goal of live updating to a newer QEMU version is to benefit from the enhancements offered by the new QEMU. So it seems more beneficial to bring old guests under such enforcements, given that this doesn't break functionalities that the guest is running. The only requirement for this is to upgrade using more explicit QEMU commands (i.e., -cpu CpuModel,-A,-B) when needed.
On Fri, Jul 05, 2024 at 10:22:23AM +0000, Wang, Wei W wrote: > On Thursday, July 4, 2024 11:59 PM, Peter Xu wrote: > > 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? > > Right for newly launched guests, regardless of whether they are new or old > machine types, they will fail to boot when the host cannot afford the features > for the configured vCPU models. This is expected, and actually part of the > intentions of this patch. > > When there is a need to boot a guest with reduced features, users need to > explicitly add "enforce_cpuid=false", which marks the new booted guest as > non-migratable, or a _better_ way, to identify the unsupported features from > the host first, and then get it booted with "-cpu CpuModel,-A,-B", this can make > it migratable with those known reduced features, and the destination guest is > required to use the same QEMU commands (as usual) to reduce the same set > of features as the source and get a enforced check by "enforce_cpuid". > > For live update of QEMU for existing running guests (as you mentioned > below), the impact is only on the running guests that have had features reduced > from vCPU models (at the time of their original launch). For this case, the > recommended way to update them to the new QEMU is also to explicitly identify > the reduced features and update them with "-cpu CpuModel,-A,-B". > > The rationale behind this is that the features reduced from the guest needs to > be explicitly determined and controllable. In terms of live migration, the > destination is ensured to have the same set of reduced features as the source > side. > > > > > 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? > > I think this can work. Not sure what you would think about the above explanations. > If agree, then probably we don’t need to add the extra complexity. > > Also, the above two things seem to impede the upgrade for guests with older machine > types to incorporate this enforcement. I think the primary goal of live updating to a > newer QEMU version is to benefit from the enhancements offered by the new QEMU. > So it seems more beneficial to bring old guests under such enforcements, given > that this doesn't break functionalities that the guest is running. The only > requirement for this is to upgrade using more explicit QEMU commands > (i.e., -cpu CpuModel,-A,-B) when needed. What you said makes sense. It's just that the concern still exists, and I'm not sure whether that'll be too much to ask for a customer. Also, see this commit: commit 15e41345906d29a319cc9cdf566347bf79134d24 Author: Eduardo Habkost <ehabkost@redhat.com> Date: Wed Aug 26 13:25:44 2015 -0300 target-i386: Enable "check" mode by default Current default behavior of QEMU is to silently disable features that are not supported by the host when a CPU model is requested in the command-line. This means that in addition to risking breaking guest ABI by default, we are silent about it. I would like to enable "enforce" by default, but this can easily break existing production systems because of the way libvirt makes assumptions about CPU models today (this will change in the future, once QEMU provide a proper interface for checking if a CPU model is runnable). But there's no reason we should be silent about it. So, change target-i386 to enable "check" mode by default so at least we have some warning printed to stderr (and hopefully logged somewhere) when QEMU disables a feature that is not supported by the host system. Reviewed-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> I don't think I know what's the "libvirt assumptions" mentioned, and how it changed as of today. I had a vague memory Libvirt constantly set off on some of the relevant flags last time Jiri explained some cpuid issues to me; maybe it's "check" not "enforce"? It would be great if Jiri or Dan can comment here. Copy Igor too from the commit. Thanks,
On Friday, July 5, 2024 9:34 PM, Peter Xu wrote: > On Fri, Jul 05, 2024 at 10:22:23AM +0000, Wang, Wei W wrote: > > On Thursday, July 4, 2024 11:59 PM, Peter Xu wrote: > > > 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? > > > > Right for newly launched guests, regardless of whether they are new or > > old machine types, they will fail to boot when the host cannot afford > > the features for the configured vCPU models. This is expected, and > > actually part of the intentions of this patch. > > > > When there is a need to boot a guest with reduced features, users need > > to explicitly add "enforce_cpuid=false", which marks the new booted > > guest as non-migratable, or a _better_ way, to identify the > > unsupported features from the host first, and then get it booted with > > "-cpu CpuModel,-A,-B", this can make it migratable with those known > > reduced features, and the destination guest is required to use the > > same QEMU commands (as usual) to reduce the same set of features as the > source and get a enforced check by "enforce_cpuid". > > > > For live update of QEMU for existing running guests (as you mentioned > > below), the impact is only on the running guests that have had > > features reduced from vCPU models (at the time of their original > > launch). For this case, the recommended way to update them to the new > > QEMU is also to explicitly identify the reduced features and update them > with "-cpu CpuModel,-A,-B". > > > > The rationale behind this is that the features reduced from the guest > > needs to be explicitly determined and controllable. In terms of live > > migration, the destination is ensured to have the same set of reduced > > features as the source side. > > > > > > > > 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? > > > > I think this can work. Not sure what you would think about the above > explanations. > > If agree, then probably we don’t need to add the extra complexity. > > > > Also, the above two things seem to impede the upgrade for guests with > > older machine types to incorporate this enforcement. I think the > > primary goal of live updating to a newer QEMU version is to benefit from the > enhancements offered by the new QEMU. > > So it seems more beneficial to bring old guests under such > > enforcements, given that this doesn't break functionalities that the > > guest is running. The only requirement for this is to upgrade using > > more explicit QEMU commands (i.e., -cpu CpuModel,-A,-B) when needed. > > What you said makes sense. It's just that the concern still exists, and I'm not > sure whether that'll be too much to ask for a customer. > > Also, see this commit: > > commit 15e41345906d29a319cc9cdf566347bf79134d24 > Author: Eduardo Habkost <ehabkost@redhat.com> > Date: Wed Aug 26 13:25:44 2015 -0300 > > target-i386: Enable "check" mode by default > > Current default behavior of QEMU is to silently disable features that > are not supported by the host when a CPU model is requested in the > command-line. This means that in addition to risking breaking guest ABI > by default, we are silent about it. > > I would like to enable "enforce" by default, but this can easily break > existing production systems because of the way libvirt makes assumptions > about CPU models today (this will change in the future, once QEMU > provide a proper interface for checking if a CPU model is runnable). > > But there's no reason we should be silent about it. So, change > target-i386 to enable "check" mode by default so at least we have some > warning printed to stderr (and hopefully logged somewhere) when QEMU > disables a feature that is not supported by the host system. > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > I don't think I know what's the "libvirt assumptions" mentioned, and how it > changed as of today. I had a vague memory Libvirt constantly set off on some > of the relevant flags last time Jiri explained some cpuid issues to me; maybe it's > "check" not "enforce"? It would be great if Jiri or Dan can comment here. I had a check in libvirt and didn't find "enforce" is set to false anywhere (except only in some tests, which I think should be fine). Seems we haven't got responses from more folks here :) Is it appropriate to open a discussion thread in devel@lists.libvirt.org to confirm?
On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > When enforce_cpuid is set to false, the guest is launched with a filtered > set of features, meaning that unsupported features by the host are removed > from the guest's vCPU model. This could cause issues for live migration. > For example, a guest on the source is running with features A and B. If > the destination host does not support feature B, the stub guest can still > be launched on the destination with feature A only if enforce_cpuid=false. > Live migration can start in this case, though it may fail later when the > states of feature B are put to the destination side. This failure occurs > in the late stage (i.e., stop© phase) of the migration flow, where the > source guest has already been paused. Tests show that in such cases the > source guest does not recover, and the destination is unable to resume to > run. > > Make "enfore_cpuid=true" a hard requirement for a guest to be migratable, > and change the default value of "enforce_cpuid" to true, making the guest > vCPUs migratable by default. If the destination stub guest has inconsistent > CPUIDs (i.e., destination host cannot support the features defined by the > guest's vCPU model), it fails to boot (with enfore_cpuid=true by default), > thereby preventing migration from occuring. If enfore_cpuid=false is > explicitly added for the guest, the guest is deemed as non-migratable > (via the migration blocker), so the above issue won't occur as the guest > won't be migrated. Blocking migration when enforce=false is making an assumption that users of that setting are inherantly broken. This is NOT the case if the user/app has already validated compatibility in some manner outside QEMU. Blocking migration in this case will break valid working use cases. IMHO this patch doesn't need to exist. If users of QEMU want strong protection they can already opt-in to that with enforce=true. With regards, Daniel
On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > When enforce_cpuid is set to false, the guest is launched with a > > filtered set of features, meaning that unsupported features by the > > host are removed from the guest's vCPU model. This could cause issues for > live migration. > > For example, a guest on the source is running with features A and B. > > If the destination host does not support feature B, the stub guest can > > still be launched on the destination with feature A only if enforce_cpuid=false. > > Live migration can start in this case, though it may fail later when > > the states of feature B are put to the destination side. This failure > > occurs in the late stage (i.e., stop© phase) of the migration > > flow, where the source guest has already been paused. Tests show that > > in such cases the source guest does not recover, and the destination > > is unable to resume to run. > > > > Make "enfore_cpuid=true" a hard requirement for a guest to be > > migratable, and change the default value of "enforce_cpuid" to true, > > making the guest vCPUs migratable by default. If the destination stub > > guest has inconsistent CPUIDs (i.e., destination host cannot support > > the features defined by the guest's vCPU model), it fails to boot > > (with enfore_cpuid=true by default), thereby preventing migration from > > occuring. If enfore_cpuid=false is explicitly added for the guest, the > > guest is deemed as non-migratable (via the migration blocker), so the > > above issue won't occur as the guest won't be migrated. > > Blocking migration when enforce=false is making an assumption that users of > that setting are inherantly broken. This is NOT the case if the user/app has > already validated compatibility in some manner outside QEMU. Blocking > migration in this case will break valid working use cases. It's just an enforcement to ensure a safe migration. Without this (i.e., the current QEMU code) is making an assumption that users always have validated compatibility in a good manner outside QEMU, which is risky to some degree? Do you see how this would break valid working use cases (any examples)? This is actually what we are looking for. Please be aware that "enforce" is changed to be true by default to make the guest to be migratable by default under the enforcement. > > IMHO this patch doesn't need to exist. If users of QEMU want strong protection > they can already opt-in to that with enforce=true. AFAIK, many users are not aware of this, and also we couldn't assume everybody knows it. That's why we want to add the enforcement.
On Thu, Jul 11, 2024 at 12:10:34PM +0000, Wang, Wei W wrote: > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > When enforce_cpuid is set to false, the guest is launched with a > > > filtered set of features, meaning that unsupported features by the > > > host are removed from the guest's vCPU model. This could cause issues for > > live migration. > > > For example, a guest on the source is running with features A and B. > > > If the destination host does not support feature B, the stub guest can > > > still be launched on the destination with feature A only if enforce_cpuid=false. > > > Live migration can start in this case, though it may fail later when > > > the states of feature B are put to the destination side. This failure > > > occurs in the late stage (i.e., stop© phase) of the migration > > > flow, where the source guest has already been paused. Tests show that > > > in such cases the source guest does not recover, and the destination > > > is unable to resume to run. > > > > > > Make "enfore_cpuid=true" a hard requirement for a guest to be > > > migratable, and change the default value of "enforce_cpuid" to true, > > > making the guest vCPUs migratable by default. If the destination stub > > > guest has inconsistent CPUIDs (i.e., destination host cannot support > > > the features defined by the guest's vCPU model), it fails to boot > > > (with enfore_cpuid=true by default), thereby preventing migration from > > > occuring. If enfore_cpuid=false is explicitly added for the guest, the > > > guest is deemed as non-migratable (via the migration blocker), so the > > > above issue won't occur as the guest won't be migrated. > > > > Blocking migration when enforce=false is making an assumption that users of > > that setting are inherantly broken. This is NOT the case if the user/app has > > already validated compatibility in some manner outside QEMU. Blocking > > migration in this case will break valid working use cases. > > It's just an enforcement to ensure a safe migration. Without this (i.e., the current > QEMU code) is making an assumption that users always have validated > compatibility in a good manner outside QEMU, which is risky to some degree? QEMU configurations must never be assumed to be migratable by default. There is a huge set of things that a user must do with QEMU configuration to guarantee migratability beyond CPU features. All aspects of guest HW device topology must be set explicitly. > Do you see how this would break valid working use cases (any examples)? > This is actually what we are looking for. Please be aware that "enforce" is > changed to be true by default to make the guest to be migratable by default > under the enforcement. Setting "enforce" will break existing use of QEMU. It is valid to launch QEMU with a CPU model that is not fully supported by the host, allowing QEMU to disable unsupported features automatically. > > IMHO this patch doesn't need to exist. If users of QEMU want strong protection > > they can already opt-in to that with enforce=true. > > AFAIK, many users are not aware of this, and also we couldn't assume everybody > knows it. That's why we want to add the enforcement. Users who directly launch QEMU are expected to know about QEMU config details for migration. If they don't, then they ought to be using a higher level tool like libvirt, which ensures the configuration is migration compatible. With regards, Daniel
On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > On Thu, Jul 11, 2024 at 12:10:34PM +0000, Wang, Wei W wrote: > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > > When enforce_cpuid is set to false, the guest is launched with a > > > > filtered set of features, meaning that unsupported features by the > > > > host are removed from the guest's vCPU model. This could cause > > > > issues for > > > live migration. > > > > For example, a guest on the source is running with features A and B. > > > > If the destination host does not support feature B, the stub guest > > > > can still be launched on the destination with feature A only if > enforce_cpuid=false. > > > > Live migration can start in this case, though it may fail later > > > > when the states of feature B are put to the destination side. This > > > > failure occurs in the late stage (i.e., stop© phase) of the > > > > migration flow, where the source guest has already been paused. > > > > Tests show that in such cases the source guest does not recover, > > > > and the destination is unable to resume to run. > > > > > > > > Make "enfore_cpuid=true" a hard requirement for a guest to be > > > > migratable, and change the default value of "enforce_cpuid" to > > > > true, making the guest vCPUs migratable by default. If the > > > > destination stub guest has inconsistent CPUIDs (i.e., destination > > > > host cannot support the features defined by the guest's vCPU > > > > model), it fails to boot (with enfore_cpuid=true by default), > > > > thereby preventing migration from occuring. If enfore_cpuid=false > > > > is explicitly added for the guest, the guest is deemed as > > > > non-migratable (via the migration blocker), so the above issue won't > occur as the guest won't be migrated. > > > > > > Blocking migration when enforce=false is making an assumption that > > > users of that setting are inherantly broken. This is NOT the case if > > > the user/app has already validated compatibility in some manner > > > outside QEMU. Blocking migration in this case will break valid working use > cases. > > > > It's just an enforcement to ensure a safe migration. Without this > > (i.e., the current QEMU code) is making an assumption that users > > always have validated compatibility in a good manner outside QEMU, which > is risky to some degree? > > QEMU configurations must never be assumed to be migratable by default. > There is a huge set of things that a user must do with QEMU configuration to > guarantee migratability beyond CPU features. All aspects of guest HW device > topology must be set explicitly. What if the source and destination are required to use exactly the same QEMU commands? Does this meet the feature and topology requirements as you mentioned above? > > > Do you see how this would break valid working use cases (any examples)? > > This is actually what we are looking for. Please be aware that > > "enforce" is changed to be true by default to make the guest to be > > migratable by default under the enforcement. > > Setting "enforce" will break existing use of QEMU. It is valid to launch QEMU > with a CPU model that is not fully supported by the host, allowing QEMU to > disable unsupported features automatically. > Yeah, I know that's valid to filter unsupported features. The only difference is that using the new QEMU (i.e., with this patch) will require users to explicitly set "enforce=off", it's not the default behavior any more. Still couldn’t understand the impact (of the default value change) on the existing use of QEMU, though. > > > IMHO this patch doesn't need to exist. If users of QEMU want strong > > > protection they can already opt-in to that with enforce=true. > > > > AFAIK, many users are not aware of this, and also we couldn't assume > > everybody knows it. That's why we want to add the enforcement. > > Users who directly launch QEMU are expected to know about QEMU config > details for migration. If they don't, then they ought to be using a higher level > tool like libvirt, which ensures the configuration is migration compatible. Could you explain how libvirt provides a more reliable assurance of migration compatibility in its configuration compared to using raw QEMU commands? Per my understanding, libvirt configs mostly map to the QEMU commands.
On Thu, Jul 11, 2024 at 01:48:59PM +0000, Wang, Wei W wrote: > On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 12:10:34PM +0000, Wang, Wei W wrote: > > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > > > When enforce_cpuid is set to false, the guest is launched with a > > > > > filtered set of features, meaning that unsupported features by the > > > > > host are removed from the guest's vCPU model. This could cause > > > > > issues for > > > > live migration. > > > > > For example, a guest on the source is running with features A and B. > > > > > If the destination host does not support feature B, the stub guest > > > > > can still be launched on the destination with feature A only if > > enforce_cpuid=false. > > > > > Live migration can start in this case, though it may fail later > > > > > when the states of feature B are put to the destination side. This > > > > > failure occurs in the late stage (i.e., stop© phase) of the > > > > > migration flow, where the source guest has already been paused. > > > > > Tests show that in such cases the source guest does not recover, > > > > > and the destination is unable to resume to run. > > > > > > > > > > Make "enfore_cpuid=true" a hard requirement for a guest to be > > > > > migratable, and change the default value of "enforce_cpuid" to > > > > > true, making the guest vCPUs migratable by default. If the > > > > > destination stub guest has inconsistent CPUIDs (i.e., destination > > > > > host cannot support the features defined by the guest's vCPU > > > > > model), it fails to boot (with enfore_cpuid=true by default), > > > > > thereby preventing migration from occuring. If enfore_cpuid=false > > > > > is explicitly added for the guest, the guest is deemed as > > > > > non-migratable (via the migration blocker), so the above issue won't > > occur as the guest won't be migrated. > > > > > > > > Blocking migration when enforce=false is making an assumption that > > > > users of that setting are inherantly broken. This is NOT the case if > > > > the user/app has already validated compatibility in some manner > > > > outside QEMU. Blocking migration in this case will break valid working use > > cases. > > > > > > It's just an enforcement to ensure a safe migration. Without this > > > (i.e., the current QEMU code) is making an assumption that users > > > always have validated compatibility in a good manner outside QEMU, which > > is risky to some degree? > > > > QEMU configurations must never be assumed to be migratable by default. > > There is a huge set of things that a user must do with QEMU configuration to > > guarantee migratability beyond CPU features. All aspects of guest HW device > > topology must be set explicitly. > > What if the source and destination are required to use exactly the same QEMU > commands? Does this meet the feature and topology requirements as you > mentioned above? That is insufficient as it does not take account of device hotplug. > > > > IMHO this patch doesn't need to exist. If users of QEMU want strong > > > > protection they can already opt-in to that with enforce=true. > > > > > > AFAIK, many users are not aware of this, and also we couldn't assume > > > everybody knows it. That's why we want to add the enforcement. > > > > Users who directly launch QEMU are expected to know about QEMU config > > details for migration. If they don't, then they ought to be using a higher level > > tool like libvirt, which ensures the configuration is migration compatible. > > Could you explain how libvirt provides a more reliable assurance of migration > compatibility in its configuration compared to using raw QEMU commands? > Per my understanding, libvirt configs mostly map to the QEMU commands. Libvirt records the full details of the guest configuration required to reproduce the exact same guest ABI, even across device hotplug. This is why libvirt QEMU command lines are absolutely enourmous compared to minimalist command lines that users usuall run directly. With regards, Daniel
On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > On Thu, Jul 11, 2024 at 12:10:34PM +0000, Wang, Wei W wrote: > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > AFAIK, many users are not aware of this, and also we couldn't assume > > everybody knows it. That's why we want to add the enforcement. > > Users who directly launch QEMU are expected to know about QEMU config > details for migration. If they don't, then they ought to be using a higher level > tool like libvirt, which ensures the configuration is migration compatible. Agree that libvirt has this advantage and is more user friendly. But it doesn't seem to solve the issue mentioned by this patch - if users don't explicitly set "enforce=true" in libvirt configs for the guest, then migrating the guest across hosts with different features could still be risky. Unless there is similar enforcement in libvirt to require users to set "enforce=true" for the guest to be migratable.
On Thu, Jul 11, 2024 at 02:13:31PM +0000, Wang, Wei W wrote: > On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 12:10:34PM +0000, Wang, Wei W wrote: > > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > AFAIK, many users are not aware of this, and also we couldn't assume > > > everybody knows it. That's why we want to add the enforcement. > > > > Users who directly launch QEMU are expected to know about QEMU config > > details for migration. If they don't, then they ought to be using a higher level > > tool like libvirt, which ensures the configuration is migration compatible. > > Agree that libvirt has this advantage and is more user friendly. But it doesn't seem to > solve the issue mentioned by this patch - if users don't explicitly set "enforce=true" > in libvirt configs for the guest, then migrating the guest across hosts with different > features could still be risky. Unless there is similar enforcement in libvirt to require > users to set "enforce=true" for the guest to be migratable. Yes, libvirt takes steps to ensure CPU compatibility before migrating. With regards, Daniel
On Thursday, July 11, 2024 10:14 PM, Daniel P. Berrangé wrote: > On Thu, Jul 11, 2024 at 02:13:31PM +0000, Wang, Wei W wrote: > > On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > > > On Thu, Jul 11, 2024 at 12:10:34PM +0000, Wang, Wei W wrote: > > > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > > AFAIK, many users are not aware of this, and also we couldn't > > > > assume everybody knows it. That's why we want to add the enforcement. > > > > > > Users who directly launch QEMU are expected to know about QEMU > > > config details for migration. If they don't, then they ought to be > > > using a higher level tool like libvirt, which ensures the configuration is > migration compatible. > > > > Agree that libvirt has this advantage and is more user friendly. But > > it doesn't seem to solve the issue mentioned by this patch - if users don't > explicitly set "enforce=true" > > in libvirt configs for the guest, then migrating the guest across > > hosts with different features could still be risky. Unless there is > > similar enforcement in libvirt to require users to set "enforce=true" for the > guest to be migratable. > > Yes, libvirt takes steps to ensure CPU compatibility before migrating. Thanks for sharing, but curious about those steps. For example, with "enforce=off" (by default), features on the destination could be filtered by QEMU (kind of silently, just has warning logs). How would the source side libvirt get informed about more features getting filtered by the destination side QEMU (as the destination host has less support for this vcpu model)? This causes inconsistencies.
On Thu, Jul 11, 2024 at 03:09:28PM +0000, Wang, Wei W wrote: > On Thursday, July 11, 2024 10:14 PM, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 02:13:31PM +0000, Wang, Wei W wrote: > > > On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > > > > On Thu, Jul 11, 2024 at 12:10:34PM +0000, Wang, Wei W wrote: > > > > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > > > AFAIK, many users are not aware of this, and also we couldn't > > > > > assume everybody knows it. That's why we want to add the enforcement. > > > > > > > > Users who directly launch QEMU are expected to know about QEMU > > > > config details for migration. If they don't, then they ought to be > > > > using a higher level tool like libvirt, which ensures the configuration is > > migration compatible. > > > > > > Agree that libvirt has this advantage and is more user friendly. But > > > it doesn't seem to solve the issue mentioned by this patch - if users don't > > explicitly set "enforce=true" > > > in libvirt configs for the guest, then migrating the guest across > > > hosts with different features could still be risky. Unless there is > > > similar enforcement in libvirt to require users to set "enforce=true" for the > > guest to be migratable. > > > > Yes, libvirt takes steps to ensure CPU compatibility before migrating. > > Thanks for sharing, but curious about those steps. For example, with "enforce=off" > (by default), features on the destination could be filtered by QEMU (kind of silently, > just has warning logs). > How would the source side libvirt get informed about more features getting filtered by > the destination side QEMU (as the destination host has less support for this vcpu model)? > This causes inconsistencies. Libvirt queries QEMU to ask it what features were actually active in the guest. With regards, Daniel
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), DEFINE_PROP_BOOL("x-force-features", X86CPU, force_features, false), DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0), diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index dd8b0f3313..aee717c1cf 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1741,7 +1741,7 @@ static int hyperv_init_vcpu(X86CPU *cpu) return 0; } -static Error *invtsc_mig_blocker; +static Error *cpu_mig_blocker; #define KVM_MAX_CPUID_ENTRIES 100 @@ -2012,6 +2012,15 @@ full: abort(); } +static bool kvm_vcpu_need_block_migration(X86CPU *cpu) +{ + CPUX86State *env = &cpu->env; + + return !cpu->enforce_cpuid || + (!env->user_tsc_khz && (env->features[FEAT_8000_0007_EDX] & + CPUID_APM_INVTSC)); +} + int kvm_arch_init_vcpu(CPUState *cs) { struct { @@ -2248,18 +2257,14 @@ int kvm_arch_init_vcpu(CPUState *cs) has_msr_mcg_ext_ctl = has_msr_feature_control = true; } - if (!env->user_tsc_khz) { - if ((env->features[FEAT_8000_0007_EDX] & CPUID_APM_INVTSC) && - invtsc_mig_blocker == NULL) { - error_setg(&invtsc_mig_blocker, - "State blocked by non-migratable CPU device" - " (invtsc flag)"); - r = migrate_add_blocker(&invtsc_mig_blocker, &local_err); + if (!cpu_mig_blocker && kvm_vcpu_need_block_migration(cpu)) { + error_setg(&cpu_mig_blocker, + "State blocked by non-migratable CPU device"); + r = migrate_add_blocker(&cpu_mig_blocker, &local_err); if (r < 0) { error_report_err(local_err); return r; } - } } if (cpu->vmware_cpuid_freq @@ -2312,7 +2317,7 @@ int kvm_arch_init_vcpu(CPUState *cs) return 0; fail: - migrate_del_blocker(&invtsc_mig_blocker); + migrate_del_blocker(&cpu_mig_blocker); return r; }