Re: kvm device assignment and MSI-X masking

[Jan Kiszka <jan.kiszka@siemens.com>]

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