Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback

[Halil Pasic]

On 06/11/2018 11:23 AM, Pierre Morel wrote:
> On 08/06/2018 23:59, Tony Krowiak wrote:
> > On 06/07/2018 01:15 PM, Pierre Morel wrote:
> > >
>
> ...snip...
>
> > > > > > Why maintain a list of kvm_ap_matrix structures if we don't have to; it is stored
> > > > > > with the mediated matrix device which is passed in to all of the vfio_ap driver
> > > > > > callbacks.
> > > > >
> > > > > Because using the vm_list which is a static in kvm makes you stick inside the kvm code.
> >
> > I understand your point here, but even if we did maintain a list of kvm_ap_matrix structures,
> > we still need the kvm code to configure the guest's CRYCB and eventually ECA.28. There is
> > also code in kvm-ap.c that is called from KVM.
>
> The only code from kvm-ap which is called from KVM is temporary code
> waiting for Harald to offer the clean interface to AP instructions.
>
> > The idea behind kvm-ap.c is that all code
> > related to configuration of AP structures in KVM is in this one spot.
>
> This I understand, but the code can be in one spot inside VFIO_AP instead
> of inside KVM.
> Putting the code inside KVM induce dependencies between KVM and AP
> while the kvm/vfio interface allows to avoid this dependency.
>
> The purpose of VFIO_AP is to handle the CRYCB, all get/clear/set crycb masks
> functions should be in VFIO AP.
>
> If we use wrappers in KVM, since the CRYCB is an a SIE extension,
> it is legitimate, the KVM interface to the CRYCB should only
> handle bitmaps and be unaware of the vfio_ap internal structures.
>
>
> Another concern, the kvm_ap_validate_queue_sharing() should not be
> inside KVM because it is a decision of current VFIO_AP driver
> to not share the queues between guest of level 2.
>
> The Z architecture does not allow to share AP queues between
> guests of level 1 but we could re-engineer the AP bus and the '
> VFIO AP to offer queue sharing for guest level 2.
>
> This would be a new VFIO_AP driver (and an AP bus extension).
> We should not have to change KVM for this.


Pierre's proposal makes a lot of sense to me. We would not need to take
the kvm_lock (which we need to traverse the vm_list safely) for the
validation, and we could have immediate validation (which is in my opinion
better).

Also your refcount (which is not a refcout) could go away. You simply
traverse your list and check for duplicates when hooking up the mdev
with KVM.

And my opinion is if we don't have to add code to the kvm module we
better not.

@Janosch: Does core KVM share my opinion?

Regards,
Halil