| Message ID | 20190926212326.4092-1-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386: Re-add "pconfig" CPUID flag name | expand |
On Thu, Sep 26, 2019 at 06:23:26PM -0300, Eduardo Habkost wrote: > QEMU 3.1.0 was shipped with the "pconfig" CPU property available, > added by commit 5131dc433df5 ("i386: Add CPUID bit for PCONFIG"). > > Then the feature was removed in QEMU 4.0.0 (and 3.1.1), by commit > 712f807e1965 ("Revert 'i386: Add CPUID bit for PCONFIG'"). > > In theory this would be OK, but we do have a problem: existing > software (like libvirt) was already using "pconfig=off" since > QEMU 3.1.0 on some cases. This means software that worked with > QEMU 3.1.0 doesn't work with QEMU 3.1.1 and newer. > > One symptom is the following error being generated by > virt-install while trying to use the 'host-model' CPU model, on a > host that's identified as Icelake-Server: > > ERROR internal error: qemu unexpectedly closed the monitor: \ > 2019-09-24T22:57:42.550032Z qemu-kvm: \ > can't apply global Icelake-Server-x86_64-cpu.pconfig=off: Property '.pconfig' not found > > Re-add "pconfig" to feature_word_info[FEAT_7_0_EDX].feat_names so > "pconfig=off" will work again. > > This change still won't let users set "monitor=on" because all Sorry, I meant "pconfig=on" above. > accelerators currently report the feature as unsupported. But to > make sure PCONFIG won't be enabled by accident in the future > before we implement the necessary migration code, also add the > feature to .unmigratable_flags. > > Fixes: 712f807e1965 ("Revert 'i386: Add CPUID bit for PCONFIG'") > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target/i386/cpu.h | 2 ++ > target/i386/cpu.c | 8 +++++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 8e090acd74..b728bd22f1 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -731,6 +731,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) > /* AVX512 Multiply Accumulation Single Precision */ > #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) > +/* PCONFIG Instruction */ > +#define CPUID_7_0_EDX_PCONFIG (1U << 18) > /* Speculation Control */ > #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) > /* Arch Capabilities */ > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 71034aeb5a..3e25505bd3 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1084,7 +1084,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, "md-clear", NULL, > NULL, NULL, NULL, NULL, > - NULL, NULL, NULL /* pconfig */, NULL, > + NULL, NULL, "pconfig", NULL, > NULL, NULL, NULL, NULL, > NULL, NULL, "spec-ctrl", "stibp", > NULL, "arch-capabilities", "core-capability", "ssbd", > @@ -1095,6 +1095,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > .reg = R_EDX, > }, > .tcg_features = TCG_7_0_EDX_FEATURES, > + /* > + * CPU state altered by the PCONFIG instruction (e.g. MKTME key table) > + * is not migrated by QEMU yet, so PCONFIG is unmigratable until > + * this is implemented. > + */ > + .unmigratable_flags = CPUID_7_0_EDX_PCONFIG, > }, > [FEAT_7_1_EAX] = { > .type = CPUID_FEATURE_WORD, > -- > 2.21.0 >
Is this really needed? QEMU's value of pconfig=on vs. off should be provided by QMP CPU model queries, if a property is not available then Libvirt should not try to set it to off. Paolo Il gio 26 set 2019, 23:23 Eduardo Habkost <ehabkost@redhat.com> ha scritto: > QEMU 3.1.0 was shipped with the "pconfig" CPU property available, > added by commit 5131dc433df5 ("i386: Add CPUID bit for PCONFIG"). > > Then the feature was removed in QEMU 4.0.0 (and 3.1.1), by commit > 712f807e1965 ("Revert 'i386: Add CPUID bit for PCONFIG'"). > > In theory this would be OK, but we do have a problem: existing > software (like libvirt) was already using "pconfig=off" since > QEMU 3.1.0 on some cases. This means software that worked with > QEMU 3.1.0 doesn't work with QEMU 3.1.1 and newer. > > One symptom is the following error being generated by > virt-install while trying to use the 'host-model' CPU model, on a > host that's identified as Icelake-Server: > > ERROR internal error: qemu unexpectedly closed the monitor: \ > 2019-09-24T22:57:42.550032Z qemu-kvm: \ > can't apply global Icelake-Server-x86_64-cpu.pconfig=off: Property > '.pconfig' not found > > Re-add "pconfig" to feature_word_info[FEAT_7_0_EDX].feat_names so > "pconfig=off" will work again. > > This change still won't let users set "monitor=on" because all > accelerators currently report the feature as unsupported. But to > make sure PCONFIG won't be enabled by accident in the future > before we implement the necessary migration code, also add the > feature to .unmigratable_flags. > > Fixes: 712f807e1965 ("Revert 'i386: Add CPUID bit for PCONFIG'") > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target/i386/cpu.h | 2 ++ > target/i386/cpu.c | 8 +++++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 8e090acd74..b728bd22f1 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -731,6 +731,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) > /* AVX512 Multiply Accumulation Single Precision */ > #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) > +/* PCONFIG Instruction */ > +#define CPUID_7_0_EDX_PCONFIG (1U << 18) > /* Speculation Control */ > #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) > /* Arch Capabilities */ > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 71034aeb5a..3e25505bd3 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1084,7 +1084,7 @@ static FeatureWordInfo > feature_word_info[FEATURE_WORDS] = { > NULL, NULL, NULL, NULL, > NULL, NULL, "md-clear", NULL, > NULL, NULL, NULL, NULL, > - NULL, NULL, NULL /* pconfig */, NULL, > + NULL, NULL, "pconfig", NULL, > NULL, NULL, NULL, NULL, > NULL, NULL, "spec-ctrl", "stibp", > NULL, "arch-capabilities", "core-capability", "ssbd", > @@ -1095,6 +1095,12 @@ static FeatureWordInfo > feature_word_info[FEATURE_WORDS] = { > .reg = R_EDX, > }, > .tcg_features = TCG_7_0_EDX_FEATURES, > + /* > + * CPU state altered by the PCONFIG instruction (e.g. MKTME key > table) > + * is not migrated by QEMU yet, so PCONFIG is unmigratable until > + * this is implemented. > + */ > + .unmigratable_flags = CPUID_7_0_EDX_PCONFIG, > }, > [FEAT_7_1_EAX] = { > .type = CPUID_FEATURE_WORD, > -- > 2.21.0 > >
(CCing libvir-list) On Thu, Sep 26, 2019 at 11:58:30PM +0200, Paolo Bonzini wrote: > Is this really needed? QEMU's value of pconfig=on vs. off should be > provided by QMP CPU model queries, if a property is not available then > Libvirt should not try to set it to off. > Libvirt can easily work around it for new VMs, and it should. The issue are VMs that were created with QEMU 3.1.0. QEMU 3.1.0 was telling libvirt "Icelake-Server can't be used unless pconfig=off is used", and libvirt was adding pconfig=off to the domain XML as expected. It would be wrong for libvirt to remove a device option when migrating an existing VM to another QEMU version. We can change the rules (and document that), but do we want to? > Paolo > > Il gio 26 set 2019, 23:23 Eduardo Habkost <ehabkost@redhat.com> ha scritto: > > > QEMU 3.1.0 was shipped with the "pconfig" CPU property available, > > added by commit 5131dc433df5 ("i386: Add CPUID bit for PCONFIG"). > > > > Then the feature was removed in QEMU 4.0.0 (and 3.1.1), by commit > > 712f807e1965 ("Revert 'i386: Add CPUID bit for PCONFIG'"). > > > > In theory this would be OK, but we do have a problem: existing > > software (like libvirt) was already using "pconfig=off" since > > QEMU 3.1.0 on some cases. This means software that worked with > > QEMU 3.1.0 doesn't work with QEMU 3.1.1 and newer. > > > > One symptom is the following error being generated by > > virt-install while trying to use the 'host-model' CPU model, on a > > host that's identified as Icelake-Server: > > > > ERROR internal error: qemu unexpectedly closed the monitor: \ > > 2019-09-24T22:57:42.550032Z qemu-kvm: \ > > can't apply global Icelake-Server-x86_64-cpu.pconfig=off: Property > > '.pconfig' not found > > > > Re-add "pconfig" to feature_word_info[FEAT_7_0_EDX].feat_names so > > "pconfig=off" will work again. > > > > This change still won't let users set "monitor=on" because all > > accelerators currently report the feature as unsupported. But to > > make sure PCONFIG won't be enabled by accident in the future > > before we implement the necessary migration code, also add the > > feature to .unmigratable_flags. > > > > Fixes: 712f807e1965 ("Revert 'i386: Add CPUID bit for PCONFIG'") > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target/i386/cpu.h | 2 ++ > > target/i386/cpu.c | 8 +++++++- > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index 8e090acd74..b728bd22f1 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -731,6 +731,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > > #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) > > /* AVX512 Multiply Accumulation Single Precision */ > > #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) > > +/* PCONFIG Instruction */ > > +#define CPUID_7_0_EDX_PCONFIG (1U << 18) > > /* Speculation Control */ > > #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) > > /* Arch Capabilities */ > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index 71034aeb5a..3e25505bd3 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -1084,7 +1084,7 @@ static FeatureWordInfo > > feature_word_info[FEATURE_WORDS] = { > > NULL, NULL, NULL, NULL, > > NULL, NULL, "md-clear", NULL, > > NULL, NULL, NULL, NULL, > > - NULL, NULL, NULL /* pconfig */, NULL, > > + NULL, NULL, "pconfig", NULL, > > NULL, NULL, NULL, NULL, > > NULL, NULL, "spec-ctrl", "stibp", > > NULL, "arch-capabilities", "core-capability", "ssbd", > > @@ -1095,6 +1095,12 @@ static FeatureWordInfo > > feature_word_info[FEATURE_WORDS] = { > > .reg = R_EDX, > > }, > > .tcg_features = TCG_7_0_EDX_FEATURES, > > + /* > > + * CPU state altered by the PCONFIG instruction (e.g. MKTME key > > table) > > + * is not migrated by QEMU yet, so PCONFIG is unmigratable until > > + * this is implemented. > > + */ > > + .unmigratable_flags = CPUID_7_0_EDX_PCONFIG, > > }, > > [FEAT_7_1_EAX] = { > > .type = CPUID_FEATURE_WORD, > > -- > > 2.21.0 > > > >
On Thu, Sep 26, 2019 at 09:42:20PM -0300, Eduardo Habkost wrote: > (CCing libvir-list) > > On Thu, Sep 26, 2019 at 11:58:30PM +0200, Paolo Bonzini wrote: > > Is this really needed? QEMU's value of pconfig=on vs. off should be > > provided by QMP CPU model queries, if a property is not available then > > Libvirt should not try to set it to off. > > > > Libvirt can easily work around it for new VMs, and it should. > > The issue are VMs that were created with QEMU 3.1.0. QEMU 3.1.0 > was telling libvirt "Icelake-Server can't be used unless > pconfig=off is used", and libvirt was adding pconfig=off to the > domain XML as expected. > > It would be wrong for libvirt to remove a device option when > migrating an existing VM to another QEMU version. We can change > the rules (and document that), but do we want to? IIUC currently any existing VMs with Icelake-Server will *not* have pconfig present, since libvirt is going to set pconfig=off QEMU has now dropped pconfig CPUID entirely. If libvirt were to stop setting pconfig=off, then there is NO guest ABI change, so we'd be safe in that sense. I'll defer to Jiri on how we'd actually go about making that work in libvirt from a code POV. Regards, Daniel
On Fri, Sep 27, 2019 at 10:09:05AM +0100, Daniel P. Berrangé wrote: > On Thu, Sep 26, 2019 at 09:42:20PM -0300, Eduardo Habkost wrote: > > (CCing libvir-list) > > > > On Thu, Sep 26, 2019 at 11:58:30PM +0200, Paolo Bonzini wrote: > > > Is this really needed? QEMU's value of pconfig=on vs. off should be > > > provided by QMP CPU model queries, if a property is not available then > > > Libvirt should not try to set it to off. > > > > > > > Libvirt can easily work around it for new VMs, and it should. > > > > The issue are VMs that were created with QEMU 3.1.0. QEMU 3.1.0 > > was telling libvirt "Icelake-Server can't be used unless > > pconfig=off is used", and libvirt was adding pconfig=off to the > > domain XML as expected. > > > > It would be wrong for libvirt to remove a device option when > > migrating an existing VM to another QEMU version. We can change > > the rules (and document that), but do we want to? > > IIUC currently any existing VMs with Icelake-Server will *not* have > pconfig present, since libvirt is going to set pconfig=off > > QEMU has now dropped pconfig CPUID entirely. If libvirt were to stop > setting pconfig=off, then there is NO guest ABI change, so we'd be > safe in that sense. This is correct. Only pconfig=off worked, and no accelerator ever supported pconfig=on.
diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 8e090acd74..b728bd22f1 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -731,6 +731,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) /* AVX512 Multiply Accumulation Single Precision */ #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) +/* PCONFIG Instruction */ +#define CPUID_7_0_EDX_PCONFIG (1U << 18) /* Speculation Control */ #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Arch Capabilities */ diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 71034aeb5a..3e25505bd3 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -1084,7 +1084,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { NULL, NULL, NULL, NULL, NULL, NULL, "md-clear", NULL, NULL, NULL, NULL, NULL, - NULL, NULL, NULL /* pconfig */, NULL, + NULL, NULL, "pconfig", NULL, NULL, NULL, NULL, NULL, NULL, NULL, "spec-ctrl", "stibp", NULL, "arch-capabilities", "core-capability", "ssbd", @@ -1095,6 +1095,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .reg = R_EDX, }, .tcg_features = TCG_7_0_EDX_FEATURES, + /* + * CPU state altered by the PCONFIG instruction (e.g. MKTME key table) + * is not migrated by QEMU yet, so PCONFIG is unmigratable until + * this is implemented. + */ + .unmigratable_flags = CPUID_7_0_EDX_PCONFIG, }, [FEAT_7_1_EAX] = { .type = CPUID_FEATURE_WORD,
QEMU 3.1.0 was shipped with the "pconfig" CPU property available, added by commit 5131dc433df5 ("i386: Add CPUID bit for PCONFIG"). Then the feature was removed in QEMU 4.0.0 (and 3.1.1), by commit 712f807e1965 ("Revert 'i386: Add CPUID bit for PCONFIG'"). In theory this would be OK, but we do have a problem: existing software (like libvirt) was already using "pconfig=off" since QEMU 3.1.0 on some cases. This means software that worked with QEMU 3.1.0 doesn't work with QEMU 3.1.1 and newer. One symptom is the following error being generated by virt-install while trying to use the 'host-model' CPU model, on a host that's identified as Icelake-Server: ERROR internal error: qemu unexpectedly closed the monitor: \ 2019-09-24T22:57:42.550032Z qemu-kvm: \ can't apply global Icelake-Server-x86_64-cpu.pconfig=off: Property '.pconfig' not found Re-add "pconfig" to feature_word_info[FEAT_7_0_EDX].feat_names so "pconfig=off" will work again. This change still won't let users set "monitor=on" because all accelerators currently report the feature as unsupported. But to make sure PCONFIG won't be enabled by accident in the future before we implement the necessary migration code, also add the feature to .unmigratable_flags. Fixes: 712f807e1965 ("Revert 'i386: Add CPUID bit for PCONFIG'") Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target/i386/cpu.h | 2 ++ target/i386/cpu.c | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-)