Skip to contentSkip to conversation

common/pv-iommu: Add stub hypercall for PV-IOMMU

RFC PATCH In review Malcolm Crossley 5 participants 21 messages Feb 10, 2016 – Nov 22, 2016 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

RFC PATCH 1/7 common/pv-iommu: Add stub hypercall for PV-IOMMU

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/arch/x86/x86_64/compat/entry.S |  2 ++
 xen/arch/x86/x86_64/entry.S        |  2 ++
 xen/common/Makefile                |  1 +
 xen/common/pv_iommu.c              | 38 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/xen.h           |  1 +
 5 files changed, 44 insertions(+)
 create mode 100644 xen/common/pv_iommu.c
xen/arch/x86/x86_64/compat/entry.S +2 -0
index 3088aa7..53a1689 100644
@@ -436,6 +436,7 @@ ENTRY(compat_hypercall_table)
         .quad do_tmem_op
         .quad do_ni_hypercall           /* reserved for XenClient */
         .quad do_xenpmu_op              /* 40 */
+        .quad do_iommu_op
         .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8)
         .quad compat_ni_hypercall
         .endr
@@ -487,6 +488,7 @@ ENTRY(compat_hypercall_args_table)
         .byte 1 /* do_tmem_op               */
         .byte 0 /* reserved for XenClient   */
         .byte 2 /* do_xenpmu_op             */  /* 40 */
+        .byte 2 /* do_iommu_op              */
         .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table)
         .byte 0 /* compat_ni_hypercall      */
         .endr
xen/arch/x86/x86_64/entry.S +2 -0
index 94a54aa..fee7191 100644
@@ -769,6 +769,7 @@ ENTRY(hypercall_table)
         .quad do_tmem_op
         .quad do_ni_hypercall       /* reserved for XenClient */
         .quad do_xenpmu_op          /* 40 */
+        .quad do_iommu_op
         .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8)
         .quad do_ni_hypercall
         .endr
@@ -820,6 +821,7 @@ ENTRY(hypercall_args_table)
         .byte 1 /* do_tmem_op           */
         .byte 0 /* reserved for XenClient */
         .byte 2 /* do_xenpmu_op         */  /* 40 */
+        .byte 2 /* do_iommu_op          */
         .rept __HYPERVISOR_arch_0-(.-hypercall_args_table)
         .byte 0 /* do_ni_hypercall      */
         .endr
xen/common/Makefile +1 -0
index 6e82b33..b498589 100644
@@ -25,6 +25,7 @@ obj-y += notifier.o
 obj-y += page_alloc.o
 obj-$(CONFIG_HAS_PDX) += pdx.o
 obj-y += preempt.o
+obj-y += pv_iommu.o
 obj-y += random.o
 obj-y += rangeset.o
 obj-y += radix-tree.o
xen/common/pv_iommu.c new file +38 -0
new file mode 100644
index 0000000..304fccf
@@ -0,0 +1,38 @@
+/******************************************************************************
+ * common/pv_iommu.c
+ * 
+ * Paravirtualised IOMMU functionality
+ * 
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/guest_access.h>
+
+#define ret_t long
+
+ret_t do_iommu_op(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned int count)
+{
+    return -ENOSYS;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
xen/include/public/xen.h +1 -0
index 7b629b1..ff50e7a 100644
@@ -102,6 +102,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #define __HYPERVISOR_tmem_op              38
 #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
 #define __HYPERVISOR_xenpmu_op            40
+#define __HYPERVISOR_iommu_op             41
 
 /* Architecture-specific hypercall definitions. */
 #define __HYPERVISOR_arch_0               48
Signature · 1 line
1.7.12.4
#2 lore raw

I forgot to CC xen-devel for the cover letter for this series. You can access the cover letter in the xen-devel archives here:
http://lists.xen.org/archives/html/xen-devel/2016-02/msg01441.html

Quoted 126 lines · Malcolm Crossley

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/arch/x86/x86_64/compat/entry.S |  2 ++
 xen/arch/x86/x86_64/entry.S        |  2 ++
 xen/common/Makefile                |  1 +
 xen/common/pv_iommu.c              | 38 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/xen.h           |  1 +
 5 files changed, 44 insertions(+)
 create mode 100644 xen/common/pv_iommu.c
xen/arch/x86/x86_64/compat/entry.S +2 -0
index 3088aa7..53a1689 100644
@@ -436,6 +436,7 @@ ENTRY(compat_hypercall_table)
         .quad do_tmem_op
         .quad do_ni_hypercall           /* reserved for XenClient */
         .quad do_xenpmu_op              /* 40 */
+        .quad do_iommu_op
         .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8)
         .quad compat_ni_hypercall
         .endr
@@ -487,6 +488,7 @@ ENTRY(compat_hypercall_args_table)
         .byte 1 /* do_tmem_op               */
         .byte 0 /* reserved for XenClient   */
         .byte 2 /* do_xenpmu_op             */  /* 40 */
+        .byte 2 /* do_iommu_op              */
         .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table)
         .byte 0 /* compat_ni_hypercall      */
         .endr
xen/arch/x86/x86_64/entry.S +2 -0
index 94a54aa..fee7191 100644
@@ -769,6 +769,7 @@ ENTRY(hypercall_table)
         .quad do_tmem_op
         .quad do_ni_hypercall       /* reserved for XenClient */
         .quad do_xenpmu_op          /* 40 */
+        .quad do_iommu_op
         .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8)
         .quad do_ni_hypercall
         .endr
@@ -820,6 +821,7 @@ ENTRY(hypercall_args_table)
         .byte 1 /* do_tmem_op           */
         .byte 0 /* reserved for XenClient */
         .byte 2 /* do_xenpmu_op         */  /* 40 */
+        .byte 2 /* do_iommu_op          */
         .rept __HYPERVISOR_arch_0-(.-hypercall_args_table)
         .byte 0 /* do_ni_hypercall      */
         .endr
xen/common/Makefile +1 -0
index 6e82b33..b498589 100644
@@ -25,6 +25,7 @@ obj-y += notifier.o
 obj-y += page_alloc.o
 obj-$(CONFIG_HAS_PDX) += pdx.o
 obj-y += preempt.o
+obj-y += pv_iommu.o
 obj-y += random.o
 obj-y += rangeset.o
 obj-y += radix-tree.o
xen/common/pv_iommu.c new file +38 -0
new file mode 100644
index 0000000..304fccf
@@ -0,0 +1,38 @@
+/******************************************************************************
+ * common/pv_iommu.c
+ * 
+ * Paravirtualised IOMMU functionality
+ * 
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/guest_access.h>
+
+#define ret_t long
+
+ret_t do_iommu_op(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned int count)
+{
+    return -ENOSYS;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
xen/include/public/xen.h +1 -0
index 7b629b1..ff50e7a 100644
@@ -102,6 +102,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #define __HYPERVISOR_tmem_op              38
 #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
 #define __HYPERVISOR_xenpmu_op            40
+#define __HYPERVISOR_iommu_op             41
 
 /* Architecture-specific hypercall definitions. */
 #define __HYPERVISOR_arch_0               48
#3 lore raw
In reply to Malcolm Crossley
Quoted 62 lines · Malcolm Crossley

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/arch/x86/x86_64/compat/entry.S |  2 ++
 xen/arch/x86/x86_64/entry.S        |  2 ++
 xen/common/Makefile                |  1 +
 xen/common/pv_iommu.c              | 38 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/xen.h           |  1 +
 5 files changed, 44 insertions(+)
 create mode 100644 xen/common/pv_iommu.c
xen/arch/x86/x86_64/compat/entry.S +2 -0
index 3088aa7..53a1689 100644
@@ -436,6 +436,7 @@ ENTRY(compat_hypercall_table)
         .quad do_tmem_op
         .quad do_ni_hypercall           /* reserved for XenClient */
         .quad do_xenpmu_op              /* 40 */
+        .quad do_iommu_op
         .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8)
         .quad compat_ni_hypercall
         .endr
@@ -487,6 +488,7 @@ ENTRY(compat_hypercall_args_table)
         .byte 1 /* do_tmem_op               */
         .byte 0 /* reserved for XenClient   */
         .byte 2 /* do_xenpmu_op             */  /* 40 */
+        .byte 2 /* do_iommu_op              */
         .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table)
         .byte 0 /* compat_ni_hypercall      */
         .endr
xen/arch/x86/x86_64/entry.S +2 -0
index 94a54aa..fee7191 100644
@@ -769,6 +769,7 @@ ENTRY(hypercall_table)
         .quad do_tmem_op
         .quad do_ni_hypercall       /* reserved for XenClient */
         .quad do_xenpmu_op          /* 40 */
+        .quad do_iommu_op
         .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8)
         .quad do_ni_hypercall
         .endr
@@ -820,6 +821,7 @@ ENTRY(hypercall_args_table)
         .byte 1 /* do_tmem_op           */
         .byte 0 /* reserved for XenClient */
         .byte 2 /* do_xenpmu_op         */  /* 40 */
+        .byte 2 /* do_iommu_op          */
         .rept __HYPERVISOR_arch_0-(.-hypercall_args_table)
         .byte 0 /* do_ni_hypercall      */
         .endr
xen/common/Makefile +0 -0
index 6e82b33..b498589 100644

xen/common/Makefile obj-y += notifier.o · line 28

Quoted 4 lines
xen/common/Makefile +1 -0
@@ -25,6 +25,7 @@ obj-y += notifier.o
 obj-y += page_alloc.o
 obj-$(CONFIG_HAS_PDX) += pdx.o
 obj-y += preempt.o
+obj-y += pv_iommu.o

Perhaps have an Kconfig entry for it?

Also you seem to be missing ARM code?

Quoted 21 lines
xen/common/Makefile +0 -0
 obj-y += random.o
 obj-y += rangeset.o
 obj-y += radix-tree.o
xen/common/pv_iommu.c new file +12 -0
new file mode 100644
index 0000000..304fccf
@@ -0,0 +1,38 @@
+/******************************************************************************
+ * common/pv_iommu.c
+ * 
+ * Paravirtualised IOMMU functionality
+ * 
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

xen/common/pv_iommu.c line 22

Quoted 10 lines
xen/common/pv_iommu.c new file +10 -0
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/guest_access.h>
+
+#define ret_t long

? What is wrong with just using 'long'?

Quoted 21 lines
xen/common/pv_iommu.c new file +16 -0
+
+ret_t do_iommu_op(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned int count)
+{
+    return -ENOSYS;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
xen/include/public/xen.h +0 -0
index 7b629b1..ff50e7a 100644

xen/include/public/xen.h DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); · line 105

Quoted 4 lines
xen/include/public/xen.h +1 -0
@@ -102,6 +102,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #define __HYPERVISOR_tmem_op              38
 #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
 #define __HYPERVISOR_xenpmu_op            40
+#define __HYPERVISOR_iommu_op             41

I would think there would be an pv_iommu.h header file as well?

Quoted 11 lines
xen/include/public/xen.h +0 -0
 /* Architecture-specific hypercall definitions. */
 #define __HYPERVISOR_arch_0               48
Signature · 7 lines
1.7.12.4


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
#4 lore raw

RFC PATCH 2/7 iommu: add iommu_lookup_page to lookup guest gfn for a particular IOMMU mapping

In reply to a message not in the archive

If IOMMU driver does not implement lookup_page function then it returns -ENOMEM.

Returns 0 on success and any other value on failure.

--
Cc: [email protected]
Cc: [email protected]

---
 xen/drivers/passthrough/iommu.c | 21 +++++++++++++++++++++
 xen/include/xen/iommu.h         |  2 ++
 2 files changed, 23 insertions(+)
xen/drivers/passthrough/iommu.c +21 -0
index 0b2abf4..06f21ee 100644
@@ -257,6 +257,27 @@ int iommu_unmap_page(struct domain *d, unsigned long gfn)
     return hd->platform_ops->unmap_page(d, gfn);
 }
 
+int iommu_lookup_page(struct domain *d, unsigned long bfn, unsigned long *gfn)
+{
+    struct hvm_iommu *hd = domain_hvm_iommu(d);
+
+    /* 
+     * BFN maps 1:1 to GFN when iommu passthrough is enabled or 
+     * when IOMMU shared page tables is in use
+     */
+    if ( iommu_use_hap_pt(d) || (iommu_passthrough && is_hardware_domain(d)) )
+    {
+        *gfn = bfn;
+        return 0;
+    }
+
+    if ( !iommu_enabled || !hd->platform_ops ||
+            !hd->platform_ops->lookup_page )
+        return -ENOMEM;
+
+    return hd->platform_ops->lookup_page(d, bfn, gfn);
+}
+
 static void iommu_free_pagetables(unsigned long unused)
 {
     do {
xen/include/xen/iommu.h +2 -0
index 8217cb7..49ca087 100644
@@ -77,6 +77,7 @@ void iommu_teardown(struct domain *d);
 int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
                    unsigned int flags);
 int iommu_unmap_page(struct domain *d, unsigned long gfn);
+int iommu_lookup_page(struct domain *d, unsigned long bfn, unsigned long *gfn);
 
 enum iommu_feature
 {
@@ -151,6 +152,7 @@ struct iommu_ops {
     int (*map_page)(struct domain *d, unsigned long gfn, unsigned long mfn,
                     unsigned int flags);
     int (*unmap_page)(struct domain *d, unsigned long gfn);
+    int (*lookup_page)(struct domain *d, unsigned long bfn, unsigned long *gfn);
     void (*free_page_table)(struct page_info *);
 #ifdef CONFIG_X86
     void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
Signature · 1 line
1.7.12.4
#5 lore raw

Re: [RFC PATCH 2/7] iommu: add iommu_lookup_page to lookup guest gfn for a particular IOMMU mapping

Quoted 1 line · Malcolm Crossley

If IOMMU driver does not implement lookup_page function then it returns -ENOMEM.

That is a very odd return code. Could you explain why -ENOMEM?

Quoted 17 lines

Returns 0 on success and any other value on failure.

--
Cc: [email protected]
Cc: [email protected]

---
 xen/drivers/passthrough/iommu.c | 21 +++++++++++++++++++++
 xen/include/xen/iommu.h         |  2 ++
 2 files changed, 23 insertions(+)
xen/drivers/passthrough/iommu.c +0 -0
index 0b2abf4..06f21ee 100644

xen/drivers/passthrough/iommu.c int iommu_unmap_page(struct domain *d, unsigned long gfn) · line 266

Quoted 10 lines
xen/drivers/passthrough/iommu.c +7 -0
@@ -257,6 +257,27 @@ int iommu_unmap_page(struct domain *d, unsigned long gfn)
     return hd->platform_ops->unmap_page(d, gfn);
 }
 
+int iommu_lookup_page(struct domain *d, unsigned long bfn, unsigned long *gfn)
+{
+    struct hvm_iommu *hd = domain_hvm_iommu(d);
+
+    /* 
+     * BFN maps 1:1 to GFN when iommu passthrough is enabled or 
+     * when IOMMU shared page tables is in use

Missing full stop.

xen/drivers/passthrough/iommu.c int iommu_unmap_page(struct domain *d, unsigned long gfn) · line 268

Quoted 2 lines
xen/drivers/passthrough/iommu.c +2 -0
+     */
+    if ( iommu_use_hap_pt(d) || (iommu_passthrough && is_hardware_domain(d)) )

The comment is not in sync with the code. But what I am curious - if the dom0 is PVH and the shared EPT is disabled, what path should we take?

I would think we would need to skip this and go to the ->lookup_page?

xen/drivers/passthrough/iommu.c int iommu_unmap_page(struct domain *d, unsigned long gfn) · line 276

Quoted 8 lines
xen/drivers/passthrough/iommu.c +8 -0
+    {
+        *gfn = bfn;
+        return 0;
+    }
+
+    if ( !iommu_enabled || !hd->platform_ops ||
+            !hd->platform_ops->lookup_page )
+        return -ENOMEM;

-ENOMEM ? ENXIO ? Or maybe -ENOSYS for the case when hd->platform_ops is not set nor ->lookup_page?

Quoted 35 lines
xen/drivers/passthrough/iommu.c +4 -0
+
+    return hd->platform_ops->lookup_page(d, bfn, gfn);
+}
+
 static void iommu_free_pagetables(unsigned long unused)
 {
     do {
xen/include/xen/iommu.h +2 -0
index 8217cb7..49ca087 100644
@@ -77,6 +77,7 @@ void iommu_teardown(struct domain *d);
 int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
                    unsigned int flags);
 int iommu_unmap_page(struct domain *d, unsigned long gfn);
+int iommu_lookup_page(struct domain *d, unsigned long bfn, unsigned long *gfn);
 
 enum iommu_feature
 {
@@ -151,6 +152,7 @@ struct iommu_ops {
     int (*map_page)(struct domain *d, unsigned long gfn, unsigned long mfn,
                     unsigned int flags);
     int (*unmap_page)(struct domain *d, unsigned long gfn);
+    int (*lookup_page)(struct domain *d, unsigned long bfn, unsigned long *gfn);
     void (*free_page_table)(struct page_info *);
 #ifdef CONFIG_X86
     void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
Signature · 7 lines
1.7.12.4


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
#6 lore raw

Re: [RFC PATCH 2/7] iommu: add iommu_lookup_page to lookup guest gfn for a particular IOMMU mapping

Quoted 11 lines · Konrad Rzeszutek Wilk

xen/drivers/passthrough/iommu.c int iommu_unmap_page(struct domain *d, unsigned long gfn) · line 276

Quoted 8 lines
xen/drivers/passthrough/iommu.c +8 -0
+    {
+        *gfn = bfn;
+        return 0;
+    }
+
+    if ( !iommu_enabled || !hd->platform_ops ||
+            !hd->platform_ops->lookup_page )
+        return -ENOMEM;

-ENOMEM ? ENXIO ? Or maybe -ENOSYS for the case when hd->platform_ops is not set nor ->lookup_page?

I think ENOTSUPP, perhaps? ENOMEM means a memory allocation failed (which it hasn't); ENOSYS means this is an invalid system call (which it isn't). ESRCH could be close too, but it seems like that would be more appropriate when you try to lookup a non-existent BFN.

-George

#7 lore raw

Re: [RFC PATCH 2/7] iommu: add iommu_lookup_page to lookup guest gfn for a particular IOMMU mapping

Quoted 10 lines · George Dunlap
Quoted 8 lines · Konrad Rzeszutek Wilk
Quoted 8 lines
xen/drivers/passthrough/iommu.c +8 -0
+    {
+        *gfn = bfn;
+        return 0;
+    }
+
+    if ( !iommu_enabled || !hd->platform_ops ||
+            !hd->platform_ops->lookup_page )
+        return -ENOMEM;
Quoted 6 lines
Quoted 2 lines

-ENOMEM ? ENXIO ? Or maybe -ENOSYS for the case when hd->platform_ops is not set nor ->lookup_page?

I think ENOTSUPP, perhaps? ENOMEM means a memory allocation failed (which it hasn't); ENOSYS means this is an invalid system call (which it isn't). ESRCH could be close too, but it seems like that would be more appropriate when you try to lookup a non-existent BFN.

Xen (fairly) consistently uses ESRCH for "domain not found". I would recommend -ENOTSUPP here.

~Andrew

#8 lore raw

RFC PATCH 3/7 VT-d: Add iommu_lookup_page support

In reply to a message not in the archive

Function does not need to handle shared EPT use of IOMMU as core code already handles this.

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/drivers/passthrough/vtd/iommu.c | 31 +++++++++++++++++++++++++++++++
 xen/drivers/passthrough/vtd/iommu.h |  1 +
 2 files changed, 32 insertions(+)
xen/drivers/passthrough/vtd/iommu.c +31 -0
index ec31c6b..0c79b48 100644
@@ -1754,6 +1754,36 @@ static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
     return 0;
 }
 
+static int intel_iommu_lookup_page(
+    struct domain *d, unsigned long gfn, unsigned long *mfn)
+{
+    struct hvm_iommu *hd = domain_hvm_iommu(d);
+    struct dma_pte *page = NULL, *pte = NULL, old;
+    u64 pg_maddr;
+
+    spin_lock(&hd->arch.mapping_lock);
+
+    pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << PAGE_SHIFT_4K, 1);
+    if ( pg_maddr == 0 )
+    {
+        spin_unlock(&hd->arch.mapping_lock);
+        return -ENOMEM;
+    }
+    page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
+    pte = page + (gfn & LEVEL_MASK);
+    old = *pte;
+    if (!dma_pte_present(old)) {
+        unmap_vtd_domain_page(page);
+        spin_unlock(&hd->arch.mapping_lock);
+        return -ENOMEM;
+    }
+    unmap_vtd_domain_page(page);
+    spin_unlock(&hd->arch.mapping_lock);
+
+    *mfn = dma_get_pte_addr(old) >> PAGE_SHIFT_4K;
+    return 0;
+}
+
 void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
                      int order, int present)
 {
@@ -2534,6 +2564,7 @@ const struct iommu_ops intel_iommu_ops = {
     .teardown = iommu_domain_teardown,
     .map_page = intel_iommu_map_page,
     .unmap_page = intel_iommu_unmap_page,
+    .lookup_page = intel_iommu_lookup_page,
     .free_page_table = iommu_free_page_table,
     .reassign_device = reassign_device_ownership,
     .get_device_group_id = intel_iommu_group_id,
xen/drivers/passthrough/vtd/iommu.h +1 -0
index c55ee08..03583ef 100644
@@ -275,6 +275,7 @@ struct dma_pte {
 #define dma_pte_addr(p) ((p).val & PADDR_MASK & PAGE_MASK_4K)
 #define dma_set_pte_addr(p, addr) do {\
             (p).val |= ((addr) & PAGE_MASK_4K); } while (0)
+#define dma_get_pte_addr(p) (((p).val & PAGE_MASK_4K))
 #define dma_pte_present(p) (((p).val & DMA_PTE_PROT) != 0)
 #define dma_pte_superpage(p) (((p).val & DMA_PTE_SP) != 0)
 
Signature · 1 line
1.7.12.4
#9 lore raw

Re: [RFC PATCH 3/7] VT-d: Add iommu_lookup_page support

Quoted 2 lines · Malcolm Crossley

Function does not need to handle shared EPT use of IOMMU as core code already handles this.

Could you mention which part of 'core code' handles this?

Also you may want to say this code can only deal with 4K pages but not with larger ones.

Quoted 34 lines

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/drivers/passthrough/vtd/iommu.c | 31 +++++++++++++++++++++++++++++++
 xen/drivers/passthrough/vtd/iommu.h |  1 +
 2 files changed, 32 insertions(+)
xen/drivers/passthrough/vtd/iommu.c +15 -0
index ec31c6b..0c79b48 100644
@@ -1754,6 +1754,36 @@ static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
     return 0;
 }
 
+static int intel_iommu_lookup_page(
+    struct domain *d, unsigned long gfn, unsigned long *mfn)
+{
+    struct hvm_iommu *hd = domain_hvm_iommu(d);
+    struct dma_pte *page = NULL, *pte = NULL, old;
+    u64 pg_maddr;
+
+    spin_lock(&hd->arch.mapping_lock);
+
+    pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << PAGE_SHIFT_4K, 1);
+    if ( pg_maddr == 0 )
+    {
+        spin_unlock(&hd->arch.mapping_lock);
+        return -ENOMEM;
+    }

xen/drivers/passthrough/vtd/iommu.c static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn) · line 1781

Quoted 10 lines
xen/drivers/passthrough/vtd/iommu.c +10 -0
+    page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
+    pte = page + (gfn & LEVEL_MASK);
+    old = *pte;
+    if (!dma_pte_present(old)) {
+        unmap_vtd_domain_page(page);
+        spin_unlock(&hd->arch.mapping_lock);
+        return -ENOMEM;
+    }
+    unmap_vtd_domain_page(page);
+    spin_unlock(&hd->arch.mapping_lock);

All this code looks close to lookup routine in dma_pte_clear_one and intel_iommu_map_page.

Could you move most of this lookup code in a static function that your code and the other ones could call?

Quoted 36 lines
xen/drivers/passthrough/vtd/iommu.c +6 -0
+
+    *mfn = dma_get_pte_addr(old) >> PAGE_SHIFT_4K;
+    return 0;
+}
+
 void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
                      int order, int present)
 {
@@ -2534,6 +2564,7 @@ const struct iommu_ops intel_iommu_ops = {
     .teardown = iommu_domain_teardown,
     .map_page = intel_iommu_map_page,
     .unmap_page = intel_iommu_unmap_page,
+    .lookup_page = intel_iommu_lookup_page,
     .free_page_table = iommu_free_page_table,
     .reassign_device = reassign_device_ownership,
     .get_device_group_id = intel_iommu_group_id,
xen/drivers/passthrough/vtd/iommu.h +1 -0
index c55ee08..03583ef 100644
@@ -275,6 +275,7 @@ struct dma_pte {
 #define dma_pte_addr(p) ((p).val & PADDR_MASK & PAGE_MASK_4K)
 #define dma_set_pte_addr(p, addr) do {\
             (p).val |= ((addr) & PAGE_MASK_4K); } while (0)
+#define dma_get_pte_addr(p) (((p).val & PAGE_MASK_4K))
 #define dma_pte_present(p) (((p).val & DMA_PTE_PROT) != 0)
 #define dma_pte_superpage(p) (((p).val & DMA_PTE_SP) != 0)
 
Signature · 7 lines
1.7.12.4


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
#10 lore raw

RFC PATCH 4/7 common/pv-iommu: Add query, map and unmap ops

In reply to a message not in the archive

Implement above ops according to PV-IOMMU design draft D.

Currently restricted to hardware domains only due to RFC status.

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/common/pv_iommu.c         | 228 +++++++++++++++++++++++++++++++++++++++++-
 xen/include/public/pv-iommu.h |  69 +++++++++++++
 2 files changed, 296 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/public/pv-iommu.h
xen/common/pv_iommu.c +227 -1
index 304fccf..91485f3 100644
@@ -17,13 +17,239 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <asm/p2m.h>
+#include <asm/event.h>
 #include <xen/guest_access.h>
+#include <public/pv-iommu.h>
 
 #define ret_t long
 
+static int get_paged_frame(unsigned long gfn, unsigned long *frame,
+                           struct page_info **page, int readonly,
+                           struct domain *rd)
+{
+    int rc = 0;
+#if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
+    p2m_type_t p2mt;
+
+    *page = get_page_from_gfn(rd, gfn, &p2mt,
+                             (readonly) ? P2M_ALLOC : P2M_UNSHARE);
+    if ( !(*page) )
+    {
+        *frame = INVALID_MFN;
+        if ( p2m_is_shared(p2mt) )
+            return -EIO;
+        if ( p2m_is_paging(p2mt) )
+        {
+            p2m_mem_paging_populate(rd, gfn);
+            return -EIO;
+        }
+        return -EIO;
+    }
+    *frame = page_to_mfn(*page);
+#else
+    *frame = gmfn_to_mfn(rd, gfn);
+    *page = mfn_valid(*frame) ? mfn_to_page(*frame) : NULL;
+    if ( (!(page)) || (!get_page*page, rd) )
+    {
+        *frame = INVALID_MFN;
+        *page = NULL;
+        rc = -EIO;
+    }
+#endif
+
+    return rc;
+}
+
+int can_use_iommu_check(struct domain *d)
+{
+    if ( !iommu_enabled || (!is_hardware_domain(d) && !need_iommu(d)) )
+        return 0;
+
+    if ( is_hardware_domain(d) && iommu_passthrough )
+        return 0;
+
+    if ( !domain_hvm_iommu(d)->platform_ops->lookup_page )
+        return 0;
+
+    return 1;
+}
+
+void do_iommu_sub_op(struct pv_iommu_op *op)
+{
+    struct domain *d = current->domain;
+    struct domain *rd = NULL;
+
+    /* Only order 0 pages supported */
+    if ( IOMMU_get_page_order(op->flags) != 0 )
+    {
+        op->status = -ENOSPC;
+        goto finish;
+    }
+
+    switch ( op->subop_id )
+    {
+        case IOMMUOP_query_caps:
+        {
+            op->flags = 0;
+            op->status = 0;
+            if ( can_use_iommu_check(d) )
+                op->flags |= IOMMU_QUERY_map_cap;
+
+            break;
+        }
+        case IOMMUOP_map_page:
+        {
+            unsigned long mfn, tmp;
+            unsigned int flags = 0;
+            struct page_info *page = NULL;
+
+            /* Check if calling domain can create IOMMU mappings */
+            if ( !can_use_iommu_check(d) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Check we are the owner of the page */
+            if ( !is_hardware_domain(d) &&
+                 ( maddr_get_owner(op->u.map_page.gfn) != d ) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Lookup page struct backing gfn */
+            if ( is_hardware_domain(d) &&
+                (op->flags & IOMMU_MAP_OP_no_ref_cnt) )
+            {
+                mfn = op->u.map_page.gfn;
+                page = mfn_to_page(mfn);
+                if (!page)
+                {
+                    op->status = -EPERM;
+                    goto finish;
+                }
+            } else if ( get_paged_frame(op->u.map_page.gfn, &mfn, &page, 0, d) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Check for conflict with existing BFN mappings */
+            if ( !iommu_lookup_page(d, op->u.map_page.bfn, &tmp) )
+            {
+                if ( !(op->flags & IOMMU_MAP_OP_no_ref_cnt) )
+                    put_page(page);
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            if ( op->flags & IOMMU_OP_readable )
+                flags |= IOMMUF_readable;
+
+            if ( op->flags & IOMMU_OP_writeable )
+                flags |= IOMMUF_writable;
+
+            if ( iommu_map_page(d, op->u.map_page.bfn, mfn, flags) )
+            {
+                if ( !(op->flags & IOMMU_MAP_OP_no_ref_cnt) )
+                    put_page(page);
+                op->status = -EIO;
+                goto finish;
+            }
+
+            op->status = 0;
+            break;
+        }
+
+        case IOMMUOP_unmap_page:
+        {
+            unsigned long mfn;
+
+            /* Check if there is a valid BFN mapping for this domain */
+            if ( iommu_lookup_page(d, op->u.unmap_page.bfn, &mfn) )
+            {
+                op->status = -ENOENT;
+                goto finish;
+            }
+
+            if ( iommu_unmap_page(d, op->u.unmap_page.bfn) )
+            {
+                op->status = -EIO;
+                goto finish;
+            }
+
+            op->status = 0;
+            break;
+        }
+
+        default:
+            op->status = -ENODEV;
+            break;
+    }
+
+finish:
+    if ( rd )
+        rcu_unlock_domain(rd);
+
+    return;
+}
+
 ret_t do_iommu_op(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned int count)
 {
-    return -ENOSYS;
+    ret_t ret = 0;
+    int i;
+    struct pv_iommu_op op;
+    struct domain *d = current->domain;
+
+    if ( !is_hardware_domain(d) )
+        return -ENOSYS;
+
+    if ( (int)count < 0 )
+        return -EINVAL;
+
+    if ( count > 1 )
+        this_cpu(iommu_dont_flush_iotlb) = 1;
+
+    for ( i = 0; i < count; i++ )
+    {
+        if ( i && hypercall_preempt_check() )
+        {
+            ret =  i;
+            goto flush_pages;
+        }
+        if ( unlikely(__copy_from_guest_offset(&op, arg, i, 1)) )
+        {
+            ret = -EFAULT;
+            goto flush_pages;
+        }
+        do_iommu_sub_op(&op);
+        if ( unlikely(__copy_to_guest_offset(arg, i, &op, 1)) )
+        {
+            ret = -EFAULT;
+            goto flush_pages;
+        }
+    }
+
+flush_pages:
+    if ( ret > 0 )
+    {
+        XEN_GUEST_HANDLE_PARAM(pv_iommu_op_t) op =
+            guest_handle_cast(arg, pv_iommu_op_t);
+        ASSERT(ret < count);
+        guest_handle_add_offset(op, i);
+        arg = guest_handle_cast(op, void);
+        ret = hypercall_create_continuation(__HYPERVISOR_iommu_op,
+                                           "hi", arg, count - i);
+    }
+    if ( count > 1 )
+    {
+        this_cpu(iommu_dont_flush_iotlb) = 0;
+        if ( i )
+            iommu_iotlb_flush_all(d);
+    }
+    return ret;
 }
 
 /*
xen/include/public/pv-iommu.h new file +69 -0

69 lines not on this page · raw message

Signature · 1 line
1.7.12.4
#11 lore raw

Re: [RFC PATCH 4/7] common/pv-iommu: Add query, map and unmap ops

Quoted 1 line · Malcolm Crossley

Implement above ops according to PV-IOMMU design draft D.

.. which would be great if they were part of this patch series and you could just: in docs/misc/blah.

Quoted 2 lines

Currently restricted to hardware domains only due to RFC status.

Um, .. that is not reassuring. Perhaps the design document (which would be in the patchset) would iterate the rest of TODOs?

Quoted 19 lines

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/common/pv_iommu.c         | 228 +++++++++++++++++++++++++++++++++++++++++-
 xen/include/public/pv-iommu.h |  69 +++++++++++++
 2 files changed, 296 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/public/pv-iommu.h
xen/common/pv_iommu.c +0 -0
index 304fccf..91485f3 100644

xen/common/pv_iommu.c line 21

Quoted 5 lines
xen/common/pv_iommu.c +2 -0
@@ -17,13 +17,239 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <asm/p2m.h>
+#include <asm/event.h>

Can those be sorted?

Quoted 40 lines
xen/common/pv_iommu.c +36 -0
 #include <xen/guest_access.h>
+#include <public/pv-iommu.h>
 
 #define ret_t long
 
+static int get_paged_frame(unsigned long gfn, unsigned long *frame,
+                           struct page_info **page, int readonly,
+                           struct domain *rd)
+{
+    int rc = 0;
+#if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
+    p2m_type_t p2mt;
+
+    *page = get_page_from_gfn(rd, gfn, &p2mt,
+                             (readonly) ? P2M_ALLOC : P2M_UNSHARE);
+    if ( !(*page) )
+    {
+        *frame = INVALID_MFN;
+        if ( p2m_is_shared(p2mt) )
+            return -EIO;
+        if ( p2m_is_paging(p2mt) )
+        {
+            p2m_mem_paging_populate(rd, gfn);
+            return -EIO;
+        }
+        return -EIO;
+    }
+    *frame = page_to_mfn(*page);
+#else
+    *frame = gmfn_to_mfn(rd, gfn);
+    *page = mfn_valid(*frame) ? mfn_to_page(*frame) : NULL;
+    if ( (!(page)) || (!get_page*page, rd) )
+    {
+        *frame = INVALID_MFN;
+        *page = NULL;
+        rc = -EIO;
+    }
+#endif
+
+    return rc;

xen/common/pv_iommu.c line 71

Quoted 10 lines
xen/common/pv_iommu.c +10 -0
+}
+
+int can_use_iommu_check(struct domain *d)
+{
+    if ( !iommu_enabled || (!is_hardware_domain(d) && !need_iommu(d)) )
+        return 0;
+
+    if ( is_hardware_domain(d) && iommu_passthrough )
+        return 0;
+

What if a PV guests calls this hypercall? Won't it crash on platform_ops derefence?

xen/common/pv_iommu.c line 83

Quoted 12 lines
xen/common/pv_iommu.c +12 -0
+    if ( !domain_hvm_iommu(d)->platform_ops->lookup_page )
+        return 0;
+
+    return 1;
+}
+
+void do_iommu_sub_op(struct pv_iommu_op *op)
+{
+    struct domain *d = current->domain;
+    struct domain *rd = NULL;
+
+    /* Only order 0 pages supported */

Missing full stop.

xen/common/pv_iommu.c line 86

Quoted 3 lines
xen/common/pv_iommu.c +3 -0
+    if ( IOMMU_get_page_order(op->flags) != 0 )
+    {
+        op->status = -ENOSPC;

Not ENOSYS? or EINVAL?

xen/common/pv_iommu.c line 97

Quoted 11 lines
xen/common/pv_iommu.c +11 -0
+        goto finish;
+    }
+
+    switch ( op->subop_id )
+    {
+        case IOMMUOP_query_caps:
+        {
+            op->flags = 0;
+            op->status = 0;
+            if ( can_use_iommu_check(d) )
+                op->flags |= IOMMU_QUERY_map_cap;

s/|=/=/ ?

xen/common/pv_iommu.c line 103

Quoted 6 lines
xen/common/pv_iommu.c +6 -0
+
+            break;
+        }
+        case IOMMUOP_map_page:
+        {
+            unsigned long mfn, tmp;

mfn_t pls.

xen/common/pv_iommu.c line 104

Quoted 1 line
xen/common/pv_iommu.c +1 -0
+            unsigned int flags = 0;

Not uint16_t ?
[edit: ah, this is for iommu_map_page. You may want to mention that]

xen/common/pv_iommu.c line 107

Quoted 3 lines
xen/common/pv_iommu.c +3 -0
+            struct page_info *page = NULL;
+
+            /* Check if calling domain can create IOMMU mappings */

Missing full stop.

xen/common/pv_iommu.c line 114

Quoted 7 lines
xen/common/pv_iommu.c +7 -0
+            if ( !can_use_iommu_check(d) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Check we are the owner of the page */

Full stop missing.

xen/common/pv_iommu.c line 122

Quoted 8 lines
xen/common/pv_iommu.c +8 -0
+            if ( !is_hardware_domain(d) &&
+                 ( maddr_get_owner(op->u.map_page.gfn) != d ) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Lookup page struct backing gfn */

Stop full missing.

xen/common/pv_iommu.c line 128

Quoted 6 lines
xen/common/pv_iommu.c +6 -0
+            if ( is_hardware_domain(d) &&
+                (op->flags & IOMMU_MAP_OP_no_ref_cnt) )
+            {
+                mfn = op->u.map_page.gfn;
+                page = mfn_to_page(mfn);
+                if (!page)

Wrong syntax. if ( !page )

xen/common/pv_iommu.c line 133

Quoted 5 lines
xen/common/pv_iommu.c +5 -0
+                {
+                    op->status = -EPERM;
+                    goto finish;
+                }
+            } else if ( get_paged_frame(op->u.map_page.gfn, &mfn, &page, 0, d) )

I think the syntax waants the 'else if' on its own line.

You may want to put a comment around the 0 value.

xen/common/pv_iommu.c line 139

Quoted 6 lines
xen/common/pv_iommu.c +6 -0
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Check for conflict with existing BFN mappings */

Stop missing full.

xen/common/pv_iommu.c line 142

Quoted 3 lines
xen/common/pv_iommu.c +3 -0
+            if ( !iommu_lookup_page(d, op->u.map_page.bfn, &tmp) )
+            {
+                if ( !(op->flags & IOMMU_MAP_OP_no_ref_cnt) )

This check is bit weak. The guest (not hardware domain) could set this flag for fun and the code above would be OK with it.

AS your your check for IOMMU_MAP_OP_no_ref_cnt just tries to do an get_paged_Frame.

You may just want to have some sanity checking on the flags at the start of this code?

xen/common/pv_iommu.c line 153

Quoted 11 lines
xen/common/pv_iommu.c +11 -0
+                    put_page(page);
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            if ( op->flags & IOMMU_OP_readable )
+                flags |= IOMMUF_readable;
+
+            if ( op->flags & IOMMU_OP_writeable )
+                flags |= IOMMUF_writable;
+

What if neither option is set? Should we disallow that?

xen/common/pv_iommu.c line 157

Quoted 4 lines
xen/common/pv_iommu.c +4 -0
+            if ( iommu_map_page(d, op->u.map_page.bfn, mfn, flags) )
+            {
+                if ( !(op->flags & IOMMU_MAP_OP_no_ref_cnt) )
+                    put_page(page);

Again, the non hardware domain could set this.

xen/common/pv_iommu.c line 168

Quoted 11 lines
xen/common/pv_iommu.c +11 -0
+                op->status = -EIO;
+                goto finish;
+            }
+
+            op->status = 0;
+            break;
+        }
+
+        case IOMMUOP_unmap_page:
+        {
+            unsigned long mfn;

mfn_t.

xen/common/pv_iommu.c line 170

Quoted 2 lines
xen/common/pv_iommu.c +2 -0
+
+            /* Check if there is a valid BFN mapping for this domain */

Full .. you know what I am going to say.

xen/common/pv_iommu.c line 171

Quoted 1 line
xen/common/pv_iommu.c +1 -0
+            if ( iommu_lookup_page(d, op->u.unmap_page.bfn, &mfn) )

Wait a minute. Just like that? Don't you want to do some sanity checks?

What if the guest decided for fun to do:
for ( long i = -1; i ; i--)

	hypercall(IOMMUOP_unmap_page, i);

It would naturally fail but we would be taking the hd->Arch.mapping_lock quite a lot! PErhaps do the check (similar to how you have in the mapping code?)

Maybe even move the checks in their own function?

xen/common/pv_iommu.c line 179

Quoted 8 lines
xen/common/pv_iommu.c +8 -0
+            {
+                op->status = -ENOENT;
+                goto finish;
+            }
+
+            if ( iommu_unmap_page(d, op->u.unmap_page.bfn) )
+            {
+                op->status = -EIO;

Why not propogage its error values?

Quoted 10 lines
xen/common/pv_iommu.c +10 -0
+                goto finish;
+            }
+
+            op->status = 0;
+            break;
+        }
+
+        default:
+            op->status = -ENODEV;
+            break;

xen/common/pv_iommu.c line 199

Quoted 10 lines
xen/common/pv_iommu.c +9 -0
+    }
+
+finish:
+    if ( rd )
+        rcu_unlock_domain(rd);
+
+    return;
+}
+
 ret_t do_iommu_op(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned int count)

Shouldn't this be changed to be pv_iommu_op_t? instead of void?

xen/common/pv_iommu.c line 202

Quoted 4 lines
xen/common/pv_iommu.c +2 -1
 {
-    return -ENOSYS;
+    ret_t ret = 0;
+    int i;

unsigned int ?

xen/common/pv_iommu.c line 207

Quoted 5 lines
xen/common/pv_iommu.c +5 -0
+    struct pv_iommu_op op;
+    struct domain *d = current->domain;
+
+    if ( !is_hardware_domain(d) )
+        return -ENOSYS;

-EPERM ?

xen/common/pv_iommu.c line 209

Quoted 2 lines
xen/common/pv_iommu.c +2 -0
+
+    if ( (int)count < 0 )

Um. If you are doing this - why don't you just make the parameter be 'int' instead of unsigned int ?

xen/common/pv_iommu.c line 213

Quoted 4 lines
xen/common/pv_iommu.c +4 -0
+        return -EINVAL;
+
+    if ( count > 1 )
+        this_cpu(iommu_dont_flush_iotlb) = 1;

What if the hypercall is IOMMUOP_query_caps and count is zero (as it seems it ought to be OK - you just query for the caps). Or what if it is IOMMUOP_query_caps and the count is 31415 ?

Perhaps you should do some sanity checks here for some of the ops?

xen/common/pv_iommu.c line 215

Quoted 2 lines
xen/common/pv_iommu.c +2 -0
+
+    for ( i = 0; i < count; i++ )

Can you keep the signes of the count and i to be the same pls?

xen/common/pv_iommu.c line 219

Quoted 4 lines
xen/common/pv_iommu.c +4 -0
+    {
+        if ( i && hypercall_preempt_check() )
+        {
+            ret =  i;

Oh, that should definitly be mentioned in the header file. [edit: I see now that you do hypercall preemption]

Quoted 60 lines
xen/common/pv_iommu.c +33 -0
+            goto flush_pages;
+        }
+        if ( unlikely(__copy_from_guest_offset(&op, arg, i, 1)) )
+        {
+            ret = -EFAULT;
+            goto flush_pages;
+        }
+        do_iommu_sub_op(&op);
+        if ( unlikely(__copy_to_guest_offset(arg, i, &op, 1)) )
+        {
+            ret = -EFAULT;
+            goto flush_pages;
+        }
+    }
+
+flush_pages:
+    if ( ret > 0 )
+    {
+        XEN_GUEST_HANDLE_PARAM(pv_iommu_op_t) op =
+            guest_handle_cast(arg, pv_iommu_op_t);
+        ASSERT(ret < count);
+        guest_handle_add_offset(op, i);
+        arg = guest_handle_cast(op, void);
+        ret = hypercall_create_continuation(__HYPERVISOR_iommu_op,
+                                           "hi", arg, count - i);
+    }
+    if ( count > 1 )
+    {
+        this_cpu(iommu_dont_flush_iotlb) = 0;
+        if ( i )
+            iommu_iotlb_flush_all(d);
+    }
+    return ret;
 }
 
 /*
xen/include/public/pv-iommu.h new file +18 -0
new file mode 100644
index 0000000..0f19f96
@@ -0,0 +1,69 @@
+/*
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.

xen/include/public/pv-iommu.h line 28

Quoted 10 lines
xen/include/public/pv-iommu.h new file +10 -0
+ */
+
+#ifndef __XEN_PUBLIC_PV_IOMMU_H__
+#define __XEN_PUBLIC_PV_IOMMU_H__
+
+#include "xen.h"
+
+#define IOMMUOP_query_caps            1
+#define IOMMUOP_map_page              2
+#define IOMMUOP_unmap_page            3

Also you are missing explanation of what those hypercalls do. You can copy-n-paste from the design doc.

Perhaps IOMMU_OP_... ?

xen/include/public/pv-iommu.h line 31

Quoted 3 lines
xen/include/public/pv-iommu.h new file +3 -0
+
+struct pv_iommu_op {
+    uint16_t subop_id;

Why subop? Isn't it 'op'? To make with IOMMUOP..

xen/include/public/pv-iommu.h line 39

Quoted 8 lines
xen/include/public/pv-iommu.h new file +8 -0
+
+#define IOMMU_page_order (0xf1 << 10)
+#define IOMMU_get_page_order(flags) ((flags & IOMMU_page_order) >> 10)
+#define IOMMU_QUERY_map_cap (1 << 0)
+#define IOMMU_QUERY_map_all_mfns (1 << 1)
+#define IOMMU_OP_readable (1 << 0)
+#define IOMMU_OP_writeable (1 << 1)
+#define IOMMU_MAP_OP_no_ref_cnt (1 << 2)

Something is off with your tabs. Could you make these be on the same column?

And since these are flags perhaps IOMMU_F_.. ?

Also the hypercall accepts two arguments. So what is the 'unsigned int size' for ?

That should be mentioned here.

Quoted 38 lines
xen/include/public/pv-iommu.h new file +30 -0
+    uint16_t flags;
+    int32_t status;
+
+    union {
+        struct {
+            uint64_t bfn;
+            uint64_t gfn;
+        } map_page;
+
+        struct {
+            uint64_t bfn;
+        } unmap_page;
+    } u;
+};
+
+
+typedef struct pv_iommu_op pv_iommu_op_t;
+DEFINE_XEN_GUEST_HANDLE(pv_iommu_op_t);
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
Signature · 7 lines
1.7.12.4


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
#12 lore raw

Re: [RFC PATCH 4/7] common/pv-iommu: Add query, map and unmap ops

Quoted 10 lines
Quoted 1 line

ret_t do_iommu_op(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned int count)

Shouldn't this be changed to be pv_iommu_op_t? instead of void?

xen/common/pv_iommu.c line 202

Quoted 4 lines

{

xen/common/pv_iommu.c +2 -1
-    return -ENOSYS;
+    ret_t ret = 0;
+    int i;
Quoted 10 lines

unsigned int ?

xen/common/pv_iommu.c line 207

Quoted 5 lines
xen/common/pv_iommu.c +5 -0
+    struct pv_iommu_op op;
+    struct domain *d = current->domain;
+
+    if ( !is_hardware_domain(d) )
+        return -ENOSYS;

-EPERM ?

Quoted 1 line

+

Also you are missing the XSM checks which should be here.

#13 lore raw

RFC PATCH 5/7 x86/m2b: Add a tracking structure for mfn to bfn mappings per page

In reply to a message not in the archive

Add a pointer to the page struct which refers to the head of m2b tracking structure for that page.

Atomically add a PGC bit to the page struct when setting the pointer to the m2b tracking structure.

Adding elements to the per page m2b tracking structure is done via a RCU protected linked list.

Callers of add_m2b and del_m2b must hold the page lock for the page. This prevents the m2b code racing with itself and allows for fine gain locking. References to the page are also expected to be taken and dropped by the callers to the m2b code.

m2b references are forcibly removed as part of domain destroy to ensure malicious guests do not cause memory

To aid emulators tracking state, with asynchronous domain destruction, an ioreq is issued to all affected emulators for each m2b mapping being removed.

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/arch/x86/domain.c          |  12 ++-
 xen/arch/x86/mm/Makefile       |   1 +
 xen/arch/x86/mm/m2b.c          | 209 +++++++++++++++++++++++++++++++++++++++++
 xen/common/memory.c            |  11 +++
 xen/include/asm-x86/domain.h   |   1 +
 xen/include/asm-x86/m2b.h      |  59 ++++++++++++
 xen/include/asm-x86/mm.h       |  12 ++-
 xen/include/public/hvm/ioreq.h |   1 +
 xen/include/xen/sched.h        |   4 +
 9 files changed, 308 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/mm/m2b.c
 create mode 100644 xen/include/asm-x86/m2b.h
xen/arch/x86/domain.c +11 -1
index 9d43f7b..715502e 100644
@@ -62,6 +62,7 @@
 #include <xen/iommu.h>
 #include <compat/vcpu.h>
 #include <asm/psr.h>
+#include <asm/m2b.h>
 
 DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
 DEFINE_PER_CPU(unsigned long, cr4);
@@ -2511,7 +2512,7 @@ int domain_relinquish_resources(struct domain *d)
                 return ret;
         }
 
-        d->arch.relmem = RELMEM_xen;
+        d->arch.relmem = RELMEM_m2b;
 
         spin_lock(&d->page_alloc_lock);
         page_list_splice(&d->arch.relmem_list, &d->page_list);
@@ -2519,6 +2520,15 @@ int domain_relinquish_resources(struct domain *d)
         spin_unlock(&d->page_alloc_lock);
 
         /* Fallthrough. Relinquish every page of memory. */
+
+    case RELMEM_m2b:
+
+        ret = m2b_domain_destroy(d, d->m2b_destroy_mfn);
+        if ( ret )
+            return ret;
+        d->arch.relmem = RELMEM_xen;
+        /* fallthrough */
+
     case RELMEM_xen:
         ret = relinquish_memory(d, &d->xenpage_list, ~0UL);
         if ( ret )
xen/arch/x86/mm/Makefile +1 -0
index 9804c3a..84ad8c0 100644
@@ -9,6 +9,7 @@ obj-y += guest_walk_3.o
 obj-y += guest_walk_4.o
 obj-y += mem_paging.o
 obj-y += mem_sharing.o
+obj-y += m2b.o
 
 guest_walk_%.o: guest_walk.c Makefile
 	$(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
xen/arch/x86/mm/m2b.c new file +209 -0

209 lines not on this page · raw message

xen/common/memory.c +11 -0
index 9ff1145..df470ba 100644
@@ -29,6 +29,7 @@
 #include <asm/p2m.h>
 #include <public/memory.h>
 #include <xsm/xsm.h>
+#include <asm/m2b.h>
 
 struct memop_args {
     /* INPUT */
@@ -316,6 +317,16 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
          test_and_clear_bit(_PGC_allocated, &page->count_info) )
         put_page(page);
 
+#ifdef CONFIG_X86
+    /* Check for M2B mapping */
+    if ( is_hvm_domain(d) &&
+            test_bit(_PGC_foreign_map, &page->count_info) )
+    {
+        /* Notifiy ioserver with M2B mappings */
+        notify_m2b_entries(page);
+    }
+#endif
+
     guest_physmap_remove_page(d, gmfn, mfn, 0);
 
     put_page(page);
xen/include/asm-x86/domain.h +1 -0
index 4072e27..6dc6ee5 100644
@@ -302,6 +302,7 @@ struct arch_domain
     enum {
         RELMEM_not_started,
         RELMEM_shared,
+        RELMEM_m2b,
         RELMEM_xen,
         RELMEM_l4,
         RELMEM_l3,
xen/include/asm-x86/m2b.h new file +59 -0

59 lines not on this page · raw message

xen/include/asm-x86/mm.h +11 -1
index a097382..620ccd5 100644
@@ -65,6 +65,12 @@ struct page_info
          */
         struct page_sharing_info *sharing;
     };
+    /* For foreign mapped pages, we use a doubly-linked list
+     * of all the {pfn,domain, ioserver} tuples that map this page.
+     * This list is allocated and freed from a page is foreign
+     * mapped/unmapped.
+     */
+    struct pv_iommu_info *pv_iommu;
 
     /* Reference count and various PGC_xxx flags and fields. */
     unsigned long count_info;
@@ -230,8 +236,12 @@ struct page_info
 #define PGC_state_free    PG_mask(3, 9)
 #define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
 
+/* Page has foreign mappings? */
+#define _PGC_foreign_map  PG_shift(10)
+#define PGC_foreign_map   PG_mask(1, 10)
+
  /* Count of references to this frame. */
-#define PGC_count_width   PG_shift(9)
+#define PGC_count_width   PG_shift(10)
 #define PGC_count_mask    ((1UL<<PGC_count_width)-1)
 
 struct spage_info
xen/include/public/hvm/ioreq.h +1 -0
index 2e5809b..c8ecc6e 100644
@@ -37,6 +37,7 @@
 #define IOREQ_TYPE_PCI_CONFIG   2
 #define IOREQ_TYPE_TIMEOFFSET   7
 #define IOREQ_TYPE_INVALIDATE   8 /* mapcache */
+#define IOREQ_TYPE_INVAL_BFN    9 /* bfn */
 
 /*
  * VMExit dispatcher should cooperate with instruction decoder to
xen/include/xen/sched.h +4 -0
index b47a3fe..3085be2 100644
@@ -464,6 +464,10 @@ struct domain
     /* vNUMA topology accesses are protected by rwlock. */
     rwlock_t vnuma_rwlock;
     struct vnuma_info *vnuma;
+
+    /* Progress of cleaning m2b for domain destroy */
+    unsigned long m2b_destroy_mfn;
+    atomic_t m2b_count;
 };
 
 /* Protect updates/reads (resp.) of domain_list and domain_hash. */
Signature · 1 line
1.7.12.4
#14 lore raw

Re: [RFC PATCH 5/7] x86/m2b: Add a tracking structure for mfn to bfn mappings per page

Quoted 9 lines · Malcolm Crossley

Add a pointer to the page struct which refers to the head of m2b tracking structure for that page.

Atomically add a PGC bit to the page struct when setting the pointer to the m2b tracking structure.

Adding elements to the per page m2b tracking structure is done via a RCU protected linked list.

Quoted 7 lines

Callers of add_m2b and del_m2b must hold the page lock for the page. This prevents the m2b code racing with itself and allows for fine gain locking. References to the page are also expected to be taken and dropped by the callers to the m2b code.

m2b references are forcibly removed as part of domain destroy to ensure malicious guests do not cause memory

Editing mistake?

[snip]

Quoted 17 lines
xen/arch/x86/mm/m2b.c new file +17 -0
+struct m2b_entry *lookup_m2b_entry(struct page_info *page, struct domain *d,
+                                   ioservid_t ioserver, unsigned long bfn)
+{
+    struct m2b_entry *m2b_e = NULL;
+    struct list_head *entry;
+    domid_t domain = d->domain_id;
+
+    if ( !test_bit(_PGC_foreign_map, &page->count_info) )
+        return NULL;
+
+    rcu_read_lock(&m2b_rcu);
+    list_for_each_rcu(entry, &page->pv_iommu->head)
+    {
+        m2b_e = list_entry(entry, struct m2b_entry, list);
+        if ( m2b_e->domain == domain && m2b_e->ioserver == ioserver &&
+                m2b_e->bfn == bfn )
+                    goto done;

xen/arch/x86/mm/m2b.c line 60

Quoted 10 lines
xen/arch/x86/mm/m2b.c new file +10 -0
+        else if ( ioserver == IOSERVER_ANY && m2b_e->domain == domain &&
+                  m2b_e->bfn == bfn )
+                    goto done;
+        else if ( bfn == BFN_ANY && m2b_e->domain == domain &&
+                  m2b_e->ioserver == ioserver )
+                    goto done;
+        else if ( bfn == BFN_ANY && ioserver == IOSERVER_ANY &&
+                  m2b_e->domain == domain )
+                    goto done;
+    }

Just a minor note while I'm thinking about it -- since every single one of these have "m2b_e->domain == domain", wouldn't it be easier to read if you just had "if (m2b_e->domain != domain) continue" first, and then the series of if statements without mentioning the domain?

-George

#15 lore raw

RFC PATCH 6/7 common/pv-iommu: Add foreign ops to PV-IOMMU interface

In reply to a message not in the archive

Currently the ops are X86 only due to a dependency on the X86 only B2M implementation.

Foreign ops conform to draft D of PV-IOMMU design.

XSM control been implemented to allow full security control of these priviledged operatins.

Page references and page locking are taken before using B2M interface which is mandated by the B2M interface itself.

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/common/pv_iommu.c         | 269 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/pv-iommu.h |  22 ++++
 xen/include/xsm/dummy.h       |   6 +
 xen/include/xsm/xsm.h         |   6 +
 xen/xsm/dummy.c               |   1 +
 5 files changed, 304 insertions(+)
xen/common/pv_iommu.c +269 -0
index 91485f3..bbf1a21 100644
@@ -21,7 +21,12 @@
 #include <asm/event.h>
 #include <xen/guest_access.h>
 #include <public/pv-iommu.h>
+#include <xsm/xsm.h>
 
+#ifdef CONFIG_X86
+#include <asm/m2b.h>
+#include <asm/setup.h>
+#endif
 #define ret_t long
 
 static int get_paged_frame(unsigned long gfn, unsigned long *frame,
@@ -79,6 +84,7 @@ void do_iommu_sub_op(struct pv_iommu_op *op)
 {
     struct domain *d = current->domain;
     struct domain *rd = NULL;
+    int ret;
 
     /* Only order 0 pages supported */
     if ( IOMMU_get_page_order(op->flags) != 0 )
@@ -183,7 +189,270 @@ void do_iommu_sub_op(struct pv_iommu_op *op)
             op->status = 0;
             break;
         }
+#ifdef CONFIG_X86
+        case IOMMUOP_map_foreign_page:
+        {
+            unsigned long mfn, tmp;
+            unsigned int flags = 0;
+            struct page_info *page = NULL;
+            struct m2b_entry *m2b_e;
+            int locked;
+
+            /* Check if calling domain can create IOMMU mappings */
+            if ( !can_use_iommu_check(d) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            rd = rcu_lock_domain_by_any_id(op->u.map_foreign_page.domid);
+            if ( !rd )
+            {
+                op->status = -ENXIO;
+                goto finish;
+            }
+
+            /* Only HVM domains can have their pages foreign mapped */
+            if ( is_pv_domain(rd) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            if ( d->domain_id == op->u.map_foreign_page.domid ||
+                    op->u.map_foreign_page.domid == DOMID_SELF )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Check for privilege over remote domain*/
+            if ( xsm_iommu_control(XSM_DM_PRIV, rd, op->subop_id) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Lookup page struct backing gfn */
+            if ( get_paged_frame(op->u.map_foreign_page.gfn, &mfn, &page, 0,
+                        rd) )
+            {
+                op->status = -ENXIO;
+                goto finish;
+            }
+            /* Check M2B for existing mapping */
+            m2b_e = lookup_m2b_entry(page, d,
+                        op->u.map_foreign_page.ioserver,
+                        op->u.map_foreign_page.bfn);
+
+            /* M2B already exists for domid, gfn, ioserver combination */
+            if ( m2b_e )
+            {
+                put_page(page);
+                op->status = 0;
+                goto finish;
+            }
+
+            if ( !mfn_valid(mfn) || xen_in_range(mfn) ||
+                 is_xen_heap_page(page)  ||
+                 (page->count_info & PGC_allocated) ||
+                 ( (page->count_info & PGC_count_mask) < 2 ))
+            {
+                put_page(page);
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Check for conflict with existing BFN mapping */
+            if ( !iommu_lookup_page(d, op->u.map_foreign_page.bfn, &tmp) )
+            {
+                put_page(page);
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            if ( op->flags & IOMMU_OP_readable )
+                flags |= IOMMUF_readable;
+
+            if ( op->flags & IOMMU_OP_writeable )
+                flags |= IOMMUF_writable;
+
+            if ( iommu_map_page(d, op->u.map_foreign_page.bfn, mfn, flags) )
+            {
+                put_page(page);
+                op->status = -EIO;
+                goto finish;
+            }
+            /* Add M2B entry */
+            locked = page_lock(page);
+            ret = add_m2b_entry(page, d,
+                        op->u.map_foreign_page.ioserver,
+                        op->u.map_foreign_page.bfn);
+            atomic_inc(&rd->m2b_count);
+            if ( locked )
+                page_unlock(page);
+            if ( ret )
+            {
+                if ( iommu_unmap_page(d, op->u.map_foreign_page.bfn) )
+                    domain_crash(d);
+
+                put_page(page);
+                op->status = -ENOMEM;
+                goto finish;
+            }
+
+            op->status = 0;
+            break;
+        }
+        case IOMMUOP_lookup_foreign_page:
+        {
+            unsigned long mfn;
+            struct page_info *page = NULL;
+            struct m2b_entry *m2b_e;
+            int rc;
+            int locked;
 
+            if ( d->domain_id == op->u.lookup_foreign_page.domid ||
+                 op->u.lookup_foreign_page.domid == DOMID_SELF )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            rd = rcu_lock_domain_by_any_id(op->u.lookup_foreign_page.domid);
+
+            if ( !rd )
+            {
+                op->status = -ENXIO;
+                goto finish;
+            }
+
+            /* Only HVM domains can have their pages foreign mapped */
+            if ( is_pv_domain(rd) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Check for privilege */
+            if ( xsm_iommu_control(XSM_DM_PRIV, rd, op->subop_id) )
+            {
+                op->status = -EPERM;
+                goto finish;
+            }
+
+            /* Lookup page struct backing gfn */
+            if ( (rc = get_paged_frame(op->u.lookup_foreign_page.gfn, &mfn, &page, 0,
+                                 rd)) )
+            {
+                op->status = -ENXIO; // Should this be something else?
+                goto finish;
+            }
+
+            /* Check M2B for existing mapping */
+            m2b_e = lookup_m2b_entry(page, d,
+                                     op->u.lookup_foreign_page.ioserver,
+                                     BFN_ANY);
+
+            /* M2B already exists for domid, gfn, ioserver combination */
+            if ( m2b_e )
+            {
+                put_page(page);
+                op->u.lookup_foreign_page.bfn = m2b_e->bfn;
+                op->status = 0;
+                goto finish;
+            }
+
+            /* Only create BFN mappings for guest mapped memory */
+            if ( !(page->count_info & PGC_allocated) ||
+                 ( (page->count_info & PGC_count_mask) < 2 ))
+            {
+                    put_page(page);
+                    op->status = -EPERM;
+                    goto finish;
+            }
+
+            /* Check if IOMMU is disabled/bypassed */
+            if ( !can_use_iommu_check(d) )
+            {
+                /* Add M2B entry using MFN */
+                locked = page_lock(page);
+                ret = add_m2b_entry(page, d,
+                                    op->u.lookup_foreign_page.ioserver, mfn);
+                atomic_inc(&rd->m2b_count);
+                if ( locked )
+                    page_unlock(page);
+                if ( ret )
+                {
+                   put_page(page);
+                   op->status = -ENOMEM;
+                   goto finish;
+                }
+                op->u.lookup_foreign_page.bfn = mfn;
+            }
+            op->status = 0;
+            break;
+        }
+        case IOMMUOP_unmap_foreign_page:
+        {
+            struct m2b_entry *m2b_e;
+            struct page_info *page;
+            unsigned long mfn;
+            int locked;
+
+
+            if ( !can_use_iommu_check(d) )
+            {
+                page = mfn_to_page(op->u.unmap_foreign_page.bfn);
+            }
+            else
+            {
+                /* Check if there is a valid BFN mapping for this domain */
+                if ( iommu_lookup_page(d, op->u.unmap_foreign_page.bfn, &mfn) )
+                {
+                   op->status = -ENOENT;
+                   goto finish;
+                }
+                /* Use MFN from B2M mapping to lookup page */
+                page = mfn_to_page(mfn);
+            }
+
+            if ( !page )
+            {
+               op->status = -ENOENT;
+               goto finish;
+            }
+
+            /* Try to remove the M2B mapping */
+            locked = page_lock(page);
+            ret = del_m2b_entry(page, d,
+                                op->u.unmap_foreign_page.ioserver,
+                                op->u.unmap_foreign_page.bfn);
+            atomic_dec(&page_get_owner(page)->m2b_count);
+            if ( locked )
+            page_unlock(page);
+            if ( ret )
+            {
+               op->status = -ENOENT;
+               goto finish;
+            }
+
+            if ( can_use_iommu_check(d) )
+            {
+                /* Check if there are any M2B mappings left for this domain */
+                m2b_e = lookup_m2b_entry(page, d,
+                                         IOSERVER_ANY,
+                                         op->u.unmap_foreign_page.bfn);
+
+                /* No M2B left for this bfn so IOMMU unmap it */
+                if ( !m2b_e )
+                {
+                    if ( iommu_unmap_page(d, op->u.map_foreign_page.bfn) )
+                        domain_crash(d);
+                }
+            }
+        }
+#endif
         default:
             op->status = -ENODEV;
             break;
xen/include/public/pv-iommu.h +22 -0
index 0f19f96..366037c 100644
@@ -26,6 +26,9 @@
 #define IOMMUOP_query_caps            1
 #define IOMMUOP_map_page              2
 #define IOMMUOP_unmap_page            3
+#define IOMMUOP_map_foreign_page      4
+#define IOMMUOP_lookup_foreign_page   5
+#define IOMMUOP_unmap_foreign_page    6
 
 struct pv_iommu_op {
     uint16_t subop_id;
@@ -49,6 +52,25 @@ struct pv_iommu_op {
         struct {
             uint64_t bfn;
         } unmap_page;
+
+        struct {
+            uint64_t bfn;
+            uint64_t gfn;
+            uint16_t domid;
+            uint16_t ioserver;
+        } map_foreign_page;
+
+        struct {
+            uint64_t bfn;
+            uint64_t gfn;
+            uint16_t domid;
+            uint16_t ioserver;
+        } lookup_foreign_page;
+
+        struct {
+            uint64_t bfn;
+            uint16_t ioserver;
+        } unmap_foreign_page;
     } u;
 };
 
xen/include/xsm/dummy.h +6 -0
index 1d13826..d0be2fe 100644
@@ -566,6 +566,12 @@ static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, int
     return xsm_default_action(action, current->domain, d);
 }
 
+static XSM_INLINE int xsm_iommu_control(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
+{
+    XSM_ASSERT_ACTION(XSM_DM_PRIV);
+    return xsm_default_action(action, current->domain, d);
+}
+
 #ifdef CONFIG_HAS_MEM_ACCESS
 static XSM_INLINE int xsm_mem_access(XSM_DEFAULT_ARG struct domain *d)
 {
xen/include/xsm/xsm.h +6 -0
index 3afed70..92e6e33 100644
@@ -114,6 +114,7 @@ struct xsm_operations {
     int (*iomem_permission) (struct domain *d, uint64_t s, uint64_t e, uint8_t allow);
     int (*iomem_mapping) (struct domain *d, uint64_t s, uint64_t e, uint8_t allow);
     int (*pci_config_permission) (struct domain *d, uint32_t machine_bdf, uint16_t start, uint16_t end, uint8_t access);
+    int (*iommu_control) (struct domain *d, unsigned long op);
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
     int (*get_device_group) (uint32_t machine_bdf);
@@ -605,6 +606,11 @@ static inline int xsm_vm_event_control (xsm_default_t def, struct domain *d, int
     return xsm_ops->vm_event_control(d, mode, op);
 }
 
+static inline int xsm_iommu_control(xsm_default_t def, struct domain *d, unsigned long op)
+{
+    return xsm_ops->iommu_control(d, op);
+}
+
 #ifdef CONFIG_HAS_MEM_ACCESS
 static inline int xsm_mem_access (xsm_default_t def, struct domain *d)
 {
xen/xsm/dummy.c +1 -0
index 0f32636..e18bb3b 100644
@@ -119,6 +119,7 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, hvm_altp2mhvm_op);
 
     set_to_dummy_if_null(ops, do_xsm_op);
+    set_to_dummy_if_null(ops, iommu_control);
 #ifdef CONFIG_COMPAT
     set_to_dummy_if_null(ops, do_compat_op);
 #endif
Signature · 1 line
1.7.12.4
#16 lore raw

Re: [RFC PATCH 6/7] common/pv-iommu: Add foreign ops to PV-IOMMU interface

Quoted 7 lines · Malcolm Crossley

Currently the ops are X86 only due to a dependency on the X86 only B2M implementation.

Foreign ops conform to draft D of PV-IOMMU design.

XSM control been implemented to allow full security control of these priviledged operatins.

But you missed the decleration of them in access_vectors, the default policy in xen.te and also the implementation in the hooks.c

And that should all be part of the code that adds the new hypercall.

Quoted 47 lines

Page references and page locking are taken before using B2M interface which is mandated by the B2M interface itself.

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/common/pv_iommu.c         | 269 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/pv-iommu.h |  22 ++++
 xen/include/xsm/dummy.h       |   6 +
 xen/include/xsm/xsm.h         |   6 +
 xen/xsm/dummy.c               |   1 +
 5 files changed, 304 insertions(+)
xen/common/pv_iommu.c +6 -0
index 91485f3..bbf1a21 100644
@@ -21,7 +21,12 @@
 #include <asm/event.h>
 #include <xen/guest_access.h>
 #include <public/pv-iommu.h>
+#include <xsm/xsm.h>
 
+#ifdef CONFIG_X86
+#include <asm/m2b.h>
+#include <asm/setup.h>
+#endif
 #define ret_t long
 
 static int get_paged_frame(unsigned long gfn, unsigned long *frame,
@@ -79,6 +84,7 @@ void do_iommu_sub_op(struct pv_iommu_op *op)
 {
     struct domain *d = current->domain;
     struct domain *rd = NULL;
+    int ret;
 
     /* Only order 0 pages supported */
     if ( IOMMU_get_page_order(op->flags) != 0 )

xen/common/pv_iommu.c void do_iommu_sub_op(struct pv_iommu_op *op) · line 192

Quoted 4 lines
xen/common/pv_iommu.c +1 -0
@@ -183,7 +189,270 @@ void do_iommu_sub_op(struct pv_iommu_op *op)
             op->status = 0;
             break;
         }
+#ifdef CONFIG_X86

Why not move all of this in arch/x86/pv_iommu.c code that would implement per-arch code?

xen/common/pv_iommu.c void do_iommu_sub_op(struct pv_iommu_op *op) · line 193

Quoted 1 line
xen/common/pv_iommu.c +1 -0
+        case IOMMUOP_map_foreign_page:

Didn't look at the rest of the code - will when you repost.

#17 lore raw

RFC PATCH 7/7 common/pv-iommu: Allow hardware_domain to pre IOMMU map foreign memory

In reply to a message not in the archive

If the hardware domain is relaxed IOMMU mode, then allow the hardware domain to pre IOMMU map host memory so that foreign IOMMU maps of guest memory are not necessary and only foreign lookups are required.

The security model is not weakened because the hardware domain in relaxed IOMMU mode already had a IOMMU map of the entire system RAM.

A special hardware_domain premap array is used to track the hardware domains current IOMMU mapppings.

This optimisation allows for emulators to not suffer a IOMMU flush delay for each batch of foreign page(s) they attempt to map/unmap.

--
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 xen/arch/x86/mm/m2b.c         |   2 +
 xen/common/pv_iommu.c         | 100 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/m2b.h     |   6 +++
 xen/include/public/pv-iommu.h |   2 +
 4 files changed, 110 insertions(+)
xen/arch/x86/mm/m2b.c +2 -0
index 078944a..59d4d02 100644
@@ -31,6 +31,8 @@ struct m2b_head
 
 DEFINE_RCU_READ_LOCK(m2b_rcu);
 
+uint64_t **hwdom_premap_m2b;
+
 struct m2b_entry *lookup_m2b_entry(struct page_info *page, struct domain *d,
                                    ioservid_t ioserver, unsigned long bfn)
 {
xen/common/pv_iommu.c +100 -0

142 lines not on this page · raw message

xen/include/asm-x86/m2b.h +6 -0
index a21502c..4d06b08 100644
@@ -46,6 +46,12 @@ int add_m2b_entry(struct page_info *page, struct domain *d,
 int del_m2b_entry(struct page_info *page, struct domain *d, ioservid_t ioserver,
                   unsigned long bfn);
 
+extern uint64_t **hwdom_premap_m2b;
+
+#define PREMAP_M2B_PAGE(x) ( pfn_to_pdx(x) / (PAGE_SIZE/sizeof(uint64_t) ) )
+#define PREMAP_M2B_IDX(x) ( pfn_to_pdx(x) % (PAGE_SIZE/sizeof(uint64_t) ) )
+#define PREMAP_M2B(x) hwdom_premap_m2b[PREMAP_M2B_PAGE(x)][PREMAP_M2B_IDX(x)]
+
 #endif
 
 /*
xen/include/public/pv-iommu.h +2 -0
index 366037c..4505f3e 100644
@@ -40,6 +40,8 @@ struct pv_iommu_op {
 #define IOMMU_OP_readable (1 << 0)
 #define IOMMU_OP_writeable (1 << 1)
 #define IOMMU_MAP_OP_no_ref_cnt (1 << 2)
+#define IOMMU_MAP_OP_add_m2b (1 << 3)
+#define IOMMU_UNMAP_OP_remove_m2b (1 << 0)
     uint16_t flags;
     int32_t status;
 
Signature · 1 line
1.7.12.4
#18 lore raw

cover Implement Xen PV-IOMMU interface

In reply to a message not in the archive

This RFC series implements the PV-IOMMU interface according to the PV-IOMMU design draft D.

The PV-IOMMU interface is currently restricted to being used by the hardware domain only as non hardware domain callers have not been fully tested yet.

Significant effort was put into implementing a m2b tracking structure without increasing the size of struct page but no union was found that could be safely used in all cases when a page is allocated to an HVM domain. Comments and feedback on the m2b design are most welcome.

The hardware domain specific IOMMU pre map mechanism was implemented in order to keep performance parity with current out of tree mechanisms to obtain BFNs for foreign guest owned memory. The pre map mechanism is not a weakening of the current security model of Xen and is only allowed when the hardware domain is allowed relaxed IOMMU mappings.

Malcolm Crossley (7):
common/pv-iommu: Add stub hypercall for PV-IOMMU
iommu: add iommu_lookup_page to lookup guest gfn for a particular
IOMMU mapping
VT-d: Add iommu_lookup_page support
common/pv-iommu: Add query, map and unmap ops
x86/m2b: Add a tracking structure for mfn to bfn mappings per page common/pv-iommu: Add foreign ops to PV-IOMMU interface common/pv-iommu: Allow hardware_domain to pre IOMMU map foreign
memory

 xen/arch/x86/domain.c               |  12 +-
 xen/arch/x86/mm/Makefile            |   1 +
 xen/arch/x86/mm/m2b.c               | 211 ++++++++++++
 xen/arch/x86/x86_64/compat/entry.S  |   2 +
 xen/arch/x86/x86_64/entry.S         |   2 +
 xen/common/Makefile                 |   1 +
 xen/common/memory.c                 |  11 +
 xen/common/pv_iommu.c               | 633 ++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/iommu.c     |  21 ++
 xen/drivers/passthrough/vtd/iommu.c |  31 ++
 xen/drivers/passthrough/vtd/iommu.h |   1 +
 xen/include/asm-x86/domain.h        |   1 +
 xen/include/asm-x86/m2b.h           |  65 ++++
 xen/include/asm-x86/mm.h            |  12 +-
 xen/include/public/hvm/ioreq.h      |   1 +
 xen/include/public/pv-iommu.h       |  93 ++++++
 xen/include/public/xen.h            |   1 +
 xen/include/xen/iommu.h             |   2 +
 xen/include/xen/sched.h             |   4 +
 xen/include/xsm/dummy.h             |   6 +
 xen/include/xsm/xsm.h               |   6 +
 xen/xsm/dummy.c                     |   1 +
 22 files changed, 1116 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/mm/m2b.c
 create mode 100644 xen/common/pv_iommu.c
 create mode 100644 xen/include/asm-x86/m2b.h
 create mode 100644 xen/include/public/pv-iommu.h
#19 lore raw

Re: [RFC PATCH 0/7] Implement Xen PV-IOMMU interface

Quoted 11 lines · Malcolm Crossley

This RFC series implements the PV-IOMMU interface according to the PV-IOMMU design draft D.

The PV-IOMMU interface is currently restricted to being used by the hardware domain only as non hardware domain callers have not been fully tested yet.

Significant effort was put into implementing a m2b tracking structure without increasing the size of struct page but no union was found that could be safely used in all cases when a page is allocated to an HVM domain. Comments and feedback on the m2b design are most welcome.

Quoted 5 lines

The hardware domain specific IOMMU pre map mechanism was implemented in order to keep performance parity with current out of tree mechanisms to obtain BFNs for foreign guest owned memory. The pre map mechanism is not a weakening of the current security model of Xen and is only allowed when the hardware domain is allowed relaxed IOMMU mappings.

I would recommend you add to this patchset:

- docs/misc/pv-iommu.txt describing the design of this.
- Add yourself to the maintainers file for this code: xen/common/pv_iommu? perhaps?
- Compile this under ARM as well - and figure out what is missing there?
- Add an KConfig entry so folks have the option of not compiling the Xen hypervisor with this?
- Have these patches in a git tree for easier testing.

Quoted 45 lines

Malcolm Crossley (7):
common/pv-iommu: Add stub hypercall for PV-IOMMU
iommu: add iommu_lookup_page to lookup guest gfn for a particular
IOMMU mapping
VT-d: Add iommu_lookup_page support
common/pv-iommu: Add query, map and unmap ops
x86/m2b: Add a tracking structure for mfn to bfn mappings per page common/pv-iommu: Add foreign ops to PV-IOMMU interface common/pv-iommu: Allow hardware_domain to pre IOMMU map foreign
memory

 xen/arch/x86/domain.c               |  12 +-
 xen/arch/x86/mm/Makefile            |   1 +
 xen/arch/x86/mm/m2b.c               | 211 ++++++++++++
 xen/arch/x86/x86_64/compat/entry.S  |   2 +
 xen/arch/x86/x86_64/entry.S         |   2 +
 xen/common/Makefile                 |   1 +
 xen/common/memory.c                 |  11 +
 xen/common/pv_iommu.c               | 633 ++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/iommu.c     |  21 ++
 xen/drivers/passthrough/vtd/iommu.c |  31 ++
 xen/drivers/passthrough/vtd/iommu.h |   1 +
 xen/include/asm-x86/domain.h        |   1 +
 xen/include/asm-x86/m2b.h           |  65 ++++
 xen/include/asm-x86/mm.h            |  12 +-
 xen/include/public/hvm/ioreq.h      |   1 +
 xen/include/public/pv-iommu.h       |  93 ++++++
 xen/include/public/xen.h            |   1 +
 xen/include/xen/iommu.h             |   2 +
 xen/include/xen/sched.h             |   4 +
 xen/include/xsm/dummy.h             |   6 +
 xen/include/xsm/xsm.h               |   6 +
 xen/xsm/dummy.c                     |   1 +
 22 files changed, 1116 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/mm/m2b.c
 create mode 100644 xen/common/pv_iommu.c
 create mode 100644 xen/include/asm-x86/m2b.h
 create mode 100644 xen/include/public/pv-iommu.h

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#20 lore raw

Re: [RFC PATCH 0/7] Implement Xen PV-IOMMU interface

In reply to Malcolm Crossley
Quoted 29 lines · Malcolm Crossley

This RFC series implements the PV-IOMMU interface according to the PV-IOMMU design draft D.

The PV-IOMMU interface is currently restricted to being used by the hardware domain only as non hardware domain callers have not been fully tested yet.

Significant effort was put into implementing a m2b tracking structure without increasing the size of struct page but no union was found that could be safely used in all cases when a page is allocated to an HVM domain. Comments and feedback on the m2b design are most welcome.

The hardware domain specific IOMMU pre map mechanism was implemented in order to keep performance parity with current out of tree mechanisms to obtain BFNs for foreign guest owned memory. The pre map mechanism is not a weakening of the current security model of Xen and is only allowed when the hardware domain is allowed relaxed IOMMU mappings.

Malcolm Crossley (7):
common/pv-iommu: Add stub hypercall for PV-IOMMU
iommu: add iommu_lookup_page to lookup guest gfn for a particular
IOMMU mapping
VT-d: Add iommu_lookup_page support
common/pv-iommu: Add query, map and unmap ops
x86/m2b: Add a tracking structure for mfn to bfn mappings per page common/pv-iommu: Add foreign ops to PV-IOMMU interface common/pv-iommu: Allow hardware_domain to pre IOMMU map foreign
memory

Quoted 3 lines
 xen/arch/x86/domain.c               |  12 +-
 xen/arch/x86/mm/Makefile            |   1 +
 xen/arch/x86/mm/m2b.c               | 211 ++++++++++++

My first question from the RFC perspective is whether mm/ is the right place for this file to go. It doesn't seem to have any interaction with any of the other p2m functionality, nor does it seem to resemble the functionality much (using linked lists from the page struct rather than the pagetable structure). I don't mind it going under mm/ per se but it seems like it might fit logically better under xen/drivers/passthrough.

Definitely agree that the design doc needs to be checked into the tree as a part of this series.

The other minor question is why you have the patches broken down quite the way you do -- what's the point of adding a stub hypercall that doesn't do anything, and adding a callback framework that doesn't have any users? Why not just squash patches 1 and 4 together, and patches 2 and 3 together?

The design doc is well-written and thorough, thanks. :-)

-George

#21 lore raw

Re: [RFC PATCH 0/7] Implement Xen PV-IOMMU interface

In reply to Malcolm Crossley

Hello.

Any update on this ?

Thanks, Martin Cerveny

Quoted 57 lines · Malcolm Crossley

This RFC series implements the PV-IOMMU interface according to the PV-IOMMU design draft D.

The PV-IOMMU interface is currently restricted to being used by the hardware domain only as non hardware domain callers have not been fully tested yet.

Significant effort was put into implementing a m2b tracking structure without increasing the size of struct page but no union was found that could be safely used in all cases when a page is allocated to an HVM domain. Comments and feedback on the m2b design are most welcome.

The hardware domain specific IOMMU pre map mechanism was implemented in order to keep performance parity with current out of tree mechanisms to obtain BFNs for foreign guest owned memory. The pre map mechanism is not a weakening of the current security model of Xen and is only allowed when the hardware domain is allowed relaxed IOMMU mappings.

Malcolm Crossley (7):
common/pv-iommu: Add stub hypercall for PV-IOMMU
iommu: add iommu_lookup_page to lookup guest gfn for a particular
IOMMU mapping
VT-d: Add iommu_lookup_page support
common/pv-iommu: Add query, map and unmap ops
x86/m2b: Add a tracking structure for mfn to bfn mappings per page common/pv-iommu: Add foreign ops to PV-IOMMU interface common/pv-iommu: Allow hardware_domain to pre IOMMU map foreign
memory

xen/arch/x86/domain.c               |  12 +-
xen/arch/x86/mm/Makefile            |   1 +
xen/arch/x86/mm/m2b.c               | 211 ++++++++++++
xen/arch/x86/x86_64/compat/entry.S  |   2 +
xen/arch/x86/x86_64/entry.S         |   2 +
xen/common/Makefile                 |   1 +
xen/common/memory.c                 |  11 +
xen/common/pv_iommu.c               | 633 ++++++++++++++++++++++++++++++++++++
xen/drivers/passthrough/iommu.c     |  21 ++
xen/drivers/passthrough/vtd/iommu.c |  31 ++
xen/drivers/passthrough/vtd/iommu.h |   1 +
xen/include/asm-x86/domain.h        |   1 +
xen/include/asm-x86/m2b.h           |  65 ++++
xen/include/asm-x86/mm.h            |  12 +-
xen/include/public/hvm/ioreq.h      |   1 +
xen/include/public/pv-iommu.h       |  93 ++++++
xen/include/public/xen.h            |   1 +
xen/include/xen/iommu.h             |   2 +
xen/include/xen/sched.h             |   4 +
xen/include/xsm/dummy.h             |   6 +
xen/include/xsm/xsm.h               |   6 +
xen/xsm/dummy.c                     |   1 +
22 files changed, 1116 insertions(+), 2 deletions(-)

create mode 100644 xen/arch/x86/mm/m2b.c
create mode 100644 xen/common/pv_iommu.c
create mode 100644 xen/include/asm-x86/m2b.h
create mode 100644 xen/include/public/pv-iommu.h

Quoted 5 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel