On Mon, Jul 13, 2009 at 05:55:09PM +0300, Michael S. Tsirkin wrote:
> 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.
>
No. The we will not get bug reports if there are problems. Otherwise you
can say the same about each and every WARN_ON() in the code.
> > > 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
--
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
