On Mon, Jul 13, 2009 at 05:39:41PM +0300, Gleb Natapov wrote:
> 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 <gleb@xxxxxxxxxx>
> > > ---
> > > 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?
Yea. Might as well replace with a comment saying the function expects
the mutex to be locked.
> > 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 majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
