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

[Stefan Hajnoczi]

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--d...@eviden.com>
> 
> 
> Reviewed-by: Michael S. Tsirkin <m...@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
> 

signature.asc
Description: PGP signature