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

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

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