On 19/03/2019 16:27, Halil Pasic wrote:
On Tue, 19 Mar 2019 15:47:05 +0100
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
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
I think it can be done both ways. If you ensure KVM != NULL if the open
succeeds and take the reference in the notifier. I suppose if open()
fails release() won't be called. But the logic/code in open() would get
quite ugly because the callback could be called assync so that it
overlaps with the rest of open().
Not necessary, but there is more than just the kvm_get_kvm().
When the user calls KVM_DEV_VFIO_GROUP_DEL he asks to break the link
between VFIO and KVM.
Currently we just ignore this instead of stopping all activity
associated with KVM.
But we have more bugs there:
We should not support multiple open of the mdev which will overwrite
matrix->kvm for the same mdev with a different KVM.
I send a bugfix for this.
Not failing open() in case of no KVM is there yet is in my opinion
cleaner anyway.
If we handle correctly the notifiers and the exclusivity, we can do this.
I will make this correctly for the next iteration.
Regards,
Pierre
Regards,
Halil
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
