Re: [PATCH v3 6/9] vfio: ap: register IOMMU VFIO notifier

[Pierre Morel]

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@xxxxxxxxxxxxx>
> > ---
> > Â 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