Re: [Qemu-devel] [PATCH v9 0/6] s390x: vfio-ap: guest dedicated crypto adapters

Re: [Qemu-devel] [PATCH v9 0/6] s390x: vfio-ap: guest dedicated crypto adapters

[Tony Krowiak]

On 09/27/2018 05:28 AM, Cornelia Huck wrote:
> On Wed, 26 Sep 2018 18:54:34 -0400
> Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote:
> 
>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>
>> This patch series is the QEMU counterpart to the KVM/kernel support for
>> guest dedicated crypto adapters. The KVM/kernel model is built on the
>> VFIO mediated device framework and provides the infrastructure for
>> granting exclusive guest access to crypto devices installed on the linux
>> host. This patch series introduces a new QEMU command line option, QEMU
>> object model and CPU model features to exploit the KVM/kernel model.
>>
>> See the detailed specifications for AP virtualization provided by this
>> patch set in docs/vfio-ap.txt for a more complete discussion of the
>> design introduced by this patch series.
> 
> This seems to look sane in general. However, before I merge this:
> 
> - This needs to wait for a proper headers sync, which can only be done
>    after the Linux code hits upstream. Shouldn't be long, I guess (I can
>    queue on a staging branch while waiting.)
> - As I don't have the hardware, I cannot run more than some generic
>    "verify it does not break anything" testing. So, I'd like to see a
>    Tested-by by someone who actually does have access to the hardware.
> - I'd like to see some more review/acks from others. If other (minor)
>    things crop up, I'd prefer a respin (I don't want to juggle too many
>    "minor changes"...)

I am planning on a v10 series to address issues raised by Thomas, David
and you.

> 

Re: [Qemu-devel] [PATCH v9 2/6] s390x/cpumodel: Set up CPU model for AP device support

[Pierre Morel]

On 27/09/2018 00:54, Tony Krowiak wrote:
> A new CPU model feature and two new CPU model facilities are
> introduced to support AP devices for a KVM guest.
> 
> CPU model features:
> 
> 1. The S390_FEAT_AP CPU model feature indicates whether AP
>     instructions are available to the guest. This feature will
>     be enabled only if the AP instructions are available on the
>     linux host as determined by the availability of the
>     KVM_S390_VM_CRYPTO_ENABLE_APIE VM attribute which is exposed
>     by KVM only if the AP instructions are available on the
>     host.
> 
>     This feature must be turned on from userspace to execute AP
>     instructions on the KVM guest. The QEMU command line to turn
>     this feature on looks something like this:
> 
> 	qemu-system-s390x ... -cpu xxx,ap=on ...
> 
>     This feature will be supported for zEC12 and newer CPU models.
>     The feature will not be supported for older models because
>     there are few older systems on which to test and the older
>     crypto cards will be going out of service in the relatively
>     near future.
> 
> CPU model facilities:
> 
> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates whether the
>     AP Query Configuration Information (QCI) facility is available
>     to the guest as determined by whether the facility is available
>     on the host. This feature will be exposed by KVM only if the
>     QCI facility is installed on the host.
> 
> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates whether the AP
>     Facility Test (APFT) facility is available to the guest as
>     determined by whether the facility is available on the host.
>     This feature will be exposed by KVM only if APFT is installed
>     on the host.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   target/s390x/cpu_features.c     | 3 +++
>   target/s390x/cpu_features_def.h | 3 +++
>   target/s390x/cpu_models.c       | 2 ++
>   target/s390x/gen-features.c     | 3 +++
>   target/s390x/kvm.c              | 5 +++++
>   5 files changed, 16 insertions(+)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 172fb18df718..60cfeba48f4e 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -39,8 +39,10 @@ static const S390FeatDef s390_features[] = {
>       FEAT_INIT("srs", S390_FEAT_TYPE_STFL, 9, "Sense-running-status facility"),
>       FEAT_INIT("csske", S390_FEAT_TYPE_STFL, 10, "Conditional-SSKE facility"),
>       FEAT_INIT("ctop", S390_FEAT_TYPE_STFL, 11, "Configuration-topology facility"),
> +    FEAT_INIT("apqci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration Information facility"),
>       FEAT_INIT("ipter", S390_FEAT_TYPE_STFL, 13, "IPTE-range facility"),
>       FEAT_INIT("nonqks", S390_FEAT_TYPE_STFL, 14, "Nonquiescing key-setting facility"),
> +    FEAT_INIT("apft", S390_FEAT_TYPE_STFL, 15, "AP Facilities Test facility"),
>       FEAT_INIT("etf2", S390_FEAT_TYPE_STFL, 16, "Extended-translation facility 2"),
>       FEAT_INIT("msa-base", S390_FEAT_TYPE_STFL, 17, "Message-security-assist facility (excluding subfunctions)"),
>       FEAT_INIT("ldisp", S390_FEAT_TYPE_STFL, 18, "Long-displacement facility"),
> @@ -129,6 +131,7 @@ static const S390FeatDef s390_features[] = {
> 
>       FEAT_INIT_MISC("dateh2", "DAT-enhancement facility 2"),
>       FEAT_INIT_MISC("cmm", "Collaborative-memory-management facility"),
> +    FEAT_INIT_MISC("ap", "AP instructions installed"),
> 
>       FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>       FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> index ac2c947f30a8..5fc7e7bf0116 100644
> --- a/target/s390x/cpu_features_def.h
> +++ b/target/s390x/cpu_features_def.h
> @@ -27,8 +27,10 @@ typedef enum {
>       S390_FEAT_SENSE_RUNNING_STATUS,
>       S390_FEAT_CONDITIONAL_SSKE,
>       S390_FEAT_CONFIGURATION_TOPOLOGY,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>       S390_FEAT_IPTE_RANGE,
>       S390_FEAT_NONQ_KEY_SETTING,
> +    S390_FEAT_AP_FACILITIES_TEST,
>       S390_FEAT_EXTENDED_TRANSLATION_2,
>       S390_FEAT_MSA,
>       S390_FEAT_LONG_DISPLACEMENT,
> @@ -119,6 +121,7 @@ typedef enum {
>       /* Misc */
>       S390_FEAT_DAT_ENH_2,
>       S390_FEAT_CMM,
> +    S390_FEAT_AP,
> 
>       /* PLO */
>       S390_FEAT_PLO_CL,
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 265d25c937bb..7c253ff308c5 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -786,6 +786,8 @@ static void check_consistency(const S390CPUModel *model)
>           { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>           { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>           { 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 },
>       };
>       int i;
> 
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 384b61cd67b9..70015eaaf5df 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>       S390_FEAT_ADAPTER_INT_SUPPRESSION,
>       S390_FEAT_EDAT_2,
>       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
> +    S390_FEAT_AP_FACILITIES_TEST,
> +    S390_FEAT_AP,
>   };
> 
>   static uint16_t full_GEN12_GA2[] = {
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 348e8cc5467a..5277acd79a2c 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2291,6 +2291,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>           error_setg(errp, "KVM: host CPU model could not be identified");
>           return;
>       }
> +    /* for now, we can only provide the AP feature with HW support */
> +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO,
> +        KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
> +        set_bit(S390_FEAT_AP, model->features);
> +    }
>       /* strip of features that are not part of the maximum model */
>       bitmap_and(model->features, model->features, model->def->full_feat,
>                  S390_FEAT_MAX);
> 

Appart the last hunk (being moved as commented by david):

Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
Tested-by: Pierre Morel<pmorel@linux.ibm.com>


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

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 5/6] s390x/vfio: ap: Introduce VFIO AP device

[Tony Krowiak]

On 09/27/2018 09:56 AM, Thomas Huth wrote:
> On 2018-09-27 00:54, Tony Krowiak wrote:
>> Introduces a VFIO based AP device. The device is defined via
>> the QEMU command line by specifying:
>>
>>      -device vfio-ap,sysfsdev=<path-to-mediated-matrix-device>
>>
>> There may be only one vfio-ap device configured for a guest.
>>
>> The mediated matrix device is created by the VFIO AP device
>> driver by writing a UUID to a sysfs attribute file (see
>> docs/vfio-ap.txt). The mediated matrix device will be named
>> after the UUID. Symbolic links to the $uuid are created in
>> many places, so the path to the mediated matrix device $uuid
>> can be specified in any of the following ways:
>>
>> /sys/devices/vfio_ap/matrix/$uuid
>> /sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
>> /sys/bus/mdev/devices/$uuid
>> /sys/bus/mdev/drivers/vfio_mdev/$uuid
>>
>> When the vfio-ap device is realized, it acquires and opens the
>> VFIO iommu group to which the mediated matrix device is
>> bound. This causes a VFIO group notification event to be
>> signaled. The vfio_ap device driver's group notification
>> handler will get called at which time the device driver
>> will configure the the AP devices to which the guest will
>> be granted access.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
> [...]
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> new file mode 100644
>> index 000000000000..429988f23f98
>> --- /dev/null
>> +++ b/hw/vfio/ap.c
>> @@ -0,0 +1,181 @@
>> +/*
>> + * VFIO based AP matrix device assignment
>> + *
>> + * Copyright 2018 IBM Corp.
>> + * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
>> + *            Halil Pasic <pasic@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
>> + * directory.
>> + */
>> +
>> +#include <linux/vfio.h>
>> +#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"
>> +#include "qemu/error-report.h"
>> +#include "qemu/queue.h"
>> +#include "qemu/option.h"
>> +#include "qemu/config-file.h"
>> +#include "cpu.h"
>> +#include "kvm_s390x.h"
>> +#include "sysemu/sysemu.h"
>> +#include "hw/s390x/ap-bridge.h"
>> +#include "exec/address-spaces.h"
>> +
>> +#define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>> +
>> +typedef struct VFIOAPDevice {
>> +    APDevice apdev;
>> +    VFIODevice vdev;
>> +} VFIOAPDevice;
>> +
>> +static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>> +{
>> +    vdev->needs_reset = false;
>> +}
>> +
>> +/*
>> + * We don't need vfio_hot_reset_multi and vfio_eoi operations for
>> + * vfio-ap device now.
>> + */
>> +struct VFIODeviceOps vfio_ap_ops = {
>> +    .vfio_compute_needs_reset = vfio_ap_compute_needs_reset,
>> +};
>> +
>> +static void vfio_ap_put_device(VFIOAPDevice *vapdev)
>> +{
>> +    g_free(vapdev->vdev.name);
>> +    vfio_put_base_device(&vapdev->vdev);
>> +}
>> +
>> +static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp)
>> +{
>> +    char *tmp, group_path[PATH_MAX];
>> +    ssize_t len;
>> +    int groupid;
>> +
>> +    tmp = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev);
>> +    len = readlink(tmp, group_path, sizeof(group_path));
>> +    g_free(tmp);
>> +
>> +    if (len <= 0 || len >= sizeof(group_path)) {
>> +        error_setg(errp, "%s: no iommu_group found for %s",
>> +                   VFIO_AP_DEVICE_TYPE, vapdev->vdev.sysfsdev);
>> +        return NULL;
>> +    }
>> +
>> +    group_path[len] = 0;
> 
> You could maybe use g_file_read_link() instead to avoid the ugliness
> that is needed around readlink().

I will make that change.

> 
>> +    if (sscanf(basename(group_path), "%d", &groupid) != 1) {
>> +        error_setg(errp, "vfio: failed to read %s", group_path);
>> +        return NULL;
>> +    }
>> +
>> +    return vfio_get_group(groupid, &address_space_memory, errp);
>> +}
>> +
>> +static void vfio_ap_realize(DeviceState *dev, Error **errp)
>> +{
>> +    int ret;
>> +    char *mdevid;
>> +    Error *local_err = NULL;
>> +    VFIOGroup *vfio_group;
>> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> 
> IIRC DO_UPCAST should be avoided in new code. So this is now here the
> right place to finally use the AP_DEVICE() macro?

Will do.

> 
>> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
>> +
>> +    vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
> 
> Double assignment to vapdev.

Ah, yes ... it needs to go

> 
>> +    vfio_group = vfio_ap_get_group(vapdev, &local_err);
>> +    if (!vfio_group) {
>> +        goto out_err;
>> +    }
>> +
>> +    vapdev->vdev.ops = &vfio_ap_ops;
>> +    vapdev->vdev.type = VFIO_DEVICE_TYPE_AP;
>> +    mdevid = basename(vapdev->vdev.sysfsdev);
>> +    vapdev->vdev.name = g_strdup_printf("%s", mdevid);
> 
> g_strdup instead of g_strdup_printf should be sufficient here, shouldn't it?

Yes, it is sufficient, I'll change it.

> 
>> +    vapdev->vdev.dev = dev;
>> +
>> +    ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>> +    if (ret) {
>> +        goto out_get_dev_err;
>> +    }
>> +
>> +    /* Enable hardware to intepret AP instructions executed on the guest */
>> +    object_property_set_bool(OBJECT(qdev_get_machine()), true, "apie", NULL);
>> +
>> +    return;
>> +
>> +out_get_dev_err:
>> +    vfio_ap_put_device(vapdev);
>> +    vfio_put_group(vfio_group);
>> +out_err:
>> +    error_propagate(errp, local_err);
>> +}
> 
>   Thomas
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[Tony Krowiak]

On 09/28/2018 08:51 AM, Halil Pasic wrote:
> 
> 
> On 09/27/2018 02:52 PM, Cornelia Huck wrote:
>> On Thu, 27 Sep 2018 14:29:01 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 2018-09-27 00:54, Tony Krowiak wrote:
>>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>
>>>> Introduces the base object model for virtualizing AP devices.
>>>>
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>> ---
>>
>>>> +typedef struct APBridge {
>>>> +    SysBusDevice sysbus_dev;
>>>> +    bool css_dev_path;
>>>
>>> What is this css_dev_path variable good for? I don't see it used in any
>>> of the other patches?
>>> If you don't need it, I think you could get rid of this struct completely?
>>
>> Huh, now I remember complaining about it before. Looks like a
>> copy-and-paste from the css bridge; that variable is used for compat
>> handling there (and should be ditched here).
>>
> 
> Yes, we definitively discussed this before. And yes, it is a copy-paste
> error.
> 
> 
>>>
>>>> +} APBridge;
>>>> +
>>>> +#define TYPE_AP_BRIDGE "ap-bridge"
>>>> +#define AP_BRIDGE(obj) \
>>>> +    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
>>>> +
>>>> +typedef struct APBus {
>>>> +    BusState parent_obj;
>>>> +} APBus;
>>>> +
>>>> +#define TYPE_AP_BUS "ap-bus"
>>>> +#define AP_BUS(obj) \
>>>> +     OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
>>>
>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
>>> struct APBus.
>>
>> If there's nothing interesting to put in these inherited structures,
>> probably yes.
>>
> 
> I was under impression that this is how inheritance/subtyping is done in
> QEMU. Was probably added for the sake of interface completeness.
> 
>>>
>>>> +void s390_init_ap(void);
>>>> +
>>>> +#endif
>>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
>>>> new file mode 100644
>>>> index 000000000000..693df90cc041
>>>> --- /dev/null
>>>> +++ b/include/hw/s390x/ap-device.h
>>>> @@ -0,0 +1,38 @@
>>>> +/*
>>>> + * Adjunct Processor (AP) matrix device interfaces
>>>> + *
>>>> + * Copyright 2018 IBM Corp.
>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
>>>> + * directory.
>>>> + */
>>>> +#ifndef HW_S390X_AP_DEVICE_H
>>>> +#define HW_S390X_AP_DEVICE_H
>>>> +
>>>> +#define AP_DEVICE_TYPE       "ap-device"
>>>> +
>>>> +typedef struct APDevice {
>>>> +    DeviceState parent_obj;
>>>> +} APDevice;
>>>> +
>>>> +typedef struct APDeviceClass {
>>>> +    DeviceClass parent_class;
>>>> +} APDeviceClass;
>>>> +
>>>> +static inline APDevice *to_ap_dev(DeviceState *dev)
>>>> +{
>>>> +    return container_of(dev, APDevice, parent_obj);
>>>> +}
>>>> +
>>>> +#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)
>>>
>>> Do you really need any of these definitions except AP_DEVICE_TYPE ?
>>
>> Same here, I think.
>>
> 
> I don't think the qom utility/casting macros will ever be used. I'm
> fine with removing what is not needed.
> 
> Regards,
> Halil

Per Thomas's suggestions and Connie's concurrence, I am going to remove
the macros as well as the structures they reference. The ap_bridge.h
will be relegated to defining the bridge and bus types and serve as a
container for AP bridge and bus definitions that might be needed in the
future.

> 
>   
> 

Re: [Qemu-devel] [PATCH v9 3/6] s390x/kvm: enable AP instruction interpretation for guest

[Pierre Morel]

On 27/09/2018 09:52, David Hildenbrand wrote:
> On 27/09/2018 00:54, Tony Krowiak wrote:
>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>
>> Let's use the KVM_SET_DEVICE_ATTR ioctl to enable hardware
>> interpretation of AP instructions executed on the guest.
>> If the S390_FEAT_AP feature is switched on for the guest,
>> AP instructions must be interpreted by default; otherwise,
>> they will be intercepted.
>>
>> This attribute setting may be overridden by a device. For example,
>> a device may want to provide AP instructions to the guest (i.e.,
>> S390_FEAT_AP turned on), but it may want to emulate them. In this
>> case, the AP instructions executed on the guest must be
>> intercepted; so when the device is realized, it must disable
>> interpretation.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   target/s390x/kvm.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 5277acd79a2c..d55d24abfd78 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2301,6 +2301,16 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>                  S390_FEAT_MAX);
>>   }
>>   
>> +static void kvm_s390_configure_apie(bool interpret)
>> +{
>> +    uint64_t attr = interpret ? KVM_S390_VM_CRYPTO_ENABLE_APIE :
>> +                                KVM_S390_VM_CRYPTO_DISABLE_APIE;
>> +
>> +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, attr)) {
>> +            kvm_s390_set_attr(attr);
> 
> Wrong indentation, apart from that
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
>> +    }
>> +}
>> +
>>   void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
>>   {
>>       struct kvm_s390_vm_cpu_processor prop  = {
>> @@ -2350,6 +2360,10 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
>>       if (test_bit(S390_FEAT_CMM, model->features)) {
>>           kvm_s390_enable_cmma();
>>       }
>> +
>> +    if (test_bit(S390_FEAT_AP, model->features)) {
>> +        kvm_s390_configure_apie(true);
>> +    }
>>   }
>>   
>>   void kvm_s390_restart_interrupt(S390CPU *cpu)
>>
> 
> 


Tested-by: Pierre Morel<pmorel@linux.ibm.com>


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

Re: [Qemu-devel] [PATCH v9 5/6] s390x/vfio: ap: Introduce VFIO AP device

[Pierre Morel]

On 27/09/2018 00:54, Tony Krowiak wrote:
> Introduces a VFIO based AP device. The device is defined via
> the QEMU command line by specifying:
> 
>      -device vfio-ap,sysfsdev=<path-to-mediated-matrix-device>
> 
> There may be only one vfio-ap device configured for a guest.
> 
> The mediated matrix device is created by the VFIO AP device
> driver by writing a UUID to a sysfs attribute file (see
> docs/vfio-ap.txt). The mediated matrix device will be named
> after the UUID. Symbolic links to the $uuid are created in
> many places, so the path to the mediated matrix device $uuid
> can be specified in any of the following ways:
> 
> /sys/devices/vfio_ap/matrix/$uuid
> /sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
> /sys/bus/mdev/devices/$uuid
> /sys/bus/mdev/drivers/vfio_mdev/$uuid
> 
> When the vfio-ap device is realized, it acquires and opens the
> VFIO iommu group to which the mediated matrix device is
> bound. This causes a VFIO group notification event to be
> signaled. The vfio_ap device driver's group notification
> handler will get called at which time the device driver
> will configure the the AP devices to which the guest will
> be granted access.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   MAINTAINERS                       |   1 +
>   default-configs/s390x-softmmu.mak |   1 +
>   hw/vfio/Makefile.objs             |   1 +
>   hw/vfio/ap.c                      | 181 ++++++++++++++++++++++++++++++
>   include/hw/vfio/vfio-common.h     |   1 +
>   5 files changed, 185 insertions(+)
>   create mode 100644 hw/vfio/ap.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 97e8ed808bc0..29041da69237 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1209,6 +1209,7 @@ F: hw/s390x/ap-device.c
>   F: hw/s390x/ap-bridge.c
>   F: include/hw/s390x/ap-device.h
>   F: include/hw/s390x/ap-bridge.h
> +F: hw/vfio/ap.c
>   L: qemu-s390x@nongnu.org
> 
>   vhost
> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> index d6b67d50f0e4..5eef37592451 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -7,3 +7,4 @@ CONFIG_S390_FLIC=y
>   CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
>   CONFIG_VFIO_CCW=$(CONFIG_LINUX)
>   CONFIG_WDT_DIAG288=y
> +CONFIG_VFIO_AP=$(CONFIG_LINUX)
> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> index a2e7a0a7cf02..8b3f664d85f7 100644
> --- a/hw/vfio/Makefile.objs
> +++ b/hw/vfio/Makefile.objs
> @@ -6,4 +6,5 @@ obj-$(CONFIG_SOFTMMU) += platform.o
>   obj-$(CONFIG_VFIO_XGMAC) += calxeda-xgmac.o
>   obj-$(CONFIG_VFIO_AMD_XGBE) += amd-xgbe.o
>   obj-$(CONFIG_SOFTMMU) += spapr.o
> +obj-$(CONFIG_VFIO_AP) += ap.o
>   endif
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> new file mode 100644
> index 000000000000..429988f23f98
> --- /dev/null
> +++ b/hw/vfio/ap.c
> @@ -0,0 +1,181 @@
> +/*
> + * VFIO based AP matrix device assignment
> + *
> + * Copyright 2018 IBM Corp.
> + * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
> + *            Halil Pasic <pasic@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
> + * directory.
> + */
> +
> +#include <linux/vfio.h>
> +#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"
> +#include "qemu/error-report.h"
> +#include "qemu/queue.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
> +#include "cpu.h"
> +#include "kvm_s390x.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/s390x/ap-bridge.h"
> +#include "exec/address-spaces.h"
> +
> +#define VFIO_AP_DEVICE_TYPE      "vfio-ap"
> +
> +typedef struct VFIOAPDevice {
> +    APDevice apdev;
> +    VFIODevice vdev;
> +} VFIOAPDevice;
> +
> +static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> +{
> +    vdev->needs_reset = false;
> +}
> +
> +/*
> + * We don't need vfio_hot_reset_multi and vfio_eoi operations for
> + * vfio-ap device now.
> + */
> +struct VFIODeviceOps vfio_ap_ops = {
> +    .vfio_compute_needs_reset = vfio_ap_compute_needs_reset,
> +};
> +
> +static void vfio_ap_put_device(VFIOAPDevice *vapdev)
> +{
> +    g_free(vapdev->vdev.name);
> +    vfio_put_base_device(&vapdev->vdev);
> +}
> +
> +static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp)
> +{
> +    char *tmp, group_path[PATH_MAX];
> +    ssize_t len;
> +    int groupid;
> +
> +    tmp = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev);
> +    len = readlink(tmp, group_path, sizeof(group_path));
> +    g_free(tmp);
> +
> +    if (len <= 0 || len >= sizeof(group_path)) {
> +        error_setg(errp, "%s: no iommu_group found for %s",
> +                   VFIO_AP_DEVICE_TYPE, vapdev->vdev.sysfsdev);
> +        return NULL;
> +    }
> +
> +    group_path[len] = 0;
> +
> +    if (sscanf(basename(group_path), "%d", &groupid) != 1) {
> +        error_setg(errp, "vfio: failed to read %s", group_path);
> +        return NULL;
> +    }
> +
> +    return vfio_get_group(groupid, &address_space_memory, errp);
> +}
> +
> +static void vfio_ap_realize(DeviceState *dev, Error **errp)
> +{
> +    int ret;
> +    char *mdevid;
> +    Error *local_err = NULL;
> +    VFIOGroup *vfio_group;
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
> +
> +    vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
> +
> +    vfio_group = vfio_ap_get_group(vapdev, &local_err);
> +    if (!vfio_group) {
> +        goto out_err;
> +    }
> +
> +    vapdev->vdev.ops = &vfio_ap_ops;
> +    vapdev->vdev.type = VFIO_DEVICE_TYPE_AP;
> +    mdevid = basename(vapdev->vdev.sysfsdev);
> +    vapdev->vdev.name = g_strdup_printf("%s", mdevid);
> +    vapdev->vdev.dev = dev;
> +
> +    ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
> +    if (ret) {
> +        goto out_get_dev_err;
> +    }
> +
> +    /* Enable hardware to intepret AP instructions executed on the guest */
> +    object_property_set_bool(OBJECT(qdev_get_machine()), true, "apie", NULL);
> +
> +    return;
> +
> +out_get_dev_err:
> +    vfio_ap_put_device(vapdev);
> +    vfio_put_group(vfio_group);
> +out_err:
> +    error_propagate(errp, local_err);
> +}
> +
> +static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
> +{
> +    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);
> +    vfio_put_group(group);
> +}
> +
> +static Property vfio_ap_properties[] = {
> +    DEFINE_PROP_STRING("sysfsdev", VFIOAPDevice, vdev.sysfsdev),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void vfio_ap_reset(DeviceState *dev)
> +{
> +    int ret;
> +    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) {
> +        error_report("%s: failed to reset %s device: %s", __func__,
> +                     vapdev->vdev.name, strerror(ret));
> +    }
> +}
> +
> +static const VMStateDescription vfio_ap_vmstate = {
> +    .name = VFIO_AP_DEVICE_TYPE,
> +    .unmigratable = 1,
> +};
> +
> +static void vfio_ap_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = vfio_ap_properties;
> +    dc->vmsd = &vfio_ap_vmstate;
> +    dc->desc = "VFIO-based AP device assignment";
> +    dc->realize = vfio_ap_realize;
> +    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 = {
> +    .name = VFIO_AP_DEVICE_TYPE,
> +    .parent = AP_DEVICE_TYPE,
> +    .instance_size = sizeof(VFIOAPDevice),
> +    .class_init = vfio_ap_class_init,
> +};
> +
> +static void vfio_ap_type_init(void)
> +{
> +    type_register_static(&vfio_ap_info);
> +}
> +
> +type_init(vfio_ap_type_init)
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 821def05658f..6be9a93f611b 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -37,6 +37,7 @@ enum {
>       VFIO_DEVICE_TYPE_PCI = 0,
>       VFIO_DEVICE_TYPE_PLATFORM = 1,
>       VFIO_DEVICE_TYPE_CCW = 2,
> +    VFIO_DEVICE_TYPE_AP = 3,
>   };
> 
>   typedef struct VFIOMmap {
> 

The functionality is working as expected.

Tested-by: Pierre Morel<pmorel@linux.ibm.com>


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

Re: [Qemu-devel] [PATCH v9 2/6] s390x/cpumodel: Set up CPU model for AP device support

[Pierre Morel]

On 27/09/2018 00:54, Tony Krowiak wrote:
> A new CPU model feature and two new CPU model facilities are
> introduced to support AP devices for a KVM guest.
> 
> CPU model features:
> 
> 1. The S390_FEAT_AP CPU model feature indicates whether AP
>     instructions are available to the guest. This feature will
>     be enabled only if the AP instructions are available on the
>     linux host as determined by the availability of the
>     KVM_S390_VM_CRYPTO_ENABLE_APIE VM attribute which is exposed
>     by KVM only if the AP instructions are available on the
>     host.
> 
>     This feature must be turned on from userspace to execute AP
>     instructions on the KVM guest. The QEMU command line to turn
>     this feature on looks something like this:
> 
> 	qemu-system-s390x ... -cpu xxx,ap=on ...
> 
>     This feature will be supported for zEC12 and newer CPU models.
>     The feature will not be supported for older models because
>     there are few older systems on which to test and the older
>     crypto cards will be going out of service in the relatively
>     near future.
> 
> CPU model facilities:
> 
> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates whether the
>     AP Query Configuration Information (QCI) facility is available
>     to the guest as determined by whether the facility is available
>     on the host. This feature will be exposed by KVM only if the
>     QCI facility is installed on the host.
> 
> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates whether the AP
>     Facility Test (APFT) facility is available to the guest as
>     determined by whether the facility is available on the host.
>     This feature will be exposed by KVM only if APFT is installed
>     on the host.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   target/s390x/cpu_features.c     | 3 +++
>   target/s390x/cpu_features_def.h | 3 +++
>   target/s390x/cpu_models.c       | 2 ++
>   target/s390x/gen-features.c     | 3 +++
>   target/s390x/kvm.c              | 5 +++++
>   5 files changed, 16 insertions(+)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 172fb18df718..60cfeba48f4e 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -39,8 +39,10 @@ static const S390FeatDef s390_features[] = {
>       FEAT_INIT("srs", S390_FEAT_TYPE_STFL, 9, "Sense-running-status facility"),
>       FEAT_INIT("csske", S390_FEAT_TYPE_STFL, 10, "Conditional-SSKE facility"),
>       FEAT_INIT("ctop", S390_FEAT_TYPE_STFL, 11, "Configuration-topology facility"),
> +    FEAT_INIT("apqci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration Information facility"),
>       FEAT_INIT("ipter", S390_FEAT_TYPE_STFL, 13, "IPTE-range facility"),
>       FEAT_INIT("nonqks", S390_FEAT_TYPE_STFL, 14, "Nonquiescing key-setting facility"),
> +    FEAT_INIT("apft", S390_FEAT_TYPE_STFL, 15, "AP Facilities Test facility"),
>       FEAT_INIT("etf2", S390_FEAT_TYPE_STFL, 16, "Extended-translation facility 2"),
>       FEAT_INIT("msa-base", S390_FEAT_TYPE_STFL, 17, "Message-security-assist facility (excluding subfunctions)"),
>       FEAT_INIT("ldisp", S390_FEAT_TYPE_STFL, 18, "Long-displacement facility"),
> @@ -129,6 +131,7 @@ static const S390FeatDef s390_features[] = {
> 
>       FEAT_INIT_MISC("dateh2", "DAT-enhancement facility 2"),
>       FEAT_INIT_MISC("cmm", "Collaborative-memory-management facility"),
> +    FEAT_INIT_MISC("ap", "AP instructions installed"),
> 
>       FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>       FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> index ac2c947f30a8..5fc7e7bf0116 100644
> --- a/target/s390x/cpu_features_def.h
> +++ b/target/s390x/cpu_features_def.h
> @@ -27,8 +27,10 @@ typedef enum {
>       S390_FEAT_SENSE_RUNNING_STATUS,
>       S390_FEAT_CONDITIONAL_SSKE,
>       S390_FEAT_CONFIGURATION_TOPOLOGY,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>       S390_FEAT_IPTE_RANGE,
>       S390_FEAT_NONQ_KEY_SETTING,
> +    S390_FEAT_AP_FACILITIES_TEST,
>       S390_FEAT_EXTENDED_TRANSLATION_2,
>       S390_FEAT_MSA,
>       S390_FEAT_LONG_DISPLACEMENT,
> @@ -119,6 +121,7 @@ typedef enum {
>       /* Misc */
>       S390_FEAT_DAT_ENH_2,
>       S390_FEAT_CMM,
> +    S390_FEAT_AP,
> 
>       /* PLO */
>       S390_FEAT_PLO_CL,
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 265d25c937bb..7c253ff308c5 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -786,6 +786,8 @@ static void check_consistency(const S390CPUModel *model)
>           { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>           { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>           { 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 },
>       };
>       int i;
> 
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 384b61cd67b9..70015eaaf5df 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>       S390_FEAT_ADAPTER_INT_SUPPRESSION,
>       S390_FEAT_EDAT_2,
>       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
> +    S390_FEAT_AP_FACILITIES_TEST,
> +    S390_FEAT_AP,
>   };
> 
>   static uint16_t full_GEN12_GA2[] = {
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 348e8cc5467a..5277acd79a2c 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2291,6 +2291,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>           error_setg(errp, "KVM: host CPU model could not be identified");
>           return;
>       }
> +    /* for now, we can only provide the AP feature with HW support */
> +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO,
> +        KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
> +        set_bit(S390_FEAT_AP, model->features);
> +    }
>       /* strip of features that are not part of the maximum model */
>       bitmap_and(model->features, model->features, model->def->full_feat,
>                  S390_FEAT_MAX);
> 


With all patches applied (as David said), functionality is OK.

Tested-by: Pierre Morel<pmorel@linux.ibm.com>



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

Re: [Qemu-devel] [PATCH v9 2/6] s390x/cpumodel: Set up CPU model for AP device support

[Pierre Morel]

On 27/09/2018 00:54, Tony Krowiak wrote:
> A new CPU model feature and two new CPU model facilities are
> introduced to support AP devices for a KVM guest.
> 
> CPU model features:
> 
> 1. The S390_FEAT_AP CPU model feature indicates whether AP
>     instructions are available to the guest. This feature will
>     be enabled only if the AP instructions are available on the
>     linux host as determined by the availability of the
>     KVM_S390_VM_CRYPTO_ENABLE_APIE VM attribute which is exposed
>     by KVM only if the AP instructions are available on the
>     host.
> 
>     This feature must be turned on from userspace to execute AP
>     instructions on the KVM guest. The QEMU command line to turn
>     this feature on looks something like this:
> 
> 	qemu-system-s390x ... -cpu xxx,ap=on ...
> 
>     This feature will be supported for zEC12 and newer CPU models.
>     The feature will not be supported for older models because
>     there are few older systems on which to test and the older
>     crypto cards will be going out of service in the relatively
>     near future.
> 
> CPU model facilities:
> 
> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates whether the
>     AP Query Configuration Information (QCI) facility is available
>     to the guest as determined by whether the facility is available
>     on the host. This feature will be exposed by KVM only if the
>     QCI facility is installed on the host.
> 
> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates whether the AP
>     Facility Test (APFT) facility is available to the guest as
>     determined by whether the facility is available on the host.
>     This feature will be exposed by KVM only if APFT is installed
>     on the host.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   target/s390x/cpu_features.c     | 3 +++
>   target/s390x/cpu_features_def.h | 3 +++
>   target/s390x/cpu_models.c       | 2 ++
>   target/s390x/gen-features.c     | 3 +++
>   target/s390x/kvm.c              | 5 +++++
>   5 files changed, 16 insertions(+)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 172fb18df718..60cfeba48f4e 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -39,8 +39,10 @@ static const S390FeatDef s390_features[] = {
>       FEAT_INIT("srs", S390_FEAT_TYPE_STFL, 9, "Sense-running-status facility"),
>       FEAT_INIT("csske", S390_FEAT_TYPE_STFL, 10, "Conditional-SSKE facility"),
>       FEAT_INIT("ctop", S390_FEAT_TYPE_STFL, 11, "Configuration-topology facility"),
> +    FEAT_INIT("apqci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration Information facility"),
>       FEAT_INIT("ipter", S390_FEAT_TYPE_STFL, 13, "IPTE-range facility"),
>       FEAT_INIT("nonqks", S390_FEAT_TYPE_STFL, 14, "Nonquiescing key-setting facility"),
> +    FEAT_INIT("apft", S390_FEAT_TYPE_STFL, 15, "AP Facilities Test facility"),
>       FEAT_INIT("etf2", S390_FEAT_TYPE_STFL, 16, "Extended-translation facility 2"),
>       FEAT_INIT("msa-base", S390_FEAT_TYPE_STFL, 17, "Message-security-assist facility (excluding subfunctions)"),
>       FEAT_INIT("ldisp", S390_FEAT_TYPE_STFL, 18, "Long-displacement facility"),
> @@ -129,6 +131,7 @@ static const S390FeatDef s390_features[] = {
> 
>       FEAT_INIT_MISC("dateh2", "DAT-enhancement facility 2"),
>       FEAT_INIT_MISC("cmm", "Collaborative-memory-management facility"),
> +    FEAT_INIT_MISC("ap", "AP instructions installed"),
> 
>       FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>       FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> index ac2c947f30a8..5fc7e7bf0116 100644
> --- a/target/s390x/cpu_features_def.h
> +++ b/target/s390x/cpu_features_def.h
> @@ -27,8 +27,10 @@ typedef enum {
>       S390_FEAT_SENSE_RUNNING_STATUS,
>       S390_FEAT_CONDITIONAL_SSKE,
>       S390_FEAT_CONFIGURATION_TOPOLOGY,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>       S390_FEAT_IPTE_RANGE,
>       S390_FEAT_NONQ_KEY_SETTING,
> +    S390_FEAT_AP_FACILITIES_TEST,
>       S390_FEAT_EXTENDED_TRANSLATION_2,
>       S390_FEAT_MSA,
>       S390_FEAT_LONG_DISPLACEMENT,
> @@ -119,6 +121,7 @@ typedef enum {
>       /* Misc */
>       S390_FEAT_DAT_ENH_2,
>       S390_FEAT_CMM,
> +    S390_FEAT_AP,
> 
>       /* PLO */
>       S390_FEAT_PLO_CL,
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 265d25c937bb..7c253ff308c5 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -786,6 +786,8 @@ static void check_consistency(const S390CPUModel *model)
>           { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>           { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>           { 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 },
>       };
>       int i;
> 
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 384b61cd67b9..70015eaaf5df 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>       S390_FEAT_ADAPTER_INT_SUPPRESSION,
>       S390_FEAT_EDAT_2,
>       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
> +    S390_FEAT_AP_FACILITIES_TEST,
> +    S390_FEAT_AP,
>   };
> 
>   static uint16_t full_GEN12_GA2[] = {
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 348e8cc5467a..5277acd79a2c 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2291,6 +2291,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>           error_setg(errp, "KVM: host CPU model could not be identified");
>           return;
>       }
> +    /* for now, we can only provide the AP feature with HW support */
> +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO,
> +        KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
> +        set_bit(S390_FEAT_AP, model->features);
> +    }
>       /* strip of features that are not part of the maximum model */
>       bitmap_and(model->features, model->features, model->def->full_feat,
>                  S390_FEAT_MAX);
> 

(Sorry for multiple posting, I missed all the CCs)

With all patches applied (as David said), functionality is OK.

Tested-by: Pierre Morel<pmorel@linux.ibm.com>

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

Re: [Qemu-devel] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[Pierre Morel]

On 27/09/2018 00:54, Tony Krowiak wrote:
> From: Tony Krowiak <akrowiak@linux.ibm.com>
> 
> Introduces the base object model for virtualizing AP devices.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   MAINTAINERS                  | 12 ++++++
>   hw/s390x/Makefile.objs       |  2 +
>   hw/s390x/ap-bridge.c         | 81 ++++++++++++++++++++++++++++++++++++
>   hw/s390x/ap-device.c         | 39 +++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c   |  4 ++
>   include/hw/s390x/ap-bridge.h | 37 ++++++++++++++++
>   include/hw/s390x/ap-device.h | 38 +++++++++++++++++
>   7 files changed, 213 insertions(+)
>   create mode 100644 hw/s390x/ap-bridge.c
>   create mode 100644 hw/s390x/ap-device.c
>   create mode 100644 include/hw/s390x/ap-bridge.h
>   create mode 100644 include/hw/s390x/ap-device.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d12518c08f10..97e8ed808bc0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1199,6 +1199,18 @@ F: include/hw/s390x/s390-ccw.h
>   T: git git://github.com/cohuck/qemu.git s390-next
>   L: qemu-s390x@nongnu.org
> 
> +vfio-ap
> +M: Christian Borntraeger <borntraeger@de.ibm.com>
> +M: Tony Krowiak <akrowiak@linux.ibm.com>
> +M: Halil Pasic <pasic@linux.ibm.com>
> +M: Pierre Morel <pmorel@linux.ibm.com>
> +S: Supported
> +F: hw/s390x/ap-device.c
> +F: hw/s390x/ap-bridge.c
> +F: include/hw/s390x/ap-device.h
> +F: include/hw/s390x/ap-bridge.h
> +L: qemu-s390x@nongnu.org
> +
>   vhost
>   M: Michael S. Tsirkin <mst@redhat.com>
>   S: Supported
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 93282f7c593c..add89b150d90 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -20,3 +20,5 @@ obj-$(CONFIG_TCG) += tod-qemu.o
>   obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>   obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
>   obj-y += s390-ccw.o
> +obj-y += ap-device.o
> +obj-y += ap-bridge.o
> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
> new file mode 100644
> index 000000000000..8564dfa96ee7
> --- /dev/null
> +++ b/hw/s390x/ap-bridge.c
> @@ -0,0 +1,81 @@
> +/*
> + * ap bridge
> + *
> + * Copyright 2018 IBM Corp.
> + * Author(s): Halil Pasic <pasic@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
> + * directory.
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/sysbus.h"
> +#include "qemu/bitops.h"
> +#include "hw/s390x/ap-bridge.h"
> +#include "cpu.h"
> +
> +static char *vfio_ap_bus_get_dev_path(DeviceState *dev)
> +{
> +    /* at most one */
> +    return g_strdup_printf("/1");
> +}
> +
> +static void vfio_ap_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    BusClass *k = BUS_CLASS(klass);
> +
> +    k->get_dev_path = vfio_ap_bus_get_dev_path;
> +    /* More than one vfio-ap device does not make sense */
> +    k->max_dev = 1;
> +}
> +
> +static const TypeInfo vfio_ap_bus_info = {
> +    .name = TYPE_AP_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(APBus),
> +    .class_init = vfio_ap_bus_class_init,
> +};
> +
> +void s390_init_ap(void)
> +{
> +    DeviceState *dev;
> +
> +    /* If no AP instructions then no need for AP bridge */
> +    if (!s390_has_feat(S390_FEAT_AP)) {
> +        return;
> +    }
> +
> +    /* Create bridge device */
> +    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
> +    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
> +                              OBJECT(dev), NULL);
> +    qdev_init_nofail(dev);
> +
> +    /* Create bus on bridge device */
> +    qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS);
> + }
> +
> +
> +
> +static void ap_bridge_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +}
> +
> +static const TypeInfo ap_bridge_info = {
> +    .name          = TYPE_AP_BRIDGE,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(APBridge),
> +    .class_init    = ap_bridge_class_init,
> +};
> +
> +static void ap_register(void)
> +{
> +    type_register_static(&ap_bridge_info);
> +    type_register_static(&vfio_ap_bus_info);
> +}
> +
> +type_init(ap_register)
> diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
> new file mode 100644
> index 000000000000..3cd4bae52591
> --- /dev/null
> +++ b/hw/s390x/ap-device.c
> @@ -0,0 +1,39 @@
> +/*
> + * Adjunct Processor (AP) matrix device
> + *
> + * Copyright 2018 IBM Corp.
> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
> + * directory.
> + */
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "hw/qdev.h"
> +#include "hw/s390x/ap-device.h"
> +
> +static void ap_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->desc = "AP device class";
> +    dc->hotpluggable = false;
> +}
> +
> +static const TypeInfo ap_device_info = {
> +    .name = AP_DEVICE_TYPE,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(APDevice),
> +    .class_size = sizeof(APDeviceClass),
> +    .class_init = ap_class_init,
> +    .abstract = true,
> +};
> +
> +static void ap_device_register(void)
> +{
> +    type_register_static(&ap_device_info);
> +}
> +
> +type_init(ap_device_register)
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f0f7fdcaddf2..3c100c24f3e8 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -32,6 +32,7 @@
>   #include "ipl.h"
>   #include "hw/s390x/s390-virtio-ccw.h"
>   #include "hw/s390x/css-bridge.h"
> +#include "hw/s390x/ap-bridge.h"
>   #include "migration/register.h"
>   #include "cpu_models.h"
>   #include "hw/nmi.h"
> @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
>       /* init the SIGP facility */
>       s390_init_sigp();
> 
> +    /* create AP bridge and bus(es) */
> +    s390_init_ap();
> +
>       /* get a BUS */
>       css_bus = virtual_css_bus_init();
>       s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline,
> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
> new file mode 100644
> index 000000000000..b6ca6ae4ab17
> --- /dev/null
> +++ b/include/hw/s390x/ap-bridge.h
> @@ -0,0 +1,37 @@
> +/*
> + * ap bridge
> + *
> + * Copyright 2018 IBM Corp.
> + * Author(s): Halil Pasic <pasic@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
> + * directory.
> + */
> +
> +#ifndef HW_S390X_AP_BRIDGE_H
> +#define HW_S390X_AP_BRIDGE_H
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +
> +typedef struct APBridge {
> +    SysBusDevice sysbus_dev;
> +    bool css_dev_path;
> +} APBridge;
> +
> +#define TYPE_AP_BRIDGE "ap-bridge"
> +#define AP_BRIDGE(obj) \
> +    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
> +
> +typedef struct APBus {
> +    BusState parent_obj;
> +} APBus;
> +
> +#define TYPE_AP_BUS "ap-bus"
> +#define AP_BUS(obj) \
> +     OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
> +
> +void s390_init_ap(void);
> +
> +#endif
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> new file mode 100644
> index 000000000000..693df90cc041
> --- /dev/null
> +++ b/include/hw/s390x/ap-device.h
> @@ -0,0 +1,38 @@
> +/*
> + * Adjunct Processor (AP) matrix device interfaces
> + *
> + * Copyright 2018 IBM Corp.
> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
> + * directory.
> + */
> +#ifndef HW_S390X_AP_DEVICE_H
> +#define HW_S390X_AP_DEVICE_H
> +
> +#define AP_DEVICE_TYPE       "ap-device"
> +
> +typedef struct APDevice {
> +    DeviceState parent_obj;
> +} APDevice;
> +
> +typedef struct APDeviceClass {
> +    DeviceClass parent_class;
> +} APDeviceClass;
> +
> +static inline APDevice *to_ap_dev(DeviceState *dev)
> +{
> +    return container_of(dev, APDevice, parent_obj);
> +}
> +
> +#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)
> +
> +#endif /* HW_S390X_AP_DEVICE_H */
> 


Appart of the current discussions on the implementation:
Bus and devices appear correctly on the qtree.


Tested-by: Pierre Morel<pmorel@linux.ibm.com>

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

Re: [Qemu-devel] [PATCH v9 5/6] s390x/vfio: ap: Introduce VFIO AP device

[Pierre Morel]

On 27/09/2018 00:54, Tony Krowiak wrote:
> Introduces a VFIO based AP device. The device is defined via
> the QEMU command line by specifying:
> 
>      -device vfio-ap,sysfsdev=<path-to-mediated-matrix-device>
> 
> There may be only one vfio-ap device configured for a guest.
> 
> The mediated matrix device is created by the VFIO AP device
> driver by writing a UUID to a sysfs attribute file (see
> docs/vfio-ap.txt). The mediated matrix device will be named
> after the UUID. Symbolic links to the $uuid are created in
> many places, so the path to the mediated matrix device $uuid
> can be specified in any of the following ways:
> 
> /sys/devices/vfio_ap/matrix/$uuid
> /sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
> /sys/bus/mdev/devices/$uuid
> /sys/bus/mdev/drivers/vfio_mdev/$uuid
> 
> When the vfio-ap device is realized, it acquires and opens the
> VFIO iommu group to which the mediated matrix device is
> bound. This causes a VFIO group notification event to be
> signaled. The vfio_ap device driver's group notification
> handler will get called at which time the device driver
> will configure the the AP devices to which the guest will
> be granted access.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   MAINTAINERS                       |   1 +
>   default-configs/s390x-softmmu.mak |   1 +
>   hw/vfio/Makefile.objs             |   1 +
>   hw/vfio/ap.c                      | 181 ++++++++++++++++++++++++++++++
>   include/hw/vfio/vfio-common.h     |   1 +
>   5 files changed, 185 insertions(+)
>   create mode 100644 hw/vfio/ap.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 97e8ed808bc0..29041da69237 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1209,6 +1209,7 @@ F: hw/s390x/ap-device.c
>   F: hw/s390x/ap-bridge.c
>   F: include/hw/s390x/ap-device.h
>   F: include/hw/s390x/ap-bridge.h
> +F: hw/vfio/ap.c
>   L: qemu-s390x@nongnu.org
> 
>   vhost
> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> index d6b67d50f0e4..5eef37592451 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -7,3 +7,4 @@ CONFIG_S390_FLIC=y
>   CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
>   CONFIG_VFIO_CCW=$(CONFIG_LINUX)
>   CONFIG_WDT_DIAG288=y
> +CONFIG_VFIO_AP=$(CONFIG_LINUX)
> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> index a2e7a0a7cf02..8b3f664d85f7 100644
> --- a/hw/vfio/Makefile.objs
> +++ b/hw/vfio/Makefile.objs
> @@ -6,4 +6,5 @@ obj-$(CONFIG_SOFTMMU) += platform.o
>   obj-$(CONFIG_VFIO_XGMAC) += calxeda-xgmac.o
>   obj-$(CONFIG_VFIO_AMD_XGBE) += amd-xgbe.o
>   obj-$(CONFIG_SOFTMMU) += spapr.o
> +obj-$(CONFIG_VFIO_AP) += ap.o
>   endif
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> new file mode 100644
> index 000000000000..429988f23f98
> --- /dev/null
> +++ b/hw/vfio/ap.c
> @@ -0,0 +1,181 @@
> +/*
> + * VFIO based AP matrix device assignment
> + *
> + * Copyright 2018 IBM Corp.
> + * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
> + *            Halil Pasic <pasic@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
> + * directory.
> + */
> +
> +#include <linux/vfio.h>
> +#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"
> +#include "qemu/error-report.h"
> +#include "qemu/queue.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
> +#include "cpu.h"
> +#include "kvm_s390x.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/s390x/ap-bridge.h"
> +#include "exec/address-spaces.h"
> +
> +#define VFIO_AP_DEVICE_TYPE      "vfio-ap"
> +
> +typedef struct VFIOAPDevice {
> +    APDevice apdev;
> +    VFIODevice vdev;
> +} VFIOAPDevice;
> +
> +static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> +{
> +    vdev->needs_reset = false;
> +}
> +
> +/*
> + * We don't need vfio_hot_reset_multi and vfio_eoi operations for
> + * vfio-ap device now.
> + */
> +struct VFIODeviceOps vfio_ap_ops = {
> +    .vfio_compute_needs_reset = vfio_ap_compute_needs_reset,
> +};
> +
> +static void vfio_ap_put_device(VFIOAPDevice *vapdev)
> +{
> +    g_free(vapdev->vdev.name);
> +    vfio_put_base_device(&vapdev->vdev);
> +}
> +
> +static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp)
> +{
> +    char *tmp, group_path[PATH_MAX];
> +    ssize_t len;
> +    int groupid;
> +
> +    tmp = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev);
> +    len = readlink(tmp, group_path, sizeof(group_path));
> +    g_free(tmp);
> +
> +    if (len <= 0 || len >= sizeof(group_path)) {
> +        error_setg(errp, "%s: no iommu_group found for %s",
> +                   VFIO_AP_DEVICE_TYPE, vapdev->vdev.sysfsdev);
> +        return NULL;
> +    }
> +
> +    group_path[len] = 0;
> +
> +    if (sscanf(basename(group_path), "%d", &groupid) != 1) {
> +        error_setg(errp, "vfio: failed to read %s", group_path);
> +        return NULL;
> +    }
> +
> +    return vfio_get_group(groupid, &address_space_memory, errp);
> +}
> +
> +static void vfio_ap_realize(DeviceState *dev, Error **errp)
> +{
> +    int ret;
> +    char *mdevid;
> +    Error *local_err = NULL;
> +    VFIOGroup *vfio_group;
> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
> +
> +    vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
> +
> +    vfio_group = vfio_ap_get_group(vapdev, &local_err);
> +    if (!vfio_group) {
> +        goto out_err;
> +    }
> +
> +    vapdev->vdev.ops = &vfio_ap_ops;
> +    vapdev->vdev.type = VFIO_DEVICE_TYPE_AP;
> +    mdevid = basename(vapdev->vdev.sysfsdev);
> +    vapdev->vdev.name = g_strdup_printf("%s", mdevid);
> +    vapdev->vdev.dev = dev;
> +
> +    ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
> +    if (ret) {
> +        goto out_get_dev_err;
> +    }
> +
> +    /* Enable hardware to intepret AP instructions executed on the guest */
> +    object_property_set_bool(OBJECT(qdev_get_machine()), true, "apie", NULL);
> +
> +    return;
> +
> +out_get_dev_err:
> +    vfio_ap_put_device(vapdev);
> +    vfio_put_group(vfio_group);
> +out_err:
> +    error_propagate(errp, local_err);
> +}
> +
> +static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
> +{
> +    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);
> +    vfio_put_group(group);
> +}
> +
> +static Property vfio_ap_properties[] = {
> +    DEFINE_PROP_STRING("sysfsdev", VFIOAPDevice, vdev.sysfsdev),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void vfio_ap_reset(DeviceState *dev)
> +{
> +    int ret;
> +    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) {
> +        error_report("%s: failed to reset %s device: %s", __func__,
> +                     vapdev->vdev.name, strerror(ret));
> +    }
> +}
> +
> +static const VMStateDescription vfio_ap_vmstate = {
> +    .name = VFIO_AP_DEVICE_TYPE,
> +    .unmigratable = 1,
> +};
> +
> +static void vfio_ap_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = vfio_ap_properties;
> +    dc->vmsd = &vfio_ap_vmstate;
> +    dc->desc = "VFIO-based AP device assignment";
> +    dc->realize = vfio_ap_realize;
> +    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 = {
> +    .name = VFIO_AP_DEVICE_TYPE,
> +    .parent = AP_DEVICE_TYPE,
> +    .instance_size = sizeof(VFIOAPDevice),
> +    .class_init = vfio_ap_class_init,
> +};
> +
> +static void vfio_ap_type_init(void)
> +{
> +    type_register_static(&vfio_ap_info);
> +}
> +
> +type_init(vfio_ap_type_init)
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 821def05658f..6be9a93f611b 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -37,6 +37,7 @@ enum {
>       VFIO_DEVICE_TYPE_PCI = 0,
>       VFIO_DEVICE_TYPE_PLATFORM = 1,
>       VFIO_DEVICE_TYPE_CCW = 2,
> +    VFIO_DEVICE_TYPE_AP = 3,
>   };
> 
>   typedef struct VFIOMmap {
> 


It works as expected.

Tested-by: Pierre Morel<pmorel@linux.ibm.com>


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

Re: [Qemu-devel] [PATCH v9 6/6] s390: doc: detailed specifications for AP virtualization

[Pierre Morel]

On 27/09/2018 00:54, Tony Krowiak wrote:
> This patch provides documentation describing the AP architecture and
> design concepts behind the virtualization of AP devices. It also
> includes an example of how to configure AP devices for exclusive
> use of KVM guests.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   MAINTAINERS      |   1 +
>   docs/vfio-ap.txt | 787 +++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 788 insertions(+)
>   create mode 100644 docs/vfio-ap.txt
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 29041da69237..b64a12034c2c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1210,6 +1210,7 @@ F: hw/s390x/ap-bridge.c
>   F: include/hw/s390x/ap-device.h
>   F: include/hw/s390x/ap-bridge.h
>   F: hw/vfio/ap.c
> +F: docs/vfio-ap.txt
>   L: qemu-s390x@nongnu.org
> 
>   vhost
> diff --git a/docs/vfio-ap.txt b/docs/vfio-ap.txt
> new file mode 100644
> index 000000000000..fd17ce48967b
> --- /dev/null
> +++ b/docs/vfio-ap.txt
> @@ -0,0 +1,787 @@
> +Adjunct Processor (AP) Device
> +=============================
> +
> +Contents:
> +=========
> +* Introduction
> +* AP Architectural Overview
> +* Start Interpretive Execution (SIE) Instruction
> +* AP Matrix Configuration on Linux Host
> +* Starting a Linux Guest Configured with an AP Matrix
> +* Example: Configure AP Matrices for Three Linux Guests
> +
> +Introduction:
> +============
> +The IBM Adjunct Processor (AP) Cryptographic Facility is comprised
> +of three AP instructions and from 1 to 256 PCIe cryptographic adapter cards.
> +These AP devices provide cryptographic functions to all CPUs assigned to a
> +linux system running in an IBM Z system LPAR.
> +
> +On s390x, AP adapter cards are exposed via the AP bus. This document
> +describes how those cards may be made available to KVM guests using the
> +VFIO mediated device framework.
> +
> +AP Architectural Overview:
> +=========================
> +In order understand the terminology used in the rest of this document, let's
> +start with some definitions:
> +
> +* AP adapter
> +
> +  An AP adapter is an IBM Z adapter card that can perform cryptographic
> +  functions. There can be from 0 to 256 adapters assigned to an LPAR; however,
> +  the maximum adapter number allowed is determined by machine model. Adapters
> +  assigned to the LPAR in which a linux host is running will be available to the
> +  linux host. Each adapter is identified by a number from 0 to 255. When
> +  installed, an AP adapter is accessed by AP instructions executed by any CPU.
> +
> +* AP domain
> +
> +  An adapter is partitioned into domains. Each domain can be thought of as
> +  a set of hardware registers for processing AP instructions. An adapter can
> +  hold up to 256 domains; however, the maximum domain number allowed is
> +  determined by machine model. Each domain is identified by a number from 0 to
> +  255. Domains can be further classified into two types:
> +
> +    * Usage domains are domains that can be accessed directly to process AP
> +      commands
> +
> +    * Control domains are domains that are accessed indirectly by AP
> +      commands sent to a usage domain to control or change the domain; for
> +      example, to set a secure private key for the domain.
> +
> +* AP Queue
> +
> +  An AP queue is the means by which an AP command-request message is sent to an
> +  AP usage domain inside a specific AP. An AP queue is identified by a tuple
> +  comprised of an AP adapter ID (APID) and an AP queue index (APQI). The
> +  APQI corresponds to a given usage domain number within the adapter. This tuple
> +  forms an AP Queue Number (APQN) uniquely identifying an AP queue. AP
> +  instructions include a field containing the APQN to identify the AP queue to
> +  which the AP command-request message is to be sent for processing.
> +
> +* AP Instructions:
> +
> +  There are three AP instructions:
> +
> +  * NQAP: to enqueue an AP command-request message to a queue
> +  * DQAP: to dequeue an AP command-reply message from a queue
> +  * PQAP: to administer the queues
> +
> +  AP instructions identify the domain that is targeted to process the AP
> +  command; this must be one of the usage domains. An AP command may modify a
> +  domain that is not one of the usage domains, but the modified domain
> +  must be one of the control domains.
> +
> +Start Interpretive Execution (SIE) Instruction
> +==============================================
> +A KVM guest is started by executing the Start Interpretive Execution (SIE)
> +instruction. The SIE state description is a control block that contains the
> +state information for a KVM guest and is supplied as input to the SIE
> +instruction. The SIE state description contains a satellite control block called
> +the Crypto Control Block (CRYCB). The CRYCB contains three fields to identify
> +the adapters, usage domains and control domains assigned to the KVM guest:
> +
> +* The AP Mask (APM) field is a bit mask that identifies the AP adapters assigned
> +  to the KVM guest. Each bit in the mask, from most significant to least
> +  significant bit, corresponds to an APID from 0-255. If a bit is set, the
> +  corresponding adapter is valid for use by the KVM guest.
> +
> +* The AP Queue Mask (AQM) field is a bit mask identifying the AP usage domains
> +  assigned to the KVM guest. Each bit in the mask, from most significant to
> +  least significant bit, corresponds to an AP queue index (APQI) from 0-255. If
> +  a bit is set, the corresponding queue is valid for use by the KVM guest.
> +
> +* The AP Domain Mask field is a bit mask that identifies the AP control domains
> +  assigned to the KVM guest. The ADM bit mask controls which domains can be
> +  changed by an AP command-request message sent to a usage domain from the
> +  guest. Each bit in the mask, from least significant to most significant bit,
> +  corresponds to a domain from 0-255. If a bit is set, the corresponding domain
> +  can be modified by an AP command-request message sent to a usage domain
> +  configured for the KVM guest.
> +
> +If you recall from the description of an AP Queue, AP instructions include
> +an APQN to identify the AP adapter and AP queue to which an AP command-request
> +message is to be sent (NQAP and PQAP instructions), or from which a
> +command-reply message is to be received (DQAP instruction). The validity of an
> +APQN is defined by the matrix calculated from the APM and AQM; it is the
> +cross product of all assigned adapter numbers (APM) with all assigned queue
> +indexes (AQM). For example, if adapters 1 and 2 and usage domains 5 and 6 are
> +assigned to a guest, the APQNs (1,5), (1,6), (2,5) and (2,6) will be valid for
> +the guest.
> +
> +The APQNs can provide secure key functionality - i.e., a private key is stored
> +on the adapter card for each of its domains - so each APQN must be assigned to
> +at most one guest or the linux host.
> +
> +   Example 1: Valid configuration:
> +   ------------------------------
> +   Guest1: adapters 1,2  domains 5,6
> +   Guest2: adapter  1,2  domain 7
> +
> +   This is valid because both guests have a unique set of APQNs: Guest1 has
> +   APQNs (1,5), (1,6), (2,5) and (2,6); Guest2 has APQNs (1,7) and (2,7).
> +
> +   Example 2: Valid configuration:
> +   ------------------------------
> +   Guest1: adapters 1,2 domains 5,6
> +   Guest2: adapters 3,4 domains 5,6
> +
> +   This is also valid because both guests have a unique set of APQNs:
> +      Guest1 has APQNs (1,5), (1,6), (2,5), (2,6);
> +      Guest2 has APQNs (3,5), (3,6), (4,5), (4,6)
> +
> +   Example 3: Invalid configuration:
> +   --------------------------------
> +   Guest1: adapters 1,2  domains 5,6
> +   Guest2: adapter  1    domains 6,7
> +
> +   This is an invalid configuration because both guests have access to
> +   APQN (1,6).
> +
> +AP Matrix Configuration on Linux Host:
> +=====================================
> +A linux system is a guest of the LPAR in which it is running and has access to
> +the AP resources configured for the LPAR. The LPAR's AP matrix is
> +configured via its Activation Profile which can be edited on the HMC. When the
> +linux system is started, the AP bus will detect the AP devices assigned to the
> +LPAR and create the following in sysfs:
> +
> +/sys/bus/ap
> +... [devices]
> +...... xx.yyyy
> +...... ...
> +...... cardxx
> +...... ...
> +
> +Where:
> +    cardxx     is AP adapter number xx (in hex)
> +....xx.yyyy    is an APQN with xx specifying the APID and yyyy specifying the
> +               APQI
> +
> +For example, if AP adapters 5 and 6 and domains 4, 71 (0x47), 171 (0xab) and
> +255 (0xff) are configured for the LPAR, the sysfs representation on the linux
> +host system would look like this:
> +
> +/sys/bus/ap
> +... [devices]
> +...... 05.0004
> +...... 05.0047
> +...... 05.00ab
> +...... 05.00ff
> +...... 06.0004
> +...... 06.0047
> +...... 06.00ab
> +...... 06.00ff
> +...... card05
> +...... card06
> +
> +A set of default device drivers are also created to control each type of AP
> +device that can be assigned to the LPAR on which a linux host is running:
> +
> +/sys/bus/ap
> +... [drivers]
> +...... [cex2acard]        for Crypto Express 2/3 accelerator cards
> +...... [cex2aqueue]       for AP queues served by Crypto Express 2/3
> +                          accelerator cards
> +...... [cex4card]         for Crypto Express 4/5/6 accelerator and coprocessor
> +                          cards
> +...... [cex4queue]        for AP queues served by Crypto Express 4/5/6
> +                          accelerator and coprocessor cards
> +...... [pcixcccard]       for Crypto Express 2/3 coprocessor cards
> +...... [pcixccqueue]      for AP queues served by Crypto Express 2/3
> +                          coprocessor cards
> +
> +Binding AP devices to device drivers
> +------------------------------------
> +There are two sysfs files that specify bitmasks marking a subset of the APQN
> +range as 'usable by the default AP queue device drivers' or 'not usable by the
> +default device drivers' and thus available for use by the alternate device
> +driver(s). The sysfs locations of the masks are:
> +
> +   /sys/bus/ap/apmask
> +   /sys/bus/ap/aqmask
> +
> +The 'apmask' is a 256-bit mask that identifies a set of AP adapter IDs
> +(APID). Each bit in the mask, from most significant to least significant bit,
> +corresponds to an APID from 0-255. If a bit is set, the APID is marked as
> +usable only by the default AP queue device drivers; otherwise, the APID is
> +usable by the vfio_ap device driver.
> +
> +The 'aqmask' is a 256-bit mask that identifies a set of AP queue indexes
> +(APQI). Each bit in the mask, from most significant to least significant bit,
> +corresponds to an APQI from 0-255. If a bit is set, the APQI is marked as
> +usable only by the default AP queue device drivers; otherwise, the APQI is
> +usable by the vfio_ap device driver.
> +
> +The APQN of each AP queue device assigned to the linux host is checked by the
> +AP bus against the set of APQNs derived from the cross product of the APIDs
> +and APQIs marked as usable only by the default AP queue device drivers. If a
> +match is detected, only the default AP queue device drivers will be probed;
> +otherwise, the alternate device driver(s) will be probed.
> +
> +By default, the two masks are set to mark all APQNs for use by the default
> +AP queue device drivers. There are two ways the default masks can be changed:
> +
> +   1. The masks can be changed at boot time with the kernel command line
> +      like this:
> +
> +      ap.apmask=0xffff ap.aqmask=0x40
> +
> +      This would give these two pools:
> +
> +         default drivers pool:    adapter 0-15, domain 1
> +         alternate drivers pool:  adapter 16-255, domains 2-255
> +
> +   2. The sysfs mask files can also be edited by echoing a string into the
> +      respective file in one of two formats:
> +
> +      * An absolute hex string starting with 0x - like "0x12345678" - sets
> +        the mask. If the given string is shorter than the mask, it is padded
> +        with 0s on the right. If the string is longer than the mask, the
> +        operation is terminated with an error (EINVAL).
> +
> +      * A plus ('+') or minus ('-') followed by a numerical value. Valid
> +        examples are "+1", "-13", "+0x41", "-0xff" and even "+0" and "-0". Only
> +        the corresponding bit in the mask is switched on ('+') or off ('-'). The
> +        values may also be specified in a comma-separated list to switch more
> +        than one bit on or off.
> +
> +Configuring an AP matrix for a linux guest.
> +------------------------------------------
> +The sysfs interfaces for configuring an AP matrix for a guest are built on the
> +VFIO mediated device framework. To configure an AP matrix for a guest, a
> +mediated matrix device must first be created for the /sys/devices/vfio_ap/matrix
> +device. When the vfio_ap device driver is loaded, it registers with the VFIO
> +mediated device framework. When the driver registers, the sysfs interfaces for
> +creating mediated matrix devices is created:
> +
> +/sys/devices
> +... [vfio_ap]
> +......[matrix]
> +......... [mdev_supported_types]
> +............ [vfio_ap-passthrough]
> +............... create
> +............... [devices]
> +
> +A mediated AP matrix device is created by writing a UUID to the attribute file
> +named 'create', for example:
> +
> +   uuidgen > create
> +
> +   or
> +
> +   echo $uuid > create
> +
> +When a mediated AP matrix device is created, a sysfs directory named after
> +the UUID is created in the 'devices' subdirectory:
> +
> +/sys/devices
> +... [vfio_ap]
> +......[matrix]
> +......... [mdev_supported_types]
> +............ [vfio_ap-passthrough]
> +............... create
> +............... [devices]
> +.................. [$uuid]
> +
> +There will also be three sets of attribute files created in the mediated
> +matrix device's sysfs directory to configure an AP matrix for the
> +KVM guest:
> +
> +/sys/devices
> +... [vfio_ap]
> +......[matrix]
> +......... [mdev_supported_types]
> +............ [vfio_ap-passthrough]
> +............... create
> +............... [devices]
> +.................. [$uuid]
> +..................... assign_adapter
> +..................... assign_control_domain
> +..................... assign_domain
> +..................... matrix
> +..................... unassign_adapter
> +..................... unassign_control_domain
> +..................... unassign_domain
> +
> +assign_adapter
> +   To assign an AP adapter to the mediated matrix device, its APID is written
> +   to the 'assign_adapter' file. This may be done multiple times to assign more
> +   than one adapter. The APID may be specified using conventional semantics
> +   as a decimal, hexidecimal, or octal number. For example, to assign adapters
> +   4, 5 and 16 to a mediated matrix device in decimal, hexidecimal and octal
> +   respectively:
> +
> +       echo 4 > assign_adapter
> +       echo 0x5 > assign_adapter
> +       echo 020 > assign_adapter
> +
> +   In order to successfully assign an adapter:
> +
> +   * The adapter number specified must represent a value from 0 up to the
> +     maximum adapter number configured for the system. If an adapter number
> +     higher than the maximum is specified, the operation will terminate with
> +     an error (ENODEV).
> +
> +   * All APQNs that can be derived from the adapter ID being assigned and the
> +     IDs of the previously assigned domains must be bound to the vfio_ap device
> +     driver. If no domains have yet been assigned, then there must be at least
> +     one APQN with the specified APID bound to the vfio_ap driver. If no such
> +     APQNs are bound to the driver, the operation will terminate with an
> +     error (EADDRNOTAVAIL).
> +
> +     No APQN that can be derived from the adapter ID and the IDs of the
> +     previously assigned domains can be assigned to another mediated matrix
> +     device. If an APQN is assigned to another mediated matrix device, the
> +     operation will terminate with an error (EADDRINUSE).
> +
> +unassign_adapter
> +   To unassign an AP adapter, its APID is written to the 'unassign_adapter'
> +   file. This may also be done multiple times to unassign more than one adapter.
> +
> +assign_domain
> +   To assign a usage domain, the domain number is written into the
> +   'assign_domain' file. This may be done multiple times to assign more than one
> +   usage domain. The domain number is specified using conventional semantics as
> +   a decimal, hexidecimal, or octal number. For example, to assign usage domains
> +   4, 8, and 71 to a mediated matrix device in decimal, hexidecimal and octal
> +   respectively:
> +
> +      echo 4 > assign_domain
> +      echo 0x8 > assign_domain
> +      echo 0107 > assign_domain
> +
> +   In order to successfully assign a domain:
> +
> +   * The domain number specified must represent a value from 0 up to the
> +     maximum domain number configured for the system. If a domain number
> +     higher than the maximum is specified, the operation will terminate with
> +     an error (ENODEV).
> +
> +   * All APQNs that can be derived from the domain ID being assigned and the IDs
> +     of the previously assigned adapters must be bound to the vfio_ap device
> +     driver. If no domains have yet been assigned, then there must be at least
> +     one APQN with the specified APQI bound to the vfio_ap driver. If no such
> +     APQNs are bound to the driver, the operation will terminate with an
> +     error (EADDRNOTAVAIL).
> +
> +     No APQN that can be derived from the domain ID being assigned and the IDs
> +     of the previously assigned adapters can be assigned to another mediated
> +     matrix device. If an APQN is assigned to another mediated matrix device,
> +     the operation will terminate with an error (EADDRINUSE).
> +
> +unassign_domain
> +   To unassign a usage domain, the domain number is written into the
> +   'unassign_domain' file. This may be done multiple times to unassign more than
> +   one usage domain.
> +
> +assign_control_domain
> +   To assign a control domain, the domain number is written into the
> +   'assign_control_domain' file. This may be done multiple times to
> +   assign more than one control domain. The domain number may be specified using
> +   conventional semantics as a decimal, hexidecimal, or octal number. For
> +   example, to assign  control domains 4, 8, and 71 to  a mediated matrix device
> +   in decimal, hexidecimal and octal respectively:
> +
> +      echo 4 > assign_domain
> +      echo 0x8 > assign_domain
> +      echo 0107 > assign_domain
> +
> +   In order to successfully assign a control domain, the domain number
> +   specified must represent a value from 0 up to the maximum domain number
> +   configured for the system. If a control domain number higher than the maximum
> +   is specified, the operation will terminate with an error (ENODEV).
> +
> +unassign_control_domain
> +   To unassign a control domain, the domain number is written into the
> +   'unassign_domain' file. This may be done multiple times to unassign more than
> +   one control domain.
> +
> +Notes: Hot plug/unplug is not currently supported for mediated AP matrix
> +devices, so no changes to the AP matrix will be allowed while a guest using
> +the mediated matrix device is running. Attempts to assign an adapter,
> +domain or control domain will be rejected and an error (EBUSY) returned.
> +
> +Starting a Linux Guest Configured with an AP Matrix:
> +===================================================
> +To provide a mediated matrix device for use by a guest, the following option
> +must be specified on the QEMU command line:
> +
> +   -device vfio_ap,sysfsdev=$path-to-mdev
> +
> +The sysfsdev parameter specifies the path to the mediated matrix device.
> +There are a number of ways to specify this path:
> +
> +/sys/devices/vfio_ap/matrix/$uuid
> +/sys/bus/mdev/devices/$uuid
> +/sys/bus/mdev/drivers/vfio_mdev/$uuid
> +/sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
> +
> +When the linux guest is started, the guest will open the mediated
> +matrix device's file descriptor to get information about the mediated matrix
> +device. The vfio_ap device driver will update the APM, AQM, and ADM fields in
> +the guest's CRYCB with the adapter, usage domain and control domains assigned
> +via the mediated matrix device's sysfs attribute files. Programs running on the
> +linux guest will then:
> +
> +1. Have direct access to the APQNs derived from the cross product of the AP
> +   adapter numbers (APID) and queue indexes (APQI) specified in the APM and AQM
> +   fields of the guests's CRYCB respectively. These APQNs identify the AP queues
> +   that are valid for use by the guest; meaning, AP commands can be sent by the
> +   guest to any of these queues for processing.
> +
> +2. Have authorization to process AP commands to change a control domain
> +   identified in the ADM field of the guest's CRYCB. The AP command must be sent
> +   to a valid APQN (see 1 above).
> +
> +CPU model features:
> +
> +Three CPU model features are available for controlling guest access to AP
> +facilities:
> +
> +1. AP facilities feature
> +
> +   The AP facilities feature indicates that AP facilities are installed on the
> +   guest. This feature will be enabled by the kernel only if the AP facilities
> +   are installed on the host system. It will be turned on automatically for
> +   guests started with CPU model zEC12 or newer. The feature is s390-specific
> +   and is represented as a parameter of the -cpu option on the QEMU command
> +   line:
> +
> +      qemu-system-s390x -cpu $model,ap=on|off
> +
> +      Where:
> +
> +         $model is the CPU model defined for the guest (defaults to the model of
> +                the host system if not specified).
> +
> +         ap=on|off indicates whether AP facilities are installed (on) or not
> +                   (off). The default for CPU models zEC12 or newer
> +                   is ap=on. AP facilities must be installed on the guest if a
> +                   vfio-ap device (-device vfio-ap,sysfsdev=$path) is configured
> +                   for the guest or the guest will fail to start.
> +
> +2. Query Configuration Information (QCI) facility
> +
> +   The QCI facility is used by the AP bus running on the guest to query the
> +   configuration of the AP facilities. This facility will be enabled by
> +   the kernel only if the QCI facility is installed on the host system. It will
> +   be turned on automatically for guests started with CPU model zEC12 or newer.
> +   The feature is s390-specific and is represented as a parameter of the -cpu
> +   option on the QEMU command line:
> +
> +      qemu-system-s390x -cpu $model,apqci=on|off
> +
> +      Where:
> +
> +         $model is the CPU model defined for the guest
> +
> +         apqci=on|off indicates whether the QCI facility is installed (on) or
> +                      not (off). The default for CPU models zEC12 or newer
> +                      is apqci=on; for older models, QCI will not be installed.
> +
> +                      If QCI is installed (apqci=on) but AP facilities are not
> +                      (ap=off), an error message will be logged, but the guest
> +                      will be allowed to start. It makes no sense to have QCI
> +                      installed if the AP facilities are not; this is considered
> +                      an invalid configuration.
> +
> +                      If the QCI facility is not installed, APQNs with an APQI
> +                      greater than 15 will not be detected by the AP bus
> +                      running on the guest.
> +
> +3. Adjunct Process Facility Test (APFT) facility
> +
> +   The APFT facility is used by the AP bus running on the guest to test the
> +   AP facilities available for a given AP queue. This facility will be enabled
> +   by the kernel only if the APFT facility is installed on the host system. It
> +   will be turned on automatically for guests started with CPU model zEC12 or
> +   newer. The feature is s390-specific and is represented as a parameter of the
> +   -cpu option on the QEMU command line:
> +
> +      qemu-system-s390x -cpu $model,apft=on|off
> +
> +      Where:
> +
> +         $model is the CPU model defined for the guest (defaults to the model of
> +                the host system if not specified).
> +
> +         apft=on|off indicates whether the APFT facility is installed (on) or
> +                     not (off). The default for CPU models zEC12 and
> +                     newer is apft=on for older models, APFT will not be
> +                     installed.
> +
> +                     If APFT is installed (apft=on) but AP facilities are not
> +                     (ap=off), an error message will be logged, but the guest
> +                     will be allowed to start. It makes no sense to have APFT
> +                     installed if the AP facilities are not; this is considered
> +                     an invalid configuration.
> +
> +                     It also makes no sense to turn APFT off because the AP bus
> +                     running on the guest will not detect CEX4 and newer devices
> +                     without it. Since only CEX4 and newer devices are supported
> +                     for guest usage, no AP devices can be made accessible to a
> +                     guest started without APFT installed.
> +
> +Example: Configure AP Matrixes for Three Linux Guests:
> +=====================================================
> +Let's now provide an example to illustrate how KVM guests may be given
> +access to AP facilities. For this example, we will show how to configure
> +three guests such that executing the lszcrypt command on the guests would
> +look like this:
> +
> +Guest1
> +------
> +CARD.DOMAIN TYPE  MODE
> +------------------------------
> +05          CEX5C CCA-Coproc
> +05.0004     CEX5C CCA-Coproc
> +05.00ab     CEX5C CCA-Coproc
> +06          CEX5A Accelerator
> +06.0004     CEX5A Accelerator
> +06.00ab     CEX5C CCA-Coproc
> +
> +Guest2
> +------
> +CARD.DOMAIN TYPE  MODE
> +------------------------------
> +05          CEX5A Accelerator
> +05.0047     CEX5A Accelerator
> +05.00ff     CEX5A Accelerator (5,4), (5,171), (6,4), (6,171),
> +
> +Guest3
> +------
> +CARD.DOMAIN TYPE  MODE
> +------------------------------
> +06          CEX5A Accelerator
> +06.0047     CEX5A Accelerator
> +06.00ff     CEX5A Accelerator
> +
> +These are the steps:
> +
> +1. Install the vfio_ap module on the linux host. The dependency chain for the
> +   vfio_ap module is:
> +   * iommu
> +   * s390
> +   * zcrypt
> +   * vfio
> +   * vfio_mdev
> +   * vfio_mdev_device
> +   * KVM
> +
> +   To build the vfio_ap module, the kernel build must be configured with the
> +   following Kconfig elements selected:
> +   * IOMMU_SUPPORT
> +   * S390
> +   * ZCRYPT
> +   * S390_AP_IOMMU
> +   * VFIO
> +   * VFIO_MDEV
> +   * VFIO_MDEV_DEVICE
> +   * KVM
> +
> +   If using make menuconfig select the following to build the vfio_ap module:
> +   -> Device Drivers
> +      -> IOMMU Hardware Support
> +         select S390 AP IOMMU Support
> +      -> VFIO Non-Privileged userspace driver framework
> +         -> Mediated device driver frramework
> +            -> VFIO driver for Mediated devices
> +   -> I/O subsystem
> +      -> VFIO support for AP devices
> +
> +2. Secure the AP queues to be used by the three guests so that the host can not
> +   access them. To secure the AP queues 05.0004, 05.0047, 05.00ab, 05.00ff,
> +   06.0004, 06.0047, 06.00ab, and 06.00ff for use by the vfio_ap device driver,
> +   the corresponding APQNs must be removed from the default queue drivers pool
> +   as follows:
> +
> +      echo -5,-6 > /sys/bus/ap/apmask
> +
> +      echo -4,-0x47,-0xab,-0xff > /sys/bus/ap/aqmask
> +
> +   This will result in AP queues 05.0004, 05.0047, 05.00ab, 05.00ff, 06.0004,
> +   06.0047, 06.00ab, and 06.00ff getting bound to the vfio_ap device driver. The
> +   sysfs directory for the vfio_ap device driver will now contain symbolic links
> +   to the AP queue devices bound to it:
> +
> +   /sys/bus/ap
> +   ... [drivers]
> +   ...... [vfio_ap]
> +   ......... [05.0004]
> +   ......... [05.0047]
> +   ......... [05.00ab]
> +   ......... [05.00ff]
> +   ......... [06.0004]
> +   ......... [06.0047]
> +   ......... [06.00ab]
> +   ......... [06.00ff]
> +
> +   Keep in mind that only type 10 and newer adapters (i.e., CEX4 and later)
> +   can be bound to the vfio_ap device driver. The reason for this is to
> +   simplify the implementation by not needlessly complicating the design by
> +   supporting older devices that will go out of service in the relatively near
> +   future, and for which there are few older systems on which to test.
> +
> +   The administrator, therefore, must take care to secure only AP queues that
> +   can be bound to the vfio_ap device driver. The device type for a given AP
> +   queue device can be read from the parent card's sysfs directory. For example,
> +   to see the hardware type of the queue 05.0004:
> +
> +   cat /sys/bus/ap/devices/card05/hwtype
> +
> +   The hwtype must be 10 or higher (CEX4 or newer) in order to be bound to the
> +   vfio_ap device driver.
> +
> +3. Create the mediated devices needed to configure the AP matrixes for the
> +   three guests and to provide an interface to the vfio_ap driver for
> +   use by the guests:
> +
> +   /sys/devices/vfio_ap/matrix/
> +   --- [mdev_supported_types]
> +   ------ [vfio_ap-passthrough] (passthrough mediated matrix device type)
> +   --------- create
> +   --------- [devices]
> +
> +   To create the mediated devices for the three guests:
> +
> +       uuidgen > create
> +       uuidgen > create
> +       uuidgen > create
> +
> +        or
> +
> +        echo $uuid1 > create
> +        echo $uuid2 > create
> +        echo $uuid3 > create
> +
> +   This will create three mediated devices in the [devices] subdirectory named
> +   after the UUID used to create the mediated device. We'll call them $uuid1,
> +   $uuid2 and $uuid3 and this is the sysfs directory structure after creation:
> +
> +   /sys/devices/vfio_ap/matrix/
> +   --- [mdev_supported_types]
> +   ------ [vfio_ap-passthrough]
> +   --------- [devices]
> +   ------------ [$uuid1]
> +   --------------- assign_adapter
> +   --------------- assign_control_domain
> +   --------------- assign_domain
> +   --------------- matrix
> +   --------------- unassign_adapter
> +   --------------- unassign_control_domain
> +   --------------- unassign_domain
> +
> +   ------------ [$uuid2]
> +   --------------- assign_adapter
> +   --------------- assign_control_domain
> +   --------------- assign_domain
> +   --------------- matrix
> +   --------------- unassign_adapter
> +   ----------------unassign_control_domain
> +   ----------------unassign_domain
> +
> +   ------------ [$uuid3]
> +   --------------- assign_adapter
> +   --------------- assign_control_domain
> +   --------------- assign_domain
> +   --------------- matrix
> +   --------------- unassign_adapter
> +   ----------------unassign_control_domain
> +   ----------------unassign_domain
> +
> +4. The administrator now needs to configure the matrixes for the mediated
> +   devices $uuid1 (for Guest1), $uuid2 (for Guest2) and $uuid3 (for Guest3).
> +
> +   This is how the matrix is configured for Guest1:
> +
> +      echo 5 > assign_adapter
> +      echo 6 > assign_adapter
> +      echo 4 > assign_domain
> +      echo 0xab > assign_domain
> +
> +      Control domains can similarly be assigned using the assign_control_domain
> +      sysfs file.
> +
> +      If a mistake is made configuring an adapter, domain or control domain,
> +      you can use the unassign_xxx interfaces to unassign the adapter, domain or
> +      control domain.
> +
> +      To display the matrix configuration for Guest1:
> +
> +         cat matrix
> +
> +         The output will display the APQNs in the format xx.yyyy, where xx is
> +         the adapter number and yyyy is the domain number. The output for Guest1
> +         will look like this:
> +
> +         05.0004
> +         05.00ab
> +         06.0004
> +         06.00ab
> +
> +   This is how the matrix is configured for Guest2:
> +
> +      echo 5 > assign_adapter
> +      echo 0x47 > assign_domain
> +      echo 0xff > assign_domain
> +
> +   This is how the matrix is configured for Guest3:
> +
> +      echo 6 > assign_adapter
> +      echo 0x47 > assign_domain
> +      echo 0xff > assign_domain
> +
> +5. Start Guest1:
> +
> +   /usr/bin/qemu-system-s390x ... -cpu host,ap=on,apqci=on,apft=on \
> +      -device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid1 ...
> +
> +7. Start Guest2:
> +
> +   /usr/bin/qemu-system-s390x ... -cpu host,ap=on,apqci=on,apft=on \
> +      -device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid2 ...
> +
> +7. Start Guest3:
> +
> +   /usr/bin/qemu-system-s390x ... -cpu host,ap=on,apqci=on,apft=on \
> +      -device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid3 ...
> +
> +When the guest is shut down, the mediated matrix devices may be removed.
> +
> +Using our example again, to remove the mediated matrix device $uuid1:
> +
> +   /sys/devices/vfio_ap/matrix/
> +      --- [mdev_supported_types]
> +      ------ [vfio_ap-passthrough]
> +      --------- [devices]
> +      ------------ [$uuid1]
> +      --------------- remove
> +
> +
> +   echo 1 > remove
> +
> +   This will remove all of the mdev matrix device's sysfs structures including
> +   the mdev device itself. To recreate and reconfigure the mdev matrix device,
> +   all of the steps starting with step 3 will have to be performed again. Note
> +   that the remove will fail if a guest using the mdev is still running.
> +
> +   It is not necessary to remove an mdev matrix device, but one may want to
> +   remove it if no guest will use it during the remaining lifetime of the linux
> +   host. If the mdev matrix device is removed, one may want to also reconfigure
> +   the pool of adapters and queues reserved for use by the default drivers.
> +
> +Limitations
> +===========
> +* The KVM/kernel interfaces do not provide a way to prevent restoring an APQN
> +  to the default drivers pool of a queue that is still assigned to a mediated
> +  device in use by a guest. It is incumbent upon the administrator to
> +  ensure there is no mediated device in use by a guest to which the APQN is
> +  assigned lest the host be given access to the private data of the AP queue
> +  device, such as a private key configured specifically for the guest.
> +
> +* Dynamically modifying the AP matrix for a running guest (which would amount to
> +  hot(un)plug of AP devices for the guest) is currently not supported
> +
> +* Live guest migration is not supported for guests using AP devices.
> 


obviously all remarks from Linux documentation for VFIO_AP are
here to be handled too.
With this:

Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>

It also can be used as is to configure Linux/Qemu and make the guest 
working as expected:

Tested-by: Pierre Morel<pmorel@linux.ibm.com>


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

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[Tony Krowiak]

On 09/27/2018 08:52 AM, Cornelia Huck wrote:
> On Thu, 27 Sep 2018 14:29:01 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 2018-09-27 00:54, Tony Krowiak wrote:
>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>
>>> Introduces the base object model for virtualizing AP devices.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>> ---
> 
>>> +typedef struct APBridge {
>>> +    SysBusDevice sysbus_dev;
>>> +    bool css_dev_path;
>>
>> What is this css_dev_path variable good for? I don't see it used in any
>> of the other patches?
>> If you don't need it, I think you could get rid of this struct completely?
> 
> Huh, now I remember complaining about it before. Looks like a
> copy-and-paste from the css bridge; that variable is used for compat
> handling there (and should be ditched here).
> 
>>
>>> +} APBridge;
>>> +
>>> +#define TYPE_AP_BRIDGE "ap-bridge"
>>> +#define AP_BRIDGE(obj) \
>>> +    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
>>> +
>>> +typedef struct APBus {
>>> +    BusState parent_obj;
>>> +} APBus;
>>> +
>>> +#define TYPE_AP_BUS "ap-bus"
>>> +#define AP_BUS(obj) \
>>> +     OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
>>
>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
>> struct APBus.
> 
> If there's nothing interesting to put in these inherited structures,
> probably yes.
> 
>>
>>> +void s390_init_ap(void);
>>> +
>>> +#endif
>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
>>> new file mode 100644
>>> index 000000000000..693df90cc041
>>> --- /dev/null
>>> +++ b/include/hw/s390x/ap-device.h
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * Adjunct Processor (AP) matrix device interfaces
>>> + *
>>> + * Copyright 2018 IBM Corp.
>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
>>> + * directory.
>>> + */
>>> +#ifndef HW_S390X_AP_DEVICE_H
>>> +#define HW_S390X_AP_DEVICE_H
>>> +
>>> +#define AP_DEVICE_TYPE       "ap-device"
>>> +
>>> +typedef struct APDevice {
>>> +    DeviceState parent_obj;
>>> +} APDevice;
>>> +
>>> +typedef struct APDeviceClass {
>>> +    DeviceClass parent_class;
>>> +} APDeviceClass;
>>> +
>>> +static inline APDevice *to_ap_dev(DeviceState *dev)
>>> +{
>>> +    return container_of(dev, APDevice, parent_obj);
>>> +}
>>> +
>>> +#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)
>>
>> Do you really need any of these definitions except AP_DEVICE_TYPE ?

Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
AP_DEVICE_CLASS(klass), but aren't those typically included in all
QOM definitions?

> 
> Same here, I think.
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[David Hildenbrand]

On 08/10/2018 16:20, Tony Krowiak wrote:
> On 09/27/2018 08:52 AM, Cornelia Huck wrote:
>> On Thu, 27 Sep 2018 14:29:01 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 2018-09-27 00:54, Tony Krowiak wrote:
>>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>
>>>> Introduces the base object model for virtualizing AP devices.
>>>>
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>> ---
>>
>>>> +typedef struct APBridge {
>>>> +    SysBusDevice sysbus_dev;
>>>> +    bool css_dev_path;
>>>
>>> What is this css_dev_path variable good for? I don't see it used in any
>>> of the other patches?
>>> If you don't need it, I think you could get rid of this struct completely?
>>
>> Huh, now I remember complaining about it before. Looks like a
>> copy-and-paste from the css bridge; that variable is used for compat
>> handling there (and should be ditched here).
>>
>>>
>>>> +} APBridge;
>>>> +
>>>> +#define TYPE_AP_BRIDGE "ap-bridge"
>>>> +#define AP_BRIDGE(obj) \
>>>> +    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
>>>> +
>>>> +typedef struct APBus {
>>>> +    BusState parent_obj;
>>>> +} APBus;
>>>> +
>>>> +#define TYPE_AP_BUS "ap-bus"
>>>> +#define AP_BUS(obj) \
>>>> +     OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
>>>
>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
>>> struct APBus.
>>
>> If there's nothing interesting to put in these inherited structures,
>> probably yes.
>>
>>>
>>>> +void s390_init_ap(void);
>>>> +
>>>> +#endif
>>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
>>>> new file mode 100644
>>>> index 000000000000..693df90cc041
>>>> --- /dev/null
>>>> +++ b/include/hw/s390x/ap-device.h
>>>> @@ -0,0 +1,38 @@
>>>> +/*
>>>> + * Adjunct Processor (AP) matrix device interfaces
>>>> + *
>>>> + * Copyright 2018 IBM Corp.
>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
>>>> + * directory.
>>>> + */
>>>> +#ifndef HW_S390X_AP_DEVICE_H
>>>> +#define HW_S390X_AP_DEVICE_H
>>>> +
>>>> +#define AP_DEVICE_TYPE       "ap-device"
>>>> +
>>>> +typedef struct APDevice {
>>>> +    DeviceState parent_obj;
>>>> +} APDevice;
>>>> +
>>>> +typedef struct APDeviceClass {
>>>> +    DeviceClass parent_class;
>>>> +} APDeviceClass;
>>>> +
>>>> +static inline APDevice *to_ap_dev(DeviceState *dev)
>>>> +{
>>>> +    return container_of(dev, APDevice, parent_obj);
>>>> +}
>>>> +
>>>> +#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)
>>>
>>> Do you really need any of these definitions except AP_DEVICE_TYPE ?
> 
> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
> patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
> AP_DEVICE_CLASS(klass), but aren't those typically included in all
> QOM definitions?

Yes, we usually add all of them although only some might actually be
used. (adding a new device usually looks like filling out a template)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[Cornelia Huck]

On Mon, 8 Oct 2018 16:22:27 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08/10/2018 16:20, Tony Krowiak wrote:
> > On 09/27/2018 08:52 AM, Cornelia Huck wrote:  
> >> On Thu, 27 Sep 2018 14:29:01 +0200
> >> Thomas Huth <thuth@redhat.com> wrote:
> >>  
> >>> On 2018-09-27 00:54, Tony Krowiak wrote:  
> >>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
> >>>>
> >>>> Introduces the base object model for virtualizing AP devices.
> >>>>
> >>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> >>>> ---  
> >>  
> >>>> +typedef struct APBridge {
> >>>> +    SysBusDevice sysbus_dev;
> >>>> +    bool css_dev_path;  
> >>>
> >>> What is this css_dev_path variable good for? I don't see it used in any
> >>> of the other patches?
> >>> If you don't need it, I think you could get rid of this struct completely?  
> >>
> >> Huh, now I remember complaining about it before. Looks like a
> >> copy-and-paste from the css bridge; that variable is used for compat
> >> handling there (and should be ditched here).
> >>  
> >>>  
> >>>> +} APBridge;
> >>>> +
> >>>> +#define TYPE_AP_BRIDGE "ap-bridge"
> >>>> +#define AP_BRIDGE(obj) \
> >>>> +    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
> >>>> +
> >>>> +typedef struct APBus {
> >>>> +    BusState parent_obj;
> >>>> +} APBus;
> >>>> +
> >>>> +#define TYPE_AP_BUS "ap-bus"
> >>>> +#define AP_BUS(obj) \
> >>>> +     OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)  
> >>>
> >>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
> >>> struct APBus.  
> >>
> >> If there's nothing interesting to put in these inherited structures,
> >> probably yes.
> >>  
> >>>  
> >>>> +void s390_init_ap(void);
> >>>> +
> >>>> +#endif
> >>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> >>>> new file mode 100644
> >>>> index 000000000000..693df90cc041
> >>>> --- /dev/null
> >>>> +++ b/include/hw/s390x/ap-device.h
> >>>> @@ -0,0 +1,38 @@
> >>>> +/*
> >>>> + * Adjunct Processor (AP) matrix device interfaces
> >>>> + *
> >>>> + * Copyright 2018 IBM Corp.
> >>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
> >>>> + * directory.
> >>>> + */
> >>>> +#ifndef HW_S390X_AP_DEVICE_H
> >>>> +#define HW_S390X_AP_DEVICE_H
> >>>> +
> >>>> +#define AP_DEVICE_TYPE       "ap-device"
> >>>> +
> >>>> +typedef struct APDevice {
> >>>> +    DeviceState parent_obj;
> >>>> +} APDevice;
> >>>> +
> >>>> +typedef struct APDeviceClass {
> >>>> +    DeviceClass parent_class;
> >>>> +} APDeviceClass;
> >>>> +
> >>>> +static inline APDevice *to_ap_dev(DeviceState *dev)
> >>>> +{
> >>>> +    return container_of(dev, APDevice, parent_obj);
> >>>> +}
> >>>> +
> >>>> +#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)  
> >>>
> >>> Do you really need any of these definitions except AP_DEVICE_TYPE ?  
> > 
> > Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
> > patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
> > AP_DEVICE_CLASS(klass), but aren't those typically included in all
> > QOM definitions?  
> 
> Yes, we usually add all of them although only some might actually be
> used. (adding a new device usually looks like filling out a template)

Much of this seems to be boilerplate in this case, and I'm not sure how
much sense it makes. On the plus side, however, it looks like
everything else :)

So, I would merge both a complete version or a
stripped-down-to-the-needed version, unless someone else has a strong
argument.

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[Thomas Huth]

On 2018-10-08 16:20, Tony Krowiak wrote:
> On 09/27/2018 08:52 AM, Cornelia Huck wrote:
>> On Thu, 27 Sep 2018 14:29:01 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 2018-09-27 00:54, Tony Krowiak wrote:
>>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>
>>>> Introduces the base object model for virtualizing AP devices.
>>>>
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>> ---
>>
>>>> +typedef struct APBridge {
>>>> +    SysBusDevice sysbus_dev;
>>>> +    bool css_dev_path;
>>>
>>> What is this css_dev_path variable good for? I don't see it used in any
>>> of the other patches?
>>> If you don't need it, I think you could get rid of this struct
>>> completely?
>>
>> Huh, now I remember complaining about it before. Looks like a
>> copy-and-paste from the css bridge; that variable is used for compat
>> handling there (and should be ditched here).
>>
>>>
>>>> +} APBridge;
>>>> +
>>>> +#define TYPE_AP_BRIDGE "ap-bridge"
>>>> +#define AP_BRIDGE(obj) \
>>>> +    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
>>>> +
>>>> +typedef struct APBus {
>>>> +    BusState parent_obj;
>>>> +} APBus;
>>>> +
>>>> +#define TYPE_AP_BUS "ap-bus"
>>>> +#define AP_BUS(obj) \
>>>> +     OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
>>>
>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
>>> struct APBus.
>>
>> If there's nothing interesting to put in these inherited structures,
>> probably yes.
>>
>>>
>>>> +void s390_init_ap(void);
>>>> +
>>>> +#endif
>>>> diff --git a/include/hw/s390x/ap-device.h
>>>> b/include/hw/s390x/ap-device.h
>>>> new file mode 100644
>>>> index 000000000000..693df90cc041
>>>> --- /dev/null
>>>> +++ b/include/hw/s390x/ap-device.h
>>>> @@ -0,0 +1,38 @@
>>>> +/*
>>>> + * Adjunct Processor (AP) matrix device interfaces
>>>> + *
>>>> + * Copyright 2018 IBM Corp.
>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
>>>> + * directory.
>>>> + */
>>>> +#ifndef HW_S390X_AP_DEVICE_H
>>>> +#define HW_S390X_AP_DEVICE_H
>>>> +
>>>> +#define AP_DEVICE_TYPE       "ap-device"
>>>> +
>>>> +typedef struct APDevice {
>>>> +    DeviceState parent_obj;
>>>> +} APDevice;
>>>> +
>>>> +typedef struct APDeviceClass {
>>>> +    DeviceClass parent_class;
>>>> +} APDeviceClass;
>>>> +
>>>> +static inline APDevice *to_ap_dev(DeviceState *dev)
>>>> +{
>>>> +    return container_of(dev, APDevice, parent_obj);
>>>> +}
>>>> +
>>>> +#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)
>>>
>>> Do you really need any of these definitions except AP_DEVICE_TYPE ?
> 
> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
> patch 5/6.

Fine for me, if you replace the DO_UPCAST in patch 5 with AP_DEVICE().

> We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
> AP_DEVICE_CLASS(klass), but aren't those typically included in all
> QOM definitions?

As long as you don't really need them, I'd simply remove them. They can
be added back when some code really needs them.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[Tony Krowiak]

On 10/08/2018 10:35 AM, Cornelia Huck wrote:
> On Mon, 8 Oct 2018 16:22:27 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 08/10/2018 16:20, Tony Krowiak wrote:
>>> On 09/27/2018 08:52 AM, Cornelia Huck wrote:
>>>> On Thu, 27 Sep 2018 14:29:01 +0200
>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>   
>>>>> On 2018-09-27 00:54, Tony Krowiak wrote:
>>>>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>>>
>>>>>> Introduces the base object model for virtualizing AP devices.
>>>>>>
>>>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>>> ---
>>>>   
>>>>>> +typedef struct APBridge {
>>>>>> +    SysBusDevice sysbus_dev;
>>>>>> +    bool css_dev_path;
>>>>>
>>>>> What is this css_dev_path variable good for? I don't see it used in any
>>>>> of the other patches?
>>>>> If you don't need it, I think you could get rid of this struct completely?
>>>>
>>>> Huh, now I remember complaining about it before. Looks like a
>>>> copy-and-paste from the css bridge; that variable is used for compat
>>>> handling there (and should be ditched here).
>>>>   
>>>>>   
>>>>>> +} APBridge;
>>>>>> +
>>>>>> +#define TYPE_AP_BRIDGE "ap-bridge"
>>>>>> +#define AP_BRIDGE(obj) \
>>>>>> +    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
>>>>>> +
>>>>>> +typedef struct APBus {
>>>>>> +    BusState parent_obj;
>>>>>> +} APBus;
>>>>>> +
>>>>>> +#define TYPE_AP_BUS "ap-bus"
>>>>>> +#define AP_BUS(obj) \
>>>>>> +     OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
>>>>>
>>>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
>>>>> struct APBus.
>>>>
>>>> If there's nothing interesting to put in these inherited structures,
>>>> probably yes.
>>>>   
>>>>>   
>>>>>> +void s390_init_ap(void);
>>>>>> +
>>>>>> +#endif
>>>>>> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
>>>>>> new file mode 100644
>>>>>> index 000000000000..693df90cc041
>>>>>> --- /dev/null
>>>>>> +++ b/include/hw/s390x/ap-device.h
>>>>>> @@ -0,0 +1,38 @@
>>>>>> +/*
>>>>>> + * Adjunct Processor (AP) matrix device interfaces
>>>>>> + *
>>>>>> + * Copyright 2018 IBM Corp.
>>>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
>>>>>> + * directory.
>>>>>> + */
>>>>>> +#ifndef HW_S390X_AP_DEVICE_H
>>>>>> +#define HW_S390X_AP_DEVICE_H
>>>>>> +
>>>>>> +#define AP_DEVICE_TYPE       "ap-device"
>>>>>> +
>>>>>> +typedef struct APDevice {
>>>>>> +    DeviceState parent_obj;
>>>>>> +} APDevice;
>>>>>> +
>>>>>> +typedef struct APDeviceClass {
>>>>>> +    DeviceClass parent_class;
>>>>>> +} APDeviceClass;
>>>>>> +
>>>>>> +static inline APDevice *to_ap_dev(DeviceState *dev)
>>>>>> +{
>>>>>> +    return container_of(dev, APDevice, parent_obj);
>>>>>> +}
>>>>>> +
>>>>>> +#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)
>>>>>
>>>>> Do you really need any of these definitions except AP_DEVICE_TYPE ?
>>>
>>> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
>>> patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
>>> AP_DEVICE_CLASS(klass), but aren't those typically included in all
>>> QOM definitions?
>>
>> Yes, we usually add all of them although only some might actually be
>> used. (adding a new device usually looks like filling out a template)
> 
> Much of this seems to be boilerplate in this case, and I'm not sure how
> much sense it makes. On the plus side, however, it looks like
> everything else :)
> 
> So, I would merge both a complete version or a
> stripped-down-to-the-needed version, unless someone else has a strong
> argument.

The 'I would merge both' implies you are asking for two versions, but 
the 'or' implies you are asking for one or the other; I'm going to
assume you are asking for one or the other. I'll provide a stripped down
version in v10 which I am planning on posting today.

> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model

[Tony Krowiak]

On 10/08/2018 10:44 AM, Thomas Huth wrote:
> On 2018-10-08 16:20, Tony Krowiak wrote:
>> On 09/27/2018 08:52 AM, Cornelia Huck wrote:
>>> On Thu, 27 Sep 2018 14:29:01 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> On 2018-09-27 00:54, Tony Krowiak wrote:
>>>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>>
>>>>> Introduces the base object model for virtualizing AP devices.
>>>>>
>>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>> ---
>>>
>>>>> +typedef struct APBridge {
>>>>> +    SysBusDevice sysbus_dev;
>>>>> +    bool css_dev_path;
>>>>
>>>> What is this css_dev_path variable good for? I don't see it used in any
>>>> of the other patches?
>>>> If you don't need it, I think you could get rid of this struct
>>>> completely?
>>>
>>> Huh, now I remember complaining about it before. Looks like a
>>> copy-and-paste from the css bridge; that variable is used for compat
>>> handling there (and should be ditched here).
>>>
>>>>
>>>>> +} APBridge;
>>>>> +
>>>>> +#define TYPE_AP_BRIDGE "ap-bridge"
>>>>> +#define AP_BRIDGE(obj) \
>>>>> +    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
>>>>> +
>>>>> +typedef struct APBus {
>>>>> +    BusState parent_obj;
>>>>> +} APBus;
>>>>> +
>>>>> +#define TYPE_AP_BUS "ap-bus"
>>>>> +#define AP_BUS(obj) \
>>>>> +     OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
>>>>
>>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
>>>> struct APBus.
>>>
>>> If there's nothing interesting to put in these inherited structures,
>>> probably yes.
>>>
>>>>
>>>>> +void s390_init_ap(void);
>>>>> +
>>>>> +#endif
>>>>> diff --git a/include/hw/s390x/ap-device.h
>>>>> b/include/hw/s390x/ap-device.h
>>>>> new file mode 100644
>>>>> index 000000000000..693df90cc041
>>>>> --- /dev/null
>>>>> +++ b/include/hw/s390x/ap-device.h
>>>>> @@ -0,0 +1,38 @@
>>>>> +/*
>>>>> + * Adjunct Processor (AP) matrix device interfaces
>>>>> + *
>>>>> + * Copyright 2018 IBM Corp.
>>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.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
>>>>> + * directory.
>>>>> + */
>>>>> +#ifndef HW_S390X_AP_DEVICE_H
>>>>> +#define HW_S390X_AP_DEVICE_H
>>>>> +
>>>>> +#define AP_DEVICE_TYPE       "ap-device"
>>>>> +
>>>>> +typedef struct APDevice {
>>>>> +    DeviceState parent_obj;
>>>>> +} APDevice;
>>>>> +
>>>>> +typedef struct APDeviceClass {
>>>>> +    DeviceClass parent_class;
>>>>> +} APDeviceClass;
>>>>> +
>>>>> +static inline APDevice *to_ap_dev(DeviceState *dev)
>>>>> +{
>>>>> +    return container_of(dev, APDevice, parent_obj);
>>>>> +}
>>>>> +
>>>>> +#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)
>>>>
>>>> Do you really need any of these definitions except AP_DEVICE_TYPE ?
>>
>> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
>> patch 5/6.
> 
> Fine for me, if you replace the DO_UPCAST in patch 5 with AP_DEVICE().
> 
>> We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
>> AP_DEVICE_CLASS(klass), but aren't those typically included in all
>> QOM definitions?
> 
> As long as you don't really need them, I'd simply remove them. They can
> be added back when some code really needs them.

That is the plan

> 
>   Thomas
>