From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kardashevskiy Subject: Re: [RFC PATCH] qemu pci: pci_add_capability enhancement to prevent damaging config space Date: Fri, 11 May 2012 22:47:11 +1000 Message-ID: <4FAD0A4F.2050506@ozlabs.ru> References: <4FACB581.2050609@ozlabs.ru> <6A22E211-BC82-49BD-A335-02D3BAA14A17@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: qemu-devel@nongnu.org, Alex Williamson , Benjamin Herrenschmidt , David Gibson , anthony@codemonkey.ws, kvm@vger.kernel.org To: Alexander Graf Return-path: Received: from mail-pb0-f46.google.com ([209.85.160.46]:48707 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753318Ab2EKMrO (ORCPT ); Fri, 11 May 2012 08:47:14 -0400 Received: by pbbrp8 with SMTP id rp8so3273722pbb.19 for ; Fri, 11 May 2012 05:47:14 -0700 (PDT) In-Reply-To: <6A22E211-BC82-49BD-A335-02D3BAA14A17@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: 11.05.2012 20:52, Alexander Graf =CE=C1=D0=C9=D3=C1=CC: >=20 > On 11.05.2012, at 08:45, Alexey Kardashevskiy wrote: >=20 >> 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. >> >> 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) { >=20 > This doesn't scale. Capabilities are a list of CAPs. You'll have to d= o a loop through all capabilities, check if the one you want to add is = there already and if so either > * replace the existing one or > * drop out and not write the new one in. >=20 > I'm not sure which way would be more natural. There is a third option - add another function, lets call it pci_fixup_capability() which would do whatever pci_add_capability() doe= s but won't touch list pointers. When vfio, pci_add_capability() is called from the code which knows exactly that the capability exists and where it is and it calls pci_add_capability() based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no? >> + 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