From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [RFC] kvm irq assignment
Date: Sat, 14 Jun 2008 20:32:01 -0300
Message-ID: <20080614233201.GA24715@dmt.cnet>
References: <51CFAB8CB6883745AE7B93B3E084EBE201CC9077@pdsmsx412.ccr.corp.intel.com> <48517616.1050607@qumranet.com> <51CFAB8CB6883745AE7B93B3E084EBE201CC9210@pdsmsx412.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Avi Kivity <avi@qumranet.com>, Alexander Graf <agraf@suse.de>,
	Jes Sorensen <jes@sgi.com>, kvm@vger.kernel.org,
	kvm-ia64@vger.kernel.org
To: "Xu, Anthony" <anthony.xu@intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([66.187.233.31]:57315 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1755179AbYFNXc1 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sat, 14 Jun 2008 19:32:27 -0400
Content-Disposition: inline
In-Reply-To: <51CFAB8CB6883745AE7B93B3E084EBE201CC9210@pdsmsx412.ccr.corp.intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Hi Anthony,

On Fri, Jun 13, 2008 at 02:38:08PM +0800, Xu, Anthony wrote:
> Hi Avi and all
> 
> This is the revised one,
> 
> All PCI devices send interrupt to both PIC and IOAPIC,  
> a). When PIC is enabled and IOAPIC is disabled,  all redirect entries in
> IOAPIC are masked.
> B) When PIC is disabled and IPAPIC is enabled, link entry bit7 is set,
> means this link entry is disable.
> Guest OS need to guarantee PIC and IOAPIC are not enabled in the same
> time. Otherwise cause many suspicious interrupt to guest.
> 
> Test by running guest linux in kvm/ia32 and kvm/ia64.

Interrupt sharing is stable under Linux, PCI hotplug is happy, and
Windows is happy. Ship it!

I had to apply your patch by hand, your mailer eats newlines and other
nasty things, please fix that (or send attached patches).

>  
> +    Name (PICD, 0)
>  
> -    /* PCI Bus definition */
> +    Method(_PIC, 1)
> +    {
> +        Store(Arg0, PICD)
> +    }
> +
> +    /*PCI Bus definition */

Why did you take off the space before the "P" of PCI? Before you ask me,
no, I don't have anything better to do :)

>      Scope(\_SB) {
>          Device(PCI0) {
>              Name (_HID, EisaId ("PNP0A03"))
>              Name (_ADR, 0x00)
>              Name (_UID, 1)
> -            Name(_PRT, Package() {
> +
> +            Method(_PRT,0){
> +                If(PICD){

Put some spaces there too.

> diff --git a/qemu/hw/pci.c b/qemu/hw/pci.c
> index a23a466..f96fbb5 100644
> --- a/qemu/hw/pci.c
> +++ b/qemu/hw/pci.c
> @@ -27,6 +27,8 @@
>  #include "net.h"
>  #include "pc.h"
>  
> +#include "qemu-kvm.h"
> +
>  //#define DEBUG_PCI
>  
>  struct PCIBus {
> @@ -534,12 +536,18 @@ static void pci_set_irq(void *opaque, int irq_num,
> int level)
>      PCIDevice *pci_dev = (PCIDevice *)opaque;
>      PCIBus *bus;
>      int change;
> -
> +#ifdef KVM_CAP_IRQCHIP
> +    int irq;
> +#endif 
>      change = level - pci_dev->irq_state[irq_num];
>      if (!change)
>          return;
>  
>      pci_dev->irq_state[irq_num] = level;
> +#ifdef KVM_CAP_IRQCHIP
> +    irq = ioapic_map_irq(pci_dev->devfn, irq_num);
> +    ioapic_set_irq(opaque, irq, change);
> +#endif

I think you should avoid any changes to pci.c. Perhaps create a new
ioapic_and_pic_map / ioapic_and_pic_set pair of functions and change
pc.c to use that instead of piix_set_irq.

Other than that (and KVM_CAP_IRQCHIP mentioned by Avi, along with making
sure this works with "-no-kvm") looks great.

Regarding the non-PIIX link devices I mentioned, that can be done later
if necessary.