[PATCH v5 0/2] intel_iommu: Fix locking issues

[PATCH v5 0/2] intel_iommu: Fix locking issues

[CLEMENT MATHIEU--DRIF]

This series introduces 2 fixes and improves locking style
consistency in the VT-d device.

Changes since v4:
	- Re-check if the address space is present once both bql and
	  iommu lock are held.


Clement Mathieu--Drif (2):
  intel_iommu: Use BQL_LOCK_GUARD to manage cleanup automatically
  intel_iommu: Take locks when looking for and creating address spaces

 hw/i386/intel_iommu.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

-- 
2.49.0

[PATCH v5 1/2] intel_iommu: Use BQL_LOCK_GUARD to manage cleanup automatically

[CLEMENT MATHIEU--DRIF]

vtd_switch_address_space needs to take the BQL if not already held.
Use BQL_LOCK_GUARD to make the iommu implementation more consistent.

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

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index dffd7ee885..dad1d9f300 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1728,8 +1728,6 @@ static bool vtd_as_pt_enabled(VTDAddressSpace *as)
 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();
 
     assert(as);
 
@@ -1746,9 +1744,7 @@ static bool vtd_switch_address_space(VTDAddressSpace *as)
      * 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();
-    }
+    BQL_LOCK_GUARD();
 
     /* Turn off first then on the other */
     if (use_iommu) {
@@ -1801,10 +1797,6 @@ static bool vtd_switch_address_space(VTDAddressSpace *as)
         memory_region_set_enabled(&as->iommu_ir_fault, false);
     }
 
-    if (take_bql) {
-        bql_unlock();
-    }
-
     return use_iommu;
 }
 
-- 
2.49.0

[PATCH v5 2/2] intel_iommu: Take locks when looking for and creating address spaces

[CLEMENT MATHIEU--DRIF]

vtd_find_add_as can be called by multiple threads which leads to a race
condition. Taking the IOMMU lock ensures we avoid such a race.
Moreover we also need to take the bql to avoid an assert to fail in
memory_region_add_subregion_overlap when actually allocating a new
address space.

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

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index dad1d9f300..144e25622a 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4205,9 +4205,30 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
     VTDAddressSpace *vtd_dev_as;
     char name[128];
 
+    vtd_iommu_lock(s);
     vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
+    vtd_iommu_unlock(s);
+
     if (!vtd_dev_as) {
-        struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));
+        struct vtd_as_key *new_key;
+        /* Slow path */
+
+        /*
+        * memory_region_add_subregion_overlap requires the bql,
+        * make sure we own it.
+        */
+        BQL_LOCK_GUARD();
+        vtd_iommu_lock(s);
+
+        /* Check again as we released the lock for a moment */
+        vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
+        if (vtd_dev_as) {
+            vtd_iommu_unlock(s);
+            return vtd_dev_as;
+        }
+
+        /* Still nothing, allocate a new address space */
+        new_key = g_malloc(sizeof(*new_key));
 
         new_key->bus = bus;
         new_key->devfn = devfn;
@@ -4298,6 +4319,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
         vtd_switch_address_space(vtd_dev_as);
 
         g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
+    
+        vtd_iommu_unlock(s);
     }
     return vtd_dev_as;
 }
-- 
2.49.0

Re: [PATCH v5 0/2] intel_iommu: Fix locking issues

[CLEMENT MATHIEU--DRIF]

Cc'ing Paolo

On 30/04/2025 2:48 pm, CLEMENT MATHIEU--DRIF wrote:
> This series introduces 2 fixes and improves locking style
> consistency in the VT-d device.
> 
> Changes since v4:
> 	- Re-check if the address space is present once both bql and
> 	  iommu lock are held.
> 
> 
> Clement Mathieu--Drif (2):
>    intel_iommu: Use BQL_LOCK_GUARD to manage cleanup automatically
>    intel_iommu: Take locks when looking for and creating address spaces
> 
>   hw/i386/intel_iommu.c | 35 +++++++++++++++++++++++++----------
>   1 file changed, 25 insertions(+), 10 deletions(-)
> 

RE: [PATCH v5 0/2] intel_iommu: Fix locking issues

[Duan, Zhenzhong]

Hi Clement,

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: [PATCH v5 0/2] intel_iommu: Fix locking issues
>
>This series introduces 2 fixes and improves locking style
>consistency in the VT-d device.
>
>Changes since v4:
>	- Re-check if the address space is present once both bql and
>	  iommu lock are held.

Yes, this is cleaner. For the whole series:

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

Re: [PATCH v5 2/2] intel_iommu: Take locks when looking for and creating address spaces

[Michael S. Tsirkin]

On Wed, Apr 30, 2025 at 12:48:06PM +0000, CLEMENT MATHIEU--DRIF wrote:
> vtd_find_add_as can be called by multiple threads which leads to a race
> condition. Taking the IOMMU lock ensures we avoid such a race.
> Moreover we also need to take the bql to avoid an assert to fail in
> memory_region_add_subregion_overlap when actually allocating a new
> address space.
> 
> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
> ---
>  hw/i386/intel_iommu.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dad1d9f300..144e25622a 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4205,9 +4205,30 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>      VTDAddressSpace *vtd_dev_as;
>      char name[128];
>  
> +    vtd_iommu_lock(s);
>      vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
> +    vtd_iommu_unlock(s);
> +
>      if (!vtd_dev_as) {
> -        struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));
> +        struct vtd_as_key *new_key;
> +        /* Slow path */
> +
> +        /*
> +        * memory_region_add_subregion_overlap requires the bql,
> +        * make sure we own it.
> +        */


not how comments should look

> +        BQL_LOCK_GUARD();
> +        vtd_iommu_lock(s);
> +
> +        /* Check again as we released the lock for a moment */
> +        vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
> +        if (vtd_dev_as) {
> +            vtd_iommu_unlock(s);
> +            return vtd_dev_as;
> +        }
> +
> +        /* Still nothing, allocate a new address space */
> +        new_key = g_malloc(sizeof(*new_key));
>  
>          new_key->bus = bus;
>          new_key->devfn = devfn;
> @@ -4298,6 +4319,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>          vtd_switch_address_space(vtd_dev_as);
>  
>          g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
> +    

trailing whitespace here

> +        vtd_iommu_unlock(s);
>      }
>      return vtd_dev_as;
>  }



I fixed these up but pls take care, Clement.

> -- 
> 2.49.0

Re: [PATCH v5 2/2] intel_iommu: Take locks when looking for and creating address spaces

[CLEMENT MATHIEU--DRIF]

On 14/05/2025 1:48 pm, Michael S. Tsirkin 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 Wed, Apr 30, 2025 at 12:48:06PM +0000, CLEMENT MATHIEU--DRIF wrote:
>> vtd_find_add_as can be called by multiple threads which leads to a race
>> condition. Taking the IOMMU lock ensures we avoid such a race.
>> Moreover we also need to take the bql to avoid an assert to fail in
>> memory_region_add_subregion_overlap when actually allocating a new
>> address space.
>>
>> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> ---
>>   hw/i386/intel_iommu.c | 25 ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index dad1d9f300..144e25622a 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -4205,9 +4205,30 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>       VTDAddressSpace *vtd_dev_as;
>>       char name[128];
>>
>> +    vtd_iommu_lock(s);
>>       vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
>> +    vtd_iommu_unlock(s);
>> +
>>       if (!vtd_dev_as) {
>> -        struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));
>> +        struct vtd_as_key *new_key;
>> +        /* Slow path */
>> +
>> +        /*
>> +        * memory_region_add_subregion_overlap requires the bql,
>> +        * make sure we own it.
>> +        */
> 
> 
> not how comments should look
> 
>> +        BQL_LOCK_GUARD();
>> +        vtd_iommu_lock(s);
>> +
>> +        /* Check again as we released the lock for a moment */
>> +        vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
>> +        if (vtd_dev_as) {
>> +            vtd_iommu_unlock(s);
>> +            return vtd_dev_as;
>> +        }
>> +
>> +        /* Still nothing, allocate a new address space */
>> +        new_key = g_malloc(sizeof(*new_key));
>>
>>           new_key->bus = bus;
>>           new_key->devfn = devfn;
>> @@ -4298,6 +4319,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>           vtd_switch_address_space(vtd_dev_as);
>>
>>           g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
>> +
> 
> trailing whitespace here
> 
>> +        vtd_iommu_unlock(s);
>>       }
>>       return vtd_dev_as;
>>   }
> 
> 
> 
> I fixed these up but pls take care, Clement.

Ouch, sorry, thanks Michael

> 
>> --
>> 2.49.0
>