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

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 2012-02-01 at 09:25 +0200, Michael S. Tsirkin wrote:
> 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.

AIUI, if I don't specifically enable unaligned access in the memory
region .valid struct, the core will reject unaligned accesses.  Thanks,

Alex

> > +
> > +    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 = {