From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38598) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGsBi-0004t6-Q6 for qemu-devel@nongnu.org; Wed, 26 Sep 2012 10:02:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TGsBe-0005Q6-IB for qemu-devel@nongnu.org; Wed, 26 Sep 2012 10:02:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26403) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGsBe-0005Pw-9s for qemu-devel@nongnu.org; Wed, 26 Sep 2012 10:02:02 -0400 Message-ID: <1348668117.28860.168.camel@bling.home> From: Alex Williamson Date: Wed, 26 Sep 2012 08:01:57 -0600 In-Reply-To: <1348628902.28860.156.camel@bling.home> References: <1348621313-47404-1-git-send-email-mjr@cs.wisc.edu> <1348628902.28860.156.camel@bling.home> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] Align PCI capabilities in pci_find_space List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mjr@cs.wisc.edu Cc: qemu-devel@nongnu.org On Tue, 2012-09-25 at 21:08 -0600, Alex Williamson wrote: > On Tue, 2012-09-25 at 20:01 -0500, mjr@cs.wisc.edu wrote: > > From: Matt Renzelmann > > > > The current implementation of pci_find_space does not correctly align > > PCI capabilities in the PCI configuration space. This patch fixes > > this issue. > > > > Signed-off-by: Matt Renzelmann > > --- > > > > Alex Williamson wrote: > > > I think you could just search every 4th byte. In fact, this whole used > > > byte-map could be turned into a single uint64_t bitmap for standard > > > config space. Thanks, > > > > I've not tested this version of the patch, in contrast to the last, so > > I'm a bit less confident of its correctness. I did not reimplement it > > as suggested as I'm not that familiar with this code, and instead just > > applied the every 4th byte strategy. > > > > hw/pci.c | 12 ++++++++---- > > 1 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index f855cf3..e99866a 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -1631,11 +1631,15 @@ static int pci_find_space(PCIDevice *pdev, uint8_t size) > > int config_size = pci_config_size(pdev); > > 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) > > + > > + for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i += 4) { > > + if (pdev->used[i]) { > > + offset = i + 4; > > + } else if (i - offset + 1 == size) { > > This test needs to change as well. Looks like it should now be: > > (i - offset + 4 >= size) > > Whereas we were previously calculating the difference from the offset to > the current pointer plus the current unused byte, we're now assuming the > current dword is empty because we're only handing out dword aligned > offsets and it would be broken for something to not mark the first entry > used. Probably worthwhile to also add a comment noting the PCI spec > requires dword alignment for capabilities. Thanks, BTW, rather than assume the rest of the dword is empty, we could just check each dword instead of each byte, something like uint32_t *dword_used = &pdev->used[PCI_CONFIG_HEADER_SIZE]; for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i +=4, dword_used++) { if (*dword_used) { offset = i + 4; } else if (i - offset + 4 >= size) { return offset; } } It also occurs to me that this function is broken for PCIe devices as we should stop at PCI_CONFIG_SPACE_SIZE instead of config_size. There should be a separate allocator for extended config space, or a flag to this function to indicate standard or extended. Thanks, Alex > > return offset; > > + } > > + } > > + > > return 0; > > } > > > >