On 05/16/2018 10:41 AM, Pierre Morel wrote:
On 16/05/2018 16:29, Tony Krowiak wrote:
On 05/11/2018 12:08 PM, Halil Pasic wrote:
On 05/07/2018 05:11 PM, Tony Krowiak wrote:
Provides interfaces to manage the AP adapters, usage domains
and control domains assigned to a KVM guest.
The guest's SIE state description has a satellite structure called the
Crypto Control Block (CRYCB) containing three bitmask fields
identifying the adapters, queues (domains) and control domains
assigned to the KVM guest:
[..]
index 00bcfb0..98b53c7 100644
--- a/arch/s390/kvm/kvm-ap.c
+++ b/arch/s390/kvm/kvm-ap.c
@@ -7,6 +7,7 @@
[..]
+
+/**
+ * kvm_ap_validate_queue_sharing
+ *
+ * Verifies that the APQNs derived from the cross product of the
AP adapter IDs
+ * and AP queue indexes comprising the AP matrix are not
configured for
+ * another guest. AP queue sharing is not allowed.
+ *
+ * @kvm: the KVM guest
+ * @matrix: the AP matrix
+ *
+ * Returns 0 if the APQNs are valid, otherwise; returns -EBUSY.
+ */
+static int kvm_ap_validate_queue_sharing(struct kvm *kvm,
+ struct kvm_ap_matrix *matrix)
+{
+ struct kvm *vm;
+ unsigned long *apm, *aqm;
+ unsigned long apid, apqi;
+
+
+ /* No other VM may share an AP Queue with the input VM */
+ list_for_each_entry(vm, &vm_list, vm_list) {
+ if (kvm == vm)
+ continue;
+
+ apm = kvm_ap_get_crycb_apm(vm);
+ if (!bitmap_and(apm, apm, matrix->apm, matrix->apm_max + 1))
+ continue;
+
+ aqm = kvm_ap_get_crycb_aqm(vm);
+ if (!bitmap_and(aqm, aqm, matrix->aqm, matrix->aqm_max + 1))
+ continue;
+
+ for_each_set_bit_inv(apid, apm, matrix->apm_max + 1)
+ for_each_set_bit_inv(apqi, aqm, matrix->aqm_max + 1)
+ kvm_ap_log_sharing_err(vm, apid, apqi);
+
+ return -EBUSY;
+ }
+
+ return 0;
+}
+
+int kvm_ap_configure_matrix(struct kvm *kvm, struct kvm_ap_matrix
*matrix)
+{
+ int ret = 0;
+
+ mutex_lock(&kvm->lock);
You seem to take only kvm->lock, vm_list however (used in
kvm_ap_validate_queue_sharing()) seems to be protected by
kvm_lock.
Can you tell me why is this supposed to be safe?
What is supposed to prevent an execution like
vm1: call kvm_ap_configure_matrix(m1)
vm2: call kvm_ap_configure_matrix(m2)
vm1: call kvm_ap_validate_queue_sharing(m1)
vm2: call kvm_ap_validate_queue_sharing(m2)
vm1: call kvm_ap_set_crycb_masks(m1)
vm2: call kvm_ap_set_crycb_masks(m2)
where, let's say, m1 and m2 are equal in the sense that the
mask values are the same?
vm1 will get the kvm->lock first in your scenario when
kvm_ap_configure_matrix(m1) is invoked. Since the other
functions - i.e., kvm_ap_validate_queue_sharing(m1) and
kvm_ap_set_crycb_masks(m1) - are static and only called
from the kvm_ap_configure_matrix(m1), your scenario
can never happen because vm2 will not get the lock until
kvm_ap_configure_matrix(m1) has completed. I see your
point, however, and maybe I should also acquire the kvm_lock.
AFAIU the locks you are talking about are KVM specific
but the example from Halil use two different VM,
i.e. two different locks are used and vm2 never wait for vw1.
Right you are! Perhaps I need to hold the kvm_lock, at least
in the kvm_ap_validate_queue_sharing() function while looping
over vm_list.
Regards,
Halil
+
+ ret = kvm_ap_validate_queue_sharing(kvm, matrix);
+ if (ret)
+ goto done;
+
+ kvm_ap_set_crycb_masks(kvm, matrix);
+
+done:
+ mutex_unlock(&kvm->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(kvm_ap_configure_matrix);
+
