[PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space

[PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space

[CLEMENT MATHIEU--DRIF]

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index dffd7ee885..cc8c9857e1 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4238,6 +4238,12 @@ 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();
 
+        /*
+         * memory_region_add_subregion_overlap requires the bql,
+         * make sure we own it.
+         */
+        BQL_LOCK_GUARD();
+
         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");
 
-- 
2.49.0

[PATCH v3 2/2] intel_iommu: Use BQL_LOCK_GUARD to manage cleanup automatically

[CLEMENT MATHIEU--DRIF]

vtd_switch_address_space needs to take the BQL if not already held.
Use BQL_LOCK_GUARD to make the iommu implementation more consistent.

Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
---
 hw/i386/intel_iommu.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index cc8c9857e1..b7855f4b87 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1728,8 +1728,6 @@ static bool vtd_as_pt_enabled(VTDAddressSpace *as)
 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();
 
     assert(as);
 
@@ -1746,9 +1744,7 @@ static bool vtd_switch_address_space(VTDAddressSpace *as)
      * 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();
-    }
+    BQL_LOCK_GUARD();
 
     /* Turn off first then on the other */
     if (use_iommu) {
@@ -1801,10 +1797,6 @@ static bool vtd_switch_address_space(VTDAddressSpace *as)
         memory_region_set_enabled(&as->iommu_ir_fault, false);
     }
 
-    if (take_bql) {
-        bql_unlock();
-    }
-
     return use_iommu;
 }
 
-- 
2.49.0

Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space

[Michael S. Tsirkin]

On Wed, Apr 23, 2025 at 05:38:20AM +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--drif@eviden.com>
> ---
>  hw/i386/intel_iommu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index dffd7ee885..cc8c9857e1 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4238,6 +4238,12 @@ 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();
>  
> +        /*
> +         * memory_region_add_subregion_overlap requires the bql,
> +         * make sure we own it.
> +         */
> +        BQL_LOCK_GUARD();
> +
>          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");

Does not look like this addresses all races here:
https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com


while this can be a separate patch on top, I'd rather we just
address everything in a single patchset.


> -- 
> 2.49.0

Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space

[CLEMENT MATHIEU--DRIF]

On 23/04/2025 8:00 am, Michael S. Tsirkin 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 Wed, Apr 23, 2025 at 05:38:20AM +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--drif@eviden.com>
>> ---
>>   hw/i386/intel_iommu.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index dffd7ee885..cc8c9857e1 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -4238,6 +4238,12 @@ 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();
>>
>> +        /*
>> +         * memory_region_add_subregion_overlap requires the bql,
>> +         * make sure we own it.
>> +         */
>> +        BQL_LOCK_GUARD();
>> +
>>           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");
>
> Does not look like this addresses all races here:
> https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com
>
>
> while this can be a separate patch on top, I'd rather we just
> address everything in a single patchset.

Hi Michael,

We only aim to fix the potential crash here.
I saw Paolo's response and I know the race exists. I will send a patch
set to fix it soon but are you sure both fixes must be in the same
series? I think the nature is different.

cmd

>
>
>> --
>> 2.49.0
>

Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space

[Michael S. Tsirkin]

On Wed, Apr 23, 2025 at 09:15:36AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 
> 
> On 23/04/2025 8:00 am, Michael S. Tsirkin 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 Wed, Apr 23, 2025 at 05:38:20AM +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--drif@eviden.com>
> >> ---
> >>   hw/i386/intel_iommu.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index dffd7ee885..cc8c9857e1 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -4238,6 +4238,12 @@ 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();
> >>
> >> +        /*
> >> +         * memory_region_add_subregion_overlap requires the bql,
> >> +         * make sure we own it.
> >> +         */
> >> +        BQL_LOCK_GUARD();
> >> +
> >>           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");
> >
> > Does not look like this addresses all races here:
> > https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com
> >
> >
> > while this can be a separate patch on top, I'd rather we just
> > address everything in a single patchset.
> 
> Hi Michael,
> 
> We only aim to fix the potential crash here.
> I saw Paolo's response and I know the race exists. I will send a patch
> set to fix it soon but are you sure both fixes must be in the same
> series? I think the nature is different.
> 
> cmd

If you have two races in the same function, fixing one can easily
make another one occur more. Let's just fix it all please,
I don't see any rush to apply a partial fix.

-- 
MST

Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space

[CLEMENT MATHIEU--DRIF]

On 23/04/2025 12:06 pm, Michael S. Tsirkin 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 Wed, Apr 23, 2025 at 09:15:36AM +0000, CLEMENT MATHIEU--DRIF wrote:
>>
>>
>> On 23/04/2025 8:00 am, Michael S. Tsirkin 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 Wed, Apr 23, 2025 at 05:38:20AM +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--drif@eviden.com>
>>>> ---
>>>>    hw/i386/intel_iommu.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index dffd7ee885..cc8c9857e1 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -4238,6 +4238,12 @@ 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();
>>>>
>>>> +        /*
>>>> +         * memory_region_add_subregion_overlap requires the bql,
>>>> +         * make sure we own it.
>>>> +         */
>>>> +        BQL_LOCK_GUARD();
>>>> +
>>>>            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");
>>>
>>> Does not look like this addresses all races here:
>>> https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com
>>>
>>>
>>> while this can be a separate patch on top, I'd rather we just
>>> address everything in a single patchset.
>>
>> Hi Michael,
>>
>> We only aim to fix the potential crash here.
>> I saw Paolo's response and I know the race exists. I will send a patch
>> set to fix it soon but are you sure both fixes must be in the same
>> series? I think the nature is different.
>>
>> cmd
>
> If you have two races in the same function, fixing one can easily
> make another one occur more. Let's just fix it all please,
> I don't see any rush to apply a partial fix.
>

Fine, will do!

> --
> MST
>