Re: [PATCH v2 8/8] pci-assign: Update MSI-X config based on table writes

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jan 31, 2012 at 10:33:14PM -0700, Alex Williamson wrote:
> @@ -1438,11 +1448,71 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
>                              uint64_t val, unsigned size)
>  {
>      AssignedDevice *adev = opaque;
> +    PCIDevice *pdev = &adev->dev;
> +    uint16_t ctrl;
> +    MSIXTableEntry orig;
> +    int i = addr >> 4;
> +
> +    if (i >= adev->msix_max) {
> +        return; /* Drop write */
> +    }
>  
> -    DEBUG("write to MSI-X entry table mmio offset 0x%lx, val 0x%lx\n",
> -          addr, val);
> +    ctrl = pci_get_word(pdev->config + pdev->msix_cap + PCI_MSIX_FLAGS);
> +
> +    DEBUG("write to MSI-X table offset 0x%lx, val 0x%lx\n", addr, val);
> +
> +    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
> +        orig = adev->msix_table[i];
> +    }
>  
>      memcpy((void *)((uint8_t *)adev->msix_table + addr), &val, size);

Does the core guarantee alignment even if guest violates the rules?
I ask because we validate i above but don't check the low bits
of addr, so a misaligned call can access beyond the array boundary.

If core does not ensure alignment we must check here ourselves.

> +
> +    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
> +        MSIXTableEntry *entry = &adev->msix_table[i];
> +
> +        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?
> +             */
> +        } 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;
> +                }
> +
> +                ret = kvm_commit_irq_routes();
> +                if (ret) {
> +                    fprintf(stderr,
> +                            "Error committing irq routes (%d)\n", ret);
> +                    return;
> +                }
> +            }
> +        }
> +    }
>  }
>  
>  static const MemoryRegionOps msix_mmio_ops = {