* [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers
2016-11-28 15:51 [Qemu-devel] [PATCH v7 0/5] IOMMU: intel_iommu support map and unmap notifications Aviv B.D
@ 2016-11-28 15:51 ` Aviv B.D
2016-11-29 3:23 ` 蓝天宇
0 siblings, 1 reply; 8+ messages in thread
From: Aviv B.D @ 2016-11-28 15:51 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Alex Williamson, Peter Xu, Jan Kiszka,
Jason Wang, Aviv Ben-David
From: "Aviv Ben-David" <bd.aviv@gmail.com>
Adds a list of registered vtd_as's to intel iommu state to save
iteration over each PCI device in a search of the corrosponding domain.
Signed-off-by: Aviv Ben-David <bd.aviv@gmail.com>
---
hw/i386/intel_iommu.c | 94 ++++++++++++++++++++++++++++++++++++++----
hw/i386/intel_iommu_internal.h | 2 +
include/hw/i386/intel_iommu.h | 9 ++++
3 files changed, 98 insertions(+), 7 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 05973b9..d872969 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -679,7 +679,7 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t gpa,
}
*reads = (*reads) && (slpte & VTD_SL_R);
*writes = (*writes) && (slpte & VTD_SL_W);
- if (!(slpte & access_right_check)) {
+ if (!(slpte & access_right_check) && !(flags & IOMMU_NO_FAIL)) {
VTD_DPRINTF(GENERAL, "error: lack of %s permission for "
"gpa 0x%"PRIx64 " slpte 0x%"PRIx64,
(flags & IOMMU_WO ? "write" : "read"), gpa, slpte);
@@ -978,6 +978,23 @@ static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
return vtd_bus;
}
+static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num, uint8_t devfn,
+ uint16_t *domain_id)
+{
+ VTDContextEntry ce;
+ int ret_fr;
+
+ assert(domain_id);
+
+ ret_fr = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
+ if (ret_fr) {
+ return -1;
+ }
+
+ *domain_id = VTD_CONTEXT_ENTRY_DID(ce.hi);
+ return 0;
+}
+
/* Do a context-cache device-selective invalidation.
* @func_mask: FM field after shifting
*/
@@ -1064,6 +1081,45 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
&domain_id);
}
+static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
+ uint16_t domain_id, hwaddr addr,
+ uint8_t am)
+{
+ IntelIOMMUNotifierNode *node;
+
+ QLIST_FOREACH(node, &(s->notifiers_list), next) {
+ VTDAddressSpace *vtd_as = node->vtd_as;
+ uint16_t vfio_domain_id;
+ int ret = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus), vtd_as->devfn,
+ &vfio_domain_id);
+
+ if (!ret && domain_id == vfio_domain_id) {
+ hwaddr original_addr = addr;
+
+ while (addr < original_addr + (1 << am) * VTD_PAGE_SIZE) {
+ IOMMUTLBEntry entry = s->iommu_ops.translate(
+ &node->vtd_as->iommu,
+ addr,
+ IOMMU_NO_FAIL);
+
+ if (entry.perm == IOMMU_NONE &&
+ node->notifier_flag & IOMMU_NOTIFIER_UNMAP) {
+ entry.target_as = &address_space_memory;
+ entry.iova = addr & VTD_PAGE_MASK_4K;
+ entry.translated_addr = 0;
+ entry.addr_mask = ~VTD_PAGE_MASK(VTD_PAGE_SHIFT);
+ memory_region_notify_iommu(&node->vtd_as->iommu, entry);
+ addr += VTD_PAGE_SIZE;
+ } else if (node->notifier_flag & IOMMU_NOTIFIER_MAP) {
+ memory_region_notify_iommu(&node->vtd_as->iommu, entry);
+ addr += entry.addr_mask + 1;
+ }
+ }
+ }
+ }
+}
+
static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
hwaddr addr, uint8_t am)
{
@@ -1074,6 +1130,8 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
info.addr = addr;
info.mask = ~((1 << am) - 1);
g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);
+
+ vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
}
/* Flush IOTLB
@@ -1999,15 +2057,37 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
IOMMUNotifierFlag new)
{
VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
+ IntelIOMMUState *s = vtd_as->iommu_state;
+ IntelIOMMUNotifierNode *node = NULL;
+ IntelIOMMUNotifierNode *next_node = NULL;
- if (new & IOMMU_NOTIFIER_MAP) {
- error_report("Device at bus %s addr %02x.%d requires iommu "
- "notifier which is currently not supported by "
- "intel-iommu emulation",
- vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
- PCI_FUNC(vtd_as->devfn));
+ if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
+ error_report("We need to set cache_mode=1 for intel-iommu to enable "
+ "device assignment with IOMMU protection.");
exit(1);
}
+
+ /* Add new ndoe if no mapping was exising before this call */
+ if (old == IOMMU_NOTIFIER_NONE) {
+ node = g_malloc0(sizeof(*node));
+ node->vtd_as = vtd_as;
+ node->notifier_flag = new;
+ QLIST_INSERT_HEAD(&s->notifiers_list, node, next);
+ return;
+ }
+
+ /* update notifier node with new flags */
+ QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
+ if (node->vtd_as == vtd_as) {
+ if (new == IOMMU_NOTIFIER_NONE) {
+ QLIST_REMOVE(node, next);
+ g_free(node);
+ } else {
+ node->notifier_flag = new;
+ }
+ return;
+ }
+ }
}
static const VMStateDescription vtd_vmstate = {
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 00cbe0d..c1b9cfc 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -381,6 +381,8 @@ typedef struct VTDIOTLBPageInvInfo VTDIOTLBPageInvInfo;
#define VTD_PAGE_SHIFT_1G 30
#define VTD_PAGE_MASK_1G (~((1ULL << VTD_PAGE_SHIFT_1G) - 1))
+#define VTD_PAGE_MASK(shift) (~((1ULL << (shift)) - 1))
+
struct VTDRootEntry {
uint64_t val;
uint64_t rsvd;
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 749eef9..1a355c1 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -63,6 +63,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
typedef struct VTDIrq VTDIrq;
typedef struct VTD_MSIMessage VTD_MSIMessage;
+typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
/* Context-Entry */
struct VTDContextEntry {
@@ -247,6 +248,12 @@ struct VTD_MSIMessage {
/* When IR is enabled, all MSI/MSI-X data bits should be zero */
#define VTD_IR_MSI_DATA (0)
+struct IntelIOMMUNotifierNode {
+ VTDAddressSpace *vtd_as;
+ IOMMUNotifierFlag notifier_flag;
+ QLIST_ENTRY(IntelIOMMUNotifierNode) next;
+};
+
/* The iommu (DMAR) device state struct */
struct IntelIOMMUState {
X86IOMMUState x86_iommu;
@@ -284,6 +291,8 @@ struct IntelIOMMUState {
MemoryRegionIOMMUOps iommu_ops;
GHashTable *vtd_as_by_busptr; /* VTDBus objects indexed by PCIBus* reference */
VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
+ /* list of registered notifiers */
+ QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
/* interrupt remapping */
bool intr_enabled; /* Whether guest enabled IR */
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers
2016-11-28 15:51 ` [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers Aviv B.D
@ 2016-11-29 3:23 ` 蓝天宇
2016-11-29 7:57 ` Aviv B.D.
0 siblings, 1 reply; 8+ messages in thread
From: 蓝天宇 @ 2016-11-29 3:23 UTC (permalink / raw)
To: Aviv B.D
Cc: qemu-devel, Michael S. Tsirkin, Jan Kiszka, Peter Xu,
Alex Williamson, Jason Wang
2016-11-28 23:51 GMT+08:00 Aviv B.D <bd.aviv@gmail.com>:
> From: "Aviv Ben-David" <bd.aviv@gmail.com>
>
> Adds a list of registered vtd_as's to intel iommu state to save
> iteration over each PCI device in a search of the corrosponding domain.
>
> Signed-off-by: Aviv Ben-David <bd.aviv@gmail.com>
> ---
> hw/i386/intel_iommu.c | 94 ++++++++++++++++++++++++++++++++++++++----
> hw/i386/intel_iommu_internal.h | 2 +
> include/hw/i386/intel_iommu.h | 9 ++++
> 3 files changed, 98 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 05973b9..d872969 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -679,7 +679,7 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t gpa,
> }
> *reads = (*reads) && (slpte & VTD_SL_R);
> *writes = (*writes) && (slpte & VTD_SL_W);
> - if (!(slpte & access_right_check)) {
> + if (!(slpte & access_right_check) && !(flags & IOMMU_NO_FAIL)) {
> VTD_DPRINTF(GENERAL, "error: lack of %s permission for "
> "gpa 0x%"PRIx64 " slpte 0x%"PRIx64,
> (flags & IOMMU_WO ? "write" : "read"), gpa, slpte);
> @@ -978,6 +978,23 @@ static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
> return vtd_bus;
> }
>
> +static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num, uint8_t devfn,
> + uint16_t *domain_id)
> +{
> + VTDContextEntry ce;
> + int ret_fr;
> +
> + assert(domain_id);
> +
> + ret_fr = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
> + if (ret_fr) {
> + return -1;
> + }
> +
> + *domain_id = VTD_CONTEXT_ENTRY_DID(ce.hi);
> + return 0;
> +}
> +
> /* Do a context-cache device-selective invalidation.
> * @func_mask: FM field after shifting
> */
> @@ -1064,6 +1081,45 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
> &domain_id);
> }
>
> +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> + uint16_t domain_id, hwaddr addr,
> + uint8_t am)
> +{
> + IntelIOMMUNotifierNode *node;
> +
> + QLIST_FOREACH(node, &(s->notifiers_list), next) {
> + VTDAddressSpace *vtd_as = node->vtd_as;
> + uint16_t vfio_domain_id;
> + int ret = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus), vtd_as->devfn,
> + &vfio_domain_id);
> +
> + if (!ret && domain_id == vfio_domain_id) {
> + hwaddr original_addr = addr;
> +
> + while (addr < original_addr + (1 << am) * VTD_PAGE_SIZE) {
> + IOMMUTLBEntry entry = s->iommu_ops.translate(
> + &node->vtd_as->iommu,
> + addr,
> + IOMMU_NO_FAIL);
> +
> + if (entry.perm == IOMMU_NONE &&
> + node->notifier_flag & IOMMU_NOTIFIER_UNMAP) {
> + entry.target_as = &address_space_memory;
> + entry.iova = addr & VTD_PAGE_MASK_4K;
> + entry.translated_addr = 0;
> + entry.addr_mask = ~VTD_PAGE_MASK(VTD_PAGE_SHIFT);
> + memory_region_notify_iommu(&node->vtd_as->iommu, entry);
> + addr += VTD_PAGE_SIZE;
> + } else if (node->notifier_flag & IOMMU_NOTIFIER_MAP) {
> + memory_region_notify_iommu(&node->vtd_as->iommu, entry);
> + addr += entry.addr_mask + 1;
> + }
> + }
> + }
> + }
> +}
> +
> static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
> hwaddr addr, uint8_t am)
> {
> @@ -1074,6 +1130,8 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
> info.addr = addr;
> info.mask = ~((1 << am) - 1);
> g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);
> +
> + vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
> }
>
> /* Flush IOTLB
> @@ -1999,15 +2057,37 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
> IOMMUNotifierFlag new)
> {
> VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
> + IntelIOMMUState *s = vtd_as->iommu_state;
> + IntelIOMMUNotifierNode *node = NULL;
> + IntelIOMMUNotifierNode *next_node = NULL;
>
> - if (new & IOMMU_NOTIFIER_MAP) {
> - error_report("Device at bus %s addr %02x.%d requires iommu "
> - "notifier which is currently not supported by "
> - "intel-iommu emulation",
> - vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> - PCI_FUNC(vtd_as->devfn));
> + if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
> + error_report("We need to set cache_mode=1 for intel-iommu to enable "
> + "device assignment with IOMMU protection.");
> exit(1);
> }
> +
> + /* Add new ndoe if no mapping was exising before this call */
s /ndoe/node /exising/existing
> + if (old == IOMMU_NOTIFIER_NONE) {
Where is the declaration of "old"?
> + node = g_malloc0(sizeof(*node));
> + node->vtd_as = vtd_as;
> + node->notifier_flag = new;
> + QLIST_INSERT_HEAD(&s->notifiers_list, node, next);
> + return;
> + }
> +
> + /* update notifier node with new flags */
> + QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
> + if (node->vtd_as == vtd_as) {
> + if (new == IOMMU_NOTIFIER_NONE) {
> + QLIST_REMOVE(node, next);
> + g_free(node);
> + } else {
> + node->notifier_flag = new;
> + }
> + return;
> + }
> + }
> }
>
> static const VMStateDescription vtd_vmstate = {
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 00cbe0d..c1b9cfc 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -381,6 +381,8 @@ typedef struct VTDIOTLBPageInvInfo VTDIOTLBPageInvInfo;
> #define VTD_PAGE_SHIFT_1G 30
> #define VTD_PAGE_MASK_1G (~((1ULL << VTD_PAGE_SHIFT_1G) - 1))
>
> +#define VTD_PAGE_MASK(shift) (~((1ULL << (shift)) - 1))
> +
> struct VTDRootEntry {
> uint64_t val;
> uint64_t rsvd;
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 749eef9..1a355c1 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -63,6 +63,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
> typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
> typedef struct VTDIrq VTDIrq;
> typedef struct VTD_MSIMessage VTD_MSIMessage;
> +typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
>
> /* Context-Entry */
> struct VTDContextEntry {
> @@ -247,6 +248,12 @@ struct VTD_MSIMessage {
> /* When IR is enabled, all MSI/MSI-X data bits should be zero */
> #define VTD_IR_MSI_DATA (0)
>
> +struct IntelIOMMUNotifierNode {
> + VTDAddressSpace *vtd_as;
> + IOMMUNotifierFlag notifier_flag;
> + QLIST_ENTRY(IntelIOMMUNotifierNode) next;
> +};
> +
> /* The iommu (DMAR) device state struct */
> struct IntelIOMMUState {
> X86IOMMUState x86_iommu;
> @@ -284,6 +291,8 @@ struct IntelIOMMUState {
> MemoryRegionIOMMUOps iommu_ops;
> GHashTable *vtd_as_by_busptr; /* VTDBus objects indexed by PCIBus* reference */
> VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
> + /* list of registered notifiers */
> + QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
>
> /* interrupt remapping */
> bool intr_enabled; /* Whether guest enabled IR */
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers
2016-11-29 3:23 ` 蓝天宇
@ 2016-11-29 7:57 ` Aviv B.D.
0 siblings, 0 replies; 8+ messages in thread
From: Aviv B.D. @ 2016-11-29 7:57 UTC (permalink / raw)
To: 蓝天宇
Cc: qemu-devel, Michael S. Tsirkin, Jan Kiszka, Peter Xu,
Alex Williamson, Jason Wang
On Tue, Nov 29, 2016 at 5:23 AM 蓝天宇 <lantianyu1986@gmail.com> wrote:
> 2016-11-28 23:51 GMT+08:00 Aviv B.D <bd.aviv@gmail.com>:
> > From: "Aviv Ben-David" <bd.aviv@gmail.com>
> >
> > Adds a list of registered vtd_as's to intel iommu state to save
> > iteration over each PCI device in a search of the corrosponding domain.
> >
> > Signed-off-by: Aviv Ben-David <bd.aviv@gmail.com>
> > ---
> > hw/i386/intel_iommu.c | 94
> ++++++++++++++++++++++++++++++++++++++----
> > hw/i386/intel_iommu_internal.h | 2 +
> > include/hw/i386/intel_iommu.h | 9 ++++
> > 3 files changed, 98 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 05973b9..d872969 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -679,7 +679,7 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce,
> uint64_t gpa,
> > }
> > *reads = (*reads) && (slpte & VTD_SL_R);
> > *writes = (*writes) && (slpte & VTD_SL_W);
> > - if (!(slpte & access_right_check)) {
> > + if (!(slpte & access_right_check) && !(flags & IOMMU_NO_FAIL)) {
> > VTD_DPRINTF(GENERAL, "error: lack of %s permission for "
> > "gpa 0x%"PRIx64 " slpte 0x%"PRIx64,
> > (flags & IOMMU_WO ? "write" : "read"), gpa,
> slpte);
> > @@ -978,6 +978,23 @@ static VTDBus
> *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
> > return vtd_bus;
> > }
> >
> > +static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num, uint8_t
> devfn,
> > + uint16_t *domain_id)
> > +{
> > + VTDContextEntry ce;
> > + int ret_fr;
> > +
> > + assert(domain_id);
> > +
> > + ret_fr = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
> > + if (ret_fr) {
> > + return -1;
> > + }
> > +
> > + *domain_id = VTD_CONTEXT_ENTRY_DID(ce.hi);
> > + return 0;
> > +}
> > +
> > /* Do a context-cache device-selective invalidation.
> > * @func_mask: FM field after shifting
> > */
> > @@ -1064,6 +1081,45 @@ static void
> vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
> > &domain_id);
> > }
> >
> > +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> > + uint16_t domain_id, hwaddr
> addr,
> > + uint8_t am)
> > +{
> > + IntelIOMMUNotifierNode *node;
> > +
> > + QLIST_FOREACH(node, &(s->notifiers_list), next) {
> > + VTDAddressSpace *vtd_as = node->vtd_as;
> > + uint16_t vfio_domain_id;
> > + int ret = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus),
> vtd_as->devfn,
> > + &vfio_domain_id);
> > +
> > + if (!ret && domain_id == vfio_domain_id) {
> > + hwaddr original_addr = addr;
> > +
> > + while (addr < original_addr + (1 << am) * VTD_PAGE_SIZE) {
> > + IOMMUTLBEntry entry = s->iommu_ops.translate(
> > +
> &node->vtd_as->iommu,
> > + addr,
> > + IOMMU_NO_FAIL);
> > +
> > + if (entry.perm == IOMMU_NONE &&
> > + node->notifier_flag & IOMMU_NOTIFIER_UNMAP) {
> > + entry.target_as = &address_space_memory;
> > + entry.iova = addr & VTD_PAGE_MASK_4K;
> > + entry.translated_addr = 0;
> > + entry.addr_mask = ~VTD_PAGE_MASK(VTD_PAGE_SHIFT);
> > + memory_region_notify_iommu(&node->vtd_as->iommu,
> entry);
> > + addr += VTD_PAGE_SIZE;
> > + } else if (node->notifier_flag & IOMMU_NOTIFIER_MAP) {
> > +
> memory_region_notify_iommu(&node->vtd_as->iommu, entry);
> > + addr += entry.addr_mask + 1;
> > + }
> > + }
> > + }
> > + }
> > +}
> > +
> > static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t
> domain_id,
> > hwaddr addr, uint8_t am)
> > {
> > @@ -1074,6 +1130,8 @@ static void
> vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
> > info.addr = addr;
> > info.mask = ~((1 << am) - 1);
> > g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page,
> &info);
> > +
> > + vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
> > }
> >
> > /* Flush IOTLB
> > @@ -1999,15 +2057,37 @@ static void
> vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
> > IOMMUNotifierFlag new)
> > {
> > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace,
> iommu);
> > + IntelIOMMUState *s = vtd_as->iommu_state;
> > + IntelIOMMUNotifierNode *node = NULL;
> > + IntelIOMMUNotifierNode *next_node = NULL;
> >
> > - if (new & IOMMU_NOTIFIER_MAP) {
> > - error_report("Device at bus %s addr %02x.%d requires iommu "
> > - "notifier which is currently not supported by "
> > - "intel-iommu emulation",
> > - vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> > - PCI_FUNC(vtd_as->devfn));
> > + if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
> > + error_report("We need to set cache_mode=1 for intel-iommu to
> enable "
> > + "device assignment with IOMMU protection.");
> > exit(1);
> > }
> > +
> > + /* Add new ndoe if no mapping was exising before this call */
>
> s /ndoe/node /exising/existing
>
Sorry, it will be changed.
>
> > + if (old == IOMMU_NOTIFIER_NONE) {
>
> Where is the declaration of "old"?
>
Old is one of the function parameters.
> > + node = g_malloc0(sizeof(*node));
> > + node->vtd_as = vtd_as;
> > + node->notifier_flag = new;
> > + QLIST_INSERT_HEAD(&s->notifiers_list, node, next);
> > + return;
> > + }
> > +
> > + /* update notifier node with new flags */
> > + QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
> > + if (node->vtd_as == vtd_as) {
> > + if (new == IOMMU_NOTIFIER_NONE) {
> > + QLIST_REMOVE(node, next);
> > + g_free(node);
> > + } else {
> > + node->notifier_flag = new;
> > + }
> > + return;
> > + }
> > + }
> > }
> >
> > static const VMStateDescription vtd_vmstate = {
> > diff --git a/hw/i386/intel_iommu_internal.h
> b/hw/i386/intel_iommu_internal.h
> > index 00cbe0d..c1b9cfc 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -381,6 +381,8 @@ typedef struct VTDIOTLBPageInvInfo
> VTDIOTLBPageInvInfo;
> > #define VTD_PAGE_SHIFT_1G 30
> > #define VTD_PAGE_MASK_1G (~((1ULL << VTD_PAGE_SHIFT_1G) - 1))
> >
> > +#define VTD_PAGE_MASK(shift) (~((1ULL << (shift)) - 1))
> > +
> > struct VTDRootEntry {
> > uint64_t val;
> > uint64_t rsvd;
> > diff --git a/include/hw/i386/intel_iommu.h
> b/include/hw/i386/intel_iommu.h
> > index 749eef9..1a355c1 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -63,6 +63,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
> > typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
> > typedef struct VTDIrq VTDIrq;
> > typedef struct VTD_MSIMessage VTD_MSIMessage;
> > +typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
> >
> > /* Context-Entry */
> > struct VTDContextEntry {
> > @@ -247,6 +248,12 @@ struct VTD_MSIMessage {
> > /* When IR is enabled, all MSI/MSI-X data bits should be zero */
> > #define VTD_IR_MSI_DATA (0)
> >
> > +struct IntelIOMMUNotifierNode {
> > + VTDAddressSpace *vtd_as;
> > + IOMMUNotifierFlag notifier_flag;
> > + QLIST_ENTRY(IntelIOMMUNotifierNode) next;
> > +};
> > +
> > /* The iommu (DMAR) device state struct */
> > struct IntelIOMMUState {
> > X86IOMMUState x86_iommu;
> > @@ -284,6 +291,8 @@ struct IntelIOMMUState {
> > MemoryRegionIOMMUOps iommu_ops;
> > GHashTable *vtd_as_by_busptr; /* VTDBus objects indexed by
> PCIBus* reference */
> > VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects
> indexed by bus number */
> > + /* list of registered notifiers */
> > + QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
> >
> > /* interrupt remapping */
> > bool intr_enabled; /* Whether guest enabled IR */
> > --
> > 1.9.1
> >
> >
>
Aviv.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers
@ 2016-12-16 9:12 Liu, Yi L
2016-12-19 10:00 ` Peter Xu
2016-12-19 11:54 ` Liu, Yi L
0 siblings, 2 replies; 8+ messages in thread
From: Liu, Yi L @ 2016-12-16 9:12 UTC (permalink / raw)
To: bd.aviv@gmail.com, qemu-devel@nongnu.org
Cc: Michael S. Tsirkin, , Jan Kiszka, , Peter Xu, , Alex Williamson,
, Jason Wang, Lan, Tianyu, Tian, Kevin, Liu, Yi L
> From: "Aviv Ben-David" <address@hidden>
>
> Adds a list of registered vtd_as's to intel iommu state to save
> iteration over each PCI device in a search of the corrosponding domain.
>
> Signed-off-by: Aviv Ben-David <address@hidden>
> ---
> hw/i386/intel_iommu.c | 94 ++++++++++++++++++++++++++++++++++++++----
> hw/i386/intel_iommu_internal.h | 2 +
> include/hw/i386/intel_iommu.h | 9 ++++
> 3 files changed, 98 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 05973b9..d872969 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -679,7 +679,7 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t
> gpa,
> }
> *reads = (*reads) && (slpte & VTD_SL_R);
> *writes = (*writes) && (slpte & VTD_SL_W);
> - if (!(slpte & access_right_check)) {
> + if (!(slpte & access_right_check) && !(flags & IOMMU_NO_FAIL)) {
> VTD_DPRINTF(GENERAL, "error: lack of %s permission for "
> "gpa 0x%"PRIx64 " slpte 0x%"PRIx64,
> (flags & IOMMU_WO ? "write" : "read"), gpa, slpte);
> @@ -978,6 +978,23 @@ static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState
> *s, uint8_t bus_num)
> return vtd_bus;
> }
>
> +static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num, uint8_t devfn,
> + uint16_t *domain_id)
> +{
> + VTDContextEntry ce;
> + int ret_fr;
> +
> + assert(domain_id);
> +
> + ret_fr = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
> + if (ret_fr) {
> + return -1;
> + }
> +
> + *domain_id = VTD_CONTEXT_ENTRY_DID(ce.hi);
> + return 0;
> +}
> +
> /* Do a context-cache device-selective invalidation.
> * @func_mask: FM field after shifting
> */
> @@ -1064,6 +1081,45 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState
> *s, uint16_t domain_id)
> &domain_id);
> }
>
> +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> + uint16_t domain_id, hwaddr addr,
> + uint8_t am)
> +{
> + IntelIOMMUNotifierNode *node;
> +
> + QLIST_FOREACH(node, &(s->notifiers_list), next) {
Aviv,
Regards to the s->notifiers_list, I didn't see the init op to it. Does it happen
in another patch? If so, it may be better to move it in this patch since this
patch introduces both the definition and usage of notifiers_list.
If it is already clarified, then ignore it.
Thanks,
Yi L
> + VTDAddressSpace *vtd_as = node->vtd_as;
> + uint16_t vfio_domain_id;
> + int ret = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus), vtd_as->devfn,
> + &vfio_domain_id);
> +
> + if (!ret && domain_id == vfio_domain_id) {
> + hwaddr original_addr = addr;
> +
> + while (addr < original_addr + (1 << am) * VTD_PAGE_SIZE) {
> + IOMMUTLBEntry entry = s->iommu_ops.translate(
> + &node->vtd_as->iommu,
> + addr,
> + IOMMU_NO_FAIL);
> +
> + if (entry.perm == IOMMU_NONE &&
> + node->notifier_flag & IOMMU_NOTIFIER_UNMAP) {
> + entry.target_as = &address_space_memory;
> + entry.iova = addr & VTD_PAGE_MASK_4K;
> + entry.translated_addr = 0;
> + entry.addr_mask = ~VTD_PAGE_MASK(VTD_PAGE_SHIFT);
> + memory_region_notify_iommu(&node->vtd_as->iommu, entry);
> + addr += VTD_PAGE_SIZE;
> + } else if (node->notifier_flag & IOMMU_NOTIFIER_MAP) {
> + memory_region_notify_iommu(&node->vtd_as->iommu,
> entry);
> + addr += entry.addr_mask + 1;
> + }
> + }
> + }
> + }
> +}
> +
> static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
> hwaddr addr, uint8_t am)
> {
> @@ -1074,6 +1130,8 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s,
> uint16_t domain_id,
> info.addr = addr;
> info.mask = ~((1 << am) - 1);
> g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);
> +
> + vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
> }
>
> /* Flush IOTLB
> @@ -1999,15 +2057,37 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion
> *iommu,
> IOMMUNotifierFlag new)
> {
> VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
> + IntelIOMMUState *s = vtd_as->iommu_state;
> + IntelIOMMUNotifierNode *node = NULL;
> + IntelIOMMUNotifierNode *next_node = NULL;
>
> - if (new & IOMMU_NOTIFIER_MAP) {
> - error_report("Device at bus %s addr %02x.%d requires iommu "
> - "notifier which is currently not supported by "
> - "intel-iommu emulation",
> - vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> - PCI_FUNC(vtd_as->devfn));
> + if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
> + error_report("We need to set cache_mode=1 for intel-iommu to enable "
> + "device assignment with IOMMU protection.");
> exit(1);
> }
> +
> + /* Add new ndoe if no mapping was exising before this call */
> + if (old == IOMMU_NOTIFIER_NONE) {
> + node = g_malloc0(sizeof(*node));
> + node->vtd_as = vtd_as;
> + node->notifier_flag = new;
> + QLIST_INSERT_HEAD(&s->notifiers_list, node, next);
> + return;
> + }
> +
> + /* update notifier node with new flags */
> + QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
> + if (node->vtd_as == vtd_as) {
> + if (new == IOMMU_NOTIFIER_NONE) {
> + QLIST_REMOVE(node, next);
> + g_free(node);
> + } else {
> + node->notifier_flag = new;
> + }
> + return;
> + }
> + }
> }
>
> static const VMStateDescription vtd_vmstate = {
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 00cbe0d..c1b9cfc 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -381,6 +381,8 @@ typedef struct VTDIOTLBPageInvInfo VTDIOTLBPageInvInfo;
> #define VTD_PAGE_SHIFT_1G 30
> #define VTD_PAGE_MASK_1G (~((1ULL << VTD_PAGE_SHIFT_1G) - 1))
>
> +#define VTD_PAGE_MASK(shift) (~((1ULL << (shift)) - 1))
> +
> struct VTDRootEntry {
> uint64_t val;
> uint64_t rsvd;
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 749eef9..1a355c1 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -63,6 +63,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
> typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
> typedef struct VTDIrq VTDIrq;
> typedef struct VTD_MSIMessage VTD_MSIMessage;
> +typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
>
> /* Context-Entry */
> struct VTDContextEntry {
> @@ -247,6 +248,12 @@ struct VTD_MSIMessage {
> /* When IR is enabled, all MSI/MSI-X data bits should be zero */
> #define VTD_IR_MSI_DATA (0)
>
> +struct IntelIOMMUNotifierNode {
> + VTDAddressSpace *vtd_as;
> + IOMMUNotifierFlag notifier_flag;
> + QLIST_ENTRY(IntelIOMMUNotifierNode) next;
> +};
> +
> /* The iommu (DMAR) device state struct */
> struct IntelIOMMUState {
> X86IOMMUState x86_iommu;
> @@ -284,6 +291,8 @@ struct IntelIOMMUState {
> MemoryRegionIOMMUOps iommu_ops;
> GHashTable *vtd_as_by_busptr; /* VTDBus objects indexed by PCIBus*
> reference */
> VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by
> bus number */
> + /* list of registered notifiers */
> + QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
>
> /* interrupt remapping */
> bool intr_enabled; /* Whether guest enabled IR */
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers
2016-12-16 9:12 [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers Liu, Yi L
@ 2016-12-19 10:00 ` Peter Xu
2016-12-19 11:53 ` Liu, Yi L
2016-12-19 11:54 ` Liu, Yi L
1 sibling, 1 reply; 8+ messages in thread
From: Peter Xu @ 2016-12-19 10:00 UTC (permalink / raw)
To: Liu, Yi L
Cc: bd.aviv@gmail.com, qemu-devel@nongnu.org, Michael S. Tsirkin,
, Jan Kiszka, , Alex Williamson, , Jason Wang, Lan, Tianyu,
Tian, Kevin
On Fri, Dec 16, 2016 at 09:12:05AM +0000, Liu, Yi L wrote:
> > From: "Aviv Ben-David" <address@hidden>
> >
> > Adds a list of registered vtd_as's to intel iommu state to save
> > iteration over each PCI device in a search of the corrosponding domain.
> >
> > Signed-off-by: Aviv Ben-David <address@hidden>
> > ---
> > hw/i386/intel_iommu.c | 94 ++++++++++++++++++++++++++++++++++++++----
> > hw/i386/intel_iommu_internal.h | 2 +
> > include/hw/i386/intel_iommu.h | 9 ++++
> > 3 files changed, 98 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 05973b9..d872969 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -679,7 +679,7 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t
> > gpa,
> > }
> > *reads = (*reads) && (slpte & VTD_SL_R);
> > *writes = (*writes) && (slpte & VTD_SL_W);
> > - if (!(slpte & access_right_check)) {
> > + if (!(slpte & access_right_check) && !(flags & IOMMU_NO_FAIL)) {
> > VTD_DPRINTF(GENERAL, "error: lack of %s permission for "
> > "gpa 0x%"PRIx64 " slpte 0x%"PRIx64,
> > (flags & IOMMU_WO ? "write" : "read"), gpa, slpte);
> > @@ -978,6 +978,23 @@ static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState
> > *s, uint8_t bus_num)
> > return vtd_bus;
> > }
> >
> > +static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num, uint8_t devfn,
> > + uint16_t *domain_id)
> > +{
> > + VTDContextEntry ce;
> > + int ret_fr;
> > +
> > + assert(domain_id);
> > +
> > + ret_fr = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
> > + if (ret_fr) {
> > + return -1;
> > + }
> > +
> > + *domain_id = VTD_CONTEXT_ENTRY_DID(ce.hi);
> > + return 0;
> > +}
> > +
> > /* Do a context-cache device-selective invalidation.
> > * @func_mask: FM field after shifting
> > */
> > @@ -1064,6 +1081,45 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState
> > *s, uint16_t domain_id)
> > &domain_id);
> > }
> >
> > +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> > + uint16_t domain_id, hwaddr addr,
> > + uint8_t am)
> > +{
> > + IntelIOMMUNotifierNode *node;
> > +
> > + QLIST_FOREACH(node, &(s->notifiers_list), next) {
> Aviv,
>
> Regards to the s->notifiers_list, I didn't see the init op to it. Does it happen
> in another patch? If so, it may be better to move it in this patch since this
> patch introduces both the definition and usage of notifiers_list.
>
> If it is already clarified, then ignore it.
I think it was missing. It IMHO accidentally worked since QLIST_INIT()
just set the head to NULL and that's what we did when we create the
IntelIOMMUState object.
And what's worse - I found this approach may not work if we do
QLIST_INSERT() in the changed() hook, since if we have more than one
assigned devices we will only register the first one not the rest. A
better approach may be traversing the vt-d buses via
IntelIOMMUState.vtd_as_by_busptr.
Thanks,
-- peterx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers
2016-12-19 10:00 ` Peter Xu
@ 2016-12-19 11:53 ` Liu, Yi L
2016-12-19 13:26 ` Peter Xu
0 siblings, 1 reply; 8+ messages in thread
From: Liu, Yi L @ 2016-12-19 11:53 UTC (permalink / raw)
To: Peter Xu
Cc: bd.aviv@gmail.com, qemu-devel@nongnu.org, Michael S. Tsirkin,
, Jan Kiszka, , Alex Williamson, , Jason Wang, Lan, Tianyu,
Tian, Kevin
> -----Original Message-----
> From: Peter Xu [mailto:peterx@redhat.com]
> Sent: Monday, December 19, 2016 6:01 PM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Cc: bd.aviv@gmail.com; qemu-devel@nongnu.org; Michael S. Tsirkin
> <mst@redhat.com>; , Jan Kiszka <jan.kiszka@siemens.com>; , Alex Williamson
> <alex.williamson@redhat.com>; , Jason Wang <jasowang@redhat.com>; Lan,
> Tianyu <tianyu.lan@intel.com>; Tian, Kevin <kevin.tian@intel.com>
> Subject: Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map
> and unmap notifiers
>
> On Fri, Dec 16, 2016 at 09:12:05AM +0000, Liu, Yi L wrote:
> > > From: "Aviv Ben-David" <address@hidden>
> > >
> > > Adds a list of registered vtd_as's to intel iommu state to save
> > > iteration over each PCI device in a search of the corrosponding domain.
> > >
> > > Signed-off-by: Aviv Ben-David <address@hidden>
> > > ---
> > > hw/i386/intel_iommu.c | 94
> ++++++++++++++++++++++++++++++++++++++----
> > > hw/i386/intel_iommu_internal.h | 2 +
> > > include/hw/i386/intel_iommu.h | 9 ++++
> > > 3 files changed, 98 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index 05973b9..d872969 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -679,7 +679,7 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce,
> uint64_t
> > > gpa,
> > > }
> > > *reads = (*reads) && (slpte & VTD_SL_R);
> > > *writes = (*writes) && (slpte & VTD_SL_W);
> > > - if (!(slpte & access_right_check)) {
> > > + if (!(slpte & access_right_check) && !(flags & IOMMU_NO_FAIL)) {
> > > VTD_DPRINTF(GENERAL, "error: lack of %s permission for "
> > > "gpa 0x%"PRIx64 " slpte 0x%"PRIx64,
> > > (flags & IOMMU_WO ? "write" : "read"), gpa, slpte);
> > > @@ -978,6 +978,23 @@ static VTDBus
> *vtd_find_as_from_bus_num(IntelIOMMUState
> > > *s, uint8_t bus_num)
> > > return vtd_bus;
> > > }
> > >
> > > +static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num, uint8_t
> devfn,
> > > + uint16_t *domain_id)
> > > +{
> > > + VTDContextEntry ce;
> > > + int ret_fr;
> > > +
> > > + assert(domain_id);
> > > +
> > > + ret_fr = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
> > > + if (ret_fr) {
> > > + return -1;
> > > + }
> > > +
> > > + *domain_id = VTD_CONTEXT_ENTRY_DID(ce.hi);
> > > + return 0;
> > > +}
> > > +
> > > /* Do a context-cache device-selective invalidation.
> > > * @func_mask: FM field after shifting
> > > */
> > > @@ -1064,6 +1081,45 @@ static void
> vtd_iotlb_domain_invalidate(IntelIOMMUState
> > > *s, uint16_t domain_id)
> > > &domain_id);
> > > }
> > >
> > > +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> > > + uint16_t domain_id, hwaddr addr,
> > > + uint8_t am)
> > > +{
> > > + IntelIOMMUNotifierNode *node;
> > > +
> > > + QLIST_FOREACH(node, &(s->notifiers_list), next) {
> > Aviv,
> >
> > Regards to the s->notifiers_list, I didn't see the init op to it. Does it happen
> > in another patch? If so, it may be better to move it in this patch since this
> > patch introduces both the definition and usage of notifiers_list.
> >
> > If it is already clarified, then ignore it.
>
> I think it was missing. It IMHO accidentally worked since QLIST_INIT()
> just set the head to NULL and that's what we did when we create the
> IntelIOMMUState object.
>
> And what's worse - I found this approach may not work if we do
> QLIST_INSERT() in the changed() hook, since if we have more than one
> assigned devices we will only register the first one not the rest. A
> better approach may be traversing the vt-d buses via
> IntelIOMMUState.vtd_as_by_busptr.
>
Peter,
In Oct, I also mailed Aviv about using IntelIOMMUState.vtd_as_by_busptr
when trying to connect the vfio notifiers(map/unmap) and vIOMMU.
However, I reconsidered it later. If I remember correctly,
IntelIOMMUState.vtd_as_by_busptr not only includes vtd_as for assigned devices,
but also includes virtual devices. When iotlb invalidation comes to vIOMMU, there
is no indication for which device in iotlb_inv_desc. So still need to have a list to record
vtd_as which needs to be looped. So I keep silent on it after that thought.
Now, you mentioned it may not work in multi-assigned scenario. Maybe it's
time to reconsider it again.
Regards,
Yi L
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers
2016-12-16 9:12 [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers Liu, Yi L
2016-12-19 10:00 ` Peter Xu
@ 2016-12-19 11:54 ` Liu, Yi L
1 sibling, 0 replies; 8+ messages in thread
From: Liu, Yi L @ 2016-12-19 11:54 UTC (permalink / raw)
To: 'bd.aviv@gmail.com', 'qemu-devel@nongnu.org'
Cc: 'Michael S. Tsirkin', ', Jan Kiszka',
', Peter Xu', ', Alex Williamson',
', Jason Wang', Lan, Tianyu, Tian, Kevin, Liu, Yi L
> -----Original Message-----
> From: Liu, Yi L
> Sent: Friday, December 16, 2016 5:12 PM
> To: bd.aviv@gmail.com; qemu-devel@nongnu.org
> Cc: Michael S. Tsirkin <mst@redhat.com>; , Jan Kiszka
> <jan.kiszka@siemens.com>; , Peter Xu <peterx@redhat.com>; , Alex Williamson
> <alex.williamson@redhat.com>; , Jason Wang <jasowang@redhat.com>; Lan,
> Tianyu <tianyu.lan@intel.com>; Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L
> <yi.l.liu@intel.com>
> Subject: RE: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map
> and unmap notifiers
>
> > From: "Aviv Ben-David" <address@hidden>
> >
> > Adds a list of registered vtd_as's to intel iommu state to save
> > iteration over each PCI device in a search of the corrosponding domain.
> >
> > Signed-off-by: Aviv Ben-David <address@hidden>
> > ---
> > hw/i386/intel_iommu.c | 94
> ++++++++++++++++++++++++++++++++++++++----
> > hw/i386/intel_iommu_internal.h | 2 +
> > include/hw/i386/intel_iommu.h | 9 ++++
> > 3 files changed, 98 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 05973b9..d872969 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -679,7 +679,7 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce,
> uint64_t
> > gpa,
> > }
> > *reads = (*reads) && (slpte & VTD_SL_R);
> > *writes = (*writes) && (slpte & VTD_SL_W);
> > - if (!(slpte & access_right_check)) {
> > + if (!(slpte & access_right_check) && !(flags & IOMMU_NO_FAIL)) {
> > VTD_DPRINTF(GENERAL, "error: lack of %s permission for "
> > "gpa 0x%"PRIx64 " slpte 0x%"PRIx64,
> > (flags & IOMMU_WO ? "write" : "read"), gpa, slpte);
> > @@ -978,6 +978,23 @@ static VTDBus
> *vtd_find_as_from_bus_num(IntelIOMMUState
> > *s, uint8_t bus_num)
> > return vtd_bus;
> > }
> >
> > +static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num, uint8_t
> devfn,
> > + uint16_t *domain_id)
> > +{
> > + VTDContextEntry ce;
> > + int ret_fr;
> > +
> > + assert(domain_id);
> > +
> > + ret_fr = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
> > + if (ret_fr) {
> > + return -1;
> > + }
> > +
> > + *domain_id = VTD_CONTEXT_ENTRY_DID(ce.hi);
> > + return 0;
> > +}
> > +
> > /* Do a context-cache device-selective invalidation.
> > * @func_mask: FM field after shifting
> > */
> > @@ -1064,6 +1081,45 @@ static void
> vtd_iotlb_domain_invalidate(IntelIOMMUState
> > *s, uint16_t domain_id)
> > &domain_id);
> > }
> >
> > +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> > + uint16_t domain_id, hwaddr addr,
> > + uint8_t am)
> > +{
> > + IntelIOMMUNotifierNode *node;
> > +
> > + QLIST_FOREACH(node, &(s->notifiers_list), next) {
> Aviv,
>
> Regards to the s->notifiers_list, I didn't see the init op to it. Does it happen
> in another patch? If so, it may be better to move it in this patch since this
> patch introduces both the definition and usage of notifiers_list.
>
> If it is already clarified, then ignore it.
>
> Thanks,
> Yi L
> > + VTDAddressSpace *vtd_as = node->vtd_as;
> > + uint16_t vfio_domain_id;
> > + int ret = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus), vtd_as->devfn,
> > + &vfio_domain_id);
> > +
> > + if (!ret && domain_id == vfio_domain_id) {
> > + hwaddr original_addr = addr;
> > +
> > + while (addr < original_addr + (1 << am) * VTD_PAGE_SIZE) {
> > + IOMMUTLBEntry entry = s->iommu_ops.translate(
> > + &node->vtd_as->iommu,
> > + addr,
> > + IOMMU_NO_FAIL);
> > +
> > + if (entry.perm == IOMMU_NONE &&
> > + node->notifier_flag & IOMMU_NOTIFIER_UNMAP) {
> > + entry.target_as = &address_space_memory;
> > + entry.iova = addr & VTD_PAGE_MASK_4K;
> > + entry.translated_addr = 0;
> > + entry.addr_mask = ~VTD_PAGE_MASK(VTD_PAGE_SHIFT);
> > + memory_region_notify_iommu(&node->vtd_as->iommu, entry);
> > + addr += VTD_PAGE_SIZE;
> > + } else if (node->notifier_flag & IOMMU_NOTIFIER_MAP) {
> > + memory_region_notify_iommu(&node->vtd_as->iommu,
> > entry);
> > + addr += entry.addr_mask + 1;
> > + }
> > + }
> > + }
> > + }
> > +}
> > +
> > static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t
> domain_id,
> > hwaddr addr, uint8_t am)
> > {
> > @@ -1074,6 +1130,8 @@ static void
> vtd_iotlb_page_invalidate(IntelIOMMUState *s,
> > uint16_t domain_id,
> > info.addr = addr;
> > info.mask = ~((1 << am) - 1);
> > g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page,
> &info);
> > +
> > + vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
> > }
> >
> > /* Flush IOTLB
> > @@ -1999,15 +2057,37 @@ static void
> vtd_iommu_notify_flag_changed(MemoryRegion
> > *iommu,
> > IOMMUNotifierFlag new)
> > {
> > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace,
> iommu);
> > + IntelIOMMUState *s = vtd_as->iommu_state;
> > + IntelIOMMUNotifierNode *node = NULL;
> > + IntelIOMMUNotifierNode *next_node = NULL;
> >
> > - if (new & IOMMU_NOTIFIER_MAP) {
> > - error_report("Device at bus %s addr %02x.%d requires iommu "
> > - "notifier which is currently not supported by "
> > - "intel-iommu emulation",
> > - vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> > - PCI_FUNC(vtd_as->devfn));
> > + if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
> > + error_report("We need to set cache_mode=1 for intel-iommu to enable
> "
> > + "device assignment with IOMMU protection.");
> > exit(1);
> > }
> > +
> > + /* Add new ndoe if no mapping was exising before this call */
a typo needed. "/* Add new node if no mapping was existing before this call */"
Regards,
Yi L
> > + if (old == IOMMU_NOTIFIER_NONE) {
> > + node = g_malloc0(sizeof(*node));
> > + node->vtd_as = vtd_as;
> > + node->notifier_flag = new;
> > + QLIST_INSERT_HEAD(&s->notifiers_list, node, next);
> > + return;
> > + }
> > +
> > + /* update notifier node with new flags */
> > + QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
> > + if (node->vtd_as == vtd_as) {
> > + if (new == IOMMU_NOTIFIER_NONE) {
> > + QLIST_REMOVE(node, next);
> > + g_free(node);
> > + } else {
> > + node->notifier_flag = new;
> > + }
> > + return;
> > + }
> > + }
> > }
> >
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers
2016-12-19 11:53 ` Liu, Yi L
@ 2016-12-19 13:26 ` Peter Xu
0 siblings, 0 replies; 8+ messages in thread
From: Peter Xu @ 2016-12-19 13:26 UTC (permalink / raw)
To: Liu, Yi L
Cc: bd.aviv@gmail.com, qemu-devel@nongnu.org, Michael S. Tsirkin,
, Jan Kiszka, , Alex Williamson, , Jason Wang, Lan, Tianyu,
Tian, Kevin
On Mon, Dec 19, 2016 at 11:53:32AM +0000, Liu, Yi L wrote:
[...]
> > > Regards to the s->notifiers_list, I didn't see the init op to it. Does it happen
> > > in another patch? If so, it may be better to move it in this patch since this
> > > patch introduces both the definition and usage of notifiers_list.
> > >
> > > If it is already clarified, then ignore it.
> >
> > I think it was missing. It IMHO accidentally worked since QLIST_INIT()
> > just set the head to NULL and that's what we did when we create the
> > IntelIOMMUState object.
> >
> > And what's worse - I found this approach may not work if we do
> > QLIST_INSERT() in the changed() hook, since if we have more than one
> > assigned devices we will only register the first one not the rest. A
> > better approach may be traversing the vt-d buses via
> > IntelIOMMUState.vtd_as_by_busptr.
> >
> Peter,
>
> In Oct, I also mailed Aviv about using IntelIOMMUState.vtd_as_by_busptr
> when trying to connect the vfio notifiers(map/unmap) and vIOMMU.
> However, I reconsidered it later. If I remember correctly,
> IntelIOMMUState.vtd_as_by_busptr not only includes vtd_as for assigned devices,
> but also includes virtual devices. When iotlb invalidation comes to vIOMMU, there
> is no indication for which device in iotlb_inv_desc. So still need to have a list to record
> vtd_as which needs to be looped. So I keep silent on it after that thought.
>
> Now, you mentioned it may not work in multi-assigned scenario. Maybe it's
> time to reconsider it again.
Hmm, first parameter of vtd_iommu_notify_flag_changed() is memory
region, and that's per-device. So current approach should work even
with multiple devices. Looks like I made a mistake, sorry. :)
Thanks,
-- peterx
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-19 13:27 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-16 9:12 [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers Liu, Yi L
2016-12-19 10:00 ` Peter Xu
2016-12-19 11:53 ` Liu, Yi L
2016-12-19 13:26 ` Peter Xu
2016-12-19 11:54 ` Liu, Yi L
-- strict thread matches above, loose matches on Subject: below --
2016-11-28 15:51 [Qemu-devel] [PATCH v7 0/5] IOMMU: intel_iommu support map and unmap notifications Aviv B.D
2016-11-28 15:51 ` [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers Aviv B.D
2016-11-29 3:23 ` 蓝天宇
2016-11-29 7:57 ` Aviv B.D.
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.