Re: [PATCH v4 13/15] KVM: s390: configure the guest's AP devices

Tony Krowiak
2018-04-17T16:36:33Z

On 04/17/2018 12:18 PM, Pierre Morel wrote:
> On 17/04/2018 18:08, Tony Krowiak wrote:
>> On 04/16/2018 09:05 AM, Pierre Morel wrote:
>>> On 15/04/2018 23:22, Tony Krowiak wrote:
>>>> Registers a group notifier during the open of the mediated
>>>> matrix device to get information on KVM presence through the
>>>> VFIO_GROUP_NOTIFY_SET_KVM event. When notified, the pointer
>>>> to the kvm structure is saved inside the mediated matrix
>>>> device. Once the VFIO AP device driver has access to KVM,
>>>> access to the APs can be configured for the guest.
>>>>
>>>> Access to APs is configured when the file descriptor for the
>>>> mediated matrix device is opened by userspace. The items to be
>>>> configured are:
>>>>
>>>> 1. The ECA.28 bit in the SIE state description determines whether
>>>> AP instructions are interpreted by the hardware or intercepted.
>>>> The VFIO AP device driver relies interpretive execution of
>>>> AP instructions so the ECA.28 bit will be set
>>>>
>>>> 2. Guest access to AP adapters, usage domains and control domains
>>>> is controlled by three bit masks referenced from the
>>>> Crypto Control Block (CRYCB) referenced from the guest's SIE state
>>>> description:
>>>>
>>>> * The AP Mask (APM) controls access to the AP adapters. Each bit
>>>> in the APM represents an adapter number - from most significant
>>>> to least significant bit - from 0 to 255. The bits in the APM
>>>> are set according to the adapter numbers assigned to the
>>>> mediated
>>>> matrix device via its 'assign_adapter' sysfs attribute file.
>>>>
>>>> * The AP Queue (AQM) controls access to the AP queues. Each bit
>>>> in the AQM represents an AP queue index - from most significant
>>>> to least significant bit - from 0 to 255. A queue index
>>>> references
>>>> a specific domain and is synonymous with the domian number. The
>>>> bits in the AQM are set according to the domain numbers assigned
>>>> to the mediated matrix device via its 'assign_domain' sysfs
>>>> attribute file.
>>>>
>>>> * The AP Domain Mask (ADM) controls access to the AP control
>>>> domains.
>>>> Each bit in the ADM represents a control domain - from most
>>>> significant to least significant bit - from 0-255. The
>>>> bits in the ADM are set according to the domain numbers assigned
>>>> to the mediated matrix device via its 'assign_control_domain'
>>>> sysfs attribute file.
>>>>
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>>>> ---
>>>> drivers/s390/crypto/vfio_ap_ops.c | 50
>>>> +++++++++++++++++++++++++++++++++
>>>> drivers/s390/crypto/vfio_ap_private.h | 2 +
>>>> 2 files changed, 52 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>>> index bc2b05e..e3ff5ab 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>>> @@ -53,6 +53,54 @@ static int vfio_ap_mdev_remove(struct
>>>> mdev_device *mdev)
>>>> return 0;
>>>> }
>>>>
>>>> +static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>>>> + unsigned long action, void *data)
>>>> +{
>>>> + struct ap_matrix_mdev *matrix_mdev;
>>>> +
>>>> + if (action == VFIO_GROUP_NOTIFY_SET_KVM) {
>>>> + matrix_mdev = container_of(nb, struct ap_matrix_mdev,
>>>> + group_notifier);
>>>> + matrix_mdev->kvm = data;
>>>> + }
>>>> +
>>>> + return NOTIFY_OK;
>>>> +}
>>>> +
>>>> +static int vfio_ap_mdev_open(struct mdev_device *mdev)
>>>> +{
>>>> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>>> + unsigned long events;
>>>> + int ret;
>>>> +
>>>> + matrix_mdev->group_notifier.notifier_call =
>>>> vfio_ap_mdev_group_notifier;
>>>> + events = VFIO_GROUP_NOTIFY_SET_KVM;
>>>> +
>>>> + ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>>> + &events, &matrix_mdev->group_notifier);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = kvm_ap_interpret_instructions(matrix_mdev->kvm, true);
>>>
>>> Do you need this call ?
>>> apie is always enabled in KVM if AP instructions are available.
>>
>> I suppose we don't, in which case we don't need the
>> kvm_ap_interpret_instructions()
>> function either ... at least not until we implement interception.
>>
>>>
>>>
>>> Setting or not the interpretation is done for the VM in a all.
>>> It is not the right place to do it here since open is device dependent.
>>
>> As I stated above, at this time we probably do not need this, however;
>> that will not always be the case. The setting is and always will be
>> for the
>> VM in all - unless the architecture changes - because it is
>> controlled by a
>> single bit (ECA.28). If you recall, I originally set interpretation
>> in the
>> vfio_ap device driver when notified of the VFIO_GROUP_NOTIFY_SET_KVM
>> event.
>> I believe ultimately that it is the device driver that should set the
>> value
>> for apie.
>>
>>
>>
>>>
>>>
>>> Or we only have one device in the VM at a time.
>>> In this case, shouldn't we make it official by returning -EEXIST for
>>> the second call?
>>
>> We do allow only one vfio-ap device at a time. QEMU will allow only
>> one vfio-ap device
>> to be configured for a guest. Should we also put a check in here?
>
> QEMU is not the only possible user of this interface.

True .... I will put a check in here to make sure only one device is
created.

>
>
>>
>>>
>>>
>>>
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = kvm_ap_configure_matrix(matrix_mdev->kvm,
>>>> + matrix_mdev->matrix);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void vfio_ap_mdev_release(struct mdev_device *mdev)
>>>> +{
>>>> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>>> +
>>>> + kvm_ap_deconfigure_matrix(matrix_mdev->kvm);
>>>> + kvm_ap_interpret_instructions(matrix_mdev->kvm, false);
>>>
>>> This call clears the apie in KVM.
>>> This is only OK if we have a single device present until the end of
>>> the VM,
>>> otherwise AP instructions in the guest will fail after the release
>>> until the end of the VM
>>> or until a new device is plugged.
>>
>> See Message ID:
>> <1523819244-29954-5-git-send-email-akrowiak@linux.vnet.ibm.com> on the
>> qemu mailing list. There will be only one vfio-ap device allowed for
>> the MVP model.
>
> dito.
> Anyone can write a userland application using this interface.

See comments above, not to mention I will probably remove this call.

>
> <2b053349-071e-17ed-6ebd-a37bcfd2f330@linux.vnet.ibm.com>
>>
>>>
>>>
>>>
>>>> + vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>>> + &matrix_mdev->group_notifier);
>>>> +}
>>>> +
>>>> static ssize_t name_show(struct kobject *kobj, struct device
>>>> *dev, char *buf)
>>>> {
>>>> return sprintf(buf, "%s\n", VFIO_AP_MDEV_NAME_HWVIRT);
>>>> @@ -754,6 +802,8 @@ static ssize_t matrix_show(struct device *dev,
>>>> struct device_attribute *attr,
>>>> .mdev_attr_groups = vfio_ap_mdev_attr_groups,
>>>> .create = vfio_ap_mdev_create,
>>>> .remove = vfio_ap_mdev_remove,
>>>> + .open = vfio_ap_mdev_open,
>>>> + .release = vfio_ap_mdev_release,
>>>> };
>>>>
>>>> int vfio_ap_mdev_register(struct ap_matrix *ap_matrix)
>>>> diff --git a/drivers/s390/crypto/vfio_ap_private.h
>>>> b/drivers/s390/crypto/vfio_ap_private.h
>>>> index f248faf..48e2806 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_private.h
>>>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>>>> @@ -31,6 +31,8 @@ struct ap_matrix {
>>>>
>>>> struct ap_matrix_mdev {
>>>> struct kvm_ap_matrix *matrix;
>>>> + struct notifier_block group_notifier;
>>>> + struct kvm *kvm;
>>>> };
>>>>
>>>> static inline struct ap_matrix *to_ap_matrix(struct device *dev)
>>>
>>>
>>
>