Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/11/2018 12:50 PM, Halil Pasic wrote: On 06/11/2018 06:26 PM, Tony Krowiak wrote: @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? I'm pretty sure he means copy the masks form the internal bookkeeping in the mdev device to the CRYCB fields. Please work on this assumption. Will do. Halil
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/11/2018 06:26 PM, Tony Krowiak wrote: @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? I'm pretty sure he means copy the masks form the internal bookkeeping in the mdev device to the CRYCB fields. Please work on this assumption. Halil
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
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
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/11/2018 05: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. Based on your, Halil's and Janosch's comments, I will make the changes. Regards, Pierre
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/11/2018 07:32 AM, 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. 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? Okay, I'll make the change. Regards, Halil
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
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. > > Regards, > Halil > signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
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
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
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. Regards, Pierre -- Pierre Morel Linux/KVM/QEMU in Böblingen - Germany
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/07/2018 01:15 PM, Pierre Morel wrote: We have internal structures like the ap_matrix and kvm_ap_matrix which look like the bus/devices we had previously but are differently or not at all integrated with the LDD. What is LDD? Are you talking about dependencies between the vfio_ap device driver and KVM? If so, see my arguments below. Also I think that with a little data structure refactoring you can avoid most of the code in the arch/s390/kvm. How will structure refactoring help us avoid the code for updating the CRYCB in the guest's SIE state description. For example, storing the kvm pointer inside the kvm_ap_matrix and maintaining a list of the kvm_ap_matrix structures allows to easily know if a guest already has an associated mediated device. How is that easier than storing the kvm pointer inside of the mediated matrix device (i.e., struct ap_matrix_mdev) which also contains the struct kvm_ap_matrix? you can put it in ap_matrix_mdev but just the name "kvm_ap_matrix" make the last one a better candidate for my opinion. It's been in ap_matrix_mdev since v2, but I'll consider moving it to kvm_ap_matrix. How does that allow us to avoid the code in arch/s390/kvm? This alone does not. We still need the code to update the CRYCB in the SIE block. I can obviously avoid placing that code in kvm-ap.c and move it to the driver, but I already explained my reasoning for keeping it in KVM. Let's face it, there is no way around the dependency between the vfio_ap device driver and KVM unless guest matrix configuration is managed solely by KVM through KVM interfaces. We get the pointer to KVM from the VFIO interface. That we both discuss on this is sterile. The only one who could say what is right is a S390 KVM maintainer. This would end the discussion. My point was just to say that we have an alternative. 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 idea behind kvm-ap.c is that all code related to configuration of AP structures in KVM is in this one spot.
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 07/06/2018 18:30, Tony Krowiak wrote:
On 06/07/2018 11:20 AM, Pierre Morel wrote:
On 07/06/2018 15:54, Tony Krowiak wrote:
On 06/06/2018 01:40 PM, Pierre Morel wrote:
On 06/06/2018 18:08, Pierre Morel wrote:
On 06/06/2018 16:28, Tony Krowiak wrote:
On 06/05/2018 08:19 AM, Pierre Morel wrote:
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive
notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the
guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all
interfaces that
access KVM in kvm-ap because I don't think it is appropriate
for the device
driver to have to have "knowledge" of the inner workings of
KVM. Why does
it matter whether any entity outside of the vfio_ap device
driver calls
these functions? I could ask a similar question if the
interfaces were
contained in vfio-ap; what if another device driver needs
access to these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it
not possible
that future drivers - e.g., when full virtualization is
implemented - will
require access to KVM?
I do not think that an access to KVM is required for full
virtualization.
You may be right, but at this point, there is no guarantee. I
stand by my
design on this one.
I really regret that we abandoned the initial design with the
matrix bus and one
single parent matrix device per guest.
We would not have the problem of these KVM dependencies.
It had the advantage of taking care of having only one device per
guest
(available_instance = 1), could take care of provisioning as you have
sysfs entries available for a matrix without having a guest and a
mediated
device.
it also had advantage for virtualization to keep host side and
guest side matrix
separate inside parent (host side) and mediated device (guest side).
Shouldn't we treat this problem with a design using standard
interfaces
Instead of adding new dedicated interfaces?
Regards,
Pierre
Forget it.
I am not happy with the design but the design I was speaking of may
not be the solution either.
The AP architecture makes virtualization of AP devices complex. We
tried the solution you
described and found it to be sorely lacking which is why we ended up
where we are now.
I did not see any explanation on why between v1 and v2 as it was
abandoned.
We have internal structures like the ap_matrix and kvm_ap_matrix
which look like the bus/devices we had previously but are differently
or not at all integrated with the LDD.
What is LDD? Are you talking about dependencies between the vfio_ap
device
driver and KVM? If so, see my arguments below.
Also I think that with a little data structure refactoring you can
avoid most of
the code in the arch/s390/kvm.
How will structure refactoring help us avoid the code for updating the
CRYCB
in the guest's SIE state description.
For example, storing the kvm pointer inside the kvm_ap_matrix and
maintaining a list of the kvm_ap_matrix structures allows to easily know
if a guest already has an associated mediated device.
How is that easier than storing the kvm pointer inside of the mediated
matrix
device (i.e., struct ap_matrix_mdev) which also contains the struct
kvm_ap_matrix?
you can put it in ap_matrix_mdev but just the name "kvm_ap_matrix" make the
last one a better candidate for my opinion.
How does that allow us to avoid the code in arch/s390/kvm?
This alone does not.
We still need the code
to update the CRYCB in the SIE block. I can obviously avoid placing
that code in
kvm-ap.c and move it to the driver, but I already explained my
reasoning for
keeping it in KVM. Let's face it, there is no way around the
dependency between
the vfio_ap device driver and KVM unless guest matrix configuration is
managed
solely by KVM through KVM interfaces.
We get the pointer to KVM from the VFIO interface.
That we both discuss on this is sterile.
The only one who could
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/07/2018 11:20 AM, Pierre Morel wrote:
On 07/06/2018 15:54, Tony Krowiak wrote:
On 06/06/2018 01:40 PM, Pierre Morel wrote:
On 06/06/2018 18:08, Pierre Morel wrote:
On 06/06/2018 16:28, Tony Krowiak wrote:
On 06/05/2018 08:19 AM, Pierre Morel wrote:
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all
interfaces that
access KVM in kvm-ap because I don't think it is appropriate
for the device
driver to have to have "knowledge" of the inner workings of
KVM. Why does
it matter whether any entity outside of the vfio_ap device
driver calls
these functions? I could ask a similar question if the
interfaces were
contained in vfio-ap; what if another device driver needs
access to these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not
possible
that future drivers - e.g., when full virtualization is
implemented - will
require access to KVM?
I do not think that an access to KVM is required for full
virtualization.
You may be right, but at this point, there is no guarantee. I
stand by my
design on this one.
I really regret that we abandoned the initial design with the
matrix bus and one
single parent matrix device per guest.
We would not have the problem of these KVM dependencies.
It had the advantage of taking care of having only one device per
guest
(available_instance = 1), could take care of provisioning as you have
sysfs entries available for a matrix without having a guest and a
mediated
device.
it also had advantage for virtualization to keep host side and
guest side matrix
separate inside parent (host side) and mediated device (guest side).
Shouldn't we treat this problem with a design using standard
interfaces
Instead of adding new dedicated interfaces?
Regards,
Pierre
Forget it.
I am not happy with the design but the design I was speaking of may
not be the solution either.
The AP architecture makes virtualization of AP devices complex. We
tried the solution you
described and found it to be sorely lacking which is why we ended up
where we are now.
I did not see any explanation on why between v1 and v2 as it was
abandoned.
We have internal structures like the ap_matrix and kvm_ap_matrix
which look like the bus/devices we had previously but are differently
or not at all integrated with the LDD.
What is LDD? Are you talking about dependencies between the vfio_ap device
driver and KVM? If so, see my arguments below.
Also I think that with a little data structure refactoring you can
avoid most of
the code in the arch/s390/kvm.
How will structure refactoring help us avoid the code for updating the CRYCB
in the guest's SIE state description.
For example, storing the kvm pointer inside the kvm_ap_matrix and
maintaining a list of the kvm_ap_matrix structures allows to easily know
if a guest already has an associated mediated device.
How is that easier than storing the kvm pointer inside of the mediated
matrix
device (i.e., struct ap_matrix_mdev) which also contains the struct
kvm_ap_matrix?
How does that allow us to avoid the code in arch/s390/kvm? We still need
the code
to update the CRYCB in the SIE block. I can obviously avoid placing that
code in
kvm-ap.c and move it to the driver, but I already explained my reasoning
for
keeping it in KVM. Let's face it, there is no way around the dependency
between
the vfio_ap device driver and KVM unless guest matrix configuration is
managed
solely by KVM through KVM interfaces.
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.
Pierre
Sorry for the noise.
Regards,
Pierre
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 07/06/2018 15:54, Tony Krowiak wrote:
On 06/06/2018 01:40 PM, Pierre Morel wrote:
On 06/06/2018 18:08, Pierre Morel wrote:
On 06/06/2018 16:28, Tony Krowiak wrote:
On 06/05/2018 08:19 AM, Pierre Morel wrote:
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all
interfaces that
access KVM in kvm-ap because I don't think it is appropriate
for the device
driver to have to have "knowledge" of the inner workings of
KVM. Why does
it matter whether any entity outside of the vfio_ap device
driver calls
these functions? I could ask a similar question if the
interfaces were
contained in vfio-ap; what if another device driver needs
access to these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not
possible
that future drivers - e.g., when full virtualization is
implemented - will
require access to KVM?
I do not think that an access to KVM is required for full
virtualization.
You may be right, but at this point, there is no guarantee. I stand
by my
design on this one.
I really regret that we abandoned the initial design with the matrix
bus and one
single parent matrix device per guest.
We would not have the problem of these KVM dependencies.
It had the advantage of taking care of having only one device per guest
(available_instance = 1), could take care of provisioning as you have
sysfs entries available for a matrix without having a guest and a
mediated
device.
it also had advantage for virtualization to keep host side and guest
side matrix
separate inside parent (host side) and mediated device (guest side).
Shouldn't we treat this problem with a design using standard interfaces
Instead of adding new dedicated interfaces?
Regards,
Pierre
Forget it.
I am not happy with the design but the design I was speaking of may
not be the solution either.
The AP architecture makes virtualization of AP devices complex. We
tried the solution you
described and found it to be sorely lacking which is why we ended up
where we are now.
I did not see any explanation on why between v1 and v2 as it was abandoned.
We have internal structures like the ap_matrix and kvm_ap_matrix
which look like the bus/devices we had previously but are differently
or not at all integrated with the LDD.
Also I think that with a little data structure refactoring you can avoid
most of
the code in the arch/s390/kvm.
For example, storing the kvm pointer inside the kvm_ap_matrix and
maintaining a list of the kvm_ap_matrix structures allows to easily know
if a guest already has an associated mediated device.
Pierre
Sorry for the noise.
Regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/06/2018 01:40 PM, Pierre Morel wrote:
On 06/06/2018 18:08, Pierre Morel wrote:
On 06/06/2018 16:28, Tony Krowiak wrote:
On 06/05/2018 08:19 AM, Pierre Morel wrote:
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all
interfaces that
access KVM in kvm-ap because I don't think it is appropriate for
the device
driver to have to have "knowledge" of the inner workings of KVM.
Why does
it matter whether any entity outside of the vfio_ap device
driver calls
these functions? I could ask a similar question if the
interfaces were
contained in vfio-ap; what if another device driver needs access
to these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not
possible
that future drivers - e.g., when full virtualization is
implemented - will
require access to KVM?
I do not think that an access to KVM is required for full
virtualization.
You may be right, but at this point, there is no guarantee. I stand
by my
design on this one.
I really regret that we abandoned the initial design with the matrix
bus and one
single parent matrix device per guest.
We would not have the problem of these KVM dependencies.
It had the advantage of taking care of having only one device per guest
(available_instance = 1), could take care of provisioning as you have
sysfs entries available for a matrix without having a guest and a
mediated
device.
it also had advantage for virtualization to keep host side and guest
side matrix
separate inside parent (host side) and mediated device (guest side).
Shouldn't we treat this problem with a design using standard interfaces
Instead of adding new dedicated interfaces?
Regards,
Pierre
Forget it.
I am not happy with the design but the design I was speaking of may
not be the solution either.
The AP architecture makes virtualization of AP devices complex. We tried
the solution you
described and found it to be sorely lacking which is why we ended up
where we are now.
Sorry for the noise.
Regards,
Pierre
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/06/2018 12:08 PM, Pierre Morel wrote:
On 06/06/2018 16:28, Tony Krowiak wrote:
On 06/05/2018 08:19 AM, Pierre Morel wrote:
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all
interfaces that
access KVM in kvm-ap because I don't think it is appropriate for
the device
driver to have to have "knowledge" of the inner workings of KVM.
Why does
it matter whether any entity outside of the vfio_ap device driver
calls
these functions? I could ask a similar question if the interfaces
were
contained in vfio-ap; what if another device driver needs access
to these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not
possible
that future drivers - e.g., when full virtualization is implemented
- will
require access to KVM?
I do not think that an access to KVM is required for full
virtualization.
You may be right, but at this point, there is no guarantee. I stand
by my
design on this one.
I really regret that we abandoned the initial design with the matrix
bus and one
single parent matrix device per guest.
This is an interesting time to be bringing this up.
We would not have the problem of these KVM dependencies.
How does that eliminate these KVM dependencies? We would still have to
configure
the guest's SIE state description - i.e., ECA.28 and the CRYCB - regardless
of the number or purpose of the matrix devices. To what KVM dependencies are
you referring?
It had the advantage of taking care of having only one device per guest
(available_instance = 1),
Maybe you didn't state this as you intended, but when you refer to
available_instances, you are referring to mediated devices. We allow
only one mediated device per guest in the current design. I suspect
that is not what you meant here.
could take care of provisioning as you have
sysfs entries available for a matrix without having a guest and a
mediated
device.
I assume here that you are saying that the matrix configuration would be
done via sysfs files for the matrix device as opposed to the mediated
device?
it also had advantage for virtualization to keep host side and guest
side matrix
separate inside parent (host side) and mediated device (guest side).
In my opinion, since the AP devices assigned to the matrix device are
used only by
a guest (i.e., pass-through) and never by the host, it is all guest side
configuration.
Even if we map virtual AP devices to real AP devices, the mapping is still
guest side configuration from my perspective. I think this can all be
handled
by using differing mediated device types for pass-through, virtualized
and emulated
devices. In fact, early on I prototyped the mediated device sysfs
structures for
configuring all three mediated device types if you recall. I see no
advantage
to keeping separate configurations for host and guest sides and in fact
think it
complicates things.
Shouldn't we treat this problem with a design using standard interfaces
Instead of adding new dedicated interfaces?
I do not understand this question. I believe we are using standard
interfaces.
We use the bind/unbind interface to reserve queues for use by guests and
have
sysfs attributes for the mediated devices that map directly to the APM, AQM
and ADM. What do you mean by dedicated interfaces?
In fact, I think the design about which you speak introduces a need for
non-standard and confusing interfaces. For example, think about securing
AP queues; you'd have to unbind the queues from a device driver on the
AP bus and bind them to a driver on a different bus, the matrix bus.
This would require radical design changes to and/or introduction of
non-standard
interfaces on the AP bus. It would also introduce some unusual sysfs
interfaces
on the matrix driver to validate and comm
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/06/2018 18:08, Pierre Morel wrote:
On 06/06/2018 16:28, Tony Krowiak wrote:
On 06/05/2018 08:19 AM, Pierre Morel wrote:
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all
interfaces that
access KVM in kvm-ap because I don't think it is appropriate for
the device
driver to have to have "knowledge" of the inner workings of KVM.
Why does
it matter whether any entity outside of the vfio_ap device driver
calls
these functions? I could ask a similar question if the interfaces
were
contained in vfio-ap; what if another device driver needs access
to these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not
possible
that future drivers - e.g., when full virtualization is implemented
- will
require access to KVM?
I do not think that an access to KVM is required for full
virtualization.
You may be right, but at this point, there is no guarantee. I stand
by my
design on this one.
I really regret that we abandoned the initial design with the matrix
bus and one
single parent matrix device per guest.
We would not have the problem of these KVM dependencies.
It had the advantage of taking care of having only one device per guest
(available_instance = 1), could take care of provisioning as you have
sysfs entries available for a matrix without having a guest and a
mediated
device.
it also had advantage for virtualization to keep host side and guest
side matrix
separate inside parent (host side) and mediated device (guest side).
Shouldn't we treat this problem with a design using standard interfaces
Instead of adding new dedicated interfaces?
Regards,
Pierre
Forget it.
I am not happy with the design but the design I was speaking of may not
be the solution either.
Sorry for the noise.
Regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/06/2018 16:28, Tony Krowiak wrote:
On 06/05/2018 08:19 AM, Pierre Morel wrote:
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all interfaces
that
access KVM in kvm-ap because I don't think it is appropriate for
the device
driver to have to have "knowledge" of the inner workings of KVM.
Why does
it matter whether any entity outside of the vfio_ap device driver
calls
these functions? I could ask a similar question if the interfaces
were
contained in vfio-ap; what if another device driver needs access
to these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not
possible
that future drivers - e.g., when full virtualization is implemented
- will
require access to KVM?
I do not think that an access to KVM is required for full
virtualization.
You may be right, but at this point, there is no guarantee. I stand by my
design on this one.
I really regret that we abandoned the initial design with the matrix bus
and one
single parent matrix device per guest.
We would not have the problem of these KVM dependencies.
It had the advantage of taking care of having only one device per guest
(available_instance = 1), could take care of provisioning as you have
sysfs entries available for a matrix without having a guest and a mediated
device.
it also had advantage for virtualization to keep host side and guest
side matrix
separate inside parent (host side) and mediated device (guest side).
Shouldn't we treat this problem with a design using standard interfaces
Instead of adding new dedicated interfaces?
Regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 06/05/2018 08:19 AM, Pierre Morel wrote:
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all interfaces
that
access KVM in kvm-ap because I don't think it is appropriate for
the device
driver to have to have "knowledge" of the inner workings of KVM.
Why does
it matter whether any entity outside of the vfio_ap device driver
calls
these functions? I could ask a similar question if the interfaces were
contained in vfio-ap; what if another device driver needs access to
these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not
possible
that future drivers - e.g., when full virtualization is implemented -
will
require access to KVM?
I do not think that an access to KVM is required for full virtualization.
You may be right, but at this point, there is no guarantee. I stand by my
design on this one.
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 30/05/2018 16:33, Tony Krowiak wrote:
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all interfaces that
access KVM in kvm-ap because I don't think it is appropriate for the
device
driver to have to have "knowledge" of the inner workings of KVM. Why
does
it matter whether any entity outside of the vfio_ap device driver calls
these functions? I could ask a similar question if the interfaces were
contained in vfio-ap; what if another device driver needs access to
these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not possible
that future drivers - e.g., when full virtualization is implemented -
will
require access to KVM?
I do not think that an access to KVM is required for full virtualization.
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 05/24/2018 05:08 AM, Pierre Morel wrote:
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all interfaces that
access KVM in kvm-ap because I don't think it is appropriate for the
device
driver to have to have "knowledge" of the inner workings of KVM. Why
does
it matter whether any entity outside of the vfio_ap device driver calls
these functions? I could ask a similar question if the interfaces were
contained in vfio-ap; what if another device driver needs access to
these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
We are going to have to agree to disagree on this one. Is it not possible
that future drivers - e.g., when full virtualization is implemented - will
require access to KVM?
Pierre
...snip...
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 23/05/2018 16:45, Tony Krowiak wrote:
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
You should explain why.
2. Configure access to the AP devices for the guest.
...snip...
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all interfaces that
access KVM in kvm-ap because I don't think it is appropriate for the
device
driver to have to have "knowledge" of the inner workings of KVM. Why does
it matter whether any entity outside of the vfio_ap device driver calls
these functions? I could ask a similar question if the interfaces were
contained in vfio-ap; what if another device driver needs access to these
interfaces?
This is very driver specific and only used during initialization.
It is not a common property of the cryptographic interface.
I really think you should handle this inside the driver.
Pierre
...snip...
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 05/16/2018 04:03 AM, Pierre Morel wrote:
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
2. Configure access to the AP devices for the guest.
Access to AP adapters, usage domains and control domains
is controlled by three bit masks contained in the Crypto Control
Block (CRYCB) referenced from the guest's SIE state description:
* The AP Mask (APM) controls access to the AP adapters. Each bit
in the APM represents an adapter number - from most significant
to least significant bit - from 0 to 255. The bits in the APM
are set according to the adapter numbers assigned to the mediated
matrix device via its 'assign_adapter' sysfs attribute file.
* The AP Queue Mask (AQM) controls access to the AP queues. Each bit
in the AQM represents an AP queue index - from most significant
to least significant bit - from 0 to 255. A queue index references
a specific domain and is synonymous with the domian number. The
bits in the AQM are set according to the domain numbers assigned
to the mediated matrix device via its 'assign_domain' sysfs
attribute file.
* The AP Domain Mask (ADM) controls access to the AP control
domains.
Each bit in the ADM represents a control domain - from most
significant to least significant bit - from 0-255. The
bits in the ADM are set according to the domain numbers assigned
to the mediated matrix device via its 'assign_control_domain'
sysfs attribute file.
Signed-off-by: Tony Krowiak
---
arch/s390/include/asm/kvm-ap.h| 21 ++
arch/s390/include/asm/kvm_host.h |1 +
arch/s390/kvm/kvm-ap.c| 19 +
drivers/s390/crypto/vfio_ap_ops.c | 68
+
drivers/s390/crypto/vfio_ap_private.h |2 +
5 files changed, 111 insertions(+), 0 deletions(-)
diff --git a/arch/s390/include/asm/kvm-ap.h
b/arch/s390/include/asm/kvm-ap.h
index 21fe9f2..68c5a67 100644
--- a/arch/s390/include/asm/kvm-ap.h
+++ b/arch/s390/include/asm/kvm-ap.h
@@ -83,6 +83,27 @@ struct kvm_ap_matrix {
bool kvm_ap_instructions_available(void);
/**
+ * kvm_ap_refcount_read
+ *
+ * Read the AP reference count and return it.
+ */
+int kvm_ap_refcount_read(struct kvm *kvm);
+
+/**
+ * kvm_ap_refcount_inc
+ *
+ * Increment the AP reference count.
+ */
+void kvm_ap_refcount_inc(struct kvm *kvm);
+
+/**
+ * kvm_ap_refcount_dec
+ *
+ * Decrement the AP reference count
+ */
+void kvm_ap_refcount_dec(struct kvm *kvm);
+
+/**
* kvm_ap_configure_matrix
*
* Configure the AP matrix for a KVM guest.
diff --git a/arch/s390/include/asm/kvm_host.h
b/arch/s390/include/asm/kvm_host.h
index 8736cde..5f1ad02 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -717,6 +717,7 @@ struct kvm_s390_crypto {
__u8 aes_kw;
__u8 dea_kw;
__u8 apie;
+atomic_t aprefs;
};
#define APCB0_MASK_SIZE 1
diff --git a/arch/s390/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c
index 98b53c7..848fb37 100644
--- a/arch/s390/kvm/kvm-ap.c
+++ b/arch/s390/kvm/kvm-ap.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include "kvm-s390.h"
@@ -218,6 +219,24 @@ static int kvm_ap_validate_queue_sharing(struct
kvm *kvm,
return 0;
}
+int kvm_ap_refcount_read(struct kvm *kvm)
+{
+return atomic_read(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_read);
+
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all interfaces that
access KVM in kvm-ap because I don't think it is appropriate for the device
driver to have to have "knowledge" of the inner workings of KVM. Why does
it matter whether any entity outside of the vfio_ap device driver calls
these functions? I could ask a similar question if the interfaces were
contained in vfio-ap; what if another device driver needs access to these
interfaces?
+
int kvm_ap_configure_matrix(struct kvm *kvm, struct kvm_ap_matrix
*matrix)
{
int ret = 0;
diff --git a/drivers/s390/crypto/vfio_ap_ops.c
b/drivers/s390/crypto/vfio_ap_ops.c
index 81e03b8..8866b0e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -11,6 +11,8 @@
#include
#include
#include
+#include
+#include
#include "
Re: [PATCH v5 11/13] KVM: s390: implement mediated device open callback
On 07/05/2018 17:11, Tony Krowiak wrote:
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
2. Configure access to the AP devices for the guest.
Access to AP adapters, usage domains and control domains
is controlled by three bit masks contained in the Crypto Control
Block (CRYCB) referenced from the guest's SIE state description:
* The AP Mask (APM) controls access to the AP adapters. Each bit
in the APM represents an adapter number - from most significant
to least significant bit - from 0 to 255. The bits in the APM
are set according to the adapter numbers assigned to the mediated
matrix device via its 'assign_adapter' sysfs attribute file.
* The AP Queue Mask (AQM) controls access to the AP queues. Each bit
in the AQM represents an AP queue index - from most significant
to least significant bit - from 0 to 255. A queue index references
a specific domain and is synonymous with the domian number. The
bits in the AQM are set according to the domain numbers assigned
to the mediated matrix device via its 'assign_domain' sysfs
attribute file.
* The AP Domain Mask (ADM) controls access to the AP control domains.
Each bit in the ADM represents a control domain - from most
significant to least significant bit - from 0-255. The
bits in the ADM are set according to the domain numbers assigned
to the mediated matrix device via its 'assign_control_domain'
sysfs attribute file.
Signed-off-by: Tony Krowiak
---
arch/s390/include/asm/kvm-ap.h| 21 ++
arch/s390/include/asm/kvm_host.h |1 +
arch/s390/kvm/kvm-ap.c| 19 +
drivers/s390/crypto/vfio_ap_ops.c | 68 +
drivers/s390/crypto/vfio_ap_private.h |2 +
5 files changed, 111 insertions(+), 0 deletions(-)
diff --git a/arch/s390/include/asm/kvm-ap.h b/arch/s390/include/asm/kvm-ap.h
index 21fe9f2..68c5a67 100644
--- a/arch/s390/include/asm/kvm-ap.h
+++ b/arch/s390/include/asm/kvm-ap.h
@@ -83,6 +83,27 @@ struct kvm_ap_matrix {
bool kvm_ap_instructions_available(void);
/**
+ * kvm_ap_refcount_read
+ *
+ * Read the AP reference count and return it.
+ */
+int kvm_ap_refcount_read(struct kvm *kvm);
+
+/**
+ * kvm_ap_refcount_inc
+ *
+ * Increment the AP reference count.
+ */
+void kvm_ap_refcount_inc(struct kvm *kvm);
+
+/**
+ * kvm_ap_refcount_dec
+ *
+ * Decrement the AP reference count
+ */
+void kvm_ap_refcount_dec(struct kvm *kvm);
+
+/**
* kvm_ap_configure_matrix
*
* Configure the AP matrix for a KVM guest.
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 8736cde..5f1ad02 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -717,6 +717,7 @@ struct kvm_s390_crypto {
__u8 aes_kw;
__u8 dea_kw;
__u8 apie;
+ atomic_t aprefs;
};
#define APCB0_MASK_SIZE 1
diff --git a/arch/s390/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c
index 98b53c7..848fb37 100644
--- a/arch/s390/kvm/kvm-ap.c
+++ b/arch/s390/kvm/kvm-ap.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include "kvm-s390.h"
@@ -218,6 +219,24 @@ static int kvm_ap_validate_queue_sharing(struct kvm *kvm,
return 0;
}
+int kvm_ap_refcount_read(struct kvm *kvm)
+{
+ return atomic_read(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_read);
+
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
Why are these functions inside kvm-ap ?
Will anyone use this outer of vfio-ap ?
+
int kvm_ap_configure_matrix(struct kvm *kvm, struct kvm_ap_matrix *matrix)
{
int ret = 0;
diff --git a/drivers/s390/crypto/vfio_ap_ops.c
b/drivers/s390/crypto/vfio_ap_ops.c
index 81e03b8..8866b0e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -11,6 +11,8 @@
#include
#include
#include
+#include
+#include
#include "vfio_ap_private.h"
@@ -47,6 +49,70 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
return 0;
}
+static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct ap_matrix_mdev *matrix_mdev;
+
+ if (action == VFIO_GROUP_NOTIFY_SET_KVM) {
+ matrix_mdev = container_of(nb, struct ap_matrix_mdev,
+ group_notifier);
+
[PATCH v5 11/13] KVM: s390: implement mediated device open callback
Implements the open callback on the mediated matrix device.
The function registers a group notifier to receive notification
of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
the vfio_ap device driver will get access to the guest's
kvm structure. With access to this structure the driver will:
1. Ensure that only one mediated device is opened for the guest
2. Configure access to the AP devices for the guest.
Access to AP adapters, usage domains and control domains
is controlled by three bit masks contained in the Crypto Control
Block (CRYCB) referenced from the guest's SIE state description:
* The AP Mask (APM) controls access to the AP adapters. Each bit
in the APM represents an adapter number - from most significant
to least significant bit - from 0 to 255. The bits in the APM
are set according to the adapter numbers assigned to the mediated
matrix device via its 'assign_adapter' sysfs attribute file.
* The AP Queue Mask (AQM) controls access to the AP queues. Each bit
in the AQM represents an AP queue index - from most significant
to least significant bit - from 0 to 255. A queue index references
a specific domain and is synonymous with the domian number. The
bits in the AQM are set according to the domain numbers assigned
to the mediated matrix device via its 'assign_domain' sysfs
attribute file.
* The AP Domain Mask (ADM) controls access to the AP control domains.
Each bit in the ADM represents a control domain - from most
significant to least significant bit - from 0-255. The
bits in the ADM are set according to the domain numbers assigned
to the mediated matrix device via its 'assign_control_domain'
sysfs attribute file.
Signed-off-by: Tony Krowiak
---
arch/s390/include/asm/kvm-ap.h| 21 ++
arch/s390/include/asm/kvm_host.h |1 +
arch/s390/kvm/kvm-ap.c| 19 +
drivers/s390/crypto/vfio_ap_ops.c | 68 +
drivers/s390/crypto/vfio_ap_private.h |2 +
5 files changed, 111 insertions(+), 0 deletions(-)
diff --git a/arch/s390/include/asm/kvm-ap.h b/arch/s390/include/asm/kvm-ap.h
index 21fe9f2..68c5a67 100644
--- a/arch/s390/include/asm/kvm-ap.h
+++ b/arch/s390/include/asm/kvm-ap.h
@@ -83,6 +83,27 @@ struct kvm_ap_matrix {
bool kvm_ap_instructions_available(void);
/**
+ * kvm_ap_refcount_read
+ *
+ * Read the AP reference count and return it.
+ */
+int kvm_ap_refcount_read(struct kvm *kvm);
+
+/**
+ * kvm_ap_refcount_inc
+ *
+ * Increment the AP reference count.
+ */
+void kvm_ap_refcount_inc(struct kvm *kvm);
+
+/**
+ * kvm_ap_refcount_dec
+ *
+ * Decrement the AP reference count
+ */
+void kvm_ap_refcount_dec(struct kvm *kvm);
+
+/**
* kvm_ap_configure_matrix
*
* Configure the AP matrix for a KVM guest.
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 8736cde..5f1ad02 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -717,6 +717,7 @@ struct kvm_s390_crypto {
__u8 aes_kw;
__u8 dea_kw;
__u8 apie;
+ atomic_t aprefs;
};
#define APCB0_MASK_SIZE 1
diff --git a/arch/s390/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c
index 98b53c7..848fb37 100644
--- a/arch/s390/kvm/kvm-ap.c
+++ b/arch/s390/kvm/kvm-ap.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include "kvm-s390.h"
@@ -218,6 +219,24 @@ static int kvm_ap_validate_queue_sharing(struct kvm *kvm,
return 0;
}
+int kvm_ap_refcount_read(struct kvm *kvm)
+{
+ return atomic_read(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_read);
+
+void kvm_ap_refcount_inc(struct kvm *kvm)
+{
+ atomic_inc(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_inc);
+
+void kvm_ap_refcount_dec(struct kvm *kvm)
+{
+ atomic_dec(&kvm->arch.crypto.aprefs);
+}
+EXPORT_SYMBOL(kvm_ap_refcount_dec);
+
int kvm_ap_configure_matrix(struct kvm *kvm, struct kvm_ap_matrix *matrix)
{
int ret = 0;
diff --git a/drivers/s390/crypto/vfio_ap_ops.c
b/drivers/s390/crypto/vfio_ap_ops.c
index 81e03b8..8866b0e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -11,6 +11,8 @@
#include
#include
#include
+#include
+#include
#include "vfio_ap_private.h"
@@ -47,6 +49,70 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
return 0;
}
+static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct ap_matrix_mdev *matrix_mdev;
+
+ if (action == VFIO_GROUP_NOTIFY_SET_KVM) {
+ matrix_mdev = container_of(nb, struct ap_matrix_mdev,
+ group_notifier);
+ matrix_mdev->kvm = data;
+ }
+
+ return NOTIFY_OK;
+}
+
+static int vfio_ap_mdev_open(struct mdev_device *mdev)
+{
+ struct ap_matrix_
