From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40851) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STlAA-0000Az-Ew for qemu-devel@nongnu.org; Sun, 13 May 2012 22:37:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1STlA7-0000Ko-VD for qemu-devel@nongnu.org; Sun, 13 May 2012 22:37:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62333) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STlA7-0000Ke-Mh for qemu-devel@nongnu.org; Sun, 13 May 2012 22:37:27 -0400 Message-ID: <1336963039.6954.4.camel@bling.home> From: Alex Williamson Date: Sun, 13 May 2012 20:37:19 -0600 In-Reply-To: <4FADAE82.4020603@ozlabs.ru> References: <4FACB581.2050609@ozlabs.ru> <20120511192031.GB5316@redhat.com> <4FADAE82.4020603@ozlabs.ru> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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, Jason Baron , qemu-devel@nongnu.org, Alex Graf , anthony@codemonkey.ws, David Gibson On Sat, 2012-05-12 at 10:27 +1000, Alexey Kardashevskiy wrote: > 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? >=20 >=20 > This happens only with VFIO. >=20 > 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. >=20 > We probably want to re-add all capabilities, do not know... Yep, I've had a msi[1] and msix[2] patches in my vfio tree for a long time, we really want to support this generically for all capabilities though. We either need to detect or allow the caller to specify that the config space is already programmed. Note that even if we don't create a loop, particularly finicky drivers may balk at just changing the order of the capabilities list. Thanks, Alex [1]https://github.com/awilliam/qemu-vfio/commit/a9f04351610ab69e22d90a76d= c85be3269000a9f [2]https://github.com/awilliam/qemu-vfio/commit/b4de3d0436b0260fbc6fcd407= 87c1c92ffca2980 > >> > >> 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 >=20 >=20