[Qemu-devel] [PATCH v1 0/5] s390x/vfio: VFIO-AP interrupt control interception

[Qemu-devel] [PATCH v1 0/5] s390x/vfio: VFIO-AP interrupt control interception

[Pierre Morel]

The S390 APQP/AQIC instruction can be intercepted by the host
to configure the AP queues interruption handling for and handle
the ISC used by the host and the guest and the indicator address.

This patch series define the AQIC feature in the cpumodel,
extend the APDevice type for per queue interrupt handling,
intercept the APQP/AQIC instruction, uses the S390 adapter interface
to setup the adapter and use a VFIO ioctl to let the VFIO-AP
driver handle the host instruction associated with the intercepted
guest instruction.

This patch serie can be tested with the Linux/KVM patch series
for the VFIO-AP driver: "s390: vfio: ap: Using GISA for AP Interrupt"

Pierre Morel (5):
  s390x/vfio: ap: Linux uapi VFIO place holder
  s390x/cpumodel: Set up CPU model for AQIC interception
  s390x/vfio: ap: Definition for AP Adapter type
  s390x/vfio: ap: Intercepting AP Queue Interrupt Control
  s390x/vfio: ap: Implementing AP Queue Interrupt Control

 hw/vfio/ap.c                    | 100 ++++++++++++++++++++++++++++++++
 include/hw/s390x/ap-device.h    |  55 ++++++++++++++++++
 include/hw/s390x/css.h          |   1 +
 linux-headers/linux/vfio.h      |  22 +++++++
 target/s390x/cpu_features.c     |   1 +
 target/s390x/cpu_features_def.h |   1 +
 target/s390x/cpu_models.c       |   1 +
 target/s390x/kvm.c              |  20 +++++++
 8 files changed, 201 insertions(+)

-- 
2.17.0

[Qemu-devel] [PATCH v1 1/5] s390x/vfio: ap: Linux uapi VFIO place holder

[Pierre Morel]

This file would be copied from Linux,
I put it here for the review.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 linux-headers/linux/vfio.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index ceb6453394..32b1fec362 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -816,6 +816,28 @@ struct vfio_iommu_spapr_tce_remove {
 };
 #define VFIO_IOMMU_SPAPR_TCE_REMOVE	_IO(VFIO_TYPE, VFIO_BASE + 20)
 
+/**
+ * VFIO_AP_SET_IRQ - _IOWR(VFIO_TYPE, VFIO_BASE + 21, struct vfio_ap_aqic)
+ *
+ * Setup IRQ for an AP Queue
+ * @cmd contains the AP queue number (apqn)
+ * @status receives the resulting status of the command
+ * @nib is the Notification Indicator byte address
+ * @adapter_id allows to retrieve the associated adapter
+ */
+struct vfio_ap_aqic {
+	__u32   argsz;
+	__u32   flags;
+	/* in */
+	__u64 cmd;
+	__u64 status;
+	__u64 nib;
+	__u32 adapter_id;
+};
+#define VFIO_AP_SET_IRQ			_IO(VFIO_TYPE, VFIO_BASE + 21)
+#define VFIO_AP_CLEAR_IRQ		_IO(VFIO_TYPE, VFIO_BASE + 22)
+
 /* ***************************************************************** */
 
+
 #endif /* VFIO_H */
-- 
2.17.0

[Qemu-devel] [PATCH v1 2/5] s390x/cpumodel: Set up CPU model for AQIC interception

[Pierre Morel]

A new CPU model facilities is introduced to support AP devices
interruption interception for a KVM guest.

CPU model facility:

The S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, CPU facility indicates
whether AP interruption interception is available to the guest.
This feature will be enabled only if the AP instructions are
available on the linux host and AQIC facility is installed on
the host.

This feature must be turned on from userspace to intercept AP
instructions on the KVM guest. The QEMU command line to turn
this feature on looks something like this:

    qemu-system-s390x ... -cpu xxx,aqci=on ...

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 target/s390x/cpu_features.c     | 1 +
 target/s390x/cpu_features_def.h | 1 +
 target/s390x/cpu_models.c       | 1 +
 3 files changed, 3 insertions(+)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 60cfeba48f..c464abf30a 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -84,6 +84,7 @@ static const S390FeatDef s390_features[] = {
     FEAT_INIT("sema", S390_FEAT_TYPE_STFL, 59, "Semaphore-assist facility"),
     FEAT_INIT("tsi", S390_FEAT_TYPE_STFL, 60, "Time-slice Instrumentation facility"),
     FEAT_INIT("ri", S390_FEAT_TYPE_STFL, 64, "CPU runtime-instrumentation facility"),
+    FEAT_INIT("aqic", S390_FEAT_TYPE_STFL, 65, "AP-Queue interruption Control facility"),
     FEAT_INIT("zpci", S390_FEAT_TYPE_STFL, 69, "z/PCI facility"),
     FEAT_INIT("aen", S390_FEAT_TYPE_STFL, 71, "General-purpose-adapter-event-notification facility"),
     FEAT_INIT("ais", S390_FEAT_TYPE_STFL, 72, "General-purpose-adapter-interruption-suppression facility"),
diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
index 5fc7e7bf01..3f22780104 100644
--- a/target/s390x/cpu_features_def.h
+++ b/target/s390x/cpu_features_def.h
@@ -72,6 +72,7 @@ typedef enum {
     S390_FEAT_SEMAPHORE_ASSIST,
     S390_FEAT_TIME_SLICE_INSTRUMENTATION,
     S390_FEAT_RUNTIME_INSTRUMENTATION,
+    S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL,
     S390_FEAT_ZPCI,
     S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
     S390_FEAT_ADAPTER_INT_SUPPRESSION,
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7c253ff308..6b5e94b9f6 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -788,6 +788,7 @@ static void check_consistency(const S390CPUModel *model)
         { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
         { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
         { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
+        { S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, S390_FEAT_AP },
     };
     int i;
 
-- 
2.17.0

[Qemu-devel] [PATCH v1 3/5] s390x/vfio: ap: Definition for AP Adapter type

[Pierre Morel]

From: Pierre Morel <pmorel@linux.vnet.ibm.com>

Let's define the AP adapter type to use it with standard
adapter interface.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 include/hw/s390x/css.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index aae19c4272..9946492214 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -217,6 +217,7 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sub);
 typedef enum {
     CSS_IO_ADAPTER_VIRTIO = 0,
     CSS_IO_ADAPTER_PCI = 1,
+    CSS_IO_ADAPTER_AP = 2,
     CSS_IO_ADAPTER_TYPE_NUMS,
 } CssIoAdapterType;
 
-- 
2.17.0

[Qemu-devel] [PATCH v1 4/5] s390x/vfio: ap: Intercepting AP Queue Interrupt Control

[Pierre Morel]

From: Pierre Morel <pmorel@linux.vnet.ibm.com>

We intercept the PQAP(AQIC) instruction.

Until we implement AQIC we return a PGM_OPERATION.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/vfio/ap.c                 | 10 ++++++++++
 include/hw/s390x/ap-device.h |  9 +++++++++
 target/s390x/kvm.c           | 20 ++++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 3962bb74e5..d8d9cadc46 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -38,6 +38,16 @@ typedef struct VFIOAPDevice {
 #define VFIO_AP_DEVICE(obj) \
         OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
 
+/*
+ * ap_pqap
+ * @env: environment pointing to registers
+ * return value: Code Condition
+ */
+int ap_pqap(CPUS390XState *env)
+{
+    return -PGM_OPERATION;
+}
+
 static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
 {
     vdev->needs_reset = false;
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
index 765e9082a3..a83ea096c7 100644
--- a/include/hw/s390x/ap-device.h
+++ b/include/hw/s390x/ap-device.h
@@ -19,4 +19,13 @@ typedef struct APDevice {
 #define AP_DEVICE(obj) \
     OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
 
+#define AP_DEVICE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
+
+#include "cpu.h"
+int ap_pqap(CPUS390XState *env);
+
 #endif /* HW_S390X_AP_DEVICE_H */
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 2ebf26adfe..3eac59549d 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -45,6 +45,7 @@
 #include "trace.h"
 #include "hw/s390x/s390-pci-inst.h"
 #include "hw/s390x/s390-pci-bus.h"
+#include "hw/s390x/ap-device.h"
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/ebcdic.h"
 #include "exec/memattrs.h"
@@ -88,6 +89,7 @@
 #define PRIV_B2_CHSC                    0x5f
 #define PRIV_B2_SIGA                    0x74
 #define PRIV_B2_XSCH                    0x76
+#define PRIV_B2_PQAP                    0xaf
 
 #define PRIV_EB_SQBS                    0x8a
 #define PRIV_EB_PCISTB                  0xd0
@@ -1154,6 +1156,21 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
     return 0;
 }
 
+static int kvm_ap_pqap(S390CPU *cpu, uint16_t ipbh0)
+{
+    int r;
+
+    r = ap_pqap(&cpu->env);
+
+    if (r < 0) {
+        kvm_s390_program_interrupt(cpu, -r);
+    } else {
+        setcc(cpu, r);
+    }
+
+    return 0;
+}
+
 static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
 {
     CPUS390XState *env = &cpu->env;
@@ -1216,6 +1233,9 @@ static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
     case PRIV_B2_SCLP_CALL:
         rc = kvm_sclp_service_call(cpu, run, ipbh0);
         break;
+    case PRIV_B2_PQAP:
+        rc = kvm_ap_pqap(cpu, ipbh0);
+        break;
     default:
         rc = -1;
         DPRINTF("KVM: unhandled PRIV: 0xb2%x\n", ipa1);
-- 
2.17.0

[Qemu-devel] [PATCH v1 5/5] s390x/vfio: ap: Implementing AP Queue Interrupt Control

[Pierre Morel]

We intercept the PQAP(AQIC) instruction and transform
the guest's AQIC command parameters for the host AQIC
parameters.

Doing this we use the standard adapter interface to provide
the adapter NIB, indicator and ISC.

We define a new structure, APQueue to keep track of
the route and indicator address and we add an array of
AP Queues in the VFIOAPDevice.

We call the VFIO ioctl to set or clear the interruption
according to the "i" bit of the parameter.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/vfio/ap.c                 | 92 +++++++++++++++++++++++++++++++++++-
 include/hw/s390x/ap-device.h | 46 ++++++++++++++++++
 2 files changed, 137 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index d8d9cadc46..67a46e163e 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -27,17 +27,90 @@
 #include "sysemu/sysemu.h"
 #include "hw/s390x/ap-bridge.h"
 #include "exec/address-spaces.h"
+#include "hw/s390x/s390_flic.h"
+#include "hw/s390x/css.h"
 
 #define VFIO_AP_DEVICE_TYPE      "vfio-ap"
 
 typedef struct VFIOAPDevice {
     APDevice apdev;
     VFIODevice vdev;
+    QTAILQ_ENTRY(VFIOAPDevice) sibling;
+    APQueue apq[MAX_AP][MAX_DOMAIN];
 } VFIOAPDevice;
 
 #define VFIO_AP_DEVICE(obj) \
         OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
 
+VFIOAPDevice *vfio_apdev;
+static APDevice *matrix;
+
+static int ap_aqic(CPUS390XState *env)
+{
+    struct pqap_cmd cmd = reg2cmd(env->regs[0]);
+    struct ap_status status = reg2status(env->regs[1]);
+    uint64_t guest_nib = env->regs[2];
+    struct vfio_ap_aqic param = {};
+    int retval;
+    VFIODevice *vdev;
+    VFIOAPDevice *ap_vdev;
+    APQueue *apq;
+
+    ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, matrix);
+    apq = &ap_vdev->apq[cmd.apid][cmd.apqi];
+    vdev = &ap_vdev->vdev;
+
+    if (status.irq) {
+        if (apq->nib) {
+            status.rc = AP_RC_BAD_STATE;
+            goto error;
+        }
+    } else {
+        if (!apq->nib) {
+            status.rc = AP_RC_BAD_STATE;
+            goto error;
+        }
+    }
+    if (!guest_nib) {
+        status.rc = AP_RC_INVALID_ADDR;
+        goto error;
+    }
+
+    apq->routes.adapter.adapter_id = css_get_adapter_id(
+                                       CSS_IO_ADAPTER_AP, status.isc);
+
+    apq->nib = get_indicator(ldq_p(&guest_nib), 8);
+
+    retval = map_indicator(&apq->routes.adapter, apq->nib);
+    if (retval) {
+        status.rc = AP_RC_INVALID_ADDR;
+        env->regs[1] = status2reg(status);
+        goto error;
+    }
+
+    param.cmd = env->regs[0];
+    param.status = env->regs[1];
+    param.nib = env->regs[2];
+    param.adapter_id = apq->routes.adapter.adapter_id;
+    param.argsz = sizeof(param);
+
+    retval = ioctl(vdev->fd, VFIO_AP_SET_IRQ, &param);
+    status = reg2status(param.status);
+    if (retval) {
+        goto err_ioctl;
+    }
+
+    env->regs[1] = param.status;
+
+    return 0;
+err_ioctl:
+    release_indicator(&apq->routes.adapter, apq->nib);
+    apq->nib = NULL;
+error:
+    env->regs[1] = status2reg(status);
+    return 0;
+}
+
 /*
  * ap_pqap
  * @env: environment pointing to registers
@@ -45,7 +118,20 @@ typedef struct VFIOAPDevice {
  */
 int ap_pqap(CPUS390XState *env)
 {
-    return -PGM_OPERATION;
+    struct pqap_cmd cmd = reg2cmd(env->regs[0]);
+    int cc = 0;
+
+    switch (cmd.fc) {
+    case AQIC:
+        if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) {
+            return -PGM_OPERATION;
+        }
+        cc = ap_aqic(env);
+        break;
+    default:
+        return -PGM_OPERATION;
+    }
+    return cc;
 }
 
 static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
@@ -119,6 +205,9 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
         goto out_get_dev_err;
     }
 
+    matrix = apdev;
+    css_register_io_adapters(CSS_IO_ADAPTER_AP, true, false,
+                             0, &error_abort);
     return;
 
 out_get_dev_err:
@@ -135,6 +224,7 @@ static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
     VFIOGroup *group = vapdev->vdev.group;
 
     vfio_ap_put_device(vapdev);
+    matrix = NULL;
     vfio_put_group(group);
 }
 
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
index a83ea096c7..bc2b7bcd8e 100644
--- a/include/hw/s390x/ap-device.h
+++ b/include/hw/s390x/ap-device.h
@@ -28,4 +28,50 @@ typedef struct APDevice {
 #include "cpu.h"
 int ap_pqap(CPUS390XState *env);
 
+#define MAX_AP 256
+#define MAX_DOMAIN 256
+
+#include "hw/s390x/s390_flic.h"
+#include "hw/s390x/css.h"
+typedef struct APQueue {
+    uint32_t apid;
+    uint32_t apqi;
+    AdapterRoutes routes;
+    IndAddr *nib;
+} APQueue;
+
+/* AP PQAP commands definitions */
+#define AQIC 0x03
+
+struct pqap_cmd {
+    uint32_t unused;
+    uint8_t fc;
+    unsigned t:1;
+    unsigned reserved:7;
+    uint8_t apid;
+    uint8_t apqi;
+};
+/* AP status returned by the AP PQAP commands */
+#define AP_RC_APQN_INVALID 0x01
+#define AP_RC_INVALID_ADDR 0x06
+#define AP_RC_BAD_STATE    0x07
+
+struct ap_status {
+    uint16_t pad;
+    unsigned irq:1;
+    unsigned pad2:15;
+    unsigned e:1;
+    unsigned r:1;
+    unsigned f:1;
+    unsigned unused:4;
+    unsigned i:1;
+    unsigned char rc;
+    unsigned reserved:13;
+    unsigned isc:3;
+};
+
+#define reg2cmd(reg) (*(struct pqap_cmd *)&(reg))
+#define status2reg(status) (*((uint64_t *)&status))
+#define reg2status(reg) (*(struct ap_status *)&(reg))
+
 #endif /* HW_S390X_AP_DEVICE_H */
-- 
2.17.0

Re: [Qemu-devel] [PATCH v1 0/5] s390x/vfio: VFIO-AP interrupt control interception

[David Hildenbrand]

On 02.11.18 11:30, Pierre Morel wrote:
> The S390 APQP/AQIC instruction can be intercepted by the host
> to configure the AP queues interruption handling for and handle
> the ISC used by the host and the guest and the indicator address.
> 
> This patch series define the AQIC feature in the cpumodel,
> extend the APDevice type for per queue interrupt handling,
> intercept the APQP/AQIC instruction, uses the S390 adapter interface
> to setup the adapter and use a VFIO ioctl to let the VFIO-AP
> driver handle the host instruction associated with the intercepted
> guest instruction.
> 
> This patch serie can be tested with the Linux/KVM patch series
> for the VFIO-AP driver: "s390: vfio: ap: Using GISA for AP Interrupt"
> 
> Pierre Morel (5):
>   s390x/vfio: ap: Linux uapi VFIO place holder
>   s390x/cpumodel: Set up CPU model for AQIC interception
>   s390x/vfio: ap: Definition for AP Adapter type
>   s390x/vfio: ap: Intercepting AP Queue Interrupt Control
>   s390x/vfio: ap: Implementing AP Queue Interrupt Control
> 
>  hw/vfio/ap.c                    | 100 ++++++++++++++++++++++++++++++++
>  include/hw/s390x/ap-device.h    |  55 ++++++++++++++++++
>  include/hw/s390x/css.h          |   1 +
>  linux-headers/linux/vfio.h      |  22 +++++++
>  target/s390x/cpu_features.c     |   1 +
>  target/s390x/cpu_features_def.h |   1 +
>  target/s390x/cpu_models.c       |   1 +
>  target/s390x/kvm.c              |  20 +++++++
>  8 files changed, 201 insertions(+)
> 

I had a very quick high level look at it and it seems to be in general fine.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 4/5] s390x/vfio: ap: Intercepting AP Queue Interrupt Control

[Cornelia Huck]

On Fri,  2 Nov 2018 11:30:20 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> From: Pierre Morel <pmorel@linux.vnet.ibm.com>

Meta: you may want to rewrite your authorship to the shorter address.

> 
> We intercept the PQAP(AQIC) instruction.
> 
> Until we implement AQIC we return a PGM_OPERATION.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/vfio/ap.c                 | 10 ++++++++++
>  include/hw/s390x/ap-device.h |  9 +++++++++
>  target/s390x/kvm.c           | 20 ++++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 3962bb74e5..d8d9cadc46 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -38,6 +38,16 @@ typedef struct VFIOAPDevice {
>  #define VFIO_AP_DEVICE(obj) \
>          OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
>  
> +/*
> + * ap_pqap
> + * @env: environment pointing to registers
> + * return value: Code Condition
> + */
> +int ap_pqap(CPUS390XState *env)
> +{
> +    return -PGM_OPERATION;
> +}

I'm not sure whether it makes sense to add such a skeleton handler
here; perhaps we should merge with the next patch that actually does
something for AQCI?

> +
>  static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>  {
>      vdev->needs_reset = false;
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> index 765e9082a3..a83ea096c7 100644
> --- a/include/hw/s390x/ap-device.h
> +++ b/include/hw/s390x/ap-device.h
> @@ -19,4 +19,13 @@ typedef struct APDevice {
>  #define AP_DEVICE(obj) \
>      OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
>  
> +#define AP_DEVICE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
> +
> +#define AP_DEVICE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)

Looks like an unrelated change -- does that maybe belong into the next
patch?

> +
> +#include "cpu.h"
> +int ap_pqap(CPUS390XState *env);
> +
>  #endif /* HW_S390X_AP_DEVICE_H */

The wiring up looks reasonable.

Re: [Qemu-devel] [PATCH v1 5/5] s390x/vfio: ap: Implementing AP Queue Interrupt Control

[Cornelia Huck]

On Fri,  2 Nov 2018 11:30:21 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We intercept the PQAP(AQIC) instruction and transform
> the guest's AQIC command parameters for the host AQIC
> parameters.
> 
> Doing this we use the standard adapter interface to provide
> the adapter NIB, indicator and ISC.
> 
> We define a new structure, APQueue to keep track of
> the route and indicator address and we add an array of
> AP Queues in the VFIOAPDevice.
> 
> We call the VFIO ioctl to set or clear the interruption
> according to the "i" bit of the parameter.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/vfio/ap.c                 | 92 +++++++++++++++++++++++++++++++++++-
>  include/hw/s390x/ap-device.h | 46 ++++++++++++++++++
>  2 files changed, 137 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index d8d9cadc46..67a46e163e 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -27,17 +27,90 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/s390x/ap-bridge.h"
>  #include "exec/address-spaces.h"
> +#include "hw/s390x/s390_flic.h"
> +#include "hw/s390x/css.h"
>  
>  #define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>  
>  typedef struct VFIOAPDevice {
>      APDevice apdev;
>      VFIODevice vdev;
> +    QTAILQ_ENTRY(VFIOAPDevice) sibling;
> +    APQueue apq[MAX_AP][MAX_DOMAIN];
>  } VFIOAPDevice;
>  
>  #define VFIO_AP_DEVICE(obj) \
>          OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
>  
> +VFIOAPDevice *vfio_apdev;
> +static APDevice *matrix;

Why do you need those variables? Can't you get the target device in a
different way?

> +
> +static int ap_aqic(CPUS390XState *env)

<bikeshed>I think ap_pqap_aqic would be slightly better.</bikeshed>

> +{
> +    struct pqap_cmd cmd = reg2cmd(env->regs[0]);
> +    struct ap_status status = reg2status(env->regs[1]);

I don't really like those reg2<whatever> helpers; they obfuscate things
IMO.

Also: These are internal structs and not copied from Linux, right? Then
they should be CamelCase.

> +    uint64_t guest_nib = env->regs[2];

Another point: What about endianness? Even though this is kvm-only, I
would like an emulation of an instruction to be endian-clean. (Maybe it
also needs a different split?)

> +    struct vfio_ap_aqic param = {};
> +    int retval;
> +    VFIODevice *vdev;
> +    VFIOAPDevice *ap_vdev;
> +    APQueue *apq;
> +
> +    ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, matrix);
> +    apq = &ap_vdev->apq[cmd.apid][cmd.apqi];
> +    vdev = &ap_vdev->vdev;
> +
> +    if (status.irq) {
> +        if (apq->nib) {
> +            status.rc = AP_RC_BAD_STATE;
> +            goto error;
> +        }
> +    } else {
> +        if (!apq->nib) {
> +            status.rc = AP_RC_BAD_STATE;
> +            goto error;
> +        }
> +    }

Maybe
    if (!!status.irq == !!apq->nib) {
        status.rc = AP_RC_BAD_STATE;
        goto error;
    }
?

> +    if (!guest_nib) {
> +        status.rc = AP_RC_INVALID_ADDR;
> +        goto error;
> +    }
> +
> +    apq->routes.adapter.adapter_id = css_get_adapter_id(
> +                                       CSS_IO_ADAPTER_AP, status.isc);
> +
> +    apq->nib = get_indicator(ldq_p(&guest_nib), 8);
> +
> +    retval = map_indicator(&apq->routes.adapter, apq->nib);
> +    if (retval) {
> +        status.rc = AP_RC_INVALID_ADDR;
> +        env->regs[1] = status2reg(status);

I do not like the backwards conversion macros, either :(

> +        goto error;
> +    }
> +
> +    param.cmd = env->regs[0];
> +    param.status = env->regs[1];
> +    param.nib = env->regs[2];
> +    param.adapter_id = apq->routes.adapter.adapter_id;
> +    param.argsz = sizeof(param);
> +
> +    retval = ioctl(vdev->fd, VFIO_AP_SET_IRQ, &param);
> +    status = reg2status(param.status);
> +    if (retval) {
> +        goto err_ioctl;
> +    }
> +
> +    env->regs[1] = param.status;
> +
> +    return 0;
> +err_ioctl:
> +    release_indicator(&apq->routes.adapter, apq->nib);
> +    apq->nib = NULL;
> +error:
> +    env->regs[1] = status2reg(status);
> +    return 0;
> +}
> +
>  /*
>   * ap_pqap
>   * @env: environment pointing to registers
> @@ -45,7 +118,20 @@ typedef struct VFIOAPDevice {
>   */
>  int ap_pqap(CPUS390XState *env)
>  {
> -    return -PGM_OPERATION;
> +    struct pqap_cmd cmd = reg2cmd(env->regs[0]);

Dito on the macro.

> +    int cc = 0;
> +
> +    switch (cmd.fc) {

This probably needs some endian handling as well.

> +    case AQIC:

What about calling this PQAP_AQCI?

> +        if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) {
> +            return -PGM_OPERATION;
> +        }
> +        cc = ap_aqic(env);
> +        break;
> +    default:
> +        return -PGM_OPERATION;
> +    }
> +    return cc;
>  }
>  
>  static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> @@ -119,6 +205,9 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>          goto out_get_dev_err;
>      }
>  
> +    matrix = apdev;

Huh?

> +    css_register_io_adapters(CSS_IO_ADAPTER_AP, true, false,
> +                             0, &error_abort);
>      return;
>  
>  out_get_dev_err:
> @@ -135,6 +224,7 @@ static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
>      VFIOGroup *group = vapdev->vdev.group;
>  
>      vfio_ap_put_device(vapdev);
> +    matrix = NULL;
>      vfio_put_group(group);
>  }
>  
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> index a83ea096c7..bc2b7bcd8e 100644
> --- a/include/hw/s390x/ap-device.h
> +++ b/include/hw/s390x/ap-device.h
> @@ -28,4 +28,50 @@ typedef struct APDevice {
>  #include "cpu.h"
>  int ap_pqap(CPUS390XState *env);
>  
> +#define MAX_AP 256
> +#define MAX_DOMAIN 256
> +
> +#include "hw/s390x/s390_flic.h"
> +#include "hw/s390x/css.h"
> +typedef struct APQueue {
> +    uint32_t apid;
> +    uint32_t apqi;
> +    AdapterRoutes routes;
> +    IndAddr *nib;
> +} APQueue;
> +
> +/* AP PQAP commands definitions */
> +#define AQIC 0x03
> +
> +struct pqap_cmd {
> +    uint32_t unused;
> +    uint8_t fc;
> +    unsigned t:1;
> +    unsigned reserved:7;

It is better to make this an uint8_t and define an accessor for the 't'
bit.

> +    uint8_t apid;
> +    uint8_t apqi;
> +};

Also, please use typedef and CamelCase :)

> +/* AP status returned by the AP PQAP commands */
> +#define AP_RC_APQN_INVALID 0x01
> +#define AP_RC_INVALID_ADDR 0x06
> +#define AP_RC_BAD_STATE    0x07
> +
> +struct ap_status {
> +    uint16_t pad;
> +    unsigned irq:1;
> +    unsigned pad2:15;
> +    unsigned e:1;
> +    unsigned r:1;
> +    unsigned f:1;
> +    unsigned unused:4;
> +    unsigned i:1;
> +    unsigned char rc;
> +    unsigned reserved:13;
> +    unsigned isc:3;
> +};

Dito on bitfields and CamelCase :)

> +
> +#define reg2cmd(reg) (*(struct pqap_cmd *)&(reg))
> +#define status2reg(status) (*((uint64_t *)&status))
> +#define reg2status(reg) (*(struct ap_status *)&(reg))
> +
>  #endif /* HW_S390X_AP_DEVICE_H */

Re: [Qemu-devel] [PATCH v1 4/5] s390x/vfio: ap: Intercepting AP Queue Interrupt Control

[Pierre Morel]

On 07/11/2018 13:40, Cornelia Huck wrote:
> On Fri,  2 Nov 2018 11:30:20 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> From: Pierre Morel <pmorel@linux.vnet.ibm.com>
> 
> Meta: you may want to rewrite your authorship to the shorter address.

right thanks (this patch is in my queue since too long)

> 
>>
>> We intercept the PQAP(AQIC) instruction.
>>
>> Until we implement AQIC we return a PGM_OPERATION.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/vfio/ap.c                 | 10 ++++++++++
>>   include/hw/s390x/ap-device.h |  9 +++++++++
>>   target/s390x/kvm.c           | 20 ++++++++++++++++++++
>>   3 files changed, 39 insertions(+)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 3962bb74e5..d8d9cadc46 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -38,6 +38,16 @@ typedef struct VFIOAPDevice {
>>   #define VFIO_AP_DEVICE(obj) \
>>           OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
>>   
>> +/*
>> + * ap_pqap
>> + * @env: environment pointing to registers
>> + * return value: Code Condition
>> + */
>> +int ap_pqap(CPUS390XState *env)
>> +{
>> +    return -PGM_OPERATION;
>> +}
> 
> I'm not sure whether it makes sense to add such a skeleton handler
> here; perhaps we should merge with the next patch that actually does
> something for AQCI?

OK

> 
>> +
>>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>   {
>>       vdev->needs_reset = false;
>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
>> index 765e9082a3..a83ea096c7 100644
>> --- a/include/hw/s390x/ap-device.h
>> +++ b/include/hw/s390x/ap-device.h
>> @@ -19,4 +19,13 @@ typedef struct APDevice {
>>   #define AP_DEVICE(obj) \
>>       OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
>>   
>> +#define AP_DEVICE_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
>> +
>> +#define AP_DEVICE_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
> 
> Looks like an unrelated change -- does that maybe belong into the next
> patch?

Seems indeed.

> 
>> +
>> +#include "cpu.h"
>> +int ap_pqap(CPUS390XState *env);
>> +
>>   #endif /* HW_S390X_AP_DEVICE_H */
> 
> The wiring up looks reasonable.
> 

Thanks,
Pierre

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

Re: [Qemu-devel] [PATCH v1 5/5] s390x/vfio: ap: Implementing AP Queue Interrupt Control

[Pierre Morel]

On 07/11/2018 14:39, Cornelia Huck wrote:
> On Fri,  2 Nov 2018 11:30:21 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We intercept the PQAP(AQIC) instruction and transform
>> the guest's AQIC command parameters for the host AQIC
>> parameters.
>>
>> Doing this we use the standard adapter interface to provide
>> the adapter NIB, indicator and ISC.
>>
>> We define a new structure, APQueue to keep track of
>> the route and indicator address and we add an array of
>> AP Queues in the VFIOAPDevice.
>>
>> We call the VFIO ioctl to set or clear the interruption
>> according to the "i" bit of the parameter.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/vfio/ap.c                 | 92 +++++++++++++++++++++++++++++++++++-
>>   include/hw/s390x/ap-device.h | 46 ++++++++++++++++++
>>   2 files changed, 137 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index d8d9cadc46..67a46e163e 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -27,17 +27,90 @@
>>   #include "sysemu/sysemu.h"
>>   #include "hw/s390x/ap-bridge.h"
>>   #include "exec/address-spaces.h"
>> +#include "hw/s390x/s390_flic.h"
>> +#include "hw/s390x/css.h"
>>   
>>   #define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>>   
>>   typedef struct VFIOAPDevice {
>>       APDevice apdev;
>>       VFIODevice vdev;
>> +    QTAILQ_ENTRY(VFIOAPDevice) sibling;
>> +    APQueue apq[MAX_AP][MAX_DOMAIN];
>>   } VFIOAPDevice;
>>   
>>   #define VFIO_AP_DEVICE(obj) \
>>           OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
>>   
>> +VFIOAPDevice *vfio_apdev;
>> +static APDevice *matrix;
> 
> Why do you need those variables? Can't you get the target device in a
> different way?
> 
>> +
>> +static int ap_aqic(CPUS390XState *env)
> 
> <bikeshed>I think ap_pqap_aqic would be slightly better.</bikeshed>

agreed.

> 
>> +{
>> +    struct pqap_cmd cmd = reg2cmd(env->regs[0]);
>> +    struct ap_status status = reg2status(env->regs[1]);
> 
> I don't really like those reg2<whatever> helpers; they obfuscate things
> IMO.
> 

I agree, I will use masks and uint64_t/uint32_t.


> Also: These are internal structs and not copied from Linux, right? Then
> they should be CamelCase.
> 
>> +    uint64_t guest_nib = env->regs[2];
> 
> Another point: What about endianness? Even though this is kvm-only, I
> would like an emulation of an instruction to be endian-clean. (Maybe it
> also needs a different split?)

OK, I will look at this

> 
>> +    struct vfio_ap_aqic param = {};
>> +    int retval;
>> +    VFIODevice *vdev;
>> +    VFIOAPDevice *ap_vdev;
>> +    APQueue *apq;
>> +
>> +    ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, matrix);
>> +    apq = &ap_vdev->apq[cmd.apid][cmd.apqi];
>> +    vdev = &ap_vdev->vdev;
>> +
>> +    if (status.irq) {
>> +        if (apq->nib) {
>> +            status.rc = AP_RC_BAD_STATE;
>> +            goto error;
>> +        }
>> +    } else {
>> +        if (!apq->nib) {
>> +            status.rc = AP_RC_BAD_STATE;
>> +            goto error;
>> +        }
>> +    }
> 
> Maybe
>      if (!!status.irq == !!apq->nib) {
>          status.rc = AP_RC_BAD_STATE;
>          goto error;
>      }
> ?

:)

> 
>> +    if (!guest_nib) {
>> +        status.rc = AP_RC_INVALID_ADDR;
>> +        goto error;
>> +    }
>> +
>> +    apq->routes.adapter.adapter_id = css_get_adapter_id(
>> +                                       CSS_IO_ADAPTER_AP, status.isc);
>> +
>> +    apq->nib = get_indicator(ldq_p(&guest_nib), 8);
>> +
>> +    retval = map_indicator(&apq->routes.adapter, apq->nib);
>> +    if (retval) {
>> +        status.rc = AP_RC_INVALID_ADDR;
>> +        env->regs[1] = status2reg(status);
> 
> I do not like the backwards conversion macros, either :(
> 
>> +        goto error;
>> +    }
>> +
>> +    param.cmd = env->regs[0];
>> +    param.status = env->regs[1];
>> +    param.nib = env->regs[2];
>> +    param.adapter_id = apq->routes.adapter.adapter_id;
>> +    param.argsz = sizeof(param);
>> +
>> +    retval = ioctl(vdev->fd, VFIO_AP_SET_IRQ, &param);
>> +    status = reg2status(param.status);
>> +    if (retval) {
>> +        goto err_ioctl;
>> +    }
>> +
>> +    env->regs[1] = param.status;
>> +
>> +    return 0;
>> +err_ioctl:
>> +    release_indicator(&apq->routes.adapter, apq->nib);
>> +    apq->nib = NULL;
>> +error:
>> +    env->regs[1] = status2reg(status);
>> +    return 0;
>> +}
>> +
>>   /*
>>    * ap_pqap
>>    * @env: environment pointing to registers
>> @@ -45,7 +118,20 @@ typedef struct VFIOAPDevice {
>>    */
>>   int ap_pqap(CPUS390XState *env)
>>   {
>> -    return -PGM_OPERATION;
>> +    struct pqap_cmd cmd = reg2cmd(env->regs[0]);
> 
> Dito on the macro.
> 
>> +    int cc = 0;
>> +
>> +    switch (cmd.fc) {
> 
> This probably needs some endian handling as well.
> 
>> +    case AQIC:
> 
> What about calling this PQAP_AQCI?

I can use it , however the function itself is called ap_pqap()
so we already know it is about PQAP...


> 
>> +        if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) {
>> +            return -PGM_OPERATION;
>> +        }
>> +        cc = ap_aqic(env);
>> +        break;
>> +    default:
>> +        return -PGM_OPERATION;
>> +    }
>> +    return cc;
>>   }
>>   
>>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>> @@ -119,6 +205,9 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>           goto out_get_dev_err;
>>       }
>>   
>> +    matrix = apdev;
> 
> Huh?

OK, sorry, I can do better. (I hope)

> 
>> +    css_register_io_adapters(CSS_IO_ADAPTER_AP, true, false,
>> +                             0, &error_abort);
>>       return;
>>   
>>   out_get_dev_err:
>> @@ -135,6 +224,7 @@ static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
>>       VFIOGroup *group = vapdev->vdev.group;
>>   
>>       vfio_ap_put_device(vapdev);
>> +    matrix = NULL;
>>       vfio_put_group(group);
>>   }
>>   
>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
>> index a83ea096c7..bc2b7bcd8e 100644
>> --- a/include/hw/s390x/ap-device.h
>> +++ b/include/hw/s390x/ap-device.h
>> @@ -28,4 +28,50 @@ typedef struct APDevice {
>>   #include "cpu.h"
>>   int ap_pqap(CPUS390XState *env);
>>   
>> +#define MAX_AP 256
>> +#define MAX_DOMAIN 256
>> +
>> +#include "hw/s390x/s390_flic.h"
>> +#include "hw/s390x/css.h"
>> +typedef struct APQueue {
>> +    uint32_t apid;
>> +    uint32_t apqi;
>> +    AdapterRoutes routes;
>> +    IndAddr *nib;
>> +} APQueue;
>> +
>> +/* AP PQAP commands definitions */
>> +#define AQIC 0x03
>> +
>> +struct pqap_cmd {
>> +    uint32_t unused;
>> +    uint8_t fc;
>> +    unsigned t:1;
>> +    unsigned reserved:7;
> 
> It is better to make this an uint8_t and define an accessor for the 't'
> bit.

OK

> 
>> +    uint8_t apid;
>> +    uint8_t apqi;
>> +};
> 
> Also, please use typedef and CamelCase :)

right, more camels

> 
>> +/* AP status returned by the AP PQAP commands */
>> +#define AP_RC_APQN_INVALID 0x01
>> +#define AP_RC_INVALID_ADDR 0x06
>> +#define AP_RC_BAD_STATE    0x07
>> +
>> +struct ap_status {
>> +    uint16_t pad;
>> +    unsigned irq:1;
>> +    unsigned pad2:15;
>> +    unsigned e:1;
>> +    unsigned r:1;
>> +    unsigned f:1;
>> +    unsigned unused:4;
>> +    unsigned i:1;
>> +    unsigned char rc;
>> +    unsigned reserved:13;
>> +    unsigned isc:3;
>> +};
> 
> Dito on bitfields and CamelCase :)

OK again, even more Camels

Thanks,
Pierre

> 
>> +
>> +#define reg2cmd(reg) (*(struct pqap_cmd *)&(reg))
>> +#define status2reg(status) (*((uint64_t *)&status))
>> +#define reg2status(reg) (*(struct ap_status *)&(reg))
>> +
>>   #endif /* HW_S390X_AP_DEVICE_H */
> 


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