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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.

Some untested idea to investigate:
-- >8 --
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index fea22200135..b2a809cb3db 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1726,3 +1726,6 @@ static bool vtd_as_pt_enabled(VTDAddressSpace *as)

-/* Return whether the device is using IOMMU translation. */
+/*
+ * Return whether the device is using IOMMU translation.
+ * Called with BQL locked.
+ */
  static bool vtd_switch_address_space(VTDAddressSpace *as)
@@ -1730,4 +1733,2 @@ static bool 
vtd_switch_address_space(VTDAddressSpace *as)
      bool use_iommu, pt;
-    /* Whether we need to take the BQL on our own */
-    bool take_bql = !bql_locked();

@@ -1743,10 +1744,3 @@ static bool 
vtd_switch_address_space(VTDAddressSpace *as)

-    /*
-     * It's possible that we reach here without BQL, e.g., when called
-     * from vtd_pt_enable_fast_path(). However the memory APIs need
-     * it. We'd better make sure we have had it already, or, take it.
-     */
-    if (take_bql) {
-        bql_lock();
-    }
+    memory_region_transaction_begin();

@@ -1803,5 +1797,3 @@ static bool 
vtd_switch_address_space(VTDAddressSpace *as)

-    if (take_bql) {
-        bql_unlock();
-    }
+    memory_region_transaction_commit();

@@ -1905,2 +1897,4 @@ static void 
vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id)

+    BQL_LOCK_GUARD();
+
      if (vtd_switch_address_space(vtd_as) == false) {
@@ -4241,6 +4235,3 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState 
*s, PCIBus *bus,

-        /* Some functions in this branch require the bql, make sure we 
own it */
-        if (take_bql) {
-            bql_lock();
-        }
+        memory_region_transaction_begin();

@@ -4313,5 +4304,3 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState 
*s, PCIBus *bus,

-        if (take_bql) {
-            bql_unlock();
-        }
+        memory_region_transaction_commit();

---