[v2] intel_iommu: Take the bql before registering a new address space

[PATCH v2] intel_iommu: Take the bql before registering a new address space

CLEMENT MATHIEU--DRIF posted 1 patch 9 months, 3 weeks ago

Diff against v1
Download mbox

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250415153207.372427-1-clement.mathieu--drif@eviden.com

Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Yi Liu <yi.l.liu@intel.com>, "Clément Mathieu--Drif" <clement.mathieu--drif@eviden.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>

hw/i386/intel_iommu.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

Expand all Fold all

[PATCH v2] intel_iommu: Take the bql before registering a new address space

Posted by CLEMENT MATHIEU--DRIF 9 months, 3 weeks ago

Address space creation might end up being called without holding the
bql as it is exposed through the IOMMU ops.

Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
---
 hw/i386/intel_iommu.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index dffd7ee885..3df7a7489b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4216,6 +4216,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
     vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
     if (!vtd_dev_as) {
         struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));
+        bool take_bql = !bql_locked();
 
         new_key->bus = bus;
         new_key->devfn = devfn;
@@ -4238,6 +4239,14 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
         vtd_dev_as->context_cache_entry.context_cache_gen = 0;
         vtd_dev_as->iova_tree = iova_tree_new();
 
+        /*
+         * memory_region_add_subregion_overlap requires the bql,
+         * make sure we own it.
+         */
+        if (take_bql) {
+            bql_lock();
+        }
+
         memory_region_init(&vtd_dev_as->root, OBJECT(s), name, UINT64_MAX);
         address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, "vtd-root");
 
@@ -4305,6 +4314,10 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
 
         vtd_switch_address_space(vtd_dev_as);
 
+        if (take_bql) {
+            bql_unlock();
+        }
+
         g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
     }
     return vtd_dev_as;
-- 
2.49.0

Re: [PATCH v2] intel_iommu: Take the bql before registering a new address space

Posted by Alex Bennée 9 months, 3 weeks ago

CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> writes:

> Address space creation might end up being called without holding the
> bql as it is exposed through the IOMMU ops.
>
> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
> ---
>  hw/i386/intel_iommu.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dffd7ee885..3df7a7489b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4216,6 +4216,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>      vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
>      if (!vtd_dev_as) {
>          struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));
> +        bool take_bql = !bql_locked();
>  
>          new_key->bus = bus;
>          new_key->devfn = devfn;
> @@ -4238,6 +4239,14 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>          vtd_dev_as->context_cache_entry.context_cache_gen = 0;
>          vtd_dev_as->iova_tree = iova_tree_new();
>  
> +        /*
> +         * memory_region_add_subregion_overlap requires the bql,
> +         * make sure we own it.
> +         */
> +        if (take_bql) {
> +            bql_lock();
> +        }
> +

Using BQL_LOCK_GUARD(); here will auto clean-up as we exit and it
handles the nested BQL state as well.

>          memory_region_init(&vtd_dev_as->root, OBJECT(s), name, UINT64_MAX);
>          address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, "vtd-root");
>  
> @@ -4305,6 +4314,10 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>  
>          vtd_switch_address_space(vtd_dev_as);
>  
> +        if (take_bql) {
> +            bql_unlock();
> +        }
> +
>          g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
>      }
>      return vtd_dev_as;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro