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); address_space_destroy(&vtd_dev_as->as); g_free(vtd_dev_as); g_free(new_key); vtd_dev_as = second_vtd_dev_as; } ... return vtd_dev_as; > > Thanks > Zhenzhong