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

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

[CLEMENT MATHIEU--DRIF]

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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index dffd7ee885..fea2220013 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,11 @@ 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();
 
+        /* Some functions in this branch require 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 +4311,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] intel_iommu: Take the bql before registering a new address space

[Philippe Mathieu-Daudé]

On 15/4/25 08:18, CLEMENT MATHIEU--DRIF wrote:
> 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 | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dffd7ee885..fea2220013 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,11 @@ 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();
>   
> +        /* Some functions in this branch require 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 +4311,10 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>   
>           vtd_switch_address_space(vtd_dev_as);

Would it help clarifying to propagate this argument down?
vtd_switch_address_space(VTDAddressSpace *as, bool need_lock);

>   
> +        if (take_bql) {
> +            bql_unlock();
> +        }
> +
>           g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
>       }
>       return vtd_dev_as;

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

[Michael S. Tsirkin]

On Tue, Apr 15, 2025 at 06:18:08AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 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>


Indeed, and this can maybe explain some failures we are seeing ...
But is it just this part?
For example, is access to vtd_address_spaces safe?

> ---
>  hw/i386/intel_iommu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dffd7ee885..fea2220013 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,11 @@ 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();
>  
> +        /* Some functions in this branch require 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 +4311,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] intel_iommu: Take the bql before registering a new address space

[Michael S. Tsirkin]

On Tue, Apr 15, 2025 at 06:18:08AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 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>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Stefan, want to pick this one up, too?


> ---
>  hw/i386/intel_iommu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dffd7ee885..fea2220013 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,11 @@ 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();
>  
> +        /* Some functions in this branch require 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 +4311,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] intel_iommu: Take the bql before registering a new address space

[CLEMENT MATHIEU--DRIF]

On 15/04/2025 8:53 am, Philippe Mathieu-Daudé wrote:
> Caution: External email. Do not open attachments or click links, unless 
> this email comes from a known sender and you know the content is safe.
> 
> 
> On 15/4/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>> 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 | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index dffd7ee885..fea2220013 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,11 @@ 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();
>>
>> +        /* Some functions in this branch require 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 +4311,10 @@ VTDAddressSpace 
>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>
>>           vtd_switch_address_space(vtd_dev_as);
> 
> Would it help clarifying to propagate this argument down?
> vtd_switch_address_space(VTDAddressSpace *as, bool need_lock);

Hi phil, vtd_switch_address_space already does the same kind of check

> 
>>
>> +        if (take_bql) {
>> +            bql_unlock();
>> +        }
>> +
>>           g_hash_table_insert(s->vtd_address_spaces, new_key, 
>> vtd_dev_as);
>>       }
>>       return vtd_dev_as;
> 

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

[Michael S. Tsirkin]

On Tue, Apr 15, 2025 at 07:28:34AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 
> 
> On 15/04/2025 8:53 am, Philippe Mathieu-Daudé wrote:
> > Caution: External email. Do not open attachments or click links, unless 
> > this email comes from a known sender and you know the content is safe.
> > 
> > 
> > On 15/4/25 08:18, CLEMENT MATHIEU--DRIF wrote:
> >> 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 | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index dffd7ee885..fea2220013 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,11 @@ 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();
> >>
> >> +        /* Some functions in this branch require 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 +4311,10 @@ VTDAddressSpace 
> >> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
> >>
> >>           vtd_switch_address_space(vtd_dev_as);
> > 
> > Would it help clarifying to propagate this argument down?
> > vtd_switch_address_space(VTDAddressSpace *as, bool need_lock);
> 
> Hi phil, vtd_switch_address_space already does the same kind of check
> 
> > 
> >>
> >> +        if (take_bql) {
> >> +            bql_unlock();
> >> +        }
> >> +
> >>           g_hash_table_insert(s->vtd_address_spaces, new_key, 
> >> vtd_dev_as);
> >>       }
> >>       return vtd_dev_as;
> > 


As an apropos, I think any caller of bql_lock really should call
bql_lock_impl so we know who took BQL. Or just use BQL_LOCK_GUARD.
But, that's an unrelated cleanup.

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

[Philippe Mathieu-Daudé]

On 15/4/25 09:42, Michael S. Tsirkin wrote:
> On Tue, Apr 15, 2025 at 07:28:34AM +0000, CLEMENT MATHIEU--DRIF wrote:
>>
>>
>> On 15/04/2025 8:53 am, Philippe Mathieu-Daudé wrote:
>>> Caution: External email. Do not open attachments or click links, unless
>>> this email comes from a known sender and you know the content is safe.
>>>
>>>
>>> On 15/4/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>>>> 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 | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index dffd7ee885..fea2220013 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,11 @@ 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();
>>>>
>>>> +        /* Some functions in this branch require 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 +4311,10 @@ VTDAddressSpace
>>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>>>
>>>>            vtd_switch_address_space(vtd_dev_as);
>>>
>>> Would it help clarifying to propagate this argument down?
>>> vtd_switch_address_space(VTDAddressSpace *as, bool need_lock);
>>
>> Hi phil, vtd_switch_address_space already does the same kind of check
>>
>>>
>>>>
>>>> +        if (take_bql) {
>>>> +            bql_unlock();
>>>> +        }
>>>> +
>>>>            g_hash_table_insert(s->vtd_address_spaces, new_key,
>>>> vtd_dev_as);
>>>>        }
>>>>        return vtd_dev_as;
>>>
> 
> 
> As an apropos, I think any caller of bql_lock really should call
> bql_lock_impl so we know who took BQL. Or just use BQL_LOCK_GUARD.
> But, that's an unrelated cleanup.
> 

Yeah unrelated cleanup. Although I don't understand why these
code paths don't use memory_region_transaction_begin/commit and
have to access BQL.

Some untested idea to investigate:
-- >8 --
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index fea22200135..b2a809cb3db 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1726,3 +1726,6 @@ static bool vtd_as_pt_enabled(VTDAddressSpace *as)

-/* Return whether the device is using IOMMU translation. */
+/*
+ * Return whether the device is using IOMMU translation.
+ * Called with BQL locked.
+ */
  static bool vtd_switch_address_space(VTDAddressSpace *as)
@@ -1730,4 +1733,2 @@ static bool 
vtd_switch_address_space(VTDAddressSpace *as)
      bool use_iommu, pt;
-    /* Whether we need to take the BQL on our own */
-    bool take_bql = !bql_locked();

@@ -1743,10 +1744,3 @@ static bool 
vtd_switch_address_space(VTDAddressSpace *as)

-    /*
-     * It's possible that we reach here without BQL, e.g., when called
-     * from vtd_pt_enable_fast_path(). However the memory APIs need
-     * it. We'd better make sure we have had it already, or, take it.
-     */
-    if (take_bql) {
-        bql_lock();
-    }
+    memory_region_transaction_begin();

@@ -1803,5 +1797,3 @@ static bool 
vtd_switch_address_space(VTDAddressSpace *as)

-    if (take_bql) {
-        bql_unlock();
-    }
+    memory_region_transaction_commit();

@@ -1905,2 +1897,4 @@ static void 
vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id)

+    BQL_LOCK_GUARD();
+
      if (vtd_switch_address_space(vtd_as) == false) {
@@ -4241,6 +4235,3 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState 
*s, PCIBus *bus,

-        /* Some functions in this branch require the bql, make sure we 
own it */
-        if (take_bql) {
-            bql_lock();
-        }
+        memory_region_transaction_begin();

@@ -4313,5 +4304,3 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState 
*s, PCIBus *bus,

-        if (take_bql) {
-            bql_unlock();
-        }
+        memory_region_transaction_commit();

---

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

[Yi Liu]

Hi CMD,

good catch. Did you see any assert log. Just curious which path complains
it.

On 2025/4/15 14:18, CLEMENT MATHIEU--DRIF wrote:
> 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 | 10 ++++++++++
>   1 file changed, 10 insertions(+)

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dffd7ee885..fea2220013 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,11 @@ 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();
>   
> +        /* Some functions in this branch require 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 +4311,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;

-- 
Regards,
Yi Liu

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

[Yi Liu]

On 2025/4/15 16:03, Philippe Mathieu-Daudé wrote:
> On 15/4/25 09:42, Michael S. Tsirkin wrote:
>> On Tue, Apr 15, 2025 at 07:28:34AM +0000, CLEMENT MATHIEU--DRIF wrote:
>>>
>>>
>>> On 15/04/2025 8:53 am, Philippe Mathieu-Daudé wrote:
>>>> Caution: External email. Do not open attachments or click links, unless
>>>> this email comes from a known sender and you know the content is safe.
>>>>
>>>>
>>>> On 15/4/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>>>>> 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 | 10 ++++++++++
>>>>>    1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>>> index dffd7ee885..fea2220013 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,11 @@ 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();
>>>>>
>>>>> +        /* Some functions in this branch require 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 +4311,10 @@ VTDAddressSpace
>>>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>>>>
>>>>>            vtd_switch_address_space(vtd_dev_as);
>>>>
>>>> Would it help clarifying to propagate this argument down?
>>>> vtd_switch_address_space(VTDAddressSpace *as, bool need_lock);
>>>
>>> Hi phil, vtd_switch_address_space already does the same kind of check
>>>
>>>>
>>>>>
>>>>> +        if (take_bql) {
>>>>> +            bql_unlock();
>>>>> +        }
>>>>> +
>>>>>            g_hash_table_insert(s->vtd_address_spaces, new_key,
>>>>> vtd_dev_as);
>>>>>        }
>>>>>        return vtd_dev_as;
>>>>
>>
>>
>> As an apropos, I think any caller of bql_lock really should call
>> bql_lock_impl so we know who took BQL. Or just use BQL_LOCK_GUARD.
>> But, that's an unrelated cleanup.
>>
> 
> Yeah unrelated cleanup. Although I don't understand why these
> code paths don't use memory_region_transaction_begin/commit and
> have to access BQL.

The below two functions would call memory_region_transaction_begin/commit().
So these paths need BQL.

memory_region_set_enabled()
memory_region_add_subregion_overlap()

-- 
Regards,
Yi Liu

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

[Paolo Bonzini]

On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
> 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>

Please use a separate lock instead of the BQL.

Paolo

> ---
>   hw/i386/intel_iommu.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dffd7ee885..fea2220013 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,11 @@ 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();
>   
> +        /* Some functions in this branch require 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 +4311,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;

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

[Philippe Mathieu-Daudé]

On 15/4/25 11:30, Paolo Bonzini wrote:
> On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>> 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>
> 
> Please use a separate lock instead of the BQL.

There is already a IntelIOMMUState::iommu_lock with
vtd_iommu_lock() / vtd_iommu_unlock() helpers.

> 
> Paolo
> 
>> ---
>>   hw/i386/intel_iommu.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)

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

[CLEMENT MATHIEU--DRIF]

On 15/04/2025 11:30 am, Paolo Bonzini wrote:
> Caution: External email. Do not open attachments or click links, unless 
> this email comes from a known sender and you know the content is safe.
> 
> 
> On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>> 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>
> 
> Please use a separate lock instead of the BQL.

Hi Paolo,

We need this particular lock because some of the functions we call
require the bql to be held.

Is it a problem?

> 
> Paolo
> 
>> ---
>>   hw/i386/intel_iommu.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index dffd7ee885..fea2220013 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,11 @@ 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();
>>
>> +        /* Some functions in this branch require 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 +4311,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;
> 

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

[Paolo Bonzini]

On Tue, Apr 15, 2025 at 1:51 PM CLEMENT MATHIEU--DRIF
<clement.mathieu--drif@eviden.com> wrote:
> On 15/04/2025 11:30 am, Paolo Bonzini wrote:
> > Caution: External email. Do not open attachments or click links, unless
> > this email comes from a known sender and you know the content is safe.
> >
> >
> > On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
> >> 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>
> >
> > Please use a separate lock instead of the BQL.
>
> Hi Paolo,
>
> We need this particular lock because some of the functions we call
> require the bql to be held.

What functions do you need?

> Is it a problem?

It depends on the function. :)

Paolo

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

[Philippe Mathieu-Daudé]

On 15/4/25 13:49, Philippe Mathieu-Daudé wrote:
> On 15/4/25 11:30, Paolo Bonzini wrote:
>> On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>>> 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>
>>
>> Please use a separate lock instead of the BQL.
> 
> There is already a IntelIOMMUState::iommu_lock with
> vtd_iommu_lock() / vtd_iommu_unlock() helpers.

commit 1d9efa73e12ddf361ea997c2d532cc4afa6674d1
Author: Peter Xu <peterx@redhat.com>
Date:   Fri May 18 15:25:11 2018 +0800

     intel-iommu: add iommu lock

     SECURITY IMPLICATION: this patch fixes a potential race when
     multiple threads access the IOMMU IOTLB cache.

     Add a per-iommu big lock to protect IOMMU status.  Currently the
     only thing to be protected is the IOTLB/context cache, since that
     can be accessed even without BQL, e.g., in IO dataplane.

     Note that we don't need to protect device page tables since that's
     fully controlled by the guest kernel.  However there is still
     possibility that malicious drivers will program the device to not
     obey the rule.  In that
     case QEMU can't really do anything useful, instead the guest itself
     will be responsible for all uncertainties.

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 032e33bcb20..016e74bedb7 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -300,6 +300,12 @@ struct IntelIOMMUState {

+
+    /*
+     * Protects IOMMU states in general.  Currently it protects the
+     * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
+     */
+    QemuMutex iommu_lock;

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

[Philippe Mathieu-Daudé]

On 15/4/25 13:51, Paolo Bonzini wrote:
> On Tue, Apr 15, 2025 at 1:51 PM CLEMENT MATHIEU--DRIF
> <clement.mathieu--drif@eviden.com> wrote:
>> On 15/04/2025 11:30 am, Paolo Bonzini wrote:
>>> Caution: External email. Do not open attachments or click links, unless
>>> this email comes from a known sender and you know the content is safe.
>>>
>>>
>>> On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>>>> 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>
>>>
>>> Please use a separate lock instead of the BQL.
>>
>> Hi Paolo,
>>
>> We need this particular lock because some of the functions we call
>> require the bql to be held.
> 
> What functions do you need?
> 
>> Is it a problem?
> 
> It depends on the function. :)

memory_region_set_enabled()
   -> memory_region_transaction_begin()
      -> assert(bql_locked())

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2432 bytes --]

On Tue, Apr 15, 2025 at 03:11:00AM -0400, Michael S. Tsirkin wrote:
> On Tue, Apr 15, 2025 at 06:18:08AM +0000, CLEMENT MATHIEU--DRIF wrote:
> > 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>
> 
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Stefan, want to pick this one up, too?

Not yet, it may need to wait until after the release:
- Discussion is still ongoing.
- Is this a regression in 10.0 or a long-standing issue?
- Who is affected and what is the impact?

There are still a few hours left before -rc4 is tagged. I will merge it
if consensus is reached and the missing information becomes clear.

Thanks,
Stefan

> 
> 
> > ---
> >  hw/i386/intel_iommu.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index dffd7ee885..fea2220013 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,11 @@ 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();
> >  
> > +        /* Some functions in this branch require 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 +4311,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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Paolo Bonzini]

On 4/15/25 13:55, Philippe Mathieu-Daudé wrote:
> On 15/4/25 13:51, Paolo Bonzini wrote:
>> On Tue, Apr 15, 2025 at 1:51 PM CLEMENT MATHIEU--DRIF
>> <clement.mathieu--drif@eviden.com> wrote:
>>> On 15/04/2025 11:30 am, Paolo Bonzini wrote:
>>>> Caution: External email. Do not open attachments or click links, unless
>>>> this email comes from a known sender and you know the content is safe.
>>>>
>>>>
>>>> On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>>>>> 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>
>>>>
>>>> Please use a separate lock instead of the BQL.
>>>
>>> Hi Paolo,
>>>
>>> We need this particular lock because some of the functions we call
>>> require the bql to be held.
>>
>> What functions do you need?
>>
>>> Is it a problem?
>>
>> It depends on the function. :)
> 
> memory_region_set_enabled()
>    -> memory_region_transaction_begin()
>       -> assert(bql_locked())

Oh, I found Yi Liu's reply that came a little before mine.

Yeah, then I guess this is unavoidable (short of adding locks to all of 
memory.c---which would be a good thing but...).  But please mention this 
in the comment that you are adding; this:

/* Some functions in this branch require the bql, make sure we own it */

describes the code but does not explain it.

Paolo

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

[Paolo Bonzini]

On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
> 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 | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dffd7ee885..fea2220013 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();

There is another problem here, in that g_hash_table_lookup is performed 
out of the lock.

You need to:
- take the lock around the first g_hash_table_lookup()
- release it before entering the BQL-protected region
- take the lock again after bql_unlock()
- do another lookup after vtd_switch_address_space(), and throw away 
vtd_dev_as if it succeeds
- then release the lock after g_hash_table_insert().

Note that g_hash_table_insert() replaces the old value with the new one, 
but that's undesirable here so you need a second g_hash_table_lookup().

Thanks,

Paolo

>           new_key->bus = bus;
>           new_key->devfn = devfn;
> @@ -4238,6 +4239,11 @@ 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();
>   
> +        /* Some functions in this branch require 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 +4311,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;

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

[Paolo Bonzini]

On 4/15/25 14:33, Stefan Hajnoczi wrote:
> On Tue, Apr 15, 2025 at 03:11:00AM -0400, Michael S. Tsirkin wrote:
>> On Tue, Apr 15, 2025 at 06:18:08AM +0000, CLEMENT MATHIEU--DRIF wrote:
>>> 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>
>>
>>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> Stefan, want to pick this one up, too?
> 
> Not yet, it may need to wait until after the release:
> - Discussion is still ongoing.
> - Is this a regression in 10.0 or a long-standing issue?
> - Who is affected and what is the impact?
> 
> There are still a few hours left before -rc4 is tagged. I will merge it
> if consensus is reached and the missing information becomes clear.
There are other thread-safety issues in the same code, so I'd prefer to 
hold this patch and not hold the release for it.  Even if it's a 
regression, it can be fixed with a quick 10.0.1 in a couple weeks.

Paolo

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

[Michael S. Tsirkin]

On Tue, Apr 15, 2025 at 08:33:30AM -0400, Stefan Hajnoczi wrote:
> On Tue, Apr 15, 2025 at 03:11:00AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Apr 15, 2025 at 06:18:08AM +0000, CLEMENT MATHIEU--DRIF wrote:
> > > 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>
> > 
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Stefan, want to pick this one up, too?
> 
> Not yet, it may need to wait until after the release:
> - Discussion is still ongoing.
> - Is this a regression in 10.0 or a long-standing issue?
> - Who is affected and what is the impact?

Yea agreed.

> There are still a few hours left before -rc4 is tagged. I will merge it
> if consensus is reached and the missing information becomes clear.
> 
> Thanks,
> Stefan
> 
> > 
> > 
> > > ---
> > >  hw/i386/intel_iommu.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index dffd7ee885..fea2220013 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,11 @@ 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();
> > >  
> > > +        /* Some functions in this branch require 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 +4311,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] intel_iommu: Take the bql before registering a new address space

[CLEMENT MATHIEU--DRIF]

On 15/04/2025 2:33 pm, Stefan Hajnoczi wrote:
> On Tue, Apr 15, 2025 at 03:11:00AM -0400, Michael S. Tsirkin wrote:
>> On Tue, Apr 15, 2025 at 06:18:08AM +0000, CLEMENT MATHIEU--DRIF wrote:
>>> 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>
>>
>>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> Stefan, want to pick this one up, too?
> 
> Not yet, it may need to wait until after the release:
> - Discussion is still ongoing.
> - Is this a regression in 10.0 or a long-standing issue?

It's a long standing issue

> - Who is affected and what is the impact?
> 
> There are still a few hours left before -rc4 is tagged. I will merge it
> if consensus is reached and the missing information becomes clear.
> 
> Thanks,
> Stefan
> 
>>
>>
>>> ---
>>>   hw/i386/intel_iommu.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index dffd7ee885..fea2220013 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,11 @@ 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();
>>>   
>>> +        /* Some functions in this branch require 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 +4311,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] intel_iommu: Take the bql before registering a new address space

[Stefan Hajnoczi]

On Tue, Apr 15, 2025 at 10:16 AM CLEMENT MATHIEU--DRIF
<clement.mathieu--drif@eviden.com> wrote:
> On 15/04/2025 2:33 pm, Stefan Hajnoczi wrote:
> > On Tue, Apr 15, 2025 at 03:11:00AM -0400, Michael S. Tsirkin wrote:
> >> On Tue, Apr 15, 2025 at 06:18:08AM +0000, CLEMENT MATHIEU--DRIF wrote:
> >>> 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>
> >>
> >>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> Stefan, want to pick this one up, too?
> >
> > Not yet, it may need to wait until after the release:
> > - Discussion is still ongoing.
> > - Is this a regression in 10.0 or a long-standing issue?
>
> It's a long standing issue

Thanks for confirming. Let's not worry about the 10.0 release. There's
time to come to a consensus and the fixes can be included in stable
releases afterwards.

Stefan

>
> > - Who is affected and what is the impact?
> >
> > There are still a few hours left before -rc4 is tagged. I will merge it
> > if consensus is reached and the missing information becomes clear.
> >
> > Thanks,
> > Stefan
> >
> >>
> >>
> >>> ---
> >>>   hw/i386/intel_iommu.c | 10 ++++++++++
> >>>   1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >>> index dffd7ee885..fea2220013 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,11 @@ 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();
> >>>
> >>> +        /* Some functions in this branch require 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 +4311,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] intel_iommu: Take the bql before registering a new address space

[CLEMENT MATHIEU--DRIF]

On 15/04/2025 2:59 pm, Paolo Bonzini wrote:
> Caution: External email. Do not open attachments or click links, unless 
> this email comes from a known sender and you know the content is safe.
> 
> 
> On 4/15/25 13:55, Philippe Mathieu-Daudé wrote:
>> On 15/4/25 13:51, Paolo Bonzini wrote:
>>> On Tue, Apr 15, 2025 at 1:51 PM CLEMENT MATHIEU--DRIF
>>> <clement.mathieu--drif@eviden.com> wrote:
>>>> On 15/04/2025 11:30 am, Paolo Bonzini wrote:
>>>>> Caution: External email. Do not open attachments or click links, 
>>>>> unless
>>>>> this email comes from a known sender and you know the content is safe.
>>>>>
>>>>>
>>>>> On 4/15/25 08:18, CLEMENT MATHIEU--DRIF wrote:
>>>>>> 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>
>>>>>
>>>>> Please use a separate lock instead of the BQL.
>>>>
>>>> Hi Paolo,
>>>>
>>>> We need this particular lock because some of the functions we call
>>>> require the bql to be held.
>>>
>>> What functions do you need?
>>>
>>>> Is it a problem?
>>>
>>> It depends on the function. :)
>>
>> memory_region_set_enabled()
>>    -> memory_region_transaction_begin()
>>       -> assert(bql_locked())
> 
> Oh, I found Yi Liu's reply that came a little before mine.
> 
> Yeah, then I guess this is unavoidable (short of adding locks to all of
> memory.c---which would be a good thing but...).  But please mention this
> in the comment that you are adding; this:
> 
> /* Some functions in this branch require the bql, make sure we own it */
> 
> describes the code but does not explain it.

Yes, I will post a v2 with a better comment.
Thanks

cmd

> 
> Paolo
>