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

[Halil Pasic]

On 03/16/2018 11:42 AM, Pierre Morel wrote:
> On 16/03/2018 00:24, 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>
>>
>> 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 <address@hidden>
>> ---
[..]
>> +static void vfio_ap_realize(DeviceState *dev, Error **errp)
>> +{
>> +    VFIODevice *vbasedev;
>> +    VFIOGroup *vfio_group;
>> +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
>> +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
>> +    char *mdevid;
>> +    Error *local_err = NULL;
>> +    int ret;
>> +
>> +    if (!s390_has_feat(S390_FEAT_AP)) {
>> +        error_setg(&local_err, "AP support not enabled");
>> +        goto out_err;
>> +    }
>> +
>> +    ret = kvm_s390_set_interpret_ap(1);
> 
> If we have several devices, this is called once per device.

I don't think having several of these in a single vm makes
any sense. Or does it? IMHO we should make sure there is at
most one device taking care of the crypto pass-through.

> However it is a general switch for the VM.
> It looks strange to have this inside the realize callback
> 

I kind of semi agree.

The previous solution (having this transparent for QEMU) had
benefits. Why did we move away from that again?

Regards,
Halil