Re: [PATCH v6 14/21] s390: vfio-ap: implement mediated device open callback

[Halil Pasic]

On 07/12/2018 06:03 PM, Tony Krowiak wrote:
> > > +staticÂintÂvfio_ap_mdev_open(structÂmdev_deviceÂ*mdev)
> > > +{
> > > +ÂÂÂÂstructÂap_matrix_mdevÂ*matrix_mdevÂ=Âmdev_get_drvdata(mdev);
> > > +ÂÂÂÂstructÂap_matrix_devÂ*matrix_devÂ=
> > > +ÂÂÂÂÂÂÂÂto_ap_matrix_dev(mdev_parent_dev(mdev));
> > > +ÂÂÂÂunsignedÂlongÂevents;
> > > +ÂÂÂÂintÂret;
> > > +
> > > +ÂÂÂÂifÂ(!try_module_get(THIS_MODULE))
> > > +ÂÂÂÂÂÂÂÂreturnÂ-ENODEV;
> > > +
> > > +ÂÂÂÂretÂ=Âvfio_ap_verify_queues_reserved(matrix_dev,Âmatrix_mdev->name,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&matrix_mdev->matrix);
> > > +ÂÂÂÂifÂ(ret)
> > > +ÂÂÂÂÂÂÂÂgotoÂout_err;
> > > +
> > > +ÂÂÂ 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)
> > > +ÂÂÂÂÂÂÂÂgotoÂout_err;
> > > +
> > > +ÂÂÂÂretÂ=Âkvm_ap_validate_crypto_setup(matrix_mdev->kvm);
> >
> > AtÂthisÂpointÂyouÂassumeÂthatÂyourÂvfio_ap_mdev_group_notifierÂcallback
> > wasÂcalledÂwithÂVFIO_GROUP_NOTIFY_SET_KVMÂandÂthatÂyouÂdoÂhave
> > matrix_mdev->kvmÂsetÂupÂproperly.
> >
> > BasedÂonÂhowÂcallbacksÂusuallyÂworkÂthisÂseemsÂratherÂstrange.ÂIt's
> > probablyÂcleanerÂtoÂsetÂupÂheÂcyrcbÂ(includingÂallÂtheÂvalidation
> > thatÂneedsÂtoÂbeÂdoneÂimmediatelyÂbefore)ÂinÂtheÂcallback
> > (vfio_ap_mdev_group_notifier).
> >
> > If that is not viable I think we need a comment here explaining why is this
> > OKÂ(atÂleast).
>
> This was originally in the callback and moved out, to the best of my recollection,
> because the validation at that time was done on the CRYCB and if that validation
> failed, there was no way to notify the client (QEMU) that configuration ofÂthe
> guest's CRYCB failed from the notification callback. This works - at leastÂasÂfar
> as I can tell from testing - because the registration of the notifier invokesÂthe
> notification callback if KVM has already been set and that appears to be theÂcase.
> YouÂareÂcorrect,Âhowever;ÂweÂprobablyÂshouldn'tÂbankÂonÂthatÂgivenÂthat
> I don't think we can guarantee that to be the case 100% of the time. Consequently,
> I will move this back into the notification callback and since consistencyÂchecking
> is now being done on the mdev devices instead of the CRYCB, we don't needÂKVMÂatÂopen
> time.

Sounds good to me. Making the open fail was not a good way to communicate this
error condition to userspace anyway.


Regards,
Halil