Re: [RFC] kvm irq assignment

[Marcelo Tosatti <mtosatti@redhat.com>]

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.