[{"id":1915077,"web_url":"http://patchwork.ozlabs.org/comment/1915077/","msgid":"<17cd0a63-47db-ca21-be34-6c131c839690@redhat.com>","list_archive_url":null,"date":"2018-05-17T14:32:52","subject":"Re: [Qemu-devel] [PATCH v2 03/10] intel-iommu: add iommu lock","submitter":{"id":69187,"url":"http://patchwork.ozlabs.org/api/people/69187/","name":"Eric Auger","email":"eric.auger@redhat.com"},"content":"Hi Peter,\n\nOn 05/04/2018 05:08 AM, Peter Xu wrote:\n> Add a per-iommu big lock to protect IOMMU status. Currently the only\n> thing to be protected is the IOTLB/context cache, since that can be\n> accessed even without BQL, e.g., in IO dataplane.\n\nAs discussed together, Peter challenged per device mutex in\n\"Re: [PATCH v11 15/17] hw/arm/smmuv3: Cache/invalidate config data\"\nhttps://lists.gnu.org/archive/html/qemu-devel/2018-04/msg02403.html\nbut at that time I fail to justify the use :-(\n\n> \n> Note that we don't need to protect device page tables since that's fully\n> controlled by the guest kernel. However there is still possibility that\n> malicious drivers will program the device to not obey the rule. In that\n> case QEMU can't really do anything useful, instead the guest itself will\n> be responsible for all uncertainties.\n> \n> Reported-by: Fam Zheng \n> Signed-off-by: Peter Xu \n> ---\n> include/hw/i386/intel_iommu.h | 6 +++++\n> hw/i386/intel_iommu.c | 43 +++++++++++++++++++++++++++++++----\n> 2 files changed, 44 insertions(+), 5 deletions(-)\n> \n> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h\n> index 220697253f..ee517704e7 100644\n> --- a/include/hw/i386/intel_iommu.h\n> +++ b/include/hw/i386/intel_iommu.h\n> @@ -300,6 +300,12 @@ struct IntelIOMMUState {\n> OnOffAuto intr_eim; /* Toggle for EIM cabability */\n> bool buggy_eim; /* Force buggy EIM unless eim=off */\n> uint8_t aw_bits; /* Host/IOVA address width (in bits) */\n> +\n> + /*\n> + * Protects IOMMU states in general. Currently it protects the\n> + * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.\n> + */\n> + QemuMutex iommu_lock;\n> };\n> \n> /* Find the VTD Address space associated with the given bus pointer,\n> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c\n> index 5987b48d43..112971638d 100644\n> --- a/hw/i386/intel_iommu.c\n> +++ b/hw/i386/intel_iommu.c\n> @@ -128,6 +128,16 @@ static uint64_t vtd_set_clear_mask_quad(IntelIOMMUState *s, hwaddr addr,\n> return new_val;\n> }\n> \n> +static inline void vtd_iommu_lock(IntelIOMMUState *s)\n> +{\n> + qemu_mutex_lock(&s->iommu_lock);\n> +}\n> +\n> +static inline void vtd_iommu_unlock(IntelIOMMUState *s)\n> +{\n> + qemu_mutex_unlock(&s->iommu_lock);\n> +}\n> +\n> /* GHashTable functions */\n> static gboolean vtd_uint64_equal(gconstpointer v1, gconstpointer v2)\n> {\n> @@ -172,7 +182,7 @@ static gboolean vtd_hash_remove_by_page(gpointer key, gpointer value,\n> }\n> \n> /* Reset all the gen of VTDAddressSpace to zero and set the gen of\n> - * IntelIOMMUState to 1.\n> + * IntelIOMMUState to 1. Must be with IOMMU lock held.\ns/Must be/ Must be called ?\nnot done in vtd_init()\n> */\n> static void vtd_reset_context_cache(IntelIOMMUState *s)\n> {\n> @@ -197,12 +207,19 @@ static void vtd_reset_context_cache(IntelIOMMUState *s)\n> s->context_cache_gen = 1;\n> }\n> \n> -static void vtd_reset_iotlb(IntelIOMMUState *s)\n> +static void vtd_reset_iotlb_locked(IntelIOMMUState *s)\nadd the above comment and keep the original name?\n> {\n> assert(s->iotlb);\n> g_hash_table_remove_all(s->iotlb);\n> }\n> \n> +static void vtd_reset_iotlb(IntelIOMMUState *s)\n> +{\n> + vtd_iommu_lock(s);\n> + vtd_reset_iotlb_locked(s);\n> + vtd_iommu_unlock(s);\n> +}\n> +\n> static uint64_t vtd_get_iotlb_key(uint64_t gfn, uint16_t source_id,\n> uint32_t level)\n> {\n> @@ -215,6 +232,7 @@ static uint64_t vtd_get_iotlb_gfn(hwaddr addr, uint32_t level)\n> return (addr & vtd_slpt_level_page_mask(level)) >> VTD_PAGE_SHIFT_4K;\n> }\n> \n> +/* Must be with IOMMU lock held */\n> static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t source_id,\n> hwaddr addr)\n> {\n> @@ -235,6 +253,7 @@ out:\n> return entry;\n> }\n> \n> +/* Must be with IOMMU lock held */\n> static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,\n> uint16_t domain_id, hwaddr addr, uint64_t slpte,\n> uint8_t access_flags, uint32_t level)\n> @@ -246,7 +265,7 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,\n> trace_vtd_iotlb_page_update(source_id, addr, slpte, domain_id);\n> if (g_hash_table_size(s->iotlb) >= VTD_IOTLB_MAX_SIZE) {\n> trace_vtd_iotlb_reset(\"iotlb exceeds size limit\");\n> - vtd_reset_iotlb(s);\n> + vtd_reset_iotlb_locked(s);\n> }\n> \n> entry->gfn = gfn;\n> @@ -1106,7 +1125,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,\n> IntelIOMMUState *s = vtd_as->iommu_state;\n> VTDContextEntry ce;\n> uint8_t bus_num = pci_bus_num(bus);\n> - VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;\n> + VTDContextCacheEntry *cc_entry;\n> uint64_t slpte, page_mask;\n> uint32_t level;\n> uint16_t source_id = vtd_make_source_id(bus_num, devfn);\n> @@ -1123,6 +1142,10 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,\n> */\n> assert(!vtd_is_interrupt_addr(addr));\n> \n> + vtd_iommu_lock(s);\n> +\n> + cc_entry = &vtd_as->context_cache_entry;\n> +\n> /* Try to fetch slpte form IOTLB */\n> iotlb_entry = vtd_lookup_iotlb(s, source_id, addr);\n> if (iotlb_entry) {\n> @@ -1182,7 +1205,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,\n> * IOMMU region can be swapped back.\n> */\n> vtd_pt_enable_fast_path(s, source_id);\n> -\n> + vtd_iommu_unlock(s);\n> return true;\n> }\n> \n> @@ -1203,6 +1226,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,\n> vtd_update_iotlb(s, source_id, VTD_CONTEXT_ENTRY_DID(ce.hi), addr, slpte,\n> access_flags, level);\n> out:\n> + vtd_iommu_unlock(s);\n> entry->iova = addr & page_mask;\n> entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask;\n> entry->addr_mask = ~page_mask;\n> @@ -1210,6 +1234,7 @@ out:\n> return true;\n> \n> error:\n> + vtd_iommu_unlock(s);\n> entry->iova = 0;\n> entry->translated_addr = 0;\n> entry->addr_mask = 0;\n> @@ -1258,10 +1283,13 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s)\n> static void vtd_context_global_invalidate(IntelIOMMUState *s)\n> {\n> trace_vtd_inv_desc_cc_global();\n> + /* Protects context cache */\n> + vtd_iommu_lock(s);\n> s->context_cache_gen++;\n> if (s->context_cache_gen == VTD_CONTEXT_CACHE_GEN_MAX) {\n> vtd_reset_context_cache(s);\n> }\n> + vtd_iommu_unlock(s);\n> vtd_switch_address_space_all(s);\n> /*\n> * From VT-d spec 6.5.2.1, a global context entry invalidation\n> @@ -1377,8 +1405,10 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)\n> \n> trace_vtd_inv_desc_iotlb_domain(domain_id);\n> \n> + vtd_iommu_lock(s);\n> g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_domain,\n> &domain_id);\n> + vtd_iommu_unlock(s);\n> \n> QLIST_FOREACH(vtd_as, &s->notifiers_list, next) {\n> if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),\n> @@ -1426,7 +1456,9 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,\n> info.domain_id = domain_id;\n> info.addr = addr;\n> info.mask = ~((1 << am) - 1);\n> + vtd_iommu_lock(s);\n> g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);\n> + vtd_iommu_unlock(s);\n> vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);\n> }\n> \n> @@ -3072,6 +3104,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)\n> }\n> \n> QLIST_INIT(&s->notifiers_list);\n> + qemu_mutex_init(&s->iommu_lock);\n> memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));\n> memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,\n> \"intel_iommu\", DMAR_REG_SIZE);\n> \nDon't you need to take the lock in vtd_context_device_invalidate() which\nmanipulates the vtd_as->context_cache_entry.context_cache_gen?\n\nThanks\n\nEric","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdmarc=fail (p=none dis=none) header.from=redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 40mv1l2Kd5z9s47\n\tfor ;\n\tFri, 18 May 2018 00:33:46 +1000 (AEST)","from localhost ([::1]:58593 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1fJJyS-0000Gb-B7\n\tfor incoming@patchwork.ozlabs.org; Thu, 17 May 2018 10:33:44 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:33086)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1fJJxl-0000Es-RL\n\tfor qemu-devel@nongnu.org; Thu, 17 May 2018 10:33:05 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1fJJxf-0000A5-L2\n\tfor qemu-devel@nongnu.org; Thu, 17 May 2018 10:33:01 -0400","from mx3-rdu2.redhat.com ([66.187.233.73]:38424\n\thelo=mx1.redhat.com)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1fJJxf-00009k-FU\n\tfor qemu-devel@nongnu.org; Thu, 17 May 2018 10:32:55 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 002FA81663C0;\n\tThu, 17 May 2018 14:32:55 +0000 (UTC)","from localhost.localdomain (ovpn-117-111.ams2.redhat.com\n\t[10.36.117.111])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 9CE092166BAD;\n\tThu, 17 May 2018 14:32:53 +0000 (UTC)"],"To":"Peter Xu , qemu-devel@nongnu.org","References":"<20180504030811.28111-1-peterx@redhat.com>\n\t<20180504030811.28111-4-peterx@redhat.com>","From":"Auger Eric ","Message-ID":"<17cd0a63-47db-ca21-be34-6c131c839690@redhat.com>","Date":"Thu, 17 May 2018 16:32:52 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tThunderbird/45.4.0","MIME-Version":"1.0","In-Reply-To":"<20180504030811.28111-4-peterx@redhat.com>","Content-Type":"text/plain; charset=windows-1252","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.78 on 10.11.54.6","X-Greylist":["Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.11.55.8]);\n\tThu, 17 May 2018 14:32:55 +0000 (UTC)","inspected by milter-greylist-4.5.16 (mx1.redhat.com\n\t[10.11.55.8]); \n\tThu, 17 May 2018 14:32:55 +0000 (UTC) for IP:'10.11.54.6'\n\tDOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com'\n\tHELO:'smtp.corp.redhat.com' FROM:'eric.auger@redhat.com' RCPT:''"],"X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"66.187.233.73","Subject":"Re: [Qemu-devel] [PATCH v2 03/10] intel-iommu: add iommu lock","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Jintack Lim , Tian Kevin ,\n\tAlex Williamson ,\n\tJason Wang , \"Michael S . Tsirkin\" ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1915630,"web_url":"http://patchwork.ozlabs.org/comment/1915630/","msgid":"<20180518053218.GC2569@xz-mi>","list_archive_url":null,"date":"2018-05-18T05:32:18","subject":"Re: [Qemu-devel] [PATCH v2 03/10] intel-iommu: add iommu lock","submitter":{"id":67717,"url":"http://patchwork.ozlabs.org/api/people/67717/","name":"Peter Xu","email":"peterx@redhat.com"},"content":"On Thu, May 17, 2018 at 04:32:52PM +0200, Auger Eric wrote:\n> Hi Peter,\n> \n> On 05/04/2018 05:08 AM, Peter Xu wrote:\n> > Add a per-iommu big lock to protect IOMMU status. Currently the only\n> > thing to be protected is the IOTLB/context cache, since that can be\n> > accessed even without BQL, e.g., in IO dataplane.\n> \n> As discussed together, Peter challenged per device mutex in\n> \"Re: [PATCH v11 15/17] hw/arm/smmuv3: Cache/invalidate config data\"\n> https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg02403.html\n> but at that time I fail to justify the use :-(\n> \n> > \n> > Note that we don't need to protect device page tables since that's fully\n> > controlled by the guest kernel. However there is still possibility that\n> > malicious drivers will program the device to not obey the rule. In that\n> > case QEMU can't really do anything useful, instead the guest itself will\n> > be responsible for all uncertainties.\n> > \n> > Reported-by: Fam Zheng \n> > Signed-off-by: Peter Xu \n> > ---\n> > include/hw/i386/intel_iommu.h | 6 +++++\n> > hw/i386/intel_iommu.c | 43 +++++++++++++++++++++++++++++++----\n> > 2 files changed, 44 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h\n> > index 220697253f..ee517704e7 100644\n> > --- a/include/hw/i386/intel_iommu.h\n> > +++ b/include/hw/i386/intel_iommu.h\n> > @@ -300,6 +300,12 @@ struct IntelIOMMUState {\n> > OnOffAuto intr_eim; /* Toggle for EIM cabability */\n> > bool buggy_eim; /* Force buggy EIM unless eim=off */\n> > uint8_t aw_bits; /* Host/IOVA address width (in bits) */\n> > +\n> > + /*\n> > + * Protects IOMMU states in general. Currently it protects the\n> > + * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.\n> > + */\n> > + QemuMutex iommu_lock;\n> > };\n> > \n> > /* Find the VTD Address space associated with the given bus pointer,\n> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c\n> > index 5987b48d43..112971638d 100644\n> > --- a/hw/i386/intel_iommu.c\n> > +++ b/hw/i386/intel_iommu.c\n> > @@ -128,6 +128,16 @@ static uint64_t vtd_set_clear_mask_quad(IntelIOMMUState *s, hwaddr addr,\n> > return new_val;\n> > }\n> > \n> > +static inline void vtd_iommu_lock(IntelIOMMUState *s)\n> > +{\n> > + qemu_mutex_lock(&s->iommu_lock);\n> > +}\n> > +\n> > +static inline void vtd_iommu_unlock(IntelIOMMUState *s)\n> > +{\n> > + qemu_mutex_unlock(&s->iommu_lock);\n> > +}\n> > +\n> > /* GHashTable functions */\n> > static gboolean vtd_uint64_equal(gconstpointer v1, gconstpointer v2)\n> > {\n> > @@ -172,7 +182,7 @@ static gboolean vtd_hash_remove_by_page(gpointer key, gpointer value,\n> > }\n> > \n> > /* Reset all the gen of VTDAddressSpace to zero and set the gen of\n> > - * IntelIOMMUState to 1.\n> > + * IntelIOMMUState to 1. Must be with IOMMU lock held.\n> s/Must be/ Must be called ?\n\nOk.\n\n> not done in vtd_init()\n\nIMHO we can omit that since it's only used during either realization\nor system reset. But sure I can add it too.\n\n> > */\n> > static void vtd_reset_context_cache(IntelIOMMUState *s)\n> > {\n> > @@ -197,12 +207,19 @@ static void vtd_reset_context_cache(IntelIOMMUState *s)\n> > s->context_cache_gen = 1;\n> > }\n> > \n> > -static void vtd_reset_iotlb(IntelIOMMUState *s)\n> > +static void vtd_reset_iotlb_locked(IntelIOMMUState *s)\n> add the above comment and keep the original name?\n\nI can add the comment; the original name is defined as another\nfunction below.\n\n> > {\n> > assert(s->iotlb);\n> > g_hash_table_remove_all(s->iotlb);\n> > }\n> > \n> > +static void vtd_reset_iotlb(IntelIOMMUState *s)\n\n[1]\n\n> > +{\n> > + vtd_iommu_lock(s);\n> > + vtd_reset_iotlb_locked(s);\n> > + vtd_iommu_unlock(s);\n> > +}\n> > +\n> > static uint64_t vtd_get_iotlb_key(uint64_t gfn, uint16_t source_id,\n> > uint32_t level)\n> > {\n> > @@ -215,6 +232,7 @@ static uint64_t vtd_get_iotlb_gfn(hwaddr addr, uint32_t level)\n> > return (addr & vtd_slpt_level_page_mask(level)) >> VTD_PAGE_SHIFT_4K;\n> > }\n> > \n> > +/* Must be with IOMMU lock held */\n> > static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t source_id,\n> > hwaddr addr)\n> > {\n> > @@ -235,6 +253,7 @@ out:\n> > return entry;\n> > }\n> > \n> > +/* Must be with IOMMU lock held */\n> > static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,\n> > uint16_t domain_id, hwaddr addr, uint64_t slpte,\n> > uint8_t access_flags, uint32_t level)\n> > @@ -246,7 +265,7 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,\n> > trace_vtd_iotlb_page_update(source_id, addr, slpte, domain_id);\n> > if (g_hash_table_size(s->iotlb) >= VTD_IOTLB_MAX_SIZE) {\n> > trace_vtd_iotlb_reset(\"iotlb exceeds size limit\");\n> > - vtd_reset_iotlb(s);\n> > + vtd_reset_iotlb_locked(s);\n> > }\n> > \n> > entry->gfn = gfn;\n> > @@ -1106,7 +1125,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,\n> > IntelIOMMUState *s = vtd_as->iommu_state;\n> > VTDContextEntry ce;\n> > uint8_t bus_num = pci_bus_num(bus);\n> > - VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;\n> > + VTDContextCacheEntry *cc_entry;\n> > uint64_t slpte, page_mask;\n> > uint32_t level;\n> > uint16_t source_id = vtd_make_source_id(bus_num, devfn);\n> > @@ -1123,6 +1142,10 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,\n> > */\n> > assert(!vtd_is_interrupt_addr(addr));\n> > \n> > + vtd_iommu_lock(s);\n> > +\n> > + cc_entry = &vtd_as->context_cache_entry;\n> > +\n> > /* Try to fetch slpte form IOTLB */\n> > iotlb_entry = vtd_lookup_iotlb(s, source_id, addr);\n> > if (iotlb_entry) {\n> > @@ -1182,7 +1205,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,\n> > * IOMMU region can be swapped back.\n> > */\n> > vtd_pt_enable_fast_path(s, source_id);\n> > -\n> > + vtd_iommu_unlock(s);\n> > return true;\n> > }\n> > \n> > @@ -1203,6 +1226,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,\n> > vtd_update_iotlb(s, source_id, VTD_CONTEXT_ENTRY_DID(ce.hi), addr, slpte,\n> > access_flags, level);\n> > out:\n> > + vtd_iommu_unlock(s);\n> > entry->iova = addr & page_mask;\n> > entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask;\n> > entry->addr_mask = ~page_mask;\n> > @@ -1210,6 +1234,7 @@ out:\n> > return true;\n> > \n> > error:\n> > + vtd_iommu_unlock(s);\n> > entry->iova = 0;\n> > entry->translated_addr = 0;\n> > entry->addr_mask = 0;\n> > @@ -1258,10 +1283,13 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s)\n> > static void vtd_context_global_invalidate(IntelIOMMUState *s)\n> > {\n> > trace_vtd_inv_desc_cc_global();\n> > + /* Protects context cache */\n> > + vtd_iommu_lock(s);\n> > s->context_cache_gen++;\n> > if (s->context_cache_gen == VTD_CONTEXT_CACHE_GEN_MAX) {\n> > vtd_reset_context_cache(s);\n> > }\n> > + vtd_iommu_unlock(s);\n> > vtd_switch_address_space_all(s);\n> > /*\n> > * From VT-d spec 6.5.2.1, a global context entry invalidation\n> > @@ -1377,8 +1405,10 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)\n> > \n> > trace_vtd_inv_desc_iotlb_domain(domain_id);\n> > \n> > + vtd_iommu_lock(s);\n> > g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_domain,\n> > &domain_id);\n> > + vtd_iommu_unlock(s);\n> > \n> > QLIST_FOREACH(vtd_as, &s->notifiers_list, next) {\n> > if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),\n> > @@ -1426,7 +1456,9 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,\n> > info.domain_id = domain_id;\n> > info.addr = addr;\n> > info.mask = ~((1 << am) - 1);\n> > + vtd_iommu_lock(s);\n> > g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);\n> > + vtd_iommu_unlock(s);\n> > vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);\n> > }\n> > \n> > @@ -3072,6 +3104,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)\n> > }\n> > \n> > QLIST_INIT(&s->notifiers_list);\n> > + qemu_mutex_init(&s->iommu_lock);\n> > memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));\n> > memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,\n> > \"intel_iommu\", DMAR_REG_SIZE);\n> > \n> Don't you need to take the lock in vtd_context_device_invalidate() which\n> manipulates the vtd_as->context_cache_entry.context_cache_gen?\n\nYes I think we need that. Thanks,","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdmarc=fail (p=none dis=none) header.from=redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 40nGzF5DK1z9s2k\n\tfor ;\n\tFri, 18 May 2018 15:32:56 +1000 (AEST)","from localhost ([::1]:36734 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1fJY0a-00021t-8e\n\tfor incoming@patchwork.ozlabs.org; Fri, 18 May 2018 01:32:52 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:55665)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1fJY0J-00021Y-8O\n\tfor qemu-devel@nongnu.org; Fri, 18 May 2018 01:32:37 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1fJY0G-0005rH-1D\n\tfor qemu-devel@nongnu.org; Fri, 18 May 2018 01:32:35 -0400","from mx3-rdu2.redhat.com ([66.187.233.73]:57120\n\thelo=mx1.redhat.com)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1fJY0F-0005r3-Rb\n\tfor qemu-devel@nongnu.org; Fri, 18 May 2018 01:32:31 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 331254023112;\n\tFri, 18 May 2018 05:32:31 +0000 (UTC)","from xz-mi (dhcp-14-151.nay.redhat.com [10.66.14.151])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 9516911166F4;\n\tFri, 18 May 2018 05:32:21 +0000 (UTC)"],"Date":"Fri, 18 May 2018 13:32:18 +0800","From":"Peter Xu ","To":"Auger Eric ","Message-ID":"<20180518053218.GC2569@xz-mi>","References":"<20180504030811.28111-1-peterx@redhat.com>\n\t<20180504030811.28111-4-peterx@redhat.com>\n\t<17cd0a63-47db-ca21-be34-6c131c839690@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<17cd0a63-47db-ca21-be34-6c131c839690@redhat.com>","User-Agent":"Mutt/1.9.5 (2018-04-13)","X-Scanned-By":"MIMEDefang 2.78 on 10.11.54.3","X-Greylist":["Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.11.55.6]);\n\tFri, 18 May 2018 05:32:31 +0000 (UTC)","inspected by milter-greylist-4.5.16 (mx1.redhat.com\n\t[10.11.55.6]); \n\tFri, 18 May 2018 05:32:31 +0000 (UTC) for IP:'10.11.54.3'\n\tDOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com'\n\tHELO:'smtp.corp.redhat.com' FROM:'peterx@redhat.com' RCPT:''"],"X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"66.187.233.73","Subject":"Re: [Qemu-devel] [PATCH v2 03/10] intel-iommu: add iommu lock","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tian Kevin , \"Michael S . Tsirkin\" ,\n\tJason Wang , qemu-devel@nongnu.org,\n\tAlex Williamson ,\n\tJintack Lim ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]