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

[Yi Liu <yi.l.liu@intel.com>]

On 2025/4/15 16:03, Philippe Mathieu-Daudé wrote:
> On 15/4/25 09:42, Michael S. Tsirkin wrote:
>> 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.
>>
> 
> Yeah unrelated cleanup. Although I don't understand why these
> code paths don't use memory_region_transaction_begin/commit and
> have to access BQL.

The below two functions would call memory_region_transaction_begin/commit().
So these paths need BQL.

memory_region_set_enabled()
memory_region_add_subregion_overlap()

-- 
Regards,
Yi Liu