Re: [v2] Shared guest irq support

[Sheng Yang <sheng@linux.intel.com>]

On Tuesday 14 October 2008 16:45:19 Zhang, Xiantao wrote:
> Hi, Amit/Sheng
> 	See the comments below.

Hi Amit

Can you help to update the patch? Thanks!

And minor comments below. 

> Xiantao
>
> > Amit Shah wrote:
> >> Sheng, can you check if this is fine? I'll need some time to test
> >> this.
> >>
> >> Amit
> >
> > index 71e37a5..6f0af16 100644
> > --- a/arch/x86/kvm/irq.h
> > +++ b/arch/x86/kvm/irq.h
> > @@ -32,6 +32,8 @@
> >
> >  #define PIC_NUM_PINS 16
> >
> > +#define KVM_USERSPACE_IRQ_SOURCE_ID	0
> > +
> >  struct kvm;
> >  struct kvm_vcpu
>
> IA64 side also needs this macro, maybe we can put it in ioapic.h or
> kvm_host.h ?
>
> > diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
> > index 4b06ca8..f01e92e 100644
> > --- a/include/asm-x86/kvm_host.h
> > +++ b/include/asm-x86/kvm_host.h
> > @@ -368,6 +368,9 @@ struct kvm_arch{
> >
> >  	struct page *ept_identity_pagetable;
> >  	bool ept_identity_pagetable_done;
> > +
> > +	unsigned long irq_sources_bitmap;
> > +	unsigned long irq_states[KVM_IOAPIC_NUM_PINS];
> >  };
>
> Asm-ia64/kvm_host.h also needs these two fields to add.  Also do we need
> logic to initialize these two fields?

Seems no need to initialize explicitly, the kzalloc ensure that they would be 
initialized to 0, and we would reserve 0 in source bitmap for userspace 
later.
>
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index dda478e..5d29c50 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1816,7 +1816,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
> >                         goto out;
> >                 if (irqchip_in_kernel(kvm)) {
> >                         mutex_lock(&kvm->lock);
> > -                       kvm_set_irq(kvm, irq_event.irq,
> > irq_event.level); +                       kvm_set_irq(kvm,
> > KVM_USERSPACE_IRQ_SOURCE_ID, +
> >                         irq_event.irq, irq_event.level);
> >                         mutex_unlock(&kvm->lock); r = 0;
> >                 }
> > @@ -4115,6 +4116,9 @@ struct  kvm *kvm_arch_create_vm(void)
> >         INIT_LIST_HEAD(&kvm->arch.active_mmu_pages);
> >         INIT_LIST_HEAD(&kvm->arch.assigned_dev_head);
> >
> > +       /* Reserve bit 0 of irq_sources_bitmap for userspace irq
> > source */ +       kvm->arch.irq_sources_bitmap = 1;
> > +

Amit, could you help to use 1 << KVM_USERSPACE_IRQ_SOURCE_ID here, I come to 
think it's more proper. :)

Thanks!
-- 
regards
Yang, Sheng
> >         return kvm;
> >  }
>
> This change should be applied to kvm-ia64.c aslo.
>
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index d0169f5..55ad76e 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -25,15 +25,23 @@
> >  #include "ioapic.h"
> >
> >  /* This should be called with the kvm->lock mutex held */
> > -void kvm_set_irq(struct kvm *kvm, int irq, int level)
> > +void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int
> >  level) {
> > +       unsigned long *irq_state = (unsigned long
> > *)&kvm->arch.irq_states[irq]; +
> > +       /* Logical OR for level trig interrupt */
> > +       if (level)
> > +               set_bit(irq_source_id, irq_state);
> > +       else
> > +               clear_bit(irq_source_id, irq_state);
> > +
> >         /* Not possible to detect if the guest uses the PIC or the
> >          * IOAPIC.  So set the bit in both. The guest will ignore
> >          * writes to the unused one.
> >          */
> > -       kvm_ioapic_set_irq(kvm->arch.vioapic, irq, level);
> > +       kvm_ioapic_set_irq(kvm->arch.vioapic, irq, !!(*irq_state));
> >  #ifdef CONFIG_X86
> > -       kvm_pic_set_irq(pic_irqchip(kvm), irq, level);
> > +       kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
> >  #endif
> >  }
>
> Seems the logic is strange.  Could you help to elaborate it ? Why not
> implment the logic same with userspace?
> Thanks
> Xiantao
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html