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 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.