On Mon, Jul 13, 2009 at 10:29:02AM -0400, Gregory Haskins wrote:
> Gleb Natapov wrote:
> > It is already protected by kvm->lock on device assignment path. Just
> > take the same lock in the PIT code.
> >
> > Signed-off-by: Gleb Natapov <g...@redhat.com>
> > ---
> > arch/x86/kvm/i8254.c | 2 ++
> > virt/kvm/irq_comm.c | 8 ++++----
> > 2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> > index 05e00a8..e1b9016 100644
> > --- a/arch/x86/kvm/i8254.c
> > +++ b/arch/x86/kvm/i8254.c
> > @@ -596,7 +596,9 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32
> > flags)
> > if (!pit)
> > return NULL;
> >
> > + mutex_lock(&kvm->lock);
> > pit->irq_source_id = kvm_request_irq_source_id(kvm);
> > + mutex_unlock(&kvm->lock);
> > if (pit->irq_source_id < 0) {
> > kfree(pit);
> > return NULL;
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index 6c57e46..ce8fcd3 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -210,7 +210,8 @@ int kvm_request_irq_source_id(struct kvm *kvm)
> > unsigned long *bitmap = &kvm->arch.irq_sources_bitmap;
> > int irq_source_id;
> >
> > - mutex_lock(&kvm->irq_lock);
> > + WARN_ON(!mutex_is_locked(&kvm->lock));
> >
>
> Shouldn't this be fatal? (e.g. BUG_ON). I know the usage between
> BUG/WARN is controversial, but it seems to me that something is
> completely broken if you expect it to be locked and its not. Might as
> well fail the system, IMO.
>
Well I don't really care but we have WARN_ON() in the code currently.
Besides the chances are good that even without locking around this
function nothing will break, so why kill host kernel?
> Regards,
> -Greg
>
> > +
> > irq_source_id = find_first_zero_bit(bitmap,
> > sizeof(kvm->arch.irq_sources_bitmap));
> >
> > @@ -221,7 +222,6 @@ int kvm_request_irq_source_id(struct kvm *kvm)
> >
> > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > set_bit(irq_source_id, bitmap);
> > - mutex_unlock(&kvm->irq_lock);
> >
> > return irq_source_id;
> > }
> > @@ -230,9 +230,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int
> > irq_source_id)
> > {
> > int i;
> >
> > + /* during vm destruction this function is called without locking */
> > + WARN_ON(!mutex_is_locked(&kvm->lock) && atomic_read(&kvm->users_count));
> > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> >
> > - mutex_lock(&kvm->irq_lock);
> > if (irq_source_id < 0 ||
> > irq_source_id >= sizeof(kvm->arch.irq_sources_bitmap)) {
> > printk(KERN_ERR "kvm: IRQ source ID out of range!\n");
> > @@ -241,7 +242,6 @@ void kvm_free_irq_source_id(struct kvm *kvm, int
> > irq_source_id)
> > for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > clear_bit(irq_source_id, &kvm->arch.irq_states[i]);
> > clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
> > - mutex_unlock(&kvm->irq_lock);
> > }
> >
> > void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq,
> >
>
>
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
