From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH v3 2/5] KVM: Switch assigned device IRQ forwarding to threaded handler Date: Wed, 03 Nov 2010 23:32:19 +0100 Message-ID: <4CD1E2F3.7040009@web.de> References: <128511f28870098dbd57bdaa081dd30ac2af70df.1288771873.git.jan.kiszka@web.de> <20101103221325.GA1147@amt.cnet> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig076BA759355EF4A0598520E9" Cc: Avi Kivity , kvm , Alex Williamson , "Michael S. Tsirkin" , Jan Kiszka To: Marcelo Tosatti Return-path: Received: from fmmailgate02.web.de ([217.72.192.227]:44964 "EHLO fmmailgate02.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752781Ab0KCWem (ORCPT ); Wed, 3 Nov 2010 18:34:42 -0400 In-Reply-To: <20101103221325.GA1147@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig076BA759355EF4A0598520E9 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Am 03.11.2010 23:13, Marcelo Tosatti wrote: > On Wed, Nov 03, 2010 at 09:11:13AM +0100, Jan Kiszka wrote: >> From: Jan Kiszka >> >> This improves the IRQ forwarding for assigned devices: By using the >> kernel's threaded IRQ scheme, we can get rid of the latency-prone work= >> queue and simplify the code in the same run. >> >> Moreover, we no longer have to hold assigned_dev_lock while raising th= e >> guest IRQ, which can be a lenghty operation as we may have to iterate >> over all VCPUs. The lock is now only used for synchronizing masking vs= =2E >> unmasking of INTx-type IRQs, thus is renames to intx_lock. >> >> Signed-off-by: Jan Kiszka >> --- >> include/linux/kvm_host.h | 12 +---- >> virt/kvm/assigned-dev.c | 106 ++++++++++++++-----------------------= -------- >> 2 files changed, 35 insertions(+), 83 deletions(-) >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index bcf71c7..eaacb5d 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -457,16 +457,8 @@ struct kvm_irq_ack_notifier { >> void (*irq_acked)(struct kvm_irq_ack_notifier *kian); >> }; >> =20 >> -#define KVM_ASSIGNED_MSIX_PENDING 0x1 >> -struct kvm_guest_msix_entry { >> - u32 vector; >> - u16 entry; >> - u16 flags; >> -}; >> - >> struct kvm_assigned_dev_kernel { >> struct kvm_irq_ack_notifier ack_notifier; >> - struct work_struct interrupt_work; >> struct list_head list; >> int assigned_dev_id; >> int host_segnr; >> @@ -477,13 +469,13 @@ struct kvm_assigned_dev_kernel { >> bool host_irq_disabled; >> struct msix_entry *host_msix_entries; >> int guest_irq; >> - struct kvm_guest_msix_entry *guest_msix_entries; >> + struct msix_entry *guest_msix_entries; >> unsigned long irq_requested_type; >> int irq_source_id; >> int flags; >> struct pci_dev *dev; >> struct kvm *kvm; >> - spinlock_t assigned_dev_lock; >> + spinlock_t intx_lock; >> }; >> =20 >> struct kvm_irq_mask_notifier { >> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c >> index ecc4419..d0b07ea 100644 >> --- a/virt/kvm/assigned-dev.c >> +++ b/virt/kvm/assigned-dev.c >> @@ -55,58 +55,31 @@ static int find_index_from_host_irq(struct kvm_ass= igned_dev_kernel >> return index; >> } >> =20 >> -static void kvm_assigned_dev_interrupt_work_handler(struct work_struc= t *work) >> +static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id) >> { >> - struct kvm_assigned_dev_kernel *assigned_dev; >> - int i; >> + struct kvm_assigned_dev_kernel *assigned_dev =3D dev_id; >> + u32 vector; >> + int index; >> =20 >> - assigned_dev =3D container_of(work, struct kvm_assigned_dev_kernel, >> - interrupt_work); >> + if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) { >> + spin_lock(&assigned_dev->intx_lock); >> + disable_irq_nosync(irq); >> + assigned_dev->host_irq_disabled =3D true; >> + spin_unlock(&assigned_dev->intx_lock); >> + } >=20 > Shouldnt this happen in the hardirq handler? Otherwise host will receiv= e > interrupts between ->eoi and this point. >=20 Yeah, there is indeed still a bug. I thought the kernel would automatically keep the line masked until the threaded handler returned, but that's only the case if we set IRQF_ONESHOT. Will fix in v4. Thanks, Jan --------------enig076BA759355EF4A0598520E9 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.15 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/ iEYEARECAAYFAkzR4vcACgkQitSsb3rl5xRNgQCffphBEaepNKwhHOZKkU+aIsZ9 150An0hmv4QHEFl2GCJbB29GZN+gMgpf =RpuB -----END PGP SIGNATURE----- --------------enig076BA759355EF4A0598520E9--