Re: [PATCH v5 06/13] KVM: s390: interfaces to manage guest's AP matrix

[Pierre Morel]

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.

> > 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);
> > > +

--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany