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.
> --
> 2.49.0
