On Tue, Apr 15, 2025 at 07:28:34AM +0000, CLEMENT MATHIEU--DRIF wrote:
>
>
> On 15/04/2025 8:53 am, Philippe Mathieu-Daudé 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 15/4/25 08:18, 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 <clement.mathieu--d...@eviden.com>
> >> ---
> >> 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);
> >
> > Would it help clarifying to propagate this argument down?
> > vtd_switch_address_space(VTDAddressSpace *as, bool need_lock);
>
> Hi phil, vtd_switch_address_space already does the same kind of check
>
> >
> >>
> >> + if (take_bql) {
> >> + bql_unlock();
> >> + }
> >> +
> >> g_hash_table_insert(s->vtd_address_spaces, new_key,
> >> vtd_dev_as);
> >> }
> >> return vtd_dev_as;
> >
As an apropos, I think any caller of bql_lock really should call
bql_lock_impl so we know who took BQL. Or just use BQL_LOCK_GUARD.
But, that's an unrelated cleanup.
