From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: kvm device assignment and MSI-X masking Date: Tue, 14 Aug 2012 16:10:43 +0200 Message-ID: <502A5C63.70803@siemens.com> References: <502A572A.5040408@siemens.com> <1344953149.4683.244.camel@ul30vt.home> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: kvm To: Alex Williamson Return-path: Received: from david.siemens.de ([192.35.17.14]:27933 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753218Ab2HNOKr (ORCPT ); Tue, 14 Aug 2012 10:10:47 -0400 In-Reply-To: <1344953149.4683.244.camel@ul30vt.home> Sender: kvm-owner@vger.kernel.org List-ID: On 2012-08-14 16:05, Alex Williamson wrote: > On Tue, 2012-08-14 at 15:48 +0200, Jan Kiszka wrote: >> Hi Alex, >> >> you once wrote this comment in device-assignment.c, msix_mmio_write: >> >> if (!msix_masked(&orig) && msix_masked(entry)) { >> /* >> * Vector masked, disable it >> * >> * XXX It's not clear if we can or should actually attempt >> * to mask or disable the interrupt. KVM doesn't have >> * support for pending bits and kvm_assign_set_msix_entry >> * doesn't modify the device hardware mask. Interrupts >> * while masked are simply not injected to the guest, so >> * are lost. Can we get away with always injecting an >> * interrupt on unmask? >> */ >> >> I'm wondering what made you think that we won't inject if the vector is >> masked like this (ie. in the shadow MSI-X table). Can you recall the >> details? >> >> I'm trying to refactor this code to make the KVM interface a bit more >> encapsulating the kernel interface details, not fixing anything. Still, >> I would also like to avoid introducing regressions. > > Yeah, I didn't leave a very good comment there. I'm sure it made more > sense to me at the time. I think I was trying to say that not only do > we not have a way to mask the physical hardware, but if we did, we don't > have a way to retrieve the pending bits, so any pending interrupts while > masked would be lost. We might be able to deal with that by posting a > spurious interrupt on unmask, but for now we do nothing as masking is > usually done just to update the vector. Thanks, Ok, thanks for the clarification. As we are at it, do you also recall if this --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -1573,28 +1573,7 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr, */ } else if (msix_masked(&orig) && !msix_masked(entry)) { /* Vector unmasked */ - if (i >= adev->irq_entries_nr || !adev->entry[i].type) { - /* Previously unassigned vector, start from scratch */ - assigned_dev_update_msix(pdev); - return; - } else { - /* Update an existing, previously masked vector */ - struct kvm_irq_routing_entry orig = adev->entry[i]; - int ret; - - adev->entry[i].u.msi.address_lo = entry->addr_lo; - adev->entry[i].u.msi.address_hi = entry->addr_hi; - adev->entry[i].u.msi.data = entry->data; - - ret = kvm_update_routing_entry(&orig, &adev->entry[i]); - if (ret) { - fprintf(stderr, - "Error updating irq routing entry (%d)\n", ret); - return; - } - - kvm_irqchip_commit_routes(kvm_state); - } + assigned_dev_update_msix(pdev); } } } would make a relevant difference for known workloads? I'm trying to get rid of direct routing table manipulations, but I would also like to avoid introducing things like kvm_irqchip_update_msi_route unless really necessary. Or could VFIO make use of that as well? Jan PS: Will try to have a look at your main VFIO patch later today. -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux