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

[CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>]

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