From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34692) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QvwOe-0007OC-CM for qemu-devel@nongnu.org; Tue, 23 Aug 2011 15:12:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QvwOc-0004Ec-Rv for qemu-devel@nongnu.org; Tue, 23 Aug 2011 15:12:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44751) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QvwOc-0004EL-IF for qemu-devel@nongnu.org; Tue, 23 Aug 2011 15:12:22 -0400 From: Alex Williamson Date: Tue, 23 Aug 2011 13:12:19 -0600 In-Reply-To: <20110823182632.GA6489@redhat.com> References: <4E53E328.90601@siemens.com> <20110823181751.GB6326@redhat.com> <1314123707.2859.87.camel@bling.home> <20110823182632.GA6489@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Message-ID: <1314126740.2859.108.camel@bling.home> Mime-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] pci: Error on PCI capability collisions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Jan Kiszka , qemu-devel On Tue, 2011-08-23 at 21:26 +0300, Michael S. Tsirkin wrote: > 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 > > > > > > > > Nothing good can happen when we overlap capabilities > > > > > > > > [ Jan: rebased over qemu, minor formatting ] > > > > > > > > Signed-off-by: Jan Kiszka > > > > > > 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. A few issues with that... Since when is error handling so difficult that we need to pretend that nothing ever fails just to make it easy for the caller? Why is device assignment such a special case? It's actually rather ironic that we're trying to add error checking to catch bugs that real hardware is exposing, but assuming that emulated drivers always get it right. How will a return void help the emulated driver that has a coding error? Alex > > > > --- > > > > 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 > > > >