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 [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
