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

["Michael S. Tsirkin" <mst@redhat.com>]

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