From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [v2] Shared guest irq support Date: Wed, 15 Oct 2008 10:36:30 +0800 Message-ID: <200810151036.30748.sheng@linux.intel.com> References: <42DFA526FC41B1429CE7279EF83C6BDC01AFB25A@pdsmsx415.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Cc: "Zhang, Xiantao" , "Amit Shah" , "avi" To: kvm@vger.kernel.org Return-path: Received: from mga09.intel.com ([134.134.136.24]:15745 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751261AbYJOCin (ORCPT ); Tue, 14 Oct 2008 22:38:43 -0400 In-Reply-To: <42DFA526FC41B1429CE7279EF83C6BDC01AFB25A@pdsmsx415.ccr.corp.intel.com> Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: 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