[libvirt] [RFC] cpu_map: Remove pconfig from Icelake-Server CPU model

[Hu, Robert]

> -----Original Message-----
> From: Eduardo Habkost <ehabkost at redhat.com>
> Sent: Monday, September 30, 2019 22:11
> To: Jiri Denemark <jdenemar at redhat.com>
> Cc: libvir-list at redhat.com; qemu-devel at nongnu.org; Paolo Bonzini
> <pbonzini at redhat.com>; Daniel P. Berrangé <berrange at redhat.com>; Kang,
> Luwei <luwei.kang at intel.com>; thomas.lendacky at amd.com; Robert Hoo
> <robert.hu at linux.intel.com>; Richard Henderson <rth at twiddle.net>; Jiri
> Denemark <jdenemar at redhat.com>; Huang, Kai <kai.huang at intel.com>; Hu,
> Robert <robert.hu at intel.com>
> Subject: Re: [RFC] cpu_map: Remove pconfig from Icelake-Server CPU model
> 
> CCing qemu-devel and QEMU developers.
> 
> On Mon, Sep 30, 2019 at 12:24:53PM +0200, Jiri Denemark wrote:
> > On Thu, Sep 26, 2019 at 18:43:05 -0300, Eduardo Habkost wrote:
> > > The pconfig feature never worked, and adding "pconfig=off" to the
> > > QEMU command-line triggers a regression in QEMU 3.1.1 and 4.0.0.
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost at redhat.com>
> > > ---
> > > I'm sending this as an RFC because I couldn't test it properly, and
> > > because I don't know what are the consequences of changing cpu_map
> > > between libvirt versions.
> > > ---
> > >  src/cpu_map/x86_Icelake-Server.xml | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/cpu_map/x86_Icelake-Server.xml
> > > b/src/cpu_map/x86_Icelake-Server.xml
> > > index fb15977a59..188a781282 100644
> > > --- a/src/cpu_map/x86_Icelake-Server.xml
> > > +++ b/src/cpu_map/x86_Icelake-Server.xml
> > > @@ -56,7 +56,9 @@
> > >      <feature name='pat'/>
> > >      <feature name='pcid'/>
> > >      <feature name='pclmuldq'/>
> > > -    <feature name='pconfig'/>
> > > +    <!-- 'pconfig' was added by accident in QEMU 3.1.0 but never worked.
> > > +         It was removed in QEMU 3.1.1 and 4.0.0.  See QEMU commits
> > > +         76e5a4d58357 and 712f807e1965 for details -->
> > >      <feature name='pdpe1gb'/>
> > >      <feature name='pge'/>
> > >      <feature name='pku'/>
> >
> > IIUC this never worked and a domain started with Icelake-Server CPU
> > model would actually end up running with pconfig=off, right? In that
> > case removing pconfig from Icelake-Server would not cause any issues
> > when a domain is started. However, I'm afraid migration would be broken.
> >
> > If a domain is started by new libvirt (with this patch in) using
> > Icelake-Server CPU model possibly with additional features added or
> > removed, but without pconfig being explicitly mentioned, libvirt would
> > not disable pconfig when updating active definition according to the
> > actual CPU created by QEMU. This is because libvirt did not ask for
> > pconfig (either explicitly or implicitly via the CPU model). When such
> > domain gets migrated to an older libvirt (which thinks pconfig is part
> > of Icelake-Server), it will complain that QEMU disabled pconfig while
> > the source domain was running with pconfig enabled (which is an
> > incorrect result caused by inconsistent view of the CPU model).
> >
> > Thus we would need to add a hack which would explicitly disable
> > pconfig in the domain definition used for migration to make sure the
> > destination libvirtd knows pconfig was disabled. New libvirt would
> > just drop the disabled pconfig feature from the CPU definition before
> > starting a domain.
> >
> > [1] From this point of view we could just keep the CPU model in
> > libvirt untouched. This way pconfig would always be explicitly
> > disabled when a domain is running and the host-model CPU definition
> > would also show it as explicitly disabled.
> >
> > [2] However starting a domain with Icelake-Server so that it can be
> > migrated or saved/restored on QEMU in 3.1.1 and 4.0.0 would be
> > impossible. This can be solved by a different hack, which would drop
> > pconfig=off from QEMU command line.
> >
> > [3] But if pconfig was removed from QEMU and never returned back, we
> > could remove it from any domain XML we see
> > (virQEMUCapsCPUFilterFeatures mentions several other features which we
> handle this way).
> >
> > That said, I think [3] would be the best option. But if QEMU cannot or
> > doesn't want to remove pconfig completely, I'd go with [1] as the hack
> > in [2] would be too ugly and confusing.
> 
> From the QEMU side, [3] is better, as pconfig was added by accident in 3.1.0 and
> it would be simpler to not re-add it.
> 
> This might be a problem if there are plans to eventually make KVM support
> pconfig, though.  Paolo, Robert, are there plans to support pconfig in KVM in the
> future?
[Robert Hoo] 
Thanks Eduardo for efforts in resolving this issue, introduced from my Icelake CPU
model patch.
I've no idea about PCONFIG's detail and plan. Let me sync with Huang, Kai and answer
you soon.
> 
> --
> Eduardo