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

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

[Pierre Morel]

This series has 3 different type of patches:

The first two:
  s390x/vfio: ap: Finding the AP bridge
  s390x/vfio: ap: Use the APdevice as a child of the APBus

Are dealing with the QEMU Object Model and how we retrieve the
AP devices from instruction interception.
A lifting of the AP bridge, bus and device was necessary to
ease the process and allow future extensions.

The third one is a place holder to ease reviewing:
  s390x/vfio: ap: Linux uapi VFIO place holder

The last three are really dealing with PQAP/AQIC instruction
interception and associate IOCTL calls to the VFIO AP device
driver.
  s390x/cpumodel: Set up CPU model for AQIC interception
  s390x/vfio: ap: Definition for AP Adapter type
  s390x/vfio: ap: Implementing AP Queue Interrupt Control

The S390 APQP/AQIC instruction is intercepted by the host
to configure the AP queues interruption redirection.
It retrieves the ISC used by the host and the one used
by the guest and setup the indicator address.

This patch series
- define the AQIC feature in the cpumodel,
- extend the APDevice type for per card and 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 (6):
  s390x/vfio: ap: Finding the AP bridge
  s390x/vfio: ap: Use the APdevice as a child of the APBus
  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: Implementing AP Queue Interrupt Control

 hw/s390x/ap-bridge.c            |  12 ++++
 hw/s390x/ap-device.c            |  22 +++++++
 hw/vfio/ap.c                    | 131 +++++++++++++++++++++++++++++++++++++---
 include/hw/s390x/ap-bridge.h    |   1 +
 include/hw/s390x/ap-device.h    |  65 ++++++++++++++++++++
 include/hw/s390x/css.h          |   1 +
 linux-headers/linux/vfio.h      |  25 ++++++++
 target/s390x/cpu_features.c     |   1 +
 target/s390x/cpu_features_def.h |   1 +
 target/s390x/cpu_models.c       |   1 +
 target/s390x/gen-features.c     |   1 +
 target/s390x/kvm.c              |  31 ++++++++++
 12 files changed, 282 insertions(+), 10 deletions(-)

-- 
2.7.4

Changelog from v1:
- legitimate "Huh" from Conny on global lead me to reconsider
  the QOM for AP Device, BUS and Bridge and remove globals
- squash patches for kvm.c and hw/vfio/ap.c
  (Conny) so related changes are easier to see
- modified the UAPI (Also comes from Linux)
- suppressed complicated structures to integer conversions
  and use masks to avoid endianess considerations,
  I did not change anything for endianess since we get all
  from the registers, AFAIU we do not need to.
  Tell me if I am wrong (I know you will :) )
- simplified the implementation of the ioctl

Tested with according Linux patch.
Take care that the UAPI changed betwen V1 and V2!
and do NOT use with the older version.

[Qemu-devel] [PATCH v2 1/6] s390x/vfio: ap: Finding the AP bridge

[Pierre Morel]

In the case we will enter QEMU through interception of instructions,
we will need to retrieve the AP devices.
The base device is the AP bridge.

Let us implement a way to retrieve the AP Bridge from qtree.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/ap-bridge.c         | 12 ++++++++++++
 include/hw/s390x/ap-bridge.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
index 3795d30..831ad5d 100644
--- a/hw/s390x/ap-bridge.c
+++ b/hw/s390x/ap-bridge.c
@@ -14,6 +14,18 @@
 #include "hw/s390x/ap-bridge.h"
 #include "cpu.h"
 
+DeviceState *s390_get_ap_bridge(void)
+{
+    static DeviceState *apb;
+
+    if (!apb) {
+        apb = DEVICE(object_resolve_path(TYPE_AP_BRIDGE, NULL));
+        assert(apb != NULL);
+    }
+
+    return apb;
+}
+
 static char *ap_bus_get_dev_path(DeviceState *dev)
 {
     /* at most one */
diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
index 470e439..dacb877 100644
--- a/include/hw/s390x/ap-bridge.h
+++ b/include/hw/s390x/ap-bridge.h
@@ -15,5 +15,6 @@
 #define TYPE_AP_BUS "ap-bus"
 
 void s390_init_ap(void);
+DeviceState *s390_get_ap_bridge(void);
 
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Pierre Morel]

Two good reasons to use the base device as a child of the
AP BUS:
- We can easily find the device without traversing the qtree.
- In case we have different APdevice instantiation, VFIO with
  interception or emulation, we will need the APDevice as
  a parent device.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
 hw/vfio/ap.c                 | 16 ++++++----------
 include/hw/s390x/ap-device.h |  2 ++
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
index f5ac8db..554d5aa 100644
--- a/hw/s390x/ap-device.c
+++ b/hw/s390x/ap-device.c
@@ -11,13 +11,35 @@
 #include "qemu/module.h"
 #include "qapi/error.h"
 #include "hw/qdev.h"
+#include "hw/s390x/ap-bridge.h"
 #include "hw/s390x/ap-device.h"
 
+APDevice *s390_get_ap(void)
+{
+    static DeviceState *apb;
+    BusState *bus;
+    BusChild *child;
+    static APDevice *ap;
+
+    if (ap) {
+        return ap;
+    }
+
+    apb = s390_get_ap_bridge();
+    /* We have only a single child on the BUS */
+    bus = qdev_get_child_bus(apb, TYPE_AP_BUS);
+    child = QTAILQ_FIRST(&bus->children);
+    assert(child != NULL);
+    ap = DO_UPCAST(APDevice, parent_obj, child->child);
+    return ap;
+}
+
 static void ap_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->desc = "AP device class";
+    dc->bus_type = TYPE_AP_BUS;
     dc->hotpluggable = false;
 }
 
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 65de952..94e5a1a 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {
     VFIODevice vdev;
 } VFIOAPDevice;
 
-#define VFIO_AP_DEVICE(obj) \
-        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
-
 static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
 {
     vdev->needs_reset = false;
@@ -90,8 +87,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
     char *mdevid;
     Error *local_err = NULL;
     VFIOGroup *vfio_group;
-    APDevice *apdev = AP_DEVICE(dev);
-    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
+    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
+    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
 
     vfio_group = vfio_ap_get_group(vapdev, &local_err);
     if (!vfio_group) {
@@ -120,8 +117,8 @@ out_err:
 
 static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
 {
-    APDevice *apdev = AP_DEVICE(dev);
-    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
+    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
+    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
     VFIOGroup *group = vapdev->vdev.group;
 
     vfio_ap_put_device(vapdev);
@@ -136,8 +133,8 @@ static Property vfio_ap_properties[] = {
 static void vfio_ap_reset(DeviceState *dev)
 {
     int ret;
-    APDevice *apdev = AP_DEVICE(dev);
-    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
+    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
+    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
 
     ret = ioctl(vapdev->vdev.fd, VFIO_DEVICE_RESET);
     if (ret) {
@@ -163,7 +160,6 @@ static void vfio_ap_class_init(ObjectClass *klass, void *data)
     dc->unrealize = vfio_ap_unrealize;
     dc->hotpluggable = false;
     dc->reset = vfio_ap_reset;
-    dc->bus_type = TYPE_AP_BUS;
 }
 
 static const TypeInfo vfio_ap_info = {
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
index 765e908..5f3c840 100644
--- a/include/hw/s390x/ap-device.h
+++ b/include/hw/s390x/ap-device.h
@@ -19,4 +19,6 @@ typedef struct APDevice {
 #define AP_DEVICE(obj) \
     OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
 
+APDevice *s390_get_ap(void);
+
 #endif /* HW_S390X_AP_DEVICE_H */
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/6] 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 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index ceb6453..d5d10bc9 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -816,6 +816,31 @@ 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;
+	/* out */
+        __u32 status;
+	/* in */
+        __u32 adapter_id;
+        __u64 nib;
+        __u16 apqn;
+        __u8 isc;
+        __u8 reserved[5];
+};
+#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.7.4

[Qemu-devel] [PATCH v2 4/6] 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 +
 target/s390x/gen-features.c     | 1 +
 4 files changed, 4 insertions(+)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 60cfeba..c464abf 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 5fc7e7b..3f22780 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 7c253ff..6b5e94b 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;
 
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 70015ea..b051221 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -448,6 +448,7 @@ static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_EDAT_2,
     S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
     S390_FEAT_AP_QUERY_CONFIG_INFO,
+    S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL,
     S390_FEAT_AP_FACILITIES_TEST,
     S390_FEAT_AP,
 };
-- 
2.7.4

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

[Pierre Morel]

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

Signed-off-by: Pierre Morel <pmorel@linux.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 aae19c4..9946492 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.7.4

[Qemu-devel] [PATCH v2 6/6] 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 new structures, APCard and APQueue to keep track of
the ISC, route and indicator address and we add an array of
APCards in the APDevice.

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                 | 117 ++++++++++++++++++++++++++++++++++++++++++-
 include/hw/s390x/ap-device.h |  63 +++++++++++++++++++++++
 target/s390x/kvm.c           |  31 ++++++++++++
 3 files changed, 210 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 94e5a1a..20d2e5f 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -4,6 +4,7 @@
  * Copyright 2018 IBM Corp.
  * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
  *            Halil Pasic <pasic@linux.ibm.com>
+ *            Pierre Morel <pmorel@linux.ibm.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or (at
  * your option) any later version. See the COPYING file in the top-level
@@ -14,7 +15,6 @@
 #include <sys/ioctl.h>
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "hw/sysbus.h"
 #include "hw/vfio/vfio.h"
 #include "hw/vfio/vfio-common.h"
 #include "hw/s390x/ap-device.h"
@@ -27,6 +27,8 @@
 #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"
 
@@ -35,6 +37,117 @@ typedef struct VFIOAPDevice {
     VFIODevice vdev;
 } VFIOAPDevice;
 
+static uint32_t ap_pqap_clear_irq(VFIODevice *vdev, APQueue *apq)
+{
+    struct vfio_ap_aqic param;
+    uint32_t retval;
+
+    param.apqn = apq->apqn;
+    param.isc = apq->isc;
+    param.argsz = sizeof(param);
+
+    retval = ioctl(vdev->fd, VFIO_AP_CLEAR_IRQ, &param);
+    switch (retval) {
+    case 0:    /* Fall through and return the instruction status */
+        release_indicator(&apq->routes.adapter, apq->nib);
+    case -EIO: /* The case the PQAP instruction failed with status */
+        return  param.status;
+    default:   /* The case the ioctl call failed without isuing instruction */
+        break;
+    }
+    return ap_reg_set_status(AP_RC_INVALID_ADDR);
+}
+
+static uint32_t ap_pqap_set_irq(VFIODevice *vdev, APQueue *apq, uint64_t g_nib)
+{
+    struct vfio_ap_aqic param;
+    uint32_t retval;
+    uint32_t id;
+
+    id = css_get_adapter_id(CSS_IO_ADAPTER_AP, apq->isc);
+    if (id == -1) {
+        return ap_reg_set_status(AP_RC_INVALID_ADDR);
+    }
+    apq->routes.adapter.adapter_id = id;
+    apq->nib = get_indicator(ldq_p(&g_nib), 8);
+
+    retval = map_indicator(&apq->routes.adapter, apq->nib);
+    if (retval) {
+        return ap_reg_set_status(AP_RC_INVALID_ADDR);
+    }
+
+    param.apqn = apq->apqn;
+    param.isc = apq->isc;
+    param.nib = g_nib;
+    param.adapter_id = id;
+    param.argsz = sizeof(param);
+
+    retval =  ioctl(vdev->fd, VFIO_AP_SET_IRQ, &param);
+    switch (retval) {
+    case -EIO: /* The case the PQAP instruction failed with status */
+        release_indicator(&apq->routes.adapter, apq->nib);
+    case 0:    /* Fall through and return the instruction status */
+        return  param.status;
+    default:   /* The case the ioctl call failed without isuing instruction */
+        break;
+    }
+    release_indicator(&apq->routes.adapter, apq->nib);
+    return ap_reg_set_status(AP_RC_INVALID_ADDR);
+}
+
+static int ap_pqap_aqic(CPUS390XState *env)
+{
+    uint64_t g_nib = env->regs[2];
+    uint8_t apid = ap_reg_get_apid(env->regs[0]);
+    uint8_t apqi = ap_reg_get_apqi(env->regs[0]);
+    uint32_t retval;
+    APDevice *ap = s390_get_ap();
+    VFIODevice *vdev;
+    VFIOAPDevice *ap_vdev;
+    APQueue *apq;
+
+    ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, ap);
+    vdev = &ap_vdev->vdev;
+    apq = &ap->card[apid].queue[apqi];
+    apq->isc = ap_reg_get_isc(env->regs[1]);
+    apq->apqn = (apid << 8) | apqi;
+
+    if (ap_reg_get_ir(env->regs[1])) {
+        retval = ap_pqap_set_irq(vdev, apq, g_nib);
+    } else {
+        retval = ap_pqap_clear_irq(vdev, apq);
+    }
+
+    env->regs[1] = retval;
+    if (retval & AP_STATUS_RC_MASK) {
+        return 3;
+    }
+
+    return 0;
+}
+
+/*
+ * ap_pqap
+ * @env: environment pointing to registers
+ * return value: Code Condition
+ */
+int ap_pqap(CPUS390XState *env)
+{
+    int cc = 0;
+
+    switch (ap_reg_get_fc(env->regs[0])) {
+    case AQIC:
+        if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) {
+            return -PGM_OPERATION;
+        }
+        cc = ap_pqap_aqic(env);
+        break;
+    default:
+        return -PGM_OPERATION;
+    }
+    return cc;
+}
+
 static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
 {
     vdev->needs_reset = false;
@@ -106,6 +219,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
         goto out_get_dev_err;
     }
 
+    css_register_io_adapters(CSS_IO_ADAPTER_AP, true, false,
+                             0, &error_abort);
     return;
 
 out_get_dev_err:
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
index 5f3c840..8a36780 100644
--- a/include/hw/s390x/ap-device.h
+++ b/include/hw/s390x/ap-device.h
@@ -12,8 +12,25 @@
 
 #define AP_DEVICE_TYPE       "ap-device"
 
+#define MAX_AP_CARD 256
+#define MAX_AP_DOMAIN 256
+
+#include "hw/s390x/s390_flic.h"
+#include "hw/s390x/css.h"
+typedef struct APQueue {
+    AdapterRoutes routes;
+    IndAddr *nib;
+    uint16_t apqn;
+    uint8_t isc;
+} APQueue;
+
+typedef struct APCard {
+    APQueue queue[MAX_AP_DOMAIN];
+} APCard;
+
 typedef struct APDevice {
     DeviceState parent_obj;
+    APCard card[MAX_AP_CARD];
 } APDevice;
 
 #define AP_DEVICE(obj) \
@@ -21,4 +38,50 @@ typedef struct APDevice {
 
 APDevice *s390_get_ap(void);
 
+#include "cpu.h"
+int ap_pqap(CPUS390XState *env);
+
+/* AP PQAP commands definitions */
+#define AQIC 0x03
+
+#define AP_AQIC_ZERO_BITS 0x0ff0000
+
+/* Register 0 hold the command and APQN */
+static inline uint8_t ap_reg_get_apid(uint64_t r)
+{
+        return (r >> 8) & 0xff;
+}
+
+static inline uint8_t ap_reg_get_apqi(uint64_t r)
+{
+        return r & 0xff;
+}
+
+static inline uint16_t ap_reg_get_fc(uint64_t r)
+{
+        return (r >> 24) & 0xff;
+}
+
+static inline uint16_t ap_reg_get_ir(uint64_t r)
+{
+        return (r >> 47) & 0x01;
+}
+
+static inline uint16_t ap_reg_get_isc(uint64_t r)
+{
+        return r  & 0x7;
+}
+
+/* AP status returned by the AP PQAP commands */
+#define AP_STATUS_RC_MASK 0x00ff0000
+#define AP_RC_APQN_INVALID 0x01
+#define AP_RC_INVALID_ADDR 0x06
+#define AP_RC_BAD_STATE    0x07
+
+/* Register 1 as input hold the AQIC information */
+static inline uint32_t ap_reg_set_status(uint8_t status)
+{
+        return status << 16;
+}
+
 #endif /* HW_S390X_AP_DEVICE_H */
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 2ebf26a..a4b5d90 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,32 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
     return 0;
 }
 
+static int kvm_ap_pqap(S390CPU *cpu, uint16_t ipbh0)
+{
+    CPUS390XState *env = &cpu->env;
+    int r;
+
+    if (env->psw.mask & PSW_MASK_PSTATE) {
+        kvm_s390_program_interrupt(cpu, PGM_PRIVILEGED);
+        return 0;
+    }
+
+    if (env->regs[0] & AP_AQIC_ZERO_BITS) {
+        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
+        return 0;
+    }
+
+    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 +1244,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.7.4

Re: [Qemu-devel] [PATCH v2 1/6] s390x/vfio: ap: Finding the AP bridge

[Cornelia Huck]

On Thu, 22 Nov 2018 17:35:50 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> In the case we will enter QEMU through interception of instructions,
> we will need to retrieve the AP devices.
> The base device is the AP bridge.
> 
> Let us implement a way to retrieve the AP Bridge from qtree.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/ap-bridge.c         | 12 ++++++++++++
>  include/hw/s390x/ap-bridge.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
> index 3795d30..831ad5d 100644
> --- a/hw/s390x/ap-bridge.c
> +++ b/hw/s390x/ap-bridge.c
> @@ -14,6 +14,18 @@
>  #include "hw/s390x/ap-bridge.h"
>  #include "cpu.h"
>  
> +DeviceState *s390_get_ap_bridge(void)
> +{
> +    static DeviceState *apb;
> +
> +    if (!apb) {
> +        apb = DEVICE(object_resolve_path(TYPE_AP_BRIDGE, NULL));
> +        assert(apb != NULL);

As you won't have an ap bridge if the ap feature is not provided,
better do a quick exit if the feature bit is not set? I'd naively
assume that this function can return NULL as well.

> +    }
> +
> +    return apb;
> +}
> +
>  static char *ap_bus_get_dev_path(DeviceState *dev)
>  {
>      /* at most one */

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Cornelia Huck]

On Thu, 22 Nov 2018 17:35:51 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Two good reasons to use the base device as a child of the
> AP BUS:
> - We can easily find the device without traversing the qtree.
> - In case we have different APdevice instantiation, VFIO with
>   interception or emulation, we will need the APDevice as
>   a parent device.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>  hw/vfio/ap.c                 | 16 ++++++----------
>  include/hw/s390x/ap-device.h |  2 ++
>  3 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
> index f5ac8db..554d5aa 100644
> --- a/hw/s390x/ap-device.c
> +++ b/hw/s390x/ap-device.c
> @@ -11,13 +11,35 @@
>  #include "qemu/module.h"
>  #include "qapi/error.h"
>  #include "hw/qdev.h"
> +#include "hw/s390x/ap-bridge.h"
>  #include "hw/s390x/ap-device.h"
>  
> +APDevice *s390_get_ap(void)
> +{
> +    static DeviceState *apb;
> +    BusState *bus;
> +    BusChild *child;
> +    static APDevice *ap;
> +
> +    if (ap) {
> +        return ap;
> +    }
> +
> +    apb = s390_get_ap_bridge();
> +    /* We have only a single child on the BUS */

So, there'll never a mixed environment? Or would that have a 'hybrid'
ap device?

> +    bus = qdev_get_child_bus(apb, TYPE_AP_BUS);
> +    child = QTAILQ_FIRST(&bus->children);
> +    assert(child != NULL);
> +    ap = DO_UPCAST(APDevice, parent_obj, child->child);
> +    return ap;
> +}
> +
>  static void ap_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->desc = "AP device class";
> +    dc->bus_type = TYPE_AP_BUS;
>      dc->hotpluggable = false;
>  }
>  
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952..94e5a1a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {
>      VFIODevice vdev;
>  } VFIOAPDevice;
>  
> -#define VFIO_AP_DEVICE(obj) \
> -        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)

Hm?

> -
>  static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>  {
>      vdev->needs_reset = false;

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Pierre Morel]

On 29/11/2018 12:45, Cornelia Huck wrote:
> On Thu, 22 Nov 2018 17:35:51 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Two good reasons to use the base device as a child of the
>> AP BUS:
>> - We can easily find the device without traversing the qtree.
>> - In case we have different APdevice instantiation, VFIO with
>>    interception or emulation, we will need the APDevice as
>>    a parent device.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>>   hw/vfio/ap.c                 | 16 ++++++----------
>>   include/hw/s390x/ap-device.h |  2 ++
>>   3 files changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
>> index f5ac8db..554d5aa 100644
>> --- a/hw/s390x/ap-device.c
>> +++ b/hw/s390x/ap-device.c
>> @@ -11,13 +11,35 @@
>>   #include "qemu/module.h"
>>   #include "qapi/error.h"
>>   #include "hw/qdev.h"
>> +#include "hw/s390x/ap-bridge.h"
>>   #include "hw/s390x/ap-device.h"
>>   
>> +APDevice *s390_get_ap(void)
>> +{
>> +    static DeviceState *apb;
>> +    BusState *bus;
>> +    BusChild *child;
>> +    static APDevice *ap;
>> +
>> +    if (ap) {
>> +        return ap;
>> +    }
>> +
>> +    apb = s390_get_ap_bridge();
>> +    /* We have only a single child on the BUS */
> 
> So, there'll never a mixed environment? Or would that have a 'hybrid'
> ap device?

Not for now, we only support VFIOAPDevice and we can only have one 
single VFIO device per guest.

> 
>> +    bus = qdev_get_child_bus(apb, TYPE_AP_BUS);
>> +    child = QTAILQ_FIRST(&bus->children);
>> +    assert(child != NULL);
>> +    ap = DO_UPCAST(APDevice, parent_obj, child->child);
>> +    return ap;
>> +}
>> +
>>   static void ap_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>   
>>       dc->desc = "AP device class";
>> +    dc->bus_type = TYPE_AP_BUS;
>>       dc->hotpluggable = false;
>>   }
>>   
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 65de952..94e5a1a 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {
>>       VFIODevice vdev;
>>   } VFIOAPDevice;
>>   
>> -#define VFIO_AP_DEVICE(obj) \
>> -        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
> 
> Hm?

We do not need this anymore.
We used to plug directly the VFIOAPDevice on the BUS but the APDevice is 
now plugged on the APBUS.

The VFIOAPDevice is plugged on the APBus through its APDevice.

This will allow to have different APDevice childs using APDevice features.
Usable for interception or emulation.


> 
>> -
>>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>   {
>>       vdev->needs_reset = false;
> 


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

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Tony Krowiak]

On 11/29/18 6:45 AM, Cornelia Huck wrote:
> On Thu, 22 Nov 2018 17:35:51 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Two good reasons to use the base device as a child of the
>> AP BUS:
>> - We can easily find the device without traversing the qtree.
>> - In case we have different APdevice instantiation, VFIO with
>>    interception or emulation, we will need the APDevice as
>>    a parent device.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>>   hw/vfio/ap.c                 | 16 ++++++----------
>>   include/hw/s390x/ap-device.h |  2 ++
>>   3 files changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
>> index f5ac8db..554d5aa 100644
>> --- a/hw/s390x/ap-device.c
>> +++ b/hw/s390x/ap-device.c
>> @@ -11,13 +11,35 @@
>>   #include "qemu/module.h"
>>   #include "qapi/error.h"
>>   #include "hw/qdev.h"
>> +#include "hw/s390x/ap-bridge.h"
>>   #include "hw/s390x/ap-device.h"
>>   
>> +APDevice *s390_get_ap(void)
>> +{
>> +    static DeviceState *apb;
>> +    BusState *bus;
>> +    BusChild *child;
>> +    static APDevice *ap;
>> +
>> +    if (ap) {
>> +        return ap;
>> +    }
>> +
>> +    apb = s390_get_ap_bridge();
>> +    /* We have only a single child on the BUS */
> 
> So, there'll never a mixed environment? Or would that have a 'hybrid'
> ap device?

It is not possible to have interpretation and interception. I suppose
one could mix emulated and virtual AP devices, but that seems highly
unlikely; in fact, I think it is highly unlikely that emulation is ever
implemented.

> 
>> +    bus = qdev_get_child_bus(apb, TYPE_AP_BUS);
>> +    child = QTAILQ_FIRST(&bus->children);
>> +    assert(child != NULL);
>> +    ap = DO_UPCAST(APDevice, parent_obj, child->child);
>> +    return ap;
>> +}
>> +
>>   static void ap_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>   
>>       dc->desc = "AP device class";
>> +    dc->bus_type = TYPE_AP_BUS;
>>       dc->hotpluggable = false;
>>   }
>>   
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 65de952..94e5a1a 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {
>>       VFIODevice vdev;
>>   } VFIOAPDevice;
>>   
>> -#define VFIO_AP_DEVICE(obj) \
>> -        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
> 
> Hm?

I received a comment from Thomas Huth in Message ID
<2291104a-4cbf-e4fd-3496-fa0910beb96a@redhat.com>
that DO_UPCAST should be avoided in new code. This macro
should probably be restored and an AP_DEVICE() macro added.

> 
>> -
>>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>   {
>>       vdev->needs_reset = false;
> 

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Cornelia Huck]

On Thu, 29 Nov 2018 10:02:24 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 11/29/18 6:45 AM, Cornelia Huck wrote:
> > On Thu, 22 Nov 2018 17:35:51 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

> >> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> >> index 65de952..94e5a1a 100644
> >> --- a/hw/vfio/ap.c
> >> +++ b/hw/vfio/ap.c
> >> @@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {
> >>       VFIODevice vdev;
> >>   } VFIOAPDevice;
> >>   
> >> -#define VFIO_AP_DEVICE(obj) \
> >> -        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)  
> > 
> > Hm?  
> 
> I received a comment from Thomas Huth in Message ID
> <2291104a-4cbf-e4fd-3496-fa0910beb96a@redhat.com>
> that DO_UPCAST should be avoided in new code. This macro
> should probably be restored and an AP_DEVICE() macro added.

Yes, that makes sense.

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

[Halil Pasic]

On Thu, 22 Nov 2018 17:35:49 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> This series has 3 different type of patches:
> 
> The first two:
>   s390x/vfio: ap: Finding the AP bridge
>   s390x/vfio: ap: Use the APdevice as a child of the APBus
> 
> Are dealing with the QEMU Object Model and how we retrieve the
> AP devices from instruction interception.
> A lifting of the AP bridge, bus and device was necessary to
> ease the process and allow future extensions.
> 
> The third one is a place holder to ease reviewing:
>   s390x/vfio: ap: Linux uapi VFIO place holder
> 
> The last three are really dealing with PQAP/AQIC instruction
> interception and associate IOCTL calls to the VFIO AP device
> driver.
>   s390x/cpumodel: Set up CPU model for AQIC interception
>   s390x/vfio: ap: Definition for AP Adapter type
>   s390x/vfio: ap: Implementing AP Queue Interrupt Control
> 
> The S390 APQP/AQIC instruction is intercepted by the host
> to configure the AP queues interruption redirection.
> It retrieves the ISC used by the host and the one used
> by the guest and setup the indicator address.
> 
> This patch series
> - define the AQIC feature in the cpumodel,
> - extend the APDevice type for per card and 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"
> 

Sorry for raising concern this late, I hope it's better late than
never.

I have strong doubts that handling PQAP/AQCI via userspace is worth
the effort. IMHO we could do what we have to do on AQCI in kernel
iff the ap is done SIE interpreted, the appropriate feature is presented
to the guest, and the queue in question belongs to the given guest. Or
am I wrong?

I do understand that doing it like this *may* end up being beneficial
*if* we decide to do some sort of ap virtualization in QEMU. But I don't
see it coming in the foreseeable future, and for ap virtualization we
would need a solution for making the host do an NQAP and an DQAP on
behalf of the guest/emulator, and not only to do the same for PQAP/QCI.

In my understanding, with this, we would end up with an infrastructure
that only makes sense in a perspective of some 'future features' which
may never come to existence.

What I ask for is, please, let us examine the other option.

Regards,
Halil


> 
> Pierre Morel (6):
>   s390x/vfio: ap: Finding the AP bridge
>   s390x/vfio: ap: Use the APdevice as a child of the APBus
>   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: Implementing AP Queue Interrupt Control
> 
>  hw/s390x/ap-bridge.c            |  12 ++++
>  hw/s390x/ap-device.c            |  22 +++++++
>  hw/vfio/ap.c                    | 131 +++++++++++++++++++++++++++++++++++++---
>  include/hw/s390x/ap-bridge.h    |   1 +
>  include/hw/s390x/ap-device.h    |  65 ++++++++++++++++++++
>  include/hw/s390x/css.h          |   1 +
>  linux-headers/linux/vfio.h      |  25 ++++++++
>  target/s390x/cpu_features.c     |   1 +
>  target/s390x/cpu_features_def.h |   1 +
>  target/s390x/cpu_models.c       |   1 +
>  target/s390x/gen-features.c     |   1 +
>  target/s390x/kvm.c              |  31 ++++++++++
>  12 files changed, 282 insertions(+), 10 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Pierre Morel]

On 29/11/2018 16:11, Cornelia Huck wrote:
> On Thu, 29 Nov 2018 10:02:24 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 11/29/18 6:45 AM, Cornelia Huck wrote:
>>> On Thu, 22 Nov 2018 17:35:51 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 65de952..94e5a1a 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {
>>>>        VFIODevice vdev;
>>>>    } VFIOAPDevice;
>>>>    
>>>> -#define VFIO_AP_DEVICE(obj) \
>>>> -        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
>>>
>>> Hm?
>>
>> I received a comment from Thomas Huth in Message ID
>> <2291104a-4cbf-e4fd-3496-fa0910beb96a@redhat.com>
>> that DO_UPCAST should be avoided in new code. This macro
>> should probably be restored and an AP_DEVICE() macro added.
> 
> Yes, that makes sense.
> 

Yes,
Thanks

Pierre

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

Re: [Qemu-devel] [PATCH v2 1/6] s390x/vfio: ap: Finding the AP bridge

[Tony Krowiak]

On 11/22/18 11:35 AM, Pierre Morel wrote:
> In the case we will enter QEMU through interception of instructions,
> we will need to retrieve the AP devices.
> The base device is the AP bridge.
> 
> Let us implement a way to retrieve the AP Bridge from qtree.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/ap-bridge.c         | 12 ++++++++++++
>   include/hw/s390x/ap-bridge.h |  1 +
>   2 files changed, 13 insertions(+)
> 
> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
> index 3795d30..831ad5d 100644
> --- a/hw/s390x/ap-bridge.c
> +++ b/hw/s390x/ap-bridge.c
> @@ -14,6 +14,18 @@
>   #include "hw/s390x/ap-bridge.h"
>   #include "cpu.h"
>   
> +DeviceState *s390_get_ap_bridge(void)
> +{
> +    static DeviceState *apb;

Since you are caching a reference to the bridge after
retrieving it, why not make apb a global scope variable
and initialize it in s390_init_ap() when the bridge is
created. You can then declare it as an extern in the
ap-bridge.h header file and you eliminate the need for
this function. If you do make it a global var, I would
name it ap_bridge;

> +
> +    if (!apb) {
> +        apb = DEVICE(object_resolve_path(TYPE_AP_BRIDGE, NULL));
> +        assert(apb != NULL);
> +    }
> +
> +    return apb;
> +}

> +
>   static char *ap_bus_get_dev_path(DeviceState *dev)
>   {
>       /* at most one */
> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
> index 470e439..dacb877 100644
> --- a/include/hw/s390x/ap-bridge.h
> +++ b/include/hw/s390x/ap-bridge.h
> @@ -15,5 +15,6 @@
>   #define TYPE_AP_BUS "ap-bus"
>   
>   void s390_init_ap(void);
> +DeviceState *s390_get_ap_bridge(void);
>   
>   #endif
> 

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Tony Krowiak]

On 11/22/18 11:35 AM, Pierre Morel wrote:
> Two good reasons to use the base device as a child of the
> AP BUS:
> - We can easily find the device without traversing the qtree.
> - In case we have different APdevice instantiation, VFIO with
>    interception or emulation, we will need the APDevice as
>    a parent device.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>   hw/vfio/ap.c                 | 16 ++++++----------
>   include/hw/s390x/ap-device.h |  2 ++
>   3 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
> index f5ac8db..554d5aa 100644
> --- a/hw/s390x/ap-device.c
> +++ b/hw/s390x/ap-device.c
> @@ -11,13 +11,35 @@
>   #include "qemu/module.h"
>   #include "qapi/error.h"
>   #include "hw/qdev.h"
> +#include "hw/s390x/ap-bridge.h"
>   #include "hw/s390x/ap-device.h"
>   
> +APDevice *s390_get_ap(void)

How about ap_get_device(void)?

> +{
> +    static DeviceState *apb;

Why static if you call s390_get_ap_bridge()
to retrieve it without first checking for NULL?

> +    BusState *bus;
> +    BusChild *child;
> +    static APDevice *ap;
> +
> +    if (ap) {
> +        return ap;
> +    }
> +
> +    apb = s390_get_ap_bridge();
> +    /* We have only a single child on the BUS */
> +    bus = qdev_get_child_bus(apb, TYPE_AP_BUS
> +    child = QTAILQ_FIRST(&bus->children);
> +    assert(child != NULL);
> +    ap = DO_UPCAST(APDevice, parent_obj, child->child);

I received a comment from Thomas Huth in Message ID
<2291104a-4cbf-e4fd-3496-fa0910beb96a@redhat.com>
that DO_UPCAST should be avoided in new code. You should
create an AP_DEVICE macro for this in ap-device.h

> +    return ap;
> +}
> +
>   static void ap_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       dc->desc = "AP device class";
> +    dc->bus_type = TYPE_AP_BUS;
>       dc->hotpluggable = false;
>   }
>   
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952..94e5a1a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {See my comment above about DO_UPCAST
>       VFIODevice vdev;
>   } VFIOAPDevice;
>   
> -#define VFIO_AP_DEVICE(obj) \
> -        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
> -
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>   {
>       vdev->needs_reset = false;
> @@ -90,8 +87,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>       char *mdevid;
>       Error *local_err = NULL;
>       VFIOGroup *vfio_group;
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);See my comment above about DO_UPCAST
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);

See my comment above about DO_UPCAST.

>   
>       vfio_group = vfio_ap_get_group(vapdev, &local_err);
>       if (!vfio_group) {
> @@ -120,8 +117,8 @@ out_err:
>   
>   static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
>   {
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);

See my comment above about DO_UPCAST

>       VFIOGroup *group = vapdev->vdev.group;
>   
>       vfio_ap_put_device(vapdev);
> @@ -136,8 +133,8 @@ static Property vfio_ap_properties[] = {
>   static void vfio_ap_reset(DeviceState *dev)
>   {
>       int ret;
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);

FYI, these macros were created in response to Thomas Huth's
comments about DO_UPCAST.

> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
>   
>       ret = ioctl(vapdev->vdev.fd, VFIO_DEVICE_RESET);
>       if (ret) {
> @@ -163,7 +160,6 @@ static void vfio_ap_class_init(ObjectClass *klass, void *data)
>       dc->unrealize = vfio_ap_unrealize;
>       dc->hotpluggable = false;
>       dc->reset = vfio_ap_reset;
> -    dc->bus_type = TYPE_AP_BUS;
>   }
>   
>   static const TypeInfo vfio_ap_info = {
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> index 765e908..5f3c840 100644
> --- a/include/hw/s390x/ap-device.h
> +++ b/include/hw/s390x/ap-device.h
> @@ -19,4 +19,6 @@ typedef struct APDevice {
>   #define AP_DEVICE(obj) \
>       OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
>   
> +APDevice *s390_get_ap(void);

How about APDevice *ap_get_device(void)?

> +
>   #endif /* HW_S390X_AP_DEVICE_H */
> 

Re: [Qemu-devel] [PATCH v2 4/6] s390x/cpumodel: Set up CPU model for AQIC interception

[Tony Krowiak]

On 11/22/18 11:35 AM, Pierre Morel wrote:
> 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 +
>   target/s390x/gen-features.c     | 1 +
>   4 files changed, 4 insertions(+)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 60cfeba..c464abf 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 5fc7e7b..3f22780 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 7c253ff..6b5e94b 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;
>   
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 70015ea..b051221 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -448,6 +448,7 @@ static uint16_t full_GEN12_GA1[] = {
>       S390_FEAT_EDAT_2,
>       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>       S390_FEAT_AP_QUERY_CONFIG_INFO,
> +    S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL,
>       S390_FEAT_AP_FACILITIES_TEST,
>       S390_FEAT_AP,
>   };

Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>

> 

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

[Tony Krowiak]

On 11/22/18 11:35 AM, Pierre Morel 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 new structures, APCard and APQueue to keep track of
> the ISC, route and indicator address and we add an array of
> APCards in the APDevice.
> 
> 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                 | 117 ++++++++++++++++++++++++++++++++++++++++++-
>   include/hw/s390x/ap-device.h |  63 +++++++++++++++++++++++
>   target/s390x/kvm.c           |  31 ++++++++++++
>   3 files changed, 210 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 94e5a1a..20d2e5f 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -4,6 +4,7 @@
>    * Copyright 2018 IBM Corp.
>    * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
>    *            Halil Pasic <pasic@linux.ibm.com>
> + *            Pierre Morel <pmorel@linux.ibm.com>
>    *
>    * This work is licensed under the terms of the GNU GPL, version 2 or (at
>    * your option) any later version. See the COPYING file in the top-level
> @@ -14,7 +15,6 @@
>   #include <sys/ioctl.h>
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
> -#include "hw/sysbus.h"
>   #include "hw/vfio/vfio.h"
>   #include "hw/vfio/vfio-common.h"
>   #include "hw/s390x/ap-device.h"
> @@ -27,6 +27,8 @@
>   #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"
>   
> @@ -35,6 +37,117 @@ typedef struct VFIOAPDevice {
>       VFIODevice vdev;
>   } VFIOAPDevice;
>   
> +static uint32_t ap_pqap_clear_irq(VFIODevice *vdev, APQueue *apq)
> +{
> +    struct vfio_ap_aqic param;
> +    uint32_t retval;
> +
> +    param.apqn = apq->apqn;
> +    param.isc = apq->isc;
> +    param.argsz = sizeof(param);
> +
> +    retval = ioctl(vdev->fd, VFIO_AP_CLEAR_IRQ, &param);
> +    switch (retval) {
> +    case 0:    /* Fall through and return the instruction status */

Unnecessary comment, we can see the code is going to fall through and
return status. Sorry, I know its a nit.

> +        release_indicator(&apq->routes.adapter, apq->nib);
> +    case -EIO: /* The case the PQAP instruction failed with status */

We know it's a case statement, so leave off the "The case the".

> +        return  param.status;
> +    default:   /* The case the ioctl call failed without isuing instruction */

Same here.

> +        break;
> +    }
> +    return ap_reg_set_status(AP_RC_INVALID_ADDR);
> +}
> +
> +static uint32_t ap_pqap_set_irq(VFIODevice *vdev, APQueue *apq, uint64_t g_nib)
> +{
> +    struct vfio_ap_aqic param;
> +    uint32_t retval;
> +    uint32_t id;
> +
> +    id = css_get_adapter_id(CSS_IO_ADAPTER_AP, apq->isc);
> +    if (id == -1) {
> +        return ap_reg_set_status(AP_RC_INVALID_ADDR);
> +    }
> +    apq->routes.adapter.adapter_id = id;
> +    apq->nib = get_indicator(ldq_p(&g_nib), 8);
> +
> +    retval = map_indicator(&apq->routes.adapter, apq->nib);
> +    if (retval) {
> +        return ap_reg_set_status(AP_RC_INVALID_ADDR);
> +    }
> +
> +    param.apqn = apq->apqn;
> +    param.isc = apq->isc;
> +    param.nib = g_nib;
> +    param.adapter_id = id;
> +    param.argsz = sizeof(param);
> +
> +    retval =  ioctl(vdev->fd, VFIO_AP_SET_IRQ, &param);
> +    switch (retval) {
> +    case -EIO: /* The case the PQAP instruction failed with status */

We know it's a case statement, so leave off the "The case the".

> +        release_indicator(&apq->routes.adapter, apq->nib);
> +    case 0:    /* Fall through and return the instruction status */

Unnecessary comment, the code speaks for itself.

> +        return  param.status;
> +    default:   /* The case the ioctl call failed without isuing instruction */

We know it's a case statement, so leave off the "The case the".

> +        break;
> +    }
> +    release_indicator(&apq->routes.adapter, apq->nib);
> +    return ap_reg_set_status(AP_RC_INVALID_ADDR);
> +}
> +
> +static int ap_pqap_aqic(CPUS390XState *env)
> +{
> +    uint64_t g_nib = env->regs[2];
> +    uint8_t apid = ap_reg_get_apid(env->regs[0]);
> +    uint8_t apqi = ap_reg_get_apqi(env->regs[0]);
> +    uint32_t retval;
> +    APDevice *ap = s390_get_ap();

To be consistent with the naming convention in the rest of
this file, can you name this variable 'apdev'?

> +    VFIODevice *vdev;
> +    VFIOAPDevice *ap_vdev;

To be consistent with the naming convention in the rest of
this file, can you name this variable 'vapdev'?

> +    APQueue *apq;
> +
> +    ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, ap);

Where is 'apdev' defined/initialized?

> +    vdev = &ap_vdev->vdev;
> +    apq = &ap->card[apid].queue[apqi];
> +    apq->isc = ap_reg_get_isc(env->regs[1]);
> +    apq->apqn = (apid << 8) | apqi;
> +
> +    if (ap_reg_get_ir(env->regs[1])) {
> +        retval = ap_pqap_set_irq(vdev, apq, g_nib);
> +    } else {
> +        retval = ap_pqap_clear_irq(vdev, apq);
> +    }
> +
> +    env->regs[1] = retval;
> +    if (retval & AP_STATUS_RC_MASK) {
> +        return 3;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * ap_pqap
> + * @env: environment pointing to registers
> + * return value: Code Condition
> + */
> +int ap_pqap(CPUS390XState *env)
> +{
> +    int cc = 0;
> +
> +    switch (ap_reg_get_fc(env->regs[0])) {
> +    case AQIC:
> +        if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) {
> +            return -PGM_OPERATION;

Shouldn't this be PGM_SPECIFICATION?

> +        }
> +        cc = ap_pqap_aqic(env);
> +        break;
> +    default:
> +        return -PGM_OPERATION;
> +    }
> +    return cc;
> +}
> +
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>   {
>       vdev->needs_reset = false;
> @@ -106,6 +219,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>           goto out_get_dev_err;
>       }
>   
> +    css_register_io_adapters(CSS_IO_ADAPTER_AP, true, false,
> +                             0, &error_abort);
>       return;
>   
>   out_get_dev_err:
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> index 5f3c840..8a36780 100644
> --- a/include/hw/s390x/ap-device.h
> +++ b/include/hw/s390x/ap-device.h
> @@ -12,8 +12,25 @@
>   
>   #define AP_DEVICE_TYPE       "ap-device"
>   
> +#define MAX_AP_CARD 256
> +#define MAX_AP_DOMAIN 256
> +
> +#include "hw/s390x/s390_flic.h"
> +#include "hw/s390x/css.h"
> +typedef struct APQueue {
> +    AdapterRoutes routes;
> +    IndAddr *nib;
> +    uint16_t apqn;
> +    uint8_t isc;
> +} APQueue;
> +
> +typedef struct APCard {
> +    APQueue queue[MAX_AP_DOMAIN];

This seems to be an unnecessary allocation of memory, particularly
if there is only a few queues. I understand this
maps to the concept of a matrix and makes for quick retrieval of
an APQueue, but I think it would be more efficient to use something
like a GTree, or a GHashTable both of which would save you space and
allow for fairly quick retrieval.

> +} APCard;
> +
>   typedef struct APDevice {
>       DeviceState parent_obj;
> +    APCard card[MAX_AP_CARD];

See comment above concerning the 'queue' field in the
APCard structure. I would also name this field 'cards'
since it represents all APCard objects.

>   } APDevice;
>   
>   #define AP_DEVICE(obj) \
> @@ -21,4 +38,50 @@ typedef struct APDevice {
>   
>   APDevice *s390_get_ap(void);
>   
> +#include "cpu.h"
> +int ap_pqap(CPUS390XState *env);
> +
> +/* AP PQAP commands definitions */
> +#define AQIC 0x03
> +
> +#define AP_AQIC_ZERO_BITS 0x0ff0000
> +
> +/* Register 0 hold the command and APQN */
> +static inline uint8_t ap_reg_get_apid(uint64_t r)
> +{
> +        return (r >> 8) & 0xff;
> +}
> +
> +static inline uint8_t ap_reg_get_apqi(uint64_t r)
> +{
> +        return r & 0xff;
> +}
> +
> +static inline uint16_t ap_reg_get_fc(uint64_t r)
> +{
> +        return (r >> 24) & 0xff;
> +}
> +
> +static inline uint16_t ap_reg_get_ir(uint64_t r)
> +{
> +        return (r >> 47) & 0x01;
> +}
> +
> +static inline uint16_t ap_reg_get_isc(uint64_t r)
> +{
> +        return r  & 0x7;
> +}
> +
> +/* AP status returned by the AP PQAP commands */
> +#define AP_STATUS_RC_MASK 0x00ff0000
> +#define AP_RC_APQN_INVALID 0x01
> +#define AP_RC_INVALID_ADDR 0x06
> +#define AP_RC_BAD_STATE    0x07
> +
> +/* Register 1 as input hold the AQIC information */
> +static inline uint32_t ap_reg_set_status(uint8_t status)

This function does not set anything, but returns an APQSW.
Maybe it should be named ap_reg_get_status_word or ap_reg_get_apqsw
or ap_reg_get_status.

> +{
> +        return status << 16;
> +}
> +
>   #endif /* HW_S390X_AP_DEVICE_H */
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 2ebf26a..a4b5d90 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,32 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>       return 0;
>   }
>   
> +static int kvm_ap_pqap(S390CPU *cpu, uint16_t ipbh0)
> +{
> +    CPUS390XState *env = &cpu->env;
> +    int r;
> +
> +    if (env->psw.mask & PSW_MASK_PSTATE) {
> +        kvm_s390_program_interrupt(cpu, PGM_PRIVILEGED);
> +        return 0;
> +    }
> +
> +    if (env->regs[0] & AP_AQIC_ZERO_BITS) {
> +        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> +        return 0;
> +    }

This check does not belong here; for example, if the PQAP(TAPQ)
instruction is intercepted and the T bit (bit 40) is set, a
specification exception would be erroneously recognized. This check
should be done in the ap_pqap_aqic() function.

> +
> +    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 +1244,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);
> 

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

[Cornelia Huck]

On Thu, 29 Nov 2018 16:53:28 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 11/22/18 11:35 AM, Pierre Morel wrote:

> > +static uint32_t ap_pqap_clear_irq(VFIODevice *vdev, APQueue *apq)
> > +{
> > +    struct vfio_ap_aqic param;
> > +    uint32_t retval;
> > +
> > +    param.apqn = apq->apqn;
> > +    param.isc = apq->isc;
> > +    param.argsz = sizeof(param);
> > +
> > +    retval = ioctl(vdev->fd, VFIO_AP_CLEAR_IRQ, &param);
> > +    switch (retval) {
> > +    case 0:    /* Fall through and return the instruction status */  
> 
> Unnecessary comment, we can see the code is going to fall through and
> return status. Sorry, I know its a nit.

FWIW, some static code checking tools look for annotations like
/* Fallthrough */ as an indication that a fallthrough is actually
intended and not a coding error.

Re: [Qemu-devel] [PATCH v2 1/6] s390x/vfio: ap: Finding the AP bridge

[Pierre Morel]

On 29/11/2018 21:30, Tony Krowiak wrote:
> On 11/22/18 11:35 AM, Pierre Morel wrote:
>> In the case we will enter QEMU through interception of instructions,
>> we will need to retrieve the AP devices.
>> The base device is the AP bridge.
>>
>> Let us implement a way to retrieve the AP Bridge from qtree.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/ap-bridge.c         | 12 ++++++++++++
>>   include/hw/s390x/ap-bridge.h |  1 +
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
>> index 3795d30..831ad5d 100644
>> --- a/hw/s390x/ap-bridge.c
>> +++ b/hw/s390x/ap-bridge.c
>> @@ -14,6 +14,18 @@
>>   #include "hw/s390x/ap-bridge.h"
>>   #include "cpu.h"
>> +DeviceState *s390_get_ap_bridge(void)
>> +{
>> +    static DeviceState *apb;
> 
> Since you are caching a reference to the bridge after
> retrieving it, why not make apb a global scope variable
> and initialize it in s390_init_ap() when the bridge is
> created. You can then declare it as an extern in the
> ap-bridge.h header file and you eliminate the need for
> this function. If you do make it a global var, I would
> name it ap_bridge;

We already had this discussion when implementing zPCI.
I use a similar solution as it was decided at that time.


Regards,
Pierre


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

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Pierre Morel]

On 29/11/2018 21:42, Tony Krowiak wrote:
> On 11/22/18 11:35 AM, Pierre Morel wrote:
>> Two good reasons to use the base device as a child of the
>> AP BUS:
>> - We can easily find the device without traversing the qtree.
>> - In case we have different APdevice instantiation, VFIO with
>>    interception or emulation, we will need the APDevice as
>>    a parent device.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>>   hw/vfio/ap.c                 | 16 ++++++----------
>>   include/hw/s390x/ap-device.h |  2 ++
>>   3 files changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
>> index f5ac8db..554d5aa 100644
>> --- a/hw/s390x/ap-device.c
>> +++ b/hw/s390x/ap-device.c
>> @@ -11,13 +11,35 @@
>>   #include "qemu/module.h"
>>   #include "qapi/error.h"
>>   #include "hw/qdev.h"
>> +#include "hw/s390x/ap-bridge.h"
>>   #include "hw/s390x/ap-device.h"
>> +APDevice *s390_get_ap(void)
> 
> How about ap_get_device(void)?

Yes, keep same conventions.

> 
>> +{
>> +    static DeviceState *apb;
> 
> Why static if you call s390_get_ap_bridge()
> to retrieve it without first checking for NULL?

static are initialized as NULL.

> 
>> +    BusState *bus;
>> +    BusChild *child;
>> +    static APDevice *ap;
>> +
>> +    if (ap) {
>> +        return ap;
>> +    }
>> +
>> +    apb = s390_get_ap_bridge();
>> +    /* We have only a single child on the BUS */
>> +    bus = qdev_get_child_bus(apb, TYPE_AP_BUS
>> +    child = QTAILQ_FIRST(&bus->children);
>> +    assert(child != NULL);
>> +    ap = DO_UPCAST(APDevice, parent_obj, child->child);
> 
> I received a comment from Thomas Huth in Message ID
> <2291104a-4cbf-e4fd-3496-fa0910beb96a@redhat.com>
> that DO_UPCAST should be avoided in new code. You should
> create an AP_DEVICE macro for this in ap-device.h
> 

Thanks I will do.

Regards,
Pierre


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

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

[Pierre Morel]

On 29/11/2018 22:53, Tony Krowiak wrote:
> On 11/22/18 11:35 AM, Pierre Morel wrote:
>> We intercept the PQAP(AQIC) instruction and transform

...

>> +static int ap_pqap_aqic(CPUS390XState *env)
>> +{
>> +    uint64_t g_nib = env->regs[2];
>> +    uint8_t apid = ap_reg_get_apid(env->regs[0]);
>> +    uint8_t apqi = ap_reg_get_apqi(env->regs[0]);
>> +    uint32_t retval;
>> +    APDevice *ap = s390_get_ap();
> 
> To be consistent with the naming convention in the rest of
> this file, can you name this variable 'apdev'?

OK

> 
>> +    VFIODevice *vdev;
>> +    VFIOAPDevice *ap_vdev;
> 
> To be consistent with the naming convention in the rest of
> this file, can you name this variable 'vapdev'?
> 
>> +    APQueue *apq;
>> +
>> +    ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, ap);
> 
> Where is 'apdev' defined/initialized?

It is part of the VFIOAPDevice structure

> 
>> +    vdev = &ap_vdev->vdev;
>> +    apq = &ap->card[apid].queue[apqi];
>> +    apq->isc = ap_reg_get_isc(env->regs[1]);
>> +    apq->apqn = (apid << 8) | apqi;
>> +
>> +    if (ap_reg_get_ir(env->regs[1])) {
>> +        retval = ap_pqap_set_irq(vdev, apq, g_nib);
>> +    } else {
>> +        retval = ap_pqap_clear_irq(vdev, apq);
>> +    }
>> +
>> +    env->regs[1] = retval;
>> +    if (retval & AP_STATUS_RC_MASK) {
>> +        return 3;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * ap_pqap
>> + * @env: environment pointing to registers
>> + * return value: Code Condition
>> + */
>> +int ap_pqap(CPUS390XState *env)
>> +{
>> +    int cc = 0;
>> +
>> +    switch (ap_reg_get_fc(env->regs[0])) {
>> +    case AQIC:
>> +        if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) {
>> +            return -PGM_OPERATION;
> 
> Shouldn't this be PGM_SPECIFICATION?

Before the patch, when PQAP is not intercepted we sent PGM_OPERATION.
I think we should continue doing the same as before if the feature is 
not activated.


> 
>> +        }

...snip...


>> +typedef struct APQueue {
>> +    AdapterRoutes routes;
>> +    IndAddr *nib;
>> +    uint16_t apqn;
>> +    uint8_t isc;
>> +} APQueue;
>> +
>> +typedef struct APCard {
>> +    APQueue queue[MAX_AP_DOMAIN];
> 
> This seems to be an unnecessary allocation of memory, particularly
> if there is only a few queues. I understand this
> maps to the concept of a matrix and makes for quick retrieval of
> an APQueue, but I think it would be more efficient to use something
> like a GTree, or a GHashTable both of which would save you space and
> allow for fairly quick retrieval.


OK.
We can optimize this later.

...

>> +/* Register 1 as input hold the AQIC information */
>> +static inline uint32_t ap_reg_set_status(uint8_t status)
> 
> This function does not set anything, but returns an APQSW.
> Maybe it should be named ap_reg_get_status_word or ap_reg_get_apqsw
> or ap_reg_get_status.

All these operations are "get" when retrieving informations from 
registers and "set" when setting information inside registers.


> 
...snip...
>>   }
>> +static int kvm_ap_pqap(S390CPU *cpu, uint16_t ipbh0)
>> +{
>> +    CPUS390XState *env = &cpu->env;
>> +    int r;
>> +
>> +    if (env->psw.mask & PSW_MASK_PSTATE) {
>> +        kvm_s390_program_interrupt(cpu, PGM_PRIVILEGED);
>> +        return 0;
>> +    }
>> +
>> +    if (env->regs[0] & AP_AQIC_ZERO_BITS) {
>> +        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
>> +        return 0;
>> +    }
> 
> This check does not belong here; for example, if the PQAP(TAPQ)
> instruction is intercepted and the T bit (bit 40) is set, a
> specification exception would be erroneously recognized. This check
> should be done in the ap_pqap_aqic() function.

Right, I will move the test.


Regards,
Pierre

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

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Pierre Morel]

On 30/11/2018 10:31, Pierre Morel wrote:
> On 29/11/2018 21:42, Tony Krowiak wrote:
>> On 11/22/18 11:35 AM, Pierre Morel wrote:
>>> Two good reasons to use the base device as a child of the
>>> AP BUS:
>>> - We can easily find the device without traversing the qtree.
>>> - In case we have different APdevice instantiation, VFIO with
>>>    interception or emulation, we will need the APDevice as
>>>    a parent device.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>>>   hw/vfio/ap.c                 | 16 ++++++----------
>>>   include/hw/s390x/ap-device.h |  2 ++
>>>   3 files changed, 30 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
>>> index f5ac8db..554d5aa 100644
>>> --- a/hw/s390x/ap-device.c
>>> +++ b/hw/s390x/ap-device.c
>>> @@ -11,13 +11,35 @@
>>>   #include "qemu/module.h"
>>>   #include "qapi/error.h"
>>>   #include "hw/qdev.h"
>>> +#include "hw/s390x/ap-bridge.h"
>>>   #include "hw/s390x/ap-device.h"
>>> +APDevice *s390_get_ap(void)
>>
>> How about ap_get_device(void)?
> 
> Yes, keep same conventions.

Apropos convention, this function is exported.
So I think the s390 prefix is important.

Pierre


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

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

[Pierre Morel]

On 29/11/2018 16:55, Halil Pasic wrote:
> On Thu, 22 Nov 2018 17:35:49 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> This series has 3 different type of patches:
>>
>> The first two:
>>    s390x/vfio: ap: Finding the AP bridge
>>    s390x/vfio: ap: Use the APdevice as a child of the APBus
>>
>> Are dealing with the QEMU Object Model and how we retrieve the
>> AP devices from instruction interception.
>> A lifting of the AP bridge, bus and device was necessary to
>> ease the process and allow future extensions.
>>
>> The third one is a place holder to ease reviewing:
>>    s390x/vfio: ap: Linux uapi VFIO place holder
>>
>> The last three are really dealing with PQAP/AQIC instruction
>> interception and associate IOCTL calls to the VFIO AP device
>> driver.
>>    s390x/cpumodel: Set up CPU model for AQIC interception
>>    s390x/vfio: ap: Definition for AP Adapter type
>>    s390x/vfio: ap: Implementing AP Queue Interrupt Control
>>
>> The S390 APQP/AQIC instruction is intercepted by the host
>> to configure the AP queues interruption redirection.
>> It retrieves the ISC used by the host and the one used
>> by the guest and setup the indicator address.
>>
>> This patch series
>> - define the AQIC feature in the cpumodel,
>> - extend the APDevice type for per card and 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"
>>
> 
> Sorry for raising concern this late, I hope it's better late than
> never.
> 
> I have strong doubts that handling PQAP/AQCI via userspace is worth
> the effort. IMHO we could do what we have to do on AQCI in kernel
> iff the ap is done SIE interpreted, the appropriate feature is presented
> to the guest, and the queue in question belongs to the given guest. Or
> am I wrong?
> 
> I do understand that doing it like this *may* end up being beneficial
> *if* we decide to do some sort of ap virtualization in QEMU. But I don't
> see it coming in the foreseeable future, and for ap virtualization we
> would need a solution for making the host do an NQAP and an DQAP on
> behalf of the guest/emulator, and not only to do the same for PQAP/QCI.
> 
> In my understanding, with this, we would end up with an infrastructure
> that only makes sense in a perspective of some 'future features' which
> may never come to existence.
> 
> What I ask for is, please, let us examine the other option.
> 
> Regards,
> Halil
> 
> 

As we discussed offline, I already began to implement prototypes for 
both options.
This is a clear design choice.

If we examine the pro and contra, I can list:

1- Pro kernel implementation of the PQAP/AQIC
-> rapidity of the reaction

Question: is this important?
Answer: NO,
Why: The PQAP/AQIC is rarely used by the AP driver of the guest. 
exclusively during RESET of the AP queue.
I do not think we need a rapid reaction there.


2- Pro userland implementation of PQAP/AQIC
-> standard implementation, already used by PCI, CCW

Question: is it important?
Answer: YES
Why:  like following the standard
It is easily extend-able to other virtualization implementation like 
interception based VFIO and emulation


There is no implementation which would be really more complicated as the 
other, for both we will need to introduce new pro APQN (queue) 
structures to hold the interrupt information (ISC, NIB), for both we 
will need to ping the NIB in memory.

So as long as there are no other opinion against the design I presented 
here I will continue this way while considering the comments I got on 
this series.

Regards,
Pierre


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

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

[Halil Pasic]

On Fri, 30 Nov 2018 14:01:42 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 29/11/2018 16:55, Halil Pasic wrote:
> > On Thu, 22 Nov 2018 17:35:49 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> > 
> >> This series has 3 different type of patches:
> >>
> >> The first two:
> >>    s390x/vfio: ap: Finding the AP bridge
> >>    s390x/vfio: ap: Use the APdevice as a child of the APBus
> >>
> >> Are dealing with the QEMU Object Model and how we retrieve the
> >> AP devices from instruction interception.
> >> A lifting of the AP bridge, bus and device was necessary to
> >> ease the process and allow future extensions.
> >>
> >> The third one is a place holder to ease reviewing:
> >>    s390x/vfio: ap: Linux uapi VFIO place holder
> >>
> >> The last three are really dealing with PQAP/AQIC instruction
> >> interception and associate IOCTL calls to the VFIO AP device
> >> driver.
> >>    s390x/cpumodel: Set up CPU model for AQIC interception
> >>    s390x/vfio: ap: Definition for AP Adapter type
> >>    s390x/vfio: ap: Implementing AP Queue Interrupt Control
> >>
> >> The S390 APQP/AQIC instruction is intercepted by the host
> >> to configure the AP queues interruption redirection.
> >> It retrieves the ISC used by the host and the one used
> >> by the guest and setup the indicator address.
> >>
> >> This patch series
> >> - define the AQIC feature in the cpumodel,
> >> - extend the APDevice type for per card and 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"
> >>
> > 
> > Sorry for raising concern this late, I hope it's better late than
> > never.
> > 
> > I have strong doubts that handling PQAP/AQCI via userspace is worth
> > the effort. IMHO we could do what we have to do on AQCI in kernel
> > iff the ap is done SIE interpreted, the appropriate feature is presented
> > to the guest, and the queue in question belongs to the given guest. Or
> > am I wrong?
> > 
> > I do understand that doing it like this *may* end up being beneficial
> > *if* we decide to do some sort of ap virtualization in QEMU. But I don't
> > see it coming in the foreseeable future, and for ap virtualization we
> > would need a solution for making the host do an NQAP and an DQAP on
> > behalf of the guest/emulator, and not only to do the same for PQAP/QCI.
> > 
> > In my understanding, with this, we would end up with an infrastructure
> > that only makes sense in a perspective of some 'future features' which
> > may never come to existence.
> > 
> > What I ask for is, please, let us examine the other option.
> > 
> > Regards,
> > Halil
> > 
> > 
> 
> As we discussed offline, I already began to implement prototypes for 
> both options.
> This is a clear design choice.
> 
> If we examine the pro and contra, I can list:
> 
> 1- Pro kernel implementation of the PQAP/AQIC
> -> rapidity of the reaction
> 
> Question: is this important?
> Answer: NO,
> Why: The PQAP/AQIC is rarely used by the AP driver of the guest. 
> exclusively during RESET of the AP queue.
> I do not think we need a rapid reaction there.
> 
> 
> 2- Pro userland implementation of PQAP/AQIC
> -> standard implementation, already used by PCI, CCW
> 
> Question: is it important?
> Answer: YES
> Why:  like following the standard
> It is easily extend-able to other virtualization implementation like 
> interception based VFIO and emulation
> 
> 
> There is no implementation which would be really more complicated as the 
> other, for both we will need to introduce new pro APQN (queue) 
> structures to hold the interrupt information (ISC, NIB), for both we 
> will need to ping the NIB in memory.
> 
> So as long as there are no other opinion against the design I presented 
> here I will continue this way while considering the comments I got on 
> this series.
> 

I'm a bit confused. Your first sentence reads like you in a process of
providing a kernel-heavy version. Your last sentence however reads like,
based on the discussion following the first sentence, you decided to not
explore, if the kernel-heavy variant (we still need a cpu model feature
in QEMU) is simpler.

Frankly, my feeling was that a kernel heavy implementation can be done
with less lines of code (considering QEMU and Linux) and without
introducing new ioctl interfaces. I may be wrong. You certainly seem to
dispute that a kernel-heavy implementation is likely to be simpler. If
you don't want to explore that option, I would like to ask you for your
permission to do it myself if my time allows.

Regards,
Halil

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Tony Krowiak]

On 11/30/18 4:31 AM, Pierre Morel wrote:
> On 29/11/2018 21:42, Tony Krowiak wrote:
>> On 11/22/18 11:35 AM, Pierre Morel wrote:
>>> Two good reasons to use the base device as a child of the
>>> AP BUS:
>>> - We can easily find the device without traversing the qtree.
>>> - In case we have different APdevice instantiation, VFIO with
>>>    interception or emulation, we will need the APDevice as
>>>    a parent device.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>>>   hw/vfio/ap.c                 | 16 ++++++----------
>>>   include/hw/s390x/ap-device.h |  2 ++
>>>   3 files changed, 30 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
>>> index f5ac8db..554d5aa 100644
>>> --- a/hw/s390x/ap-device.c
>>> +++ b/hw/s390x/ap-device.c
>>> @@ -11,13 +11,35 @@
>>>   #include "qemu/module.h"
>>>   #include "qapi/error.h"
>>>   #include "hw/qdev.h"
>>> +#include "hw/s390x/ap-bridge.h"
>>>   #include "hw/s390x/ap-device.h"
>>> +APDevice *s390_get_ap(void)
>>
>> How about ap_get_device(void)?
> 
> Yes, keep same conventions.
> 
>>
>>> +{
>>> +    static DeviceState *apb;
>>
>> Why static if you call s390_get_ap_bridge()
>> to retrieve it without first checking for NULL?
> 
> static are initialized as NULL.

Yes, but down below, you call s390_get_ap_bridge() to set apb
regardless of whether apb is NULL or not. It makes no sense to
declare is as static here. It is also redundant because the 
s390_get_ap_bridge() function already caches it in a static
variable.

> 
>>
>>> +    BusState *bus;
>>> +    BusChild *child;
>>> +    static APDevice *ap;
>>> +
>>> +    if (ap) {
>>> +        return ap;
>>> +    }
>>> +
>>> +    apb = s390_get_ap_bridge();
>>> +    /* We have only a single child on the BUS */
>>> +    bus = qdev_get_child_bus(apb, TYPE_AP_BUS
>>> +    child = QTAILQ_FIRST(&bus->children);
>>> +    assert(child != NULL);
>>> +    ap = DO_UPCAST(APDevice, parent_obj, child->child);
>>
>> I received a comment from Thomas Huth in Message ID
>> <2291104a-4cbf-e4fd-3496-fa0910beb96a@redhat.com>
>> that DO_UPCAST should be avoided in new code. You should
>> create an AP_DEVICE macro for this in ap-device.h
>>
> 
> Thanks I will do.
> 
> Regards,
> Pierre
> 
> 

Re: [Qemu-devel] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Pierre Morel]

On 30/11/2018 16:58, Tony Krowiak wrote:
> On 11/30/18 4:31 AM, Pierre Morel wrote:
>> On 29/11/2018 21:42, Tony Krowiak wrote:
>>> On 11/22/18 11:35 AM, Pierre Morel wrote:
>>>> Two good reasons to use the base device as a child of the
>>>> AP BUS:
>>>> - We can easily find the device without traversing the qtree.
>>>> - In case we have different APdevice instantiation, VFIO with
>>>>    interception or emulation, we will need the APDevice as
>>>>    a parent device.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>   hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>>>>   hw/vfio/ap.c                 | 16 ++++++----------
>>>>   include/hw/s390x/ap-device.h |  2 ++
>>>>   3 files changed, 30 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
>>>> index f5ac8db..554d5aa 100644
>>>> --- a/hw/s390x/ap-device.c
>>>> +++ b/hw/s390x/ap-device.c
>>>> @@ -11,13 +11,35 @@
>>>>   #include "qemu/module.h"
>>>>   #include "qapi/error.h"
>>>>   #include "hw/qdev.h"
>>>> +#include "hw/s390x/ap-bridge.h"
>>>>   #include "hw/s390x/ap-device.h"
>>>> +APDevice *s390_get_ap(void)
>>>
>>> How about ap_get_device(void)?
>>
>> Yes, keep same conventions.
>>
>>>
>>>> +{
>>>> +    static DeviceState *apb;
>>>
>>> Why static if you call s390_get_ap_bridge()
>>> to retrieve it without first checking for NULL?
>>
>> static are initialized as NULL.
> 
> Yes, but down below, you call s390_get_ap_bridge() to set apb
> regardless of whether apb is NULL or not. It makes no sense to
> declare is as static here. It is also redundant because the 
> s390_get_ap_bridge() function already caches it in a static
> variable.


OK thanks, yes we do not need the static for apb here.



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

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/6] s390x/vfio: ap: Use the APdevice as a child of the APBus

[Halil Pasic]

On Thu, 22 Nov 2018 17:35:51 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Two good reasons to use the base device as a child of the
> AP BUS:
> - We can easily find the device without traversing the qtree.
> - In case we have different APdevice instantiation, VFIO with
>   interception or emulation, we will need the APDevice as
>   a parent device.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/ap-device.c         | 22 ++++++++++++++++++++++
>  hw/vfio/ap.c                 | 16 ++++++----------
>  include/hw/s390x/ap-device.h |  2 ++
>  3 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
> index f5ac8db..554d5aa 100644
> --- a/hw/s390x/ap-device.c
> +++ b/hw/s390x/ap-device.c
> @@ -11,13 +11,35 @@
>  #include "qemu/module.h"
>  #include "qapi/error.h"
>  #include "hw/qdev.h"
> +#include "hw/s390x/ap-bridge.h"
>  #include "hw/s390x/ap-device.h"
>  
> +APDevice *s390_get_ap(void)
> +{
> +    static DeviceState *apb;
> +    BusState *bus;
> +    BusChild *child;
> +    static APDevice *ap;
> +
> +    if (ap) {
> +        return ap;
> +    }
> +
> +    apb = s390_get_ap_bridge();
> +    /* We have only a single child on the BUS */
> +    bus = qdev_get_child_bus(apb, TYPE_AP_BUS);
> +    child = QTAILQ_FIRST(&bus->children);
> +    assert(child != NULL);
> +    ap = DO_UPCAST(APDevice, parent_obj, child->child);
> +    return ap;
> +}
> +
>  static void ap_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->desc = "AP device class";
> +    dc->bus_type = TYPE_AP_BUS;
>      dc->hotpluggable = false;
>  }
>  
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952..94e5a1a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -35,9 +35,6 @@ typedef struct VFIOAPDevice {
>      VFIODevice vdev;
>  } VFIOAPDevice;
>  
> -#define VFIO_AP_DEVICE(obj) \
> -        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
> -
>  static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>  {
>      vdev->needs_reset = false;
> @@ -90,8 +87,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>      char *mdevid;
>      Error *local_err = NULL;
>      VFIOGroup *vfio_group;
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
>  
>      vfio_group = vfio_ap_get_group(vapdev, &local_err);
>      if (!vfio_group) {
> @@ -120,8 +117,8 @@ out_err:
>  
>  static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
>  {
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
>      VFIOGroup *group = vapdev->vdev.group;
>  
>      vfio_ap_put_device(vapdev);
> @@ -136,8 +133,8 @@ static Property vfio_ap_properties[] = {
>  static void vfio_ap_reset(DeviceState *dev)
>  {
>      int ret;
> -    APDevice *apdev = AP_DEVICE(dev);
> -    VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
>  
>      ret = ioctl(vapdev->vdev.fd, VFIO_DEVICE_RESET);
>      if (ret) {
> @@ -163,7 +160,6 @@ static void vfio_ap_class_init(ObjectClass *klass, void *data)
>      dc->unrealize = vfio_ap_unrealize;
>      dc->hotpluggable = false;
>      dc->reset = vfio_ap_reset;
> -    dc->bus_type = TYPE_AP_BUS;

Inheriting this to the parent class as opposed to specifying it here
makes sense. 

>  }
>  
>  static const TypeInfo vfio_ap_info = {
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> index 765e908..5f3c840 100644
> --- a/include/hw/s390x/ap-device.h
> +++ b/include/hw/s390x/ap-device.h
> @@ -19,4 +19,6 @@ typedef struct APDevice {
>  #define AP_DEVICE(obj) \
>      OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
>  
> +APDevice *s390_get_ap(void);
> +
>  #endif /* HW_S390X_AP_DEVICE_H */