On 2025/4/15 16:03, Philippe Mathieu-Daudé wrote:
On 15/4/25 09:42, Michael S. Tsirkin wrote:
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.
Yeah unrelated cleanup. Although I don't understand why these
code paths don't use memory_region_transaction_begin/commit and
have to access BQL.
The below two functions would call memory_region_transaction_begin/commit().
So these paths need BQL.
memory_region_set_enabled()
memory_region_add_subregion_overlap()
--
Regards,
Yi Liu
