[Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Pierre Morel]

Currently AIS support has several problems:

- AIS support in KVM is reported if KVM has AIS capability.
- Testing if KVM FLIC attributes for AIS are supported does not take into
  account if AIS is supported by KVM.
- KVM report supporting the AIS FLIC features but denies their usage
  if the host kernel does not support the AIS feature.
- Testing if the Adapter interrupt must be suppressed is done looking at the
  ISC and ignores the adapter properties.
- Emulation of PCI devices can only be done with KVM support for AIS.
- Migration of emulated devices can only be done if both side support AIS in KVM

I would like to make some modifications to the code where I think things are
not handled at the right place.

Therefor I propose these changes.
- Use the CPU model to enable AIS in the guest, even without KVM backup
- Ask KVM for AIS support in kvm_flic realize
- add simm/nimm attributes to the KVM FLIC interface to support emulation
  and migration between hosts with and without AIS support in KVM.
- Modify the zPCI VFIO realize function to refuse VFIO PCI devices without
  AIS support in KVM.
- Modify the AIS migration to support emulation and heterogeneous hosts.


Pierre Morel (5):
  s390x/kvm: Enable AIS from CPU model always
  s390x/css: Use AIS AIRQ injection only if adapter support AIS
  s390x/intc: Emulate Adapter Interrupt Suppression
  s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported
  s390x/intc: AIS is now always migratable

 hw/intc/s390_flic.c     |  3 +-
 hw/intc/s390_flic_kvm.c | 94 ++++++++++++++++++++++++++++++++++++++-----------
 hw/s390x/css.c          |  6 ++--
 hw/s390x/s390-pci-bus.c | 12 +++++++
 target/s390x/kvm.c      |  1 +
 5 files changed, 91 insertions(+), 25 deletions(-)

-- 
2.3.0

[Qemu-devel] [PATCH v1 1/5] s390x/kvm: Enable AIS from CPU model always

[Pierre Morel]

As this patchset will introduce AIS emulation, we always enable
Adapter Interrupt Suppression, depending only from the CPU model.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 target/s390x/kvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 931b85f..06fe56d 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2807,6 +2807,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
     if (pci_available) {
         set_bit(S390_FEAT_ZPCI, model->features);
     }
+    set_bit(S390_FEAT_ADAPTER_INT_SUPPRESSION, model->features);
     set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
 
     if (s390_known_cpu_type(cpu_type)) {
-- 
2.3.0

[Qemu-devel] [PATCH v1 2/5] s390x/css: Use AIS AIRQ injection only if adapter support AIS

[Pierre Morel]

Testing to use Adapter Interrupt suppression or not depend on AIS
being enabled in the kernel.
To implement AIS emulation we must move this test inside the FLIC
dedicated irq_inject function.

Furthermore, a test to verify that the adapter is subject to the AIS
must be added.

Last, there is no need to crash QEMU if the injection failed, the
guest may recover from it.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 hw/s390x/css.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 901dc6a..6e74a5c 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -672,10 +672,12 @@ void css_adapter_interrupt(CssIoAdapterType type, uint8_t isc)
     }
 
     trace_css_adapter_interrupt(isc);
-    if (fs->ais_supported) {
+    /* Use standard IRQ injection for adapters not supporting AIS */
+    if (adapter->flags & S390_ADAPTER_SUPPRESSIBLE) {
+        /* Use AIRQ injection for adapters subject to AIS */
         if (fsc->inject_airq(fs, type, isc, adapter->flags)) {
             error_report("Failed to inject airq with AIS supported");
-            exit(1);
+            /* Report error - guest will handle not receiving interrupts */
         }
     } else {
         s390_io_interrupt(0, 0, 0, io_int_word);
-- 
2.3.0

[Qemu-devel] [PATCH v1 3/5] s390x/intc: Emulate Adapter Interrupt Suppression

[Pierre Morel]

Emulate the Adapter Interrupt Suppression in the KVM FLIC interface when
the kernel does not support AIS.

When the kernel KVM does not support AIS, we can not support VFIO PCI
devices but we still can support emulated devices if we emulate AIS
inside QEMU.
Let's emulate AIS, allowing to use emulated PCI devices without KVM AIS
support.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 hw/intc/s390_flic.c     |  3 +-
 hw/intc/s390_flic_kvm.c | 76 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 64 insertions(+), 15 deletions(-)

diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index 6eaf178..33a7cde 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -185,8 +185,7 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
                    " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
         return;
     }
-
-    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
+    fs->ais_supported = false;
 }
 
 static void s390_flic_class_init(ObjectClass *oc, void *data)
diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index 7ead17a..fd1aa22 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -33,6 +33,8 @@ typedef struct KVMS390FLICState {
 
     uint32_t fd;
     bool clear_io_supported;
+    uint8_t simm;
+    uint8_t nimm;
 } KVMS390FLICState;
 
 DeviceState *s390_flic_kvm_create(void)
@@ -164,11 +166,24 @@ static int kvm_s390_modify_ais_mode(S390FLICState *fs, uint8_t isc,
         .addr = (uint64_t)&req,
     };
 
-    if (!fs->ais_supported) {
-        return -ENOSYS;
+    if (fs->ais_supported) {
+        return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
     }
 
-    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
+    /* Kernel does not support AIS, emulate it here */
+    switch (mode) {
+    case SIC_IRQ_MODE_ALL:
+        flic->simm &= ~AIS_MODE_MASK(isc);
+        flic->nimm &= ~AIS_MODE_MASK(isc);
+        break;
+    case SIC_IRQ_MODE_SINGLE:
+        flic->simm |= AIS_MODE_MASK(isc);
+        flic->nimm &= ~AIS_MODE_MASK(isc);
+        break;
+    default:
+        return -EINVAL;
+    }
+    return 0;
 }
 
 static int kvm_s390_inject_airq(S390FLICState *fs, uint8_t type,
@@ -180,12 +195,23 @@ static int kvm_s390_inject_airq(S390FLICState *fs, uint8_t type,
         .group = KVM_DEV_FLIC_AIRQ_INJECT,
         .attr = id,
     };
+    uint32_t io_int_word = (isc << 27) | IO_INT_WORD_AI;
 
-    if (!fs->ais_supported) {
-        return -ENOSYS;
+    if (fs->ais_supported) {
+        return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
     }
 
-    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
+    /* Kernel does not support AIS, emulate it here */
+    if (flags && (flic->nimm & AIS_MODE_MASK(isc))) {
+        return 0;
+    }
+
+    s390_io_interrupt(0, 0, 0, io_int_word);
+
+    if (flags && (flic->simm & AIS_MODE_MASK(isc))) {
+        flic->nimm |= AIS_MODE_MASK(isc);
+    }
+    return 0;
 }
 
 /**
@@ -557,6 +583,32 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
     flic_state->clear_io_supported = !ioctl(flic_state->fd,
                                             KVM_HAS_DEVICE_ATTR, test_attr);
+    /* To support ais we need all these three FLIC attributes */
+    test_attr.group = KVM_DEV_FLIC_AISM;
+    ret = !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
+    flic_state->parent_obj.ais_supported &= ret;
+
+    test_attr.group = KVM_DEV_FLIC_AIRQ_INJECT;
+    ret = !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
+    flic_state->parent_obj.ais_supported &= ret;
+
+    test_attr.group = KVM_DEV_FLIC_AISM_ALL;
+    ret &= !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
+    flic_state->parent_obj.ais_supported &= ret;
+
+    /* For buggy kernels we need to really test that the attribute is supported */
+    {
+        struct kvm_s390_ais_req req = {
+            .mode = SIC_IRQ_MODE_ALL,
+        };
+        struct kvm_device_attr attr = {
+            .group = KVM_DEV_FLIC_AISM,
+            .addr = (uint64_t)&req,
+        };
+        ret = !ioctl(flic_state->fd, KVM_SET_DEVICE_ATTR, &attr);
+    }
+    flic_state->parent_obj.ais_supported &= ret;
+
     return;
 fail:
     error_propagate(errp, errp_local);
@@ -578,13 +630,11 @@ static void kvm_s390_flic_reset(DeviceState *dev)
 
     flic_disable_wait_pfault(flic);
 
-    if (fs->ais_supported) {
-        for (isc = 0; isc <= MAX_ISC; isc++) {
-            rc = kvm_s390_modify_ais_mode(fs, isc, SIC_IRQ_MODE_ALL);
-            if (rc) {
-                error_report("Failed to reset ais mode for isc %d: %s",
-                             isc, strerror(-rc));
-            }
+    for (isc = 0; isc <= MAX_ISC; isc++) {
+        rc = kvm_s390_modify_ais_mode(fs, isc, SIC_IRQ_MODE_ALL);
+        if (rc) {
+            error_report("Failed to reset ais mode for isc %d: %s",
+                         isc, strerror(-rc));
         }
     }
 
-- 
2.3.0

[Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Pierre Morel]

In S390x the Adapter Interrupt Suppression facility is used to mask
interrupts of other PCI devices during interruption handling.

VFIO PCI allows the interrupts to be delivered rapidely through KVM via
IRQfd or to be delivered through QEMU.
The choice is made through the x-kvm-intx and x-kvo-misx properties of
the VFIO PCI device.

If the VFIO PCI device is using the direct KVM access through IRQfd and
we know that KVM does not implement AIS support we refuse to realize the
VFIO PCI device.

In all other cases, emulation and VFIO PCI sending interrupts through
QEMU, we intercept the propagated IRQ, and protect it with the QEMU AIS
implementation before to send it to the guest through KVM.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-bus.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index d9c294a..4afe49b 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -21,18 +21,21 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/msi.h"
+#include "hw/vfio/pci.h"
 #include "qemu/error-report.h"
 
 #ifndef DEBUG_S390PCI_BUS
 #define DEBUG_S390PCI_BUS  0
 #endif
 
+#ifndef DPRINTF
 #define DPRINTF(fmt, ...)                                         \
     do {                                                          \
         if (DEBUG_S390PCI_BUS) {                                  \
             fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \
         }                                                         \
     } while (0)
+#endif
 
 S390pciState *s390_get_phb(void)
 {
@@ -751,6 +754,15 @@ static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
         }
 
         if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
+            S390FLICState *fs = s390_get_flic();
+            VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+
+            if ((!vdev->no_kvm_msix || !vdev->no_kvm_msix) &&
+                (!fs || !fs->ais_supported)) {
+                error_setg(errp, "VFIO PCI is not supported "
+                           "because kernel has no AIS capability.");
+                return;
+            }
             pbdev->fh |= FH_SHM_VFIO;
         } else {
             pbdev->fh |= FH_SHM_EMUL;
-- 
2.3.0

[Qemu-devel] [PATCH v1 5/5] s390x/intc: AIS is now always migratable

[Pierre Morel]

Adapter Interrupt Suppression can now be emulated if the host kernel
does not support the FLIC AIS attributes in KVM.

It follows it can always be migrated.

In the case AIS is supported by the kernel at one side of the migration

1) SRC and DST AIS in KVM
All PCI devices can be created on SRC and DST
All PCI devices can migrate

2) SRC and DST AIS NOT in  KVM
Only emulated PCI devices can be used on SRC and DST
Only emulated PCI devices can migrate

3) SRC AIS in KVM DST has NOT AIS in KVM
Only emulated PCI devices can migrate
Only emulated PCI devices can be created on SRC and DST
VFIO PCI devices can be used only on SRC

4) SRC has NOT AIS in KVM DST AIS in KVM
Only emulated PCI devices can migrate
Only emulated PCI devices can be created on SRC and DST
VFIO PCI devices can be used only on DST

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 hw/intc/s390_flic_kvm.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index fd1aa22..6f44b1d 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -457,6 +457,12 @@ static void kvm_flic_ais_pre_save(void *opaque)
         .attr = sizeof(ais),
     };
 
+    if (!flic->parent_obj.ais_supported) {
+        tmp->simm = flic->simm;
+        tmp->nimm = flic->nimm;
+        return;
+    }
+
     if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
         error_report("Failed to retrieve kvm flic ais states");
         return;
@@ -479,14 +485,10 @@ static int kvm_flic_ais_post_load(void *opaque, int version_id)
         .addr = (uint64_t)&ais,
     };
 
-    /* This can happen when the user mis-configures its guests in an
-     * incompatible fashion or without a CPU model. For example using
-     * qemu with -cpu host (which is not migration safe) and do a
-     * migration from a host that has AIS to a host that has no AIS.
-     * In that case the target system will reject the migration here.
-     */
-    if (!ais_needed(flic)) {
-        return -ENOSYS;
+    if (!flic->parent_obj.ais_supported) {
+        flic->simm = tmp->simm;
+        flic->nimm = tmp->nimm;
+        return 0;
     }
 
     return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
-- 
2.3.0

Re: [Qemu-devel] [PATCH v1 2/5] s390x/css: Use AIS AIRQ injection only if adapter support AIS

[Cornelia Huck]

On Wed,  4 Oct 2017 15:49:36 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Testing to use Adapter Interrupt suppression or not depend on AIS
> being enabled in the kernel.
> To implement AIS emulation we must move this test inside the FLIC
> dedicated irq_inject function.
> 
> Furthermore, a test to verify that the adapter is subject to the AIS
> must be added.
> 
> Last, there is no need to crash QEMU if the injection failed, the
> guest may recover from it.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 901dc6a..6e74a5c 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -672,10 +672,12 @@ void css_adapter_interrupt(CssIoAdapterType type, uint8_t isc)
>      }
>  
>      trace_css_adapter_interrupt(isc);
> -    if (fs->ais_supported) {
> +    /* Use standard IRQ injection for adapters not supporting AIS */

I'd move that comment to the else branch.

> +    if (adapter->flags & S390_ADAPTER_SUPPRESSIBLE) {
> +        /* Use AIRQ injection for adapters subject to AIS */
>          if (fsc->inject_airq(fs, type, isc, adapter->flags)) {
>              error_report("Failed to inject airq with AIS supported");
> -            exit(1);
> +            /* Report error - guest will handle not receiving interrupts */

I'm not 100% sure that this is the right thing to do. I have always
operated under the assumption "if the hardware (host) accepts the I/O,
it owes you an interrupt". (Don't the other failed interrupt injection
paths do a hard stop as well? Is there any reasonable way for an
injection failure to be transient?)

>          }
>      } else {
>          s390_io_interrupt(0, 0, 0, io_int_word);

Re: [Qemu-devel] [PATCH v1 3/5] s390x/intc: Emulate Adapter Interrupt Suppression

[Cornelia Huck]

On Wed,  4 Oct 2017 15:49:37 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Emulate the Adapter Interrupt Suppression in the KVM FLIC interface when
> the kernel does not support AIS.
> 
> When the kernel KVM does not support AIS, we can not support VFIO PCI
> devices but we still can support emulated devices if we emulate AIS
> inside QEMU.
> Let's emulate AIS, allowing to use emulated PCI devices without KVM AIS
> support.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  hw/intc/s390_flic.c     |  3 +-
>  hw/intc/s390_flic_kvm.c | 76 ++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 64 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index 6eaf178..33a7cde 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -185,8 +185,7 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>          return;
>      }
> -
> -    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
> +    fs->ais_supported = false;
>  }
>  
>  static void s390_flic_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index 7ead17a..fd1aa22 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -33,6 +33,8 @@ typedef struct KVMS390FLICState {
>  
>      uint32_t fd;
>      bool clear_io_supported;
> +    uint8_t simm;
> +    uint8_t nimm;
>  } KVMS390FLICState;

Instead of duplicating this, move simm/nimm into the common flic state?

>  
>  DeviceState *s390_flic_kvm_create(void)
> @@ -164,11 +166,24 @@ static int kvm_s390_modify_ais_mode(S390FLICState *fs, uint8_t isc,
>          .addr = (uint64_t)&req,
>      };
>  
> -    if (!fs->ais_supported) {
> -        return -ENOSYS;
> +    if (fs->ais_supported) {
> +        return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
>      }
>  
> -    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
> +    /* Kernel does not support AIS, emulate it here */
> +    switch (mode) {
> +    case SIC_IRQ_MODE_ALL:
> +        flic->simm &= ~AIS_MODE_MASK(isc);
> +        flic->nimm &= ~AIS_MODE_MASK(isc);
> +        break;
> +    case SIC_IRQ_MODE_SINGLE:
> +        flic->simm |= AIS_MODE_MASK(isc);
> +        flic->nimm &= ~AIS_MODE_MASK(isc);
> +        break;
> +    default:
> +        return -EINVAL;
> +    }

That's just the same as for the qemu flic. What about creating a
wrapper (to be called from css.c) in the common flic code that calls a
specific callback (only needed for kvm) and falls back to emulating if
it was not successful?

> +    return 0;
>  }
>  
>  static int kvm_s390_inject_airq(S390FLICState *fs, uint8_t type,
> @@ -180,12 +195,23 @@ static int kvm_s390_inject_airq(S390FLICState *fs, uint8_t type,
>          .group = KVM_DEV_FLIC_AIRQ_INJECT,
>          .attr = id,
>      };
> +    uint32_t io_int_word = (isc << 27) | IO_INT_WORD_AI;
>  
> -    if (!fs->ais_supported) {
> -        return -ENOSYS;
> +    if (fs->ais_supported) {
> +        return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
>      }
>  
> -    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
> +    /* Kernel does not support AIS, emulate it here */
> +    if (flags && (flic->nimm & AIS_MODE_MASK(isc))) {
> +        return 0;
> +    }
> +
> +    s390_io_interrupt(0, 0, 0, io_int_word);
> +
> +    if (flags && (flic->simm & AIS_MODE_MASK(isc))) {
> +        flic->nimm |= AIS_MODE_MASK(isc);
> +    }

Same here: I think doing this in the generic flic would make sense.

> +    return 0;
>  }
>  
>  /**
> @@ -557,6 +583,32 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>      test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
>      flic_state->clear_io_supported = !ioctl(flic_state->fd,
>                                              KVM_HAS_DEVICE_ATTR, test_attr);
> +    /* To support ais we need all these three FLIC attributes */
> +    test_attr.group = KVM_DEV_FLIC_AISM;
> +    ret = !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
> +    flic_state->parent_obj.ais_supported &= ret;
> +
> +    test_attr.group = KVM_DEV_FLIC_AIRQ_INJECT;
> +    ret = !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
> +    flic_state->parent_obj.ais_supported &= ret;
> +
> +    test_attr.group = KVM_DEV_FLIC_AISM_ALL;
> +    ret &= !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
> +    flic_state->parent_obj.ais_supported &= ret;
> +
> +    /* For buggy kernels we need to really test that the attribute is supported */

Ugh.

> +    {
> +        struct kvm_s390_ais_req req = {
> +            .mode = SIC_IRQ_MODE_ALL,
> +        };
> +        struct kvm_device_attr attr = {
> +            .group = KVM_DEV_FLIC_AISM,
> +            .addr = (uint64_t)&req,
> +        };
> +        ret = !ioctl(flic_state->fd, KVM_SET_DEVICE_ATTR, &attr);
> +    }
> +    flic_state->parent_obj.ais_supported &= ret;
> +
>      return;
>  fail:
>      error_propagate(errp, errp_local);
> @@ -578,13 +630,11 @@ static void kvm_s390_flic_reset(DeviceState *dev)
>  
>      flic_disable_wait_pfault(flic);
>  
> -    if (fs->ais_supported) {
> -        for (isc = 0; isc <= MAX_ISC; isc++) {
> -            rc = kvm_s390_modify_ais_mode(fs, isc, SIC_IRQ_MODE_ALL);
> -            if (rc) {
> -                error_report("Failed to reset ais mode for isc %d: %s",
> -                             isc, strerror(-rc));
> -            }
> +    for (isc = 0; isc <= MAX_ISC; isc++) {
> +        rc = kvm_s390_modify_ais_mode(fs, isc, SIC_IRQ_MODE_ALL);
> +        if (rc) {
> +            error_report("Failed to reset ais mode for isc %d: %s",
> +                         isc, strerror(-rc));
>          }
>      }
>  

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Cornelia Huck]

On Wed,  4 Oct 2017 15:49:38 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

"not supported", surely?

> In S390x the Adapter Interrupt Suppression facility is used to mask
> interrupts of other PCI devices during interruption handling.
> 
> VFIO PCI allows the interrupts to be delivered rapidely through KVM via
> IRQfd or to be delivered through QEMU.
> The choice is made through the x-kvm-intx and x-kvo-misx properties of
> the VFIO PCI device.
> 
> If the VFIO PCI device is using the direct KVM access through IRQfd and
> we know that KVM does not implement AIS support we refuse to realize the
> VFIO PCI device.
> 
> In all other cases, emulation and VFIO PCI sending interrupts through
> QEMU, we intercept the propagated IRQ, and protect it with the QEMU AIS
> implementation before to send it to the guest through KVM.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index d9c294a..4afe49b 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -21,18 +21,21 @@
>  #include "hw/pci/pci_bus.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/msi.h"
> +#include "hw/vfio/pci.h"
>  #include "qemu/error-report.h"
>  
>  #ifndef DEBUG_S390PCI_BUS
>  #define DEBUG_S390PCI_BUS  0
>  #endif
>  
> +#ifndef DPRINTF
>  #define DPRINTF(fmt, ...)                                         \
>      do {                                                          \
>          if (DEBUG_S390PCI_BUS) {                                  \
>              fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \
>          }                                                         \
>      } while (0)
> +#endif

Ugh. Is there DPRINTF redefinition going on?

>  
>  S390pciState *s390_get_phb(void)
>  {
> @@ -751,6 +754,15 @@ static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
>          }
>  
>          if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> +            S390FLICState *fs = s390_get_flic();
> +            VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +
> +            if ((!vdev->no_kvm_msix || !vdev->no_kvm_msix) &&

One of the no_kvm_msix should probably be something else :)

> +                (!fs || !fs->ais_supported)) {
> +                error_setg(errp, "VFIO PCI is not supported "
> +                           "because kernel has no AIS capability.");

I think failure is per-device (i.e., you would be able to plug a
different vfio device if you turned off irqfd support for it); should
that be reflected in the error message?

> +                return;
> +            }

Would it make sense to fail plugging before we try to create a zpci
device?

>              pbdev->fh |= FH_SHM_VFIO;
>          } else {
>              pbdev->fh |= FH_SHM_EMUL;

Re: [Qemu-devel] [PATCH v1 3/5] s390x/intc: Emulate Adapter Interrupt Suppression

[Cornelia Huck]

On Mon, 9 Oct 2017 10:42:44 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed,  4 Oct 2017 15:49:37 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
> > Emulate the Adapter Interrupt Suppression in the KVM FLIC interface when
> > the kernel does not support AIS.
> > 
> > When the kernel KVM does not support AIS, we can not support VFIO PCI
> > devices but we still can support emulated devices if we emulate AIS
> > inside QEMU.
> > Let's emulate AIS, allowing to use emulated PCI devices without KVM AIS
> > support.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> > ---
> >  hw/intc/s390_flic.c     |  3 +-
> >  hw/intc/s390_flic_kvm.c | 76 ++++++++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 64 insertions(+), 15 deletions(-)
> > 
> > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> > index 6eaf178..33a7cde 100644
> > --- a/hw/intc/s390_flic.c
> > +++ b/hw/intc/s390_flic.c
> > @@ -185,8 +185,7 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
> >                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> >          return;
> >      }
> > -
> > -    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
> > +    fs->ais_supported = false;
> >  }
> >  
> >  static void s390_flic_class_init(ObjectClass *oc, void *data)
> > diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> > index 7ead17a..fd1aa22 100644
> > --- a/hw/intc/s390_flic_kvm.c
> > +++ b/hw/intc/s390_flic_kvm.c
> > @@ -33,6 +33,8 @@ typedef struct KVMS390FLICState {
> >  
> >      uint32_t fd;
> >      bool clear_io_supported;
> > +    uint8_t simm;
> > +    uint8_t nimm;
> >  } KVMS390FLICState;  
> 
> Instead of duplicating this, move simm/nimm into the common flic state?

Also, simm/nimm need to be reset (done for the qemu flic, but not for
the kvm flic). Doing that in common flic code would take care of that
as well.

Re: [Qemu-devel] [PATCH v1 1/5] s390x/kvm: Enable AIS from CPU model always

[Cornelia Huck]

On Wed,  4 Oct 2017 15:49:35 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> As this patchset will introduce AIS emulation, we always enable
> Adapter Interrupt Suppression, depending only from the CPU model.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  target/s390x/kvm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 931b85f..06fe56d 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2807,6 +2807,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>      if (pci_available) {
>          set_bit(S390_FEAT_ZPCI, model->features);
>      }
> +    set_bit(S390_FEAT_ADAPTER_INT_SUPPRESSION, model->features);
>      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
>  
>      if (s390_known_cpu_type(cpu_type)) {

I would move that patch until after emulation has actually been
provided.

Re: [Qemu-devel] [PATCH v1 2/5] s390x/css: Use AIS AIRQ injection only if adapter support AIS

[Pierre Morel]

On 09/10/2017 10:17, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 15:49:36 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Testing to use Adapter Interrupt suppression or not depend on AIS
>> being enabled in the kernel.
>> To implement AIS emulation we must move this test inside the FLIC
>> dedicated irq_inject function.
>>
>> Furthermore, a test to verify that the adapter is subject to the AIS
>> must be added.
>>
>> Last, there is no need to crash QEMU if the injection failed, the
>> guest may recover from it.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/css.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 901dc6a..6e74a5c 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -672,10 +672,12 @@ void css_adapter_interrupt(CssIoAdapterType type, uint8_t isc)
>>       }
>>   
>>       trace_css_adapter_interrupt(isc);
>> -    if (fs->ais_supported) {
>> +    /* Use standard IRQ injection for adapters not supporting AIS */
> 
> I'd move that comment to the else branch.

yes, clear.

> 
>> +    if (adapter->flags & S390_ADAPTER_SUPPRESSIBLE) {
>> +        /* Use AIRQ injection for adapters subject to AIS */
>>           if (fsc->inject_airq(fs, type, isc, adapter->flags)) {
>>               error_report("Failed to inject airq with AIS supported");
>> -            exit(1);
>> +            /* Report error - guest will handle not receiving interrupts */
> 
> I'm not 100% sure that this is the right thing to do. I have always
> operated under the assumption "if the hardware (host) accepts the I/O,
> it owes you an interrupt". (Don't the other failed interrupt injection
> paths do a hard stop as well? Is there any reasonable way for an
> injection failure to be transient?)

You are right and it is out of scope.
So I will let this untouch.

> 
>>           }
>>       } else {
>>           s390_io_interrupt(0, 0, 0, io_int_word);
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 1/5] s390x/kvm: Enable AIS from CPU model always

[Pierre Morel]

On 09/10/2017 11:09, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 15:49:35 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> As this patchset will introduce AIS emulation, we always enable
>> Adapter Interrupt Suppression, depending only from the CPU model.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   target/s390x/kvm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 931b85f..06fe56d 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2807,6 +2807,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>       if (pci_available) {
>>           set_bit(S390_FEAT_ZPCI, model->features);
>>       }
>> +    set_bit(S390_FEAT_ADAPTER_INT_SUPPRESSION, model->features);
>>       set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
>>   
>>       if (s390_known_cpu_type(cpu_type)) {
> 
> I would move that patch until after emulation has actually been
> provided.
> 

Yes, clear.

Thanks,
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 3/5] s390x/intc: Emulate Adapter Interrupt Suppression

[Pierre Morel]

On 09/10/2017 10:42, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 15:49:37 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Emulate the Adapter Interrupt Suppression in the KVM FLIC interface when
>> the kernel does not support AIS.
>>
>> When the kernel KVM does not support AIS, we can not support VFIO PCI
>> devices but we still can support emulated devices if we emulate AIS
>> inside QEMU.
>> Let's emulate AIS, allowing to use emulated PCI devices without KVM AIS
>> support.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   hw/intc/s390_flic.c     |  3 +-
>>   hw/intc/s390_flic_kvm.c | 76 ++++++++++++++++++++++++++++++++++++++++---------
>>   2 files changed, 64 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>> index 6eaf178..33a7cde 100644
>> --- a/hw/intc/s390_flic.c
>> +++ b/hw/intc/s390_flic.c
>> @@ -185,8 +185,7 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>                      " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>>           return;
>>       }
>> -
>> -    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
>> +    fs->ais_supported = false;
>>   }
>>   
>>   static void s390_flic_class_init(ObjectClass *oc, void *data)
>> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
>> index 7ead17a..fd1aa22 100644
>> --- a/hw/intc/s390_flic_kvm.c
>> +++ b/hw/intc/s390_flic_kvm.c
>> @@ -33,6 +33,8 @@ typedef struct KVMS390FLICState {
>>   
>>       uint32_t fd;
>>       bool clear_io_supported;
>> +    uint8_t simm;
>> +    uint8_t nimm;
>>   } KVMS390FLICState;
> 
> Instead of duplicating this, move simm/nimm into the common flic state?

OK.

> 
>>   
>>   DeviceState *s390_flic_kvm_create(void)
>> @@ -164,11 +166,24 @@ static int kvm_s390_modify_ais_mode(S390FLICState *fs, uint8_t isc,
>>           .addr = (uint64_t)&req,
>>       };
>>   
>> -    if (!fs->ais_supported) {
>> -        return -ENOSYS;
>> +    if (fs->ais_supported) {
>> +        return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
>>       }
>>   
>> -    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
>> +    /* Kernel does not support AIS, emulate it here */
>> +    switch (mode) {
>> +    case SIC_IRQ_MODE_ALL:
>> +        flic->simm &= ~AIS_MODE_MASK(isc);
>> +        flic->nimm &= ~AIS_MODE_MASK(isc);
>> +        break;
>> +    case SIC_IRQ_MODE_SINGLE:
>> +        flic->simm |= AIS_MODE_MASK(isc);
>> +        flic->nimm &= ~AIS_MODE_MASK(isc);
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
> 
> That's just the same as for the qemu flic. What about creating a
> wrapper (to be called from css.c) in the common flic code that calls a
> specific callback (only needed for kvm) and falls back to emulating if
> it was not successful?

Yes, OK.

> 
>> +    return 0;
>>   }
>>   
>>   static int kvm_s390_inject_airq(S390FLICState *fs, uint8_t type,
>> @@ -180,12 +195,23 @@ static int kvm_s390_inject_airq(S390FLICState *fs, uint8_t type,
>>           .group = KVM_DEV_FLIC_AIRQ_INJECT,
>>           .attr = id,
>>       };
>> +    uint32_t io_int_word = (isc << 27) | IO_INT_WORD_AI;
>>   
>> -    if (!fs->ais_supported) {
>> -        return -ENOSYS;
>> +    if (fs->ais_supported) {
>> +        return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
>>       }
>>   
>> -    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
>> +    /* Kernel does not support AIS, emulate it here */
>> +    if (flags && (flic->nimm & AIS_MODE_MASK(isc))) {
>> +        return 0;
>> +    }
>> +
>> +    s390_io_interrupt(0, 0, 0, io_int_word);
>> +
>> +    if (flags && (flic->simm & AIS_MODE_MASK(isc))) {
>> +        flic->nimm |= AIS_MODE_MASK(isc);
>> +    }
> 
> Same here: I think doing this in the generic flic would make sense.

OK

> 
>> +    return 0;
>>   }
>>   
>>   /**
>> @@ -557,6 +583,32 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>>       test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
>>       flic_state->clear_io_supported = !ioctl(flic_state->fd,
>>                                               KVM_HAS_DEVICE_ATTR, test_attr);
>> +    /* To support ais we need all these three FLIC attributes */
>> +    test_attr.group = KVM_DEV_FLIC_AISM;
>> +    ret = !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
>> +    flic_state->parent_obj.ais_supported &= ret;
>> +
>> +    test_attr.group = KVM_DEV_FLIC_AIRQ_INJECT;
>> +    ret = !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
>> +    flic_state->parent_obj.ais_supported &= ret;
>> +
>> +    test_attr.group = KVM_DEV_FLIC_AISM_ALL;
>> +    ret &= !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr);
>> +    flic_state->parent_obj.ais_supported &= ret;
>> +
>> +    /* For buggy kernels we need to really test that the attribute is supported */
> 
> Ugh.
> 
>> +    {
>> +        struct kvm_s390_ais_req req = {
>> +            .mode = SIC_IRQ_MODE_ALL,
>> +        };
>> +        struct kvm_device_attr attr = {
>> +            .group = KVM_DEV_FLIC_AISM,
>> +            .addr = (uint64_t)&req,
>> +        };
>> +        ret = !ioctl(flic_state->fd, KVM_SET_DEVICE_ATTR, &attr);
>> +    }
>> +    flic_state->parent_obj.ais_supported &= ret;
>> +
>>       return;
>>   fail:
>>       error_propagate(errp, errp_local);
>> @@ -578,13 +630,11 @@ static void kvm_s390_flic_reset(DeviceState *dev)
>>   
>>       flic_disable_wait_pfault(flic);
>>   
>> -    if (fs->ais_supported) {
>> -        for (isc = 0; isc <= MAX_ISC; isc++) {
>> -            rc = kvm_s390_modify_ais_mode(fs, isc, SIC_IRQ_MODE_ALL);
>> -            if (rc) {
>> -                error_report("Failed to reset ais mode for isc %d: %s",
>> -                             isc, strerror(-rc));
>> -            }
>> +    for (isc = 0; isc <= MAX_ISC; isc++) {
>> +        rc = kvm_s390_modify_ais_mode(fs, isc, SIC_IRQ_MODE_ALL);
>> +        if (rc) {
>> +            error_report("Failed to reset ais mode for isc %d: %s",
>> +                         isc, strerror(-rc));
>>           }
>>       }
>>   
> 
> 

OK, I promote the simm/nimm to the common flic state .

Thanks for review.
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 3/5] s390x/intc: Emulate Adapter Interrupt Suppression

[Pierre Morel]

On 09/10/2017 11:08, Cornelia Huck wrote:
> On Mon, 9 Oct 2017 10:42:44 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Wed,  4 Oct 2017 15:49:37 +0200
>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>
>>> Emulate the Adapter Interrupt Suppression in the KVM FLIC interface when
>>> the kernel does not support AIS.
>>>
>>> When the kernel KVM does not support AIS, we can not support VFIO PCI
>>> devices but we still can support emulated devices if we emulate AIS
>>> inside QEMU.
>>> Let's emulate AIS, allowing to use emulated PCI devices without KVM AIS
>>> support.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>> ---
>>>   hw/intc/s390_flic.c     |  3 +-
>>>   hw/intc/s390_flic_kvm.c | 76 ++++++++++++++++++++++++++++++++++++++++---------
>>>   2 files changed, 64 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>>> index 6eaf178..33a7cde 100644
>>> --- a/hw/intc/s390_flic.c
>>> +++ b/hw/intc/s390_flic.c
>>> @@ -185,8 +185,7 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>>                      " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>>>           return;
>>>       }
>>> -
>>> -    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
>>> +    fs->ais_supported = false;
>>>   }
>>>   
>>>   static void s390_flic_class_init(ObjectClass *oc, void *data)
>>> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
>>> index 7ead17a..fd1aa22 100644
>>> --- a/hw/intc/s390_flic_kvm.c
>>> +++ b/hw/intc/s390_flic_kvm.c
>>> @@ -33,6 +33,8 @@ typedef struct KVMS390FLICState {
>>>   
>>>       uint32_t fd;
>>>       bool clear_io_supported;
>>> +    uint8_t simm;
>>> +    uint8_t nimm;
>>>   } KVMS390FLICState;
>>
>> Instead of duplicating this, move simm/nimm into the common flic state?
> 
> Also, simm/nimm need to be reset (done for the qemu flic, but not for
> the kvm flic). Doing that in common flic code would take care of that
> as well.
> 

right.
Thanks,

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Pierre Morel]

On 09/10/2017 11:06, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 15:49:38 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
> "not supported", surely?
:)
yes, "not supported"

> 
>> In S390x the Adapter Interrupt Suppression facility is used to mask
>> interrupts of other PCI devices during interruption handling.
>>
>> VFIO PCI allows the interrupts to be delivered rapidely through KVM via
>> IRQfd or to be delivered through QEMU.
>> The choice is made through the x-kvm-intx and x-kvo-misx properties of
>> the VFIO PCI device.
>>
>> If the VFIO PCI device is using the direct KVM access through IRQfd and
>> we know that KVM does not implement AIS support we refuse to realize the
>> VFIO PCI device.
>>
>> In all other cases, emulation and VFIO PCI sending interrupts through
>> QEMU, we intercept the propagated IRQ, and protect it with the QEMU AIS
>> implementation before to send it to the guest through KVM.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index d9c294a..4afe49b 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -21,18 +21,21 @@
>>   #include "hw/pci/pci_bus.h"
>>   #include "hw/pci/pci_bridge.h"
>>   #include "hw/pci/msi.h"
>> +#include "hw/vfio/pci.h"
>>   #include "qemu/error-report.h"
>>   
>>   #ifndef DEBUG_S390PCI_BUS
>>   #define DEBUG_S390PCI_BUS  0
>>   #endif
>>   
>> +#ifndef DPRINTF
>>   #define DPRINTF(fmt, ...)                                         \
>>       do {                                                          \
>>           if (DEBUG_S390PCI_BUS) {                                  \
>>               fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \
>>           }                                                         \
>>       } while (0)
>> +#endif
> 
> Ugh. Is there DPRINTF redefinition going on?

Yes in hw/vfio/vfio-common.h

But I think I better do an undef / def instead of a ifndef

#ifdef DPRINTF
#undef DPRINTF
#endif
#define DPRINTF(fmt, ...)
...



> 
>>   
>>   S390pciState *s390_get_phb(void)
>>   {
>> @@ -751,6 +754,15 @@ static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
>>           }
>>   
>>           if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>> +            S390FLICState *fs = s390_get_flic();
>> +            VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> +
>> +            if ((!vdev->no_kvm_msix || !vdev->no_kvm_msix) &&
> 
> One of the no_kvm_msix should probably be something else :)

:) yes

Historicaly, it used to be a no_kvm_intx but since AIS only works with 
AEN intx has no reason to be here.

So the second no_kvm_msix must disappear.

> 
>> +                (!fs || !fs->ais_supported)) {
>> +                error_setg(errp, "VFIO PCI is not supported "
>> +                           "because kernel has no AIS capability.");
> 
> I think failure is per-device (i.e., you would be able to plug a
> different vfio device if you turned off irqfd support for it); should
> that be reflected in the error message?

OK, I will find something better.

> 
>> +                return;
>> +            }
> 
> Would it make sense to fail plugging before we try to create a zpci
> device?

clearly yes. I will put these test before the eventual zpci allocation.

> 
>>               pbdev->fh |= FH_SHM_VFIO;
>>           } else {
>>               pbdev->fh |= FH_SHM_EMUL;
> 
> 

Thanks,

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Alex Williamson]

On Wed,  4 Oct 2017 15:49:38 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> In S390x the Adapter Interrupt Suppression facility is used to mask
> interrupts of other PCI devices during interruption handling.
> 
> VFIO PCI allows the interrupts to be delivered rapidely through KVM via
> IRQfd or to be delivered through QEMU.
> The choice is made through the x-kvm-intx and x-kvo-misx properties of
> the VFIO PCI device.
> 
> If the VFIO PCI device is using the direct KVM access through IRQfd and
> we know that KVM does not implement AIS support we refuse to realize the
> VFIO PCI device.
> 
> In all other cases, emulation and VFIO PCI sending interrupts through
> QEMU, we intercept the propagated IRQ, and protect it with the QEMU AIS
> implementation before to send it to the guest through KVM.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index d9c294a..4afe49b 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -21,18 +21,21 @@
>  #include "hw/pci/pci_bus.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/msi.h"
> +#include "hw/vfio/pci.h"
>  #include "qemu/error-report.h"
>  
>  #ifndef DEBUG_S390PCI_BUS
>  #define DEBUG_S390PCI_BUS  0
>  #endif
>  
> +#ifndef DPRINTF
>  #define DPRINTF(fmt, ...)                                         \
>      do {                                                          \
>          if (DEBUG_S390PCI_BUS) {                                  \
>              fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \
>          }                                                         \
>      } while (0)
> +#endif

Maybe a sign we shouldn't be including vfio/pci.h

>  S390pciState *s390_get_phb(void)
>  {
> @@ -751,6 +754,15 @@ static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
>          }
>  
>          if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> +            S390FLICState *fs = s390_get_flic();
> +            VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +
> +            if ((!vdev->no_kvm_msix || !vdev->no_kvm_msix) &&
> +                (!fs || !fs->ais_supported)) {
> +                error_setg(errp, "VFIO PCI is not supported "
> +                           "because kernel has no AIS capability.");
> +                return;
> +            }


Hmm, you're basically looking at private data structure fields for
experimental disable flags, which no user, or more importantly no
management tool, could rightfully be expected to provide.  Can we not
take this into account to automatically do the right thing, ie.
automatically set the necessary flag during the device realize?  Thanks,

Alex

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Pierre Morel]

On 09/10/2017 16:45, Alex Williamson wrote:
> On Wed,  4 Oct 2017 15:49:38 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> In S390x the Adapter Interrupt Suppression facility is used to mask
>> interrupts of other PCI devices during interruption handling.
>>
>> VFIO PCI allows the interrupts to be delivered rapidely through KVM via
>> IRQfd or to be delivered through QEMU.
>> The choice is made through the x-kvm-intx and x-kvo-misx properties of
>> the VFIO PCI device.
>>
>> If the VFIO PCI device is using the direct KVM access through IRQfd and
>> we know that KVM does not implement AIS support we refuse to realize the
>> VFIO PCI device.
>>
>> In all other cases, emulation and VFIO PCI sending interrupts through
>> QEMU, we intercept the propagated IRQ, and protect it with the QEMU AIS
>> implementation before to send it to the guest through KVM.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index d9c294a..4afe49b 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -21,18 +21,21 @@
>>   #include "hw/pci/pci_bus.h"
>>   #include "hw/pci/pci_bridge.h"
>>   #include "hw/pci/msi.h"
>> +#include "hw/vfio/pci.h"
>>   #include "qemu/error-report.h"
>>   
>>   #ifndef DEBUG_S390PCI_BUS
>>   #define DEBUG_S390PCI_BUS  0
>>   #endif
>>   
>> +#ifndef DPRINTF
>>   #define DPRINTF(fmt, ...)                                         \
>>       do {                                                          \
>>           if (DEBUG_S390PCI_BUS) {                                  \
>>               fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \
>>           }                                                         \
>>       } while (0)
>> +#endif
> 
> Maybe a sign we shouldn't be including vfio/pci.h
> 
>>   S390pciState *s390_get_phb(void)
>>   {
>> @@ -751,6 +754,15 @@ static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
>>           }
>>   
>>           if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>> +            S390FLICState *fs = s390_get_flic();
>> +            VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> +
>> +            if ((!vdev->no_kvm_msix || !vdev->no_kvm_msix) &&
>> +                (!fs || !fs->ais_supported)) {
>> +                error_setg(errp, "VFIO PCI is not supported "
>> +                           "because kernel has no AIS capability.");
>> +                return;
>> +            }
> 
> 
> Hmm, you're basically looking at private data structure fields for
> experimental disable flags, which no user, or more importantly no
> management tool, could rightfully be expected to provide.  Can we not
> take this into account to automatically do the right thing, ie.
> automatically set the necessary flag during the device realize?  Thanks,

I am not sure to understand.

Here we are in a s390x specific device and without vfio/pci.h, we have 
no access to the PCI VFIOdevice structure from the realize function.

Do you mean to add an entry in the VFIOPCIdevice realize?
A new quirk ?
An architecture callback, because we also need access to S390FLICState ?


We also have the possibility to not fall-back to AIS emulation for VFIO
but we would loose the possibility to migrate to a host not supporting AIS.


Thanks for your help.

Pierre


> 
> Alex
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Cornelia Huck]

On Mon, 9 Oct 2017 19:16:23 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 09/10/2017 16:45, Alex Williamson wrote:
> > On Wed,  4 Oct 2017 15:49:38 +0200
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >   
> >> In S390x the Adapter Interrupt Suppression facility is used to mask
> >> interrupts of other PCI devices during interruption handling.
> >>
> >> VFIO PCI allows the interrupts to be delivered rapidely through KVM via
> >> IRQfd or to be delivered through QEMU.
> >> The choice is made through the x-kvm-intx and x-kvo-misx properties of
> >> the VFIO PCI device.
> >>
> >> If the VFIO PCI device is using the direct KVM access through IRQfd and
> >> we know that KVM does not implement AIS support we refuse to realize the
> >> VFIO PCI device.
> >>
> >> In all other cases, emulation and VFIO PCI sending interrupts through
> >> QEMU, we intercept the propagated IRQ, and protect it with the QEMU AIS
> >> implementation before to send it to the guest through KVM.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> ---
> >>   hw/s390x/s390-pci-bus.c | 12 ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >> index d9c294a..4afe49b 100644
> >> --- a/hw/s390x/s390-pci-bus.c
> >> +++ b/hw/s390x/s390-pci-bus.c
> >> @@ -21,18 +21,21 @@
> >>   #include "hw/pci/pci_bus.h"
> >>   #include "hw/pci/pci_bridge.h"
> >>   #include "hw/pci/msi.h"
> >> +#include "hw/vfio/pci.h"
> >>   #include "qemu/error-report.h"
> >>   
> >>   #ifndef DEBUG_S390PCI_BUS
> >>   #define DEBUG_S390PCI_BUS  0
> >>   #endif
> >>   
> >> +#ifndef DPRINTF
> >>   #define DPRINTF(fmt, ...)                                         \
> >>       do {                                                          \
> >>           if (DEBUG_S390PCI_BUS) {                                  \
> >>               fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \
> >>           }                                                         \
> >>       } while (0)
> >> +#endif  
> > 
> > Maybe a sign we shouldn't be including vfio/pci.h
> >   
> >>   S390pciState *s390_get_phb(void)
> >>   {
> >> @@ -751,6 +754,15 @@ static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
> >>           }
> >>   
> >>           if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> >> +            S390FLICState *fs = s390_get_flic();
> >> +            VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> >> +
> >> +            if ((!vdev->no_kvm_msix || !vdev->no_kvm_msix) &&
> >> +                (!fs || !fs->ais_supported)) {
> >> +                error_setg(errp, "VFIO PCI is not supported "
> >> +                           "because kernel has no AIS capability.");
> >> +                return;
> >> +            }  
> > 
> > 
> > Hmm, you're basically looking at private data structure fields for
> > experimental disable flags, which no user, or more importantly no
> > management tool, could rightfully be expected to provide.  Can we not
> > take this into account to automatically do the right thing, ie.
> > automatically set the necessary flag during the device realize?  Thanks,  
> 
> I am not sure to understand.
> 
> Here we are in a s390x specific device and without vfio/pci.h, we have 
> no access to the PCI VFIOdevice structure from the realize function.
> 
> Do you mean to add an entry in the VFIOPCIdevice realize?
> A new quirk ?
> An architecture callback, because we also need access to S390FLICState ?
> 
> 
> We also have the possibility to not fall-back to AIS emulation for VFIO
> but we would loose the possibility to migrate to a host not supporting AIS.

Thinking about this some more: The problem is generally "if we have to
fall back to ais emulation, we cannot support any pci device that
bypasses qemu's interrupt injection", no?

So this would imply vfio devices where the experimental switch is not
used, but also virtio-pci if it is configured to use irqfd, I think.

The ugly thing is that the platform-specific code depends on a property
of the plugged device being set or not set... and that's likely to be
different from device type to device type, and may not even be
configurable for some device types.

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Pierre Morel]

On 10/10/2017 11:35, Cornelia Huck wrote:
> On Mon, 9 Oct 2017 19:16:23 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 09/10/2017 16:45, Alex Williamson wrote:
>>> On Wed,  4 Oct 2017 15:49:38 +0200
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>    
>>>> In S390x the Adapter Interrupt Suppression facility is used to mask
>>>> interrupts of other PCI devices during interruption handling.
>>>>
>>>> VFIO PCI allows the interrupts to be delivered rapidely through KVM via
>>>> IRQfd or to be delivered through QEMU.
>>>> The choice is made through the x-kvm-intx and x-kvo-misx properties of
>>>> the VFIO PCI device.
>>>>
>>>> If the VFIO PCI device is using the direct KVM access through IRQfd and
>>>> we know that KVM does not implement AIS support we refuse to realize the
>>>> VFIO PCI device.
>>>>
>>>> In all other cases, emulation and VFIO PCI sending interrupts through
>>>> QEMU, we intercept the propagated IRQ, and protect it with the QEMU AIS
>>>> implementation before to send it to the guest through KVM.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> ---
>>>>    hw/s390x/s390-pci-bus.c | 12 ++++++++++++
>>>>    1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>> index d9c294a..4afe49b 100644
>>>> --- a/hw/s390x/s390-pci-bus.c
>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>> @@ -21,18 +21,21 @@
>>>>    #include "hw/pci/pci_bus.h"
>>>>    #include "hw/pci/pci_bridge.h"
>>>>    #include "hw/pci/msi.h"
>>>> +#include "hw/vfio/pci.h"
>>>>    #include "qemu/error-report.h"
>>>>    
>>>>    #ifndef DEBUG_S390PCI_BUS
>>>>    #define DEBUG_S390PCI_BUS  0
>>>>    #endif
>>>>    
>>>> +#ifndef DPRINTF
>>>>    #define DPRINTF(fmt, ...)                                         \
>>>>        do {                                                          \
>>>>            if (DEBUG_S390PCI_BUS) {                                  \
>>>>                fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \
>>>>            }                                                         \
>>>>        } while (0)
>>>> +#endif
>>>
>>> Maybe a sign we shouldn't be including vfio/pci.h
>>>    
>>>>    S390pciState *s390_get_phb(void)
>>>>    {
>>>> @@ -751,6 +754,15 @@ static void s390_pcihost_hot_plug(HotplugHandler *hotplug_dev,
>>>>            }
>>>>    
>>>>            if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>>>> +            S390FLICState *fs = s390_get_flic();
>>>> +            VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>>>> +
>>>> +            if ((!vdev->no_kvm_msix || !vdev->no_kvm_msix) &&
>>>> +                (!fs || !fs->ais_supported)) {
>>>> +                error_setg(errp, "VFIO PCI is not supported "
>>>> +                           "because kernel has no AIS capability.");
>>>> +                return;
>>>> +            }
>>>
>>>
>>> Hmm, you're basically looking at private data structure fields for
>>> experimental disable flags, which no user, or more importantly no
>>> management tool, could rightfully be expected to provide.  Can we not
>>> take this into account to automatically do the right thing, ie.
>>> automatically set the necessary flag during the device realize?  Thanks,
>>
>> I am not sure to understand.
>>
>> Here we are in a s390x specific device and without vfio/pci.h, we have
>> no access to the PCI VFIOdevice structure from the realize function.
>>
>> Do you mean to add an entry in the VFIOPCIdevice realize?
>> A new quirk ?
>> An architecture callback, because we also need access to S390FLICState ?
>>
>>
>> We also have the possibility to not fall-back to AIS emulation for VFIO
>> but we would loose the possibility to migrate to a host not supporting AIS.
> 
> Thinking about this some more: The problem is generally "if we have to
> fall back to ais emulation, we cannot support any pci device that
> bypasses qemu's interrupt injection", no?
> 
> So this would imply vfio devices where the experimental switch is not
> used, but also virtio-pci if it is configured to use irqfd, I think.
> 
> The ugly thing is that the platform-specific code depends on a property
> of the plugged device being set or not set... and that's likely to be
> different from device type to device type, and may not even be
> configurable for some device types.
> 

Yes, right.
Since in the case we have no AIS in KVM only PCI is impacted, do you 
think we can fence IRQFD for MSI with

kvm_msi_via_irqfd_allowed = false;

in kvm_arch_init_irq_routing() ?

In fact, if we also verify the AIS availability in the same routine we 
can avoid the ugly include of vfio/pci.h

Regards,

Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Cornelia Huck]

On Tue, 10 Oct 2017 18:01:53 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Since in the case we have no AIS in KVM only PCI is impacted, do you 
> think we can fence IRQFD for MSI with
> 
> kvm_msi_via_irqfd_allowed = false;
> 
> in kvm_arch_init_irq_routing() ?

Hm, it seems we never set that for s390x... does that imply that we
never support irqfd for pci anyway?

> 
> In fact, if we also verify the AIS availability in the same routine we 
> can avoid the ugly include of vfio/pci.h

If we can get the serialization right (and setting that value to false
does indeed have the desired effect), that's probably the way to go.

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Pierre Morel]

On 11/10/2017 14:20, Cornelia Huck wrote:
> On Tue, 10 Oct 2017 18:01:53 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Since in the case we have no AIS in KVM only PCI is impacted, do you
>> think we can fence IRQFD for MSI with
>>
>> kvm_msi_via_irqfd_allowed = false;
>>
>> in kvm_arch_init_irq_routing() ?
> 
> Hm, it seems we never set that for s390x... does that imply that we
> never support irqfd for pci anyway?
> 
>>
>> In fact, if we also verify the AIS availability in the same routine we
>> can avoid the ugly include of vfio/pci.h
> 
> If we can get the serialization right (and setting that value to false
> does indeed have the desired effect), that's probably the way to go.
> 

OK, thanks, I give it a try and propose a v2 on this basis.



-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Pierre Morel]

On 11/10/2017 14:20, Cornelia Huck wrote:
> On Tue, 10 Oct 2017 18:01:53 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Since in the case we have no AIS in KVM only PCI is impacted, do you
>> think we can fence IRQFD for MSI with
>>
>> kvm_msi_via_irqfd_allowed = false;
>>
>> in kvm_arch_init_irq_routing() ?
> 
> Hm, it seems we never set that for s390x... does that imply that we
> never support irqfd for pci anyway?

... yes, we never supported irqfd for virtio-pci anyway.
This explains a lot of things to me.
:)

> 
>>
>> In fact, if we also verify the AIS availability in the same routine we
>> can avoid the ugly include of vfio/pci.h
> 
> If we can get the serialization right (and setting that value to false
> does indeed have the desired effect), that's probably the way to go.
> 

So now, in v2, we will be able to **enable** irqfd if we detect that we 
handle AIS in kernel. :)


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Cornelia Huck]

On Thu, 12 Oct 2017 16:48:26 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 11/10/2017 14:20, Cornelia Huck wrote:
> > On Tue, 10 Oct 2017 18:01:53 +0200
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >   
> >> Since in the case we have no AIS in KVM only PCI is impacted, do you
> >> think we can fence IRQFD for MSI with
> >>
> >> kvm_msi_via_irqfd_allowed = false;
> >>
> >> in kvm_arch_init_irq_routing() ?  
> > 
> > Hm, it seems we never set that for s390x... does that imply that we
> > never support irqfd for pci anyway?  
> 
> ... yes, we never supported irqfd for virtio-pci anyway.
> This explains a lot of things to me.
> :)

:D

> 
> >   
> >>
> >> In fact, if we also verify the AIS availability in the same routine we
> >> can avoid the ugly include of vfio/pci.h  
> > 
> > If we can get the serialization right (and setting that value to false
> > does indeed have the desired effect), that's probably the way to go.
> >   
> 
> So now, in v2, we will be able to **enable** irqfd if we detect that we 
> handle AIS in kernel. :)

Let's see how that works out :)

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Christian Borntraeger]

Now I thought about that for a while and I start to think that we cannot implement ais
in QEMU and cover all cases.
One aspect was certainly passthrough (like you handled in patch 4).
Another aspect is that some interrupts might be injected from the kernel - even for
emulated devices. e.g. virtio-pci together with vhost-net, will inject interrupts via
the set_irq callback. I think disabling irqfd for these cases is not a good idea.

So what about adding a new KVM capability (for 4.14), fixup the other things in
QEMU and then bind it to the new capability?

Christian

On 10/04/2017 03:49 PM, Pierre Morel wrote:
> Currently AIS support has several problems:
> 
> - AIS support in KVM is reported if KVM has AIS capability.
> - Testing if KVM FLIC attributes for AIS are supported does not take into
>   account if AIS is supported by KVM.
> - KVM report supporting the AIS FLIC features but denies their usage
>   if the host kernel does not support the AIS feature.
> - Testing if the Adapter interrupt must be suppressed is done looking at the
>   ISC and ignores the adapter properties.
> - Emulation of PCI devices can only be done with KVM support for AIS.
> - Migration of emulated devices can only be done if both side support AIS in KVM
> 
> I would like to make some modifications to the code where I think things are
> not handled at the right place.
> 
> Therefor I propose these changes.
> - Use the CPU model to enable AIS in the guest, even without KVM backup
> - Ask KVM for AIS support in kvm_flic realize
> - add simm/nimm attributes to the KVM FLIC interface to support emulation
>   and migration between hosts with and without AIS support in KVM.
> - Modify the zPCI VFIO realize function to refuse VFIO PCI devices without
>   AIS support in KVM.
> - Modify the AIS migration to support emulation and heterogeneous hosts.
> 
> 
> Pierre Morel (5):
>   s390x/kvm: Enable AIS from CPU model always
>   s390x/css: Use AIS AIRQ injection only if adapter support AIS
>   s390x/intc: Emulate Adapter Interrupt Suppression
>   s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported
>   s390x/intc: AIS is now always migratable
> 
>  hw/intc/s390_flic.c     |  3 +-
>  hw/intc/s390_flic_kvm.c | 94 ++++++++++++++++++++++++++++++++++++++-----------
>  hw/s390x/css.c          |  6 ++--
>  hw/s390x/s390-pci-bus.c | 12 +++++++
>  target/s390x/kvm.c      |  1 +
>  5 files changed, 91 insertions(+), 25 deletions(-)
> 

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Cornelia Huck]

On Mon, 30 Oct 2017 09:28:09 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Now I thought about that for a while and I start to think that we cannot implement ais
> in QEMU and cover all cases.
> One aspect was certainly passthrough (like you handled in patch 4).
> Another aspect is that some interrupts might be injected from the kernel - even for
> emulated devices. e.g. virtio-pci together with vhost-net, will inject interrupts via
> the set_irq callback. I think disabling irqfd for these cases is not a good idea.

Is there still a fallback for irqfd emulation?

> 
> So what about adding a new KVM capability (for 4.14), fixup the other things in
> QEMU and then bind it to the new capability?

For 4.15, surely?

Probably the only way we can make this work correctly...

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Christian Borntraeger]

On 10/30/2017 01:42 PM, Cornelia Huck wrote:
> On Mon, 30 Oct 2017 09:28:09 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> Now I thought about that for a while and I start to think that we cannot implement ais
>> in QEMU and cover all cases.
>> One aspect was certainly passthrough (like you handled in patch 4).
>> Another aspect is that some interrupts might be injected from the kernel - even for
>> emulated devices. e.g. virtio-pci together with vhost-net, will inject interrupts via
>> the set_irq callback. I think disabling irqfd for these cases is not a good idea.
> 
> Is there still a fallback for irqfd emulation?

it might disable dataplane or other things. (it once did). So I think we should not
go down this path.

> 
>>
>> So what about adding a new KVM capability (for 4.14), fixup the other things in
>> QEMU and then bind it to the new capability?
> 
> For 4.15, surely?
> 
> Probably the only way we can make this work correctly...
> 

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Pierre Morel]

On 30/10/2017 13:44, Christian Borntraeger wrote:
> 
> 
> On 10/30/2017 01:42 PM, Cornelia Huck wrote:
>> On Mon, 30 Oct 2017 09:28:09 +0100
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> Now I thought about that for a while and I start to think that we cannot implement ais
>>> in QEMU and cover all cases.
>>> One aspect was certainly passthrough (like you handled in patch 4).
>>> Another aspect is that some interrupts might be injected from the kernel - even for
>>> emulated devices. e.g. virtio-pci together with vhost-net, will inject interrupts via
>>> the set_irq callback. I think disabling irqfd for these cases is not a good idea.
>>
>> Is there still a fallback for irqfd emulation?
> 
> it might disable dataplane or other things. (it once did). So I think we should not
> go down this path.
> 
>>
>>>
>>> So what about adding a new KVM capability (for 4.14), fixup the other things in
>>> QEMU and then bind it to the new capability?
>>
>> For 4.15, surely?
>>
>> Probably the only way we can make this work correctly...
>>

I may have not understand.

Why do we need a new capability, we already have the KVM_CAP_S390_AIS 
capability?
The PCI device has a netdev property pointing to a netdev, if this 
netdev sets the vhost property, can't we test this to know if we can 
realize this device or not?

Using virtio-pci instead of virtio-ccw is not the first choice for S390. 
The use case I see for S390 using virtio-pci is as a fallback in case 
for the migration of a PCI device the target host does not support AIS 
or do not have VFIO device and one do not want to modify the guest.

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Christian Borntraeger]

On 10/30/2017 02:44 PM, Pierre Morel wrote:
> On 30/10/2017 13:44, Christian Borntraeger wrote:
>>
>>
>> On 10/30/2017 01:42 PM, Cornelia Huck wrote:
>>> On Mon, 30 Oct 2017 09:28:09 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>
>>>> Now I thought about that for a while and I start to think that we cannot implement ais
>>>> in QEMU and cover all cases.
>>>> One aspect was certainly passthrough (like you handled in patch 4).
>>>> Another aspect is that some interrupts might be injected from the kernel - even for
>>>> emulated devices. e.g. virtio-pci together with vhost-net, will inject interrupts via
>>>> the set_irq callback. I think disabling irqfd for these cases is not a good idea.
>>>
>>> Is there still a fallback for irqfd emulation?
>>
>> it might disable dataplane or other things. (it once did). So I think we should not
>> go down this path.
>>
>>>
>>>>
>>>> So what about adding a new KVM capability (for 4.14), fixup the other things in
>>>> QEMU and then bind it to the new capability?
>>>
>>> For 4.15, surely?
>>>
>>> Probably the only way we can make this work correctly...
>>>
> 
> I may have not understand.
> 
> Why do we need a new capability, we already have the KVM_CAP_S390_AIS capability?

To mark a kernel that supports AIS+migration without having to instantiate a flic device.


> The PCI device has a netdev property pointing to a netdev, if this netdev sets the vhost property, can't we test this to know if we can realize this device or not?
> 
> Using virtio-pci instead of virtio-ccw is not the first choice for S390. The use case I see for S390 using virtio-pci is as a fallback in case for the migration of a PCI device the target host does not support AIS or do not have VFIO device and one do not want to modify the guest.

This was just one example. Having the interrupt controller in the kernel, implementing AIS in
qemu is very prone to break something that we have forgotten about.


FWIW, I am testing a guest patch that enables zPCI without AIS. Its as simple as


diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 7b30af5..9b24836 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -953,7 +953,7 @@ static int __init pci_base_init(void)
        if (!s390_pci_probe)
                return 0;
 
-       if (!test_facility(69) || !test_facility(71) || !test_facility(72))
+       if (!test_facility(69) || !test_facility(71))
                return 0;
 
        rc = zpci_debug_init();
diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
index ea34086..61f8c82 100644
--- a/arch/s390/pci/pci_insn.c
+++ b/arch/s390/pci/pci_insn.c
@@ -7,6 +7,7 @@
 #include <linux/export.h>
 #include <linux/errno.h>
 #include <linux/delay.h>
+#include <asm/facility.h>
 #include <asm/pci_insn.h>
 #include <asm/pci_debug.h>
 #include <asm/processor.h>
@@ -93,6 +94,8 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
 /* Set Interruption Controls */
 void zpci_set_irq_ctrl(u16 ctl, char *unused, u8 isc)
 {
+       if (!test_facility(72))
+               return;
        asm volatile (
                "       .insn   rsy,0xeb00000000d1,%[ctl],%[isc],%[u]\n"
                : : [ctl] "d" (ctl), [isc] "d" (isc << 27), [u] "Q" (*unused));

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Halil Pasic]

On 10/30/2017 02:48 PM, Christian Borntraeger wrote:
> 
> 
> On 10/30/2017 02:44 PM, Pierre Morel wrote:
>> On 30/10/2017 13:44, Christian Borntraeger wrote:
>>>
>>>
>>> On 10/30/2017 01:42 PM, Cornelia Huck wrote:
>>>> On Mon, 30 Oct 2017 09:28:09 +0100
>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>>
>>>>> Now I thought about that for a while and I start to think that we cannot implement ais
>>>>> in QEMU and cover all cases.
>>>>> One aspect was certainly passthrough (like you handled in patch 4).
>>>>> Another aspect is that some interrupts might be injected from the kernel - even for
>>>>> emulated devices. e.g. virtio-pci together with vhost-net, will inject interrupts via
>>>>> the set_irq callback. I think disabling irqfd for these cases is not a good idea.
>>>>
>>>> Is there still a fallback for irqfd emulation?
>>>
>>> it might disable dataplane or other things. (it once did). So I think we should not
>>> go down this path.
>>>
>>>>
>>>>>
>>>>> So what about adding a new KVM capability (for 4.14), fixup the other things in
>>>>> QEMU and then bind it to the new capability?
>>>>
>>>> For 4.15, surely?
>>>>
>>>> Probably the only way we can make this work correctly...
>>>>
>>
>> I may have not understand.
>>
>> Why do we need a new capability, we already have the KVM_CAP_S390_AIS capability?
> 
> To mark a kernel that supports AIS+migration without having to instantiate a flic device.
> 

It's about libvirt probing AFAIU. Right now, to tell if we have migration for
AIS we need a flic device instantiated in the kernel (we need the fd to tell
if the flic attributes are supported by the kernel).

> 
>> The PCI device has a netdev property pointing to a netdev, if this netdev sets the vhost property, can't we test this to know if we can realize this device or not?
>>
>> Using virtio-pci instead of virtio-ccw is not the first choice for S390. The use case I see for S390 using virtio-pci is as a fallback in case for the migration of a PCI device the target host does not support AIS or do not have VFIO device and one do not want to modify the guest.
> 
> This was just one example. Having the interrupt controller in the kernel, implementing AIS in
> qemu is very prone to break something that we have forgotten about.
> 

I tend to agree with Christian. While doing emulation AIS in QEMU
for scenarios where all adapter interrupts (subject to AIS) are
guaranteed to go through QEMU is possible, it is also bound to
introduce a whole lot of added complexity.

Frankly, I doubt the gain outweighs the pain in this case.

Regards,
Halil

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Cornelia Huck]

On Mon, 30 Oct 2017 14:48:23 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:


> FWIW, I am testing a guest patch that enables zPCI without AIS. Its as simple as
> 
> 
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 7b30af5..9b24836 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -953,7 +953,7 @@ static int __init pci_base_init(void)
>         if (!s390_pci_probe)
>                 return 0;
>  
> -       if (!test_facility(69) || !test_facility(71) || !test_facility(72))
> +       if (!test_facility(69) || !test_facility(71))
>                 return 0;
>  
>         rc = zpci_debug_init();
> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
> index ea34086..61f8c82 100644
> --- a/arch/s390/pci/pci_insn.c
> +++ b/arch/s390/pci/pci_insn.c
> @@ -7,6 +7,7 @@
>  #include <linux/export.h>
>  #include <linux/errno.h>
>  #include <linux/delay.h>
> +#include <asm/facility.h>
>  #include <asm/pci_insn.h>
>  #include <asm/pci_debug.h>
>  #include <asm/processor.h>
> @@ -93,6 +94,8 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
>  /* Set Interruption Controls */
>  void zpci_set_irq_ctrl(u16 ctl, char *unused, u8 isc)
>  {
> +       if (!test_facility(72))
> +               return;
>         asm volatile (
>                 "       .insn   rsy,0xeb00000000d1,%[ctl],%[isc],%[u]\n"
>                 : : [ctl] "d" (ctl), [isc] "d" (isc << 27), [u] "Q" (*unused));
> 

Sounds good. Presumably this makes the adapter interrupt handling work
as for virtio (and qdio)? Is there any ais-less pci hardware out in the
wild?

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Christian Borntraeger]

On 10/30/2017 05:59 PM, Cornelia Huck wrote:
> On Mon, 30 Oct 2017 14:48:23 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> 
>> FWIW, I am testing a guest patch that enables zPCI without AIS. Its as simple as
>>
>>
>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>> index 7b30af5..9b24836 100644
>> --- a/arch/s390/pci/pci.c
>> +++ b/arch/s390/pci/pci.c
>> @@ -953,7 +953,7 @@ static int __init pci_base_init(void)
>>         if (!s390_pci_probe)
>>                 return 0;
>>  
>> -       if (!test_facility(69) || !test_facility(71) || !test_facility(72))
>> +       if (!test_facility(69) || !test_facility(71))
>>                 return 0;
>>  
>>         rc = zpci_debug_init();
>> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
>> index ea34086..61f8c82 100644
>> --- a/arch/s390/pci/pci_insn.c
>> +++ b/arch/s390/pci/pci_insn.c
>> @@ -7,6 +7,7 @@
>>  #include <linux/export.h>
>>  #include <linux/errno.h>
>>  #include <linux/delay.h>
>> +#include <asm/facility.h>
>>  #include <asm/pci_insn.h>
>>  #include <asm/pci_debug.h>
>>  #include <asm/processor.h>
>> @@ -93,6 +94,8 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
>>  /* Set Interruption Controls */
>>  void zpci_set_irq_ctrl(u16 ctl, char *unused, u8 isc)
>>  {
>> +       if (!test_facility(72))
>> +               return;
>>         asm volatile (
>>                 "       .insn   rsy,0xeb00000000d1,%[ctl],%[isc],%[u]\n"
>>                 : : [ctl] "d" (ctl), [isc] "d" (isc << 27), [u] "Q" (*unused));
>>
> 
> Sounds good. Presumably this makes the adapter interrupt handling work
> as for virtio (and qdio)? Is there any ais-less pci hardware out in the
> wild?
> 

ais is z specific, not PCI specific. So PCI cards should not care as far as I can tell.

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Pierre Morel]

On 30/10/2017 18:08, Christian Borntraeger wrote:
> 
> On 10/30/2017 05:59 PM, Cornelia Huck wrote:
>> On Mon, 30 Oct 2017 14:48:23 +0100
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>
>>> FWIW, I am testing a guest patch that enables zPCI without AIS. Its as simple as
>>>
>>>
>>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>>> index 7b30af5..9b24836 100644
>>> --- a/arch/s390/pci/pci.c
>>> +++ b/arch/s390/pci/pci.c
>>> @@ -953,7 +953,7 @@ static int __init pci_base_init(void)
>>>          if (!s390_pci_probe)
>>>                  return 0;
>>>   
>>> -       if (!test_facility(69) || !test_facility(71) || !test_facility(72))
>>> +       if (!test_facility(69) || !test_facility(71))
>>>                  return 0;
>>>   
>>>          rc = zpci_debug_init();
>>> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
>>> index ea34086..61f8c82 100644
>>> --- a/arch/s390/pci/pci_insn.c
>>> +++ b/arch/s390/pci/pci_insn.c
>>> @@ -7,6 +7,7 @@
>>>   #include <linux/export.h>
>>>   #include <linux/errno.h>
>>>   #include <linux/delay.h>
>>> +#include <asm/facility.h>
>>>   #include <asm/pci_insn.h>
>>>   #include <asm/pci_debug.h>
>>>   #include <asm/processor.h>
>>> @@ -93,6 +94,8 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
>>>   /* Set Interruption Controls */
>>>   void zpci_set_irq_ctrl(u16 ctl, char *unused, u8 isc)
>>>   {
>>> +       if (!test_facility(72))
>>> +               return;
>>>          asm volatile (
>>>                  "       .insn   rsy,0xeb00000000d1,%[ctl],%[isc],%[u]\n"
>>>                  : : [ctl] "d" (ctl), [isc] "d" (isc << 27), [u] "Q" (*unused));
>>>
>>
>> Sounds good. Presumably this makes the adapter interrupt handling work
>> as for virtio (and qdio)? Is there any ais-less pci hardware out in the
>> wild?
>>
> 
> ais is z specific, not PCI specific. So PCI cards should not care as far as I can tell.
> 
> 

I bet Conny meant PCI hardware globaly, including zPCI adapters.
I bet none but I will better ask Sebastian or Gerald if they know.


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Halil Pasic]

      

On 10/30/2017 06:38 PM, Pierre Morel wrote:
> On 30/10/2017 18:08, Christian Borntraeger wrote:
>>
>> On 10/30/2017 05:59 PM, Cornelia Huck wrote:
>>> On Mon, 30 Oct 2017 14:48:23 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>
>>>
>>>> FWIW, I am testing a guest patch that enables zPCI without AIS. Its as simple as
>>>>
>>>>
>>>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>>>> index 7b30af5..9b24836 100644
>>>> --- a/arch/s390/pci/pci.c
>>>> +++ b/arch/s390/pci/pci.c
>>>> @@ -953,7 +953,7 @@ static int __init pci_base_init(void)
>>>>          if (!s390_pci_probe)
>>>>                  return 0;
>>>>   -       if (!test_facility(69) || !test_facility(71) || !test_facility(72))
>>>> +       if (!test_facility(69) || !test_facility(71))
>>>>                  return 0;
>>>>            rc = zpci_debug_init();
>>>> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
>>>> index ea34086..61f8c82 100644
>>>> --- a/arch/s390/pci/pci_insn.c
>>>> +++ b/arch/s390/pci/pci_insn.c
>>>> @@ -7,6 +7,7 @@
>>>>   #include <linux/export.h>
>>>>   #include <linux/errno.h>
>>>>   #include <linux/delay.h>
>>>> +#include <asm/facility.h>
>>>>   #include <asm/pci_insn.h>
>>>>   #include <asm/pci_debug.h>
>>>>   #include <asm/processor.h>
>>>> @@ -93,6 +94,8 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
>>>>   /* Set Interruption Controls */
>>>>   void zpci_set_irq_ctrl(u16 ctl, char *unused, u8 isc)
>>>>   {
>>>> +       if (!test_facility(72))
>>>> +               return;
>>>>          asm volatile (
>>>>                  "       .insn   rsy,0xeb00000000d1,%[ctl],%[isc],%[u]\n"
>>>>                  : : [ctl] "d" (ctl), [isc] "d" (isc << 27), [u] "Q" (*unused));
>>>>
>>>
>>> Sounds good. Presumably this makes the adapter interrupt handling work
>>> as for virtio (and qdio)? Is there any ais-less pci hardware out in the
>>> wild?

@Connie:
Do you have something in mind, or is curiosity the only reason for asking?

>>>
>>
>> ais is z specific, not PCI specific. So PCI cards should not care as far as I can tell.
>>
>>
> 
> I bet Conny meant PCI hardware globaly, including zPCI adapters.
> I bet none but I will better ask Sebastian or Gerald if they know.
> 

I side with Christian: it is a machine thing and not a card thing. So
the question is if there is a z machine which supports zpci but does
not support AIS. My guess is that such a machine was never produced.

Halil

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Cornelia Huck]

On Mon, 30 Oct 2017 19:58:15 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 10/30/2017 06:38 PM, Pierre Morel wrote:
> > On 30/10/2017 18:08, Christian Borntraeger wrote:  
> >>
> >> On 10/30/2017 05:59 PM, Cornelia Huck wrote:  
> >>> On Mon, 30 Oct 2017 14:48:23 +0100
> >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>>
> >>>  
> >>>> FWIW, I am testing a guest patch that enables zPCI without AIS. Its as simple as
> >>>>
> >>>>
> >>>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> >>>> index 7b30af5..9b24836 100644
> >>>> --- a/arch/s390/pci/pci.c
> >>>> +++ b/arch/s390/pci/pci.c
> >>>> @@ -953,7 +953,7 @@ static int __init pci_base_init(void)
> >>>>          if (!s390_pci_probe)
> >>>>                  return 0;
> >>>>   -       if (!test_facility(69) || !test_facility(71) || !test_facility(72))
> >>>> +       if (!test_facility(69) || !test_facility(71))
> >>>>                  return 0;
> >>>>            rc = zpci_debug_init();
> >>>> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
> >>>> index ea34086..61f8c82 100644
> >>>> --- a/arch/s390/pci/pci_insn.c
> >>>> +++ b/arch/s390/pci/pci_insn.c
> >>>> @@ -7,6 +7,7 @@
> >>>>   #include <linux/export.h>
> >>>>   #include <linux/errno.h>
> >>>>   #include <linux/delay.h>
> >>>> +#include <asm/facility.h>
> >>>>   #include <asm/pci_insn.h>
> >>>>   #include <asm/pci_debug.h>
> >>>>   #include <asm/processor.h>
> >>>> @@ -93,6 +94,8 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
> >>>>   /* Set Interruption Controls */
> >>>>   void zpci_set_irq_ctrl(u16 ctl, char *unused, u8 isc)
> >>>>   {
> >>>> +       if (!test_facility(72))
> >>>> +               return;
> >>>>          asm volatile (
> >>>>                  "       .insn   rsy,0xeb00000000d1,%[ctl],%[isc],%[u]\n"
> >>>>                  : : [ctl] "d" (ctl), [isc] "d" (isc << 27), [u] "Q" (*unused));
> >>>>  
> >>>
> >>> Sounds good. Presumably this makes the adapter interrupt handling work
> >>> as for virtio (and qdio)? Is there any ais-less pci hardware out in the
> >>> wild?  
> 
> @Connie:
> Do you have something in mind, or is curiosity the only reason for asking?

I just want to make sure we don't miss anything.

> 
> >>>  
> >>
> >> ais is z specific, not PCI specific. So PCI cards should not care as far as I can tell.
> >>
> >>  
> > 
> > I bet Conny meant PCI hardware globaly, including zPCI adapters.
> > I bet none but I will better ask Sebastian or Gerald if they know.
> >   
> 
> I side with Christian: it is a machine thing and not a card thing. So
> the question is if there is a z machine which supports zpci but does
> not support AIS. My guess is that such a machine was never produced.

That's exactly what I meant, and I would not expect such a machine to
exist either.

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Christian Borntraeger]

On 11/06/2017 09:52 AM, Cornelia Huck wrote:
> On Mon, 30 Oct 2017 19:58:15 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 10/30/2017 06:38 PM, Pierre Morel wrote:
>>> On 30/10/2017 18:08, Christian Borntraeger wrote:  
>>>>
>>>> On 10/30/2017 05:59 PM, Cornelia Huck wrote:  
>>>>> On Mon, 30 Oct 2017 14:48:23 +0100
>>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>>>
>>>>>  
>>>>>> FWIW, I am testing a guest patch that enables zPCI without AIS. Its as simple as
>>>>>>
>>>>>>
>>>>>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>>>>>> index 7b30af5..9b24836 100644
>>>>>> --- a/arch/s390/pci/pci.c
>>>>>> +++ b/arch/s390/pci/pci.c
>>>>>> @@ -953,7 +953,7 @@ static int __init pci_base_init(void)
>>>>>>          if (!s390_pci_probe)
>>>>>>                  return 0;
>>>>>>   -       if (!test_facility(69) || !test_facility(71) || !test_facility(72))
>>>>>> +       if (!test_facility(69) || !test_facility(71))
>>>>>>                  return 0;
>>>>>>            rc = zpci_debug_init();
>>>>>> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
>>>>>> index ea34086..61f8c82 100644
>>>>>> --- a/arch/s390/pci/pci_insn.c
>>>>>> +++ b/arch/s390/pci/pci_insn.c
>>>>>> @@ -7,6 +7,7 @@
>>>>>>   #include <linux/export.h>
>>>>>>   #include <linux/errno.h>
>>>>>>   #include <linux/delay.h>
>>>>>> +#include <asm/facility.h>
>>>>>>   #include <asm/pci_insn.h>
>>>>>>   #include <asm/pci_debug.h>
>>>>>>   #include <asm/processor.h>
>>>>>> @@ -93,6 +94,8 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
>>>>>>   /* Set Interruption Controls */
>>>>>>   void zpci_set_irq_ctrl(u16 ctl, char *unused, u8 isc)
>>>>>>   {
>>>>>> +       if (!test_facility(72))
>>>>>> +               return;
>>>>>>          asm volatile (
>>>>>>                  "       .insn   rsy,0xeb00000000d1,%[ctl],%[isc],%[u]\n"
>>>>>>                  : : [ctl] "d" (ctl), [isc] "d" (isc << 27), [u] "Q" (*unused));
>>>>>>  
>>>>>
>>>>> Sounds good. Presumably this makes the adapter interrupt handling work
>>>>> as for virtio (and qdio)? Is there any ais-less pci hardware out in the
>>>>> wild?  
>>
>> @Connie:
>> Do you have something in mind, or is curiosity the only reason for asking?
> 
> I just want to make sure we don't miss anything.
> 
>>
>>>>>  
>>>>
>>>> ais is z specific, not PCI specific. So PCI cards should not care as far as I can tell.
>>>>
>>>>  
>>>
>>> I bet Conny meant PCI hardware globaly, including zPCI adapters.
>>> I bet none but I will better ask Sebastian or Gerald if they know.
>>>   
>>
>> I side with Christian: it is a machine thing and not a card thing. So
>> the question is if there is a z machine which supports zpci but does
>> not support AIS. My guess is that such a machine was never produced.
> 
> That's exactly what I meant, and I would not expect such a machine to
> exist either.

That machine is called qemu 2.10 :-p



FWIW, I talked with Sebastian about that and he is fine with such a change. He
required some changes and I will work on such a patch.

Re: [Qemu-devel] [PATCH v1 0/5][RFC] Refactoring of AIS support

[Pierre Morel]

On 06/11/2017 09:54, Christian Borntraeger wrote:
> 
> 
> On 11/06/2017 09:52 AM, Cornelia Huck wrote:
>> On Mon, 30 Oct 2017 19:58:15 +0100
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 10/30/2017 06:38 PM, Pierre Morel wrote:
>>>> On 30/10/2017 18:08, Christian Borntraeger wrote:
>>>>>
>>>>> On 10/30/2017 05:59 PM, Cornelia Huck wrote:
>>>>>> On Mon, 30 Oct 2017 14:48:23 +0100
>>>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>>>>
>>>>>>   
>>>>>>> FWIW, I am testing a guest patch that enables zPCI without AIS. Its as simple as
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>>>>>>> index 7b30af5..9b24836 100644
>>>>>>> --- a/arch/s390/pci/pci.c
>>>>>>> +++ b/arch/s390/pci/pci.c
>>>>>>> @@ -953,7 +953,7 @@ static int __init pci_base_init(void)
>>>>>>>           if (!s390_pci_probe)
>>>>>>>                   return 0;
>>>>>>>    -       if (!test_facility(69) || !test_facility(71) || !test_facility(72))
>>>>>>> +       if (!test_facility(69) || !test_facility(71))
>>>>>>>                   return 0;
>>>>>>>             rc = zpci_debug_init();
>>>>>>> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
>>>>>>> index ea34086..61f8c82 100644
>>>>>>> --- a/arch/s390/pci/pci_insn.c
>>>>>>> +++ b/arch/s390/pci/pci_insn.c
>>>>>>> @@ -7,6 +7,7 @@
>>>>>>>    #include <linux/export.h>
>>>>>>>    #include <linux/errno.h>
>>>>>>>    #include <linux/delay.h>
>>>>>>> +#include <asm/facility.h>
>>>>>>>    #include <asm/pci_insn.h>
>>>>>>>    #include <asm/pci_debug.h>
>>>>>>>    #include <asm/processor.h>
>>>>>>> @@ -93,6 +94,8 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
>>>>>>>    /* Set Interruption Controls */
>>>>>>>    void zpci_set_irq_ctrl(u16 ctl, char *unused, u8 isc)
>>>>>>>    {
>>>>>>> +       if (!test_facility(72))
>>>>>>> +               return;
>>>>>>>           asm volatile (
>>>>>>>                   "       .insn   rsy,0xeb00000000d1,%[ctl],%[isc],%[u]\n"
>>>>>>>                   : : [ctl] "d" (ctl), [isc] "d" (isc << 27), [u] "Q" (*unused));
>>>>>>>   
>>>>>>
>>>>>> Sounds good. Presumably this makes the adapter interrupt handling work
>>>>>> as for virtio (and qdio)? Is there any ais-less pci hardware out in the
>>>>>> wild?
>>>
>>> @Connie:
>>> Do you have something in mind, or is curiosity the only reason for asking?
>>
>> I just want to make sure we don't miss anything.
>>
>>>
>>>>>>   
>>>>>
>>>>> ais is z specific, not PCI specific. So PCI cards should not care as far as I can tell.
>>>>>
>>>>>   
>>>>
>>>> I bet Conny meant PCI hardware globaly, including zPCI adapters.
>>>> I bet none but I will better ask Sebastian or Gerald if they know.
>>>>    
>>>
>>> I side with Christian: it is a machine thing and not a card thing. So
>>> the question is if there is a z machine which supports zpci but does
>>> not support AIS. My guess is that such a machine was never produced.
>>
>> That's exactly what I meant, and I would not expect such a machine to
>> exist either.
> 
> That machine is called qemu 2.10 :-p
> 
> 
> 
> FWIW, I talked with Sebastian about that and he is fine with such a change. He
> required some changes and I will work on such a patch.
> 
> 

My analyze is that providing AIS to the guest can work, with the 
precaution to handle spurious interrupts, until we use GISA for PCI.

Then when we introduce GISA for PCI, we will need to think again about 
providing AIS to the guest.

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany