| Message ID | 1348621313-47404-1-git-send-email-mjr@cs.wisc.edu |
|---|---|
| State | New |
| Headers | show |
On Tue, 2012-09-25 at 20:01 -0500, mjr@cs.wisc.edu wrote: > From: Matt Renzelmann <mjr@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 <mjr@cs.wisc.edu> > --- > > Alex Williamson <alex.williamson@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, Alex > return offset; > + } > + } > + > return 0; > } >
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 <mjr@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 <mjr@cs.wisc.edu> > > --- > > > > Alex Williamson <alex.williamson@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; > > } > > > >
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) { return offset; + } + } + return 0; }