common/pv-iommu: Add stub hypercall for PV-IOMMU
A message that is not in the archive (sent off-list or not yet received): [email protected]
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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
Signed-off-by Malcolm Crossley <[email protected]>--
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.cxen/arch/x86/x86_64/compat/entry.S +2
-0@@ -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 */ .endrxen/arch/x86/x86_64/entry.S +2
-0@@ -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 */ .endrxen/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 obj-y += random.o obj-y += rangeset.o obj-y += radix-tree.oxen/common/pv_iommu.c new file +38
-0@@ -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@@ -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
Quoted 62 lines · Malcolm Crossley
Signed-off-by Malcolm Crossley <[email protected]>--
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.cxen/arch/x86/x86_64/compat/entry.S +2
-0@@ -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 */ .endrxen/arch/x86/x86_64/entry.S +2
-0@@ -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 */ .endrxen/common/Makefile +0
-0
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
-0obj-y += random.o obj-y += rangeset.o obj-y += radix-tree.oxen/common/pv_iommu.c new file +12
-0@@ -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
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 48Signature · 7 lines
1.7.12.4 _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
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 archiveIf 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
@@ -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
@@ -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
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_pagefunction 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.
Signed-off-by Malcolm Crossley <[email protected]>--
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
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@@ -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
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-ENOSYSfor the case whenhd->platform_opsis 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
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-ENOSYSfor the case whenhd->platform_opsis 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
RFC PATCH 3/7 VT-d: Add iommu_lookup_page support
In reply to a message not in the archiveFunction 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
@@ -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
@@ -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
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
Signed-off-by Malcolm Crossley <[email protected]>--
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@@ -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@@ -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
RFC PATCH 4/7 common/pv-iommu: Add query, map and unmap ops
In reply to a message not in the archiveImplement 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
@@ -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
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
Signed-off-by Malcolm Crossley <[email protected]>--
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.hxen/common/pv_iommu.c +0
-0
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@@ -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
Re: [RFC PATCH 4/7] common/pv-iommu: Add query, map and unmap ops
Quoted 10 lines
Quoted 1 line
ret_tdo_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.
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 archiveAdd 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
@@ -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
@@ -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
@@ -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
@@ -302,6 +302,7 @@ struct arch_domainenum { 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
@@ -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
@@ -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
@@ -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
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_m2banddel_m2bmust 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
RFC PATCH 6/7 common/pv-iommu: Add foreign ops to PV-IOMMU interface
In reply to a message not in the archiveCurrently 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
@@ -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
@@ -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 6struct 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
@@ -566,6 +566,12 @@ static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, intreturn 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
@@ -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, intreturn 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
@@ -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
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.
Signed-off-by Malcolm Crossley <[email protected]>--
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@@ -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.
RFC PATCH 7/7 common/pv-iommu: Allow hardware_domain to pre IOMMU map foreign memory
In reply to a message not in the archiveIf 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
@@ -31,6 +31,8 @@ struct m2b_headDEFINE_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
@@ -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
@@ -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
cover Implement Xen PV-IOMMU interface
In reply to a message not in the archiveThis 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
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: addiommu_lookup_pageto lookup guest gfn for a particular
IOMMU mapping
VT-d: Addiommu_lookup_pagesupport
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: Allowhardware_domainto pre IOMMU map foreign
memoryxen/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
Re: [RFC PATCH 0/7] Implement Xen PV-IOMMU interface
In reply to Malcolm CrossleyQuoted 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: addiommu_lookup_pageto lookup guest gfn for a particular
IOMMU mapping
VT-d: Addiommu_lookup_pagesupport
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: Allowhardware_domainto 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
Re: [RFC PATCH 0/7] Implement Xen PV-IOMMU interface
In reply to Malcolm CrossleyHello.
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: addiommu_lookup_pageto lookup guest gfn for a particular
IOMMU mapping
VT-d: Addiommu_lookup_pagesupport
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: Allowhardware_domainto pre IOMMU map foreign
memoryxen/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 100644xen/common/pv_iommu.c
create mode 100644xen/include/asm-x86/m2b.h
create mode 100644xen/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