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.
Some untested idea to investigate:
-- >8 --
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index fea22200135..b2a809cb3db 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1726,3 +1726,6 @@ static bool vtd_as_pt_enabled(VTDAddressSpace *as)
-/* Return whether the device is using IOMMU translation. */
+/*
+ * Return whether the device is using IOMMU translation.
+ * Called with BQL locked.
+ */
static bool vtd_switch_address_space(VTDAddressSpace *as)
@@ -1730,4 +1733,2 @@ static bool
vtd_switch_address_space(VTDAddressSpace *as)
bool use_iommu, pt;
- /* Whether we need to take the BQL on our own */
- bool take_bql = !bql_locked();
@@ -1743,10 +1744,3 @@ static bool
vtd_switch_address_space(VTDAddressSpace *as)
- /*
- * It's possible that we reach here without BQL, e.g., when called
- * from vtd_pt_enable_fast_path(). However the memory APIs need
- * it. We'd better make sure we have had it already, or, take it.
- */
- if (take_bql) {
- bql_lock();
- }
+ memory_region_transaction_begin();
@@ -1803,5 +1797,3 @@ static bool
vtd_switch_address_space(VTDAddressSpace *as)
- if (take_bql) {
- bql_unlock();
- }
+ memory_region_transaction_commit();
@@ -1905,2 +1897,4 @@ static void
vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id)
+ BQL_LOCK_GUARD();
+
if (vtd_switch_address_space(vtd_as) == false) {
@@ -4241,6 +4235,3 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState
*s, PCIBus *bus,
- /* Some functions in this branch require the bql, make sure we
own it */
- if (take_bql) {
- bql_lock();
- }
+ memory_region_transaction_begin();
@@ -4313,5 +4304,3 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState
*s, PCIBus *bus,
- if (take_bql) {
- bql_unlock();
- }
+ memory_region_transaction_commit();
---
