| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <81c8422d-4952-747e-6c91-a613ce6d626e@linux.ibm.com>
Date: Tue, 19 Feb 2019 19:51:13 +0100
From: Pierre Morel <pmorel@...ux.ibm.com>
To: Tony Krowiak <akrowiak@...ux.ibm.com>, borntraeger@...ibm.com
Cc: alex.williamson@...hat.com, cohuck@...hat.com,
linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
kvm@...r.kernel.org, frankja@...ux.ibm.com, pasic@...ux.ibm.com,
david@...hat.com, schwidefsky@...ibm.com,
heiko.carstens@...ibm.com, freude@...ux.ibm.com, mimu@...ux.ibm.com
Subject: Re: [PATCH v3 6/9] vfio: ap: register IOMMU VFIO notifier
On 15/02/2019 23:55, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
>> To be able to use the VFIO interface to facilitate the
>> mediated device memory pining/unpining we need to register
>> a notifier for IOMMU.
>>
>> Signed-off-by: Pierre Morel <pmorel@...ux.ibm.com>
>> ---
>> drivers/s390/crypto/vfio_ap_ops.c | 64
>> +++++++++++++++++++++++++++++++----
>> drivers/s390/crypto/vfio_ap_private.h | 2 ++
>> 2 files changed, 60 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 1851b24..6eddc2c 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -781,6 +781,36 @@ static const struct attribute_group
>> *vfio_ap_mdev_attr_groups[] = {
>> };
>> /**
>> + * vfio_ap_mdev_iommu_notifier: IOMMU notifier callback
>> + *
>> + * @nb: The notifier block
>> + * @action: Action to be taken (VFIO_IOMMU_NOTIFY_DMA_UNMAP)
>> + * @data: the specific unmap structure for vfio_iommu_type1
>> + *
>> + * Unpins the guest IOVA. (The NIB guest address we pinned before).
>> + * Return NOTIFY_OK after unpining on a UNMAP request.
>> + * otherwise, returns NOTIFY_DONE .
>> + */
>> +static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct ap_matrix_mdev *matrix_mdev;
>> +
>> + matrix_mdev = container_of(nb, struct ap_matrix_mdev,
>> iommu_notifier);
>> +
>> + if (action == VFIO_IOMMU_NOTIFY_DMA_UNMAP) {
>> + struct vfio_iommu_type1_dma_unmap *unmap = data;
>> + unsigned long g_pfn = unmap->iova >> PAGE_SHIFT;
>> +
>> + vfio_unpin_pages(mdev_dev(matrix_mdev->mdev), &g_pfn, 1);
>> + return NOTIFY_OK;
>> + }
>> +
>> + return NOTIFY_DONE;
>> +}
>> +
>> +
>> +/**
>> * vfio_ap_mdev_set_kvm
>> *
>> * @matrix_mdev: a mediated matrix device
>> @@ -904,8 +934,7 @@ static void vfio_ap_dissociate_queues(struct
>> ap_matrix_mdev *matrix_mdev)
>> * In the case a queue could not be found return -ENODEV.
>> * Otherwise return 0.
>> */
>> -static __attribute__((unused))
>> - int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
>> +static int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
>
> Maybe this function should be introduced in this patch instead?
>
>> {
>> unsigned long apid, apqi;
>> struct vfio_ap_queue *q;
>> @@ -967,12 +996,32 @@ static int vfio_ap_mdev_open(struct mdev_device
>> *mdev)
>> ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>> &events, &matrix_mdev->group_notifier);
>> - if (ret) {
>> - module_put(THIS_MODULE);
>> - return ret;
>> - }
>> + if (ret)
>> + goto err_group;
>> +
>> + matrix_mdev->iommu_notifier.notifier_call =
>> vfio_ap_mdev_iommu_notifier;
>> + events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>> +
>> + ret = vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>> + &events, &matrix_mdev->iommu_notifier);
>> + if (ret)
>> + goto err_iommu;
>> +
>> + ret = vfio_ap_associate_queues(matrix_mdev);
>> + if (ret)
>> + goto err_associate;
>
> I think the matrix_mdev should be associated with queues when an
> assignment of an adapter or domain is made to the mdev device via its
> sysfs interfaces. I say this because assigning an adapter or domain to
> an mdev device effectively grants ownership of any additional AP queues
> added to the mdev device's AP matrix as a result of the assignment. It
> only makes sense to assign ownership to the vfio_ap_queue objects
> representing the queues at that time. If an adapter or domain is
> dynamically assigned while a guest is using the affected queues, then
> the associations will have to be made at that time and this code will
> likely go bye bye.
>
>> return 0;
>> +
>> +err_associate:
>> + vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>> + &matrix_mdev->iommu_notifier);
>> +err_iommu:
>> + vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>> + &matrix_mdev->group_notifier);
>> +err_group:
>> + module_put(THIS_MODULE);
>> + return ret;
>> }
>> static void vfio_ap_mdev_release(struct mdev_device *mdev)
>> @@ -985,6 +1034,9 @@ static void vfio_ap_mdev_release(struct
>> mdev_device *mdev)
>> vfio_ap_mdev_reset_queues(mdev);
>> vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>> &matrix_mdev->group_notifier);
>> + vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>> + &matrix_mdev->iommu_notifier);
>> + vfio_ap_dissociate_queues(matrix_mdev);
>
> I think the matrix_mdev should be dissociated from queues when an
> adapter or domain is unassigned from the mdev device via its
> sysfs interfaces. I say this because unassigning an adapter or domain
> from an mdev device effectively takes away ownership of any AP queues
> removed from the mdev device's AP matrix as a result of the
> unassignment. It only makes sense to remove ownership from the
> vfio_ap_queue objects representing the queues at that time. This will
> become necessary for the forthcoming dynamic configuration patches.
> If an adapter or domain is dynamically unassigned while a guest is
> using the affected queues, then the dissociation will have to be made
> at that time and this code will likely go bye bye.
Effectively,having a link from the vfio_ap_queue to the mediated device
will simplify the sanity checking during assignment.
I will go this way.
Regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
Powered by blists - more mailing lists