* [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
@ 2019-09-18 10:41 Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables Paul Durrant
` (4 more replies)
0 siblings, 5 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-18 10:41 UTC (permalink / raw)
To: xen-devel
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Paul Durrant, Tamas K Lengyel, David Scott,
Anthony PERARD, Volodymyr Babchuk, Roger Pau Monné
These are the remaining uncommitted patches from my previous series:
https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg01208.html
The only patch that has been revised is patch #4 (previously patch #6).
Ian Jackson (1):
tools/ocaml: abi check: Cope with consecutive relevant enums
Paul Durrant (3):
remove late (on-demand) construction of IOMMU page tables
iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros
introduce a 'passthrough' configuration option to xl.cfg...
docs/man/xl.cfg.5.pod.in | 57 +++++++++
tools/libxl/libxl.h | 16 +++
tools/libxl/libxl_create.c | 30 ++++-
tools/libxl/libxl_mem.c | 6 +-
tools/libxl/libxl_types.idl | 9 ++
tools/libxl/libxl_utils.c | 15 +++
tools/libxl/libxl_utils.h | 1 +
tools/ocaml/libs/xc/abi-check | 17 +--
tools/ocaml/libs/xc/xenctrl.ml | 4 +
tools/ocaml/libs/xc/xenctrl.mli | 5 +
tools/ocaml/libs/xc/xenctrl_stubs.c | 17 ++-
tools/xl/xl_parse.c | 178 ++++++++++++++++++--------
xen/arch/arm/Kconfig | 1 +
xen/arch/arm/domain.c | 10 +-
xen/arch/arm/p2m.c | 2 +-
xen/arch/x86/dom0_build.c | 2 +-
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/hvm/mtrr.c | 5 +-
xen/arch/x86/mm/mem_sharing.c | 2 +-
xen/arch/x86/mm/p2m.c | 4 +-
xen/arch/x86/mm/paging.c | 2 +-
xen/arch/x86/x86_64/mm.c | 2 +-
xen/common/domain.c | 7 +
xen/common/domctl.c | 13 --
xen/common/memory.c | 4 +-
xen/common/vm_event.c | 2 +-
xen/drivers/passthrough/Kconfig | 3 +
xen/drivers/passthrough/device_tree.c | 11 --
xen/drivers/passthrough/iommu.c | 147 ++++++---------------
xen/drivers/passthrough/pci.c | 12 --
xen/drivers/passthrough/vtd/iommu.c | 10 +-
xen/drivers/passthrough/x86/iommu.c | 97 --------------
xen/include/asm-arm/iommu.h | 3 -
xen/include/asm-x86/iommu.h | 4 -
xen/include/public/domctl.h | 10 +-
xen/include/xen/iommu.h | 40 +++---
xen/include/xen/sched.h | 8 --
37 files changed, 388 insertions(+), 370 deletions(-)
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: David Scott <dave@recoil.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Petre Pircalabu <ppircalabu@bitdefender.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Wei Liu <wl@xen.org>
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables
2019-09-18 10:41 [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
@ 2019-09-18 10:41 ` Paul Durrant
2019-09-25 9:00 ` Paul Durrant
2019-09-25 9:16 ` Wei Liu
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 2/4] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
` (3 subsequent siblings)
4 siblings, 2 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-18 10:41 UTC (permalink / raw)
To: xen-devel
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Paul Durrant, Tamas K Lengyel,
Jan Beulich, Alexandru Isaila, Volodymyr Babchuk,
Roger Pau Monné
Now that there is a per-domain IOMMU-enable flag, which should be set if
any device is going to be passed through, stop deferring page table
construction until the assignment is done. Also don't tear down the tables
again when the last device is de-assigned; defer that task until domain
destruction.
This allows the has_iommu_pt() helper and iommu_status enumeration to be
removed. Calls to has_iommu_pt() are simply replaced by calls to
is_iommu_enabled(). Remaining open-coded tests of iommu_hap_pt_share can
also be replaced by calls to iommu_use_hap_pt().
The arch_iommu_populate_page_table() and iommu_construct() functions become
redundant, as does the 'strict mode' dom0 page_list mapping code in
iommu_hwdom_init(), and iommu_teardown() can be made static is its only
remaining caller, iommu_domain_destroy(), is within the same source
module.
All in all, about 220 lines of code are removed from the hypervisor (at
the expense of some additions in the toolstack).
NOTE: This patch will cause a small amount of extra resource to be used
to accommodate IOMMU page tables that may never be used, since the
per-domain IOMMU-enable flag is currently set to the value of the
global iommu_enable flag. A subsequent patch will add an option to
the toolstack to allow it to be turned off if there is no intention
to assign passthrough hardware to the domain.
To account for the extra resource, 'iommu_memkb' has been added to
domain_build_info. This patch sets it to a value calculated based
on the domain's maximum memory when the P2M sharing is either not
supported or globally disabled, or zero otherwise. However, when
the toolstack option mentioned above is added, it will also be zero
if the per-domain IOMMU-enable flag is turned off.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Alexandru Isaila <aisaila@bitdefender.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Petre Pircalabu <ppircalabu@bitdefender.com>
Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
v9:
- Avoid the iommu_memkb overhead if the IOMMU is disable or page tables
are shared
v7:
- Add toolstack memory reservation for IOMMU page tables... Re-use of
shadow calculation didn't seem appropriate so a new helper function is
added
v5:
- Minor style fixes
---
tools/libxl/libxl.h | 7 ++
tools/libxl/libxl_mem.c | 6 +-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/libxl_utils.c | 15 +++
tools/libxl/libxl_utils.h | 1 +
tools/xl/xl_parse.c | 24 ++++-
xen/arch/arm/p2m.c | 2 +-
xen/arch/x86/dom0_build.c | 2 +-
xen/arch/x86/hvm/mtrr.c | 5 +-
xen/arch/x86/mm/mem_sharing.c | 2 +-
xen/arch/x86/mm/p2m.c | 4 +-
xen/arch/x86/mm/paging.c | 2 +-
xen/arch/x86/x86_64/mm.c | 2 +-
xen/common/memory.c | 4 +-
xen/common/vm_event.c | 2 +-
xen/drivers/passthrough/device_tree.c | 11 ---
xen/drivers/passthrough/iommu.c | 134 ++++++--------------------
xen/drivers/passthrough/pci.c | 12 ---
xen/drivers/passthrough/vtd/iommu.c | 10 +-
xen/drivers/passthrough/x86/iommu.c | 97 -------------------
xen/include/asm-arm/iommu.h | 2 +-
xen/include/asm-x86/iommu.h | 2 +-
xen/include/xen/iommu.h | 16 ---
xen/include/xen/sched.h | 2 -
24 files changed, 94 insertions(+), 271 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 8169d44bda..12545130df 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -408,6 +408,13 @@
*/
#define LIBXL_HAVE_PHYSINFO_CAP_IOMMU_HAP_PT_SHARE 1
+/*
+ * LIBXL_HAVE_BUILDINFO_IOMMU_MEMKB indicates thate libxl_domain_build_info
+ * has an iommu_memkb field which should be set with the amount of memory
+ * overhead needed by the domain for populating IOMMU page tables.
+ */
+#define LIBXL_HAVE_BUILDINFO_IOMMU_MEMKB 1
+
/*
* libxl ABI compatibility
*
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index 448a2af8fd..fd6f33312e 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -461,15 +461,17 @@ int libxl_domain_need_memory(libxl_ctx *ctx,
if (rc) goto out;
*need_memkb = b_info->target_memkb;
+ *need_memkb += b_info->shadow_memkb + b_info->iommu_memkb;
+
switch (b_info->type) {
case LIBXL_DOMAIN_TYPE_PVH:
case LIBXL_DOMAIN_TYPE_HVM:
- *need_memkb += b_info->shadow_memkb + LIBXL_HVM_EXTRA_MEMORY;
+ *need_memkb += LIBXL_HVM_EXTRA_MEMORY;
if (libxl_defbool_val(b_info->device_model_stubdomain))
*need_memkb += 32 * 1024;
break;
case LIBXL_DOMAIN_TYPE_PV:
- *need_memkb += b_info->shadow_memkb + LIBXL_PV_EXTRA_MEMORY;
+ *need_memkb += LIBXL_PV_EXTRA_MEMORY;
break;
default:
rc = ERROR_INVAL;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 7253d6e0fb..d52c63b6b0 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -486,6 +486,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("target_memkb", MemKB),
("video_memkb", MemKB),
("shadow_memkb", MemKB),
+ ("iommu_memkb", MemKB),
("rtc_timeoffset", uint32),
("exec_ssidref", uint32),
("exec_ssid_label", string),
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index f360f5e228..405733b7e1 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -48,6 +48,21 @@ unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned
return 4 * (256 * smp_cpus + 2 * (maxmem_kb / 1024));
}
+unsigned long libxl_get_required_iommu_memory(unsigned long maxmem_kb)
+{
+ unsigned long iommu_pages = 0, mem_pages = maxmem_kb / 4;
+ unsigned int level;
+
+ /* Assume a 4 level page table with 512 entries per level */
+ for (level = 0; level < 4; level++)
+ {
+ mem_pages = DIV_ROUNDUP(mem_pages, 512);
+ iommu_pages += mem_pages;
+ }
+
+ return iommu_pages * 4;
+}
+
char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
{
unsigned int len;
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 44409afdc4..630ccbe28a 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -24,6 +24,7 @@ const
char *libxl_basename(const char *name); /* returns string from strdup */
unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus);
+unsigned long libxl_get_required_iommu_memory(unsigned long maxmem_kb);
int libxl_name_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
int libxl_domain_qualifier_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e105bda2bb..293f5f730e 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1207,6 +1207,7 @@ void parse_config_data(const char *config_source,
int config_len,
libxl_domain_config *d_config)
{
+ libxl_physinfo physinfo;
const char *buf;
long l, vcpus = 0;
XLU_Config *config;
@@ -1221,10 +1222,22 @@ void parse_config_data(const char *config_source,
int pci_seize = 0;
int i, e;
char *kernel_basename;
+ bool iommu_enabled, iommu_hap_pt_share;
libxl_domain_create_info *c_info = &d_config->c_info;
libxl_domain_build_info *b_info = &d_config->b_info;
+ libxl_physinfo_init(&physinfo);
+ if (libxl_get_physinfo(ctx, &physinfo) != 0) {
+ libxl_physinfo_dispose(&physinfo);
+ fprintf(stderr, "libxl_get_physinfo failed\n");
+ exit(EXIT_FAILURE);
+ }
+
+ iommu_enabled = physinfo.cap_hvm_directio;
+ iommu_hap_pt_share = physinfo.cap_iommu_hap_pt_share;
+ libxl_physinfo_dispose(&physinfo);
+
config= xlu_cfg_init(stderr, config_source);
if (!config) {
fprintf(stderr, "Failed to allocate for configuration\n");
@@ -1448,14 +1461,21 @@ void parse_config_data(const char *config_source,
exit(1);
}
- /* libxl_get_required_shadow_memory() must be called after final values
+ /* libxl_get_required_shadow_memory() and
+ * libxl_get_required_iommu_memory() must be called after final values
* (default or specified) for vcpus and memory are set, because the
- * calculation depends on those values. */
+ * calculations depend on those values. */
b_info->shadow_memkb = !xlu_cfg_get_long(config, "shadow_memory", &l, 0)
? l * 1024
: libxl_get_required_shadow_memory(b_info->max_memkb,
b_info->max_vcpus);
+ /* No IOMMU reservation is needed if either the IOMMU is disabled or it
+ * can share the P2M. */
+ b_info->iommu_memkb = (!iommu_enabled || iommu_hap_pt_share)
+ ? 0
+ : libxl_get_required_iommu_memory(b_info->max_memkb);
+
xlu_cfg_get_defbool(config, "nomigrate", &b_info->disable_migrate, 0);
if (!xlu_cfg_get_long(config, "tsc_mode", &l, 1)) {
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 7f1442932a..692565757e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1056,7 +1056,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
!mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) )
p2m_free_entry(p2m, orig_pte, level);
- if ( has_iommu_pt(p2m->domain) &&
+ if ( is_iommu_enabled(p2m->domain) &&
(lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
{
unsigned int flush_flags = 0;
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index d381784edd..7cfab2dc25 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -365,7 +365,7 @@ unsigned long __init dom0_compute_nr_pages(
}
need_paging = is_hvm_domain(d) &&
- (!iommu_hap_pt_share || !paging_mode_hap(d));
+ (!iommu_use_hap_pt(d) || !paging_mode_hap(d));
for ( ; ; need_paging = false )
{
nr_pages = get_memsize(&dom0_size, avail);
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 7ccd85bcea..5ad15eafe0 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr, 1,
void memory_type_changed(struct domain *d)
{
- if ( (has_iommu_pt(d) || cache_flush_permitted(d)) && d->vcpu && d->vcpu[0] )
+ if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) &&
+ d->vcpu && d->vcpu[0] )
{
p2m_memory_type_changed(d);
flush_all(FLUSH_CACHE);
@@ -831,7 +832,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
return MTRR_TYPE_UNCACHABLE;
}
- if ( !has_iommu_pt(d) && !cache_flush_permitted(d) )
+ if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
{
*ipat = 1;
return MTRR_TYPE_WRBACK;
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index a5fe89e339..efb8821768 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1664,7 +1664,7 @@ int mem_sharing_domctl(struct domain *d, struct xen_domctl_mem_sharing_op *mec)
case XEN_DOMCTL_MEM_SHARING_CONTROL:
{
rc = 0;
- if ( unlikely(has_iommu_pt(d) && mec->u.enable) )
+ if ( unlikely(is_iommu_enabled(d) && mec->u.enable) )
rc = -EXDEV;
else
d->arch.hvm.mem_sharing_enabled = mec->u.enable;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 8a5229ee21..e5e4349dea 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1341,7 +1341,7 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
if ( !paging_mode_translate(p2m->domain) )
{
- if ( !has_iommu_pt(d) )
+ if ( !is_iommu_enabled(d) )
return 0;
return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K,
IOMMUF_readable | IOMMUF_writable);
@@ -1432,7 +1432,7 @@ int clear_identity_p2m_entry(struct domain *d, unsigned long gfn_l)
if ( !paging_mode_translate(d) )
{
- if ( !has_iommu_pt(d) )
+ if ( !is_iommu_enabled(d) )
return 0;
return iommu_legacy_unmap(d, _dfn(gfn_l), PAGE_ORDER_4K);
}
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 69aa228e46..d9a52c4db4 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -213,7 +213,7 @@ int paging_log_dirty_enable(struct domain *d, bool_t log_global)
{
int ret;
- if ( has_iommu_pt(d) && log_global )
+ if ( is_iommu_enabled(d) && log_global )
{
/*
* Refuse to turn on global log-dirty mode
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 795a467462..fa55f3474e 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1434,7 +1434,7 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
* shared or being kept in sync then newly added memory needs to be
* mapped here.
*/
- if ( has_iommu_pt(hardware_domain) &&
+ if ( is_iommu_enabled(hardware_domain) &&
!iommu_use_hap_pt(hardware_domain) &&
!need_iommu_pt_sync(hardware_domain) )
{
diff --git a/xen/common/memory.c b/xen/common/memory.c
index d5aff83f2d..7364fd2c33 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -823,7 +823,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
xatp->gpfn += start;
xatp->size -= start;
- if ( has_iommu_pt(d) )
+ if ( is_iommu_enabled(d) )
this_cpu(iommu_dont_flush_iotlb) = 1;
while ( xatp->size > done )
@@ -844,7 +844,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
}
}
- if ( has_iommu_pt(d) )
+ if ( is_iommu_enabled(d) )
{
int ret;
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 2a1c87e44b..3b18195ebf 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -630,7 +630,7 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec)
/* No paging if iommu is used */
rc = -EMLINK;
- if ( unlikely(has_iommu_pt(d)) )
+ if ( unlikely(is_iommu_enabled(d)) )
break;
rc = -EXDEV;
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 12f2c4c3f2..ea9fd54e3b 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -40,17 +40,6 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
if ( !list_empty(&dev->domain_list) )
goto fail;
- /*
- * The hwdom is forced to use IOMMU for protecting assigned
- * device. Therefore the IOMMU data is already set up.
- */
- ASSERT(!is_hardware_domain(d) ||
- hd->status == IOMMU_STATUS_initialized);
-
- rc = iommu_construct(d);
- if ( rc )
- goto fail;
-
/* The flag field doesn't matter to DT device. */
rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev), 0);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 11ece4d1f3..71f17e7edc 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -152,6 +152,17 @@ static int __init parse_dom0_iommu_param(const char *s)
}
custom_param("dom0-iommu", parse_dom0_iommu_param);
+static void __hwdom_init check_hwdom_reqs(struct domain *d)
+{
+ if ( iommu_hwdom_none || !paging_mode_translate(d) )
+ return;
+
+ arch_iommu_check_autotranslated_hwdom(d);
+
+ iommu_hwdom_passthrough = false;
+ iommu_hwdom_strict = true;
+}
+
int iommu_domain_init(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
@@ -169,129 +180,44 @@ int iommu_domain_init(struct domain *d)
return ret;
hd->platform_ops = iommu_get_ops();
- return hd->platform_ops->init(d);
-}
+ ret = hd->platform_ops->init(d);
+ if ( ret )
+ return ret;
-static void __hwdom_init check_hwdom_reqs(struct domain *d)
-{
- if ( iommu_hwdom_none || !paging_mode_translate(d) )
- return;
+ if ( is_hardware_domain(d) )
+ check_hwdom_reqs(d); /* may modify iommu_hwdom_strict */
- arch_iommu_check_autotranslated_hwdom(d);
+ /*
+ * NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept
+ * in-sync with their assigned pages because all host RAM will be
+ * mapped during hwdom_init().
+ */
+ if ( !is_hardware_domain(d) || iommu_hwdom_strict )
+ hd->need_sync = !iommu_use_hap_pt(d);
- iommu_hwdom_passthrough = false;
- iommu_hwdom_strict = true;
+ return 0;
}
void __hwdom_init iommu_hwdom_init(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
- check_hwdom_reqs(d);
-
if ( !is_iommu_enabled(d) )
return;
register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0);
- hd->status = IOMMU_STATUS_initializing;
- /*
- * NB: relaxed hw domains don't need sync because all ram is already
- * mapped in the iommu page tables.
- */
- hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
- if ( need_iommu_pt_sync(d) )
- {
- struct page_info *page;
- unsigned int i = 0, flush_flags = 0;
- int rc = 0;
-
- page_list_for_each ( page, &d->page_list )
- {
- unsigned long mfn = mfn_x(page_to_mfn(page));
- unsigned long dfn = mfn_to_gmfn(d, mfn);
- unsigned int mapping = IOMMUF_readable;
- int ret;
-
- if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
- ((page->u.inuse.type_info & PGT_type_mask)
- == PGT_writable_page) )
- mapping |= IOMMUF_writable;
-
- ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping,
- &flush_flags);
-
- if ( !rc )
- rc = ret;
-
- if ( !(i++ & 0xfffff) )
- process_pending_softirqs();
- }
-
- /* Use while-break to avoid compiler warning */
- while ( iommu_iotlb_flush_all(d, flush_flags) )
- break;
-
- if ( rc )
- printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
- d->domain_id, rc);
- }
-
hd->platform_ops->hwdom_init(d);
-
- hd->status = IOMMU_STATUS_initialized;
}
-void iommu_teardown(struct domain *d)
+static void iommu_teardown(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
- hd->status = IOMMU_STATUS_disabled;
hd->platform_ops->teardown(d);
tasklet_schedule(&iommu_pt_cleanup_tasklet);
}
-int iommu_construct(struct domain *d)
-{
- struct domain_iommu *hd = dom_iommu(d);
-
- if ( hd->status == IOMMU_STATUS_initialized )
- return 0;
-
- hd->status = IOMMU_STATUS_initializing;
-
- if ( !iommu_use_hap_pt(d) )
- {
- int rc;
-
- hd->need_sync = true;
-
- rc = arch_iommu_populate_page_table(d);
- if ( rc )
- {
- if ( rc != -ERESTART )
- {
- hd->need_sync = false;
- hd->status = IOMMU_STATUS_disabled;
- }
-
- return rc;
- }
- }
-
- hd->status = IOMMU_STATUS_initialized;
-
- /*
- * There may be dirty cache lines when a device is assigned
- * and before has_iommu_pt(d) becoming true, this will cause
- * memory_type_changed lose effect if memory type changes.
- * Call memory_type_changed here to amend this.
- */
- memory_type_changed(d);
-
- return 0;
-}
-
void iommu_domain_destroy(struct domain *d)
{
if ( !is_iommu_enabled(d) )
@@ -587,11 +513,8 @@ int iommu_do_domctl(
void iommu_share_p2m_table(struct domain* d)
{
ASSERT(hap_enabled(d));
- /*
- * iommu_use_hap_pt(d) cannot be used here because during domain
- * construction has_iommu_pt(d) will always return false here.
- */
- if ( is_iommu_enabled(d) && iommu_hap_pt_share )
+
+ if ( iommu_use_hap_pt(d) )
iommu_get_ops()->share_p2m(d);
}
@@ -638,8 +561,7 @@ static void iommu_dump_p2m_table(unsigned char key)
ops = iommu_get_ops();
for_each_domain(d)
{
- if ( is_hardware_domain(d) ||
- dom_iommu(d)->status < IOMMU_STATUS_initialized )
+ if ( is_hardware_domain(d) || !is_iommu_enabled(d) )
continue;
if ( iommu_use_hap_pt(d) )
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index d28f17af75..1fad0af534 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -939,9 +939,6 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
pdev->fault.count = 0;
- if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
- iommu_teardown(d);
-
return ret;
}
@@ -1490,13 +1487,6 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
if ( !pcidevs_trylock() )
return -ERESTART;
- rc = iommu_construct(d);
- if ( rc )
- {
- pcidevs_unlock();
- return rc;
- }
-
pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
if ( !pdev )
{
@@ -1525,8 +1515,6 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
}
done:
- if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
- iommu_teardown(d);
pcidevs_unlock();
return rc;
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index f123760ee2..3c17f11386 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1721,15 +1721,7 @@ static void iommu_domain_teardown(struct domain *d)
ASSERT(is_iommu_enabled(d));
- /*
- * We can't use iommu_use_hap_pt here because either IOMMU state
- * is already changed to IOMMU_STATUS_disabled at this point or
- * has always been IOMMU_STATUS_disabled.
- *
- * We also need to test if HAP is enabled because PV guests can
- * enter this path too.
- */
- if ( hap_enabled(d) && iommu_hap_pt_share )
+ if ( iommu_use_hap_pt(d) )
return;
spin_lock(&hd->arch.mapping_lock);
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 8319fe0a69..47a3e55213 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -81,103 +81,6 @@ int __init iommu_setup_hpet_msi(struct msi_desc *msi)
return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : -ENODEV;
}
-int arch_iommu_populate_page_table(struct domain *d)
-{
- struct page_info *page;
- int rc = 0, n = 0;
-
- spin_lock(&d->page_alloc_lock);
-
- if ( unlikely(d->is_dying) )
- rc = -ESRCH;
-
- while ( !rc && (page = page_list_remove_head(&d->page_list)) )
- {
- if ( is_hvm_domain(d) ||
- (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
- {
- mfn_t mfn = page_to_mfn(page);
- gfn_t gfn = mfn_to_gfn(d, mfn);
- unsigned int flush_flags = 0;
-
- if ( !gfn_eq(gfn, INVALID_GFN) )
- {
- dfn_t dfn = _dfn(gfn_x(gfn));
-
- ASSERT(!(gfn_x(gfn) >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
- BUG_ON(SHARED_M2P(gfn_x(gfn)));
- rc = iommu_map(d, dfn, mfn, PAGE_ORDER_4K,
- IOMMUF_readable | IOMMUF_writable,
- &flush_flags);
-
- /*
- * We may be working behind the back of a running guest, which
- * may change the type of a page at any time. We can't prevent
- * this (for instance, by bumping the type count while mapping
- * the page) without causing legitimate guest type-change
- * operations to fail. So after adding the page to the IOMMU,
- * check again to make sure this is still valid. NB that the
- * writable entry in the iommu is harmless until later, when
- * the actual device gets assigned.
- */
- if ( !rc && !is_hvm_domain(d) &&
- ((page->u.inuse.type_info & PGT_type_mask) !=
- PGT_writable_page) )
- {
- rc = iommu_unmap(d, dfn, PAGE_ORDER_4K, &flush_flags);
- /* If the type changed yet again, simply force a retry. */
- if ( !rc && ((page->u.inuse.type_info & PGT_type_mask) ==
- PGT_writable_page) )
- rc = -ERESTART;
- }
- }
- if ( rc )
- {
- page_list_add(page, &d->page_list);
- break;
- }
- }
- page_list_add_tail(page, &d->arch.relmem_list);
- if ( !(++n & 0xff) && !page_list_empty(&d->page_list) &&
- hypercall_preempt_check() )
- rc = -ERESTART;
- }
-
- if ( !rc )
- {
- /*
- * The expectation here is that generally there are many normal pages
- * on relmem_list (the ones we put there) and only few being in an
- * offline/broken state. The latter ones are always at the head of the
- * list. Hence we first move the whole list, and then move back the
- * first few entries.
- */
- page_list_move(&d->page_list, &d->arch.relmem_list);
- while ( !page_list_empty(&d->page_list) &&
- (page = page_list_first(&d->page_list),
- (page->count_info & (PGC_state|PGC_broken))) )
- {
- page_list_del(page, &d->page_list);
- page_list_add_tail(page, &d->arch.relmem_list);
- }
- }
-
- spin_unlock(&d->page_alloc_lock);
-
- if ( !rc )
- /*
- * flush_flags are not tracked across hypercall pre-emption so
- * assume a full flush is necessary.
- */
- rc = iommu_iotlb_flush_all(
- d, IOMMU_FLUSHF_added | IOMMU_FLUSHF_modified);
-
- if ( rc && rc != -ERESTART )
- iommu_teardown(d);
-
- return rc;
-}
-
void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
{
if ( !is_iommu_enabled(d) )
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index 904c9aec11..1577e83d2b 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -21,7 +21,7 @@ struct arch_iommu
};
/* Always share P2M Table between the CPU and the IOMMU */
-#define iommu_use_hap_pt(d) (has_iommu_pt(d))
+#define iommu_use_hap_pt(d) is_iommu_enabled(d)
const struct iommu_ops *iommu_get_ops(void);
void iommu_set_ops(const struct iommu_ops *ops);
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 31fda4b0cf..5071afd6a5 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -88,7 +88,7 @@ extern const struct iommu_init_ops *iommu_init_ops;
/* Are we using the domain P2M table as its IOMMU pagetable? */
#define iommu_use_hap_pt(d) \
- (hap_enabled(d) && has_iommu_pt(d) && iommu_hap_pt_share)
+ (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index c5ed7efe98..dfec0ca3fc 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -88,15 +88,9 @@ void iommu_domain_destroy(struct domain *d);
void arch_iommu_domain_destroy(struct domain *d);
int arch_iommu_domain_init(struct domain *d);
-int arch_iommu_populate_page_table(struct domain *d);
void arch_iommu_check_autotranslated_hwdom(struct domain *d);
void arch_iommu_hwdom_init(struct domain *d);
-int iommu_construct(struct domain *d);
-
-/* Function used internally, use iommu_domain_destroy */
-void iommu_teardown(struct domain *d);
-
/*
* The following flags are passed to map operations and passed by lookup
* operations.
@@ -263,13 +257,6 @@ struct iommu_ops {
# define iommu_vcall iommu_call
#endif
-enum iommu_status
-{
- IOMMU_STATUS_disabled,
- IOMMU_STATUS_initializing,
- IOMMU_STATUS_initialized
-};
-
struct domain_iommu {
struct arch_iommu arch;
@@ -289,9 +276,6 @@ struct domain_iommu {
/* Features supported by the IOMMU */
DECLARE_BITMAP(features, IOMMU_FEAT_count);
- /* Status of guest IOMMU mappings */
- enum iommu_status status;
-
/*
* Does the guest reqire mappings to be synchonized, to maintain
* the default dfn == pfn map. (See comment on dfn at the top of
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 2d17c84915..ae1faf70d3 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -966,10 +966,8 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
}
#ifdef CONFIG_HAS_PASSTHROUGH
-#define has_iommu_pt(d) (dom_iommu(d)->status != IOMMU_STATUS_disabled)
#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
#else
-#define has_iommu_pt(d) false
#define need_iommu_pt_sync(d) false
#endif
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Xen-devel] [PATCH v13 2/4] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros
2019-09-18 10:41 [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables Paul Durrant
@ 2019-09-18 10:41 ` Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 3/4] tools/ocaml: abi check: Cope with consecutive relevant enums Paul Durrant
` (2 subsequent siblings)
4 siblings, 0 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-18 10:41 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Paul Durrant, Jan Beulich, Volodymyr Babchuk,
Roger Pau Monné
Thes macros really ought to live in the common xen/iommu.h header rather
then being distributed amongst architecture specific iommu headers and
xen/sched.h. This patch moves them there.
NOTE: Disabling 'sharept' in the command line iommu options should really
be hard error on ARM (as opposed to just being ignored), so define
'iommu_hap_pt_share' to be true for ARM (via ARM-selected
CONFIG_IOMMU_FORCE_PT_SHARE).
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Previously part of https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
v11:
- Fix #if/#elif ordering issue
v9:
- Add new Kconfig option to cause 'iommu_hap_pt_share' to be defined to
true, rather than using CONFIG_ARM, as requested by Julien
- Assuming Jan's R-b stands since this is a mainly a cosmetic change
directly requested by another maintainer
v7:
- Re-work the ARM handling of 'sharept' as suggested by Jan
- Make sure that need_iommu_pt_sync() always evaluates its argument
---
xen/arch/arm/Kconfig | 1 +
xen/drivers/passthrough/Kconfig | 3 +++
xen/include/asm-arm/iommu.h | 3 ---
xen/include/asm-x86/iommu.h | 4 ----
xen/include/xen/iommu.h | 15 ++++++++++++++-
xen/include/xen/sched.h | 6 ------
6 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index c2db2a6953..a51aa7bfa8 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -20,6 +20,7 @@ config ARM
select HAS_DEVICE_TREE
select HAS_PASSTHROUGH
select HAS_PDX
+ select IOMMU_FORCE_PT_SHARE
config ARCH_DEFCONFIG
string
diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
index a3c06491be..61f944639e 100644
--- a/xen/drivers/passthrough/Kconfig
+++ b/xen/drivers/passthrough/Kconfig
@@ -13,3 +13,6 @@ config ARM_SMMU
Say Y here if your SoC includes an IOMMU device implementing the
ARM SMMU architecture.
endif
+
+config IOMMU_FORCE_PT_SHARE
+ bool
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index 1577e83d2b..77a94b29eb 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -20,9 +20,6 @@ struct arch_iommu
void *priv;
};
-/* Always share P2M Table between the CPU and the IOMMU */
-#define iommu_use_hap_pt(d) is_iommu_enabled(d)
-
const struct iommu_ops *iommu_get_ops(void);
void iommu_set_ops(const struct iommu_ops *ops);
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 5071afd6a5..85741f7c96 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -86,10 +86,6 @@ struct iommu_init_ops {
extern const struct iommu_init_ops *iommu_init_ops;
-/* Are we using the domain P2M table as its IOMMU pagetable? */
-#define iommu_use_hap_pt(d) \
- (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
-
void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
int iommu_setup_hpet_msi(struct msi_desc *);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index dfec0ca3fc..42a92a3379 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -56,7 +56,9 @@ extern bool_t iommu_enable, iommu_enabled;
extern bool_t force_iommu, iommu_verbose, iommu_igfx;
extern bool_t iommu_snoop, iommu_qinval, iommu_intremap, iommu_intpost;
-#ifdef CONFIG_HVM
+#if defined(CONFIG_IOMMU_FORCE_PT_SHARE)
+#define iommu_hap_pt_share true
+#elif defined(CONFIG_HVM)
extern bool iommu_hap_pt_share;
#else
#define iommu_hap_pt_share false
@@ -288,6 +290,17 @@ struct domain_iommu {
#define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
#define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
+/* Are we using the domain P2M table as its IOMMU pagetable? */
+#define iommu_use_hap_pt(d) \
+ (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
+
+/* Does the IOMMU pagetable need to be kept synchronized with the P2M */
+#ifdef CONFIG_HAS_PASSTHROUGH
+#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
+#else
+#define need_iommu_pt_sync(d) ({ (void)(d); false; })
+#endif
+
int __must_check iommu_suspend(void);
void iommu_resume(void);
void iommu_crash_shutdown(void);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ae1faf70d3..a6896221f9 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -965,12 +965,6 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
cpumask_weight(v->cpu_hard_affinity) == 1);
}
-#ifdef CONFIG_HAS_PASSTHROUGH
-#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
-#else
-#define need_iommu_pt_sync(d) false
-#endif
-
static inline bool is_vcpu_online(const struct vcpu *v)
{
return !test_bit(_VPF_down, &v->pause_flags);
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Xen-devel] [PATCH v13 3/4] tools/ocaml: abi check: Cope with consecutive relevant enums
2019-09-18 10:41 [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 2/4] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
@ 2019-09-18 10:41 ` Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 4/4] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
2019-09-25 8:40 ` [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
4 siblings, 0 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-18 10:41 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson
From: Ian Jackson <ian.jackson@eu.citrix.com>
If the end of one enum is the `type' line for the next enum, we would
not notice it.
Fix this by reordering the code, and getting rid of the else: now if
the "we are within an enum" branch decides that it's the end of the
enum, it unsets $ei and we then immediately process the line as a "not
within an enum" line - ie as the start of the next one.
Reported-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
v13:
- New in this version
---
tools/ocaml/libs/xc/abi-check | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/tools/ocaml/libs/xc/abi-check b/tools/ocaml/libs/xc/abi-check
index d532f37271..3cbdec582f 100755
--- a/tools/ocaml/libs/xc/abi-check
+++ b/tools/ocaml/libs/xc/abi-check
@@ -70,14 +70,7 @@ my $cval;
$ei = undef;
my $bitnum = 0;
while (<OCAML_FILE>) {
- if (!$ei) {
- if (m{^type \s+ (\w+) \s* \= \s* $}x && $enums{$1}) {
- print "// found ocaml type $1 at $o:$.\n" or die $!;
- $ei = $enums{$1};
- $cval = '';
- $bitnum = 0;
- }
- } else {
+ if ($ei) {
if (m{^\s+ \| \s* $ei->{OPrefix} (\w+) \s*$}x) {
$cval = $1;
if ($ei->{Mangle} eq 'lc') {
@@ -104,6 +97,14 @@ while (<OCAML_FILE>) {
die "$_ ?";
}
}
+ if (!$ei) {
+ if (m{^type \s+ (\w+) \s* \= \s* $}x && $enums{$1}) {
+ print "// found ocaml type $1 at $o:$.\n" or die $!;
+ $ei = $enums{$1};
+ $cval = '';
+ $bitnum = 0;
+ }
+ }
}
foreach $ei (values %enums) {
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Xen-devel] [PATCH v13 4/4] introduce a 'passthrough' configuration option to xl.cfg...
2019-09-18 10:41 [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
` (2 preceding siblings ...)
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 3/4] tools/ocaml: abi check: Cope with consecutive relevant enums Paul Durrant
@ 2019-09-18 10:41 ` Paul Durrant
2019-09-18 15:20 ` Anthony PERARD
2019-09-25 8:40 ` [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
4 siblings, 1 reply; 26+ messages in thread
From: Paul Durrant @ 2019-09-18 10:41 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Paul Durrant, Christian Lindig, Jan Beulich,
David Scott, Anthony PERARD, Volodymyr Babchuk,
Roger Pau Monné
...and hence the ability to disable IOMMU mappings, and control EPT
sharing.
This patch introduces a new 'libxl_passthrough' enumeration into
libxl_domain_create_info. The value will be set by xl either when it parses
a new 'passthrough' option in xl.cfg, or implicitly if there is passthrough
hardware specified for the domain.
If the value of the passthrough configuration option is 'disabled' then
the XEN_DOMCTL_CDF_iommu flag will be clear in the xen_domctl_createdomain
flags, thus allowing the toolstack to control whether the domain gets
IOMMU mappings or not (where previously they were globally set).
If the value of the passthrough configuration option is 'sync_pt' then
a new 'iommu_opts' field in xen_domctl_createdomain will be set with the
value XEN_DOMCTL_IOMMU_no_sharept. This will override the global default
set in iommu_hap_pt_share, thus allowing the toolstack to control whether
EPT sharing is used for the domain.
If the value of passthrough is 'enabled' then xl will choose an appropriate
default according to the type of domain and hardware support.
NOTE: The 'iommu_memkb' overhead in libxl_domain_build_info will now only
be set if passthrough is 'sync_pt' (or xl has chosen this mode as
a default).
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: David Scott <dave@recoil.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
v13:
- Small documentation adjustments requested by George and Julien
- Fix default passthrough option
v12:
- Set passthrough default in libxl
v11:
- Fixed abi-check runes
v10:
- Added abi-check runes
v9:
- Added the passthrough='enabled' option to xl
- One cosmetic change in xen
- Assume Jan's R-b stands since non-cosmetic changes are only in the
toolstack
v7:
- Added missing breaks
- Added missing ocaml binding changes
v6:
- Remove the libxl_physinfo() call since it's usefulness is limited to x86
v5:
- Expand xen_domctl_createdomain flags field and hence bump interface
version
- Fix spelling mistakes in context line
---
docs/man/xl.cfg.5.pod.in | 57 ++++++++++
tools/libxl/libxl.h | 9 ++
tools/libxl/libxl_create.c | 30 ++++-
tools/libxl/libxl_types.idl | 8 ++
tools/ocaml/libs/xc/xenctrl.ml | 4 +
tools/ocaml/libs/xc/xenctrl.mli | 5 +
tools/ocaml/libs/xc/xenctrl_stubs.c | 17 ++-
tools/xl/xl_parse.c | 164 +++++++++++++++++++---------
xen/arch/arm/domain.c | 10 +-
xen/arch/x86/domain.c | 2 +-
xen/common/domain.c | 7 ++
xen/common/domctl.c | 13 ---
xen/drivers/passthrough/iommu.c | 13 ++-
xen/include/public/domctl.h | 10 +-
xen/include/xen/iommu.h | 15 ++-
15 files changed, 277 insertions(+), 87 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c99d40307e..7795c73556 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -605,6 +605,63 @@ option should only be used with a trusted device tree.
Note that the partial device tree should avoid using the phandle 65000
which is reserved by the toolstack.
+=item B<passthrough="STRING">
+
+Specify whether IOMMU mappings are enabled for the domain and hence whether
+it will be enabled for passthrough hardware. Valid values for this option
+are:
+
+=over 4
+
+=item B<disabled>
+
+IOMMU mappings are disabled for the domain and so hardware may not be
+passed through.
+
+This option is the default if no passthrough hardware is specified in the
+domain's configuration.
+
+=item B<enabled>
+
+This option enables IOMMU mappings and selects an appropriate default
+operating mode (see below for details of the operating modes). For HVM/PVH
+domains running on platforms where the option is available, this is
+equivalent to B<share_pt>. Otherwise, and also for PV domains, this
+option is equivalent to B<sync_pt>.
+
+This option is the default if passthrough hardware is specified in the
+domain's configuration.
+
+=item B<sync_pt>
+
+This option means that IOMMU mappings will be synchronized with the
+domain's P2M table as follows:
+
+For a PV domain, all writable pages assigned to the domain are identity
+mapped by MFN in the IOMMU page table. Thus a device driver running in the
+domain may program passthrough hardware for DMA using MFN values
+(i.e. host/machine frame numbers) looked up in its P2M.
+
+For an HVM/PVH domain, all non-foreign RAM pages present in its P2M will be
+mapped by GFN in the IOMMU page table. Thus a device driver running in the
+domain may program passthrough hardware using GFN values (i.e. guest
+physical frame numbers) without any further translation.
+
+This option is not currently available on Arm.
+
+=item B<share_pt>
+
+This option is unavailable for a PV domain. For an HVM/PVH domain, this
+option means that the IOMMU will be programmed to directly reference the
+domain's P2M table as its page table. From the point of view of a device
+driver running in the domain this is functionally equivalent to B<sync_pt>
+but places less load on the hypervisor and so should generally be selected
+in preference. However, the availability of this option is hardware
+specific. If B<xl info> reports B<virt_caps> containing
+B<iommu_hap_pt_share> then this option may be used.
+
+=back
+
=back
=head2 Devices
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 12545130df..e4b9c539b6 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -415,6 +415,15 @@
*/
#define LIBXL_HAVE_BUILDINFO_IOMMU_MEMKB 1
+/*
+ * LIBXL_HAVE_CREATEINFO_PASSTHROUGH indicates that
+ * libxl_domain_create_info has a passthrough field (which is a
+ * libxl_passthrough enumeration) that indicates whether device pass-
+ * through is enabled for the domain and, if so, whether the IOMMU and
+ * HAP page tables may be shared or not.
+ */
+#define LIBXL_HAVE_CREATEINFO_PASSTHROUGH 1
+
/*
* libxl ABI compatibility
*
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 59dbcb50a0..3876a72b78 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -30,6 +30,13 @@
int libxl__domain_create_info_setdefault(libxl__gc *gc,
libxl_domain_create_info *c_info)
{
+ libxl_physinfo info;
+ int rc;
+
+ rc = libxl_get_physinfo(CTX, &info);
+ if (rc)
+ return rc;
+
if (!c_info->type) {
LOG(ERROR, "domain type unspecified");
return ERROR_INVAL;
@@ -38,12 +45,6 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
libxl__arch_domain_create_info_setdefault(gc, c_info);
if (c_info->type != LIBXL_DOMAIN_TYPE_PV) {
- libxl_physinfo info;
- int rc = libxl_get_physinfo(CTX, &info);
-
- if (rc)
- return rc;
-
if (info.cap_hap)
libxl_defbool_setdefault(&c_info->hap, true);
else if (info.cap_shadow)
@@ -62,6 +63,13 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
if (!c_info->ssidref)
c_info->ssidref = SECINITSID_DOMU;
+ if (info.cap_hvm_directio &&
+ (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED)) {
+ c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
+ !info.cap_iommu_hap_pt_share) ?
+ LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
+ }
+
return 0;
}
@@ -578,6 +586,16 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
}
+ assert(info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
+ LOG(DETAIL, "passthrough: %s",
+ libxl_passthrough_to_string(info->passthrough));
+
+ if (info->passthrough != LIBXL_PASSTHROUGH_DISABLED)
+ create.flags |= XEN_DOMCTL_CDF_iommu;
+
+ if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
+ create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
+
/* Ultimately, handle is an array of 16 uint8_t, same as uuid */
libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index d52c63b6b0..74987f3f30 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -263,6 +263,13 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
(2, "LINUX")
])
+libxl_passthrough = Enumeration("passthrough", [
+ (0, "enabled"),
+ (1, "disabled"),
+ (2, "sync_pt"),
+ (3, "share_pt"),
+ ])
+
#
# Complex libxl types
#
@@ -408,6 +415,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
("pool_name", string),
("run_hotplug_scripts",libxl_defbool),
("driver_domain",libxl_defbool),
+ ("passthrough", libxl_passthrough),
], dir=DIR_IN)
libxl_domain_restore_params = Struct("domain_restore_params", [
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index de4bae6012..e00a74d48d 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -65,11 +65,15 @@ type domain_create_flag =
| CDF_XS_DOMAIN
| CDF_IOMMU
+type domain_create_iommu_opts =
+ | IOMMU_NO_SHAREPT
+
type domctl_create_config =
{
ssidref: int32;
handle: string;
flags: domain_create_flag list;
+ iommu_opts: domain_create_iommu_opts list;
max_vcpus: int;
max_evtchn_port: int;
max_grant_frames: int;
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index c885e75895..0e7049d708 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -57,10 +57,15 @@ type domain_create_flag =
| CDF_OOS_OFF
| CDF_XS_DOMAIN
| CDF_IOMMU
+
+type domain_create_iommu_opts =
+ | IOMMU_NO_SHAREPT
+
type domctl_create_config = {
ssidref: int32;
handle: string;
flags: domain_create_flag list;
+ iommu_opts: domain_create_iommu_opts list;
max_vcpus: int;
max_evtchn_port: int;
max_grant_frames: int;
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 0cdd873599..48f39f81d5 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -190,11 +190,12 @@ CAMLprim value stub_xc_domain_create(value xch, value config)
#define VAL_SSIDREF Field(config, 0)
#define VAL_HANDLE Field(config, 1)
#define VAL_FLAGS Field(config, 2)
-#define VAL_MAX_VCPUS Field(config, 3)
-#define VAL_MAX_EVTCHN_PORT Field(config, 4)
-#define VAL_MAX_GRANT_FRAMES Field(config, 5)
-#define VAL_MAX_MAPTRACK_FRAMES Field(config, 6)
-#define VAL_ARCH Field(config, 7)
+#define VAL_IOMMU_OPTS Field(config, 3)
+#define VAL_MAX_VCPUS Field(config, 4)
+#define VAL_MAX_EVTCHN_PORT Field(config, 5)
+#define VAL_MAX_GRANT_FRAMES Field(config, 6)
+#define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
+#define VAL_ARCH Field(config, 8)
uint32_t domid = 0;
int result;
@@ -213,6 +214,11 @@ CAMLprim value stub_xc_domain_create(value xch, value config)
/* ! XEN_DOMCTL_CDF_ XEN_DOMCTL_CDF_MAX max */
(VAL_FLAGS);
+ cfg.iommu_opts = ocaml_list_to_c_bitmap
+ /* ! domain_create_iommu_opts IOMMU_ lc */
+ /* ! XEN_DOMCTL_IOMMU_ XEN_DOMCTL_IOMMU_MAX max */
+ (VAL_IOMMU_OPTS);
+
arch_domconfig = Field(VAL_ARCH, 0);
switch ( Tag_val(VAL_ARCH) )
{
@@ -247,6 +253,7 @@ CAMLprim value stub_xc_domain_create(value xch, value config)
#undef VAL_MAX_GRANT_FRAMES
#undef VAL_MAX_EVTCHN_PORT
#undef VAL_MAX_VCPUS
+#undef VAL_IOMMU_OPTS
#undef VAL_FLAGS
#undef VAL_HANDLE
#undef VAL_SSIDREF
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 293f5f730e..17b458e94c 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1461,6 +1461,113 @@ void parse_config_data(const char *config_source,
exit(1);
}
+ if (!xlu_cfg_get_list (config, "pci", &pcis, 0, 0)) {
+ d_config->num_pcidevs = 0;
+ d_config->pcidevs = NULL;
+ for(i = 0; (buf = xlu_cfg_get_listitem (pcis, i)) != NULL; i++) {
+ libxl_device_pci *pcidev;
+
+ pcidev = ARRAY_EXTEND_INIT_NODEVID(d_config->pcidevs,
+ d_config->num_pcidevs,
+ libxl_device_pci_init);
+ pcidev->msitranslate = pci_msitranslate;
+ pcidev->power_mgmt = pci_power_mgmt;
+ pcidev->permissive = pci_permissive;
+ pcidev->seize = pci_seize;
+ /*
+ * Like other pci option, the per-device policy always follows
+ * the global policy by default.
+ */
+ pcidev->rdm_policy = b_info->u.hvm.rdm.policy;
+ e = xlu_pci_parse_bdf(config, pcidev, buf);
+ if (e) {
+ fprintf(stderr,
+ "unable to parse PCI BDF `%s' for passthrough\n",
+ buf);
+ exit(-e);
+ }
+ }
+ if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
+ libxl_defbool_set(&b_info->u.pv.e820_host, true);
+ }
+
+ if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
+ d_config->num_dtdevs = 0;
+ d_config->dtdevs = NULL;
+ for (i = 0; (buf = xlu_cfg_get_listitem(dtdevs, i)) != NULL; i++) {
+ libxl_device_dtdev *dtdev;
+
+ dtdev = ARRAY_EXTEND_INIT_NODEVID(d_config->dtdevs,
+ d_config->num_dtdevs,
+ libxl_device_dtdev_init);
+
+ dtdev->path = strdup(buf);
+ if (dtdev->path == NULL) {
+ fprintf(stderr, "unable to duplicate string for dtdevs\n");
+ exit(-1);
+ }
+ }
+ }
+
+ if (xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
+ c_info->passthrough =
+ (d_config->num_pcidevs || d_config->num_dtdevs)
+ ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED;
+ } else {
+ libxl_passthrough o;
+
+ e = libxl_passthrough_from_string(buf, &o);
+ if (e) {
+ fprintf(stderr,
+ "ERROR: unknown passthrough option '%s'\n",
+ buf);
+ exit(-ERROR_FAIL);
+ }
+
+ c_info->passthrough = o;
+ }
+
+ switch (c_info->passthrough) {
+ case LIBXL_PASSTHROUGH_ENABLED:
+ /*
+ * Choose a suitable default. libxl would also do this but
+ * choosing here allows the code calculating 'iommu_memkb'
+ * below make an informed decision.
+ */
+ c_info->passthrough =
+ (c_info->type == LIBXL_DOMAIN_TYPE_PV) || !iommu_hap_pt_share
+ ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
+ break;
+
+ case LIBXL_PASSTHROUGH_DISABLED:
+ if (d_config->num_pcidevs || d_config->num_dtdevs) {
+ fprintf(stderr,
+ "ERROR: passthrough disabled but devices are specified\n");
+ exit(-ERROR_FAIL);
+ }
+ break;
+ case LIBXL_PASSTHROUGH_SHARE_PT:
+ if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
+ fprintf(stderr,
+ "ERROR: passthrough=\"share_pt\" not valid for PV domain\n");
+ exit(-ERROR_FAIL);
+ } else if (!iommu_hap_pt_share) {
+ fprintf(stderr,
+ "ERROR: passthrough=\"share_pt\" not supported on this platform\n");
+ exit(-ERROR_FAIL);
+ }
+ break;
+ case LIBXL_PASSTHROUGH_SYNC_PT:
+ break;
+ }
+
+ if ((c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED) &&
+ !iommu_enabled) {
+ fprintf(stderr,
+ "ERROR: passthrough not supported on this platform\n");
+ exit(-ERROR_FAIL);
+ }
+
/* libxl_get_required_shadow_memory() and
* libxl_get_required_iommu_memory() must be called after final values
* (default or specified) for vcpus and memory are set, because the
@@ -1470,11 +1577,10 @@ void parse_config_data(const char *config_source,
: libxl_get_required_shadow_memory(b_info->max_memkb,
b_info->max_vcpus);
- /* No IOMMU reservation is needed if either the IOMMU is disabled or it
- * can share the P2M. */
- b_info->iommu_memkb = (!iommu_enabled || iommu_hap_pt_share)
- ? 0
- : libxl_get_required_iommu_memory(b_info->max_memkb);
+ /* No IOMMU reservation is needed if passthrough mode is not 'sync_pt' */
+ b_info->iommu_memkb = (c_info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
+ ? libxl_get_required_iommu_memory(b_info->max_memkb)
+ : 0;
xlu_cfg_get_defbool(config, "nomigrate", &b_info->disable_migrate, 0);
@@ -2298,54 +2404,6 @@ skip_vfb:
}
}
- if (!xlu_cfg_get_list (config, "pci", &pcis, 0, 0)) {
- d_config->num_pcidevs = 0;
- d_config->pcidevs = NULL;
- for(i = 0; (buf = xlu_cfg_get_listitem (pcis, i)) != NULL; i++) {
- libxl_device_pci *pcidev;
-
- pcidev = ARRAY_EXTEND_INIT_NODEVID(d_config->pcidevs,
- d_config->num_pcidevs,
- libxl_device_pci_init);
- pcidev->msitranslate = pci_msitranslate;
- pcidev->power_mgmt = pci_power_mgmt;
- pcidev->permissive = pci_permissive;
- pcidev->seize = pci_seize;
- /*
- * Like other pci option, the per-device policy always follows
- * the global policy by default.
- */
- pcidev->rdm_policy = b_info->u.hvm.rdm.policy;
- e = xlu_pci_parse_bdf(config, pcidev, buf);
- if (e) {
- fprintf(stderr,
- "unable to parse PCI BDF `%s' for passthrough\n",
- buf);
- exit(-e);
- }
- }
- if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
- libxl_defbool_set(&b_info->u.pv.e820_host, true);
- }
-
- if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
- d_config->num_dtdevs = 0;
- d_config->dtdevs = NULL;
- for (i = 0; (buf = xlu_cfg_get_listitem(dtdevs, i)) != NULL; i++) {
- libxl_device_dtdev *dtdev;
-
- dtdev = ARRAY_EXTEND_INIT_NODEVID(d_config->dtdevs,
- d_config->num_dtdevs,
- libxl_device_dtdev_init);
-
- dtdev->path = strdup(buf);
- if (dtdev->path == NULL) {
- fprintf(stderr, "unable to duplicate string for dtdevs\n");
- exit(-1);
- }
- }
- }
-
if (!xlu_cfg_get_list(config, "usbctrl", &usbctrls, 0, 0)) {
d_config->num_usbctrls = 0;
d_config->usbctrls = NULL;
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index ae13e47e86..61d35cd120 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -617,6 +617,14 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ /* The P2M table must always be shared between the CPU and the IOMMU */
+ if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept )
+ {
+ dprintk(XENLOG_INFO,
+ "Unsupported iommu option: XEN_DOMCTL_IOMMU_no_sharept\n");
+ return -EINVAL;
+ }
+
/* Fill in the native GIC version, passed back to the toolstack. */
if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
{
@@ -677,7 +685,7 @@ int arch_domain_create(struct domain *d,
ASSERT(config != NULL);
/* p2m_init relies on some value initialized by the IOMMU subsystem */
- if ( (rc = iommu_domain_init(d)) != 0 )
+ if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
goto fail;
if ( (rc = p2m_init(d)) != 0 )
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index dbdf6b1bc2..c0faf68852 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -611,7 +611,7 @@ int arch_domain_create(struct domain *d,
if ( (rc = init_domain_irq_mapping(d)) != 0 )
goto fail;
- if ( (rc = iommu_domain_init(d)) != 0 )
+ if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
goto fail;
psr_domain_init(d);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 4681f29c8b..0733ee8b0a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -308,6 +308,13 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ if ( !(config->flags & XEN_DOMCTL_CDF_iommu) && config->iommu_opts )
+ {
+ dprintk(XENLOG_INFO,
+ "IOMMU options specified but IOMMU not enabled\n");
+ return -EINVAL;
+ }
+
if ( config->max_vcpus < 1 )
{
dprintk(XENLOG_INFO, "No vCPUS\n");
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 5dcfe3c8f6..6e6e9b9866 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -515,19 +515,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
rover = dom;
}
- /*
- * For now, make sure the createdomain IOMMU flag is set if the
- * IOMMU is enabled. When the flag comes under toolstack control
- * this can go away.
- */
- if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_iommu )
- {
- ASSERT_UNREACHABLE();
- return -EINVAL;
- }
- if ( iommu_enabled )
- op->u.createdomain.flags |= XEN_DOMCTL_CDF_iommu;
-
d = domain_create(dom, &op->u.createdomain, false);
if ( IS_ERR(d) )
{
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 71f17e7edc..e8fddc2dc7 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -163,7 +163,7 @@ static void __hwdom_init check_hwdom_reqs(struct domain *d)
iommu_hwdom_strict = true;
}
-int iommu_domain_init(struct domain *d)
+int iommu_domain_init(struct domain *d, unsigned int opts)
{
struct domain_iommu *hd = dom_iommu(d);
int ret = 0;
@@ -187,6 +187,15 @@ int iommu_domain_init(struct domain *d)
if ( is_hardware_domain(d) )
check_hwdom_reqs(d); /* may modify iommu_hwdom_strict */
+ /*
+ * Use shared page tables for HAP and IOMMU if the global option
+ * is enabled (from which we can infer the h/w is capable) and
+ * the domain options do not disallow it. HAP must, of course, also
+ * be enabled.
+ */
+ hd->hap_pt_share = hap_enabled(d) && iommu_hap_pt_share &&
+ !(opts & XEN_DOMCTL_IOMMU_no_sharept);
+
/*
* NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept
* in-sync with their assigned pages because all host RAM will be
@@ -195,6 +204,8 @@ int iommu_domain_init(struct domain *d)
if ( !is_hardware_domain(d) || iommu_hwdom_strict )
hd->need_sync = !iommu_use_hap_pt(d);
+ ASSERT(!(hd->need_sync && hd->hap_pt_share));
+
return 0;
}
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 1b3176adb5..ba84aea6ab 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -38,7 +38,7 @@
#include "hvm/save.h"
#include "memory.h"
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000011
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000012
/*
* NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -73,6 +73,14 @@ struct xen_domctl_createdomain {
uint32_t flags;
+#define _XEN_DOMCTL_IOMMU_no_sharept 0
+#define XEN_DOMCTL_IOMMU_no_sharept (1U << _XEN_DOMCTL_IOMMU_no_sharept)
+
+/* Max XEN_DOMCTL_IOMMU_* constant. Used for ABI checking. */
+#define XEN_DOMCTL_IOMMU_MAX XEN_DOMCTL_IOMMU_no_sharept
+
+ uint32_t iommu_opts;
+
/*
* Various domain limits, which impact the quantity of resources (global
* mapping space, xenheap, etc) a guest may consume.
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 42a92a3379..7c3003f3f1 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -84,7 +84,7 @@ extern unsigned int iommu_dev_iotlb_timeout;
int iommu_setup(void);
int iommu_hardware_setup(void);
-int iommu_domain_init(struct domain *d);
+int iommu_domain_init(struct domain *d, unsigned int opts);
void iommu_hwdom_init(struct domain *d);
void iommu_domain_destroy(struct domain *d);
@@ -278,10 +278,14 @@ struct domain_iommu {
/* Features supported by the IOMMU */
DECLARE_BITMAP(features, IOMMU_FEAT_count);
+ /* Does the guest share HAP mapping with the IOMMU? */
+ bool hap_pt_share;
+
/*
- * Does the guest reqire mappings to be synchonized, to maintain
- * the default dfn == pfn map. (See comment on dfn at the top of
- * include/xen/mm.h).
+ * Does the guest require mappings to be synchronized, to maintain
+ * the default dfn == pfn map? (See comment on dfn at the top of
+ * include/xen/mm.h). Note that hap_pt_share == false does not
+ * necessarily imply this is true.
*/
bool need_sync;
};
@@ -291,8 +295,7 @@ struct domain_iommu {
#define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
/* Are we using the domain P2M table as its IOMMU pagetable? */
-#define iommu_use_hap_pt(d) \
- (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
+#define iommu_use_hap_pt(d) (dom_iommu(d)->hap_pt_share)
/* Does the IOMMU pagetable need to be kept synchronized with the P2M */
#ifdef CONFIG_HAS_PASSTHROUGH
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 4/4] introduce a 'passthrough' configuration option to xl.cfg...
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 4/4] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
@ 2019-09-18 15:20 ` Anthony PERARD
0 siblings, 0 replies; 26+ messages in thread
From: Anthony PERARD @ 2019-09-18 15:20 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Christian Lindig, Jan Beulich, David Scott,
xen-devel, Volodymyr Babchuk, Roger Pau Monné
On Wed, Sep 18, 2019 at 11:41:13AM +0100, Paul Durrant wrote:
> ...and hence the ability to disable IOMMU mappings, and control EPT
> sharing.
>
> This patch introduces a new 'libxl_passthrough' enumeration into
> libxl_domain_create_info. The value will be set by xl either when it parses
> a new 'passthrough' option in xl.cfg, or implicitly if there is passthrough
> hardware specified for the domain.
>
> If the value of the passthrough configuration option is 'disabled' then
> the XEN_DOMCTL_CDF_iommu flag will be clear in the xen_domctl_createdomain
> flags, thus allowing the toolstack to control whether the domain gets
> IOMMU mappings or not (where previously they were globally set).
>
> If the value of the passthrough configuration option is 'sync_pt' then
> a new 'iommu_opts' field in xen_domctl_createdomain will be set with the
> value XEN_DOMCTL_IOMMU_no_sharept. This will override the global default
> set in iommu_hap_pt_share, thus allowing the toolstack to control whether
> EPT sharing is used for the domain.
>
> If the value of passthrough is 'enabled' then xl will choose an appropriate
> default according to the type of domain and hardware support.
>
> NOTE: The 'iommu_memkb' overhead in libxl_domain_build_info will now only
> be set if passthrough is 'sync_pt' (or xl has chosen this mode as
> a default).
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Christian Lindig <christian.lindig@citrix.com>
> Acked-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-18 10:41 [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
` (3 preceding siblings ...)
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 4/4] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
@ 2019-09-25 8:40 ` Paul Durrant
2019-09-25 8:50 ` Jan Beulich
4 siblings, 1 reply; 26+ messages in thread
From: Paul Durrant @ 2019-09-25 8:40 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, David Scott, Anthony Perard,
Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
Ping? I don't think this series is awaiting any more acks.
Paul
> -----Original Message-----
> From: Paul Durrant <paul.durrant@citrix.com>
> Sent: 18 September 2019 11:41
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Anthony Perard
> <anthony.perard@citrix.com>; David Scott <dave@recoil.org>; George Dunlap <George.Dunlap@citrix.com>;
> Ian Jackson <Ian.Jackson@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Petre Pircalabu
> <ppircalabu@bitdefender.com>; Roger Pau Monne <roger.pau@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Tamas K Lengyel <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Wei Liu <wl@xen.org>
> Subject: [PATCH v13 0/4] add per-domain IOMMU control
>
> These are the remaining uncommitted patches from my previous series:
>
> https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg01208.html
>
> The only patch that has been revised is patch #4 (previously patch #6).
>
> Ian Jackson (1):
> tools/ocaml: abi check: Cope with consecutive relevant enums
>
> Paul Durrant (3):
> remove late (on-demand) construction of IOMMU page tables
> iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros
> introduce a 'passthrough' configuration option to xl.cfg...
>
> docs/man/xl.cfg.5.pod.in | 57 +++++++++
> tools/libxl/libxl.h | 16 +++
> tools/libxl/libxl_create.c | 30 ++++-
> tools/libxl/libxl_mem.c | 6 +-
> tools/libxl/libxl_types.idl | 9 ++
> tools/libxl/libxl_utils.c | 15 +++
> tools/libxl/libxl_utils.h | 1 +
> tools/ocaml/libs/xc/abi-check | 17 +--
> tools/ocaml/libs/xc/xenctrl.ml | 4 +
> tools/ocaml/libs/xc/xenctrl.mli | 5 +
> tools/ocaml/libs/xc/xenctrl_stubs.c | 17 ++-
> tools/xl/xl_parse.c | 178 ++++++++++++++++++--------
> xen/arch/arm/Kconfig | 1 +
> xen/arch/arm/domain.c | 10 +-
> xen/arch/arm/p2m.c | 2 +-
> xen/arch/x86/dom0_build.c | 2 +-
> xen/arch/x86/domain.c | 2 +-
> xen/arch/x86/hvm/mtrr.c | 5 +-
> xen/arch/x86/mm/mem_sharing.c | 2 +-
> xen/arch/x86/mm/p2m.c | 4 +-
> xen/arch/x86/mm/paging.c | 2 +-
> xen/arch/x86/x86_64/mm.c | 2 +-
> xen/common/domain.c | 7 +
> xen/common/domctl.c | 13 --
> xen/common/memory.c | 4 +-
> xen/common/vm_event.c | 2 +-
> xen/drivers/passthrough/Kconfig | 3 +
> xen/drivers/passthrough/device_tree.c | 11 --
> xen/drivers/passthrough/iommu.c | 147 ++++++---------------
> xen/drivers/passthrough/pci.c | 12 --
> xen/drivers/passthrough/vtd/iommu.c | 10 +-
> xen/drivers/passthrough/x86/iommu.c | 97 --------------
> xen/include/asm-arm/iommu.h | 3 -
> xen/include/asm-x86/iommu.h | 4 -
> xen/include/public/domctl.h | 10 +-
> xen/include/xen/iommu.h | 40 +++---
> xen/include/xen/sched.h | 8 --
> 37 files changed, 388 insertions(+), 370 deletions(-)
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: David Scott <dave@recoil.org>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Cc: Wei Liu <wl@xen.org>
> --
> 2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 8:40 ` [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
@ 2019-09-25 8:50 ` Jan Beulich
2019-09-25 8:56 ` Paul Durrant
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2019-09-25 8:50 UTC (permalink / raw)
To: Paul Durrant
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, David Scott, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, Ian Jackson, Anthony Perard,
xen-devel, Volodymyr Babchuk, Roger Pau Monne
On 25.09.2019 10:40, Paul Durrant wrote:
> Ping? I don't think this series is awaiting any more acks.
I did enumerate the other day on irc what is still missing according
to my records. Would you mind pointing me at a libxl maintainer ack
for patch 1? I think I've managed to spot the missing one for patch 3,
which hung off a "REPOST" thread. And I realize I was wrongly looking
for a tool stack maintainer ack for patch 4, when a libxl one is
sufficient and already there.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 8:50 ` Jan Beulich
@ 2019-09-25 8:56 ` Paul Durrant
2019-09-25 15:49 ` Oleksandr
0 siblings, 1 reply; 26+ messages in thread
From: Paul Durrant @ 2019-09-25 8:56 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, David Scott, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, Ian Jackson, Anthony Perard,
xen-devel, Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 25 September 2019 09:51
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Petre Pircalabu <ppircalabu@bitdefender.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Stefano Stabellini
> <sstabellini@kernel.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; David Scott
> <dave@recoil.org>; Tamas K Lengyel <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu
> <wl@xen.org>
> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>
> On 25.09.2019 10:40, Paul Durrant wrote:
> > Ping? I don't think this series is awaiting any more acks.
>
> I did enumerate the other day on irc what is still missing according
> to my records.
Oh, I must have dropped off an missed that. Connectivity can be somewhat flakey here in the office :-(
> Would you mind pointing me at a libxl maintainer ack
> for patch 1? I think I've managed to spot the missing one for patch 3,
> which hung off a "REPOST" thread. And I realize I was wrongly looking
> for a tool stack maintainer ack for patch 4, when a libxl one is
> sufficient and already there.
I thought I'd had an toolstack ack on patch #1 but now that I look again, you're right... it's still missing. I'll chase that one.
Thanks,
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables Paul Durrant
@ 2019-09-25 9:00 ` Paul Durrant
2019-09-25 9:16 ` Wei Liu
1 sibling, 0 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-25 9:00 UTC (permalink / raw)
To: Paul Durrant, Ian Jackson, Wei Liu, Anthony Perard
Cc: xen-devel (xen-devel@lists.xenproject.org)
Libxl maintainers... ping?
> -----Original Message-----
> From: Paul Durrant <paul.durrant@citrix.com>
> Sent: 18 September 2019 11:41
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Alexandru Isaila <aisaila@bitdefender.com>; Razvan
> Cojocaru <rcojocaru@bitdefender.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall
> <julien.grall@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu <wl@xen.org>; Roger Pau Monne
> <roger.pau@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; George Dunlap
> <George.Dunlap@citrix.com>; Petre Pircalabu <ppircalabu@bitdefender.com>
> Subject: [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables
>
> Now that there is a per-domain IOMMU-enable flag, which should be set if
> any device is going to be passed through, stop deferring page table
> construction until the assignment is done. Also don't tear down the tables
> again when the last device is de-assigned; defer that task until domain
> destruction.
>
> This allows the has_iommu_pt() helper and iommu_status enumeration to be
> removed. Calls to has_iommu_pt() are simply replaced by calls to
> is_iommu_enabled(). Remaining open-coded tests of iommu_hap_pt_share can
> also be replaced by calls to iommu_use_hap_pt().
> The arch_iommu_populate_page_table() and iommu_construct() functions become
> redundant, as does the 'strict mode' dom0 page_list mapping code in
> iommu_hwdom_init(), and iommu_teardown() can be made static is its only
> remaining caller, iommu_domain_destroy(), is within the same source
> module.
>
> All in all, about 220 lines of code are removed from the hypervisor (at
> the expense of some additions in the toolstack).
>
> NOTE: This patch will cause a small amount of extra resource to be used
> to accommodate IOMMU page tables that may never be used, since the
> per-domain IOMMU-enable flag is currently set to the value of the
> global iommu_enable flag. A subsequent patch will add an option to
> the toolstack to allow it to be turned off if there is no intention
> to assign passthrough hardware to the domain.
> To account for the extra resource, 'iommu_memkb' has been added to
> domain_build_info. This patch sets it to a value calculated based
> on the domain's maximum memory when the P2M sharing is either not
> supported or globally disabled, or zero otherwise. However, when
> the toolstack option mentioned above is added, it will also be zero
> if the per-domain IOMMU-enable flag is turned off.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Alexandru Isaila <aisaila@bitdefender.com>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Julien Grall <julien.grall@arm.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>
>
> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
>
> v9:
> - Avoid the iommu_memkb overhead if the IOMMU is disable or page tables
> are shared
>
> v7:
> - Add toolstack memory reservation for IOMMU page tables... Re-use of
> shadow calculation didn't seem appropriate so a new helper function is
> added
>
> v5:
> - Minor style fixes
> ---
> tools/libxl/libxl.h | 7 ++
> tools/libxl/libxl_mem.c | 6 +-
> tools/libxl/libxl_types.idl | 1 +
> tools/libxl/libxl_utils.c | 15 +++
> tools/libxl/libxl_utils.h | 1 +
> tools/xl/xl_parse.c | 24 ++++-
> xen/arch/arm/p2m.c | 2 +-
> xen/arch/x86/dom0_build.c | 2 +-
> xen/arch/x86/hvm/mtrr.c | 5 +-
> xen/arch/x86/mm/mem_sharing.c | 2 +-
> xen/arch/x86/mm/p2m.c | 4 +-
> xen/arch/x86/mm/paging.c | 2 +-
> xen/arch/x86/x86_64/mm.c | 2 +-
> xen/common/memory.c | 4 +-
> xen/common/vm_event.c | 2 +-
> xen/drivers/passthrough/device_tree.c | 11 ---
> xen/drivers/passthrough/iommu.c | 134 ++++++--------------------
> xen/drivers/passthrough/pci.c | 12 ---
> xen/drivers/passthrough/vtd/iommu.c | 10 +-
> xen/drivers/passthrough/x86/iommu.c | 97 -------------------
> xen/include/asm-arm/iommu.h | 2 +-
> xen/include/asm-x86/iommu.h | 2 +-
> xen/include/xen/iommu.h | 16 ---
> xen/include/xen/sched.h | 2 -
> 24 files changed, 94 insertions(+), 271 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 8169d44bda..12545130df 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -408,6 +408,13 @@
> */
> #define LIBXL_HAVE_PHYSINFO_CAP_IOMMU_HAP_PT_SHARE 1
>
> +/*
> + * LIBXL_HAVE_BUILDINFO_IOMMU_MEMKB indicates thate libxl_domain_build_info
> + * has an iommu_memkb field which should be set with the amount of memory
> + * overhead needed by the domain for populating IOMMU page tables.
> + */
> +#define LIBXL_HAVE_BUILDINFO_IOMMU_MEMKB 1
> +
> /*
> * libxl ABI compatibility
> *
> diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
> index 448a2af8fd..fd6f33312e 100644
> --- a/tools/libxl/libxl_mem.c
> +++ b/tools/libxl/libxl_mem.c
> @@ -461,15 +461,17 @@ int libxl_domain_need_memory(libxl_ctx *ctx,
> if (rc) goto out;
>
> *need_memkb = b_info->target_memkb;
> + *need_memkb += b_info->shadow_memkb + b_info->iommu_memkb;
> +
> switch (b_info->type) {
> case LIBXL_DOMAIN_TYPE_PVH:
> case LIBXL_DOMAIN_TYPE_HVM:
> - *need_memkb += b_info->shadow_memkb + LIBXL_HVM_EXTRA_MEMORY;
> + *need_memkb += LIBXL_HVM_EXTRA_MEMORY;
> if (libxl_defbool_val(b_info->device_model_stubdomain))
> *need_memkb += 32 * 1024;
> break;
> case LIBXL_DOMAIN_TYPE_PV:
> - *need_memkb += b_info->shadow_memkb + LIBXL_PV_EXTRA_MEMORY;
> + *need_memkb += LIBXL_PV_EXTRA_MEMORY;
> break;
> default:
> rc = ERROR_INVAL;
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 7253d6e0fb..d52c63b6b0 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -486,6 +486,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("target_memkb", MemKB),
> ("video_memkb", MemKB),
> ("shadow_memkb", MemKB),
> + ("iommu_memkb", MemKB),
> ("rtc_timeoffset", uint32),
> ("exec_ssidref", uint32),
> ("exec_ssid_label", string),
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index f360f5e228..405733b7e1 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -48,6 +48,21 @@ unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned
> return 4 * (256 * smp_cpus + 2 * (maxmem_kb / 1024));
> }
>
> +unsigned long libxl_get_required_iommu_memory(unsigned long maxmem_kb)
> +{
> + unsigned long iommu_pages = 0, mem_pages = maxmem_kb / 4;
> + unsigned int level;
> +
> + /* Assume a 4 level page table with 512 entries per level */
> + for (level = 0; level < 4; level++)
> + {
> + mem_pages = DIV_ROUNDUP(mem_pages, 512);
> + iommu_pages += mem_pages;
> + }
> +
> + return iommu_pages * 4;
> +}
> +
> char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
> {
> unsigned int len;
> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> index 44409afdc4..630ccbe28a 100644
> --- a/tools/libxl/libxl_utils.h
> +++ b/tools/libxl/libxl_utils.h
> @@ -24,6 +24,7 @@ const
> char *libxl_basename(const char *name); /* returns string from strdup */
>
> unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus);
> +unsigned long libxl_get_required_iommu_memory(unsigned long maxmem_kb);
> int libxl_name_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
> int libxl_domain_qualifier_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
> char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index e105bda2bb..293f5f730e 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1207,6 +1207,7 @@ void parse_config_data(const char *config_source,
> int config_len,
> libxl_domain_config *d_config)
> {
> + libxl_physinfo physinfo;
> const char *buf;
> long l, vcpus = 0;
> XLU_Config *config;
> @@ -1221,10 +1222,22 @@ void parse_config_data(const char *config_source,
> int pci_seize = 0;
> int i, e;
> char *kernel_basename;
> + bool iommu_enabled, iommu_hap_pt_share;
>
> libxl_domain_create_info *c_info = &d_config->c_info;
> libxl_domain_build_info *b_info = &d_config->b_info;
>
> + libxl_physinfo_init(&physinfo);
> + if (libxl_get_physinfo(ctx, &physinfo) != 0) {
> + libxl_physinfo_dispose(&physinfo);
> + fprintf(stderr, "libxl_get_physinfo failed\n");
> + exit(EXIT_FAILURE);
> + }
> +
> + iommu_enabled = physinfo.cap_hvm_directio;
> + iommu_hap_pt_share = physinfo.cap_iommu_hap_pt_share;
> + libxl_physinfo_dispose(&physinfo);
> +
> config= xlu_cfg_init(stderr, config_source);
> if (!config) {
> fprintf(stderr, "Failed to allocate for configuration\n");
> @@ -1448,14 +1461,21 @@ void parse_config_data(const char *config_source,
> exit(1);
> }
>
> - /* libxl_get_required_shadow_memory() must be called after final values
> + /* libxl_get_required_shadow_memory() and
> + * libxl_get_required_iommu_memory() must be called after final values
> * (default or specified) for vcpus and memory are set, because the
> - * calculation depends on those values. */
> + * calculations depend on those values. */
> b_info->shadow_memkb = !xlu_cfg_get_long(config, "shadow_memory", &l, 0)
> ? l * 1024
> : libxl_get_required_shadow_memory(b_info->max_memkb,
> b_info->max_vcpus);
>
> + /* No IOMMU reservation is needed if either the IOMMU is disabled or it
> + * can share the P2M. */
> + b_info->iommu_memkb = (!iommu_enabled || iommu_hap_pt_share)
> + ? 0
> + : libxl_get_required_iommu_memory(b_info->max_memkb);
> +
> xlu_cfg_get_defbool(config, "nomigrate", &b_info->disable_migrate, 0);
>
> if (!xlu_cfg_get_long(config, "tsc_mode", &l, 1)) {
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 7f1442932a..692565757e 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1056,7 +1056,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
> !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) )
> p2m_free_entry(p2m, orig_pte, level);
>
> - if ( has_iommu_pt(p2m->domain) &&
> + if ( is_iommu_enabled(p2m->domain) &&
> (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
> {
> unsigned int flush_flags = 0;
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index d381784edd..7cfab2dc25 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -365,7 +365,7 @@ unsigned long __init dom0_compute_nr_pages(
> }
>
> need_paging = is_hvm_domain(d) &&
> - (!iommu_hap_pt_share || !paging_mode_hap(d));
> + (!iommu_use_hap_pt(d) || !paging_mode_hap(d));
> for ( ; ; need_paging = false )
> {
> nr_pages = get_memsize(&dom0_size, avail);
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 7ccd85bcea..5ad15eafe0 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr, 1,
>
> void memory_type_changed(struct domain *d)
> {
> - if ( (has_iommu_pt(d) || cache_flush_permitted(d)) && d->vcpu && d->vcpu[0] )
> + if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) &&
> + d->vcpu && d->vcpu[0] )
> {
> p2m_memory_type_changed(d);
> flush_all(FLUSH_CACHE);
> @@ -831,7 +832,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
> return MTRR_TYPE_UNCACHABLE;
> }
>
> - if ( !has_iommu_pt(d) && !cache_flush_permitted(d) )
> + if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
> {
> *ipat = 1;
> return MTRR_TYPE_WRBACK;
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index a5fe89e339..efb8821768 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1664,7 +1664,7 @@ int mem_sharing_domctl(struct domain *d, struct xen_domctl_mem_sharing_op *mec)
> case XEN_DOMCTL_MEM_SHARING_CONTROL:
> {
> rc = 0;
> - if ( unlikely(has_iommu_pt(d) && mec->u.enable) )
> + if ( unlikely(is_iommu_enabled(d) && mec->u.enable) )
> rc = -EXDEV;
> else
> d->arch.hvm.mem_sharing_enabled = mec->u.enable;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 8a5229ee21..e5e4349dea 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1341,7 +1341,7 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
>
> if ( !paging_mode_translate(p2m->domain) )
> {
> - if ( !has_iommu_pt(d) )
> + if ( !is_iommu_enabled(d) )
> return 0;
> return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K,
> IOMMUF_readable | IOMMUF_writable);
> @@ -1432,7 +1432,7 @@ int clear_identity_p2m_entry(struct domain *d, unsigned long gfn_l)
>
> if ( !paging_mode_translate(d) )
> {
> - if ( !has_iommu_pt(d) )
> + if ( !is_iommu_enabled(d) )
> return 0;
> return iommu_legacy_unmap(d, _dfn(gfn_l), PAGE_ORDER_4K);
> }
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index 69aa228e46..d9a52c4db4 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -213,7 +213,7 @@ int paging_log_dirty_enable(struct domain *d, bool_t log_global)
> {
> int ret;
>
> - if ( has_iommu_pt(d) && log_global )
> + if ( is_iommu_enabled(d) && log_global )
> {
> /*
> * Refuse to turn on global log-dirty mode
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 795a467462..fa55f3474e 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1434,7 +1434,7 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
> * shared or being kept in sync then newly added memory needs to be
> * mapped here.
> */
> - if ( has_iommu_pt(hardware_domain) &&
> + if ( is_iommu_enabled(hardware_domain) &&
> !iommu_use_hap_pt(hardware_domain) &&
> !need_iommu_pt_sync(hardware_domain) )
> {
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index d5aff83f2d..7364fd2c33 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -823,7 +823,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
> xatp->gpfn += start;
> xatp->size -= start;
>
> - if ( has_iommu_pt(d) )
> + if ( is_iommu_enabled(d) )
> this_cpu(iommu_dont_flush_iotlb) = 1;
>
> while ( xatp->size > done )
> @@ -844,7 +844,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
> }
> }
>
> - if ( has_iommu_pt(d) )
> + if ( is_iommu_enabled(d) )
> {
> int ret;
>
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 2a1c87e44b..3b18195ebf 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -630,7 +630,7 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec)
>
> /* No paging if iommu is used */
> rc = -EMLINK;
> - if ( unlikely(has_iommu_pt(d)) )
> + if ( unlikely(is_iommu_enabled(d)) )
> break;
>
> rc = -EXDEV;
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 12f2c4c3f2..ea9fd54e3b 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -40,17 +40,6 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
> if ( !list_empty(&dev->domain_list) )
> goto fail;
>
> - /*
> - * The hwdom is forced to use IOMMU for protecting assigned
> - * device. Therefore the IOMMU data is already set up.
> - */
> - ASSERT(!is_hardware_domain(d) ||
> - hd->status == IOMMU_STATUS_initialized);
> -
> - rc = iommu_construct(d);
> - if ( rc )
> - goto fail;
> -
> /* The flag field doesn't matter to DT device. */
> rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev), 0);
>
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 11ece4d1f3..71f17e7edc 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -152,6 +152,17 @@ static int __init parse_dom0_iommu_param(const char *s)
> }
> custom_param("dom0-iommu", parse_dom0_iommu_param);
>
> +static void __hwdom_init check_hwdom_reqs(struct domain *d)
> +{
> + if ( iommu_hwdom_none || !paging_mode_translate(d) )
> + return;
> +
> + arch_iommu_check_autotranslated_hwdom(d);
> +
> + iommu_hwdom_passthrough = false;
> + iommu_hwdom_strict = true;
> +}
> +
> int iommu_domain_init(struct domain *d)
> {
> struct domain_iommu *hd = dom_iommu(d);
> @@ -169,129 +180,44 @@ int iommu_domain_init(struct domain *d)
> return ret;
>
> hd->platform_ops = iommu_get_ops();
> - return hd->platform_ops->init(d);
> -}
> + ret = hd->platform_ops->init(d);
> + if ( ret )
> + return ret;
>
> -static void __hwdom_init check_hwdom_reqs(struct domain *d)
> -{
> - if ( iommu_hwdom_none || !paging_mode_translate(d) )
> - return;
> + if ( is_hardware_domain(d) )
> + check_hwdom_reqs(d); /* may modify iommu_hwdom_strict */
>
> - arch_iommu_check_autotranslated_hwdom(d);
> + /*
> + * NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept
> + * in-sync with their assigned pages because all host RAM will be
> + * mapped during hwdom_init().
> + */
> + if ( !is_hardware_domain(d) || iommu_hwdom_strict )
> + hd->need_sync = !iommu_use_hap_pt(d);
>
> - iommu_hwdom_passthrough = false;
> - iommu_hwdom_strict = true;
> + return 0;
> }
>
> void __hwdom_init iommu_hwdom_init(struct domain *d)
> {
> struct domain_iommu *hd = dom_iommu(d);
>
> - check_hwdom_reqs(d);
> -
> if ( !is_iommu_enabled(d) )
> return;
>
> register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0);
>
> - hd->status = IOMMU_STATUS_initializing;
> - /*
> - * NB: relaxed hw domains don't need sync because all ram is already
> - * mapped in the iommu page tables.
> - */
> - hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
> - if ( need_iommu_pt_sync(d) )
> - {
> - struct page_info *page;
> - unsigned int i = 0, flush_flags = 0;
> - int rc = 0;
> -
> - page_list_for_each ( page, &d->page_list )
> - {
> - unsigned long mfn = mfn_x(page_to_mfn(page));
> - unsigned long dfn = mfn_to_gmfn(d, mfn);
> - unsigned int mapping = IOMMUF_readable;
> - int ret;
> -
> - if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
> - ((page->u.inuse.type_info & PGT_type_mask)
> - == PGT_writable_page) )
> - mapping |= IOMMUF_writable;
> -
> - ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping,
> - &flush_flags);
> -
> - if ( !rc )
> - rc = ret;
> -
> - if ( !(i++ & 0xfffff) )
> - process_pending_softirqs();
> - }
> -
> - /* Use while-break to avoid compiler warning */
> - while ( iommu_iotlb_flush_all(d, flush_flags) )
> - break;
> -
> - if ( rc )
> - printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> - d->domain_id, rc);
> - }
> -
> hd->platform_ops->hwdom_init(d);
> -
> - hd->status = IOMMU_STATUS_initialized;
> }
>
> -void iommu_teardown(struct domain *d)
> +static void iommu_teardown(struct domain *d)
> {
> struct domain_iommu *hd = dom_iommu(d);
>
> - hd->status = IOMMU_STATUS_disabled;
> hd->platform_ops->teardown(d);
> tasklet_schedule(&iommu_pt_cleanup_tasklet);
> }
>
> -int iommu_construct(struct domain *d)
> -{
> - struct domain_iommu *hd = dom_iommu(d);
> -
> - if ( hd->status == IOMMU_STATUS_initialized )
> - return 0;
> -
> - hd->status = IOMMU_STATUS_initializing;
> -
> - if ( !iommu_use_hap_pt(d) )
> - {
> - int rc;
> -
> - hd->need_sync = true;
> -
> - rc = arch_iommu_populate_page_table(d);
> - if ( rc )
> - {
> - if ( rc != -ERESTART )
> - {
> - hd->need_sync = false;
> - hd->status = IOMMU_STATUS_disabled;
> - }
> -
> - return rc;
> - }
> - }
> -
> - hd->status = IOMMU_STATUS_initialized;
> -
> - /*
> - * There may be dirty cache lines when a device is assigned
> - * and before has_iommu_pt(d) becoming true, this will cause
> - * memory_type_changed lose effect if memory type changes.
> - * Call memory_type_changed here to amend this.
> - */
> - memory_type_changed(d);
> -
> - return 0;
> -}
> -
> void iommu_domain_destroy(struct domain *d)
> {
> if ( !is_iommu_enabled(d) )
> @@ -587,11 +513,8 @@ int iommu_do_domctl(
> void iommu_share_p2m_table(struct domain* d)
> {
> ASSERT(hap_enabled(d));
> - /*
> - * iommu_use_hap_pt(d) cannot be used here because during domain
> - * construction has_iommu_pt(d) will always return false here.
> - */
> - if ( is_iommu_enabled(d) && iommu_hap_pt_share )
> +
> + if ( iommu_use_hap_pt(d) )
> iommu_get_ops()->share_p2m(d);
> }
>
> @@ -638,8 +561,7 @@ static void iommu_dump_p2m_table(unsigned char key)
> ops = iommu_get_ops();
> for_each_domain(d)
> {
> - if ( is_hardware_domain(d) ||
> - dom_iommu(d)->status < IOMMU_STATUS_initialized )
> + if ( is_hardware_domain(d) || !is_iommu_enabled(d) )
> continue;
>
> if ( iommu_use_hap_pt(d) )
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index d28f17af75..1fad0af534 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -939,9 +939,6 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>
> pdev->fault.count = 0;
>
> - if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
> - iommu_teardown(d);
> -
> return ret;
> }
>
> @@ -1490,13 +1487,6 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
> if ( !pcidevs_trylock() )
> return -ERESTART;
>
> - rc = iommu_construct(d);
> - if ( rc )
> - {
> - pcidevs_unlock();
> - return rc;
> - }
> -
> pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
> if ( !pdev )
> {
> @@ -1525,8 +1515,6 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
> }
>
> done:
> - if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
> - iommu_teardown(d);
> pcidevs_unlock();
>
> return rc;
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index f123760ee2..3c17f11386 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1721,15 +1721,7 @@ static void iommu_domain_teardown(struct domain *d)
>
> ASSERT(is_iommu_enabled(d));
>
> - /*
> - * We can't use iommu_use_hap_pt here because either IOMMU state
> - * is already changed to IOMMU_STATUS_disabled at this point or
> - * has always been IOMMU_STATUS_disabled.
> - *
> - * We also need to test if HAP is enabled because PV guests can
> - * enter this path too.
> - */
> - if ( hap_enabled(d) && iommu_hap_pt_share )
> + if ( iommu_use_hap_pt(d) )
> return;
>
> spin_lock(&hd->arch.mapping_lock);
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index 8319fe0a69..47a3e55213 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -81,103 +81,6 @@ int __init iommu_setup_hpet_msi(struct msi_desc *msi)
> return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : -ENODEV;
> }
>
> -int arch_iommu_populate_page_table(struct domain *d)
> -{
> - struct page_info *page;
> - int rc = 0, n = 0;
> -
> - spin_lock(&d->page_alloc_lock);
> -
> - if ( unlikely(d->is_dying) )
> - rc = -ESRCH;
> -
> - while ( !rc && (page = page_list_remove_head(&d->page_list)) )
> - {
> - if ( is_hvm_domain(d) ||
> - (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
> - {
> - mfn_t mfn = page_to_mfn(page);
> - gfn_t gfn = mfn_to_gfn(d, mfn);
> - unsigned int flush_flags = 0;
> -
> - if ( !gfn_eq(gfn, INVALID_GFN) )
> - {
> - dfn_t dfn = _dfn(gfn_x(gfn));
> -
> - ASSERT(!(gfn_x(gfn) >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> - BUG_ON(SHARED_M2P(gfn_x(gfn)));
> - rc = iommu_map(d, dfn, mfn, PAGE_ORDER_4K,
> - IOMMUF_readable | IOMMUF_writable,
> - &flush_flags);
> -
> - /*
> - * We may be working behind the back of a running guest, which
> - * may change the type of a page at any time. We can't prevent
> - * this (for instance, by bumping the type count while mapping
> - * the page) without causing legitimate guest type-change
> - * operations to fail. So after adding the page to the IOMMU,
> - * check again to make sure this is still valid. NB that the
> - * writable entry in the iommu is harmless until later, when
> - * the actual device gets assigned.
> - */
> - if ( !rc && !is_hvm_domain(d) &&
> - ((page->u.inuse.type_info & PGT_type_mask) !=
> - PGT_writable_page) )
> - {
> - rc = iommu_unmap(d, dfn, PAGE_ORDER_4K, &flush_flags);
> - /* If the type changed yet again, simply force a retry. */
> - if ( !rc && ((page->u.inuse.type_info & PGT_type_mask) ==
> - PGT_writable_page) )
> - rc = -ERESTART;
> - }
> - }
> - if ( rc )
> - {
> - page_list_add(page, &d->page_list);
> - break;
> - }
> - }
> - page_list_add_tail(page, &d->arch.relmem_list);
> - if ( !(++n & 0xff) && !page_list_empty(&d->page_list) &&
> - hypercall_preempt_check() )
> - rc = -ERESTART;
> - }
> -
> - if ( !rc )
> - {
> - /*
> - * The expectation here is that generally there are many normal pages
> - * on relmem_list (the ones we put there) and only few being in an
> - * offline/broken state. The latter ones are always at the head of the
> - * list. Hence we first move the whole list, and then move back the
> - * first few entries.
> - */
> - page_list_move(&d->page_list, &d->arch.relmem_list);
> - while ( !page_list_empty(&d->page_list) &&
> - (page = page_list_first(&d->page_list),
> - (page->count_info & (PGC_state|PGC_broken))) )
> - {
> - page_list_del(page, &d->page_list);
> - page_list_add_tail(page, &d->arch.relmem_list);
> - }
> - }
> -
> - spin_unlock(&d->page_alloc_lock);
> -
> - if ( !rc )
> - /*
> - * flush_flags are not tracked across hypercall pre-emption so
> - * assume a full flush is necessary.
> - */
> - rc = iommu_iotlb_flush_all(
> - d, IOMMU_FLUSHF_added | IOMMU_FLUSHF_modified);
> -
> - if ( rc && rc != -ERESTART )
> - iommu_teardown(d);
> -
> - return rc;
> -}
> -
> void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
> {
> if ( !is_iommu_enabled(d) )
> diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
> index 904c9aec11..1577e83d2b 100644
> --- a/xen/include/asm-arm/iommu.h
> +++ b/xen/include/asm-arm/iommu.h
> @@ -21,7 +21,7 @@ struct arch_iommu
> };
>
> /* Always share P2M Table between the CPU and the IOMMU */
> -#define iommu_use_hap_pt(d) (has_iommu_pt(d))
> +#define iommu_use_hap_pt(d) is_iommu_enabled(d)
>
> const struct iommu_ops *iommu_get_ops(void);
> void iommu_set_ops(const struct iommu_ops *ops);
> diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
> index 31fda4b0cf..5071afd6a5 100644
> --- a/xen/include/asm-x86/iommu.h
> +++ b/xen/include/asm-x86/iommu.h
> @@ -88,7 +88,7 @@ extern const struct iommu_init_ops *iommu_init_ops;
>
> /* Are we using the domain P2M table as its IOMMU pagetable? */
> #define iommu_use_hap_pt(d) \
> - (hap_enabled(d) && has_iommu_pt(d) && iommu_hap_pt_share)
> + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
>
> void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
> unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index c5ed7efe98..dfec0ca3fc 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -88,15 +88,9 @@ void iommu_domain_destroy(struct domain *d);
>
> void arch_iommu_domain_destroy(struct domain *d);
> int arch_iommu_domain_init(struct domain *d);
> -int arch_iommu_populate_page_table(struct domain *d);
> void arch_iommu_check_autotranslated_hwdom(struct domain *d);
> void arch_iommu_hwdom_init(struct domain *d);
>
> -int iommu_construct(struct domain *d);
> -
> -/* Function used internally, use iommu_domain_destroy */
> -void iommu_teardown(struct domain *d);
> -
> /*
> * The following flags are passed to map operations and passed by lookup
> * operations.
> @@ -263,13 +257,6 @@ struct iommu_ops {
> # define iommu_vcall iommu_call
> #endif
>
> -enum iommu_status
> -{
> - IOMMU_STATUS_disabled,
> - IOMMU_STATUS_initializing,
> - IOMMU_STATUS_initialized
> -};
> -
> struct domain_iommu {
> struct arch_iommu arch;
>
> @@ -289,9 +276,6 @@ struct domain_iommu {
> /* Features supported by the IOMMU */
> DECLARE_BITMAP(features, IOMMU_FEAT_count);
>
> - /* Status of guest IOMMU mappings */
> - enum iommu_status status;
> -
> /*
> * Does the guest reqire mappings to be synchonized, to maintain
> * the default dfn == pfn map. (See comment on dfn at the top of
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 2d17c84915..ae1faf70d3 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -966,10 +966,8 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
> }
>
> #ifdef CONFIG_HAS_PASSTHROUGH
> -#define has_iommu_pt(d) (dom_iommu(d)->status != IOMMU_STATUS_disabled)
> #define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
> #else
> -#define has_iommu_pt(d) false
> #define need_iommu_pt_sync(d) false
> #endif
>
> --
> 2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables Paul Durrant
2019-09-25 9:00 ` Paul Durrant
@ 2019-09-25 9:16 ` Wei Liu
1 sibling, 0 replies; 26+ messages in thread
From: Wei Liu @ 2019-09-25 9:16 UTC (permalink / raw)
To: Paul Durrant
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Tamas K Lengyel, Jan Beulich,
Alexandru Isaila, xen-devel, Volodymyr Babchuk,
Roger Pau Monné
On Wed, Sep 18, 2019 at 11:41:10AM +0100, Paul Durrant wrote:
> Now that there is a per-domain IOMMU-enable flag, which should be set if
> any device is going to be passed through, stop deferring page table
> construction until the assignment is done. Also don't tear down the tables
> again when the last device is de-assigned; defer that task until domain
> destruction.
>
> This allows the has_iommu_pt() helper and iommu_status enumeration to be
> removed. Calls to has_iommu_pt() are simply replaced by calls to
> is_iommu_enabled(). Remaining open-coded tests of iommu_hap_pt_share can
> also be replaced by calls to iommu_use_hap_pt().
> The arch_iommu_populate_page_table() and iommu_construct() functions become
> redundant, as does the 'strict mode' dom0 page_list mapping code in
> iommu_hwdom_init(), and iommu_teardown() can be made static is its only
> remaining caller, iommu_domain_destroy(), is within the same source
> module.
>
> All in all, about 220 lines of code are removed from the hypervisor (at
> the expense of some additions in the toolstack).
>
> NOTE: This patch will cause a small amount of extra resource to be used
> to accommodate IOMMU page tables that may never be used, since the
> per-domain IOMMU-enable flag is currently set to the value of the
> global iommu_enable flag. A subsequent patch will add an option to
> the toolstack to allow it to be turned off if there is no intention
> to assign passthrough hardware to the domain.
> To account for the extra resource, 'iommu_memkb' has been added to
> domain_build_info. This patch sets it to a value calculated based
> on the domain's maximum memory when the P2M sharing is either not
> supported or globally disabled, or zero otherwise. However, when
> the toolstack option mentioned above is added, it will also be zero
> if the per-domain IOMMU-enable flag is turned off.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Alexandru Isaila <aisaila@bitdefender.com>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Julien Grall <julien.grall@arm.com>
Acked-by: Wei Liu <wl@xen.org>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 8:56 ` Paul Durrant
@ 2019-09-25 15:49 ` Oleksandr
2019-09-25 15:55 ` Paul Durrant
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Oleksandr @ 2019-09-25 15:49 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, David Scott,
Anthony Perard, Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
[CC Julien]
Hi Paul
I may mistake, but looks like
80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
iommu_use_hap_pt() and need_iommu_pt_sync() macros
triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
So, iommu_setup() calls clear_iommu_hap_pt_share() with
iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
actually, triggers ASSERT.
...
(XEN) Assertion 'unreachable' failed at
...ild-workspace/build/xen/xen/include/xen/iommu.h:72
(XEN) ----[ Xen-4.13-unstable arm64 debug=y Not tainted ]----
(XEN) CPU: 0
(XEN) PC: 00000000002b3ae0 iommu_setup+0xa0/0x18c
(XEN) LR: 00000000002b3a8c
(XEN) SP: 00000000002f7dc0
(XEN) CPSR: a0000249 MODE:64-bit EL2h (Hypervisor, handler)
(XEN) X0: 00000000002a7000 X1: 0000000000000000 X2: 2c736173656e6572
(XEN) X3: 0000000000000002 X4: 0000000000000001 X5: 0000000000000000
(XEN) X6: 0000000000000080 X7: 2b726072646d6471 X8: 7f7f7f7f7f7f7f7f
(XEN) X9: ff65685e6c6f7275 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
(XEN) X12: 0000000000000038 X13: 0000000000280910 X14: 0000000000000020
(XEN) X15: 0000000000000000 X16: 00000000002a7000 X17: 00000000002a7000
(XEN) X18: 00000000002a7000 X19: 0000000000000000 X20: 00000000ffffffed
(XEN) X21: 00000000002a6380 X22: 0000000000335430 X23: 0000000000000002
(XEN) X24: 000000000029b1f0 X25: 00000000002d83d0 X26: 0000000048000000
(XEN) X27: 00000000c0000000 X28: 0000000000000001 FP: 00000000002f7dc0
(XEN)
(XEN) VTCR_EL2: 80000000
(XEN) VTTBR_EL2: 0000000000000000
(XEN)
(XEN) SCTLR_EL2: 30cd183d
(XEN) HCR_EL2: 0000000000000038
(XEN) TTBR0_EL2: 00000000781b4000
(XEN)
(XEN) ESR_EL2: f2000001
(XEN) HPFAR_EL2: 0000000000000000
(XEN) FAR_EL2: 0000000000000000
(XEN)
(XEN) Xen stack trace from sp=00000000002f7dc0:
(XEN) 00000000002f7de0 00000000002bdd94 0000000000000002 0000000000000002
(XEN) 00000000bfe0b660 00000000002001b4 0000000078080000 0000000077e80000
(XEN) 0000000048000000 0000000000000000 0000000000400000 0000000000000003
(XEN) 0000000000000001 0000000000000000 0000000078080000 0000000048080040
(XEN) 0000000000000000 000000000000f080 0000000048000000 0000000078000000
(XEN) 00000000002d83c0 00000000002aa440 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000300000000 0000000000000000 00000040ffffffff
(XEN) 0000000000000400 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN) [<00000000002b3ae0>] iommu_setup+0xa0/0x18c (PC)
(XEN) [<00000000002b3a8c>] iommu_setup+0x4c/0x18c (LR)
(XEN) [<00000000002bdd94>] start_xen+0xaa0/0xc7c
(XEN) [<00000000002001b4>] arm64/head.o#primary_switched+0xc/0x2c
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion 'unreachable' failed at
...ild-workspace/build/xen/xen/include/xen/iommu.h:72
(XEN) ****************************************
(XEN)
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 15:49 ` Oleksandr
@ 2019-09-25 15:55 ` Paul Durrant
2019-09-25 16:03 ` Paul Durrant
2019-09-25 16:10 ` Paul Durrant
2 siblings, 0 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-25 15:55 UTC (permalink / raw)
To: 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, David Scott,
Anthony Perard, Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Oleksandr <olekstysh@gmail.com>
> Sent: 25 September 2019 16:50
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George Dunlap
> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>
>
> [CC Julien]
>
>
> Hi Paul
>
> I may mistake, but looks like
>
> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>
> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
Oh, I'm sure I was told this was not a possibility for ARM, which is why iommu_hap_pt_share ended up being #defined. Seems I was misled, in which case ARM ought to be have a more dynamic config. as with x86.
Paul
>
> So, iommu_setup() calls clear_iommu_hap_pt_share() with
> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
> actually, triggers ASSERT.
>
> ...
>
>
> (XEN) Assertion 'unreachable' failed at
> ...ild-workspace/build/xen/xen/include/xen/iommu.h:72
> (XEN) ----[ Xen-4.13-unstable arm64 debug=y Not tainted ]----
> (XEN) CPU: 0
> (XEN) PC: 00000000002b3ae0 iommu_setup+0xa0/0x18c
> (XEN) LR: 00000000002b3a8c
> (XEN) SP: 00000000002f7dc0
> (XEN) CPSR: a0000249 MODE:64-bit EL2h (Hypervisor, handler)
> (XEN) X0: 00000000002a7000 X1: 0000000000000000 X2: 2c736173656e6572
> (XEN) X3: 0000000000000002 X4: 0000000000000001 X5: 0000000000000000
> (XEN) X6: 0000000000000080 X7: 2b726072646d6471 X8: 7f7f7f7f7f7f7f7f
> (XEN) X9: ff65685e6c6f7275 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
> (XEN) X12: 0000000000000038 X13: 0000000000280910 X14: 0000000000000020
> (XEN) X15: 0000000000000000 X16: 00000000002a7000 X17: 00000000002a7000
> (XEN) X18: 00000000002a7000 X19: 0000000000000000 X20: 00000000ffffffed
> (XEN) X21: 00000000002a6380 X22: 0000000000335430 X23: 0000000000000002
> (XEN) X24: 000000000029b1f0 X25: 00000000002d83d0 X26: 0000000048000000
> (XEN) X27: 00000000c0000000 X28: 0000000000000001 FP: 00000000002f7dc0
> (XEN)
> (XEN) VTCR_EL2: 80000000
> (XEN) VTTBR_EL2: 0000000000000000
> (XEN)
> (XEN) SCTLR_EL2: 30cd183d
> (XEN) HCR_EL2: 0000000000000038
> (XEN) TTBR0_EL2: 00000000781b4000
> (XEN)
> (XEN) ESR_EL2: f2000001
> (XEN) HPFAR_EL2: 0000000000000000
> (XEN) FAR_EL2: 0000000000000000
> (XEN)
> (XEN) Xen stack trace from sp=00000000002f7dc0:
> (XEN) 00000000002f7de0 00000000002bdd94 0000000000000002 0000000000000002
> (XEN) 00000000bfe0b660 00000000002001b4 0000000078080000 0000000077e80000
> (XEN) 0000000048000000 0000000000000000 0000000000400000 0000000000000003
> (XEN) 0000000000000001 0000000000000000 0000000078080000 0000000048080040
> (XEN) 0000000000000000 000000000000f080 0000000048000000 0000000078000000
> (XEN) 00000000002d83c0 00000000002aa440 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000300000000 0000000000000000 00000040ffffffff
> (XEN) 0000000000000400 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) Xen call trace:
> (XEN) [<00000000002b3ae0>] iommu_setup+0xa0/0x18c (PC)
> (XEN) [<00000000002b3a8c>] iommu_setup+0x4c/0x18c (LR)
> (XEN) [<00000000002bdd94>] start_xen+0xaa0/0xc7c
> (XEN) [<00000000002001b4>] arm64/head.o#primary_switched+0xc/0x2c
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Assertion 'unreachable' failed at
> ...ild-workspace/build/xen/xen/include/xen/iommu.h:72
> (XEN) ****************************************
> (XEN)
>
>
> --
> Regards,
>
> Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 15:49 ` Oleksandr
2019-09-25 15:55 ` Paul Durrant
@ 2019-09-25 16:03 ` Paul Durrant
2019-09-25 16:04 ` Paul Durrant
2019-09-25 16:14 ` Oleksandr
2019-09-25 16:10 ` Paul Durrant
2 siblings, 2 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-25 16:03 UTC (permalink / raw)
To: 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, David Scott,
Anthony Perard, Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Oleksandr <olekstysh@gmail.com>
> Sent: 25 September 2019 16:50
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George Dunlap
> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>
>
> [CC Julien]
>
>
> Hi Paul
>
> I may mistake, but looks like
>
> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>
> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
>
> So, iommu_setup() calls clear_iommu_hap_pt_share() with
> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
> actually, triggers ASSERT.
BTW, I assume disabling the iommu on the xen command like will work round the issue?
Paul
>
> ...
>
>
> (XEN) Assertion 'unreachable' failed at
> ...ild-workspace/build/xen/xen/include/xen/iommu.h:72
> (XEN) ----[ Xen-4.13-unstable arm64 debug=y Not tainted ]----
> (XEN) CPU: 0
> (XEN) PC: 00000000002b3ae0 iommu_setup+0xa0/0x18c
> (XEN) LR: 00000000002b3a8c
> (XEN) SP: 00000000002f7dc0
> (XEN) CPSR: a0000249 MODE:64-bit EL2h (Hypervisor, handler)
> (XEN) X0: 00000000002a7000 X1: 0000000000000000 X2: 2c736173656e6572
> (XEN) X3: 0000000000000002 X4: 0000000000000001 X5: 0000000000000000
> (XEN) X6: 0000000000000080 X7: 2b726072646d6471 X8: 7f7f7f7f7f7f7f7f
> (XEN) X9: ff65685e6c6f7275 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
> (XEN) X12: 0000000000000038 X13: 0000000000280910 X14: 0000000000000020
> (XEN) X15: 0000000000000000 X16: 00000000002a7000 X17: 00000000002a7000
> (XEN) X18: 00000000002a7000 X19: 0000000000000000 X20: 00000000ffffffed
> (XEN) X21: 00000000002a6380 X22: 0000000000335430 X23: 0000000000000002
> (XEN) X24: 000000000029b1f0 X25: 00000000002d83d0 X26: 0000000048000000
> (XEN) X27: 00000000c0000000 X28: 0000000000000001 FP: 00000000002f7dc0
> (XEN)
> (XEN) VTCR_EL2: 80000000
> (XEN) VTTBR_EL2: 0000000000000000
> (XEN)
> (XEN) SCTLR_EL2: 30cd183d
> (XEN) HCR_EL2: 0000000000000038
> (XEN) TTBR0_EL2: 00000000781b4000
> (XEN)
> (XEN) ESR_EL2: f2000001
> (XEN) HPFAR_EL2: 0000000000000000
> (XEN) FAR_EL2: 0000000000000000
> (XEN)
> (XEN) Xen stack trace from sp=00000000002f7dc0:
> (XEN) 00000000002f7de0 00000000002bdd94 0000000000000002 0000000000000002
> (XEN) 00000000bfe0b660 00000000002001b4 0000000078080000 0000000077e80000
> (XEN) 0000000048000000 0000000000000000 0000000000400000 0000000000000003
> (XEN) 0000000000000001 0000000000000000 0000000078080000 0000000048080040
> (XEN) 0000000000000000 000000000000f080 0000000048000000 0000000078000000
> (XEN) 00000000002d83c0 00000000002aa440 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000300000000 0000000000000000 00000040ffffffff
> (XEN) 0000000000000400 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) Xen call trace:
> (XEN) [<00000000002b3ae0>] iommu_setup+0xa0/0x18c (PC)
> (XEN) [<00000000002b3a8c>] iommu_setup+0x4c/0x18c (LR)
> (XEN) [<00000000002bdd94>] start_xen+0xaa0/0xc7c
> (XEN) [<00000000002001b4>] arm64/head.o#primary_switched+0xc/0x2c
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Assertion 'unreachable' failed at
> ...ild-workspace/build/xen/xen/include/xen/iommu.h:72
> (XEN) ****************************************
> (XEN)
>
>
> --
> Regards,
>
> Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 16:03 ` Paul Durrant
@ 2019-09-25 16:04 ` Paul Durrant
2019-09-25 16:14 ` Oleksandr
1 sibling, 0 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-25 16:04 UTC (permalink / raw)
To: 'Oleksandr', 'Jan Beulich'
Cc: 'Petre Pircalabu', 'Stefano Stabellini',
'xen-devel@lists.xenproject.org', 'Wei Liu',
'KonradRzeszutek Wilk', Andrew Cooper, Tim (Xen.org),
George Dunlap, 'Julien Grall', 'Tamas K Lengyel',
'David Scott',
Anthony Perard, Ian Jackson, 'Volodymyr Babchuk',
Roger Pau Monne
> -----Original Message-----
> From: Paul Durrant
> Sent: 25 September 2019 17:04
> To: 'Oleksandr' <olekstysh@gmail.com>; 'Jan Beulich' <jbeulich@suse.com>
> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George Dunlap
> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: RE: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>
> > -----Original Message-----
> > From: Oleksandr <olekstysh@gmail.com>
> > Sent: 25 September 2019 16:50
> > To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> > Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei
> Liu
> > <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George
> Dunlap
> > <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> > <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-
> devel@lists.xenproject.org;
> > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien Grall
> > <julien.grall@arm.com>
> > Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
> >
> >
> > [CC Julien]
> >
> >
> > Hi Paul
> >
> > I may mistake, but looks like
> >
> > 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
> > iommu_use_hap_pt() and need_iommu_pt_sync() macros
> >
> > triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
> > with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
> >
> > So, iommu_setup() calls clear_iommu_hap_pt_share() with
> > iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
> > actually, triggers ASSERT.
>
> BTW, I assume disabling the iommu on the xen command like will work round the issue?
Actually, no, that would be no good either.
Paul
>
> Paul
>
> >
> > ...
> >
> >
> > (XEN) Assertion 'unreachable' failed at
> > ...ild-workspace/build/xen/xen/include/xen/iommu.h:72
> > (XEN) ----[ Xen-4.13-unstable arm64 debug=y Not tainted ]----
> > (XEN) CPU: 0
> > (XEN) PC: 00000000002b3ae0 iommu_setup+0xa0/0x18c
> > (XEN) LR: 00000000002b3a8c
> > (XEN) SP: 00000000002f7dc0
> > (XEN) CPSR: a0000249 MODE:64-bit EL2h (Hypervisor, handler)
> > (XEN) X0: 00000000002a7000 X1: 0000000000000000 X2: 2c736173656e6572
> > (XEN) X3: 0000000000000002 X4: 0000000000000001 X5: 0000000000000000
> > (XEN) X6: 0000000000000080 X7: 2b726072646d6471 X8: 7f7f7f7f7f7f7f7f
> > (XEN) X9: ff65685e6c6f7275 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
> > (XEN) X12: 0000000000000038 X13: 0000000000280910 X14: 0000000000000020
> > (XEN) X15: 0000000000000000 X16: 00000000002a7000 X17: 00000000002a7000
> > (XEN) X18: 00000000002a7000 X19: 0000000000000000 X20: 00000000ffffffed
> > (XEN) X21: 00000000002a6380 X22: 0000000000335430 X23: 0000000000000002
> > (XEN) X24: 000000000029b1f0 X25: 00000000002d83d0 X26: 0000000048000000
> > (XEN) X27: 00000000c0000000 X28: 0000000000000001 FP: 00000000002f7dc0
> > (XEN)
> > (XEN) VTCR_EL2: 80000000
> > (XEN) VTTBR_EL2: 0000000000000000
> > (XEN)
> > (XEN) SCTLR_EL2: 30cd183d
> > (XEN) HCR_EL2: 0000000000000038
> > (XEN) TTBR0_EL2: 00000000781b4000
> > (XEN)
> > (XEN) ESR_EL2: f2000001
> > (XEN) HPFAR_EL2: 0000000000000000
> > (XEN) FAR_EL2: 0000000000000000
> > (XEN)
> > (XEN) Xen stack trace from sp=00000000002f7dc0:
> > (XEN) 00000000002f7de0 00000000002bdd94 0000000000000002 0000000000000002
> > (XEN) 00000000bfe0b660 00000000002001b4 0000000078080000 0000000077e80000
> > (XEN) 0000000048000000 0000000000000000 0000000000400000 0000000000000003
> > (XEN) 0000000000000001 0000000000000000 0000000078080000 0000000048080040
> > (XEN) 0000000000000000 000000000000f080 0000000048000000 0000000078000000
> > (XEN) 00000000002d83c0 00000000002aa440 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000300000000 0000000000000000 00000040ffffffff
> > (XEN) 0000000000000400 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN) Xen call trace:
> > (XEN) [<00000000002b3ae0>] iommu_setup+0xa0/0x18c (PC)
> > (XEN) [<00000000002b3a8c>] iommu_setup+0x4c/0x18c (LR)
> > (XEN) [<00000000002bdd94>] start_xen+0xaa0/0xc7c
> > (XEN) [<00000000002001b4>] arm64/head.o#primary_switched+0xc/0x2c
> > (XEN)
> > (XEN)
> > (XEN) ****************************************
> > (XEN) Panic on CPU 0:
> > (XEN) Assertion 'unreachable' failed at
> > ...ild-workspace/build/xen/xen/include/xen/iommu.h:72
> > (XEN) ****************************************
> > (XEN)
> >
> >
> > --
> > Regards,
> >
> > Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 15:49 ` Oleksandr
2019-09-25 15:55 ` Paul Durrant
2019-09-25 16:03 ` Paul Durrant
@ 2019-09-25 16:10 ` Paul Durrant
2019-09-25 16:24 ` Oleksandr
2019-09-25 21:34 ` Julien Grall
2 siblings, 2 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-25 16:10 UTC (permalink / raw)
To: 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, David Scott,
Anthony Perard, Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Oleksandr <olekstysh@gmail.com>
> Sent: 25 September 2019 16:50
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George Dunlap
> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>
>
> [CC Julien]
>
>
> Hi Paul
>
> I may mistake, but looks like
>
> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>
> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
>
> So, iommu_setup() calls clear_iommu_hap_pt_share() with
> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
> actually, triggers ASSERT.
>
Here a minimal patch, leaving 'force pt share' in place. Does this avoid the problem?
---8<---
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index e8763c7fdf..f88a285e7f 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
pi->max_mfn = get_upper_mfn_bound();
arch_do_physinfo(pi);
if ( iommu_enabled )
+ {
pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
- if ( iommu_hap_pt_share )
- pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
+ if ( iommu_hap_pt_share )
+ pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
+ }
if ( copy_to_guest(u_sysctl, op, 1) )
ret = -EFAULT;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 7c3003f3f1..6a10a24128 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
{
#ifndef iommu_hap_pt_share
iommu_hap_pt_share = false;
-#elif iommu_hap_pt_share
- ASSERT_UNREACHABLE();
#endif
}
---8<---
Paul
> ...
>
>
> (XEN) Assertion 'unreachable' failed at
> ...ild-workspace/build/xen/xen/include/xen/iommu.h:72
> (XEN) ----[ Xen-4.13-unstable arm64 debug=y Not tainted ]----
> (XEN) CPU: 0
> (XEN) PC: 00000000002b3ae0 iommu_setup+0xa0/0x18c
> (XEN) LR: 00000000002b3a8c
> (XEN) SP: 00000000002f7dc0
> (XEN) CPSR: a0000249 MODE:64-bit EL2h (Hypervisor, handler)
> (XEN) X0: 00000000002a7000 X1: 0000000000000000 X2: 2c736173656e6572
> (XEN) X3: 0000000000000002 X4: 0000000000000001 X5: 0000000000000000
> (XEN) X6: 0000000000000080 X7: 2b726072646d6471 X8: 7f7f7f7f7f7f7f7f
> (XEN) X9: ff65685e6c6f7275 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
> (XEN) X12: 0000000000000038 X13: 0000000000280910 X14: 0000000000000020
> (XEN) X15: 0000000000000000 X16: 00000000002a7000 X17: 00000000002a7000
> (XEN) X18: 00000000002a7000 X19: 0000000000000000 X20: 00000000ffffffed
> (XEN) X21: 00000000002a6380 X22: 0000000000335430 X23: 0000000000000002
> (XEN) X24: 000000000029b1f0 X25: 00000000002d83d0 X26: 0000000048000000
> (XEN) X27: 00000000c0000000 X28: 0000000000000001 FP: 00000000002f7dc0
> (XEN)
> (XEN) VTCR_EL2: 80000000
> (XEN) VTTBR_EL2: 0000000000000000
> (XEN)
> (XEN) SCTLR_EL2: 30cd183d
> (XEN) HCR_EL2: 0000000000000038
> (XEN) TTBR0_EL2: 00000000781b4000
> (XEN)
> (XEN) ESR_EL2: f2000001
> (XEN) HPFAR_EL2: 0000000000000000
> (XEN) FAR_EL2: 0000000000000000
> (XEN)
> (XEN) Xen stack trace from sp=00000000002f7dc0:
> (XEN) 00000000002f7de0 00000000002bdd94 0000000000000002 0000000000000002
> (XEN) 00000000bfe0b660 00000000002001b4 0000000078080000 0000000077e80000
> (XEN) 0000000048000000 0000000000000000 0000000000400000 0000000000000003
> (XEN) 0000000000000001 0000000000000000 0000000078080000 0000000048080040
> (XEN) 0000000000000000 000000000000f080 0000000048000000 0000000078000000
> (XEN) 00000000002d83c0 00000000002aa440 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000300000000 0000000000000000 00000040ffffffff
> (XEN) 0000000000000400 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN) Xen call trace:
> (XEN) [<00000000002b3ae0>] iommu_setup+0xa0/0x18c (PC)
> (XEN) [<00000000002b3a8c>] iommu_setup+0x4c/0x18c (LR)
> (XEN) [<00000000002bdd94>] start_xen+0xaa0/0xc7c
> (XEN) [<00000000002001b4>] arm64/head.o#primary_switched+0xc/0x2c
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Assertion 'unreachable' failed at
> ...ild-workspace/build/xen/xen/include/xen/iommu.h:72
> (XEN) ****************************************
> (XEN)
>
>
> --
> Regards,
>
> Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 16:03 ` Paul Durrant
2019-09-25 16:04 ` Paul Durrant
@ 2019-09-25 16:14 ` Oleksandr
1 sibling, 0 replies; 26+ messages in thread
From: Oleksandr @ 2019-09-25 16:14 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, David Scott,
Anthony Perard, Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
On 25.09.19 19:03, Paul Durrant wrote:
Hi, Paul
>> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>>
>>
>> [CC Julien]
>>
>>
>> Hi Paul
>>
>> I may mistake, but looks like
>>
>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>>
>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
>>
>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
>> actually, triggers ASSERT.
> BTW, I assume disabling the iommu on the xen command like will work round the issue?
No. Disabling the iommu will lead to ASSERT in all cases.
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 16:10 ` Paul Durrant
@ 2019-09-25 16:24 ` Oleksandr
2019-09-25 18:07 ` Oleksandr
2019-09-25 21:34 ` Julien Grall
1 sibling, 1 reply; 26+ messages in thread
From: Oleksandr @ 2019-09-25 16:24 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, David Scott,
Anthony Perard, Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
On 25.09.19 19:10, Paul Durrant wrote:
Hi Paul
>> -----Original Message-----
>> From: Oleksandr <olekstysh@gmail.com>
>> Sent: 25 September 2019 16:50
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
>> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George Dunlap
>> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
>> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien Grall
>> <julien.grall@arm.com>
>> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>>
>>
>> [CC Julien]
>>
>>
>> Hi Paul
>>
>> I may mistake, but looks like
>>
>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>>
>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
>>
>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
>> actually, triggers ASSERT.
>>
> Here a minimal patch, leaving 'force pt share' in place. Does this avoid the problem?
>
> ---8<---
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index e8763c7fdf..f88a285e7f 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> pi->max_mfn = get_upper_mfn_bound();
> arch_do_physinfo(pi);
> if ( iommu_enabled )
> + {
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
> - if ( iommu_hap_pt_share )
> - pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> + if ( iommu_hap_pt_share )
> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> + }
>
> if ( copy_to_guest(u_sysctl, op, 1) )
> ret = -EFAULT;
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 7c3003f3f1..6a10a24128 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
> {
> #ifndef iommu_hap_pt_share
> iommu_hap_pt_share = false;
> -#elif iommu_hap_pt_share
> - ASSERT_UNREACHABLE();
> #endif
> }
> ---8<---
>
> Paul
Thank you for the patch, but I need some time to check it (now I have
some troubles with starting guest VM). I will inform you once I check.
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 16:24 ` Oleksandr
@ 2019-09-25 18:07 ` Oleksandr
2019-09-26 8:32 ` Paul Durrant
0 siblings, 1 reply; 26+ messages in thread
From: Oleksandr @ 2019-09-25 18:07 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, David Scott,
Anthony Perard, Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
Hi Paul
>>>
>>> I may mistake, but looks like
>>>
>>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
>>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>>>
>>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
>>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not
>>> set) .
>>>
>>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
>>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
>>> actually, triggers ASSERT.
>>>
>> Here a minimal patch, leaving 'force pt share' in place. Does this
>> avoid the problem?
>>
>> ---8<---
>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>> index e8763c7fdf..f88a285e7f 100644
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -268,9 +268,11 @@ long
>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>> pi->max_mfn = get_upper_mfn_bound();
>> arch_do_physinfo(pi);
>> if ( iommu_enabled )
>> + {
>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
>> - if ( iommu_hap_pt_share )
>> - pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
>> + if ( iommu_hap_pt_share )
>> + pi->capabilities |=
>> XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
>> + }
>>
>> if ( copy_to_guest(u_sysctl, op, 1) )
>> ret = -EFAULT;
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 7c3003f3f1..6a10a24128 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
>> {
>> #ifndef iommu_hap_pt_share
>> iommu_hap_pt_share = false;
>> -#elif iommu_hap_pt_share
>> - ASSERT_UNREACHABLE();
>> #endif
>> }
>> ---8<---
>>
>> Paul
>
> Thank you for the patch, but I need some time to check it (now I have
> some troubles with starting guest VM). I will inform you once I check.
With the patch applied, the issue I have faced (during Xen boot) is gone
away. But, I haven't analyzed your "per-domain IOMMU control series" yet
to have opinion regarding your patch itself.
I noticed the following:
When iommu_enabled = false (my case, when IOMMU driver is disable), I
can't create guest VM if "dtdev" property is present and contains DMA
masters in the domain config:
Parsing config from /xt/dom.cfg/domd.cfg
ERROR: passthrough not supported on this platform
Even if I add passthrough = "disabled", it still denies:
Parsing config from /xt/dom.cfg/domd.cfg
ERROR: passthrough disabled but devices are specified
Looks like, correct behavior...
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 16:10 ` Paul Durrant
2019-09-25 16:24 ` Oleksandr
@ 2019-09-25 21:34 ` Julien Grall
2019-09-26 8:39 ` Paul Durrant
1 sibling, 1 reply; 26+ messages in thread
From: Julien Grall @ 2019-09-25 21:34 UTC (permalink / raw)
To: Paul Durrant, 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, David Scott, Anthony Perard,
Ian Jackson, nd, Volodymyr Babchuk, Roger Pau Monne
Hi,
On 25/09/2019 17:10, Paul Durrant wrote:
>> -----Original Message-----
>> From: Oleksandr <olekstysh@gmail.com>
>> Sent: 25 September 2019 16:50
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
>> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George Dunlap
>> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
>> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien Grall
>> <julien.grall@arm.com>
>> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>>
>>
>> [CC Julien]
>>
>>
>> Hi Paul
>>
>> I may mistake, but looks like
>>
>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>>
>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
>>
>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
>> actually, triggers ASSERT.
>>
>
> Here a minimal patch, leaving 'force pt share' in place. Does this avoid the problem?
>
> ---8<---
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index e8763c7fdf..f88a285e7f 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> pi->max_mfn = get_upper_mfn_bound();
> arch_do_physinfo(pi);
> if ( iommu_enabled )
> + {
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
> - if ( iommu_hap_pt_share )
> - pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> + if ( iommu_hap_pt_share )
> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> + }
>
> if ( copy_to_guest(u_sysctl, op, 1) )
> ret = -EFAULT;
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 7c3003f3f1..6a10a24128 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
> {
> #ifndef iommu_hap_pt_share
> iommu_hap_pt_share = false;
> -#elif iommu_hap_pt_share
> - ASSERT_UNREACHABLE();
> #endif
IHMO, calling this function is a mistake on platform only supporting
shared page-table so the ASSERT() should be kept here.
This raises the question why the function is actually called from common
code. iommu_hap_enabled() should technically not be used in any code if
the IOMMU is not enabled/present. So what are you trying to prevent here?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 18:07 ` Oleksandr
@ 2019-09-26 8:32 ` Paul Durrant
0 siblings, 0 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-26 8:32 UTC (permalink / raw)
To: 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, David Scott,
Anthony Perard, Ian Jackson, Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Oleksandr <olekstysh@gmail.com>
> Sent: 25 September 2019 19:07
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George Dunlap
> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>
>
> Hi Paul
>
>
> >>>
> >>> I may mistake, but looks like
> >>>
> >>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
> >>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
> >>>
> >>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
> >>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not
> >>> set) .
> >>>
> >>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
> >>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
> >>> actually, triggers ASSERT.
> >>>
> >> Here a minimal patch, leaving 'force pt share' in place. Does this
> >> avoid the problem?
> >>
> >> ---8<---
> >> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> >> index e8763c7fdf..f88a285e7f 100644
> >> --- a/xen/common/sysctl.c
> >> +++ b/xen/common/sysctl.c
> >> @@ -268,9 +268,11 @@ long
> >> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> >> pi->max_mfn = get_upper_mfn_bound();
> >> arch_do_physinfo(pi);
> >> if ( iommu_enabled )
> >> + {
> >> pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
> >> - if ( iommu_hap_pt_share )
> >> - pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> >> + if ( iommu_hap_pt_share )
> >> + pi->capabilities |=
> >> XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> >> + }
> >>
> >> if ( copy_to_guest(u_sysctl, op, 1) )
> >> ret = -EFAULT;
> >> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> >> index 7c3003f3f1..6a10a24128 100644
> >> --- a/xen/include/xen/iommu.h
> >> +++ b/xen/include/xen/iommu.h
> >> @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
> >> {
> >> #ifndef iommu_hap_pt_share
> >> iommu_hap_pt_share = false;
> >> -#elif iommu_hap_pt_share
> >> - ASSERT_UNREACHABLE();
> >> #endif
> >> }
> >> ---8<---
> >>
> >> Paul
> >
> > Thank you for the patch, but I need some time to check it (now I have
> > some troubles with starting guest VM). I will inform you once I check.
>
>
> With the patch applied, the issue I have faced (during Xen boot) is gone
> away. But, I haven't analyzed your "per-domain IOMMU control series" yet
> to have opinion regarding your patch itself.
>
>
> I noticed the following:
>
> When iommu_enabled = false (my case, when IOMMU driver is disable), I
> can't create guest VM if "dtdev" property is present and contains DMA
> masters in the domain config:
>
> Parsing config from /xt/dom.cfg/domd.cfg
> ERROR: passthrough not supported on this platform
>
> Even if I add passthrough = "disabled", it still denies:
>
> Parsing config from /xt/dom.cfg/domd.cfg
> ERROR: passthrough disabled but devices are specified
>
> Looks like, correct behavior...
>
Yes, that all seems correct. Passthrough should not be done without an IOMMU.
Paul
>
> --
> Regards,
>
> Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-25 21:34 ` Julien Grall
@ 2019-09-26 8:39 ` Paul Durrant
2019-09-26 9:13 ` Julien Grall
2019-09-26 9:22 ` Oleksandr
0 siblings, 2 replies; 26+ messages in thread
From: Paul Durrant @ 2019-09-26 8:39 UTC (permalink / raw)
To: 'Julien Grall', 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, David Scott, Anthony Perard,
Ian Jackson, nd, Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Julien Grall <Julien.Grall@arm.com>
> Sent: 25 September 2019 22:34
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Oleksandr' <olekstysh@gmail.com>; 'Jan Beulich'
> <jbeulich@suse.com>
> Cc: nd <nd@arm.com>; Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George
> Dunlap <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>
> Hi,
>
> On 25/09/2019 17:10, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Oleksandr <olekstysh@gmail.com>
> >> Sent: 25 September 2019 16:50
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> >> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei
> Liu
> >> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George
> Dunlap
> >> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> >> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-
> devel@lists.xenproject.org;
> >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien
> Grall
> >> <julien.grall@arm.com>
> >> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
> >>
> >>
> >> [CC Julien]
> >>
> >>
> >> Hi Paul
> >>
> >> I may mistake, but looks like
> >>
> >> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
> >> iommu_use_hap_pt() and need_iommu_pt_sync() macros
> >>
> >> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
> >> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
> >>
> >> So, iommu_setup() calls clear_iommu_hap_pt_share() with
> >> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
> >> actually, triggers ASSERT.
> >>
> >
> > Here a minimal patch, leaving 'force pt share' in place. Does this avoid the problem?
> >
> > ---8<---
> > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> > index e8763c7fdf..f88a285e7f 100644
> > --- a/xen/common/sysctl.c
> > +++ b/xen/common/sysctl.c
> > @@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> > pi->max_mfn = get_upper_mfn_bound();
> > arch_do_physinfo(pi);
> > if ( iommu_enabled )
> > + {
> > pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
> > - if ( iommu_hap_pt_share )
> > - pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> > + if ( iommu_hap_pt_share )
> > + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> > + }
> >
> > if ( copy_to_guest(u_sysctl, op, 1) )
> > ret = -EFAULT;
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index 7c3003f3f1..6a10a24128 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
> > {
> > #ifndef iommu_hap_pt_share
> > iommu_hap_pt_share = false;
> > -#elif iommu_hap_pt_share
> > - ASSERT_UNREACHABLE();
> > #endif
>
> IHMO, calling this function is a mistake on platform only supporting
> shared page-table so the ASSERT() should be kept here.
>
> This raises the question why the function is actually called from common
> code. iommu_hap_enabled() should technically not be used in any code if
> the IOMMU is not enabled/present. So what are you trying to prevent here?
What I'm trying to prevent, on x86, is a situation where the iommu_enabled == false but iommu_hap_pt_share == true. I had, mistakenly, believed that iommu_enabled would never be false for ARM but it seems this is not the case so that situation has to be tolerated. I guess, given the other hunk of my patch, I can actually leave the ASSERT in place and avoid making the call from common code, in which case the function ought to move into an x86 header as well.
Paul
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-26 8:39 ` Paul Durrant
@ 2019-09-26 9:13 ` Julien Grall
2019-09-26 9:17 ` Paul Durrant
2019-09-26 9:22 ` Oleksandr
1 sibling, 1 reply; 26+ messages in thread
From: Julien Grall @ 2019-09-26 9:13 UTC (permalink / raw)
To: Paul Durrant, 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, David Scott, Anthony Perard,
Ian Jackson, nd, Volodymyr Babchuk, Roger Pau Monne
Hi,
On 9/26/19 9:39 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <Julien.Grall@arm.com>
>> Sent: 25 September 2019 22:34
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Oleksandr' <olekstysh@gmail.com>; 'Jan Beulich'
>> <jbeulich@suse.com>
>> Cc: nd <nd@arm.com>; Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew
>> Cooper <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George
>> Dunlap <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
>> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>>
>> Hi,
>>
>> On 25/09/2019 17:10, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Oleksandr <olekstysh@gmail.com>
>>>> Sent: 25 September 2019 16:50
>>>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
>>>> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei
>> Liu
>>>> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
>>>> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George
>> Dunlap
>>>> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
>>>> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-
>> devel@lists.xenproject.org;
>>>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien
>> Grall
>>>> <julien.grall@arm.com>
>>>> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>>>>
>>>>
>>>> [CC Julien]
>>>>
>>>>
>>>> Hi Paul
>>>>
>>>> I may mistake, but looks like
>>>>
>>>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
>>>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>>>>
>>>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
>>>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
>>>>
>>>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
>>>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
>>>> actually, triggers ASSERT.
>>>>
>>>
>>> Here a minimal patch, leaving 'force pt share' in place. Does this avoid the problem?
>>>
>>> ---8<---
>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>> index e8763c7fdf..f88a285e7f 100644
>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>> @@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>> pi->max_mfn = get_upper_mfn_bound();
>>> arch_do_physinfo(pi);
>>> if ( iommu_enabled )
>>> + {
>>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
>>> - if ( iommu_hap_pt_share )
>>> - pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
>>> + if ( iommu_hap_pt_share )
>>> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
>>> + }
>>>
>>> if ( copy_to_guest(u_sysctl, op, 1) )
>>> ret = -EFAULT;
>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>> index 7c3003f3f1..6a10a24128 100644
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
>>> {
>>> #ifndef iommu_hap_pt_share
>>> iommu_hap_pt_share = false;
>>> -#elif iommu_hap_pt_share
>>> - ASSERT_UNREACHABLE();
>>> #endif
>>
>> IHMO, calling this function is a mistake on platform only supporting
>> shared page-table so the ASSERT() should be kept here.
>>
>> This raises the question why the function is actually called from common
>> code. iommu_hap_enabled() should technically not be used in any code if
>> the IOMMU is not enabled/present. So what are you trying to prevent here?
>
> What I'm trying to prevent, on x86, is a situation where the iommu_enabled == false but iommu_hap_pt_share == true.
This is not entirely uncommon to have other variables gated by others.
So what could happen if you have iommu_enabled == false and
iommu_hap_pt_share == true on x86?
> I had, mistakenly, believed that iommu_enabled would never be false for ARM but it seems this is not the case so that situation has to be tolerated. I guess, given the other hunk of my patch, I can actually leave the ASSERT in place and avoid making the call from common code, in which case the function ought to move into an x86 header as well.
By "the function", do you mean clear_iommu_hap_pt_share? If so, I think
it should stay were it is. This is a generic function that might be
re-used for other architecture in the future.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-26 9:13 ` Julien Grall
@ 2019-09-26 9:17 ` Paul Durrant
2019-09-26 9:26 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Paul Durrant @ 2019-09-26 9:17 UTC (permalink / raw)
To: 'Julien Grall', 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, David Scott, Anthony Perard,
Ian Jackson, nd, Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Julien Grall <julien.grall@arm.com>
> Sent: 26 September 2019 10:13
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Oleksandr' <olekstysh@gmail.com>; 'Jan Beulich'
> <jbeulich@suse.com>
> Cc: nd <nd@arm.com>; Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George
> Dunlap <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>
> Hi,
>
> On 9/26/19 9:39 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall <Julien.Grall@arm.com>
> >> Sent: 25 September 2019 22:34
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Oleksandr' <olekstysh@gmail.com>; 'Jan Beulich'
> >> <jbeulich@suse.com>
> >> Cc: nd <nd@arm.com>; Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>;
> Andrew
> >> Cooper <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>;
> George
> >> Dunlap <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> >> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-
> devel@lists.xenproject.org;
> >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>
> >> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
> >>
> >> Hi,
> >>
> >> On 25/09/2019 17:10, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Oleksandr <olekstysh@gmail.com>
> >>>> Sent: 25 September 2019 16:50
> >>>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> >>>> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei
> >> Liu
> >>>> <wl@xen.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> >>>> <Andrew.Cooper3@citrix.com>; David Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; George
> >> Dunlap
> >>>> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; Ian Jackson
> >>>> <Ian.Jackson@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; xen-
> >> devel@lists.xenproject.org;
> >>>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>; Julien
> >> Grall
> >>>> <julien.grall@arm.com>
> >>>> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
> >>>>
> >>>>
> >>>> [CC Julien]
> >>>>
> >>>>
> >>>> Hi Paul
> >>>>
> >>>> I may mistake, but looks like
> >>>>
> >>>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
> >>>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
> >>>>
> >>>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
> >>>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
> >>>>
> >>>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
> >>>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
> >>>> actually, triggers ASSERT.
> >>>>
> >>>
> >>> Here a minimal patch, leaving 'force pt share' in place. Does this avoid the problem?
> >>>
> >>> ---8<---
> >>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> >>> index e8763c7fdf..f88a285e7f 100644
> >>> --- a/xen/common/sysctl.c
> >>> +++ b/xen/common/sysctl.c
> >>> @@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> >>> pi->max_mfn = get_upper_mfn_bound();
> >>> arch_do_physinfo(pi);
> >>> if ( iommu_enabled )
> >>> + {
> >>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
> >>> - if ( iommu_hap_pt_share )
> >>> - pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> >>> + if ( iommu_hap_pt_share )
> >>> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> >>> + }
> >>>
> >>> if ( copy_to_guest(u_sysctl, op, 1) )
> >>> ret = -EFAULT;
> >>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> >>> index 7c3003f3f1..6a10a24128 100644
> >>> --- a/xen/include/xen/iommu.h
> >>> +++ b/xen/include/xen/iommu.h
> >>> @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
> >>> {
> >>> #ifndef iommu_hap_pt_share
> >>> iommu_hap_pt_share = false;
> >>> -#elif iommu_hap_pt_share
> >>> - ASSERT_UNREACHABLE();
> >>> #endif
> >>
> >> IHMO, calling this function is a mistake on platform only supporting
> >> shared page-table so the ASSERT() should be kept here.
> >>
> >> This raises the question why the function is actually called from common
> >> code. iommu_hap_enabled() should technically not be used in any code if
> >> the IOMMU is not enabled/present. So what are you trying to prevent here?
> >
> > What I'm trying to prevent, on x86, is a situation where the iommu_enabled == false but
> iommu_hap_pt_share == true.
>
> This is not entirely uncommon to have other variables gated by others.
> So what could happen if you have iommu_enabled == false and
> iommu_hap_pt_share == true on x86?
>
Well, I was hoping to avoid the nested if in sysctl.c.
> > I had, mistakenly, believed that iommu_enabled would never be false for ARM but it seems this is not
> the case so that situation has to be tolerated. I guess, given the other hunk of my patch, I can
> actually leave the ASSERT in place and avoid making the call from common code, in which case the
> function ought to move into an x86 header as well.
>
> By "the function", do you mean clear_iommu_hap_pt_share?
Yes.
> If so, I think
> it should stay were it is. This is a generic function that might be
> re-used for other architecture in the future.
>
That seems like a bit of a long shot. If I remove the call from iommu_setup() then the only remaining callers are in x86 code, but I suppose it can stay where it is to avoid the churn. I'll spin a new test patch.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-26 8:39 ` Paul Durrant
2019-09-26 9:13 ` Julien Grall
@ 2019-09-26 9:22 ` Oleksandr
1 sibling, 0 replies; 26+ messages in thread
From: Oleksandr @ 2019-09-26 9:22 UTC (permalink / raw)
To: Paul Durrant, 'Julien Grall', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, David Scott, Anthony Perard,
Ian Jackson, nd, Volodymyr Babchuk, Roger Pau Monne
On 26.09.19 11:39, Paul Durrant wrote:
Hi Paul
>>
>>>> [CC Julien]
>>>>
>>>>
>>>> Hi Paul
>>>>
>>>> I may mistake, but looks like
>>>>
>>>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
>>>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>>>>
>>>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
>>>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
>>>>
>>>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
>>>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
>>>> actually, triggers ASSERT.
>>>>
>>> Here a minimal patch, leaving 'force pt share' in place. Does this avoid the problem?
>>>
>>> ---8<---
>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>> index e8763c7fdf..f88a285e7f 100644
>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>> @@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>> pi->max_mfn = get_upper_mfn_bound();
>>> arch_do_physinfo(pi);
>>> if ( iommu_enabled )
>>> + {
>>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
>>> - if ( iommu_hap_pt_share )
>>> - pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
>>> + if ( iommu_hap_pt_share )
>>> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
>>> + }
>>>
>>> if ( copy_to_guest(u_sysctl, op, 1) )
>>> ret = -EFAULT;
>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>> index 7c3003f3f1..6a10a24128 100644
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
>>> {
>>> #ifndef iommu_hap_pt_share
>>> iommu_hap_pt_share = false;
>>> -#elif iommu_hap_pt_share
>>> - ASSERT_UNREACHABLE();
>>> #endif
>> IHMO, calling this function is a mistake on platform only supporting
>> shared page-table so the ASSERT() should be kept here.
>>
>> This raises the question why the function is actually called from common
>> code. iommu_hap_enabled() should technically not be used in any code if
>> the IOMMU is not enabled/present. So what are you trying to prevent here?
> What I'm trying to prevent, on x86, is a situation where the iommu_enabled == false but iommu_hap_pt_share == true. I had, mistakenly, believed that iommu_enabled would never be false for ARM but it seems this is not the case so that situation has to be tolerated. I guess, given the other hunk of my patch, I can actually leave the ASSERT in place and avoid making the call from common code, in which case the function ought to move into an x86 header as well.
Not all Arm based SoCs (which supported by Xen) contains SMMU (the only
one supported driver at the moment) or IPMMU-VMSA (on review now, but,
it will be under CONFIG_EXPERT when merged, so disabled by default). So,
"iommu_enabled" can be false.
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
2019-09-26 9:17 ` Paul Durrant
@ 2019-09-26 9:26 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2019-09-26 9:26 UTC (permalink / raw)
To: Paul Durrant, 'Oleksandr', 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, xen-devel, Wei Liu,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Tamas K Lengyel, David Scott, Anthony Perard,
Ian Jackson, nd, Volodymyr Babchuk, Roger Pau Monne
Hi Paul,
On 9/26/19 10:17 AM, Paul Durrant wrote:
>> If so, I think
>> it should stay were it is. This is a generic function that might be
>> re-used for other architecture in the future.
>>
>
> That seems like a bit of a long shot. If I remove the call from iommu_setup() then the only remaining callers are in x86 code, but I suppose it can stay where it is to avoid the churn. I'll spin a new test patch.
Not really, I know that we will likely need it on Arm when MSI doorbell
will be exposed to the guest because some mappings cannot be accessed by
the processor. Although, I can't tell when this will happen.
Anyway, I will have a look at your next patch :).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-09-26 9:26 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-18 10:41 [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 1/4] remove late (on-demand) construction of IOMMU page tables Paul Durrant
2019-09-25 9:00 ` Paul Durrant
2019-09-25 9:16 ` Wei Liu
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 2/4] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 3/4] tools/ocaml: abi check: Cope with consecutive relevant enums Paul Durrant
2019-09-18 10:41 ` [Xen-devel] [PATCH v13 4/4] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
2019-09-18 15:20 ` Anthony PERARD
2019-09-25 8:40 ` [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control Paul Durrant
2019-09-25 8:50 ` Jan Beulich
2019-09-25 8:56 ` Paul Durrant
2019-09-25 15:49 ` Oleksandr
2019-09-25 15:55 ` Paul Durrant
2019-09-25 16:03 ` Paul Durrant
2019-09-25 16:04 ` Paul Durrant
2019-09-25 16:14 ` Oleksandr
2019-09-25 16:10 ` Paul Durrant
2019-09-25 16:24 ` Oleksandr
2019-09-25 18:07 ` Oleksandr
2019-09-26 8:32 ` Paul Durrant
2019-09-25 21:34 ` Julien Grall
2019-09-26 8:39 ` Paul Durrant
2019-09-26 9:13 ` Julien Grall
2019-09-26 9:17 ` Paul Durrant
2019-09-26 9:26 ` Julien Grall
2019-09-26 9:22 ` Oleksandr
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.