Re: [PATCH v5 4/7] s390: ap: setup relation betwen KVM and mediated device

[Pierre Morel]

On 19/03/2019 15:23, Pierre Morel wrote:
> On 19/03/2019 12:54, Halil Pasic wrote:
> > On Tue, 19 Mar 2019 10:38:42 +0100
> > Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
> >
> > > On 15/03/2019 19:15, Halil Pasic wrote:
> > > > On Wed, 13 Mar 2019 17:05:01 +0100
> > > > Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
> > > >
> > > > > When the mediated device is open we setup the relation with KVM 
> > > > > unset it
> > > > > when the mediated device is released.
> > > > >
> > > > > We ensure KVM is present on opening of the mediated device.
> > > > >
> > > > > We ensure that KVM survives the mediated device, and establish a 
> > > > > direct
> > > >
> > > > survives?
> > >
> > > what alternative do you prefer?
> >
> > Increase kvm's refcount to ensure the guest is alive when the
> > ap_matrix_mdev is active. An ap mp_matrix becomes active with
> > a successful open() and ceases to be active with a release().
>
> Right, it is mdev usage not mdev.
>
> > Your sentence was materially wrong as the mdev is allowed to outlive
> > the KVM. BTW survive tends to have an 'in spite of' note to it, which
> > outlive does not. vfio-ap is, I hope, not a calamity that threatens
> > the life of KVM ;). https://en.oxforddictionaries.com/definition/survive
>
> Thanks, your description is much better.
>
> > > > > link from KVM to the mediated device to simplify the relationship.
> > > > >
> > > > > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > > > > ---
> > >
> > > ...snip...
> > >
> > > > > ÂÂ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long action, void *data)
> > > > > ÂÂ {
> > > > > -ÂÂÂ int ret;
> > > > > ÂÂÂÂÂÂ struct ap_matrix_mdev *matrix_mdev;
> > > > > ÂÂÂÂÂÂ if (action != VFIO_GROUP_NOTIFY_SET_KVM)
> > > > > ÂÂÂÂÂÂÂÂÂÂ return NOTIFY_OK;
> > > > > ÂÂÂÂÂÂ matrix_mdev = container_of(nb, struct ap_matrix_mdev, 
> > > > > group_notifier);
> > > > > -
> > > > > -ÂÂÂ if (!data) {
> > > > > -ÂÂÂÂÂÂÂ matrix_mdev->kvm = NULL;
> > > > > -ÂÂÂÂÂÂÂ return NOTIFY_OK;
> > > > > -ÂÂÂ }
> > > > > -
> > > > > -ÂÂÂ ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
> > > > > -ÂÂÂ if (ret)
> > > > > -ÂÂÂÂÂÂÂ return NOTIFY_DONE;
> > > > > -
> > > > > -ÂÂÂ /* If there is no CRYCB pointer, then we can't copy the masks */
> > > > > -ÂÂÂ if (!matrix_mdev->kvm->arch.crypto.crycbd)
> > > > > -ÂÂÂÂÂÂÂ return NOTIFY_DONE;
> > > > > -
> > > > > -ÂÂÂ kvm_arch_crypto_set_masks(matrix_mdev->kvm, 
> > > > > matrix_mdev->matrix.apm,
> > > > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->matrix.aqm,
> > > > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->matrix.adm);
> > > > > +ÂÂÂ matrix_mdev->kvm = data;
> > > > > ÂÂÂÂÂÂ return NOTIFY_OK;
> > > > > ÂÂ }
> > > > > @@ -888,6 +873,12 @@ static int vfio_ap_mdev_open(struct 
> > > > > mdev_device *mdev)
> > > > > ÂÂÂÂÂÂ if (ret)
> > > > > ÂÂÂÂÂÂÂÂÂÂ goto err_group;
> > > > > +ÂÂÂ /* We do not support opening the mediated device without KVM */
> > > > > +ÂÂÂ if (!matrix_mdev->kvm) {
> > > > > +ÂÂÂÂÂÂÂ ret = -ENODEV;
> > > > > +ÂÂÂÂÂÂÂ goto err_group;
> > > > > +ÂÂÂ }
> > > > > +
> > > > > ÂÂÂÂÂÂ matrix_mdev->iommu_notifier.notifier_call = 
> > > > > vfio_ap_mdev_iommu_notifier;
> > > > > ÂÂÂÂÂÂ events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> > > > > @@ -896,8 +887,15 @@ static int vfio_ap_mdev_open(struct 
> > > > > mdev_device *mdev)
> > > > > ÂÂÂÂÂÂ if (ret)
> > > > > ÂÂÂÂÂÂÂÂÂÂ goto err_iommu;
> > > > > +ÂÂÂ ret = vfio_ap_mdev_set_kvm(matrix_mdev);
> > > >
> > > > At this point the matrix_mdev->kvm ain't guaranteed to be valid 
> > > > IMHO. Or
> > > > am I wrong? If I'm right kvm_get_kvm(matrix_mdev->kvm) could be too 
> > > > late.
> > >
> > > What about the if (!matrix_mdev->kvm) 10 lines above ?
> >
> > That check is not sufficient.
> >
> > You should do the kvm_get_kvm() in vfio_ap_mdev_group_notifier(). VFIO
> > must ensure that the kvm pointer you get is valid, in a sense that it
> > points to a valid struct kvm and the kvm object is alive, while you are
> > in the callback. But not beyond.
> >
> > If another thread were to decrement the refcount of the kvm object you
> > would end up with matrix_mdev->kvm pointing to an object that has already
> > died.
> >
> > Does my analysis make sense to you?
>
> Yes thanks the explication is good, it would have been worth to get it 
> the first time.
>
> > > > > +ÂÂÂ if (ret)
> > > > > +ÂÂÂÂÂÂÂ goto err_kvm;
> > > > > +
> > > > > ÂÂÂÂÂÂ return 0;
> > > > > +err_kvm:
> > > > > +ÂÂÂ 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);
> > > > > @@ -906,19 +904,33 @@ static int vfio_ap_mdev_open(struct 
> > > > > mdev_device *mdev)
> > > > > ÂÂÂÂÂÂ return ret;
> > > > > ÂÂ }
> > > > > -static void vfio_ap_mdev_release(struct mdev_device *mdev)
> > > > > +static int vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> > > > > ÂÂ {
> > > > > -ÂÂÂ struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> > > > > +ÂÂÂ struct kvm *kvm = matrix_mdev->kvm;
> > > > > ÂÂÂÂÂÂ if (matrix_mdev->kvm)
> > > > > ÂÂÂÂÂÂÂÂÂÂ kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
> > > >
> > > > This still conditional?
> > >
> > > Yes, nothing to clear if there is no KVM.
> >
> > Since we have ensured the open only works if there is a KVM at that
> > point in time, and we have taken a reference to KVM, I would expect
> > KVM can not go away before we give up our reference.
>
> Right.

Right but based on the assumption we do a kvm_get_kvm() during open.

But now we will do it inside the notifier, so the logic is to do a 
kvm_put_kvm in the notifier too.
This is important because userland will ask us to release the KVM/VFIO 
link through this notifier.
So I will have to rework this part where KVM==NULL in the notifier too.

Regards,
Pierre


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