[PATCH v2] target/s390x/kvm: Enable adapter interruption suppression again

[PATCH v2] target/s390x/kvm: Enable adapter interruption suppression again

[Thomas Huth]

The AIS feature has been disabled late in the v2.10 development cycle since
there were some issues with migration (see commit 3f2d07b3b01ea61126b -
"s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
to enable it again for newer machine types, but apparently we forgot to do
this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.

While at it, also add a more verbose comment why we need the *_allowed()
wrappers in s390-virtio-ccw.c.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 Matthew, could you please give this another try on your system? Thanks!

 hw/s390x/s390-virtio-ccw.c         | 20 +++++++++++++++++---
 include/hw/s390x/s390-virtio-ccw.h |  3 +++
 target/s390x/kvm.c                 |  9 ++++++---
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e0e28139a2..4de6c340aa 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -452,6 +452,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     s390mc->cpu_model_allowed = true;
     s390mc->css_migration_enabled = true;
     s390mc->hpage_1m_allowed = true;
+    s390mc->kvm_ais_allowed = true;
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
     mc->hot_add_cpu = s390_hot_add_cpu;
@@ -505,6 +506,14 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value,
 
 static S390CcwMachineClass *current_mc;
 
+/*
+ * Get the class of the s390-ccw-virtio machine that is currently in use.
+ * Note: libvirt is using the "none" machine to probe for the features of the
+ * host CPU, so in case this is called with the "none" machine, the function
+ * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the
+ * various "*_allowed" variables are enabled, so that the *_allowed() wrappers
+ * below return the correct default value for the "none" machine.
+ */
 static S390CcwMachineClass *get_machine_class(void)
 {
     if (unlikely(!current_mc)) {
@@ -521,22 +530,24 @@ static S390CcwMachineClass *get_machine_class(void)
 
 bool ri_allowed(void)
 {
-    /* for "none" machine this results in true */
     return get_machine_class()->ri_allowed;
 }
 
 bool cpu_model_allowed(void)
 {
-    /* for "none" machine this results in true */
     return get_machine_class()->cpu_model_allowed;
 }
 
 bool hpage_1m_allowed(void)
 {
-    /* for "none" machine this results in true */
     return get_machine_class()->hpage_1m_allowed;
 }
 
+bool kvm_ais_allowed(void)
+{
+    return get_machine_class()->kvm_ais_allowed;
+}
+
 static char *machine_get_loadparm(Object *obj, Error **errp)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
@@ -658,6 +669,9 @@ static void ccw_machine_4_2_instance_options(MachineState *machine)
 
 static void ccw_machine_4_2_class_options(MachineClass *mc)
 {
+    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
+
+    s390mc->kvm_ais_allowed = false;
     ccw_machine_5_0_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 8aa27199c9..e3ba3b88b1 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -40,6 +40,7 @@ typedef struct S390CcwMachineClass {
     bool cpu_model_allowed;
     bool css_migration_enabled;
     bool hpage_1m_allowed;
+    bool kvm_ais_allowed;
 } S390CcwMachineClass;
 
 /* runtime-instrumentation allowed by the machine */
@@ -48,6 +49,8 @@ bool ri_allowed(void);
 bool cpu_model_allowed(void);
 /* 1M huge page mappings allowed by the machine */
 bool hpage_1m_allowed(void);
+/* adapter-interrupt suppression allowed by the machine? */
+bool kvm_ais_allowed(void);
 
 /**
  * Returns true if (vmstate based) migration of the channel subsystem
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 15260aeb9a..cf4fb4f2d9 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     /*
      * The migration interface for ais was introduced with kernel 4.13
      * but the capability itself had been active since 4.12. As migration
-     * support is considered necessary let's disable ais in the 2.10
-     * machine.
+     * support is considered necessary, we only try to enable this for
+     * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
      */
-    /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
+    if (kvm_ais_allowed() &&
+        kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
+        kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
+    }
 
     kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
     return 0;
-- 
2.18.1

Re: [PATCH v2] target/s390x/kvm: Enable adapter interruption suppression again

[David Hildenbrand]

On 20.01.20 10:49, Thomas Huth wrote:
> The AIS feature has been disabled late in the v2.10 development cycle since
> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
> to enable it again for newer machine types, but apparently we forgot to do
> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
> 
> While at it, also add a more verbose comment why we need the *_allowed()
> wrappers in s390-virtio-ccw.c.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  Matthew, could you please give this another try on your system? Thanks!
> 
>  hw/s390x/s390-virtio-ccw.c         | 20 +++++++++++++++++---
>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>  target/s390x/kvm.c                 |  9 ++++++---
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e0e28139a2..4de6c340aa 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -452,6 +452,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>      s390mc->cpu_model_allowed = true;
>      s390mc->css_migration_enabled = true;
>      s390mc->hpage_1m_allowed = true;
> +    s390mc->kvm_ais_allowed = true;
>      mc->init = ccw_init;
>      mc->reset = s390_machine_reset;
>      mc->hot_add_cpu = s390_hot_add_cpu;
> @@ -505,6 +506,14 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value,
>  
>  static S390CcwMachineClass *current_mc;
>  
> +/*
> + * Get the class of the s390-ccw-virtio machine that is currently in use.
> + * Note: libvirt is using the "none" machine to probe for the features of the
> + * host CPU, so in case this is called with the "none" machine, the function
> + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the
> + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers
> + * below return the correct default value for the "none" machine.
> + */
>  static S390CcwMachineClass *get_machine_class(void)
>  {
>      if (unlikely(!current_mc)) {
> @@ -521,22 +530,24 @@ static S390CcwMachineClass *get_machine_class(void)
>  
>  bool ri_allowed(void)
>  {
> -    /* for "none" machine this results in true */
>      return get_machine_class()->ri_allowed;
>  }
>  
>  bool cpu_model_allowed(void)
>  {
> -    /* for "none" machine this results in true */
>      return get_machine_class()->cpu_model_allowed;
>  }
>  
>  bool hpage_1m_allowed(void)
>  {
> -    /* for "none" machine this results in true */
>      return get_machine_class()->hpage_1m_allowed;
>  }
>  
> +bool kvm_ais_allowed(void)
> +{
> +    return get_machine_class()->kvm_ais_allowed;
> +}
> +
>  static char *machine_get_loadparm(Object *obj, Error **errp)
>  {
>      S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> @@ -658,6 +669,9 @@ static void ccw_machine_4_2_instance_options(MachineState *machine)
>  
>  static void ccw_machine_4_2_class_options(MachineClass *mc)
>  {
> +    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
> +
> +    s390mc->kvm_ais_allowed = false;
>      ccw_machine_5_0_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>  }
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 8aa27199c9..e3ba3b88b1 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -40,6 +40,7 @@ typedef struct S390CcwMachineClass {
>      bool cpu_model_allowed;
>      bool css_migration_enabled;
>      bool hpage_1m_allowed;
> +    bool kvm_ais_allowed;
>  } S390CcwMachineClass;
>  
>  /* runtime-instrumentation allowed by the machine */
> @@ -48,6 +49,8 @@ bool ri_allowed(void);
>  bool cpu_model_allowed(void);
>  /* 1M huge page mappings allowed by the machine */
>  bool hpage_1m_allowed(void);
> +/* adapter-interrupt suppression allowed by the machine? */
> +bool kvm_ais_allowed(void);
>  
>  /**
>   * Returns true if (vmstate based) migration of the channel subsystem
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 15260aeb9a..cf4fb4f2d9 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      /*
>       * The migration interface for ais was introduced with kernel 4.13
>       * but the capability itself had been active since 4.12. As migration
> -     * support is considered necessary let's disable ais in the 2.10
> -     * machine.
> +     * support is considered necessary, we only try to enable this for
> +     * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
>       */
> -    /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
> +    if (kvm_ais_allowed() &&
> +        kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
> +        kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
> +    }
>  
>      kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
>      return 0;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2] target/s390x/kvm: Enable adapter interruption suppression again

[Cornelia Huck]

On Mon, 20 Jan 2020 10:49:01 +0100
Thomas Huth <thuth@redhat.com> wrote:

> The AIS feature has been disabled late in the v2.10 development cycle since
> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
> to enable it again for newer machine types, but apparently we forgot to do
> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
> 
> While at it, also add a more verbose comment why we need the *_allowed()
> wrappers in s390-virtio-ccw.c.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  Matthew, could you please give this another try on your system? Thanks!
> 
>  hw/s390x/s390-virtio-ccw.c         | 20 +++++++++++++++++---
>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>  target/s390x/kvm.c                 |  9 ++++++---
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 

> @@ -658,6 +669,9 @@ static void ccw_machine_4_2_instance_options(MachineState *machine)
>  
>  static void ccw_machine_4_2_class_options(MachineClass *mc)
>  {
> +    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
> +
> +    s390mc->kvm_ais_allowed = false;
>      ccw_machine_5_0_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);

One thing I've noticed: You set the _allowed value to false, and
afterwards apply the options from any 'later' class; this is the same
order as for the other _allowed values. There's also
css_migration_enabled, which is set to false _after_ the class options
from 'later' classes have been applied.

Both variants end up the same, as we only ever set the value to true in
the base class and to false just in a single class option callback; but
I wonder whether it would be cleaner to set it to false after the other
options have been applied. Or am I thinking backwards here?

>  }

Re: [PATCH v2] target/s390x/kvm: Enable adapter interruption suppression again

[Thomas Huth]

On 20/01/2020 11.30, Cornelia Huck wrote:
> On Mon, 20 Jan 2020 10:49:01 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> The AIS feature has been disabled late in the v2.10 development cycle since
>> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
>> to enable it again for newer machine types, but apparently we forgot to do
>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>>
>> While at it, also add a more verbose comment why we need the *_allowed()
>> wrappers in s390-virtio-ccw.c.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  Matthew, could you please give this another try on your system? Thanks!
>>
>>  hw/s390x/s390-virtio-ccw.c         | 20 +++++++++++++++++---
>>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>>  target/s390x/kvm.c                 |  9 ++++++---
>>  3 files changed, 26 insertions(+), 6 deletions(-)
>>
> 
>> @@ -658,6 +669,9 @@ static void ccw_machine_4_2_instance_options(MachineState *machine)
>>  
>>  static void ccw_machine_4_2_class_options(MachineClass *mc)
>>  {
>> +    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
>> +
>> +    s390mc->kvm_ais_allowed = false;
>>      ccw_machine_5_0_class_options(mc);
>>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> 
> One thing I've noticed: You set the _allowed value to false, and
> afterwards apply the options from any 'later' class; this is the same
> order as for the other _allowed values. There's also
> css_migration_enabled, which is set to false _after_ the class options
> from 'later' classes have been applied.
> 
> Both variants end up the same, as we only ever set the value to true in
> the base class and to false just in a single class option callback; but
> I wonder whether it would be cleaner to set it to false after the other
> options have been applied. Or am I thinking backwards here?

You're right, it should not matter right now, but it might be better
(from the copy-n-paste perspective) / more future-proof to do it the
other way round. I'll send a v3...

 Thomas