>-----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);
Thanks
Zhenzhong
