Re: [PATCH] i386: Re-add "pconfig" CPUID flag name

[Eduardo Habkost]

(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 <address@hidden> 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 <address@hidden>
> > ---
> >  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
> >
> >

-- 
Eduardo