From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50414) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGuvw-00012f-Vq for qemu-devel@nongnu.org; Wed, 26 Sep 2012 12:58:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TGuvr-0005za-5J for qemu-devel@nongnu.org; Wed, 26 Sep 2012 12:58:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46318) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGuvq-0005zT-SW for qemu-devel@nongnu.org; Wed, 26 Sep 2012 12:57:55 -0400 Message-ID: <1348678672.28860.204.camel@bling.home> From: Alex Williamson Date: Wed, 26 Sep 2012 10:57:52 -0600 In-Reply-To: <004401cd9c07$0b36d760$21a48620$@cs.wisc.edu> References: <1348673453-3248-1-git-send-email-mjr@cs.wisc.edu> <1348676771.28860.199.camel@bling.home> <004401cd9c07$0b36d760$21a48620$@cs.wisc.edu> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] Align PCI capabilities in pci_find_space List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Matt Renzelmann Cc: qemu-devel@nongnu.org On Wed, 2012-09-26 at 11:50 -0500, Matt Renzelmann wrote: > > > > > > hw/pci.c | 28 +++++++++++++++++++++------- > > > 1 files changed, 21 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > index f855cf3..2217dda 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -1626,16 +1626,30 @@ 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, uint8_t size, bool include_pcie) > > > { > > > - int config_size = pci_config_size(pdev); > > > + int config_size; > > > int offset = PCI_CONFIG_HEADER_SIZE; > > > 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[PCI_CONFIG_HEADER_SIZE]; > > > + > > > + if (include_pcie) { > > > + assert (pci_config_size(pdev) >= PCIE_CONFIG_SPACE_SIZE); > > > + config_size = PCIE_CONFIG_SPACE_SIZE; > > > + } else { > > > + config_size = PCI_CONFIG_SPACE_SIZE; > > > + } > > > + > > > + /* This approach ensures the capability is dword-aligned, as > > > + required by the PCI specification */ > > > + for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i += 4, dword_used++) > > { > > > > I don't believe there's ever a case where a driver would want space and > > not care if it's in standard or extended config space. They'll want one > > or the other. So we'd be searching two distinct ranges. Thanks, > > > > Alex > > Ah, that makes sense, so would something like this work? I can re-send as a patch once we've got it. > > static int pci_find_space(PCIDevice *pdev, uint8_t size, bool pcie_space) > { > int config_base; > int config_size; > int offset = PCI_CONFIG_HEADER_SIZE; > int i; > uint32_t *dword_used = &pdev->used[PCI_CONFIG_HEADER_SIZE]; This needs to change too. Is there a different alignment requirement for pcie? Seems like you might be better off creating some helper like: pci_find_space(PCIDevice *pdev, unsigned start, unsigned size, unsigned align) Then a pci_find_legacy_space and pci_find_express_space could both call into it. Thanks, Alex > > if (pcie_space) { > assert (pci_config_size(pdev) >= PCIE_CONFIG_SPACE_SIZE); > config_base = PCI_CONFIG_SPACE_SIZE; > config_size = PCIE_CONFIG_SPACE_SIZE; > } else { > config_base = PCI_CONFIG_HEADER_SIZE; > config_size = PCI_CONFIG_SPACE_SIZE; > } > > /* This approach ensures the capability is dword-aligned, as > required by the PCI specification */ > for (i = config_base; i < config_size; i += 4, dword_used++) { > if (*dword_used) > offset = i + 4; > else if (i - offset + 4 >= size) > return offset; > } > > return 0; > } > > Thanks for all your help with this, > Matt > > > > > > + if (*dword_used) { > > > + offset = i + 4; > > > + } else if (i - offset + 4 >= size) { > > > return offset; > > > + } > > > + } > > > + > > > return 0; > > > } > > > > > > @@ -1826,7 +1840,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_space(pdev, size, false); > > > if (!offset) { > > > return -ENOSPC; > > > } > > > > > >