From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kardashevskiy Subject: Re: [Qemu-devel] [RFC PATCH] qemu pci: pci_add_capability enhancement to prevent damaging config space Date: Sat, 12 May 2012 10:27:46 +1000 Message-ID: <4FADAE82.4020603@ozlabs.ru> References: <4FACB581.2050609@ozlabs.ru> <20120511192031.GB5316@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, Alex Graf , Alex Williamson , anthony@codemonkey.ws, David Gibson To: Jason Baron Return-path: Received: from mail-pb0-f46.google.com ([209.85.160.46]:44465 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758717Ab2ELA1t (ORCPT ); Fri, 11 May 2012 20:27:49 -0400 Received: by pbbrp8 with SMTP id rp8so3901961pbb.19 for ; Fri, 11 May 2012 17:27:49 -0700 (PDT) In-Reply-To: <20120511192031.GB5316@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 12.05.2012 5:20, Jason Baron =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= : > 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. >=20 > Hi, >=20 > I don't quite understand how we get a loop, if 'offset' is supplied t= o > 'pci_add_capability' and there is an overlap we get -EINVAL. Otherwis= e, > 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 lo= op. > what am I missing? This happens only with VFIO. The capability already exists in the config space as it is fetched from the host kernel _before_ msi_init is called. Furthermore, msi_init() is called when VFIO sees this capability in the config space. We probably want to re-add all capabilities, do not know... > Thanks, >=20 > -Jason >=20 >> >> 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, uint= 8_t cap_id, >> } >> >> config =3D pdev->config + offset; >> - config[PCI_CAP_LIST_ID] =3D cap_id; >> - config[PCI_CAP_LIST_NEXT] =3D pdev->config[PCI_CAPABILITY_LIST]= ; >> - pdev->config[PCI_CAPABILITY_LIST] =3D offset; >> - pdev->config[PCI_STATUS] |=3D PCI_STATUS_CAP_LIST; >> + if (config[PCI_CAP_LIST_ID] !=3D cap_id) { >> + config[PCI_CAP_LIST_ID] =3D cap_id; >> + config[PCI_CAP_LIST_NEXT] =3D pdev->config[PCI_CAPABILITY_L= IST]; >> + pdev->config[PCI_CAPABILITY_LIST] =3D offset; >> + pdev->config[PCI_STATUS] |=3D PCI_STATUS_CAP_LIST; >> + } >> memset(pdev->used + offset, 0xFF, size); >> /* Make capability read-only by default */ >> memset(pdev->wmask + offset, 0, size); --=20 With best regards Alexey Kardashevskiy -- icq: 52150396 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53984) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ST0Bd-0000lp-SA for qemu-devel@nongnu.org; Fri, 11 May 2012 20:27:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ST0Bb-000172-Da for qemu-devel@nongnu.org; Fri, 11 May 2012 20:27:53 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:51078) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ST0Bb-00016r-7E for qemu-devel@nongnu.org; Fri, 11 May 2012 20:27:51 -0400 Received: by pbbro12 with SMTP id ro12so5338239pbb.4 for ; Fri, 11 May 2012 17:27:49 -0700 (PDT) Message-ID: <4FADAE82.4020603@ozlabs.ru> Date: Sat, 12 May 2012 10:27:46 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <4FACB581.2050609@ozlabs.ru> <20120511192031.GB5316@redhat.com> In-Reply-To: <20120511192031.GB5316@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Jason Baron Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, Alex Graf , Alex Williamson , anthony@codemonkey.ws, David Gibson 12.05.2012 5:20, Jason Baron написал: > 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? This happens only with VFIO. The capability already exists in the config space as it is fetched from the host kernel _before_ msi_init is called. Furthermore, msi_init() is called when VFIO sees this capability in the config space. We probably want to re-add all capabilities, do not know... > 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); -- With best regards Alexey Kardashevskiy -- icq: 52150396