From patchwork Mon Oct 22 10:44:11 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 193122 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id A95C82C0096 for ; Mon, 22 Oct 2012 20:42:24 +1100 (EST) Received: from localhost ([::1]:52518 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQEWc-0001BJ-Mo for incoming@patchwork.ozlabs.org; Mon, 22 Oct 2012 05:42:22 -0400 Received: from eggs.gnu.org ([208.118.235.92]:50368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQEWV-0001B8-7i for qemu-devel@nongnu.org; Mon, 22 Oct 2012 05:42:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQEWO-0003DE-Em for qemu-devel@nongnu.org; Mon, 22 Oct 2012 05:42:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32794) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQEWO-0003DA-6L for qemu-devel@nongnu.org; Mon, 22 Oct 2012 05:42:08 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9M9g2a7005509 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 22 Oct 2012 05:42:02 -0400 Received: from redhat.com (vpn1-7-193.ams2.redhat.com [10.36.7.193]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id q9M9fw5S009847; Mon, 22 Oct 2012 05:41:59 -0400 Date: Mon, 22 Oct 2012 12:44:11 +0200 From: "Michael S. Tsirkin" To: Matt Renzelmann Message-ID: <20121022104411.GA28814@redhat.com> References: <1350766872-4623-1-git-send-email-mjr@cs.wisc.edu> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1350766872-4623-1-git-send-email-mjr@cs.wisc.edu> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: blauwirbel@gmail.com, alex.williamson@redhat.com, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCHv5] Align PCI capabilities in pci_find_space X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Sat, Oct 20, 2012 at 04:01:12PM -0500, Matt Renzelmann wrote: > The current implementation of pci_find_space does not correctly align > PCI capabilities in the PCI configuration space. It also does not > support PCI-Express devices. This patch fixes these issues. > > Thanks to Alex Williamson for feedback. > > Signed-off-by: Matt Renzelmann > --- > > Re-sending to add CC Michael S. Tsirkin . Thanks > Andreas for pointing out my mistake. > > hw/pci.c | 36 ++++++++++++++++++++++++++++-------- > 1 files changed, 28 insertions(+), 8 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 2ca6ff6..4b617f6 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1644,19 +1644,39 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name) > return pci_create_simple_multifunction(bus, devfn, false, name); > } > > -static int pci_find_space(PCIDevice *pdev, uint8_t size) > +static int pci_find_space(PCIDevice *pdev, uint32_t start, > + uint32_t end, uint32_t size) > { > - int config_size = pci_config_size(pdev); > - int offset = PCI_CONFIG_HEADER_SIZE; > + int offset = start; > int i; > - for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i) > - if (pdev->used[i]) > - offset = i + 1; > - else if (i - offset + 1 == size) > + uint32_t *dword_used = &pdev->used[start]; > + > + assert(pci_config_size(pdev) >= end); > + assert(!(start & 0x3)); > + > + /* This approach ensures the capability is dword-aligned, as > + required by the PCI and PCI-E specifications */ > + for (i = start; i < end; i += 4, dword_used++) { > + if (*dword_used) { > + offset = i + 4; > + } else if (i - offset + 4 >= size) { > return offset; > + } > + } > + > return 0; > } I agree ability to get misaligned capabilities is a bug. Thanks for reorting this. But it seems easier to fix just by aligning size. See patch below. > > +static int pci_find_legacy_space(PCIDevice *pdev, uint8_t size) { > + return pci_find_space(pdev, PCI_CONFIG_HEADER_SIZE, > + PCI_CONFIG_SPACE_SIZE, size); > +} I think it makes more sense to make pci_find_space imply legacy and add a new API for express. This is exactly what patches that Jason Baron posted do, so I'll apply them instead. > + > +static int pci_find_express_space(PCIDevice *pdev, uint16_t size) { > + return pci_find_space(pdev, PCI_CONFIG_SPACE_SIZE, > + PCIE_CONFIG_SPACE_SIZE, size); > +} > + This is dead code I think, it's probably not a good idea to add yet at this stage. > static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id, > uint8_t *prev_p) > { > @@ -1844,7 +1864,7 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, > int i, overlapping_cap; > > if (!offset) { > - offset = pci_find_space(pdev, size); > + offset = pci_find_legacy_space(pdev, size); > if (!offset) { > return -ENOSPC; > } Below is what I applied. Thanks for the report! ---> pci: make each capability DWORD aligned PCI spec (see e.g. 6.7 Capabilities List in spec rev 3.0) requires that each capability is DWORD aligned. Ensure this when allocating space by rounding size up to 4. Signed-off-by: Michael S. Tsirkin Reported-by: Matt Renzelmann diff --git a/hw/pci.c b/hw/pci.c index 6a66b32..28fdb19 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -1883,7 +1883,7 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST]; pdev->config[PCI_CAPABILITY_LIST] = offset; pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST; - memset(pdev->used + offset, 0xFF, size); + memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4)); /* Make capability read-only by default */ memset(pdev->wmask + offset, 0, size); /* Check capability by default */ @@ -1903,7 +1903,7 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) memset(pdev->w1cmask + offset, 0, size); /* Clear cmask as device-specific registers can't be checked */ memset(pdev->cmask + offset, 0, size); - memset(pdev->used + offset, 0, size); + memset(pdev->used + offset, 0, QEMU_ALIGN_UP(size, 4)); if (!pdev->config[PCI_CAPABILITY_LIST]) pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;