On Friday 28 November 2008 18:25:51 Mark McLoughlin wrote:
> Hi,
>
> I just got an oops (with 2.6.28-rc6) when running "qemu-kvm -S
> -pcidevice ..." and immediately quitting rather than starting the guest.
>
> The issue is that at this point ASSIGN_PCI_DEVICE has been called, but
> not ASSIGN_IRQ, so kvm_unregister_irq_ack_notifier() oops when we try
> and remove a notifier which hasn't already been added.
>
> The fix is simple - use hlist_del_init() rather than hlist_del() - but I
> also came across this patch in Avi's tree ...
Yes, that's what I meant to fix. Thanks for point out the bug. It's indeed a
buggy fix for (!kian).
>
> On Mon, 2008-10-20 at 16:07 +0800, Sheng Yang wrote:
> ...
>
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index 55ad76e..9fbbdea 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -58,12 +58,16 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned
> > gsi) void kvm_register_irq_ack_notifier(struct kvm *kvm,
> > struct kvm_irq_ack_notifier *kian)
> > {
> > + /* Must be called with in-kernel IRQ chip, otherwise it's nonsense */
> > + ASSERT(irqchip_in_kernel(kvm));
>
> This is a seriously ugly assertion - there is no reason for the IRQ ACK
> notifier abstraction to know anything about when it is called, and it's
> easy to verify that kvm_register_irq_ack_notifier() is only called with
> the in-kernel irqchip ... it's only called in one place:
>
> if (irqchip_in_kernel(kvm)) {
> /* Register ack nofitier */
> match->ack_notifier.gsi = -1;
> match->ack_notifier.irq_acked =
> kvm_assigned_dev_ack_irq;
> kvm_register_irq_ack_notifier(kvm,
> &match->ack_notifier);
Should be two. Another one is PIT. Of course PIT should also be used with in-
kernel-irqchip. My feeling here this one is not that unnecessary...
Anyway, I think your patches are OK for now.
--
regards
Yang, Sheng
>
> > + ASSERT(kian);
>
> This is bogus; the ack notifier structure is embedded in assigned device
> structure, so we can never pass NULL here - it's not like it's a
> dynamically allocated structure.
>
> > hlist_add_head(&kian->link, &kvm->arch.irq_ack_notifier_list);
> > }
> >
> > -void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
> > - struct kvm_irq_ack_notifier *kian)
> > +void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian)
> > {
> > + if (!kian)
> > + return;
> > hlist_del(&kian->link);
>
> This is where I think you were trying to fix the issue I saw ... but
> again, it's bogus. We will never pass a NULL ack notifier struct, but we
> may well pass one which hasn't been previously registered.
>
> I'm going to follow up with a number of patches to clean some of this
> up.
>
> Cheers,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
