From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56137) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SSvON-0007QU-0f for qemu-devel@nongnu.org; Fri, 11 May 2012 15:20:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SSvOK-0007vE-P7 for qemu-devel@nongnu.org; Fri, 11 May 2012 15:20:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59369) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SSvOK-0007uz-GP for qemu-devel@nongnu.org; Fri, 11 May 2012 15:20:40 -0400 Date: Fri, 11 May 2012 15:20:31 -0400 From: Jason Baron Message-ID: <20120511192031.GB5316@redhat.com> References: <4FACB581.2050609@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4FACB581.2050609@ozlabs.ru> Subject: Re: [Qemu-devel] [RFC PATCH] qemu pci: pci_add_capability enhancement to prevent damaging config space List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, Alex Graf , Alex Williamson , anthony@codemonkey.ws, David Gibson On Fri, May 11, 2012 at 04:45:21PM +1000, Alexey Kardashevskiy wrote: > Normally the pci_add_capability is called on devices to add new > capability. This is ok for emulated devices which capabilities list > is being built by QEMU. > > In the case of VFIO the capability may already exist and adding new > capability into the beginning of the linked list may create a loop. Hi, I don't quite understand how we get a loop, if 'offset' is supplied to 'pci_add_capability' and there is an overlap we get -EINVAL. Otherwise, we are adding the capability in a new empty space. So, I see how we could get the capability in the list twice, but not how there is a loop. what am I missing? Thanks, -Jason > > For example, the old code destroys the following config > of PCIe Intel E1000E: > > before adding PCI_CAP_ID_MSI (0x05): > 0x34: 0xC8 > 0xC8: 0x01 0xD0 > 0xD0: 0x05 0xE0 > 0xE0: 0x10 0x00 > > after: > 0x34: 0xD0 > 0xC8: 0x01 0xD0 > 0xD0: 0x05 0xC8 > 0xE0: 0x10 0x00 > > As result capabilities 0x01 and 0x05 point to each other. > > The proposed patch does not change capability pointers when > the same type capability is about to add. > > Signed-off-by: Alexey Kardashevskiy > --- > hw/pci.c | 10 ++++++---- > 1 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index aa0c0b8..1f7c924 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, > } > > config = pdev->config + offset; > - config[PCI_CAP_LIST_ID] = cap_id; > - config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST]; > - pdev->config[PCI_CAPABILITY_LIST] = offset; > - pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST; > + if (config[PCI_CAP_LIST_ID] != cap_id) { > + config[PCI_CAP_LIST_ID] = cap_id; > + config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST]; > + pdev->config[PCI_CAPABILITY_LIST] = offset; > + pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST; > + } > memset(pdev->used + offset, 0xFF, size); > /* Make capability read-only by default */ > memset(pdev->wmask + offset, 0, size); > > > -- > Alexey >