I noticed two issues with 'kvm-asyncpf-int' enablement:
1) We forgot to add to to kvm_default_props[] so it doesn't get enabled
 automatically (unless '-cpu host' is used or the feature is enabled
 manually on the command line)
2) We forgot to disable it for older machine types to preserve migration.
 This went unnoticed because of 1) I believe.

Vitaly Kuznetsov (2):
  i386: Add 'kvm-asyncpf-int' to kvm_default_props array
  i386: Disable 'kvm-asyncpf-int' feature for machine types <= 5.1

 hw/i386/pc.c      | 1 +
 target/i386/cpu.c | 1 +
 2 files changed, 2 insertions(+)

-- 
2.30.2

On 01/04/21 17:19, Vitaly Kuznetsov wrote:
> I noticed two issues with 'kvm-asyncpf-int' enablement:
> 1) We forgot to add to to kvm_default_props[] so it doesn't get enabled
>   automatically (unless '-cpu host' is used or the feature is enabled
>   manually on the command line)
> 2) We forgot to disable it for older machine types to preserve migration.
>   This went unnoticed because of 1) I believe.
> 
> Vitaly Kuznetsov (2):
>    i386: Add 'kvm-asyncpf-int' to kvm_default_props array
>    i386: Disable 'kvm-asyncpf-int' feature for machine types <= 5.1
> 
>   hw/i386/pc.c      | 1 +
>   target/i386/cpu.c | 1 +
>   2 files changed, 2 insertions(+)
> 

Wasn't this intentional to avoid requiring a new kernel version?

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 01/04/21 17:19, Vitaly Kuznetsov wrote:
>> I noticed two issues with 'kvm-asyncpf-int' enablement:
>> 1) We forgot to add to to kvm_default_props[] so it doesn't get enabled
>>   automatically (unless '-cpu host' is used or the feature is enabled
>>   manually on the command line)
>> 2) We forgot to disable it for older machine types to preserve migration.
>>   This went unnoticed because of 1) I believe.
>> 
>> Vitaly Kuznetsov (2):
>>    i386: Add 'kvm-asyncpf-int' to kvm_default_props array
>>    i386: Disable 'kvm-asyncpf-int' feature for machine types <= 5.1
>> 
>>   hw/i386/pc.c      | 1 +
>>   target/i386/cpu.c | 1 +
>>   2 files changed, 2 insertions(+)
>> 
>
> Wasn't this intentional to avoid requiring a new kernel version?

I think I forgot the initial plan :-( The problem is that after we
disabled the original APF (#PF based) almost nobody is using the feature
as it needs to be enabled explicitly on the command line.

Several considerations regarding the default: if your kernel doesn't
support the feature you get as much as a warning:

qemu-system-x86_64: warning: host doesn't support requested feature:
CPUID.40000001H:EAX.kvm-asyncpf-int [bit 14]

older machine types are still available (I disable it for <= 5.1 but we
can consider disabling it for 5.2 too). The feature is upstream since
Linux 5.8, I know that QEMU supports much older kernels but this doesn't
probably mean that we can't enable new KVM PV features unless all
supported kernels have it, we'd have to wait many years otherwise. 

-- 
Vitaly

On 06/04/21 13:42, Vitaly Kuznetsov wrote:
> older machine types are still available (I disable it for <= 5.1 but we
> can consider disabling it for 5.2 too). The feature is upstream since
> Linux 5.8, I know that QEMU supports much older kernels but this doesn't
> probably mean that we can't enable new KVM PV features unless all
> supported kernels have it, we'd have to wait many years otherwise.

Yes, this is a known problem in fact. :(  In 6.0 we even support RHEL 7, 
though that will go away in 6.1.

We should take the occasion of dropping RHEL7 to be clearer about which 
kernels are supported.

Paolo

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 06/04/21 13:42, Vitaly Kuznetsov wrote:
> > older machine types are still available (I disable it for <= 5.1 but we
> > can consider disabling it for 5.2 too). The feature is upstream since
> > Linux 5.8, I know that QEMU supports much older kernels but this doesn't
> > probably mean that we can't enable new KVM PV features unless all
> > supported kernels have it, we'd have to wait many years otherwise.
> 
> Yes, this is a known problem in fact. :(  In 6.0 we even support RHEL 7,
> though that will go away in 6.1.
> 
> We should take the occasion of dropping RHEL7 to be clearer about which
> kernels are supported.

It would be nice to be able to define sets of KVM functonality that we
can either start given machine types with, or provide a separate switch
to limit kvm functionality back to some defined point.  We do trip over
the same things pretty regularly when accidentally turning on new
features.

Dave

> Paolo
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On Thu, Apr 15, 2021 at 08:14:30PM +0100, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
> > On 06/04/21 13:42, Vitaly Kuznetsov wrote:
> > > older machine types are still available (I disable it for <= 5.1 but we
> > > can consider disabling it for 5.2 too). The feature is upstream since
> > > Linux 5.8, I know that QEMU supports much older kernels but this doesn't
> > > probably mean that we can't enable new KVM PV features unless all
> > > supported kernels have it, we'd have to wait many years otherwise.
> > 
> > Yes, this is a known problem in fact. :(  In 6.0 we even support RHEL 7,
> > though that will go away in 6.1.
> > 
> > We should take the occasion of dropping RHEL7 to be clearer about which
> > kernels are supported.
> 
> It would be nice to be able to define sets of KVM functonality that we
> can either start given machine types with, or provide a separate switch
> to limit kvm functionality back to some defined point.  We do trip over
> the same things pretty regularly when accidentally turning on new
> features.

The same idea can apply to the hyperv=on stuff Vitaly is working
on.  Maybe we should consider making a generic version of the
s390x FeatGroup code, use it to define convenient sets of KVM and
hyperv features.

-- 
Eduardo

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Apr 15, 2021 at 08:14:30PM +0100, Dr. David Alan Gilbert wrote:
>> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> > On 06/04/21 13:42, Vitaly Kuznetsov wrote:
>> > > older machine types are still available (I disable it for <= 5.1 but we
>> > > can consider disabling it for 5.2 too). The feature is upstream since
>> > > Linux 5.8, I know that QEMU supports much older kernels but this doesn't
>> > > probably mean that we can't enable new KVM PV features unless all
>> > > supported kernels have it, we'd have to wait many years otherwise.
>> > 
>> > Yes, this is a known problem in fact. :(  In 6.0 we even support RHEL 7,
>> > though that will go away in 6.1.
>> > 
>> > We should take the occasion of dropping RHEL7 to be clearer about which
>> > kernels are supported.
>> 
>> It would be nice to be able to define sets of KVM functonality that we
>> can either start given machine types with, or provide a separate switch
>> to limit kvm functionality back to some defined point.  We do trip over
>> the same things pretty regularly when accidentally turning on new
>> features.
>
> The same idea can apply to the hyperv=on stuff Vitaly is working
> on.  Maybe we should consider making a generic version of the
> s390x FeatGroup code, use it to define convenient sets of KVM and
> hyperv features.

True, the more I look at PV features enablement, the more I think that
we're missing something important in the logic. All machine types we
have are generally suposed to work with the oldest supported kernel so
we should wait many years before enabling some of the new PV features
(KVM or Hyper-V) by default.

This also links to our parallel discussion regarding migration
policies. Currently, we can't enable PV features by default based on
their availability on the host because of migration, the set may differ
on the destination host. What if we introduce (and maybe even switch to
it by default) something like

 -migratable opportunistic (stupid name, I know)

which would allow to enable all features supported by the source host
and then somehow checking that the destination host has them all. This
would effectively mean that it is possible to migrate a VM to a
same-or-newer software (both kernel an QEMU) but not the other way
around. This may be a reasonable choice.

-- 
Vitaly