Libguestfs - May 2022 - [PATCH] always 'max' for the appliance CPU model on all targes except ppc

On Wed, 25 May 2022 at 16:07, Laszlo Ersek <lersek at redhat.com>
wrote:
>
> + Drew & Peter
>
> On 05/25/22 15:30, Daniel P. Berrang? wrote:
> - The patch seems to do what it says in the commit message.
>
> - QEMU commit bab52d4bba3f ("target/arm: Add "-cpu max"
support",
>   2018-03-09) confirms what the commit message says, about both TCG and
>   KVM.
>
> - To smoke-test the TCG-related change, I've edited a long-term TCG
>   aarch64 libvirt domain of mine, replacing "cortex-a57" with
"max".
>   Both edk2 and the Linux guest continued working. So I guess the TCG
>   change is OK.
One thing to note here is that if you are using:
 * TCG -cpu max
 * 'virt' with no named version or with 'virt-7.0' or later
 * a Linux kernel version prior to v5.12
then a bug in Linux means it won't boot. (This is because of
the LPA2 CPU feature which TCG -cpu max now emulates; older
kernels were buggy and won't boot on an LPA2 CPU, including
a real hardware one.)
You might or might not feel this is something worth noting in
release notes or equivalent.

> - In additional support of the above, QEMU commit ddaebdda53fc
>   ("target/arm: Unindent unnecessary else-clause", 2022-02-21)
added a
>   comment saying
>
>   /* '-cpu max' for TCG: we currently do this as "A57 with
extra things" */
>
> - Although I was more surprised by the TCG-related statement initially
>   (i.e. that "max" was a superset of "cortex-a57" when
using TCG), now
>   I'm actually more concerned about the KVM case.
The TCG part is an implementation convenience (mostly it just
means that 'max' has the A57's IMPDEF registers).
>   Specifically QEMU commit 0baa21be497d ("target/arm: Make KVM -cpu
max
>   exactly like -cpu host", 2022-02-21) eliminated a difference where
>   "-cpu max" had been a superset of "-cpu host",
featuring the
>   "sve-max-vq" extra property.
>
>   The fix is part of release v7.0.0.
>
>   The difference was introduced in commits
>
>   [1] 6fa8a37949d9 ("target/arm/cpu64: max cpu: Support sve properties
>       with KVM", 2019-11-01)
>
>   [2] 87014c6b3660 ("target/arm/kvm: host cpu: Add support for
sve<N>
>       properties", 2019-11-01)
>
>   and apparently *deliberately*.
No, this was unintentional. See the discussion in this thread:
https://lore.kernel.org/qemu-devel/20220203173640.shxkmatdcsfzzvtj at gator/

In particular, although KVM '-cpu max' had the sve-max-vq
property, Drew notes "sve-max-vq won't work for any of the machines
that
support SVE that I know of". Which is why we removed it, rather
than adding it to '-cpu host'.
>   Therefore it seems that starting with qemu-4.2, but strictly preceding
>   qemu-7.0, "-cpu max" and "-cpu host" are not
"identical" when KVM is
>   enabled; "-cpu max" has more features. Because of that, I think
there
>   are two options:
>
>   (a) This extra feature is actually harmless, so we should only update
>       the commit message (i.e., generally speaking, "-cpu max"
has been
>       a superset of "-cpu host" on KVM and of "-cpu
cortex-a57" on TCG).
>
>   (b) The feature actually presents a problem, and qemu in [v4.2.0,
>       v7.0.0) will not start when KVM accel and "-cpu max" are
requested
>       simultaneously. In this case, I think the appliance needs to stick
>       with "-cpu host" on KVM.
I don't understand why you think these are the only two options. The
actual situation is:

 (c) -cpu max and -cpu host have always been identical on KVM,
 and this commit does not change that.
 There happens to have been a QOM property 'sve-max-vq' on 'max'
 that should not have existed there and that nobody can actually have
 been usefully setting, but now there isn't.

thanks
-- PMM

On Wed, May 25, 2022 at 05:13:53PM +0100, Peter Maydell
wrote:
> On Wed, 25 May 2022 at 16:07, Laszlo Ersek <lersek at redhat.com>
wrote:
> >
> > + Drew & Peter
> >
> > On 05/25/22 15:30, Daniel P. Berrang? wrote:
> > - The patch seems to do what it says in the commit message.
> >
> > - QEMU commit bab52d4bba3f ("target/arm: Add "-cpu max"
support",
> >   2018-03-09) confirms what the commit message says, about both TCG
and
> >   KVM.
> >
> > - To smoke-test the TCG-related change, I've edited a long-term
TCG
> >   aarch64 libvirt domain of mine, replacing "cortex-a57"
with "max".
> >   Both edk2 and the Linux guest continued working. So I guess the TCG
> >   change is OK.
> 
> One thing to note here is that if you are using:
>  * TCG -cpu max
>  * 'virt' with no named version or with 'virt-7.0' or later
>  * a Linux kernel version prior to v5.12
> then a bug in Linux means it won't boot. (This is because of
> the LPA2 CPU feature which TCG -cpu max now emulates; older
> kernels were buggy and won't boot on an LPA2 CPU, including
> a real hardware one.)
Is this related at all to the 5-level page tables (la57) failure with
TCG and -cpu max?

https://gitlab.com/qemu-project/qemu/-/issues/1023

Rich.
> You might or might not feel this is something worth noting in
> release notes or equivalent.
> 
> 
> > - In additional support of the above, QEMU commit ddaebdda53fc
> >   ("target/arm: Unindent unnecessary else-clause",
2022-02-21) added a
> >   comment saying
> >
> >   /* '-cpu max' for TCG: we currently do this as "A57
with extra things" */
> >
> > - Although I was more surprised by the TCG-related statement initially
> >   (i.e. that "max" was a superset of "cortex-a57"
when using TCG), now
> >   I'm actually more concerned about the KVM case.
> 
> The TCG part is an implementation convenience (mostly it just
> means that 'max' has the A57's IMPDEF registers).
> 
> >   Specifically QEMU commit 0baa21be497d ("target/arm: Make KVM
-cpu max
> >   exactly like -cpu host", 2022-02-21) eliminated a difference
where
> >   "-cpu max" had been a superset of "-cpu host",
featuring the
> >   "sve-max-vq" extra property.
> >
> >   The fix is part of release v7.0.0.
> >
> >   The difference was introduced in commits
> >
> >   [1] 6fa8a37949d9 ("target/arm/cpu64: max cpu: Support sve
properties
> >       with KVM", 2019-11-01)
> >
> >   [2] 87014c6b3660 ("target/arm/kvm: host cpu: Add support for
sve<N>
> >       properties", 2019-11-01)
> >
> >   and apparently *deliberately*.
> 
> No, this was unintentional. See the discussion in this thread:
> https://lore.kernel.org/qemu-devel/20220203173640.shxkmatdcsfzzvtj at
gator/
> 
> In particular, although KVM '-cpu max' had the sve-max-vq
> property, Drew notes "sve-max-vq won't work for any of the
machines that
> support SVE that I know of". Which is why we removed it, rather
> than adding it to '-cpu host'.
> 
> >   Therefore it seems that starting with qemu-4.2, but strictly
preceding
> >   qemu-7.0, "-cpu max" and "-cpu host" are not
"identical" when KVM is
> >   enabled; "-cpu max" has more features. Because of that, I
think there
> >   are two options:
> >
> >   (a) This extra feature is actually harmless, so we should only
update
> >       the commit message (i.e., generally speaking, "-cpu
max" has been
> >       a superset of "-cpu host" on KVM and of "-cpu
cortex-a57" on TCG).
> >
> >   (b) The feature actually presents a problem, and qemu in [v4.2.0,
> >       v7.0.0) will not start when KVM accel and "-cpu max"
are requested
> >       simultaneously. In this case, I think the appliance needs to
stick
> >       with "-cpu host" on KVM.
> 
> I don't understand why you think these are the only two options. The
> actual situation is:
> 
>  (c) -cpu max and -cpu host have always been identical on KVM,
>  and this commit does not change that.
>  There happens to have been a QOM property 'sve-max-vq' on
'max'
>  that should not have existed there and that nobody can actually have
>  been usefully setting, but now there isn't.
> 
> thanks
> -- PMM
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wed, May 25, 2022 at 05:13:53PM +0100, Peter Maydell
wrote:
> On Wed, 25 May 2022 at 16:07, Laszlo Ersek <lersek at redhat.com>
wrote:
...
> >   Therefore it seems that starting with qemu-4.2, but strictly
preceding
> >   qemu-7.0, "-cpu max" and "-cpu host" are not
"identical" when KVM is
> >   enabled; "-cpu max" has more features. Because of that, I
think there
> >   are two options:
> >
> >   (a) This extra feature is actually harmless, so we should only
update
> >       the commit message (i.e., generally speaking, "-cpu
max" has been
> >       a superset of "-cpu host" on KVM and of "-cpu
cortex-a57" on TCG).
> >
> >   (b) The feature actually presents a problem, and qemu in [v4.2.0,
> >       v7.0.0) will not start when KVM accel and "-cpu max"
are requested
> >       simultaneously. In this case, I think the appliance needs to
stick
> >       with "-cpu host" on KVM.
> 
> I don't understand why you think these are the only two options. The
> actual situation is:
> 
>  (c) -cpu max and -cpu host have always been identical on KVM,
>  and this commit does not change that.
>  There happens to have been a QOM property 'sve-max-vq' on
'max'
>  that should not have existed there and that nobody can actually have
>  been usefully setting, but now there isn't.
>
Hi Peter,

I think I understand Laszlo's concern. If we advertise 'max' as
effectively being an alias for 'host' when accel=kvm, then we
should be able to take any given '-cpu max,...' command line
parameter and replace it with '-cpu host,...' and have it still
work. That's not the case, at least when sve-max-vq, and I think
maybe also pauth-impdef, are used.

Also, if we did want max and host to be aliases for accel=kvm, then
that implies we need max to work for all '-cpu host,...' with
accel=tcg as well. And, in that case, we'd need max with TCG to
"support" kvm-no-adjvtime and kvm-steal-time, which doesn't make
much sense.

I think the "solution" is to not infer that max and host are
effectively aliases allowing seamless transition from tcg to kvm
and back. One may treat them as aliases when any additional
properties provided are from the intersection of their supported
properties, though.

In summary, if an appliance doesn't provide additional CPU properties
or it selects only properties that intersect TCG and KVM, then,
regarding the CPU, when 'max' is used, it can seamlessly change the
accelerator. Otherwise, while the appliance can leave the CPU as 'max'
in all cases, it will need to modify selected CPU properties depending
on the accelerator.

Thanks,
drew

On Thu, May 26, 2022 at 10:10:02AM +0200, Andrew Jones
wrote:
> On Wed, May 25, 2022 at 05:13:53PM +0100, Peter Maydell wrote:
> > On Wed, 25 May 2022 at 16:07, Laszlo Ersek <lersek at
redhat.com> wrote:
> ...
> > >   Therefore it seems that starting with qemu-4.2, but strictly
preceding
> > >   qemu-7.0, "-cpu max" and "-cpu host" are
not "identical" when KVM is
> > >   enabled; "-cpu max" has more features. Because of
that, I think there
> > >   are two options:
> > >
> > >   (a) This extra feature is actually harmless, so we should only
update
> > >       the commit message (i.e., generally speaking, "-cpu
max" has been
> > >       a superset of "-cpu host" on KVM and of
"-cpu cortex-a57" on TCG).
> > >
> > >   (b) The feature actually presents a problem, and qemu in
[v4.2.0,
> > >       v7.0.0) will not start when KVM accel and "-cpu
max" are requested
> > >       simultaneously. In this case, I think the appliance needs
to stick
> > >       with "-cpu host" on KVM.
> > 
> > I don't understand why you think these are the only two options.
The
> > actual situation is:
> > 
> >  (c) -cpu max and -cpu host have always been identical on KVM,
> >  and this commit does not change that.
> >  There happens to have been a QOM property 'sve-max-vq' on
'max'
> >  that should not have existed there and that nobody can actually have
> >  been usefully setting, but now there isn't.
> >
> 
> Hi Peter,
> 
> I think I understand Laszlo's concern. If we advertise 'max' as
> effectively being an alias for 'host' when accel=kvm, then we
> should be able to take any given '-cpu max,...' command line
> parameter and replace it with '-cpu host,...' and have it still
> work. That's not the case, at least when sve-max-vq, and I think
> maybe also pauth-impdef, are used.
> 
> Also, if we did want max and host to be aliases for accel=kvm, then
> that implies we need max to work for all '-cpu host,...' with
> accel=tcg as well. And, in that case, we'd need max with TCG to
> "support" kvm-no-adjvtime and kvm-steal-time, which doesn't
make
> much sense.
> 
> I think the "solution" is to not infer that max and host are
> effectively aliases allowing seamless transition from tcg to kvm
> and back. One may treat them as aliases when any additional
> properties provided are from the intersection of their supported
> properties, though.
> 
> In summary, if an appliance doesn't provide additional CPU properties
> or it selects only properties that intersect TCG and KVM, then,
> regarding the CPU, when 'max' is used, it can seamlessly change the
> accelerator. Otherwise, while the appliance can leave the CPU as
'max'
> in all cases, it will need to modify selected CPU properties depending
> on the accelerator.
We've never said that 'max' is the same for TCG and KVM, nor do
apps using it require/expect that to be the case.

It is simply intended to expose the maximum featureset available to
any given accelerator backend. On KVM "maximum featureset" is the
same as "host" as you can't expose more than what the hardware
has,
while on TCG "maximum featureset" is the most that emulation supports.

The intent is/was that it serves as good CPU choice for apps which
maximises features available, without them needing to think.

Essentially every app should pick 'max' by default unless they need
to restrict features for sake of live migration compatibility, or
need to select a very specific model for some functional reason.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 05/25/22 18:13, Peter Maydell wrote:
> On Wed, 25 May 2022 at 16:07, Laszlo Ersek <lersek at redhat.com>
wrote:
>>
>> + Drew & Peter
>>
>> On 05/25/22 15:30, Daniel P. Berrang? wrote:
>> - The patch seems to do what it says in the commit message.
>>
>> - QEMU commit bab52d4bba3f ("target/arm: Add "-cpu max"
support",
>>   2018-03-09) confirms what the commit message says, about both TCG and
>>   KVM.
>>
>> - To smoke-test the TCG-related change, I've edited a long-term TCG
>>   aarch64 libvirt domain of mine, replacing "cortex-a57" with
"max".
>>   Both edk2 and the Linux guest continued working. So I guess the TCG
>>   change is OK.
> 
> One thing to note here is that if you are using:
>  * TCG -cpu max
>  * 'virt' with no named version or with 'virt-7.0' or later
>  * a Linux kernel version prior to v5.12
> then a bug in Linux means it won't boot. (This is because of
> the LPA2 CPU feature which TCG -cpu max now emulates; older
> kernels were buggy and won't boot on an LPA2 CPU, including
> a real hardware one.)
> You might or might not feel this is something worth noting in
> release notes or equivalent.
> 
> 
>> - In additional support of the above, QEMU commit ddaebdda53fc
>>   ("target/arm: Unindent unnecessary else-clause",
2022-02-21) added a
>>   comment saying
>>
>>   /* '-cpu max' for TCG: we currently do this as "A57 with
extra things" */
>>
>> - Although I was more surprised by the TCG-related statement initially
>>   (i.e. that "max" was a superset of "cortex-a57"
when using TCG), now
>>   I'm actually more concerned about the KVM case.
> 
> The TCG part is an implementation convenience (mostly it just
> means that 'max' has the A57's IMPDEF registers).
> 
>>   Specifically QEMU commit 0baa21be497d ("target/arm: Make KVM
-cpu max
>>   exactly like -cpu host", 2022-02-21) eliminated a difference
where
>>   "-cpu max" had been a superset of "-cpu host",
featuring the
>>   "sve-max-vq" extra property.
>>
>>   The fix is part of release v7.0.0.
>>
>>   The difference was introduced in commits
>>
>>   [1] 6fa8a37949d9 ("target/arm/cpu64: max cpu: Support sve
properties
>>       with KVM", 2019-11-01)
>>
>>   [2] 87014c6b3660 ("target/arm/kvm: host cpu: Add support for
sve<N>
>>       properties", 2019-11-01)
>>
>>   and apparently *deliberately*.
> 
> No, this was unintentional. See the discussion in this thread:
> https://lore.kernel.org/qemu-devel/20220203173640.shxkmatdcsfzzvtj at
gator/
> 
> In particular, although KVM '-cpu max' had the sve-max-vq
> property, Drew notes "sve-max-vq won't work for any of the
machines that
> support SVE that I know of". Which is why we removed it, rather
> than adding it to '-cpu host'.
> 
>>   Therefore it seems that starting with qemu-4.2, but strictly
preceding
>>   qemu-7.0, "-cpu max" and "-cpu host" are not
"identical" when KVM is
>>   enabled; "-cpu max" has more features. Because of that, I
think there
>>   are two options:
>>
>>   (a) This extra feature is actually harmless, so we should only update
>>       the commit message (i.e., generally speaking, "-cpu
max" has been
>>       a superset of "-cpu host" on KVM and of "-cpu
cortex-a57" on TCG).
>>
>>   (b) The feature actually presents a problem, and qemu in [v4.2.0,
>>       v7.0.0) will not start when KVM accel and "-cpu max"
are requested
>>       simultaneously. In this case, I think the appliance needs to
stick
>>       with "-cpu host" on KVM.
> 
> I don't understand why you think these are the only two options. The
> actual situation is:
> 
>  (c) -cpu max and -cpu host have always been identical on KVM,
>  and this commit does not change that.
>  There happens to have been a QOM property 'sve-max-vq' on
'max'
>  that should not have existed there and that nobody can actually have
>  been usefully setting, but now there isn't.
Ah, so that's my big misunderstanding -- I didn't realize it was *just*
a QOM property. I thought it caused a guest-visible VCPU feature to
appear, only on "max" (with KVM).

If that's not the case, then my concern is gone.

Thanks!
Laszlo

On 05/26/22 10:10, Andrew Jones wrote:
> On Wed, May 25, 2022 at 05:13:53PM +0100, Peter Maydell wrote:
>> On Wed, 25 May 2022 at 16:07, Laszlo Ersek <lersek at redhat.com>
wrote:
> ...
>>>   Therefore it seems that starting with qemu-4.2, but strictly
preceding
>>>   qemu-7.0, "-cpu max" and "-cpu host" are not
"identical" when KVM is
>>>   enabled; "-cpu max" has more features. Because of that,
I think there
>>>   are two options:
>>>
>>>   (a) This extra feature is actually harmless, so we should only
update
>>>       the commit message (i.e., generally speaking, "-cpu
max" has been
>>>       a superset of "-cpu host" on KVM and of "-cpu
cortex-a57" on TCG).
>>>
>>>   (b) The feature actually presents a problem, and qemu in [v4.2.0,
>>>       v7.0.0) will not start when KVM accel and "-cpu
max" are requested
>>>       simultaneously. In this case, I think the appliance needs to
stick
>>>       with "-cpu host" on KVM.
>>
>> I don't understand why you think these are the only two options.
The
>> actual situation is:
>>
>>  (c) -cpu max and -cpu host have always been identical on KVM,
>>  and this commit does not change that.
>>  There happens to have been a QOM property 'sve-max-vq' on
'max'
>>  that should not have existed there and that nobody can actually have
>>  been usefully setting, but now there isn't.
>>
> 
> Hi Peter,
> 
> I think I understand Laszlo's concern. If we advertise 'max' as
> effectively being an alias for 'host' when accel=kvm, then we
> should be able to take any given '-cpu max,...' command line
> parameter and replace it with '-cpu host,...' and have it still
> work.
My concern was not about command line compatibility (libguestfs doesn't
tend to pack the command line with lots of nifty properties); instead I
thought there was a guest ABI difference between these two VCPU types on
KVM. Peter says (IIUC) that there never has been one, so that's good.
> That's not the case, at least when sve-max-vq, and I think
> maybe also pauth-impdef, are used.
> 
> Also, if we did want max and host to be aliases for accel=kvm, then
> that implies we need max to work for all '-cpu host,...' with
> accel=tcg as well. And, in that case, we'd need max with TCG to
> "support" kvm-no-adjvtime and kvm-steal-time, which doesn't
make
> much sense.
> 
> I think the "solution" is to not infer that max and host are
> effectively aliases allowing seamless transition from tcg to kvm
> and back. One may treat them as aliases when any additional
> properties provided are from the intersection of their supported
> properties, though.
> 
> In summary, if an appliance doesn't provide additional CPU properties
> or it selects only properties that intersect TCG and KVM, then,
> regarding the CPU, when 'max' is used, it can seamlessly change the
> accelerator.
Yes, I think this is the case with libguestfs's appliance (for aarch64
anyway): it does not provide additional CPU properties (that I know of).

Thanks!
Laszlo
> Otherwise, while the appliance can leave the CPU as 'max'
> in all cases, it will need to modify selected CPU properties depending
> on the accelerator.
> 
> Thanks,
> drew
>