* [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes
@ 2017-05-10 8:01 Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 01/12] pc: add 2.10 machine type Peter Xu
` (12 more replies)
0 siblings, 13 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
v3:
- add one patch to provide machine type 2.10 for pc, add r-b for Edurado
- add r-b/a-b for David on the two memory patches
- add a-b for Paolo on the two memory patches
- remove useless if in vtd_switch_address_space() [Jason]
- check pt_supported when needed [Yi]
- one more patch to check whether dev-iotlb is supported before
allowing such type of context entry
- enable pt fast path (squashed into current patch 10). when we found
pt is setup on the first translation, we do address space switch.
When it's unset, we can capture it via invalidations. [Jason]
- add compat bit for HW_COMPAT_2_9 for "pt" param [Jason]
- one vhost fix for pt (added patch 4)
- faster vhost caching (added patch 12) [Jason]
This series add support for per-device passthrough mode for VT-d
emulation, along with some tweaks on existing codes.
Patch 1: introduce pc 2.10 machine type, it's in mst's pull request
already, but to be complete, I included this patch.
Patches 2-3: memory related cleanups.
Patch 4: a fix for vhost to be prepared for passthrough mode.
Patches 5-9: some VT-d cleanups and fixes.
Patch 10: add support for passthrough.
Patch 11: turn pt off for machine type <=2.9, for compatibility.
Patch 12: vhost enhancement when used with passthrough, to pre-cache
static mappings.
A simple test with PT mode using 10g nic is line speed.
Please review. Thanks.
Peter Xu (12):
pc: add 2.10 machine type
memory: tune last param of iommu_ops.translate()
memory: remove the last param in memory_region_iommu_replay()
memory: fix address_space_get_iotlb_entry()
x86-iommu: use DeviceClass properties
intel_iommu: renaming context entry helpers
intel_iommu: provide vtd_ce_get_type()
intel_iommu: use IOMMU_ACCESS_FLAG()
intel_iommu: allow dev-iotlb context entry conditionally
intel_iommu: support passthrough (PT)
intel_iommu: turn off pt before 2.9
vhost: iommu: cache static mapping if there is
exec.c | 26 ++++-
hw/alpha/typhoon.c | 2 +-
hw/dma/rc4030.c | 2 +-
hw/i386/amd_iommu.c | 4 +-
hw/i386/intel_iommu.c | 260 +++++++++++++++++++++++++++++++----------
hw/i386/intel_iommu_internal.h | 1 +
hw/i386/pc_piix.c | 15 ++-
hw/i386/pc_q35.c | 13 ++-
hw/i386/trace-events | 2 +
hw/i386/x86-iommu.c | 48 ++------
hw/pci-host/apb.c | 2 +-
hw/ppc/spapr_iommu.c | 2 +-
hw/s390x/s390-pci-bus.c | 2 +-
hw/s390x/s390-pci-inst.c | 2 +-
hw/vfio/common.c | 2 +-
hw/virtio/trace-events | 4 +
hw/virtio/vhost.c | 49 ++++++++
include/exec/memory.h | 15 ++-
include/hw/compat.h | 6 +-
include/hw/i386/pc.h | 3 +
include/hw/i386/x86-iommu.h | 1 +
memory.c | 7 +-
22 files changed, 339 insertions(+), 129 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 01/12] pc: add 2.10 machine type
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 02/12] memory: tune last param of iommu_ops.translate() Peter Xu
` (11 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/i386/pc_piix.c | 15 ++++++++++++---
hw/i386/pc_q35.c | 13 +++++++++++--
include/hw/i386/pc.h | 3 +++
3 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9f102aa..8fb6553 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -437,21 +437,30 @@ static void pc_i440fx_machine_options(MachineClass *m)
m->default_display = "std";
}
-static void pc_i440fx_2_9_machine_options(MachineClass *m)
+static void pc_i440fx_2_10_machine_options(MachineClass *m)
{
pc_i440fx_machine_options(m);
m->alias = "pc";
m->is_default = 1;
}
+DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
+ pc_i440fx_2_10_machine_options);
+
+static void pc_i440fx_2_9_machine_options(MachineClass *m)
+{
+ pc_i440fx_2_10_machine_options(m);
+ m->is_default = 0;
+ m->alias = NULL;
+ SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
+}
+
DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
pc_i440fx_2_9_machine_options);
static void pc_i440fx_2_8_machine_options(MachineClass *m)
{
pc_i440fx_2_9_machine_options(m);
- m->is_default = 0;
- m->alias = NULL;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index dd792a8..f07ebec 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -301,19 +301,28 @@ static void pc_q35_machine_options(MachineClass *m)
m->max_cpus = 288;
}
-static void pc_q35_2_9_machine_options(MachineClass *m)
+static void pc_q35_2_10_machine_options(MachineClass *m)
{
pc_q35_machine_options(m);
m->alias = "q35";
}
+DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
+ pc_q35_2_10_machine_options);
+
+static void pc_q35_2_9_machine_options(MachineClass *m)
+{
+ pc_q35_2_10_machine_options(m);
+ m->alias = NULL;
+ SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
+}
+
DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
pc_q35_2_9_machine_options);
static void pc_q35_2_8_machine_options(MachineClass *m)
{
pc_q35_2_9_machine_options(m);
- m->alias = NULL;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
}
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index f278b3a..7546d01 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -373,6 +373,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
int e820_get_num_entries(void);
bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
+#define PC_COMPAT_2_9 \
+ HW_COMPAT_2_9 \
+
#define PC_COMPAT_2_8 \
HW_COMPAT_2_8 \
{\
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 02/12] memory: tune last param of iommu_ops.translate()
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 01/12] pc: add 2.10 machine type Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 03/12] memory: remove the last param in memory_region_iommu_replay() Peter Xu
` (10 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang, Paolo Bonzini
This patch converts the old "is_write" bool into IOMMUAccessFlags. The
difference is that "is_write" can only express either read/write, but
sometimes what we really want is "none" here (neither read nor write).
Replay is an good example - during replay, we should not check any RW
permission bits since thats not an actual IO at all.
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
exec.c | 6 ++++--
hw/alpha/typhoon.c | 2 +-
hw/dma/rc4030.c | 2 +-
hw/i386/amd_iommu.c | 4 ++--
hw/i386/intel_iommu.c | 4 ++--
hw/pci-host/apb.c | 2 +-
hw/ppc/spapr_iommu.c | 2 +-
hw/s390x/s390-pci-bus.c | 2 +-
hw/s390x/s390-pci-inst.c | 2 +-
include/exec/memory.h | 10 ++++++++--
memory.c | 3 ++-
11 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/exec.c b/exec.c
index eac6085..072de5d 100644
--- a/exec.c
+++ b/exec.c
@@ -481,7 +481,8 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
break;
}
- iotlb = mr->iommu_ops->translate(mr, addr, is_write);
+ iotlb = mr->iommu_ops->translate(mr, addr, is_write ?
+ IOMMU_WO : IOMMU_RO);
if (!(iotlb.perm & (1 << is_write))) {
iotlb.target_as = NULL;
break;
@@ -513,7 +514,8 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
break;
}
- iotlb = mr->iommu_ops->translate(mr, addr, is_write);
+ iotlb = mr->iommu_ops->translate(mr, addr, is_write ?
+ IOMMU_WO : IOMMU_RO);
addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
| (addr & iotlb.addr_mask));
*plen = MIN(*plen, (addr | iotlb.addr_mask) - addr + 1);
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index f50f5cf..c1cf780 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -664,7 +664,7 @@ static bool window_translate(TyphoonWindow *win, hwaddr addr,
/* TODO: A translation failure here ought to set PCI error codes on the
Pchip and generate a machine check interrupt. */
static IOMMUTLBEntry typhoon_translate_iommu(MemoryRegion *iommu, hwaddr addr,
- bool is_write)
+ IOMMUAccessFlags flag)
{
TyphoonPchip *pchip = container_of(iommu, TyphoonPchip, iommu);
IOMMUTLBEntry ret;
diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index 0080141..edf9432 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -489,7 +489,7 @@ static const MemoryRegionOps jazzio_ops = {
};
static IOMMUTLBEntry rc4030_dma_translate(MemoryRegion *iommu, hwaddr addr,
- bool is_write)
+ IOMMUAccessFlags flag)
{
rc4030State *s = container_of(iommu, rc4030State, dma_mr);
IOMMUTLBEntry ret = {
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index f86a40a..42b34ef 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -987,7 +987,7 @@ static inline bool amdvi_is_interrupt_addr(hwaddr addr)
}
static IOMMUTLBEntry amdvi_translate(MemoryRegion *iommu, hwaddr addr,
- bool is_write)
+ IOMMUAccessFlags flag)
{
AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
AMDVIState *s = as->iommu_state;
@@ -1016,7 +1016,7 @@ static IOMMUTLBEntry amdvi_translate(MemoryRegion *iommu, hwaddr addr,
return ret;
}
- amdvi_do_translate(as, addr, is_write, &ret);
+ amdvi_do_translate(as, addr, flag & IOMMU_WO, &ret);
trace_amdvi_translation_result(as->bus_num, PCI_SLOT(as->devfn),
PCI_FUNC(as->devfn), addr, ret.translated_addr);
return ret;
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 02f047c..ea54ec3 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2221,7 +2221,7 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
}
static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr,
- bool is_write)
+ IOMMUAccessFlags flag)
{
VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
IntelIOMMUState *s = vtd_as->iommu_state;
@@ -2243,7 +2243,7 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr,
}
vtd_do_iommu_translate(vtd_as, vtd_as->bus, vtd_as->devfn, addr,
- is_write, &ret);
+ flag & IOMMU_WO, &ret);
VTD_DPRINTF(MMU,
"bus %"PRIu8 " slot %"PRIu8 " func %"PRIu8 " devfn %"PRIu8
" iova 0x%"PRIx64 " hpa 0x%"PRIx64, pci_bus_num(vtd_as->bus),
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 653e711..ad7abb2 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -209,7 +209,7 @@ static AddressSpace *pbm_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
/* Called from RCU critical section */
static IOMMUTLBEntry pbm_translate_iommu(MemoryRegion *iommu, hwaddr addr,
- bool is_write)
+ IOMMUAccessFlags flag)
{
IOMMUState *is = container_of(iommu, IOMMUState, iommu);
hwaddr baseaddr, offset;
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 29c80bb..0341bc0 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -111,7 +111,7 @@ static void spapr_tce_free_table(uint64_t *table, int fd, uint32_t nb_table)
/* Called from RCU critical section */
static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
- bool is_write)
+ IOMMUAccessFlags flag)
{
sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
uint64_t tce;
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index a8a1bab..6bdc795 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -357,7 +357,7 @@ out:
}
static IOMMUTLBEntry s390_translate_iommu(MemoryRegion *mr, hwaddr addr,
- bool is_write)
+ IOMMUAccessFlags flag)
{
uint64_t pte;
uint32_t flags;
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 314a9cb..8bc7c98 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -624,7 +624,7 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
mr = &iommu->iommu_mr;
while (start < end) {
- entry = mr->iommu_ops->translate(mr, start, 0);
+ entry = mr->iommu_ops->translate(mr, start, IOMMU_NONE);
if (!entry.translated_addr) {
pbdev->state = ZPCI_FS_ERROR;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 99e0f54..97fd0c2 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -185,8 +185,14 @@ struct MemoryRegionOps {
typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMUOps;
struct MemoryRegionIOMMUOps {
- /* Return a TLB entry that contains a given address. */
- IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write);
+ /*
+ * Return a TLB entry that contains a given address. Flag should
+ * be the access permission of this translation operation. We can
+ * set flag to IOMMU_NONE to mean that we don't need any
+ * read/write permission checks, like, when for region replay.
+ */
+ IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr,
+ IOMMUAccessFlags flag);
/* Returns minimum supported page size */
uint64_t (*get_min_page_size)(MemoryRegion *iommu);
/* Called when IOMMU Notifier flag changed */
diff --git a/memory.c b/memory.c
index b727f5e..3f0aae8 100644
--- a/memory.c
+++ b/memory.c
@@ -1625,6 +1625,7 @@ void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
{
hwaddr addr, granularity;
IOMMUTLBEntry iotlb;
+ IOMMUAccessFlags flag = is_write ? IOMMU_WO : IOMMU_RO;
/* If the IOMMU has its own replay callback, override */
if (mr->iommu_ops->replay) {
@@ -1635,7 +1636,7 @@ void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
granularity = memory_region_iommu_get_min_page_size(mr);
for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
- iotlb = mr->iommu_ops->translate(mr, addr, is_write);
+ iotlb = mr->iommu_ops->translate(mr, addr, flag);
if (iotlb.perm != IOMMU_NONE) {
n->notify(n, &iotlb);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 03/12] memory: remove the last param in memory_region_iommu_replay()
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 01/12] pc: add 2.10 machine type Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 02/12] memory: tune last param of iommu_ops.translate() Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry() Peter Xu
` (9 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang, Paolo Bonzini
We were always passing in that one as "false" to assume that's an read
operation, and we also assume that IOMMU translation would always have
that read permission. A better permission would be IOMMU_NONE since the
replay is after all not a real read operation, but just a page table
rebuilding process.
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/vfio/common.c | 2 +-
include/exec/memory.h | 5 +----
memory.c | 8 +++-----
3 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index a8f12ee..b9abe77 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -502,7 +502,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
- memory_region_iommu_replay(giommu->iommu, &giommu->n, false);
+ memory_region_iommu_replay(giommu->iommu, &giommu->n);
return;
}
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 97fd0c2..bfdc685 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -731,11 +731,8 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr,
*
* @mr: the memory region to observe
* @n: the notifier to which to replay iommu mappings
- * @is_write: Whether to treat the replay as a translate "write"
- * through the iommu
*/
-void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
- bool is_write);
+void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n);
/**
* memory_region_iommu_replay_all: replay existing IOMMU translations
diff --git a/memory.c b/memory.c
index 3f0aae8..0ddc4cc 100644
--- a/memory.c
+++ b/memory.c
@@ -1620,12 +1620,10 @@ uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr)
return TARGET_PAGE_SIZE;
}
-void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
- bool is_write)
+void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n)
{
hwaddr addr, granularity;
IOMMUTLBEntry iotlb;
- IOMMUAccessFlags flag = is_write ? IOMMU_WO : IOMMU_RO;
/* If the IOMMU has its own replay callback, override */
if (mr->iommu_ops->replay) {
@@ -1636,7 +1634,7 @@ void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
granularity = memory_region_iommu_get_min_page_size(mr);
for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
- iotlb = mr->iommu_ops->translate(mr, addr, flag);
+ iotlb = mr->iommu_ops->translate(mr, addr, IOMMU_NONE);
if (iotlb.perm != IOMMU_NONE) {
n->notify(n, &iotlb);
}
@@ -1654,7 +1652,7 @@ void memory_region_iommu_replay_all(MemoryRegion *mr)
IOMMUNotifier *notifier;
IOMMU_NOTIFIER_FOREACH(notifier, mr) {
- memory_region_iommu_replay(mr, notifier, false);
+ memory_region_iommu_replay(mr, notifier);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry()
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (2 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 03/12] memory: remove the last param in memory_region_iommu_replay() Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-11 1:56 ` David Gibson
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 05/12] x86-iommu: use DeviceClass properties Peter Xu
` (8 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang, Paolo Bonzini
This function has an assumption that we will definitely call translate()
once (or say, the addr will be located inside one IOMMU memory region),
otherwise an empty IOTLB will be returned. Nevertheless, this is not
what we want. When there is no IOMMU memory region, we should build up a
static mapping for the caller, instead of an invalid IOTLB.
We won't trigger this path before VT-d passthrough mode. When
passthrough mode for a vhost device is setup, VT-d is possible to
disable the IOMMU region for that device. Without current patch, we'll
get a vhost boot failure, and it'll be failed over to virtio userspace
mode.
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
exec.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/exec.c b/exec.c
index 072de5d..5cfdacd 100644
--- a/exec.c
+++ b/exec.c
@@ -463,12 +463,13 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
}
/* Called from RCU critical section */
-IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
+IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr iova,
bool is_write)
{
IOMMUTLBEntry iotlb = {0};
MemoryRegionSection *section;
MemoryRegion *mr;
+ hwaddr addr = iova, psize;
for (;;) {
AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
@@ -478,6 +479,23 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
mr = section->mr;
if (!mr->iommu_ops) {
+ /*
+ * We didn't translate() but reached here. It possibly
+ * means it's a static mapping. If so (it should be RAM),
+ * we set the IOTLB up.
+ */
+ if (!iotlb.target_as && memory_region_is_ram(mr) &&
+ !memory_region_is_rom(mr)) {
+ psize = mr->ram_block->page_size;
+ iova &= ~(psize - 1);
+ iotlb = (IOMMUTLBEntry) {
+ .target_as = &address_space_memory,
+ .iova = iova,
+ .translated_addr = iova,
+ .addr_mask = psize - 1,
+ .perm = IOMMU_RW,
+ };
+ }
break;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 05/12] x86-iommu: use DeviceClass properties
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (3 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry() Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 06/12] intel_iommu: renaming context entry helpers Peter Xu
` (7 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
No reason to keep tens of lines if we can do it actually far shorter.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/i386/x86-iommu.c | 47 +++++++----------------------------------------
1 file changed, 7 insertions(+), 40 deletions(-)
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 23dcd3f..02b8825 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -88,55 +88,22 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
x86_iommu_set_default(X86_IOMMU_DEVICE(dev));
}
+static Property x86_iommu_properties[] = {
+ DEFINE_PROP_BOOL("intremap", X86IOMMUState, intr_supported, false),
+ DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void x86_iommu_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = x86_iommu_realize;
-}
-
-static bool x86_iommu_intremap_prop_get(Object *o, Error **errp)
-{
- X86IOMMUState *s = X86_IOMMU_DEVICE(o);
- return s->intr_supported;
-}
-
-static void x86_iommu_intremap_prop_set(Object *o, bool value, Error **errp)
-{
- X86IOMMUState *s = X86_IOMMU_DEVICE(o);
- s->intr_supported = value;
-}
-
-static bool x86_iommu_device_iotlb_prop_get(Object *o, Error **errp)
-{
- X86IOMMUState *s = X86_IOMMU_DEVICE(o);
- return s->dt_supported;
-}
-
-static void x86_iommu_device_iotlb_prop_set(Object *o, bool value, Error **errp)
-{
- X86IOMMUState *s = X86_IOMMU_DEVICE(o);
- s->dt_supported = value;
-}
-
-static void x86_iommu_instance_init(Object *o)
-{
- X86IOMMUState *s = X86_IOMMU_DEVICE(o);
-
- /* By default, do not support IR */
- s->intr_supported = false;
- object_property_add_bool(o, "intremap", x86_iommu_intremap_prop_get,
- x86_iommu_intremap_prop_set, NULL);
- s->dt_supported = false;
- object_property_add_bool(o, "device-iotlb",
- x86_iommu_device_iotlb_prop_get,
- x86_iommu_device_iotlb_prop_set,
- NULL);
+ dc->props = x86_iommu_properties;
}
static const TypeInfo x86_iommu_info = {
.name = TYPE_X86_IOMMU_DEVICE,
.parent = TYPE_SYS_BUS_DEVICE,
- .instance_init = x86_iommu_instance_init,
.instance_size = sizeof(X86IOMMUState),
.class_init = x86_iommu_class_init,
.class_size = sizeof(X86IOMMUClass),
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 06/12] intel_iommu: renaming context entry helpers
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (4 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 05/12] x86-iommu: use DeviceClass properties Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 07/12] intel_iommu: provide vtd_ce_get_type() Peter Xu
` (6 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
The old names are too long and less ordered. Let's start to use
vtd_ce_*() as a pattern.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/i386/intel_iommu.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index ea54ec3..4628f04 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -512,7 +512,7 @@ static int vtd_get_root_entry(IntelIOMMUState *s, uint8_t index,
return 0;
}
-static inline bool vtd_context_entry_present(VTDContextEntry *context)
+static inline bool vtd_ce_present(VTDContextEntry *context)
{
return context->lo & VTD_CONTEXT_ENTRY_P;
}
@@ -533,7 +533,7 @@ static int vtd_get_context_entry_from_root(VTDRootEntry *root, uint8_t index,
return 0;
}
-static inline dma_addr_t vtd_get_slpt_base_from_context(VTDContextEntry *ce)
+static inline dma_addr_t vtd_ce_get_slpt_base(VTDContextEntry *ce)
{
return ce->lo & VTD_CONTEXT_ENTRY_SLPTPTR;
}
@@ -585,19 +585,19 @@ static inline bool vtd_is_level_supported(IntelIOMMUState *s, uint32_t level)
/* Get the page-table level that hardware should use for the second-level
* page-table walk from the Address Width field of context-entry.
*/
-static inline uint32_t vtd_get_level_from_context_entry(VTDContextEntry *ce)
+static inline uint32_t vtd_ce_get_level(VTDContextEntry *ce)
{
return 2 + (ce->hi & VTD_CONTEXT_ENTRY_AW);
}
-static inline uint32_t vtd_get_agaw_from_context_entry(VTDContextEntry *ce)
+static inline uint32_t vtd_ce_get_agaw(VTDContextEntry *ce)
{
return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9;
}
static inline uint64_t vtd_iova_limit(VTDContextEntry *ce)
{
- uint32_t ce_agaw = vtd_get_agaw_from_context_entry(ce);
+ uint32_t ce_agaw = vtd_ce_get_agaw(ce);
return 1ULL << MIN(ce_agaw, VTD_MGAW);
}
@@ -642,8 +642,8 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write,
uint64_t *slptep, uint32_t *slpte_level,
bool *reads, bool *writes)
{
- dma_addr_t addr = vtd_get_slpt_base_from_context(ce);
- uint32_t level = vtd_get_level_from_context_entry(ce);
+ dma_addr_t addr = vtd_ce_get_slpt_base(ce);
+ uint32_t level = vtd_ce_get_level(ce);
uint32_t offset;
uint64_t slpte;
uint64_t access_right_check;
@@ -664,7 +664,7 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write,
VTD_DPRINTF(GENERAL, "error: fail to access second-level paging "
"entry at level %"PRIu32 " for iova 0x%"PRIx64,
level, iova);
- if (level == vtd_get_level_from_context_entry(ce)) {
+ if (level == vtd_ce_get_level(ce)) {
/* Invalid programming of context-entry */
return -VTD_FR_CONTEXT_ENTRY_INV;
} else {
@@ -809,8 +809,8 @@ static int vtd_page_walk(VTDContextEntry *ce, uint64_t start, uint64_t end,
vtd_page_walk_hook hook_fn, void *private,
bool notify_unmap)
{
- dma_addr_t addr = vtd_get_slpt_base_from_context(ce);
- uint32_t level = vtd_get_level_from_context_entry(ce);
+ dma_addr_t addr = vtd_ce_get_slpt_base(ce);
+ uint32_t level = vtd_ce_get_level(ce);
if (!vtd_iova_range_check(start, ce)) {
return -VTD_FR_ADDR_BEYOND_MGAW;
@@ -851,7 +851,7 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
return ret_fr;
}
- if (!vtd_context_entry_present(ce)) {
+ if (!vtd_ce_present(ce)) {
/* Not error - it's okay we don't have context entry. */
trace_vtd_ce_not_present(bus_num, devfn);
return -VTD_FR_CONTEXT_ENTRY_P;
@@ -861,7 +861,7 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
return -VTD_FR_CONTEXT_ENTRY_RSVD;
}
/* Check if the programming of context-entry is valid */
- if (!vtd_is_level_supported(s, vtd_get_level_from_context_entry(ce))) {
+ if (!vtd_is_level_supported(s, vtd_ce_get_level(ce))) {
trace_vtd_ce_invalid(ce->hi, ce->lo);
return -VTD_FR_CONTEXT_ENTRY_INV;
} else {
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 07/12] intel_iommu: provide vtd_ce_get_type()
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (5 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 06/12] intel_iommu: renaming context entry helpers Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 08/12] intel_iommu: use IOMMU_ACCESS_FLAG() Peter Xu
` (5 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
Helper to fetch VT-d context entry type.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/i386/intel_iommu.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4628f04..c8751ba 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -595,6 +595,11 @@ static inline uint32_t vtd_ce_get_agaw(VTDContextEntry *ce)
return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9;
}
+static inline uint32_t vtd_ce_get_type(VTDContextEntry *ce)
+{
+ return ce->lo & VTD_CONTEXT_ENTRY_TT;
+}
+
static inline uint64_t vtd_iova_limit(VTDContextEntry *ce)
{
uint32_t ce_agaw = vtd_ce_get_agaw(ce);
@@ -865,7 +870,7 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
trace_vtd_ce_invalid(ce->hi, ce->lo);
return -VTD_FR_CONTEXT_ENTRY_INV;
} else {
- switch (ce->lo & VTD_CONTEXT_ENTRY_TT) {
+ switch (vtd_ce_get_type(ce)) {
case VTD_CONTEXT_TT_MULTI_LEVEL:
/* fall through */
case VTD_CONTEXT_TT_DEV_IOTLB:
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 08/12] intel_iommu: use IOMMU_ACCESS_FLAG()
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (6 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 07/12] intel_iommu: provide vtd_ce_get_type() Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 09/12] intel_iommu: allow dev-iotlb context entry conditionally Peter Xu
` (4 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
We have that now, so why not use it.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/i386/intel_iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c8751ba..05ae631 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1010,7 +1010,7 @@ out:
entry->iova = addr & page_mask;
entry->translated_addr = vtd_get_slpte_addr(slpte) & page_mask;
entry->addr_mask = ~page_mask;
- entry->perm = (writes ? 2 : 0) + (reads ? 1 : 0);
+ entry->perm = IOMMU_ACCESS_FLAG(reads, writes);
}
static void vtd_root_table_setup(IntelIOMMUState *s)
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 09/12] intel_iommu: allow dev-iotlb context entry conditionally
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (7 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 08/12] intel_iommu: use IOMMU_ACCESS_FLAG() Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-11 2:56 ` Jason Wang
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 10/12] intel_iommu: support passthrough (PT) Peter Xu
` (3 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
When device-iotlb is not specified, we should fail this check.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/i386/intel_iommu.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 05ae631..1a7eba2 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -836,6 +836,8 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
{
VTDRootEntry re;
int ret_fr;
+ bool type_fail = false;
+ X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
ret_fr = vtd_get_root_entry(s, bus_num, &re);
if (ret_fr) {
@@ -872,10 +874,19 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
} else {
switch (vtd_ce_get_type(ce)) {
case VTD_CONTEXT_TT_MULTI_LEVEL:
- /* fall through */
+ /* Always supported */
+ break;
case VTD_CONTEXT_TT_DEV_IOTLB:
+ if (!x86_iommu->dt_supported) {
+ type_fail = true;
+ }
break;
default:
+ /* Unknwon type */
+ type_fail = true;
+ break;
+ }
+ if (type_fail) {
trace_vtd_ce_invalid(ce->hi, ce->lo);
return -VTD_FR_CONTEXT_ENTRY_INV;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 10/12] intel_iommu: support passthrough (PT)
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (8 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 09/12] intel_iommu: allow dev-iotlb context entry conditionally Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-11 8:31 ` Jason Wang
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 11/12] intel_iommu: turn off pt before 2.9 Peter Xu
` (2 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
Hardware support for VT-d device passthrough. Although current Linux can
live with iommu=pt even without this, but this is faster than when using
software passthrough.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/i386/intel_iommu.c | 210 ++++++++++++++++++++++++++++++++---------
hw/i386/intel_iommu_internal.h | 1 +
hw/i386/trace-events | 2 +
hw/i386/x86-iommu.c | 1 +
include/hw/i386/x86-iommu.h | 1 +
5 files changed, 171 insertions(+), 44 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 1a7eba2..1d034f9 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -640,6 +640,29 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
}
}
+/* Find the VTD address space associated with a given bus number */
+static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
+{
+ VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
+ if (!vtd_bus) {
+ /*
+ * Iterate over the registered buses to find the one which
+ * currently hold this bus number, and update the bus_num
+ * lookup table:
+ */
+ GHashTableIter iter;
+
+ g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
+ while (g_hash_table_iter_next (&iter, NULL, (void**)&vtd_bus)) {
+ if (pci_bus_num(vtd_bus->bus) == bus_num) {
+ s->vtd_as_by_bus_num[bus_num] = vtd_bus;
+ return vtd_bus;
+ }
+ }
+ }
+ return vtd_bus;
+}
+
/* Given the @iova, get relevant @slptep. @slpte_level will be the last level
* of the translation, can be used for deciding the size of large page.
*/
@@ -881,6 +904,11 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
type_fail = true;
}
break;
+ case VTD_CONTEXT_TT_PASS_THROUGH:
+ if (!x86_iommu->pt_supported) {
+ type_fail = true;
+ }
+ break;
default:
/* Unknwon type */
type_fail = true;
@@ -894,6 +922,84 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
return 0;
}
+/*
+ * Fetch translation type for specific device. Returns <0 if error
+ * happens, otherwise return the shifted type to check against
+ * VTD_CONTEXT_TT_*.
+ */
+static int vtd_dev_get_trans_type(VTDAddressSpace *as)
+{
+ IntelIOMMUState *s;
+ VTDContextEntry ce;
+ int ret;
+
+ s = as->iommu_state;
+
+ ret = vtd_dev_to_context_entry(s, pci_bus_num(as->bus),
+ as->devfn, &ce);
+ if (ret) {
+ return ret;
+ }
+
+ return vtd_ce_get_type(&ce);
+}
+
+static bool vtd_dev_pt_enabled(VTDAddressSpace *as)
+{
+ int ret;
+
+ assert(as);
+
+ ret = vtd_dev_get_trans_type(as);
+ if (ret < 0) {
+ /*
+ * Possibly failed to parse the context entry for some reason
+ * (e.g., during init, or any guest configuration errors on
+ * context entries). We should assume PT not enabled for
+ * safety.
+ */
+ return false;
+ }
+
+ return ret == VTD_CONTEXT_TT_PASS_THROUGH;
+}
+
+/*
+ * When we are during init phase (device realizations, global
+ * enable/disable of translations), we should not detect PT
+ * (passthrough) when switching address spaces. In that cases, we
+ * should set `detect_pt' to false.
+ *
+ * Return whether the device is using IOMMU translation.
+ */
+static bool vtd_switch_address_space(VTDAddressSpace *as, bool detect_pt)
+{
+ bool use_iommu;
+
+ assert(as);
+
+ use_iommu = as->iommu_state->dmar_enabled;
+ if (detect_pt) {
+ use_iommu &= !vtd_dev_pt_enabled(as);
+ }
+
+ trace_vtd_switch_address_space(pci_bus_num(as->bus),
+ VTD_PCI_SLOT(as->devfn),
+ VTD_PCI_FUNC(as->devfn),
+ use_iommu);
+
+ /* Turn off first then on the other */
+ if (use_iommu) {
+ memory_region_set_enabled(&as->sys_alias, false);
+ memory_region_set_enabled(&as->iommu, true);
+ } else {
+ memory_region_set_enabled(&as->iommu, false);
+ memory_region_set_enabled(&as->sys_alias, true);
+ }
+
+ return use_iommu;
+}
+
static inline uint16_t vtd_make_source_id(uint8_t bus_num, uint8_t devfn)
{
return ((bus_num & 0xffUL) << 8) | (devfn & 0xffUL);
@@ -931,6 +1037,31 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr)
return VTD_INTERRUPT_ADDR_FIRST <= addr && addr <= VTD_INTERRUPT_ADDR_LAST;
}
+static void vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id)
+{
+ VTDBus *vtd_bus;
+ VTDAddressSpace *vtd_as;
+ const char *msg = "FAIL";
+
+ vtd_bus = vtd_find_as_from_bus_num(s, VTD_SID_TO_BUS(source_id));
+ if (!vtd_bus) {
+ goto out;
+ }
+
+ vtd_as = vtd_bus->dev_as[VTD_SID_TO_DEVFN(source_id)];
+ if (!vtd_as) {
+ goto out;
+ }
+
+ if (vtd_switch_address_space(vtd_as, true) == false) {
+ /* We switched off IOMMU region successfully. */
+ msg = "SUCCESS";
+ }
+
+out:
+ trace_vtd_pt_enable_fast_path(source_id, msg);
+}
+
/* Map dev to context-entry then do a paging-structures walk to do a iommu
* translation.
*
@@ -1002,6 +1133,30 @@ static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
cc_entry->context_cache_gen = s->context_cache_gen;
}
+ /*
+ * We don't need to translate for pass-through context entries.
+ * Also, let's ignore IOTLB caching as well for PT devices.
+ */
+ if (vtd_ce_get_type(&ce) == VTD_CONTEXT_TT_PASS_THROUGH) {
+ entry->translated_addr = entry->iova;
+ entry->addr_mask = VTD_PAGE_SIZE - 1;
+ entry->perm = IOMMU_RW;
+ trace_vtd_translate_pt(source_id, entry->iova);
+
+ /*
+ * When this happens, it means firstly caching-mode is not
+ * enabled, and this is the first passthrough translation for
+ * the device. Let's enable the fast path for passthrough.
+ *
+ * When passthrough is disabled again for the device, we can
+ * capture it via the context entry invalidation, then the
+ * IOMMU region can be swapped back.
+ */
+ vtd_pt_enable_fast_path(s, source_id);
+
+ return;
+ }
+
ret_fr = vtd_iova_to_slpte(&ce, addr, is_write, &slpte, &level,
&reads, &writes);
if (ret_fr) {
@@ -1081,29 +1236,6 @@ static void vtd_context_global_invalidate(IntelIOMMUState *s)
vtd_iommu_replay_all(s);
}
-
-/* Find the VTD address space currently associated with a given bus number,
- */
-static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
-{
- VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
- if (!vtd_bus) {
- /* Iterate over the registered buses to find the one
- * which currently hold this bus number, and update the bus_num lookup table:
- */
- GHashTableIter iter;
-
- g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
- while (g_hash_table_iter_next (&iter, NULL, (void**)&vtd_bus)) {
- if (pci_bus_num(vtd_bus->bus) == bus_num) {
- s->vtd_as_by_bus_num[bus_num] = vtd_bus;
- return vtd_bus;
- }
- }
- }
- return vtd_bus;
-}
-
/* Do a context-cache device-selective invalidation.
* @func_mask: FM field after shifting
*/
@@ -1146,6 +1278,11 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
VTD_PCI_FUNC(devfn_it));
vtd_as->context_cache_entry.context_cache_gen = 0;
/*
+ * Do switch address space when needed, in case if the
+ * device passthrough bit is switched.
+ */
+ vtd_switch_address_space(vtd_as, true);
+ /*
* So a device is moving out of (or moving into) a
* domain, a replay() suites here to notify all the
* IOMMU_NOTIFIER_MAP registers about this change.
@@ -1377,25 +1514,6 @@ static void vtd_handle_gcmd_sirtp(IntelIOMMUState *s)
vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRTPS);
}
-static void vtd_switch_address_space(VTDAddressSpace *as)
-{
- assert(as);
-
- trace_vtd_switch_address_space(pci_bus_num(as->bus),
- VTD_PCI_SLOT(as->devfn),
- VTD_PCI_FUNC(as->devfn),
- as->iommu_state->dmar_enabled);
-
- /* Turn off first then on the other */
- if (as->iommu_state->dmar_enabled) {
- memory_region_set_enabled(&as->sys_alias, false);
- memory_region_set_enabled(&as->iommu, true);
- } else {
- memory_region_set_enabled(&as->iommu, false);
- memory_region_set_enabled(&as->sys_alias, true);
- }
-}
-
static void vtd_switch_address_space_all(IntelIOMMUState *s)
{
GHashTableIter iter;
@@ -1408,7 +1526,7 @@ static void vtd_switch_address_space_all(IntelIOMMUState *s)
if (!vtd_bus->dev_as[i]) {
continue;
}
- vtd_switch_address_space(vtd_bus->dev_as[i]);
+ vtd_switch_address_space(vtd_bus->dev_as[i], false);
}
}
}
@@ -2712,7 +2830,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
&vtd_dev_as->sys_alias, 1);
memory_region_add_subregion_overlap(&vtd_dev_as->root, 0,
&vtd_dev_as->iommu, 1);
- vtd_switch_address_space(vtd_dev_as);
+ vtd_switch_address_space(vtd_dev_as, false);
}
return vtd_dev_as;
}
@@ -2860,6 +2978,10 @@ static void vtd_init(IntelIOMMUState *s)
s->ecap |= VTD_ECAP_DT;
}
+ if (x86_iommu->pt_supported) {
+ s->ecap |= VTD_ECAP_PT;
+ }
+
if (s->caching_mode) {
s->cap |= VTD_CAP_CM;
}
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 29d6707..0e73a65 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -187,6 +187,7 @@
/* Interrupt Remapping support */
#define VTD_ECAP_IR (1ULL << 3)
#define VTD_ECAP_EIM (1ULL << 4)
+#define VTD_ECAP_PT (1ULL << 6)
#define VTD_ECAP_MHMV (15ULL << 20)
/* CAP_REG */
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index 04a6980..5c3e466 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -38,6 +38,8 @@ vtd_page_walk_skip_perm(uint64_t iova, uint64_t next) "Page walk skip iova 0x%"P
vtd_page_walk_skip_reserve(uint64_t iova, uint64_t next) "Page walk skip iova 0x%"PRIx64" - 0x%"PRIx64" due to rsrv set"
vtd_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, bool on) "Device %02x:%02x.%x switching address space (iommu enabled=%d)"
vtd_as_unmap_whole(uint8_t bus, uint8_t slot, uint8_t fn, uint64_t iova, uint64_t size) "Device %02x:%02x.%x start 0x%"PRIx64" size 0x%"PRIx64
+vtd_translate_pt(uint16_t sid, uint64_t addr) "source id 0x%"PRIu16", iova 0x%"PRIx64
+vtd_pt_enable_fast_path(uint16_t sid, const char *msg) "sid 0x%"PRIu16" %s"
# hw/i386/amd_iommu.c
amdvi_evntlog_fail(uint64_t addr, uint32_t head) "error: fail to write at addr 0x%"PRIx64" + offset 0x%"PRIx32
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 02b8825..293caf8 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -91,6 +91,7 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
static Property x86_iommu_properties[] = {
DEFINE_PROP_BOOL("intremap", X86IOMMUState, intr_supported, false),
DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
+ DEFINE_PROP_BOOL("pt", X86IOMMUState, pt_supported, true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
index 361c07c..ef89c0c 100644
--- a/include/hw/i386/x86-iommu.h
+++ b/include/hw/i386/x86-iommu.h
@@ -74,6 +74,7 @@ struct X86IOMMUState {
SysBusDevice busdev;
bool intr_supported; /* Whether vIOMMU supports IR */
bool dt_supported; /* Whether vIOMMU supports DT */
+ bool pt_supported; /* Whether vIOMMU supports pass-through */
IommuType type; /* IOMMU type - AMD/Intel */
QLIST_HEAD(, IEC_Notifier) iec_notifiers; /* IEC notify list */
};
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 11/12] intel_iommu: turn off pt before 2.9
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (9 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 10/12] intel_iommu: support passthrough (PT) Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 12/12] vhost: iommu: cache static mapping if there is Peter Xu
2017-05-10 8:55 ` [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes no-reply
12 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
This is for compatibility.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/hw/compat.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 846b90e..ff08ec8 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,11 @@
#define HW_COMPAT_H
#define HW_COMPAT_2_9 \
- /* empty */
+ {\
+ .driver = "intel-iommu",\
+ .property = "pt",\
+ .value = "off",\
+ },
#define HW_COMPAT_2_8 \
{\
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v3 12/12] vhost: iommu: cache static mapping if there is
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (10 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 11/12] intel_iommu: turn off pt before 2.9 Peter Xu
@ 2017-05-10 8:01 ` Peter Xu
2017-05-11 8:35 ` Jason Wang
2017-05-10 8:55 ` [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes no-reply
12 siblings, 1 reply; 25+ messages in thread
From: Peter Xu @ 2017-05-10 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, peterx, Jason Wang
This patch pre-heat vhost iotlb cache when passthrough mode enabled.
Sometimes, even if user specified iommu_platform for vhost devices,
IOMMU might still be disabled. One case is passthrough mode in VT-d
implementation. We can detect this by observing iommu_list. If it's
empty, it means IOMMU translation is disabled, then we can actually
pre-heat the translation (it'll be static mapping then) by first
invalidating all IOTLB, then cache existing memory ranges into vhost
backend iotlb using 1:1 mapping.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/virtio/trace-events | 4 ++++
hw/virtio/vhost.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 53 insertions(+)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 1f7a7c1..54dcbb3 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -24,3 +24,7 @@ virtio_balloon_handle_output(const char *name, uint64_t gpa) "section name: %s g
virtio_balloon_get_config(uint32_t num_pages, uint32_t actual) "num_pages: %d actual: %d"
virtio_balloon_set_config(uint32_t actual, uint32_t oldactual) "actual: %d oldactual: %d"
virtio_balloon_to_target(uint64_t target, uint32_t num_pages) "balloon target: %"PRIx64" num_pages: %d"
+
+# hw/virtio/vhost.c
+vhost_iommu_commit(void) ""
+vhost_iommu_static_preheat(void) ""
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 0001e60..1c92e62 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -27,6 +27,7 @@
#include "hw/virtio/virtio-access.h"
#include "migration/migration.h"
#include "sysemu/dma.h"
+#include "trace.h"
/* enabled until disconnected backend stabilizes */
#define _VHOST_DEBUG 1
@@ -730,6 +731,11 @@ static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
}
}
+static bool vhost_iommu_mr_enabled(struct vhost_dev *dev)
+{
+ return !QLIST_EMPTY(&dev->iommu_list);
+}
+
static void vhost_iommu_region_add(MemoryListener *listener,
MemoryRegionSection *section)
{
@@ -782,6 +788,48 @@ static void vhost_iommu_region_del(MemoryListener *listener,
}
}
+static void vhost_iommu_commit(MemoryListener *listener)
+{
+ struct vhost_dev *dev = container_of(listener, struct vhost_dev,
+ iommu_listener);
+ struct vhost_memory_region *r;
+ int i;
+
+ trace_vhost_iommu_commit();
+
+ if (!vhost_iommu_mr_enabled(dev)) {
+ /*
+ * This means iommu_platform is enabled, however iommu memory
+ * region is disabled, e.g., when device passthrough is setup.
+ * Then, no translation is needed any more.
+ *
+ * Let's first invalidate the whole IOTLB, then pre-heat the
+ * static mapping by looping over vhost memory ranges.
+ */
+
+ if (dev->vhost_ops->vhost_invalidate_device_iotlb(dev, 0,
+ UINT64_MAX-1)) {
+ error_report("%s: flush existing IOTLB failed", __func__);
+ return;
+ }
+
+ for (i = 0; i < dev->mem->nregions; i++) {
+ r = &dev->mem->regions[i];
+ /* Vhost regions are writable RAM, so IOMMU_RW suites. */
+ if (dev->vhost_ops->vhost_update_device_iotlb(dev,
+ r->guest_phys_addr,
+ r->userspace_addr,
+ r->memory_size,
+ IOMMU_RW)) {
+ error_report("%s: pre-heat static mapping failed", __func__);
+ return;
+ }
+ }
+
+ trace_vhost_iommu_static_preheat();
+ }
+}
+
static void vhost_region_nop(MemoryListener *listener,
MemoryRegionSection *section)
{
@@ -1298,6 +1346,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
hdev->iommu_listener = (MemoryListener) {
.region_add = vhost_iommu_region_add,
.region_del = vhost_iommu_region_del,
+ .commit = vhost_iommu_commit,
};
if (hdev->migration_blocker == NULL) {
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
` (11 preceding siblings ...)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 12/12] vhost: iommu: cache static mapping if there is Peter Xu
@ 2017-05-10 8:55 ` no-reply
12 siblings, 0 replies; 25+ messages in thread
From: no-reply @ 2017-05-10 8:55 UTC (permalink / raw)
To: peterx
Cc: famz, qemu-devel, tianyu.lan, yi.l.liu, mst, jasowang, marcel, david
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 1494403315-12760-1-git-send-email-peterx@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1494405683-20877-1-git-send-email-pradeep.jagadeesh@huawei.com -> patchew/1494405683-20877-1-git-send-email-pradeep.jagadeesh@huawei.com
* [new tag] patchew/20170510083259.3900-1-xiaoguangrong@tencent.com -> patchew/20170510083259.3900-1-xiaoguangrong@tencent.com
Switched to a new branch 'test'
9d93e4b vhost: iommu: cache static mapping if there is
0b1f60a intel_iommu: turn off pt before 2.9
d3b8897 intel_iommu: support passthrough (PT)
d6288b6 intel_iommu: allow dev-iotlb context entry conditionally
cde41e2 intel_iommu: use IOMMU_ACCESS_FLAG()
c0e7f1a intel_iommu: provide vtd_ce_get_type()
cf9559d intel_iommu: renaming context entry helpers
4177ae0 x86-iommu: use DeviceClass properties
fd8a285 memory: fix address_space_get_iotlb_entry()
ca5bda6 memory: remove the last param in memory_region_iommu_replay()
03ec6e5 memory: tune last param of iommu_ops.translate()
b663efd pc: add 2.10 machine type
=== OUTPUT BEGIN ===
Checking PATCH 1/12: pc: add 2.10 machine type...
Checking PATCH 2/12: memory: tune last param of iommu_ops.translate()...
Checking PATCH 3/12: memory: remove the last param in memory_region_iommu_replay()...
Checking PATCH 4/12: memory: fix address_space_get_iotlb_entry()...
Checking PATCH 5/12: x86-iommu: use DeviceClass properties...
Checking PATCH 6/12: intel_iommu: renaming context entry helpers...
Checking PATCH 7/12: intel_iommu: provide vtd_ce_get_type()...
Checking PATCH 8/12: intel_iommu: use IOMMU_ACCESS_FLAG()...
Checking PATCH 9/12: intel_iommu: allow dev-iotlb context entry conditionally...
Checking PATCH 10/12: intel_iommu: support passthrough (PT)...
ERROR: "(foo**)" should be "(foo **)"
#34: FILE: hw/i386/intel_iommu.c:656:
+ while (g_hash_table_iter_next (&iter, NULL, (void**)&vtd_bus)) {
ERROR: space prohibited between function name and open parenthesis '('
#34: FILE: hw/i386/intel_iommu.c:656:
+ while (g_hash_table_iter_next (&iter, NULL, (void**)&vtd_bus)) {
total: 2 errors, 0 warnings, 305 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/12: intel_iommu: turn off pt before 2.9...
Checking PATCH 12/12: vhost: iommu: cache static mapping if there is...
ERROR: spaces required around that '-' (ctx:VxV)
#79: FILE: hw/virtio/vhost.c:811:
+ UINT64_MAX-1)) {
^
total: 1 errors, 0 warnings, 80 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry()
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry() Peter Xu
@ 2017-05-11 1:56 ` David Gibson
2017-05-11 9:36 ` Peter Xu
0 siblings, 1 reply; 25+ messages in thread
From: David Gibson @ 2017-05-11 1:56 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, Jason Wang, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 3321 bytes --]
On Wed, May 10, 2017 at 04:01:47PM +0800, Peter Xu wrote:
> This function has an assumption that we will definitely call translate()
> once (or say, the addr will be located inside one IOMMU memory region),
> otherwise an empty IOTLB will be returned. Nevertheless, this is not
> what we want. When there is no IOMMU memory region, we should build up a
> static mapping for the caller, instead of an invalid IOTLB.
>
> We won't trigger this path before VT-d passthrough mode. When
> passthrough mode for a vhost device is setup, VT-d is possible to
> disable the IOMMU region for that device. Without current patch, we'll
> get a vhost boot failure, and it'll be failed over to virtio userspace
> mode.
This doesn't look right to me. You're assuming the target is
address_space_memory, which might not be the case - and you should be
able to check from the MR you do hit. Furthermore it doesn't look
like you're accounting for the trivial translation if the section's
offset in the address space is different from its offset in the MR.
I think for the fallback path you're going to want something based on
address_space_translate() instead.
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> exec.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/exec.c b/exec.c
> index 072de5d..5cfdacd 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -463,12 +463,13 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
> }
>
> /* Called from RCU critical section */
> -IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> +IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr iova,
> bool is_write)
> {
> IOMMUTLBEntry iotlb = {0};
> MemoryRegionSection *section;
> MemoryRegion *mr;
> + hwaddr addr = iova, psize;
>
> for (;;) {
> AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
> @@ -478,6 +479,23 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> mr = section->mr;
>
> if (!mr->iommu_ops) {
> + /*
> + * We didn't translate() but reached here. It possibly
> + * means it's a static mapping. If so (it should be RAM),
> + * we set the IOTLB up.
> + */
> + if (!iotlb.target_as && memory_region_is_ram(mr) &&
> + !memory_region_is_rom(mr)) {
> + psize = mr->ram_block->page_size;
> + iova &= ~(psize - 1);
> + iotlb = (IOMMUTLBEntry) {
> + .target_as = &address_space_memory,
> + .iova = iova,
> + .translated_addr = iova,
> + .addr_mask = psize - 1,
> + .perm = IOMMU_RW,
> + };
> + }
> break;
> }
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 09/12] intel_iommu: allow dev-iotlb context entry conditionally
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 09/12] intel_iommu: allow dev-iotlb context entry conditionally Peter Xu
@ 2017-05-11 2:56 ` Jason Wang
2017-05-11 3:51 ` Peter Xu
0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2017-05-11 2:56 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Lan Tianyu, yi.l.liu, Michael S . Tsirkin, Marcel Apfelbaum,
David Gibson
On 2017年05月10日 16:01, Peter Xu wrote:
> When device-iotlb is not specified, we should fail this check.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> hw/i386/intel_iommu.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 05ae631..1a7eba2 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -836,6 +836,8 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> {
> VTDRootEntry re;
> int ret_fr;
> + bool type_fail = false;
> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>
> ret_fr = vtd_get_root_entry(s, bus_num, &re);
> if (ret_fr) {
> @@ -872,10 +874,19 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> } else {
> switch (vtd_ce_get_type(ce)) {
> case VTD_CONTEXT_TT_MULTI_LEVEL:
> - /* fall through */
> + /* Always supported */
> + break;
> case VTD_CONTEXT_TT_DEV_IOTLB:
> + if (!x86_iommu->dt_supported) {
> + type_fail = true;
> + }
> break;
> default:
> + /* Unknwon type */
> + type_fail = true;
> + break;
> + }
> + if (type_fail) {
> trace_vtd_ce_invalid(ce->hi, ce->lo);
> return -VTD_FR_CONTEXT_ENTRY_INV;
> }
How about e.g exclude the type if not supported in vtd_ce_get_type()?
This looks better than using something like type_fail.
Thanks
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 09/12] intel_iommu: allow dev-iotlb context entry conditionally
2017-05-11 2:56 ` Jason Wang
@ 2017-05-11 3:51 ` Peter Xu
0 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-11 3:51 UTC (permalink / raw)
To: Jason Wang
Cc: qemu-devel, Lan Tianyu, yi.l.liu, Michael S . Tsirkin,
Marcel Apfelbaum, David Gibson
On Thu, May 11, 2017 at 10:56:42AM +0800, Jason Wang wrote:
>
>
> On 2017年05月10日 16:01, Peter Xu wrote:
> >When device-iotlb is not specified, we should fail this check.
> >
> >Signed-off-by: Peter Xu <peterx@redhat.com>
> >---
> > hw/i386/intel_iommu.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >index 05ae631..1a7eba2 100644
> >--- a/hw/i386/intel_iommu.c
> >+++ b/hw/i386/intel_iommu.c
> >@@ -836,6 +836,8 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> > {
> > VTDRootEntry re;
> > int ret_fr;
> >+ bool type_fail = false;
> >+ X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
> > ret_fr = vtd_get_root_entry(s, bus_num, &re);
> > if (ret_fr) {
> >@@ -872,10 +874,19 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> > } else {
> > switch (vtd_ce_get_type(ce)) {
> > case VTD_CONTEXT_TT_MULTI_LEVEL:
> >- /* fall through */
> >+ /* Always supported */
> >+ break;
> > case VTD_CONTEXT_TT_DEV_IOTLB:
> >+ if (!x86_iommu->dt_supported) {
> >+ type_fail = true;
> >+ }
> > break;
> > default:
> >+ /* Unknwon type */
> >+ type_fail = true;
> >+ break;
> >+ }
> >+ if (type_fail) {
> > trace_vtd_ce_invalid(ce->hi, ce->lo);
> > return -VTD_FR_CONTEXT_ENTRY_INV;
> > }
>
> How about e.g exclude the type if not supported in vtd_ce_get_type()? This
> looks better than using something like type_fail.
(after a quick discussion with Jason offlist)
I'll keep current vtd_ce_get_type() since there are other places that
used it, and introduce another vtd_ce_type_check() to make codes more
elegant (and get rid of type_fail variable). Thanks.
--
Peter Xu
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 10/12] intel_iommu: support passthrough (PT)
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 10/12] intel_iommu: support passthrough (PT) Peter Xu
@ 2017-05-11 8:31 ` Jason Wang
2017-05-11 8:48 ` Peter Xu
0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2017-05-11 8:31 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Lan Tianyu, yi.l.liu, Michael S . Tsirkin, Marcel Apfelbaum,
David Gibson
On 2017年05月10日 16:01, Peter Xu wrote:
> Hardware support for VT-d device passthrough. Although current Linux can
> live with iommu=pt even without this, but this is faster than when using
> software passthrough.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> hw/i386/intel_iommu.c | 210 ++++++++++++++++++++++++++++++++---------
> hw/i386/intel_iommu_internal.h | 1 +
> hw/i386/trace-events | 2 +
> hw/i386/x86-iommu.c | 1 +
> include/hw/i386/x86-iommu.h | 1 +
> 5 files changed, 171 insertions(+), 44 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1a7eba2..1d034f9 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -640,6 +640,29 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
> }
> }
>
> +/* Find the VTD address space associated with a given bus number */
> +static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
> +{
> + VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
> + if (!vtd_bus) {
> + /*
> + * Iterate over the registered buses to find the one which
> + * currently hold this bus number, and update the bus_num
> + * lookup table:
> + */
> + GHashTableIter iter;
> +
> + g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
> + while (g_hash_table_iter_next (&iter, NULL, (void**)&vtd_bus)) {
> + if (pci_bus_num(vtd_bus->bus) == bus_num) {
> + s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> + return vtd_bus;
> + }
> + }
> + }
> + return vtd_bus;
> +}
> +
> /* Given the @iova, get relevant @slptep. @slpte_level will be the last level
> * of the translation, can be used for deciding the size of large page.
> */
> @@ -881,6 +904,11 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> type_fail = true;
> }
> break;
> + case VTD_CONTEXT_TT_PASS_THROUGH:
> + if (!x86_iommu->pt_supported) {
> + type_fail = true;
> + }
> + break;
> default:
> /* Unknwon type */
> type_fail = true;
> @@ -894,6 +922,84 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> return 0;
> }
>
> +/*
> + * Fetch translation type for specific device. Returns <0 if error
> + * happens, otherwise return the shifted type to check against
> + * VTD_CONTEXT_TT_*.
> + */
> +static int vtd_dev_get_trans_type(VTDAddressSpace *as)
> +{
> + IntelIOMMUState *s;
> + VTDContextEntry ce;
> + int ret;
> +
> + s = as->iommu_state;
> +
> + ret = vtd_dev_to_context_entry(s, pci_bus_num(as->bus),
> + as->devfn, &ce);
> + if (ret) {
> + return ret;
> + }
> +
> + return vtd_ce_get_type(&ce);
> +}
> +
> +static bool vtd_dev_pt_enabled(VTDAddressSpace *as)
> +{
> + int ret;
> +
> + assert(as);
> +
> + ret = vtd_dev_get_trans_type(as);
> + if (ret < 0) {
> + /*
> + * Possibly failed to parse the context entry for some reason
> + * (e.g., during init, or any guest configuration errors on
> + * context entries). We should assume PT not enabled for
> + * safety.
> + */
> + return false;
> + }
> +
> + return ret == VTD_CONTEXT_TT_PASS_THROUGH;
> +}
> +
> +/*
> + * When we are during init phase (device realizations, global
> + * enable/disable of translations), we should not detect PT
> + * (passthrough) when switching address spaces. In that cases, we
> + * should set `detect_pt' to false.
> + *
> + * Return whether the device is using IOMMU translation.
> + */
> +static bool vtd_switch_address_space(VTDAddressSpace *as, bool detect_pt)
> +{
The detect_pt looks suspicious. E.g if the context entry does not exist,
vtd_dev_pt_enabled() will return false.
> + bool use_iommu;
> +
> + assert(as);
> +
> + use_iommu = as->iommu_state->dmar_enabled;
> + if (detect_pt) {
> + use_iommu &= !vtd_dev_pt_enabled(as);
> + }
> +
> + trace_vtd_switch_address_space(pci_bus_num(as->bus),
> + VTD_PCI_SLOT(as->devfn),
> + VTD_PCI_FUNC(as->devfn),
> + use_iommu);
> +
> + /* Turn off first then on the other */
> + if (use_iommu) {
> + memory_region_set_enabled(&as->sys_alias, false);
> + memory_region_set_enabled(&as->iommu, true);
> + } else {
> + memory_region_set_enabled(&as->iommu, false);
> + memory_region_set_enabled(&as->sys_alias, true);
> + }
> +
> + return use_iommu;
> +}
> +
> static inline uint16_t vtd_make_source_id(uint8_t bus_num, uint8_t devfn)
> {
> return ((bus_num & 0xffUL) << 8) | (devfn & 0xffUL);
> @@ -931,6 +1037,31 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr)
> return VTD_INTERRUPT_ADDR_FIRST <= addr && addr <= VTD_INTERRUPT_ADDR_LAST;
> }
>
> +static void vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id)
> +{
> + VTDBus *vtd_bus;
> + VTDAddressSpace *vtd_as;
> + const char *msg = "FAIL";
> +
> + vtd_bus = vtd_find_as_from_bus_num(s, VTD_SID_TO_BUS(source_id));
> + if (!vtd_bus) {
> + goto out;
> + }
> +
> + vtd_as = vtd_bus->dev_as[VTD_SID_TO_DEVFN(source_id)];
> + if (!vtd_as) {
> + goto out;
> + }
> +
> + if (vtd_switch_address_space(vtd_as, true) == false) {
> + /* We switched off IOMMU region successfully. */
> + msg = "SUCCESS";
> + }
> +
> +out:
> + trace_vtd_pt_enable_fast_path(source_id, msg);
Looks like using a boolean is better here.
> +}
> +
> /* Map dev to context-entry then do a paging-structures walk to do a iommu
> * translation.
> *
> @@ -1002,6 +1133,30 @@ static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
> cc_entry->context_cache_gen = s->context_cache_gen;
> }
>
> + /*
> + * We don't need to translate for pass-through context entries.
> + * Also, let's ignore IOTLB caching as well for PT devices.
> + */
> + if (vtd_ce_get_type(&ce) == VTD_CONTEXT_TT_PASS_THROUGH) {
> + entry->translated_addr = entry->iova;
> + entry->addr_mask = VTD_PAGE_SIZE - 1;
> + entry->perm = IOMMU_RW;
> + trace_vtd_translate_pt(source_id, entry->iova);
> +
> + /*
> + * When this happens, it means firstly caching-mode is not
> + * enabled, and this is the first passthrough translation for
> + * the device. Let's enable the fast path for passthrough.
> + *
> + * When passthrough is disabled again for the device, we can
> + * capture it via the context entry invalidation, then the
> + * IOMMU region can be swapped back.
> + */
> + vtd_pt_enable_fast_path(s, source_id);
> +
> + return;
> + }
> +
> ret_fr = vtd_iova_to_slpte(&ce, addr, is_write, &slpte, &level,
> &reads, &writes);
> if (ret_fr) {
> @@ -1081,29 +1236,6 @@ static void vtd_context_global_invalidate(IntelIOMMUState *s)
> vtd_iommu_replay_all(s);
> }
>
> -
> -/* Find the VTD address space currently associated with a given bus number,
> - */
> -static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
> -{
> - VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
> - if (!vtd_bus) {
> - /* Iterate over the registered buses to find the one
> - * which currently hold this bus number, and update the bus_num lookup table:
> - */
> - GHashTableIter iter;
> -
> - g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
> - while (g_hash_table_iter_next (&iter, NULL, (void**)&vtd_bus)) {
> - if (pci_bus_num(vtd_bus->bus) == bus_num) {
> - s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> - return vtd_bus;
> - }
> - }
> - }
> - return vtd_bus;
> -}
> -
> /* Do a context-cache device-selective invalidation.
> * @func_mask: FM field after shifting
> */
> @@ -1146,6 +1278,11 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
> VTD_PCI_FUNC(devfn_it));
> vtd_as->context_cache_entry.context_cache_gen = 0;
> /*
> + * Do switch address space when needed, in case if the
> + * device passthrough bit is switched.
> + */
> + vtd_switch_address_space(vtd_as, true);
Do we need to do this also in DSI and GLOBAL invalidation?
Thanks
> + /*
> * So a device is moving out of (or moving into) a
> * domain, a replay() suites here to notify all the
> * IOMMU_NOTIFIER_MAP registers about this change.
> @@ -1377,25 +1514,6 @@ static void vtd_handle_gcmd_sirtp(IntelIOMMUState *s)
> vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRTPS);
> }
>
> -static void vtd_switch_address_space(VTDAddressSpace *as)
> -{
> - assert(as);
> -
> - trace_vtd_switch_address_space(pci_bus_num(as->bus),
> - VTD_PCI_SLOT(as->devfn),
> - VTD_PCI_FUNC(as->devfn),
> - as->iommu_state->dmar_enabled);
> -
> - /* Turn off first then on the other */
> - if (as->iommu_state->dmar_enabled) {
> - memory_region_set_enabled(&as->sys_alias, false);
> - memory_region_set_enabled(&as->iommu, true);
> - } else {
> - memory_region_set_enabled(&as->iommu, false);
> - memory_region_set_enabled(&as->sys_alias, true);
> - }
> -}
> -
> static void vtd_switch_address_space_all(IntelIOMMUState *s)
> {
> GHashTableIter iter;
> @@ -1408,7 +1526,7 @@ static void vtd_switch_address_space_all(IntelIOMMUState *s)
> if (!vtd_bus->dev_as[i]) {
> continue;
> }
> - vtd_switch_address_space(vtd_bus->dev_as[i]);
> + vtd_switch_address_space(vtd_bus->dev_as[i], false);
> }
> }
> }
> @@ -2712,7 +2830,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
> &vtd_dev_as->sys_alias, 1);
> memory_region_add_subregion_overlap(&vtd_dev_as->root, 0,
> &vtd_dev_as->iommu, 1);
> - vtd_switch_address_space(vtd_dev_as);
> + vtd_switch_address_space(vtd_dev_as, false);
> }
> return vtd_dev_as;
> }
> @@ -2860,6 +2978,10 @@ static void vtd_init(IntelIOMMUState *s)
> s->ecap |= VTD_ECAP_DT;
> }
>
> + if (x86_iommu->pt_supported) {
> + s->ecap |= VTD_ECAP_PT;
> + }
> +
> if (s->caching_mode) {
> s->cap |= VTD_CAP_CM;
> }
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 29d6707..0e73a65 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -187,6 +187,7 @@
> /* Interrupt Remapping support */
> #define VTD_ECAP_IR (1ULL << 3)
> #define VTD_ECAP_EIM (1ULL << 4)
> +#define VTD_ECAP_PT (1ULL << 6)
> #define VTD_ECAP_MHMV (15ULL << 20)
>
> /* CAP_REG */
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index 04a6980..5c3e466 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -38,6 +38,8 @@ vtd_page_walk_skip_perm(uint64_t iova, uint64_t next) "Page walk skip iova 0x%"P
> vtd_page_walk_skip_reserve(uint64_t iova, uint64_t next) "Page walk skip iova 0x%"PRIx64" - 0x%"PRIx64" due to rsrv set"
> vtd_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, bool on) "Device %02x:%02x.%x switching address space (iommu enabled=%d)"
> vtd_as_unmap_whole(uint8_t bus, uint8_t slot, uint8_t fn, uint64_t iova, uint64_t size) "Device %02x:%02x.%x start 0x%"PRIx64" size 0x%"PRIx64
> +vtd_translate_pt(uint16_t sid, uint64_t addr) "source id 0x%"PRIu16", iova 0x%"PRIx64
> +vtd_pt_enable_fast_path(uint16_t sid, const char *msg) "sid 0x%"PRIu16" %s"
>
> # hw/i386/amd_iommu.c
> amdvi_evntlog_fail(uint64_t addr, uint32_t head) "error: fail to write at addr 0x%"PRIx64" + offset 0x%"PRIx32
> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
> index 02b8825..293caf8 100644
> --- a/hw/i386/x86-iommu.c
> +++ b/hw/i386/x86-iommu.c
> @@ -91,6 +91,7 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
> static Property x86_iommu_properties[] = {
> DEFINE_PROP_BOOL("intremap", X86IOMMUState, intr_supported, false),
> DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
> + DEFINE_PROP_BOOL("pt", X86IOMMUState, pt_supported, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
> index 361c07c..ef89c0c 100644
> --- a/include/hw/i386/x86-iommu.h
> +++ b/include/hw/i386/x86-iommu.h
> @@ -74,6 +74,7 @@ struct X86IOMMUState {
> SysBusDevice busdev;
> bool intr_supported; /* Whether vIOMMU supports IR */
> bool dt_supported; /* Whether vIOMMU supports DT */
> + bool pt_supported; /* Whether vIOMMU supports pass-through */
> IommuType type; /* IOMMU type - AMD/Intel */
> QLIST_HEAD(, IEC_Notifier) iec_notifiers; /* IEC notify list */
> };
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 12/12] vhost: iommu: cache static mapping if there is
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 12/12] vhost: iommu: cache static mapping if there is Peter Xu
@ 2017-05-11 8:35 ` Jason Wang
2017-05-11 8:59 ` Peter Xu
0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2017-05-11 8:35 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: David Gibson, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu
On 2017年05月10日 16:01, Peter Xu wrote:
> This patch pre-heat vhost iotlb cache when passthrough mode enabled.
>
> Sometimes, even if user specified iommu_platform for vhost devices,
> IOMMU might still be disabled. One case is passthrough mode in VT-d
> implementation. We can detect this by observing iommu_list. If it's
> empty, it means IOMMU translation is disabled, then we can actually
> pre-heat the translation (it'll be static mapping then) by first
> invalidating all IOTLB, then cache existing memory ranges into vhost
> backend iotlb using 1:1 mapping.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> hw/virtio/trace-events | 4 ++++
> hw/virtio/vhost.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 1f7a7c1..54dcbb3 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -24,3 +24,7 @@ virtio_balloon_handle_output(const char *name, uint64_t gpa) "section name: %s g
> virtio_balloon_get_config(uint32_t num_pages, uint32_t actual) "num_pages: %d actual: %d"
> virtio_balloon_set_config(uint32_t actual, uint32_t oldactual) "actual: %d oldactual: %d"
> virtio_balloon_to_target(uint64_t target, uint32_t num_pages) "balloon target: %"PRIx64" num_pages: %d"
> +
> +# hw/virtio/vhost.c
> +vhost_iommu_commit(void) ""
> +vhost_iommu_static_preheat(void) ""
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 0001e60..1c92e62 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -27,6 +27,7 @@
> #include "hw/virtio/virtio-access.h"
> #include "migration/migration.h"
> #include "sysemu/dma.h"
> +#include "trace.h"
>
> /* enabled until disconnected backend stabilizes */
> #define _VHOST_DEBUG 1
> @@ -730,6 +731,11 @@ static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> }
> }
>
> +static bool vhost_iommu_mr_enabled(struct vhost_dev *dev)
> +{
> + return !QLIST_EMPTY(&dev->iommu_list);
> +}
> +
> static void vhost_iommu_region_add(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> @@ -782,6 +788,48 @@ static void vhost_iommu_region_del(MemoryListener *listener,
> }
> }
>
> +static void vhost_iommu_commit(MemoryListener *listener)
> +{
> + struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> + iommu_listener);
> + struct vhost_memory_region *r;
> + int i;
> +
> + trace_vhost_iommu_commit();
> +
> + if (!vhost_iommu_mr_enabled(dev)) {
> + /*
> + * This means iommu_platform is enabled, however iommu memory
> + * region is disabled, e.g., when device passthrough is setup.
> + * Then, no translation is needed any more.
> + *
> + * Let's first invalidate the whole IOTLB, then pre-heat the
> + * static mapping by looping over vhost memory ranges.
> + */
> +
> + if (dev->vhost_ops->vhost_invalidate_device_iotlb(dev, 0,
> + UINT64_MAX-1)) {
> + error_report("%s: flush existing IOTLB failed", __func__);
> + return;
> + }
> +
> + for (i = 0; i < dev->mem->nregions; i++) {
> + r = &dev->mem->regions[i];
> + /* Vhost regions are writable RAM, so IOMMU_RW suites. */
> + if (dev->vhost_ops->vhost_update_device_iotlb(dev,
> + r->guest_phys_addr,
> + r->userspace_addr,
> + r->memory_size,
> + IOMMU_RW)) {
> + error_report("%s: pre-heat static mapping failed", __func__);
> + return;
> + }
> + }
> +
> + trace_vhost_iommu_static_preheat();
> + }
> +}
Looks like vfio does the map in region_add(), if we can have different
types of memory regions (e.g some were under an IOMMU but others were
not), do we need to switch to do this in vhost_iommu_region_add() ?
Thanks
> +
> static void vhost_region_nop(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> @@ -1298,6 +1346,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> hdev->iommu_listener = (MemoryListener) {
> .region_add = vhost_iommu_region_add,
> .region_del = vhost_iommu_region_del,
> + .commit = vhost_iommu_commit,
> };
>
> if (hdev->migration_blocker == NULL) {
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 10/12] intel_iommu: support passthrough (PT)
2017-05-11 8:31 ` Jason Wang
@ 2017-05-11 8:48 ` Peter Xu
0 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-11 8:48 UTC (permalink / raw)
To: Jason Wang
Cc: qemu-devel, Lan Tianyu, yi.l.liu, Michael S . Tsirkin,
Marcel Apfelbaum, David Gibson
On Thu, May 11, 2017 at 04:31:40PM +0800, Jason Wang wrote:
>
>
> On 2017年05月10日 16:01, Peter Xu wrote:
> >Hardware support for VT-d device passthrough. Although current Linux can
> >live with iommu=pt even without this, but this is faster than when using
> >software passthrough.
> >
> >Signed-off-by: Peter Xu <peterx@redhat.com>
> >---
> > hw/i386/intel_iommu.c | 210 ++++++++++++++++++++++++++++++++---------
> > hw/i386/intel_iommu_internal.h | 1 +
> > hw/i386/trace-events | 2 +
> > hw/i386/x86-iommu.c | 1 +
> > include/hw/i386/x86-iommu.h | 1 +
> > 5 files changed, 171 insertions(+), 44 deletions(-)
> >
> >diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >index 1a7eba2..1d034f9 100644
> >--- a/hw/i386/intel_iommu.c
> >+++ b/hw/i386/intel_iommu.c
> >@@ -640,6 +640,29 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
> > }
> > }
> >+/* Find the VTD address space associated with a given bus number */
> >+static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
> >+{
> >+ VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
> >+ if (!vtd_bus) {
> >+ /*
> >+ * Iterate over the registered buses to find the one which
> >+ * currently hold this bus number, and update the bus_num
> >+ * lookup table:
> >+ */
> >+ GHashTableIter iter;
> >+
> >+ g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
> >+ while (g_hash_table_iter_next (&iter, NULL, (void**)&vtd_bus)) {
> >+ if (pci_bus_num(vtd_bus->bus) == bus_num) {
> >+ s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> >+ return vtd_bus;
> >+ }
> >+ }
> >+ }
> >+ return vtd_bus;
> >+}
> >+
> > /* Given the @iova, get relevant @slptep. @slpte_level will be the last level
> > * of the translation, can be used for deciding the size of large page.
> > */
> >@@ -881,6 +904,11 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> > type_fail = true;
> > }
> > break;
> >+ case VTD_CONTEXT_TT_PASS_THROUGH:
> >+ if (!x86_iommu->pt_supported) {
> >+ type_fail = true;
> >+ }
> >+ break;
> > default:
> > /* Unknwon type */
> > type_fail = true;
> >@@ -894,6 +922,84 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> > return 0;
> > }
> >+/*
> >+ * Fetch translation type for specific device. Returns <0 if error
> >+ * happens, otherwise return the shifted type to check against
> >+ * VTD_CONTEXT_TT_*.
> >+ */
> >+static int vtd_dev_get_trans_type(VTDAddressSpace *as)
> >+{
> >+ IntelIOMMUState *s;
> >+ VTDContextEntry ce;
> >+ int ret;
> >+
> >+ s = as->iommu_state;
> >+
> >+ ret = vtd_dev_to_context_entry(s, pci_bus_num(as->bus),
> >+ as->devfn, &ce);
> >+ if (ret) {
> >+ return ret;
> >+ }
> >+
> >+ return vtd_ce_get_type(&ce);
> >+}
> >+
> >+static bool vtd_dev_pt_enabled(VTDAddressSpace *as)
> >+{
> >+ int ret;
> >+
> >+ assert(as);
> >+
> >+ ret = vtd_dev_get_trans_type(as);
> >+ if (ret < 0) {
> >+ /*
> >+ * Possibly failed to parse the context entry for some reason
> >+ * (e.g., during init, or any guest configuration errors on
> >+ * context entries). We should assume PT not enabled for
> >+ * safety.
> >+ */
> >+ return false;
> >+ }
> >+
> >+ return ret == VTD_CONTEXT_TT_PASS_THROUGH;
> >+}
> >+
> >+/*
> >+ * When we are during init phase (device realizations, global
> >+ * enable/disable of translations), we should not detect PT
> >+ * (passthrough) when switching address spaces. In that cases, we
> >+ * should set `detect_pt' to false.
> >+ *
> >+ * Return whether the device is using IOMMU translation.
> >+ */
> >+static bool vtd_switch_address_space(VTDAddressSpace *as, bool detect_pt)
> >+{
>
> The detect_pt looks suspicious. E.g if the context entry does not exist,
> vtd_dev_pt_enabled() will return false.
I forgot why I added that even after reading the comments I wrote. I
blame too much context switches recently in my brain. :(
(this is an excuse of mine :)
I did some test and I see nothing wrong to not hack on this bit. I
will remove that in next version, until one day I remembered
something.
And I will try to add more detailed comments in the future.
>
> >+ bool use_iommu;
> >+
> >+ assert(as);
> >+
> >+ use_iommu = as->iommu_state->dmar_enabled;
> >+ if (detect_pt) {
> >+ use_iommu &= !vtd_dev_pt_enabled(as);
> >+ }
> >+
> >+ trace_vtd_switch_address_space(pci_bus_num(as->bus),
> >+ VTD_PCI_SLOT(as->devfn),
> >+ VTD_PCI_FUNC(as->devfn),
> >+ use_iommu);
> >+
> >+ /* Turn off first then on the other */
> >+ if (use_iommu) {
> >+ memory_region_set_enabled(&as->sys_alias, false);
> >+ memory_region_set_enabled(&as->iommu, true);
> >+ } else {
> >+ memory_region_set_enabled(&as->iommu, false);
> >+ memory_region_set_enabled(&as->sys_alias, true);
> >+ }
> >+
> >+ return use_iommu;
> >+}
> >+
> > static inline uint16_t vtd_make_source_id(uint8_t bus_num, uint8_t devfn)
> > {
> > return ((bus_num & 0xffUL) << 8) | (devfn & 0xffUL);
> >@@ -931,6 +1037,31 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr)
> > return VTD_INTERRUPT_ADDR_FIRST <= addr && addr <= VTD_INTERRUPT_ADDR_LAST;
> > }
> >+static void vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id)
> >+{
> >+ VTDBus *vtd_bus;
> >+ VTDAddressSpace *vtd_as;
> >+ const char *msg = "FAIL";
> >+
> >+ vtd_bus = vtd_find_as_from_bus_num(s, VTD_SID_TO_BUS(source_id));
> >+ if (!vtd_bus) {
> >+ goto out;
> >+ }
> >+
> >+ vtd_as = vtd_bus->dev_as[VTD_SID_TO_DEVFN(source_id)];
> >+ if (!vtd_as) {
> >+ goto out;
> >+ }
> >+
> >+ if (vtd_switch_address_space(vtd_as, true) == false) {
> >+ /* We switched off IOMMU region successfully. */
> >+ msg = "SUCCESS";
> >+ }
> >+
> >+out:
> >+ trace_vtd_pt_enable_fast_path(source_id, msg);
>
> Looks like using a boolean is better here.
Sure.
>
> >+}
> >+
> > /* Map dev to context-entry then do a paging-structures walk to do a iommu
> > * translation.
> > *
> >@@ -1002,6 +1133,30 @@ static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
> > cc_entry->context_cache_gen = s->context_cache_gen;
> > }
> >+ /*
> >+ * We don't need to translate for pass-through context entries.
> >+ * Also, let's ignore IOTLB caching as well for PT devices.
> >+ */
> >+ if (vtd_ce_get_type(&ce) == VTD_CONTEXT_TT_PASS_THROUGH) {
> >+ entry->translated_addr = entry->iova;
> >+ entry->addr_mask = VTD_PAGE_SIZE - 1;
> >+ entry->perm = IOMMU_RW;
> >+ trace_vtd_translate_pt(source_id, entry->iova);
> >+
> >+ /*
> >+ * When this happens, it means firstly caching-mode is not
> >+ * enabled, and this is the first passthrough translation for
> >+ * the device. Let's enable the fast path for passthrough.
> >+ *
> >+ * When passthrough is disabled again for the device, we can
> >+ * capture it via the context entry invalidation, then the
> >+ * IOMMU region can be swapped back.
> >+ */
> >+ vtd_pt_enable_fast_path(s, source_id);
> >+
> >+ return;
> >+ }
> >+
> > ret_fr = vtd_iova_to_slpte(&ce, addr, is_write, &slpte, &level,
> > &reads, &writes);
> > if (ret_fr) {
> >@@ -1081,29 +1236,6 @@ static void vtd_context_global_invalidate(IntelIOMMUState *s)
> > vtd_iommu_replay_all(s);
> > }
> >-
> >-/* Find the VTD address space currently associated with a given bus number,
> >- */
> >-static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
> >-{
> >- VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
> >- if (!vtd_bus) {
> >- /* Iterate over the registered buses to find the one
> >- * which currently hold this bus number, and update the bus_num lookup table:
> >- */
> >- GHashTableIter iter;
> >-
> >- g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
> >- while (g_hash_table_iter_next (&iter, NULL, (void**)&vtd_bus)) {
> >- if (pci_bus_num(vtd_bus->bus) == bus_num) {
> >- s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> >- return vtd_bus;
> >- }
> >- }
> >- }
> >- return vtd_bus;
> >-}
> >-
> > /* Do a context-cache device-selective invalidation.
> > * @func_mask: FM field after shifting
> > */
> >@@ -1146,6 +1278,11 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
> > VTD_PCI_FUNC(devfn_it));
> > vtd_as->context_cache_entry.context_cache_gen = 0;
> > /*
> >+ * Do switch address space when needed, in case if the
> >+ * device passthrough bit is switched.
> >+ */
> >+ vtd_switch_address_space(vtd_as, true);
>
> Do we need to do this also in DSI and GLOBAL invalidation?
Yes. Though this should be optional at least for Linux, but I will add
that later.
Thanks!
--
Peter Xu
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 12/12] vhost: iommu: cache static mapping if there is
2017-05-11 8:35 ` Jason Wang
@ 2017-05-11 8:59 ` Peter Xu
2017-05-12 1:54 ` Jason Wang
0 siblings, 1 reply; 25+ messages in thread
From: Peter Xu @ 2017-05-11 8:59 UTC (permalink / raw)
To: Jason Wang
Cc: qemu-devel, David Gibson, Michael S . Tsirkin, yi.l.liu,
Marcel Apfelbaum, Lan Tianyu
On Thu, May 11, 2017 at 04:35:21PM +0800, Jason Wang wrote:
>
>
> On 2017年05月10日 16:01, Peter Xu wrote:
> >This patch pre-heat vhost iotlb cache when passthrough mode enabled.
> >
> >Sometimes, even if user specified iommu_platform for vhost devices,
> >IOMMU might still be disabled. One case is passthrough mode in VT-d
> >implementation. We can detect this by observing iommu_list. If it's
> >empty, it means IOMMU translation is disabled, then we can actually
> >pre-heat the translation (it'll be static mapping then) by first
> >invalidating all IOTLB, then cache existing memory ranges into vhost
> >backend iotlb using 1:1 mapping.
> >
> >Signed-off-by: Peter Xu <peterx@redhat.com>
> >---
> > hw/virtio/trace-events | 4 ++++
> > hw/virtio/vhost.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 53 insertions(+)
> >
> >diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> >index 1f7a7c1..54dcbb3 100644
> >--- a/hw/virtio/trace-events
> >+++ b/hw/virtio/trace-events
> >@@ -24,3 +24,7 @@ virtio_balloon_handle_output(const char *name, uint64_t gpa) "section name: %s g
> > virtio_balloon_get_config(uint32_t num_pages, uint32_t actual) "num_pages: %d actual: %d"
> > virtio_balloon_set_config(uint32_t actual, uint32_t oldactual) "actual: %d oldactual: %d"
> > virtio_balloon_to_target(uint64_t target, uint32_t num_pages) "balloon target: %"PRIx64" num_pages: %d"
> >+
> >+# hw/virtio/vhost.c
> >+vhost_iommu_commit(void) ""
> >+vhost_iommu_static_preheat(void) ""
> >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >index 0001e60..1c92e62 100644
> >--- a/hw/virtio/vhost.c
> >+++ b/hw/virtio/vhost.c
> >@@ -27,6 +27,7 @@
> > #include "hw/virtio/virtio-access.h"
> > #include "migration/migration.h"
> > #include "sysemu/dma.h"
> >+#include "trace.h"
> > /* enabled until disconnected backend stabilizes */
> > #define _VHOST_DEBUG 1
> >@@ -730,6 +731,11 @@ static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > }
> > }
> >+static bool vhost_iommu_mr_enabled(struct vhost_dev *dev)
> >+{
> >+ return !QLIST_EMPTY(&dev->iommu_list);
> >+}
> >+
> > static void vhost_iommu_region_add(MemoryListener *listener,
> > MemoryRegionSection *section)
> > {
> >@@ -782,6 +788,48 @@ static void vhost_iommu_region_del(MemoryListener *listener,
> > }
> > }
> >+static void vhost_iommu_commit(MemoryListener *listener)
> >+{
> >+ struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> >+ iommu_listener);
> >+ struct vhost_memory_region *r;
> >+ int i;
> >+
> >+ trace_vhost_iommu_commit();
> >+
> >+ if (!vhost_iommu_mr_enabled(dev)) {
> >+ /*
> >+ * This means iommu_platform is enabled, however iommu memory
> >+ * region is disabled, e.g., when device passthrough is setup.
> >+ * Then, no translation is needed any more.
> >+ *
> >+ * Let's first invalidate the whole IOTLB, then pre-heat the
> >+ * static mapping by looping over vhost memory ranges.
> >+ */
> >+
> >+ if (dev->vhost_ops->vhost_invalidate_device_iotlb(dev, 0,
> >+ UINT64_MAX-1)) {
> >+ error_report("%s: flush existing IOTLB failed", __func__);
> >+ return;
> >+ }
> >+
> >+ for (i = 0; i < dev->mem->nregions; i++) {
> >+ r = &dev->mem->regions[i];
> >+ /* Vhost regions are writable RAM, so IOMMU_RW suites. */
> >+ if (dev->vhost_ops->vhost_update_device_iotlb(dev,
> >+ r->guest_phys_addr,
> >+ r->userspace_addr,
> >+ r->memory_size,
> >+ IOMMU_RW)) {
> >+ error_report("%s: pre-heat static mapping failed", __func__);
> >+ return;
> >+ }
> >+ }
> >+
> >+ trace_vhost_iommu_static_preheat();
> >+ }
> >+}
>
> Looks like vfio does the map in region_add(), if we can have different types
> of memory regions (e.g some were under an IOMMU but others were not), do we
> need to switch to do this in vhost_iommu_region_add() ?
Currently this is only a pre-heat of cache only if IOMMU is totally
disabled (!vhost_iommu_mr_enabled(dev) means no IOMMU memory regions).
This patch won't be activated without this condition, so for the cases
(non-x86 platforms) where there are some IOMMU regions, it'll be just
automatically disabled. And, I don't really quite sure whether we
should cache non-IOMMU regions when there are some IOMMU regions... So
imho we can keep this until one day we really want to support some
non-x86 platforms for vhost-dmar, then we can work on top. Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry()
2017-05-11 1:56 ` David Gibson
@ 2017-05-11 9:36 ` Peter Xu
2017-05-12 5:25 ` David Gibson
0 siblings, 1 reply; 25+ messages in thread
From: Peter Xu @ 2017-05-11 9:36 UTC (permalink / raw)
To: David Gibson
Cc: qemu-devel, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, Jason Wang, Paolo Bonzini
On Thu, May 11, 2017 at 11:56:38AM +1000, David Gibson wrote:
> On Wed, May 10, 2017 at 04:01:47PM +0800, Peter Xu wrote:
> > This function has an assumption that we will definitely call translate()
> > once (or say, the addr will be located inside one IOMMU memory region),
> > otherwise an empty IOTLB will be returned. Nevertheless, this is not
> > what we want. When there is no IOMMU memory region, we should build up a
> > static mapping for the caller, instead of an invalid IOTLB.
> >
> > We won't trigger this path before VT-d passthrough mode. When
> > passthrough mode for a vhost device is setup, VT-d is possible to
> > disable the IOMMU region for that device. Without current patch, we'll
> > get a vhost boot failure, and it'll be failed over to virtio userspace
> > mode.
>
> This doesn't look right to me. You're assuming the target is
> address_space_memory, which might not be the case - and you should be
> able to check from the MR you do hit. Furthermore it doesn't look
> like you're accounting for the trivial translation if the section's
> offset in the address space is different from its offset in the MR.
Do you mean this line?
addr = addr - section->offset_within_address_space
+ section->offset_within_region;
I thought it was calculating the relative address against that memory
region. That should only be useful if we want to do further
translate(), right? For the path that this patch tries to handle (when
there is no translate() call), then this "addr" is useless here?
Regarding to the address space assignment - do you mean, e.g., I
should use section->address_space here instead of
&system_address_space? If so, I can do the switch. But after all, for
now address_space_get_iotlb_entry() is only used by vhost codes, and
it only check against iotlb.target_as == NULL, so the address space
didn't count too much here...
Another reason I used &address_space_memory is that in
vfio_iommu_map_notify() we have a check against it:
if (iotlb->target_as != &address_space_memory) {
error_report("Wrong target AS \"%s\", only system memory is allowed",
iotlb->target_as->name ? iotlb->target_as->name : "none");
return;
}
Or say, we have some assumptions (not only in this patch) that assumes
this iotlb.target_as should be system_address_space.
Thanks,
>
> I think for the fallback path you're going to want something based on
> address_space_translate() instead.
>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Jason Wang <jasowang@redhat.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > exec.c | 20 +++++++++++++++++++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 072de5d..5cfdacd 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -463,12 +463,13 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
> > }
> >
> > /* Called from RCU critical section */
> > -IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> > +IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr iova,
> > bool is_write)
> > {
> > IOMMUTLBEntry iotlb = {0};
> > MemoryRegionSection *section;
> > MemoryRegion *mr;
> > + hwaddr addr = iova, psize;
> >
> > for (;;) {
> > AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
> > @@ -478,6 +479,23 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> > mr = section->mr;
> >
> > if (!mr->iommu_ops) {
> > + /*
> > + * We didn't translate() but reached here. It possibly
> > + * means it's a static mapping. If so (it should be RAM),
> > + * we set the IOTLB up.
> > + */
> > + if (!iotlb.target_as && memory_region_is_ram(mr) &&
> > + !memory_region_is_rom(mr)) {
> > + psize = mr->ram_block->page_size;
> > + iova &= ~(psize - 1);
> > + iotlb = (IOMMUTLBEntry) {
> > + .target_as = &address_space_memory,
> > + .iova = iova,
> > + .translated_addr = iova,
> > + .addr_mask = psize - 1,
> > + .perm = IOMMU_RW,
> > + };
> > + }
> > break;
> > }
> >
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
--
Peter Xu
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 12/12] vhost: iommu: cache static mapping if there is
2017-05-11 8:59 ` Peter Xu
@ 2017-05-12 1:54 ` Jason Wang
0 siblings, 0 replies; 25+ messages in thread
From: Jason Wang @ 2017-05-12 1:54 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, David Gibson, Michael S . Tsirkin, yi.l.liu,
Marcel Apfelbaum, Lan Tianyu
On 2017年05月11日 16:59, Peter Xu wrote:
> On Thu, May 11, 2017 at 04:35:21PM +0800, Jason Wang wrote:
>>
>> On 2017年05月10日 16:01, Peter Xu wrote:
>>> This patch pre-heat vhost iotlb cache when passthrough mode enabled.
>>>
>>> Sometimes, even if user specified iommu_platform for vhost devices,
>>> IOMMU might still be disabled. One case is passthrough mode in VT-d
>>> implementation. We can detect this by observing iommu_list. If it's
>>> empty, it means IOMMU translation is disabled, then we can actually
>>> pre-heat the translation (it'll be static mapping then) by first
>>> invalidating all IOTLB, then cache existing memory ranges into vhost
>>> backend iotlb using 1:1 mapping.
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>> hw/virtio/trace-events | 4 ++++
>>> hw/virtio/vhost.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 53 insertions(+)
>>>
>>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>>> index 1f7a7c1..54dcbb3 100644
>>> --- a/hw/virtio/trace-events
>>> +++ b/hw/virtio/trace-events
>>> @@ -24,3 +24,7 @@ virtio_balloon_handle_output(const char *name, uint64_t gpa) "section name: %s g
>>> virtio_balloon_get_config(uint32_t num_pages, uint32_t actual) "num_pages: %d actual: %d"
>>> virtio_balloon_set_config(uint32_t actual, uint32_t oldactual) "actual: %d oldactual: %d"
>>> virtio_balloon_to_target(uint64_t target, uint32_t num_pages) "balloon target: %"PRIx64" num_pages: %d"
>>> +
>>> +# hw/virtio/vhost.c
>>> +vhost_iommu_commit(void) ""
>>> +vhost_iommu_static_preheat(void) ""
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index 0001e60..1c92e62 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -27,6 +27,7 @@
>>> #include "hw/virtio/virtio-access.h"
>>> #include "migration/migration.h"
>>> #include "sysemu/dma.h"
>>> +#include "trace.h"
>>> /* enabled until disconnected backend stabilizes */
>>> #define _VHOST_DEBUG 1
>>> @@ -730,6 +731,11 @@ static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>> }
>>> }
>>> +static bool vhost_iommu_mr_enabled(struct vhost_dev *dev)
>>> +{
>>> + return !QLIST_EMPTY(&dev->iommu_list);
>>> +}
>>> +
>>> static void vhost_iommu_region_add(MemoryListener *listener,
>>> MemoryRegionSection *section)
>>> {
>>> @@ -782,6 +788,48 @@ static void vhost_iommu_region_del(MemoryListener *listener,
>>> }
>>> }
>>> +static void vhost_iommu_commit(MemoryListener *listener)
>>> +{
>>> + struct vhost_dev *dev = container_of(listener, struct vhost_dev,
>>> + iommu_listener);
>>> + struct vhost_memory_region *r;
>>> + int i;
>>> +
>>> + trace_vhost_iommu_commit();
>>> +
>>> + if (!vhost_iommu_mr_enabled(dev)) {
>>> + /*
>>> + * This means iommu_platform is enabled, however iommu memory
>>> + * region is disabled, e.g., when device passthrough is setup.
>>> + * Then, no translation is needed any more.
>>> + *
>>> + * Let's first invalidate the whole IOTLB, then pre-heat the
>>> + * static mapping by looping over vhost memory ranges.
>>> + */
>>> +
>>> + if (dev->vhost_ops->vhost_invalidate_device_iotlb(dev, 0,
>>> + UINT64_MAX-1)) {
>>> + error_report("%s: flush existing IOTLB failed", __func__);
>>> + return;
>>> + }
>>> +
>>> + for (i = 0; i < dev->mem->nregions; i++) {
>>> + r = &dev->mem->regions[i];
>>> + /* Vhost regions are writable RAM, so IOMMU_RW suites. */
>>> + if (dev->vhost_ops->vhost_update_device_iotlb(dev,
>>> + r->guest_phys_addr,
>>> + r->userspace_addr,
>>> + r->memory_size,
>>> + IOMMU_RW)) {
>>> + error_report("%s: pre-heat static mapping failed", __func__);
>>> + return;
>>> + }
>>> + }
>>> +
>>> + trace_vhost_iommu_static_preheat();
>>> + }
>>> +}
>> Looks like vfio does the map in region_add(), if we can have different types
>> of memory regions (e.g some were under an IOMMU but others were not), do we
>> need to switch to do this in vhost_iommu_region_add() ?
> Currently this is only a pre-heat of cache only if IOMMU is totally
> disabled (!vhost_iommu_mr_enabled(dev) means no IOMMU memory regions).
> This patch won't be activated without this condition, so for the cases
> (non-x86 platforms) where there are some IOMMU regions, it'll be just
> automatically disabled. And, I don't really quite sure whether we
> should cache non-IOMMU regions when there are some IOMMU regions... So
> imho we can keep this until one day we really want to support some
> non-x86 platforms for vhost-dmar, then we can work on top. Thanks,
>
Right, so let's keep this as is and do optimization on top.
Thanks
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry()
2017-05-11 9:36 ` Peter Xu
@ 2017-05-12 5:25 ` David Gibson
2017-05-15 9:00 ` Peter Xu
0 siblings, 1 reply; 25+ messages in thread
From: David Gibson @ 2017-05-12 5:25 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, Jason Wang, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 6010 bytes --]
On Thu, May 11, 2017 at 05:36:03PM +0800, Peter Xu wrote:
> On Thu, May 11, 2017 at 11:56:38AM +1000, David Gibson wrote:
> > On Wed, May 10, 2017 at 04:01:47PM +0800, Peter Xu wrote:
> > > This function has an assumption that we will definitely call translate()
> > > once (or say, the addr will be located inside one IOMMU memory region),
> > > otherwise an empty IOTLB will be returned. Nevertheless, this is not
> > > what we want. When there is no IOMMU memory region, we should build up a
> > > static mapping for the caller, instead of an invalid IOTLB.
> > >
> > > We won't trigger this path before VT-d passthrough mode. When
> > > passthrough mode for a vhost device is setup, VT-d is possible to
> > > disable the IOMMU region for that device. Without current patch, we'll
> > > get a vhost boot failure, and it'll be failed over to virtio userspace
> > > mode.
> >
> > This doesn't look right to me. You're assuming the target is
> > address_space_memory, which might not be the case - and you should be
> > able to check from the MR you do hit. Furthermore it doesn't look
> > like you're accounting for the trivial translation if the section's
> > offset in the address space is different from its offset in the MR.
>
> Do you mean this line?
>
> addr = addr - section->offset_within_address_space
> + section->offset_within_region;
Uh.. where is that line? But.. wait, yes, I think I was mistaken. I saw:
.translated_addr = iova,
and thought that meant you were assuming an identify mapping from iova
to translated addr. But thinking more carefull, IIRC iova and
translated_addr are both relative to the MR, not the AS, so I think
that is correct after all.
> I thought it was calculating the relative address against that memory
> region. That should only be useful if we want to do further
> translate(), right? For the path that this patch tries to handle (when
> there is no translate() call), then this "addr" is useless here?
>
> Regarding to the address space assignment - do you mean, e.g., I
> should use section->address_space here instead of
> &system_address_space? If so, I can do the switch.
Yes, I think you should.
> But after all, for
> now address_space_get_iotlb_entry() is only used by vhost codes, and
> it only check against iotlb.target_as == NULL, so the address space
> didn't count too much here...
> Another reason I used &address_space_memory is that in
> vfio_iommu_map_notify() we have a check against it:
>
> if (iotlb->target_as != &address_space_memory) {
> error_report("Wrong target AS \"%s\", only system memory is allowed",
> iotlb->target_as->name ? iotlb->target_as->name : "none");
> return;
> }
>
> Or say, we have some assumptions (not only in this patch) that assumes
> this iotlb.target_as should be system_address_space.
Right, the vhost code can only handle some IOMMU setups - something
like nested IOMMUs would break it. But this way if someone sets up a
machine with an IOMMU configuration that vhost can't handle, you'll
get an error message, rather than accesses to unexpected locations,
which could cause really hard to debug corruption.
In other words we make assumptions, but we should _test_ those
assumptions.
I also think it would make sense to use address_space_translate() if we
can, since it's an existing interface for a very similar operation.
>
> Thanks,
>
> >
> > I think for the fallback path you're going to want something based on
> > address_space_translate() instead.
> >
> > > CC: Paolo Bonzini <pbonzini@redhat.com>
> > > CC: Jason Wang <jasowang@redhat.com>
> > > CC: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > > exec.c | 20 +++++++++++++++++++-
> > > 1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/exec.c b/exec.c
> > > index 072de5d..5cfdacd 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -463,12 +463,13 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
> > > }
> > >
> > > /* Called from RCU critical section */
> > > -IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> > > +IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr iova,
> > > bool is_write)
> > > {
> > > IOMMUTLBEntry iotlb = {0};
> > > MemoryRegionSection *section;
> > > MemoryRegion *mr;
> > > + hwaddr addr = iova, psize;
> > >
> > > for (;;) {
> > > AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
> > > @@ -478,6 +479,23 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> > > mr = section->mr;
> > >
> > > if (!mr->iommu_ops) {
> > > + /*
> > > + * We didn't translate() but reached here. It possibly
> > > + * means it's a static mapping. If so (it should be RAM),
> > > + * we set the IOTLB up.
> > > + */
> > > + if (!iotlb.target_as && memory_region_is_ram(mr) &&
> > > + !memory_region_is_rom(mr)) {
> > > + psize = mr->ram_block->page_size;
> > > + iova &= ~(psize - 1);
> > > + iotlb = (IOMMUTLBEntry) {
> > > + .target_as = &address_space_memory,
> > > + .iova = iova,
> > > + .translated_addr = iova,
> > > + .addr_mask = psize - 1,
> > > + .perm = IOMMU_RW,
> > > + };
> > > + }
> > > break;
> > > }
> > >
> >
>
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry()
2017-05-12 5:25 ` David Gibson
@ 2017-05-15 9:00 ` Peter Xu
0 siblings, 0 replies; 25+ messages in thread
From: Peter Xu @ 2017-05-15 9:00 UTC (permalink / raw)
To: David Gibson
Cc: qemu-devel, Michael S . Tsirkin, yi.l.liu, Marcel Apfelbaum,
Lan Tianyu, Jason Wang, Paolo Bonzini
On Fri, May 12, 2017 at 03:25:08PM +1000, David Gibson wrote:
> On Thu, May 11, 2017 at 05:36:03PM +0800, Peter Xu wrote:
> > On Thu, May 11, 2017 at 11:56:38AM +1000, David Gibson wrote:
> > > On Wed, May 10, 2017 at 04:01:47PM +0800, Peter Xu wrote:
> > > > This function has an assumption that we will definitely call translate()
> > > > once (or say, the addr will be located inside one IOMMU memory region),
> > > > otherwise an empty IOTLB will be returned. Nevertheless, this is not
> > > > what we want. When there is no IOMMU memory region, we should build up a
> > > > static mapping for the caller, instead of an invalid IOTLB.
> > > >
> > > > We won't trigger this path before VT-d passthrough mode. When
> > > > passthrough mode for a vhost device is setup, VT-d is possible to
> > > > disable the IOMMU region for that device. Without current patch, we'll
> > > > get a vhost boot failure, and it'll be failed over to virtio userspace
> > > > mode.
> > >
> > > This doesn't look right to me. You're assuming the target is
> > > address_space_memory, which might not be the case - and you should be
> > > able to check from the MR you do hit. Furthermore it doesn't look
> > > like you're accounting for the trivial translation if the section's
> > > offset in the address space is different from its offset in the MR.
> >
> > Do you mean this line?
> >
> > addr = addr - section->offset_within_address_space
> > + section->offset_within_region;
>
> Uh.. where is that line? But.. wait, yes, I think I was mistaken. I saw:
> .translated_addr = iova,
>
> and thought that meant you were assuming an identify mapping from iova
> to translated addr. But thinking more carefull, IIRC iova and
> translated_addr are both relative to the MR, not the AS, so I think
> that is correct after all.
>
> > I thought it was calculating the relative address against that memory
> > region. That should only be useful if we want to do further
> > translate(), right? For the path that this patch tries to handle (when
> > there is no translate() call), then this "addr" is useless here?
> >
> > Regarding to the address space assignment - do you mean, e.g., I
> > should use section->address_space here instead of
> > &system_address_space? If so, I can do the switch.
>
> Yes, I think you should.
>
> > But after all, for
> > now address_space_get_iotlb_entry() is only used by vhost codes, and
> > it only check against iotlb.target_as == NULL, so the address space
> > didn't count too much here...
>
> > Another reason I used &address_space_memory is that in
> > vfio_iommu_map_notify() we have a check against it:
> >
> > if (iotlb->target_as != &address_space_memory) {
> > error_report("Wrong target AS \"%s\", only system memory is allowed",
> > iotlb->target_as->name ? iotlb->target_as->name : "none");
> > return;
> > }
> >
> > Or say, we have some assumptions (not only in this patch) that assumes
> > this iotlb.target_as should be system_address_space.
>
> Right, the vhost code can only handle some IOMMU setups - something
> like nested IOMMUs would break it. But this way if someone sets up a
> machine with an IOMMU configuration that vhost can't handle, you'll
> get an error message, rather than accesses to unexpected locations,
> which could cause really hard to debug corruption.
>
> In other words we make assumptions, but we should _test_ those
> assumptions.
>
> I also think it would make sense to use address_space_translate() if we
> can, since it's an existing interface for a very similar operation.
I did give it a shot to generalize these two functions in this series
(I just posted it):
[PATCH 0/4] exec: address space translation cleanups
Please see whether that'll be a good replacement of this single patch.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-05-15 9:00 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-10 8:01 [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 01/12] pc: add 2.10 machine type Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 02/12] memory: tune last param of iommu_ops.translate() Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 03/12] memory: remove the last param in memory_region_iommu_replay() Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry() Peter Xu
2017-05-11 1:56 ` David Gibson
2017-05-11 9:36 ` Peter Xu
2017-05-12 5:25 ` David Gibson
2017-05-15 9:00 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 05/12] x86-iommu: use DeviceClass properties Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 06/12] intel_iommu: renaming context entry helpers Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 07/12] intel_iommu: provide vtd_ce_get_type() Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 08/12] intel_iommu: use IOMMU_ACCESS_FLAG() Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 09/12] intel_iommu: allow dev-iotlb context entry conditionally Peter Xu
2017-05-11 2:56 ` Jason Wang
2017-05-11 3:51 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 10/12] intel_iommu: support passthrough (PT) Peter Xu
2017-05-11 8:31 ` Jason Wang
2017-05-11 8:48 ` Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 11/12] intel_iommu: turn off pt before 2.9 Peter Xu
2017-05-10 8:01 ` [Qemu-devel] [PATCH v3 12/12] vhost: iommu: cache static mapping if there is Peter Xu
2017-05-11 8:35 ` Jason Wang
2017-05-11 8:59 ` Peter Xu
2017-05-12 1:54 ` Jason Wang
2017-05-10 8:55 ` [Qemu-devel] [PATCH v3 00/12] VT-d: PT (passthrough) mode support and misc fixes no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.