On 2018年04月28日 10:24, Peter Xu wrote:
On Sat, Apr 28, 2018 at 09:43:54AM +0800, Jason Wang wrote:
On 2018年04月27日 14:26, Peter Xu wrote:
On Fri, Apr 27, 2018 at 01:13:02PM +0800, Jason Wang wrote:
On 2018年04月25日 12:51, Peter Xu wrote:
Add a per-iommu big lock to protect IOMMU status. Currently the only
thing to be protected is the IOTLB cache, since that can be accessed
even without BQL, e.g., in IO dataplane.
Note that device page tables should not need any protection. The safety
of that should be provided by guest OS. E.g., when a page entry is
freed, the guest OS should be responsible to make sure that no device
will be using that page any more.
Reported-by: Fam Zheng<address@hidden>
Signed-off-by: Peter Xu<address@hidden>
---
include/hw/i386/intel_iommu.h | 8 ++++++++
hw/i386/intel_iommu.c | 31 +++++++++++++++++++++++++++++--
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 220697253f..1a8ba8e415 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -262,6 +262,14 @@ struct IntelIOMMUState {
uint8_t w1cmask[DMAR_REG_SIZE]; /* RW1C(Write 1 to Clear) bytes */
uint8_t womask[DMAR_REG_SIZE]; /* WO (write only - read returns 0) */
uint32_t version;
+ /*
+ * Protects IOMMU states in general. Normally we don't need to
+ * take this lock when we are with BQL held. However we have code
+ * paths that may run even without BQL. In those cases, we need
+ * to take the lock when we have access to IOMMU state
+ * informations, e.g., the IOTLB.
+ */
+ QemuMutex iommu_lock;
Some questions:
1) Do we need to protect context cache too?
IMHO the context cache entry should work even without lock. That's a
bit trickly since we have two cases that this cache will be updated:
(1) first translation of the address space of a device
(2) invalidation of context entries
For (2) IMHO we don't need to worry about since guest OS should be
controlling that part, say, device should not be doing any translation
(DMA operations) when the context entry is invalidated.
For (1) the worst case is that the context entry cache be updated
multiple times with the same value by multiple threads. IMHO that'll
be fine too.
But yes for sure we can protect that too with the iommu lock.
2) Can we just reuse qemu BQL here?
I would prefer not. As I mentioned, at least I have spent too much
time on fighting BQL already. I really hope we can start to use
isolated locks when capable. BQL is always the worst choice to me.
Just a thought, using BQL may greatly simplify the code actually (consider
we don't plan to remove BQL now).
Frankly speaking I don't understand why using BQL may greatly simplify
the code... :( IMHO the lock here is really not a complicated one.
Note that IMO BQL is mostly helpful when we really want something to
be run sequentially with some other things _already_ protected by BQL.
Except for the translate path from dataplane, I belive all other codes were
already protected by BQL.
In this case, all the stuff is inside VT-d code itself (or other
IOMMUs), why bother taking the BQL to make our life harder?
It looks to me it was as simple as:
@@ -494,6 +494,7 @@ static MemoryRegionSection
flatview_do_translate(FlatView *fv,
IOMMUMemoryRegionClass *imrc;
hwaddr page_mask = (hwaddr)(-1);
hwaddr plen = (hwaddr)(-1);
+ int locked = false;
if (plen_out) {
plen = *plen_out;
@@ -510,8 +511,15 @@ static MemoryRegionSection
flatview_do_translate(FlatView *fv,
}
imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
+ if (!qemu_mutex_iothread_locked()) {
+ locked = true;
+ qemu_mutex_lock_iothread();
+ }
iotlb = imrc->translate(iommu_mr, addr, is_write ?
IOMMU_WO : IOMMU_RO);
+ if (locked) {
+ qemu_mutex_unlock_iothread();
+ }
addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
| (addr & iotlb.addr_mask));
page_mask &= iotlb.addr_mask;