From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mark McLoughlin <markmc@redhat.com>
Subject: Re: [PATCH 1/1] KVM: IRQ ACK notifier should be used with
	in-kernel irqchip
Date: Fri, 28 Nov 2008 10:25:51 +0000
Message-ID: <1227867951.3643.24.camel@blaa>
References: <1224490030-7984-1-git-send-email-sheng@linux.intel.com>
Reply-To: Mark McLoughlin <markmc@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Cc: Avi Kivity <avi@redhat.com>, kvm@vger.kernel.org
To: Sheng Yang <sheng@linux.intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:38186 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750900AbYK1K1L (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 28 Nov 2008 05:27:11 -0500
In-Reply-To: <1224490030-7984-1-git-send-email-sheng@linux.intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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 ...

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

> +	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.