On Tue, Aug 23, 2011 at 12:21:47PM -0600, Alex Williamson wrote: > On Tue, 2011-08-23 at 21:17 +0300, Michael S. Tsirkin wrote: > > On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote: > > > From: Alex Williamson <alex.william...@redhat.com> > > > > > > Nothing good can happen when we overlap capabilities > > > > > > [ Jan: rebased over qemu, minor formatting ] > > > > > > Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> > > > > I'll stick an assert there instead. Normal devices > > don't generate overlapping caps unless there's a bug, > > and device assignment should do it's own checks. > > > > I really have a mind to rip out the used array too. > > So you'd rather kill qemu rather than have a reasonable error return > path... great :( > > Alex
Well that will make it possible to make pci_add_capability return void, less work for callers :) Dev assignment is really the only place where capability offsets need to be verified. > > > --- > > > hw/pci.c | 14 ++++++++++++++ > > > 1 files changed, 14 insertions(+), 0 deletions(-) > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > index 6124790..ff20631 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t > > > cap_id, > > > uint8_t offset, uint8_t size) > > > { > > > uint8_t *config; > > > + int i; > > > + > > > if (!offset) { > > > offset = pci_find_space(pdev, size); > > > if (!offset) { > > > return -ENOSPC; > > > } > > > + } else { > > > + for (i = offset; i < offset + size; i++) { > > > + if (pdev->used[i]) { > > > + fprintf(stderr, "ERROR: %04x:%02x:%02x.%x " > > > + "Attempt to add PCI capability %x at offset " > > > + "%x overlaps existing capability %x at offset > > > %x\n", > > > + pci_find_domain(pdev->bus), > > > pci_bus_num(pdev->bus), > > > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), > > > + cap_id, offset, pdev->config_map[i], i); > > > + return -EFAULT; > > > + } > > > + } > > > } > > > > > > config = pdev->config + offset; > > > -- > > > 1.7.3.4 > >