On Tue, Apr 15, 2025 at 10:16 AM CLEMENT MATHIEU--DRIF
<[email protected]> wrote:
> On 15/04/2025 2:33 pm, Stefan Hajnoczi wrote:
> > On Tue, Apr 15, 2025 at 03:11:00AM -0400, Michael S. Tsirkin wrote:
> >> On Tue, Apr 15, 2025 at 06:18:08AM +0000, CLEMENT MATHIEU--DRIF wrote:
> >>> Address space creation might end up being called without holding the
> >>> bql as it is exposed through the IOMMU ops.
> >>>
> >>> Signed-off-by: Clement Mathieu--Drif <[email protected]>
> >>
> >>
> >> Reviewed-by: Michael S. Tsirkin <[email protected]>
> >>
> >> Stefan, want to pick this one up, too?
> >
> > Not yet, it may need to wait until after the release:
> > - Discussion is still ongoing.
> > - Is this a regression in 10.0 or a long-standing issue?
>
> It's a long standing issue
Thanks for confirming. Let's not worry about the 10.0 release. There's
time to come to a consensus and the fixes can be included in stable
releases afterwards.
Stefan
>
> > - Who is affected and what is the impact?
> >
> > There are still a few hours left before -rc4 is tagged. I will merge it
> > if consensus is reached and the missing information becomes clear.
> >
> > Thanks,
> > Stefan
> >
> >>
> >>
> >>> ---
> >>> hw/i386/intel_iommu.c | 10 ++++++++++
> >>> 1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >>> index dffd7ee885..fea2220013 100644
> >>> --- a/hw/i386/intel_iommu.c
> >>> +++ b/hw/i386/intel_iommu.c
> >>> @@ -4216,6 +4216,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState
> >>> *s, PCIBus *bus,
> >>> vtd_dev_as = g_hash_table_lookup(s->vtd_address_spaces, &key);
> >>> if (!vtd_dev_as) {
> >>> struct vtd_as_key *new_key = g_malloc(sizeof(*new_key));
> >>> + bool take_bql = !bql_locked();
> >>>
> >>> new_key->bus = bus;
> >>> new_key->devfn = devfn;
> >>> @@ -4238,6 +4239,11 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState
> >>> *s, PCIBus *bus,
> >>> vtd_dev_as->context_cache_entry.context_cache_gen = 0;
> >>> vtd_dev_as->iova_tree = iova_tree_new();
> >>>
> >>> + /* Some functions in this branch require the bql, make sure we
> >>> own it */
> >>> + if (take_bql) {
> >>> + bql_lock();
> >>> + }
> >>> +
> >>> memory_region_init(&vtd_dev_as->root, OBJECT(s), name,
> >>> UINT64_MAX);
> >>> address_space_init(&vtd_dev_as->as, &vtd_dev_as->root,
> >>> "vtd-root");
> >>>
> >>> @@ -4305,6 +4311,10 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState
> >>> *s, PCIBus *bus,
> >>>
> >>> vtd_switch_address_space(vtd_dev_as);
> >>>
> >>> + if (take_bql) {
> >>> + bql_unlock();
> >>> + }
> >>> +
> >>> g_hash_table_insert(s->vtd_address_spaces, new_key, vtd_dev_as);
> >>> }
> >>> return vtd_dev_as;
> >>> --
> >>> 2.49.0
> >>
