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

[Eduardo Habkost <ehabkost@redhat.com>]

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@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?

-- 
Eduardo