From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=45747 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PCwk2-0003Km-Jv for qemu-devel@nongnu.org; Mon, 01 Nov 2010 11:56:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PCwjw-0003E8-42 for qemu-devel@nongnu.org; Mon, 01 Nov 2010 11:56:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9269) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PCwjv-0003Di-Sy for qemu-devel@nongnu.org; Mon, 01 Nov 2010 11:56:08 -0400 From: Alex Williamson In-Reply-To: <4CC9900D.6010100@redhat.com> References: <20101022193845.10161.52907.stgit@s20.home> <20101022203151.10161.17562.stgit@s20.home> <20101023161858.GB11488@redhat.com> <1287852913.5129.8.camel@x201> <4CC9900D.6010100@redhat.com> Content-Type: text/plain; charset="UTF-8" Date: Mon, 01 Nov 2010 09:56:05 -0600 Message-ID: <1288626965.20007.28.camel@x201> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, "Michael S. Tsirkin" On Thu, 2010-10-28 at 17:00 +0200, Avi Kivity wrote: > On 10/23/2010 06:55 PM, Alex Williamson wrote: > > On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote: > > > On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote: > > > > To enable common msix support to be used with pass through devices, > > > > don't attempt to change the BAR if the device already has an > > > > MSI-X capability. This also means we want to pay closer attention > > > > to the size when we map the msix table page, as it isn't necessarily > > > > covering the entire end of the BAR. > > > > > > > > Signed-off-by: Alex Williamson > > > > --- > > > > > > > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++-------------------------- > > > > 1 files changed, 38 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/hw/msix.c b/hw/msix.c > > > > index 43efbd2..4122395 100644 > > > > --- a/hw/msix.c > > > > +++ b/hw/msix.c > > > > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, > > > > { > > > > int config_offset; > > > > uint8_t *config; > > > > - uint32_t new_size; > > > > > > > > - if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1) > > > > - return -EINVAL; > > > > - if (bar_size> 0x80000000) > > > > - return -ENOSPC; > > > > - > > > > - /* Add space for MSI-X structures */ > > > > - if (!bar_size) { > > > > - new_size = MSIX_PAGE_SIZE; > > > > - } else if (bar_size< MSIX_PAGE_SIZE) { > > > > - bar_size = MSIX_PAGE_SIZE; > > > > - new_size = MSIX_PAGE_SIZE * 2; > > > > - } else { > > > > - new_size = bar_size * 2; > > > > - } > > > > - > > > > - pdev->msix_bar_size = new_size; > > > > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH); > > > > - if (config_offset< 0) > > > > - return config_offset; > > > > - config = pdev->config + config_offset; > > > > - > > > > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > > > - /* Table on top of BAR */ > > > > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > > > > - /* Pending bits on top of that */ > > > > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > > > > - bar_nr); > > > > + pdev->msix_bar_size = bar_size; > > > > + > > > > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX); > > > > + > > > > + if (!config_offset) { > > > > + uint32_t new_size; > > > > + > > > > + if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1) > > > > + return -EINVAL; > > > > + if (bar_size> 0x80000000) > > > > + return -ENOSPC; > > > > + > > > > + /* Add space for MSI-X structures */ > > > > + if (!bar_size) { > > > > + new_size = MSIX_PAGE_SIZE; > > > > + } else if (bar_size< MSIX_PAGE_SIZE) { > > > > + bar_size = MSIX_PAGE_SIZE; > > > > + new_size = MSIX_PAGE_SIZE * 2; > > > > + } else { > > > > + new_size = bar_size * 2; > > > > + } > > > > + > > > > + pdev->msix_bar_size = new_size; > > > > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, > > > > + MSIX_CAP_LENGTH); > > > > + if (config_offset< 0) > > > > + return config_offset; > > > > + config = pdev->config + config_offset; > > > > + > > > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > > > + /* Table on top of BAR */ > > > > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > > > > + /* Pending bits on top of that */ > > > > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > > > > + bar_nr); > > > > + } > > > > pdev->msix_cap = config_offset; > > > > /* Make flags bit writeable. */ > > > > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | > > > > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num, > > > > return; > > > > if (size<= offset) > > > > return; > > > > - cpu_register_physical_memory(addr + offset, size - offset, > > > > + cpu_register_physical_memory(addr + offset, > > > > + MIN(size - offset, MSIX_PAGE_SIZE), > > > > > > This is wrong I think, the table might not fit in a single page. > > > You would need to read table size out of from device config. > > > > That's true, but I was hoping to save that for later since we don't seem > > to be running into that problem yet. Current device assignment code > > assumes a single page, and I haven't heard of anyone with a vector table > > that exceeds that yet. Thanks, > > > > Ok; applied. Please add some warning if the condition happens so the > breakage is at least not silent. Looks like this is already handled before we even get here. msix_init() checks nentries against the max supported in a single page and returns -EINVAL if over. So I think we're safely covered until we add support for more pages. Thanks, Alex