From patchwork Wed Oct 12 05:02:54 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yi Liu X-Patchwork-Id: 9372077 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 32F2F60772 for ; Wed, 12 Oct 2016 05:03:36 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2A61529185 for ; Wed, 12 Oct 2016 05:03:36 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1F4F92923D; Wed, 12 Oct 2016 05:03:36 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 0E8D829185 for ; Wed, 12 Oct 2016 05:03:34 +0000 (UTC) Received: from localhost ([::1]:59289 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buBhV-00020b-57 for patchwork-qemu-devel@patchwork.kernel.org; Wed, 12 Oct 2016 01:03:33 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44575) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buBh0-0001x3-Oj for qemu-devel@nongnu.org; Wed, 12 Oct 2016 01:03:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buBgw-0007dX-MU for qemu-devel@nongnu.org; Wed, 12 Oct 2016 01:03:02 -0400 Received: from mga03.intel.com ([134.134.136.65]:25938) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buBgw-0007cy-9b for qemu-devel@nongnu.org; Wed, 12 Oct 2016 01:02:58 -0400 Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP; 11 Oct 2016 22:02:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,332,1473145200"; d="scan'208";a="18676686" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga006.fm.intel.com with ESMTP; 11 Oct 2016 22:02:57 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 11 Oct 2016 22:02:56 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 11 Oct 2016 22:02:56 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.206]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.104]) with mapi id 14.03.0248.002; Wed, 12 Oct 2016 13:02:54 +0800 From: "Liu, Yi L" To: "qemu-devel@nongnu.org" Thread-Topic: Potential Bug in vIOMMU which may result in memory wasting Thread-Index: AdIkRQQGaY3L6HmcR3KT8vCcBuiXOA== Date: Wed, 12 Oct 2016 05:02:54 +0000 Message-ID: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 134.134.136.65 Subject: [Qemu-devel] Potential Bug in vIOMMU which may result in memory wasting X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Ji, John" , "Wu, Feng" Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Hi, Resend it here since there is code style issue with debug patch in previous email. 1. Problem description: Recently, I find a strange thing with vIOMMU in QEMU. It looks like g_hash_table_lookup() is not 100% give same result when the key is the same. And this results in redundant memory allocation. I'm not sure if it is expected. Has anyone else encountered it? This potential issue is in vtd_find_add_as(), it uses the input PCIBus pointer to lookup hash table and get a vtd_bus back. If no hit, allocate one and inset it to hash_table. In my understanding, a pci bus only requires a single vtd_bus structure. But what I observed denied it. 2. Command to start guest: x86_64-softmmu/qemu-system-x86_64 -boot c -hda /home/sky/vms/vm-for-svm/svm-disk.img -m 5120 -enable-kvm -machine type=q35 -device intel-iommu -net nic -net tap,ifname=tap0, script=no,downscript=no -device vfio-pci,host=00:02.0,id=hostdev0,addr=0x6 3. Log: a) First enter of vtd_find_add_as(), no corresponding vtd_bus in s->vtd_as_by_busptr, so allocate one, this is quite reasonable. ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0x0 lookup result: no vtd_bus, allocate one vtd_bus: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 lookup hash_table again, vtd_bus: 0x56067457a620 b) Second enter of vtd_find_add_as(), again no corresponding vtd_bus in s->vtd_as_by_busptr, so allocate one, this is strange since devfn: 0x30 is actually also under pci bus 0. ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0x30 lookup result: no vtd_bus, allocate one vtd_bus: 0x56067598fef0 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067598fef0 lookup hash_table again, vtd_bus: 0x56067598fef0 c) Third enter of vtd_find_add_as(),no corresponding vtd_bus in s->vtd_as_by_busptr, so allocate one, this also strange since there should have a vtd_bus in hash table for pci bus 0. ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0x0 lookup result: no vtd_bus, allocate one vtd_bus: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067598fef0 lookup hash_table again, vtd_bus: 0x5606746aa400 d) reset seems to be correct, however, there is totally 3 vtd_bus structure for pci bus 0. this is a waste of memory in my understanding. ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0x8 lookup result: got vtd_bus vtd_bus: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067598fef0 lookup hash_table again, vtd_bus: 0x5606746aa400 ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0x10 lookup result: got vtd_bus vtd_bus: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067598fef0 lookup hash_table again, vtd_bus: 0x5606746aa400 ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0x30 lookup result: got vtd_bus vtd_bus: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067598fef0 lookup hash_table again, vtd_bus: 0x5606746aa400 ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0xf8 lookup result: got vtd_bus vtd_bus: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067598fef0 lookup hash_table again, vtd_bus: 0x5606746aa400 ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0xfa lookup result: got vtd_bus vtd_bus: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067598fef0 lookup hash_table again, vtd_bus: 0x5606746aa400 ------------------------------ YiLiu - vtd_find_add_as() bus: 0x5606747f99c0 s: 0x560675a2e000 s->vtd_as_by_busptr: 0x560675c521e0, devfn: 0xfb lookup result: got vtd_bus vtd_bus: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x5606746aa400 vtd_bus in s->vtd_as_by_busptr: 0x56067457a620 vtd_bus in s->vtd_as_by_busptr: 0x56067598fef0 lookup hash_table again, vtd_bus: 0x5606746aa400 4. QEMU version: commit 48f592118ab42f83a1a7561c4bfd2b72a100f241 Author: Ed Maste Date: Tue Oct 4 16:02:49 2016 -0400 bsd-user: fix FreeBSD build after d148d90e Signed-off-by: Ed Maste Message-id: 1475611369-74971-1-git-send-email-emaste@freebsd.org Signed-off-by: Peter Maydell 5. Debug code: From d45c1ae01c0b6b953c95d9af1b6ffce64a6e2382 Mon Sep 17 00:00:00 2001 From: Yi Liu Date: Wed, 12 Oct 2016 12:26:45 +0800 Subject: [PATCH 1/2] enable vIOMMU work together with VFIO Signed-off-by: Yi Liu --- hw/i386/intel_iommu.c | 2 ++ hw/pci/pcie.c | 2 +- hw/vfio/common.c | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) -- 1.9.1 Best Wishes, Yi Liu diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 9f4e64a..d76985e 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1978,6 +1978,7 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, IOMMUNotifierFlag old, IOMMUNotifierFlag new) { +/* VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); if (new & IOMMU_NOTIFIER_MAP) { @@ -1988,6 +1989,7 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, PCI_FUNC(vtd_as->devfn)); exit(1); } +*/ } static const VMStateDescription vtd_vmstate = { diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 99cfb45..0355ffd 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -683,7 +683,7 @@ void pcie_add_capability(PCIDevice *dev, in the linked list */ next = pcie_find_capability_list(dev, 0, &prev); - assert(prev >= PCI_CONFIG_SPACE_SIZE); + /*assert(prev >= PCI_CONFIG_SPACE_SIZE);*/ assert(next == 0); pcie_ext_cap_set_next(dev, prev, offset); } diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 29188a1..242a5b1 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -457,7 +457,7 @@ static void vfio_listener_region_add(MemoryListener *listener, QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); - memory_region_iommu_replay(giommu->iommu, &giommu->n, false); + /*memory_region_iommu_replay(giommu->iommu, &giommu->n, false);*/ return; } -- 1.9.1 From c193868dcb1179637d0aa646500a17daf91e8429 Mon Sep 17 00:00:00 2001 From: Yi Liu Date: Wed, 12 Oct 2016 12:29:18 +0800 Subject: [PATCH 2/2] debug code for potential issue in vtd_find_add_as() Signed-off-by: Yi Liu --- hw/i386/intel_iommu.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index d76985e..04b62b2 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -2312,14 +2312,31 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key); VTDAddressSpace *vtd_dev_as; + /* Yi: add for debug */ + printf("------------------------------\n" + "YiLiu - %s()\n" + " bus: 0x%llx\n" + " s: 0x%llx\n" + " s->vtd_as_by_busptr: 0x%llx," + " devfn: 0x%x\n", + __func__, + (unsigned long long int) bus, + (unsigned long long int) s, + (unsigned long long int) s->vtd_as_by_busptr, + (unsigned int) devfn); + if (!vtd_bus) { + printf(" lookup result: no vtd_bus, allocate one\n"); /* No corresponding free() */ vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * \ X86_IOMMU_PCI_DEVFN_MAX); vtd_bus->bus = bus; key = (uintptr_t)bus; g_hash_table_insert(s->vtd_as_by_busptr, &key, vtd_bus); + } else { + printf(" lookup result: got vtd_bus\n"); } + printf(" vtd_bus: 0x%llx\n", (unsigned long long int) vtd_bus); vtd_dev_as = vtd_bus->dev_as[devfn]; @@ -2340,6 +2357,23 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) address_space_init(&vtd_dev_as->as, &vtd_dev_as->iommu, "intel_iommu"); } + + {/* Yi: code snippet for debug, dump the vtd_as_by_busptr + to see if all the allocated vtd_bus are still there */ + GHashTableIter iter; + g_hash_table_iter_init(&iter, s->vtd_as_by_busptr); + while (g_hash_table_iter_next(&iter, NULL, (void **)&vtd_bus)) { + if (pci_bus_num(vtd_bus->bus) == pci_bus_num(bus)) { + printf(" vtd_bus in s->vtd_as_by_busptr: 0x%llx\n", + (unsigned long long int) vtd_bus); + } + } + /* look up hash table again */ + vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key); + printf(" lookup hash_table again, vtd_bus: 0x%llx\n", + (unsigned long long int) vtd_bus); + } + return vtd_dev_as; }