[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] Align PCI capabilities in pci_find_space
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH v2] Align PCI capabilities in pci_find_space |
Date: |
Wed, 26 Sep 2012 08:01:57 -0600 |
On Tue, 2012-09-25 at 21:08 -0600, Alex Williamson wrote:
> On Tue, 2012-09-25 at 20:01 -0500, address@hidden wrote:
> > From: Matt Renzelmann <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >
> > Alex Williamson <address@hidden> 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;
> > }
> >
>
>