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

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

[Eduardo Habkost]

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

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

[Hu, Robert]

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Monday, September 30, 2019 22:11
> To: Jiri Denemark <jdenemar@redhat.com>
> Cc: libvir-list@redhat.com; qemu-devel@nongnu.org; Paolo Bonzini
> <pbonzini@redhat.com>; Daniel P. Berrangé <berrange@redhat.com>; Kang,
> Luwei <luwei.kang@intel.com>; thomas.lendacky@amd.com; Robert Hoo
> <robert.hu@linux.intel.com>; Richard Henderson <rth@twiddle.net>; Jiri
> Denemark <jdenemar@redhat.com>; Huang, Kai <kai.huang@intel.com>; Hu,
> Robert <robert.hu@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@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

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

[Paolo Bonzini]

On 30/09/19 16:31, Hu, Robert wrote:
>> 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.

It's really, really unlikely.  It's possible that some future processor
overloads PCONFIG in such a way that it will become virtualizable, but
not IceLake.

Would it make sense for libvirt to treat absent CPU flags as "default
off" during migration, so that it can leave out the flag in the command
line if it's off?  If it's on, libvirt would pass pconfig=on as usual.
This is a variant of [2], but more generally applicable:

> [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.


Paolo

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

[Jiri Denemark]

On Mon, Sep 30, 2019 at 17:16:27 +0200, Paolo Bonzini wrote:
> On 30/09/19 16:31, Hu, Robert wrote:
> >> 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.
> 
> It's really, really unlikely.  It's possible that some future processor
> overloads PCONFIG in such a way that it will become virtualizable, but
> not IceLake.

I guess, the likelihood of this happening would be similar to
reintroducing other features, such as osxsave or ospke, right?

> Would it make sense for libvirt to treat absent CPU flags as "default
> off" during migration, so that it can leave out the flag in the command
> line if it's off?  If it's on, libvirt would pass pconfig=on as usual.
> This is a variant of [2], but more generally applicable:
> 
> > [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.

The domain XML does not contain a complete list of all CPU features.
Features which are implicitly included in a CPU model are not listed in
the XML. Count in the differences in libvirt's vs QEMU's definitions of
a particular CPU model and you can see feat=off cannot be mechanically
dropped from the command line as the CPU model itself could turn it on
by default and thus feat=off is not redundant.

Jirka

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

[Paolo Bonzini]

On 30/09/19 18:16, Jiri Denemark wrote:
> On Mon, Sep 30, 2019 at 17:16:27 +0200, Paolo Bonzini wrote:
>> On 30/09/19 16:31, Hu, Robert wrote:
>>>> 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.
>>
>> It's really, really unlikely.  It's possible that some future processor
>> overloads PCONFIG in such a way that it will become virtualizable, but
>> not IceLake.
> 
> I guess, the likelihood of this happening would be similar to
> reintroducing other features, such as osxsave or ospke, right?

No, haveing osxsave and ospke was a mistake in the first place (they are
not CPU features at all; they are more like a special way to let
unprivileged programs read some bits of CR4).  For pconfig, it's just
very unlikely.

>> Would it make sense for libvirt to treat absent CPU flags as "default
>> off" during migration, so that it can leave out the flag in the command
>> line if it's off?  If it's on, libvirt would pass pconfig=on as usual.
>> This is a variant of [2], but more generally applicable:
>>
>>> [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.
> 
> The domain XML does not contain a complete list of all CPU features.
> Features which are implicitly included in a CPU model are not listed in
> the XML. Count in the differences in libvirt's vs QEMU's definitions of
> a particular CPU model and you can see feat=off cannot be mechanically
> dropped from the command line as the CPU model itself could turn it on
> by default and thus feat=off is not redundant.

I think I wasn't very clear, I meant "unsupported by QEMU" when I said
"absent".  Libvirt on the destination knows that from
query-cpu-model-expansion, so it can leave off pconfig if it is not
supported by the destination QEMU.

Paolo

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

[Jiri Denemark]

On Tue, Oct 01, 2019 at 11:20:42 +0200, Paolo Bonzini wrote:
> On 30/09/19 18:16, Jiri Denemark wrote:
> > On Mon, Sep 30, 2019 at 17:16:27 +0200, Paolo Bonzini wrote:
> >> On 30/09/19 16:31, Hu, Robert wrote:
> >>>> 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.
> >>
> >> It's really, really unlikely.  It's possible that some future processor
> >> overloads PCONFIG in such a way that it will become virtualizable, but
> >> not IceLake.
> > 
> > I guess, the likelihood of this happening would be similar to
> > reintroducing other features, such as osxsave or ospke, right?
> 
> No, haveing osxsave and ospke was a mistake in the first place (they are
> not CPU features at all; they are more like a special way to let
> unprivileged programs read some bits of CR4).  For pconfig, it's just
> very unlikely.
> 
> >> Would it make sense for libvirt to treat absent CPU flags as "default
> >> off" during migration, so that it can leave out the flag in the command
> >> line if it's off?  If it's on, libvirt would pass pconfig=on as usual.
> >> This is a variant of [2], but more generally applicable:
> >>
> >>> [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.
> > 
> > The domain XML does not contain a complete list of all CPU features.
> > Features which are implicitly included in a CPU model are not listed in
> > the XML. Count in the differences in libvirt's vs QEMU's definitions of
> > a particular CPU model and you can see feat=off cannot be mechanically
> > dropped from the command line as the CPU model itself could turn it on
> > by default and thus feat=off is not redundant.
> 
> I think I wasn't very clear, I meant "unsupported by QEMU" when I said
> "absent".  Libvirt on the destination knows that from
> query-cpu-model-expansion, so it can leave off pconfig if it is not
> supported by the destination QEMU.

Oh yeah, we should do this (and I plan to do so), but it won't really
help us in this case. Although it could potentially save us some work in
case we end up in a similar situation.

Jirka

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

[Huang, Kai]

On Mon, 2019-09-30 at 17:16 +0200, Paolo Bonzini wrote:
> On 30/09/19 16:31, Hu, Robert wrote:
> > > 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.
> 
> It's really, really unlikely.  It's possible that some future processor
> overloads PCONFIG in such a way that it will become virtualizable, but
> not IceLake.

I agree. Not Icelake.

Thanks,
-Kai

> 
> Would it make sense for libvirt to treat absent CPU flags as "default
> off" during migration, so that it can leave out the flag in the command
> line if it's off?  If it's on, libvirt would pass pconfig=on as usual.
> This is a variant of [2], but more generally applicable:
> 
> > [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.
> 
> Paolo