On 29/04/2025 8:01 am, Duan, Zhenzhong 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.
>
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--d...@eviden.com>
>> Subject: Re: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for
>> and
>> creating address spaces
>>
>> Hi Zhenzhong,
>>
>> On 28/04/2025 10:55 am, Duan, Zhenzhong 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.
>>>
>>> Hi Clement,
>>>
>>>> -----Original Message-----
>>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--d...@eviden.com>
>>>> Subject: [PATCH v4 3/3] intel_iommu: Take the VTD lock when looking for and
>>>> creating address spaces
>>>>
>>>> vtd_find_add_as can be called by multiple threads which leads to a race
>>>> condition on address space creation. The IOMMU lock must be taken to
>>>> avoid such a race.
>>>>
>>>> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--d...@eviden.com>
>>>> ---
>>>> hw/i386/intel_iommu.c | 28 ++++++++++++++++++++++++++--
>>>> 1 file changed, 26 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index b7855f4b87..931ac01ef0 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -4203,11 +4203,15 @@ VTDAddressSpace
>>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>>> .pasid = pasid,
>>>> };
>>>> VTDAddressSpace *vtd_dev_as;
>>>> + struct vtd_as_key *new_key = NULL;
>>>> char name[128];
>>>>
>>>> + vtd_iommu_lock(s);
>>>> vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
>>>> + vtd_iommu_unlock(s);
>>>> +
>>>> if (!vtd_dev_as) {
>>>> - struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));
>>>> + new_key = g_malloc(sizeof(*new_key));
>>>>
>>>> new_key->bus = bus;
>>>> new_key->devfn = devfn;
>>>> @@ -4302,9 +4306,29 @@ VTDAddressSpace
>>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>>> &vtd_dev_as->nodmar, 0);
>>>>
>>>> vtd_switch_address_space(vtd_dev_as);
>>>> + }
>>>>
>>>> - g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
>>>> + if (new_key != NULL) {
>>>> + VTDAddressSpace *second_vtd_dev_as;
>>>> +
>>>> + /*
>>>> + * Take the lock again and recheck as the AS might have
>>>> + * been created in the meantime.
>>>> + */
>>>> + vtd_iommu_lock(s);
>>>> +
>>>> + second_vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces,
>> &key);
>>>> + if (!second_vtd_dev_as) {
>>>> + g_hash_table_insert(s->vtd_address_spaces, new_key,
>>>> vtd_dev_as);
>>>> + } else {
>>>> + vtd_dev_as = second_vtd_dev_as;
>>>> + g_free(vtd_dev_as);
>>>> + g_free(new_key);
>>> We need to release memory regions under this vtd_dev_as to avoid leak.
>>
>> Indeed, I'll have to take the BQL again.
>>
>> Is it ok for you if it look like this:
>>
>> vtd_iommu_lock(s);
>>
>> second_vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
>> if (!second_vtd_dev_as) {
>> g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
>> vtd_iommu_unlock(s);
>> } else {
>> vtd_iommu_unlock(s);
>> BQL_LOCK_GUARD();
>>
>> memory_region_del_subregion(&vtd_dev_as->root, &vtd_dev_as->nodmar);
>> memory_region_del_subregion(&vtd_dev_as->root,
>> MEMORY_REGION(&vtd_dev_as->iommu));
>> memory_region_del_subregion(&vtd_dev_as->root,
>> &vtd_dev_as->iommu_ir_fault);
>> memory_region_del_subregion(&vtd_dev_as->root,
>> &vtd_dev_as->iommu_ir);
>>
>> memory_region_unref(MEMORY_REGION(&vtd_dev_as->iommu));
>> memory_region_unref(&vtd_dev_as->iommu_ir_fault);
>> memory_region_unref(&vtd_dev_as->iommu_ir);
>> memory_region_unref(&vtd_dev_as->nodmar);
> s/memory_region_unref/object_unparent?
>
>> address_space_destroy(&vtd_dev_as->as);
> object_unparent(vtd_dev_as->root);
Hi Zhenzhong,
I think the issue can be fixed differently in a cleaner way.
I sent a v5 for that.
Thanks
>
> Thanks
> Zhenzhong
