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

Pierre Morel
2019-03-19T09:38:42Z

On 15/03/2019 19:15, Halil Pasic wrote:
> On Wed, 13 Mar 2019 17:05:01 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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?

>
>> link from KVM to the mediated device to simplify the relationship.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---

...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 ?

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

>
>> -
>> + vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
>
> I guess your intention was to move vfio_ap_mdev_reset_queues()
> here from vfio_ap_mdev_release(), but you still have a
> vfio_ap_mdev_reset_queues() call in vfio_ap_mdev_release().
>
>> + matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
>> matrix_mdev->kvm = NULL;
>> +
>> + kvm_put_kvm(kvm);
>> + return 0;
>> +}
>> +
>> +static void vfio_ap_mdev_release(struct mdev_device *mdev)
>> +{
>> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +
>> + mutex_lock(&matrix_dev->lock);
>> +
>> vfio_ap_mdev_reset_queues(mdev);

right, this one will go away.
Thanks for reviewing.

Regards,
Pierre

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