Messages in this thread | | | Subject | Re: [PATCH v5 06/13] KVM: s390: interfaces to manage guest's AP matrix | From | Tony Krowiak <> | Date | Wed, 16 May 2018 10:29:10 -0400 |
| |
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.
> > > 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); >> +
| |