On 14/05/2025 1:48 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 30, 2025 at 12:48:06PM +0000, CLEMENT MATHIEU--DRIF wrote: >> vtd_find_add_as can be called by multiple threads which leads to a race >> condition. Taking the IOMMU lock ensures we avoid such a race. >> Moreover we also need to take the bql to avoid an assert to fail in >> memory_region_add_subregion_overlap when actually allocating a new >> address space. >> >> Signed-off-by: Clement Mathieu--Drif <[email protected]> >> --- >> hw/i386/intel_iommu.c | 25 ++++++++++++++++++++++++- >> 1 file changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index dad1d9f300..144e25622a 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -4205,9 +4205,30 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, >> PCIBus *bus, >> VTDAddressSpace *vtd_dev_as; >> 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)); >> + struct vtd_as_key *new_key; >> + /* Slow path */ >> + >> + /* >> + * memory_region_add_subregion_overlap requires the bql, >> + * make sure we own it. >> + */ > > > not how comments should look > >> + BQL_LOCK_GUARD(); >> + vtd_iommu_lock(s); >> + >> + /* Check again as we released the lock for a moment */ >> + vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key); >> + if (vtd_dev_as) { >> + vtd_iommu_unlock(s); >> + return vtd_dev_as; >> + } >> + >> + /* Still nothing, allocate a new address space */ >> + new_key = g_malloc(sizeof(*new_key)); >> >> new_key->bus = bus; >> new_key->devfn = devfn; >> @@ -4298,6 +4319,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, >> PCIBus *bus, >> vtd_switch_address_space(vtd_dev_as); >> >> g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as); >> + > > trailing whitespace here > >> + vtd_iommu_unlock(s); >> } >> return vtd_dev_as; >> } > > > > I fixed these up but pls take care, Clement.
Ouch, sorry, thanks Michael > >> -- >> 2.49.0 >
