From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 3/8] KVM: Handle device assignment to guests Date: Thu, 17 Jul 2008 12:50:06 +0300 Message-ID: <487F15CE.5050204@qumranet.com> References: <1216214225-18030-1-git-send-email-benami@il.ibm.com> <1216214225-18030-2-git-send-email-benami@il.ibm.com> <1216214225-18030-3-git-send-email-benami@il.ibm.com> <1216214225-18030-4-git-send-email-benami@il.ibm.com> <487E0E10.4000009@qumranet.com> <1216287258.31546.337.camel@cluwyn.haifa.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: amit.shah@qumranet.com, kvm@vger.kernel.org, Muli Ben-Yehuda , weidong.han@intel.com, anthony@codemonkey.ws To: Ben-Ami Yassour Return-path: Received: from il.qumranet.com ([212.179.150.194]:20873 "EHLO il.qumranet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754760AbYGQJuJ (ORCPT ); Thu, 17 Jul 2008 05:50:09 -0400 In-Reply-To: <1216287258.31546.337.camel@cluwyn.haifa.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: Ben-Ami Yassour wrote: > On Wed, 2008-07-16 at 18:04 +0300, Avi Kivity wrote: > >> Ben-Ami Yassour wrote: >> >>> +/* Stores information for identifying host PCI devices assigned to the >>> + * guest: this is used in the host kernel and in the userspace. >>> + */ >>> +struct kvm_pci_pt_info { >>> + unsigned char busnr; >>> + unsigned int devfn; >>> + __u32 irq; >>> +}; >>> >>> >> Badly aligned. Please use __u32 for all fields, and add a __u32 padding >> field at the end so we have the same ABI for i386 and x86_64. >> >> Some pci devices support more than one irq. Should we make irq an >> array? Alternatively, decouple irq assignment from pci information >> and >> let userspace handle everything. This lets us assign non-pci devices >> (like serial ports, etc.). >> > Can we limit the first version to single irq devices, and handle this > after the merge? > > Yes. But we need to get at least the userspace interface right (and perhaps only partially implement it). Breaking the userspace interface later is possible, but I'd rather avoid it. >>> * ioctls for vcpu fds >>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c >>> index 8ce93c7..0b5bc40 100644 >>> --- a/virt/kvm/ioapic.c >>> +++ b/virt/kvm/ioapic.c >>> @@ -288,13 +288,22 @@ void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level) >>> static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int gsi) >>> { >>> union ioapic_redir_entry *ent; >>> + struct kvm_pci_pt_dev_list *match; >>> + unsigned long flags; >>> >>> ent = &ioapic->redirtbl[gsi]; >>> ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); >>> >>> ent->fields.remote_irr = 0; >>> - if (!ent->fields.mask && (ioapic->irr & (1 << gsi))) >>> - ioapic_service(ioapic, gsi); >>> + >>> + read_lock_irqsave(&kvm_pci_pt_lock, flags); >>> + match = kvm_find_pci_pt_dev(&ioapic->kvm->arch.pci_pt_dev_head, NULL, >>> + gsi, KVM_PT_SOURCE_IRQ_ACK); >>> + read_unlock_irqrestore(&kvm_pci_pt_lock, flags); >>> + if (!match) { >>> + if (!ent->fields.mask && (ioapic->irr & (1 << gsi))) >>> + ioapic_service(ioapic, gsi); >>> + } >>> >>> >> What's device assignment code doing in the ioapic? This should be done >> through the notifier (if it needs to return a value, great). >> >> > > This purpose of this code is to avoid the call to ioapic_service for > assigned devices, because it is not required and causes a double > injection of the interrupt. > > Can you please explain why as part of eoi we inject a new interrupt? > If a level triggered interrupt remains active after the eoi, the ioapic has to inject it. This is used to support shared interrupts, or when the device has re-raised the line by the time the ack arrives. I don't see why it should behave differently for assigned devices. -- error compiling committee.c: too many arguments to function