From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH v2 8/8] pci-assign: Update MSI-X config based on table
 writes
Date: Wed, 1 Feb 2012 09:25:39 +0200
Message-ID: <20120201072539.GD26228@redhat.com>
References: <20120201052203.9843.80792.stgit@bling.home>
 <20120201053314.9843.8445.stgit@bling.home>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, avi@redhat.com, jan.kiszka@siemens.com,
	shashidhar.patil@gmail.com
To: Alex Williamson <alex.williamson@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:35235 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753505Ab2BAHZl (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 1 Feb 2012 02:25:41 -0500
Content-Disposition: inline
In-Reply-To: <20120201053314.9843.8445.stgit@bling.home>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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