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

From: Tony Krowiak
Date: Mon Jun 11 2018 - 12:27:10 EST


On 06/11/2018 07:49 AM, Janosch Frank wrote:
On 11.06.2018 13:32, Halil Pasic wrote:

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.
Yes, please!

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).
Please do not use the kvm_lock if possible.

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?
At least I do.

KVM does not care about who has which crypto queue/card.
I'd like to have a driver that does internal bookkeeping and then
registers the crycb with KVM, so the VM can use it.

I am not sure what you mean by "registers the crycb with KVM".
Can you provide more detail?


Regards,
Halil