Message-Id: <105bce81-9359-6096-88ce-01ec1aef523e@linux.vnet.ibm.com>
Date:   Tue, 17 Apr 2018 18:18:50 +0200
From:   Pierre Morel <pmorel@...ux.vnet.ibm.com>
To:     Tony Krowiak <akrowiak@...ux.vnet.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     freude@...ibm.com, schwidefsky@...ibm.com,
        heiko.carstens@...ibm.com, borntraeger@...ibm.com,
        cohuck@...hat.com, kwankhede@...dia.com,
        bjsdjshi@...ux.vnet.ibm.com, pbonzini@...hat.com,
        alex.williamson@...hat.com, alifm@...ux.vnet.ibm.com,
        mjrosato@...ux.vnet.ibm.com, jjherne@...ux.vnet.ibm.com,
        thuth@...hat.com, pasic@...ux.vnet.ibm.com, berrange@...hat.com,
        fiuczy@...ux.vnet.ibm.com, buendgen@...ibm.com
Subject: Re: [PATCH v4 13/15] KVM: s390: configure the guest's AP devices

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@...ux.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.

>
>>
>>
>>
>>> +    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@...ux.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.

>
>>
>>
>>
>>> + 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)
>>
>>
>

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

Powered by blists - more mailing lists