From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SVFUh-00069h-9U for qemu-devel@nongnu.org; Fri, 18 May 2012 01:12:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SVFUf-0004kA-4r for qemu-devel@nongnu.org; Fri, 18 May 2012 01:12:50 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:37471) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SVFUe-0004je-RW for qemu-devel@nongnu.org; Fri, 18 May 2012 01:12:49 -0400 Received: by pbbro12 with SMTP id ro12so4347760pbb.4 for ; Thu, 17 May 2012 22:12:45 -0700 (PDT) Message-ID: <4FB5DA43.90907@ozlabs.ru> Date: Fri, 18 May 2012 15:12:35 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <4FACB581.2050609@ozlabs.ru> <6A22E211-BC82-49BD-A335-02D3BAA14A17@suse.de> <4FAD0A4F.2050506@ozlabs.ru> <4FB080CE.3030703@ozlabs.ru> In-Reply-To: <4FB080CE.3030703@ozlabs.ru> Content-Type: text/plain; charset=KOI8-R 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: Alexander Graf Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, Alex Williamson , anthony@codemonkey.ws, David Gibson Alexander, Is that any better? :) @@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev) * in pci config space */ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { - uint8_t *config; + uint8_t *config, existing; int i, overlapping_cap; + existing = pci_find_capability(pdev, cap_id); + if (existing) { + if (offset && (existing != offset)) { + return -EEXIST; + } + for (i = existing; i < size; ++i) { + if (pdev->used[i]) { + return -EFAULT; + } + } + memset(pdev->used + offset, 0xFF, size); + /* Make capability read-only by default */ + memset(pdev->wmask + offset, 0, size); + /* Check capability by default */ + memset(pdev->cmask + offset, 0xFF, size); + return existing; + } + if (!offset) { offset = pci_find_space(pdev, size); if (!offset) { return -ENOSPC; On 14/05/12 13:49, Alexey Kardashevskiy wrote: > On 12/05/12 00:13, Alexander Graf wrote: >> >> On 11.05.2012, at 14:47, Alexey Kardashevskiy wrote: >> >>> 11.05.2012 20:52, Alexander Graf ΞΑΠΙΣΑΜ: >>>> >>>> On 11.05.2012, at 08:45, 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. >>>>> >>>>> 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) { >>>> >>>> This doesn't scale. Capabilities are a list of CAPs. You'll have to do 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. >> >> * hw_error :) >> >>>> >>>> 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() does >>> but won't touch list pointers. >> >> What good is a function that breaks internal consistency? > > > It is broken already by having PCIDevice.used field. Normally pci_add_capability() would go through > the whole list and add a capability if it does not exist. Emulated devices which care about having a > capability at some fixed offset would have initialized their config space before calling this > capabilities API (as VFIO does). > > If we really want to support emulated devices which want some capabilities be at fixed offset and > others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency > by special function (pci_fixup_capability()) to avoid its rewriting at different location as a guest > driver may care about its offset. > > > >>> 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? >> >> Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability. >> >> >> Alex >> > > -- Alexey