From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 0/16 v5] Device assignment improvement in userspace Date: Mon, 16 Mar 2009 15:12:11 -0300 Message-ID: <20090316181211.GA3783@amt.cnet> References: <1236865019-30321-1-git-send-email-sheng@linux.intel.com> <49BE1797.4010305@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Sheng Yang , Anthony Liguori , kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mx2.redhat.com ([66.187.237.31]:33867 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752191AbZCPSNV (ORCPT ); Mon, 16 Mar 2009 14:13:21 -0400 Content-Disposition: inline In-Reply-To: <49BE1797.4010305@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Mar 16, 2009 at 11:10:47AM +0200, Avi Kivity wrote: > Sheng Yang wrote: >> Patch 1 and 2 are new ones, all the others had been sent before. >> >> This (huge) patchset, contained: >> >> Patch 1..2 are new interface after reworked device assignment kernel part. >> >> Patch 3..6 are generic capability support mechanism. These may can be adopted >> by QEmu upstream as well. >> >> Patch 7..10 enable MSI with device assignment on KVM. Also due to reworked >> device assignment kernel part discard MSI convert to INTx mechanism, patch 10 >> enable it again in userspace. >> >> Patch 11..13 enable MSI-X with device assignment on KVM. >> >> And Patch 14..16 enable SR-IOV with KVM. >> >> Update from latest series: >> >> 1. Convert to the new ioctl interface. >> 2. Merge capability configuration space with PCIDevice one. >> 3. Support of deassign IRQ(unload driver) with MSI/MSI-X better. >> 4. Not assume IRQ0 means no INTx any longer, but check interrupt pin field in >> configuration space for the judgment. >> >> Please help to review! Thanks! >> > > This looks ready to apply. I'd like Marcelo to look it over, though, > before. Looks good to me, ready to be applied. There is one pending detail in the ioctl interface. Its a minor issue, but might become troublesome later (and can be fixed after the patchset has been applied). The unassign ioctl takes "struct kvm_assigned_irq" and parses its flags to decide what to do, in this way: - If any bit is set in the guest mask (GUEST_INTX, GUEST_MSI, GUEST_MSIX), we disable guest-side interrupt. - Likewise for host, disabling host-side interrupt. host_irq_type = irq_requested_type & KVM_DEV_IRQ_HOST_MASK; guest_irq_type = irq_requested_type & KVM_DEV_IRQ_GUEST_MASK; if (host_irq_type) deassign_host_irq(kvm, assigned_dev); if (guest_irq_type) deassign_guest_irq(kvm, assigned_dev); This is a little confusing. If we simply want to disable _whatever is assigned_ in either guest or host side, we want a UNASSIGN_GUEST/UNASSIGN_HOST pair of flags (this is how the ioctl behaves, but we pass more flags and don't use them effectively). Or, if the unassign ioctl continues to receive guest/host flags with interrupt type detail, it should error out if userspace passed a type that does not match what is currently assigned. The current behaviour is simpler for userspace, but then we'd need not to pass "struct kvm_assigned_irq". Sheng, what do you say?