On Tue, 2012-09-25 at 21:08 -0600, Alex Williamson wrote: > On Tue, 2012-09-25 at 20:01 -0500, m...@cs.wisc.edu wrote: > > From: Matt Renzelmann <m...@cs.wisc.edu> > > > > 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 <m...@cs.wisc.edu> > > --- > > > > Alex Williamson <alex.william...@redhat.com> 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; > > } > > > >