[RFC 0/7] Proposal to make x86 IOMMU driver support configurable

[RFC 0/7] Proposal to make x86 IOMMU driver support configurable

[Xenia Ragiadakou]

This series aims to provide a means to render the iommu driver support for x86
configurable. Currently, irrespectively of the target platform, both AMD and
Intel iommu drivers are built. This is the case because the existent Kconfig
infrastructure does not provide any facilities for finer-grained configuration.

The series adds two new Kconfig options, AMD_IOMMU and INTEL_VTD, that can be
used to generate a tailored iommu configuration for a given platform.

This series will be part of a broader effort to separate platform specific
code and it is sent as an RFC to gather feedback regarding the way the
separation should be performed.

Xenia Ragiadakou (7):
  x86/iommu: make AMD-Vi and Intel VT-d support configurable
  x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific
  x86/iommu: iommu_igfx, iommu_qinval and iommu_snoop are VT-d specific
  x86/acpi: separate AMD-Vi and VT-d specific functions
  x86/iommu: the code addressing CVE-2011-1898 is VT-d specific
  x86/iommu: call pi_update_irte through an hvm_function callback
  x86/dpci: move hvm_dpci_isairq_eoi() to generic HVM code

 xen/arch/x86/hvm/vmx/vmx.c               | 10 ++++
 xen/arch/x86/include/asm/acpi.h          | 14 ++++++
 xen/arch/x86/include/asm/hvm/hvm.h       | 22 ++++++++
 xen/arch/x86/include/asm/hvm/io.h        |  1 +
 xen/arch/x86/include/asm/hvm/vmx/vmx.h   | 11 ++++
 xen/arch/x86/include/asm/iommu.h         |  5 +-
 xen/arch/x86/pv/hypercall.c              |  2 +
 xen/arch/x86/x86_64/entry.S              |  2 +
 xen/drivers/passthrough/Kconfig          | 16 ++++++
 xen/drivers/passthrough/Makefile         |  4 +-
 xen/drivers/passthrough/amd/iommu_init.c |  2 +
 xen/drivers/passthrough/iommu.c          |  7 ++-
 xen/drivers/passthrough/vtd/x86/Makefile |  1 -
 xen/drivers/passthrough/vtd/x86/hvm.c    | 64 ------------------------
 xen/drivers/passthrough/x86/hvm.c        | 47 +++++++++++++++--
 xen/include/xen/acpi.h                   |  3 --
 xen/include/xen/iommu.h                  | 11 ++--
 17 files changed, 141 insertions(+), 81 deletions(-)
 delete mode 100644 xen/drivers/passthrough/vtd/x86/hvm.c

-- 
2.37.2

[RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Xenia Ragiadakou]

Currently, for x86 platforms, Xen does not provide to the users any
configuration control over the IOMMU support and can only be built with
both AMD and Intel IOMMU drivers enabled.
However, there are use cases, e.g in safety-critical systems, that require
Xen to be able to be configured to exclude unused code. A smaller tailored
configuration would help Xen to meet faster certification requirements for
individual platforms.

Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow code
specific to each IOMMU technology to be separated and, when not required,
stripped. AMD_IOMMU enables IOMMU support for platforms that implement the
AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for
platforms that implement the Intel Virtualization Technology for Directed I/O.

Since no functional change is intended regarding the default configuration
of an x86 system, both options depend on x86 and default to 'y'.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/drivers/passthrough/Kconfig  | 16 ++++++++++++++++
 xen/drivers/passthrough/Makefile |  4 ++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
index 479d7de57a..82465aa627 100644
--- a/xen/drivers/passthrough/Kconfig
+++ b/xen/drivers/passthrough/Kconfig
@@ -37,6 +37,22 @@ config IPMMU_VMSA
 
 endif
 
+config AMD_IOMMU
+	bool "AMD IOMMU"
+	depends on X86
+	default y
+	---help---
+	  Enables I/O virtualization on platforms that implement the
+	  AMD I/O Virtualization Technology (IOMMU).
+
+config INTEL_VTD
+	bool "Intel VT-d"
+	depends on X86
+	default y
+	---help---
+	  Enables I/O virtualization on platforms that implement the
+	  Intel Virtualization Technology for Directed I/O (Intel VT-d).
+
 config IOMMU_FORCE_PT_SHARE
 	bool
 
diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
index a5efa22714..d4fc7a3ddc 100644
--- a/xen/drivers/passthrough/Makefile
+++ b/xen/drivers/passthrough/Makefile
@@ -1,5 +1,5 @@
-obj-$(CONFIG_X86) += vtd/
-obj-$(CONFIG_X86) += amd/
+obj-$(CONFIG_INTEL_VTD) += vtd/
+obj-$(CONFIG_AMD_IOMMU) += amd/
 obj-$(CONFIG_X86) += x86/
 obj-$(CONFIG_ARM) += arm/
 
-- 
2.37.2

[RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific

[Xenia Ragiadakou]

Move its definition to the AMD-Vi driver and use CONFIG_AMD_IOMMU
to guard its usage in common code.

Take the opportunity to replace bool_t with bool and 1 with true.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/drivers/passthrough/amd/iommu_init.c | 2 ++
 xen/drivers/passthrough/iommu.c          | 3 ++-
 xen/include/xen/iommu.h                  | 4 +++-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 1f14aaf49e..c4a41630e5 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -36,6 +36,8 @@ static struct radix_tree_root ivrs_maps;
 LIST_HEAD_READ_MOSTLY(amd_iommu_head);
 bool_t iommuv2_enabled;
 
+bool __read_mostly amd_iommu_perdev_intremap = true;
+
 static bool iommu_has_ht_flag(struct amd_iommu *iommu, u8 mask)
 {
     return iommu->ht_flags & mask;
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5e2a720d29..1a02fdc453 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -58,7 +58,6 @@ bool __read_mostly iommu_hap_pt_share = true;
 #endif
 
 bool_t __read_mostly iommu_debug;
-bool_t __read_mostly amd_iommu_perdev_intremap = 1;
 
 DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
 
@@ -115,8 +114,10 @@ static int __init cf_check parse_iommu_param(const char *s)
             if ( val )
                 iommu_verbose = 1;
         }
+#ifdef CONFIG_AMD_IOMMU
         else if ( (val = parse_boolean("amd-iommu-perdev-intremap", s, ss)) >= 0 )
             amd_iommu_perdev_intremap = val;
+#endif
         else if ( (val = parse_boolean("dom0-passthrough", s, ss)) >= 0 )
             iommu_hwdom_passthrough = val;
         else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 )
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 4f22fc1bed..6b06732792 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -104,7 +104,9 @@ static inline void clear_iommu_hap_pt_share(void)
 }
 
 extern bool_t iommu_debug;
-extern bool_t amd_iommu_perdev_intremap;
+#ifdef CONFIG_AMD_IOMMU
+extern bool amd_iommu_perdev_intremap;
+#endif
 
 extern bool iommu_hwdom_strict, iommu_hwdom_passthrough, iommu_hwdom_inclusive;
 extern int8_t iommu_hwdom_reserved;
-- 
2.37.2

[RFC 3/7] x86/iommu: iommu_igfx, iommu_qinval and iommu_snoop are VT-d specific

[Xenia Ragiadakou]

Use CONFIG_INTEL_VTD to guard their usage in common code.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/drivers/passthrough/iommu.c | 4 +++-
 xen/include/xen/iommu.h         | 6 +++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 1a02fdc453..dcb5ec49ac 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -82,11 +82,13 @@ static int __init cf_check parse_iommu_param(const char *s)
         else if ( ss == s + 23 && !strncmp(s, "quarantine=scratch-page", 23) )
             iommu_quarantine = IOMMU_quarantine_scratch_page;
 #endif
-#ifdef CONFIG_X86
+#ifdef CONFIG_INTEL_VTD
         else if ( (val = parse_boolean("igfx", s, ss)) >= 0 )
             iommu_igfx = val;
         else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
             iommu_qinval = val;
+#endif
+#ifdef CONFIG_X86
         else if ( (val = parse_boolean("superpages", s, ss)) >= 0 )
             iommu_superpages = val;
 #endif
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 6b06732792..d9285a7760 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -74,9 +74,13 @@ extern enum __packed iommu_intremap {
    iommu_intremap_restricted,
    iommu_intremap_full,
 } iommu_intremap;
-extern bool iommu_igfx, iommu_qinval, iommu_snoop;
 #else
 # define iommu_intremap false
+#endif
+
+#ifdef CONFIG_INTEL_VTD
+extern bool iommu_igfx, iommu_qinval, iommu_snoop;
+#else
 # define iommu_snoop false
 #endif
 
-- 
2.37.2

[RFC 4/7] x86/acpi: separate AMD-Vi and VT-d specific functions

[Xenia Ragiadakou]

The functions acpi_dmar_init() and acpi_dmar_zap/reinstate() are
VT-d specific while the function acpi_ivrs_init() is AMD-Vi specific.
To eliminate dead code, they need to be guarded under CONFIG_INTEL_VTD
and CONFIG_AMD_IOMMU, respectively.

Instead of adding #ifdef guards around the function calls, implement them
as empty static inline functions.

Take the opportunity to move the declarations of acpi_dmar_zap/reinstate() to
the arch specific header.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/x86/include/asm/acpi.h | 14 ++++++++++++++
 xen/include/xen/acpi.h          |  3 ---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/acpi.h b/xen/arch/x86/include/asm/acpi.h
index c453450a74..06461fedcd 100644
--- a/xen/arch/x86/include/asm/acpi.h
+++ b/xen/arch/x86/include/asm/acpi.h
@@ -140,8 +140,22 @@ extern u32 pmtmr_ioport;
 extern unsigned int pmtmr_width;
 
 void acpi_iommu_init(void);
+
+#ifdef CONFIG_INTEL_VTD
 int acpi_dmar_init(void);
+void acpi_dmar_zap(void);
+void acpi_dmar_reinstate(void);
+#else
+static inline int acpi_dmar_init(void) { return -ENODEV; }
+static inline void acpi_dmar_zap(void) {}
+static inline void acpi_dmar_reinstate(void) {}
+#endif
+
+#ifdef CONFIG_AMD_IOMMU
 int acpi_ivrs_init(void);
+#else
+static inline int acpi_ivrs_init(void) { return -ENODEV; }
+#endif
 
 void acpi_mmcfg_init(void);
 
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index 1b9c75e68f..82b24a5ef0 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -206,9 +206,6 @@ static inline int acpi_get_pxm(acpi_handle handle)
 
 void acpi_reboot(void);
 
-void acpi_dmar_zap(void);
-void acpi_dmar_reinstate(void);
-
 #endif /* __ASSEMBLY__ */
 
 #endif /*_LINUX_ACPI_H*/
-- 
2.37.2

[RFC 5/7] x86/iommu: the code addressing CVE-2011-1898 is VT-d specific

[Xenia Ragiadakou]

The variable untrusted_msi indicates whether the system is vulnerable to
CVE-2011-1898. This vulnerablity is VT-d specific.
Place the code that addresses the issue under CONFIG_INTEL_VTD.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/x86/include/asm/iommu.h | 2 ++
 xen/arch/x86/pv/hypercall.c      | 2 ++
 xen/arch/x86/x86_64/entry.S      | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index fc0afe35bf..41bd1b9e05 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -127,7 +127,9 @@ int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma,
                            unsigned int flag);
 void iommu_identity_map_teardown(struct domain *d);
 
+#ifdef CONFIG_INTEL_VTD
 extern bool untrusted_msi;
+#endif
 
 int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
                    const uint8_t gvec);
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 2eedfbfae8..0e1b03904c 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -193,8 +193,10 @@ void pv_ring1_init_hypercall_page(void *p)
 
 void do_entry_int82(struct cpu_user_regs *regs)
 {
+#ifdef CONFIG_INTEL_VTD
     if ( unlikely(untrusted_msi) )
         check_for_unexpected_msi((uint8_t)regs->entry_vector);
+#endif
 
     _pv_hypercall(regs, true /* compat */);
 }
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index ae01285181..2e06c0a6c1 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -406,11 +406,13 @@ ENTRY(int80_direct_trap)
 .Lint80_cr3_okay:
         sti
 
+#ifdef CONFIG_INTEL_VTD
         cmpb  $0,untrusted_msi(%rip)
 UNLIKELY_START(ne, msi_check)
         movl  $0x80,%edi
         call  check_for_unexpected_msi
 UNLIKELY_END(msi_check)
+#endif
 
         movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
 
-- 
2.37.2

[RFC 6/7] x86/iommu: call pi_update_irte through an hvm_function callback

[Xenia Ragiadakou]

Posted interrupt support in Xen is currently implemented only for the
Intel platforms. Instead of calling directly pi_update_irte() from the
common hvm code, add a pi_update_irte callback to the hvm_function_table.
Then, create a wrapper function hvm_pi_update_irte() to be used by the
common hvm code.

In the pi_update_irte callback prototype, pass the vcpu as first parameter
instead of the posted-interrupt descriptor that is platform specific, and
remove the const qualifier from the parameter gvec since it is not needed
and because it does not compile with the alternative code patching in use.

Move the declaration of pi_update_irte() from asm/iommu.h to asm/hvm/vmx/vmx.h
since it is hvm and Intel specific.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/x86/hvm/vmx/vmx.c             | 10 ++++++++++
 xen/arch/x86/include/asm/hvm/hvm.h     | 22 ++++++++++++++++++++++
 xen/arch/x86/include/asm/hvm/vmx/vmx.h | 11 +++++++++++
 xen/arch/x86/include/asm/iommu.h       |  3 ---
 xen/drivers/passthrough/x86/hvm.c      |  5 ++---
 5 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 7c81b80710..3080f1ef41 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2134,6 +2134,14 @@ static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec)
     return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc);
 }
 
+static int cf_check vmx_pi_update_irte(const struct vcpu *v,
+                                       const struct pirq *pirq, uint8_t gvec)
+{
+    const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
+
+    return pi_update_irte(pi_desc, pirq, gvec);
+}
+
 static void cf_check vmx_handle_eoi(uint8_t vector, int isr)
 {
     uint8_t old_svi = set_svi(isr);
@@ -2582,6 +2590,8 @@ static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
     .tsc_scaling = {
         .max_ratio = VMX_TSC_MULTIPLIER_MAX,
     },
+
+    .pi_update_irte = vmx_pi_update_irte,
 };
 
 /* Handle VT-d posted-interrupt when VCPU is blocked. */
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 93254651f2..f16b9edeaf 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -28,6 +28,8 @@
 #include <asm/x86_emulate.h>
 #include <asm/hvm/asid.h>
 
+struct pirq; /* needed by pi_update_irte */
+
 #ifdef CONFIG_HVM_FEP
 /* Permit use of the Forced Emulation Prefix in HVM guests */
 extern bool_t opt_hvm_fep;
@@ -250,6 +252,9 @@ struct hvm_function_table {
         /* Architecture function to setup TSC scaling ratio */
         void (*setup)(struct vcpu *v);
     } tsc_scaling;
+
+    int (*pi_update_irte)(const struct vcpu *v,
+                          const struct pirq *pirq, uint8_t gvec);
 };
 
 extern struct hvm_function_table hvm_funcs;
@@ -774,6 +779,16 @@ static inline void hvm_set_nonreg_state(struct vcpu *v,
         alternative_vcall(hvm_funcs.set_nonreg_state, v, nrs);
 }
 
+static inline int hvm_pi_update_irte(const struct vcpu *v,
+                                     const struct pirq *pirq, uint8_t gvec)
+{
+    if ( hvm_funcs.pi_update_irte )
+        return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec);
+
+    return -EOPNOTSUPP;
+}
+
+
 #else  /* CONFIG_HVM */
 
 #define hvm_enabled false
@@ -893,6 +908,13 @@ static inline void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
     ASSERT_UNREACHABLE();
 }
 
+static inline int hvm_pi_update_irte(const struct vcpu *v,
+                                     const struct pirq *pirq, uint8_t gvec)
+{
+    ASSERT_UNREACHABLE();
+    return -EOPNOTSUPP;
+}
+
 #define is_viridian_domain(d) ((void)(d), false)
 #define is_viridian_vcpu(v) ((void)(v), false)
 #define has_viridian_time_ref_count(d) ((void)(d), false)
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 8eedf59155..17f13e79a0 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -155,6 +155,17 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
     clear_bit(POSTED_INTR_SN, &pi_desc->control);
 }
 
+#ifdef CONFIG_INTEL_VTD
+int pi_update_irte(const struct pi_desc *pi_desc,
+                   const struct pirq *pirq, const uint8_t gvec);
+#else
+static inline int pi_update_irte(const struct pi_desc *pi_desc,
+                                 const struct pirq *pirq, const uint8_t gvec)
+{
+    return -EOPNOTSUPP;
+}
+#endif
+
 /*
  * Exit Reasons
  */
diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index 41bd1b9e05..c433c75b76 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -131,9 +131,6 @@ void iommu_identity_map_teardown(struct domain *d);
 extern bool untrusted_msi;
 #endif
 
-int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
-                   const uint8_t gvec);
-
 extern bool iommu_non_coherent, iommu_superpages;
 
 static inline void iommu_sync_cache(const void *addr, unsigned int size)
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index a16e0e5344..e720461a14 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -381,8 +381,7 @@ int pt_irq_create_bind(
 
         /* Use interrupt posting if it is supported. */
         if ( iommu_intpost )
-            pi_update_irte(vcpu ? &vcpu->arch.hvm.vmx.pi_desc : NULL,
-                           info, pirq_dpci->gmsi.gvec);
+            hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec);
 
         if ( pt_irq_bind->u.msi.gflags & XEN_DOMCTL_VMSI_X86_UNMASKED )
         {
@@ -672,7 +671,7 @@ int pt_irq_destroy_bind(
             what = "bogus";
     }
     else if ( pirq_dpci && pirq_dpci->gmsi.posted )
-        pi_update_irte(NULL, pirq, 0);
+        hvm_pi_update_irte(NULL, pirq, 0);
 
     if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) &&
          list_empty(&pirq_dpci->digl_list) )
-- 
2.37.2

[RFC 7/7] x86/dpci: move hvm_dpci_isairq_eoi() to generic HVM code

[Xenia Ragiadakou]

The function hvm_dpci_isairq_eoi() has no dependencies on VT-d driver code
and can be moved from xen/drivers/passthrough/vtd/x86/hvm.c to
xen/drivers/passthrough/x86/hvm.c.

Remove the now empty xen/drivers/passthrough/vtd/x86/hvm.c.

Since the funcion is hvm specific, move its declaration from xen/iommu.h
to asm/hvm/io.h.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---

I was not sure how to handle the copyright. I assume that I have to
retain the copyright of Weidong Han <weidong.han@intel.com>, right?

 xen/arch/x86/include/asm/hvm/io.h        |  1 +
 xen/drivers/passthrough/vtd/x86/Makefile |  1 -
 xen/drivers/passthrough/vtd/x86/hvm.c    | 64 ------------------------
 xen/drivers/passthrough/x86/hvm.c        | 42 ++++++++++++++++
 xen/include/xen/iommu.h                  |  1 -
 5 files changed, 43 insertions(+), 66 deletions(-)
 delete mode 100644 xen/drivers/passthrough/vtd/x86/hvm.c

diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 54e0161b49..5e1071b798 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -102,6 +102,7 @@ bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
 bool handle_pio(uint16_t port, unsigned int size, int dir);
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq);
+void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq);
 void msix_write_completion(struct vcpu *);
 
 #ifdef CONFIG_HVM
diff --git a/xen/drivers/passthrough/vtd/x86/Makefile b/xen/drivers/passthrough/vtd/x86/Makefile
index 4ef00a4c5b..fe20a0b019 100644
--- a/xen/drivers/passthrough/vtd/x86/Makefile
+++ b/xen/drivers/passthrough/vtd/x86/Makefile
@@ -1,3 +1,2 @@
 obj-y += ats.o
-obj-$(CONFIG_HVM) += hvm.o
 obj-y += vtd.o
diff --git a/xen/drivers/passthrough/vtd/x86/hvm.c b/xen/drivers/passthrough/vtd/x86/hvm.c
deleted file mode 100644
index bc776cf7da..0000000000
--- a/xen/drivers/passthrough/vtd/x86/hvm.c
+++ /dev/null
@@ -1,64 +0,0 @@
-/*
- * Copyright (c) 2008, Intel Corporation.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program; If not, see <http://www.gnu.org/licenses/>.
- *
- * Copyright (C) Allen Kay <allen.m.kay@intel.com>
- * Copyright (C) Weidong Han <weidong.han@intel.com>
- */
-
-#include <xen/iommu.h>
-#include <xen/irq.h>
-#include <xen/sched.h>
-
-static int cf_check _hvm_dpci_isairq_eoi(
-    struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg)
-{
-    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
-    unsigned int isairq = (long)arg;
-    const struct dev_intx_gsi_link *digl;
-
-    list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
-    {
-        unsigned int link = hvm_pci_intx_link(digl->device, digl->intx);
-
-        if ( hvm_irq->pci_link.route[link] == isairq )
-        {
-            hvm_pci_intx_deassert(d, digl->device, digl->intx);
-            if ( --pirq_dpci->pending == 0 )
-                pirq_guest_eoi(dpci_pirq(pirq_dpci));
-        }
-    }
-
-    return 0;
-}
-
-void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq)
-{
-    struct hvm_irq_dpci *dpci = NULL;
-
-    ASSERT(isairq < NR_ISAIRQS);
-    if ( !is_iommu_enabled(d) )
-        return;
-
-    write_lock(&d->event_lock);
-
-    dpci = domain_get_irq_dpci(d);
-
-    if ( dpci && test_bit(isairq, dpci->isairq_map) )
-    {
-        /* Multiple mirq may be mapped to one isa irq */
-        pt_pirq_iterate(d, _hvm_dpci_isairq_eoi, (void *)(long)isairq);
-    }
-    write_unlock(&d->event_lock);
-}
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index e720461a14..94cc1646c0 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -924,6 +924,48 @@ static void hvm_gsi_eoi(struct domain *d, unsigned int gsi)
     hvm_pirq_eoi(pirq);
 }
 
+static int cf_check _hvm_dpci_isairq_eoi(
+    struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg)
+{
+    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+    unsigned int isairq = (long)arg;
+    const struct dev_intx_gsi_link *digl;
+
+    list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
+    {
+        unsigned int link = hvm_pci_intx_link(digl->device, digl->intx);
+
+        if ( hvm_irq->pci_link.route[link] == isairq )
+        {
+            hvm_pci_intx_deassert(d, digl->device, digl->intx);
+            if ( --pirq_dpci->pending == 0 )
+                pirq_guest_eoi(dpci_pirq(pirq_dpci));
+        }
+    }
+
+    return 0;
+}
+
+void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq)
+{
+    struct hvm_irq_dpci *dpci = NULL;
+
+    ASSERT(isairq < NR_ISAIRQS);
+    if ( !is_iommu_enabled(d) )
+        return;
+
+    write_lock(&d->event_lock);
+
+    dpci = domain_get_irq_dpci(d);
+
+    if ( dpci && test_bit(isairq, dpci->isairq_map) )
+    {
+        /* Multiple mirq may be mapped to one isa irq */
+        pt_pirq_iterate(d, _hvm_dpci_isairq_eoi, (void *)(long)isairq);
+    }
+    write_unlock(&d->event_lock);
+}
+
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi)
 {
     const struct hvm_irq_dpci *hvm_irq_dpci;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index d9285a7760..b9e1288de8 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -202,7 +202,6 @@ int hvm_do_IRQ_dpci(struct domain *, struct pirq *);
 int pt_irq_create_bind(struct domain *, const struct xen_domctl_bind_pt_irq *);
 int pt_irq_destroy_bind(struct domain *, const struct xen_domctl_bind_pt_irq *);
 
-void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq);
 struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *);
 void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci);
 
-- 
2.37.2

Re: [RFC 0/7] Proposal to make x86 IOMMU driver support configurable

[Andrew Cooper]

On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
> This series aims to provide a means to render the iommu driver support for x86
> configurable. Currently, irrespectively of the target platform, both AMD and
> Intel iommu drivers are built. This is the case because the existent Kconfig
> infrastructure does not provide any facilities for finer-grained configuration.
>
> The series adds two new Kconfig options, AMD_IOMMU and INTEL_VTD, that can be
> used to generate a tailored iommu configuration for a given platform.
>
> This series will be part of a broader effort to separate platform specific
> code and it is sent as an RFC to gather feedback regarding the way the
> separation should be performed.

Hello,

Thanks for the series.

From discussions in the past, we do want CONFIG_INTEL/AMD/etc and we do
want these to be selectable and available for randconfig to test.

Some sub-features are more complicated, because e.g. Centaur have CPUs
with a VT-x implementation.  This will need expressing in whatever
Kconfig scheme we end up with.

IOMMUs are more tricky still.  They are (for most intents and purposes)
mandatory on systems with >254 CPUs.  We could in principle start
supporting asymmetric IRQ routing on large systems, but Xen doesn't
currently and it would be a lot work that's definitely not high on the
priority list.  At a minimum, this will need expressing in the Kconfig
help text.

We need to decide whether it is sensible to allow users to turn off
IOMMU support.  It probably is, because you could trivially do this by
selecting CONFIG_INTEL only, and booting the result on an AMD system.


For the names, I don't think AMD_IOMMU vs INTEL_VTD is a sensible. 
Probably wants to be INTEL_IOMMU to match.

~Andrew

Re: [RFC 0/7] Proposal to make x86 IOMMU driver support configurable

[Jan Beulich]

On 19.12.2022 19:28, Andrew Cooper wrote:
> IOMMUs are more tricky still.  They are (for most intents and purposes)
> mandatory on systems with >254 CPUs.  We could in principle start
> supporting asymmetric IRQ routing on large systems, but Xen doesn't
> currently and it would be a lot work that's definitely not high on the
> priority list.  At a minimum, this will need expressing in the Kconfig
> help text.
> 
> We need to decide whether it is sensible to allow users to turn off
> IOMMU support.  It probably is, because you could trivially do this by
> selecting CONFIG_INTEL only, and booting the result on an AMD system.

One other thing Andrew and I have been talking about: We probably do
not want to tie the IOMMU vendor control to CPU vendor one. IOW we'd
like to be able to e.g. build a hypervisor supporting Intel (only) as
the CPU vendor, but at the same time having support for an AMD IOMMU.

> For the names, I don't think AMD_IOMMU vs INTEL_VTD is a sensible. 
> Probably wants to be INTEL_IOMMU to match.

Or simply VTD, covering the case than some other vendor comes up with a
clone. But of course we have that same issue with "AMD" and Hygon ...

Jan

Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Andrew Cooper]

On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
> diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
> index 479d7de57a..82465aa627 100644
> --- a/xen/drivers/passthrough/Kconfig
> +++ b/xen/drivers/passthrough/Kconfig
> @@ -37,6 +37,22 @@ config IPMMU_VMSA
>  
>  endif
>  
> +config AMD_IOMMU
> +	bool "AMD IOMMU"
> +	depends on X86
> +	default y
> +	---help---

We're trying to phase out ---help---, so please just use help.

~Andrew

Re: [RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific

[Andrew Cooper]

On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 5e2a720d29..1a02fdc453 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -58,7 +58,6 @@ bool __read_mostly iommu_hap_pt_share = true;
>  #endif
>  
>  bool_t __read_mostly iommu_debug;
> -bool_t __read_mostly amd_iommu_perdev_intremap = 1;
>  
>  DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>  
> @@ -115,8 +114,10 @@ static int __init cf_check parse_iommu_param(const char *s)
>              if ( val )
>                  iommu_verbose = 1;
>          }
> +#ifdef CONFIG_AMD_IOMMU
>          else if ( (val = parse_boolean("amd-iommu-perdev-intremap", s, ss)) >= 0 )
>              amd_iommu_perdev_intremap = val;
> +#endif

See parse_cet() and the use of no_config_param() so users get a bit of a
hint as to why the option they specified is getting ignored.

~Andrew

Re: [RFC 5/7] x86/iommu: the code addressing CVE-2011-1898 is VT-d specific

[Andrew Cooper]

On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
> The variable untrusted_msi indicates whether the system is vulnerable to
> CVE-2011-1898. This vulnerablity is VT-d specific.
> Place the code that addresses the issue under CONFIG_INTEL_VTD.
>
> No functional change intended.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

Actually, this variable is pretty bogus.  I think I'd like to delete it
entirely.

There are systems with no IOMMU at all, and we certainly used to let PV
Passthrough go ahead.  (Not sure we do any more.)

There are systems with DMA remapping only, but no interrupt remapping. 
These are known insecure.  I'm honestly not convinced that an ISR read
and crash is useful when the user has already constructed an
known-unsafe configuration, because a malicious guest in that case can
still fully mess with dom0 by sending vectors other than 0x80 and 0x82.

In particular, this option does not get activated on AMD when the user
elects to disable interrupt remapping, and I'm disinclined to wire it up
in that case too.

~Andrew

P.S. It occurs to me that FRED obsoletes the need for this anyway,
seeing as it does properly distinguish the source of an event.

Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Jan Beulich]

On 19.12.2022 07:34, Xenia Ragiadakou wrote:
> Currently, for x86 platforms, Xen does not provide to the users any
> configuration control over the IOMMU support and can only be built with
> both AMD and Intel IOMMU drivers enabled.
> However, there are use cases, e.g in safety-critical systems, that require
> Xen to be able to be configured to exclude unused code. A smaller tailored
> configuration would help Xen to meet faster certification requirements for
> individual platforms.
> 
> Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow code
> specific to each IOMMU technology to be separated and, when not required,
> stripped. AMD_IOMMU enables IOMMU support for platforms that implement the
> AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for
> platforms that implement the Intel Virtualization Technology for Directed I/O.
> 
> Since no functional change is intended regarding the default configuration
> of an x86 system, both options depend on x86 and default to 'y'.

But do things also build successfully when one or both options are disabled?
I have to say that I would be quite surprised if that worked without further
adjustments. In which case initially these options want to be prompt-less,
with the prompts only added once 'n' also works.

Jan

Re: [RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific

[Jan Beulich]

On 19.12.2022 07:34, Xenia Ragiadakou wrote:
> Move its definition to the AMD-Vi driver and use CONFIG_AMD_IOMMU
> to guard its usage in common code.
> 
> Take the opportunity to replace bool_t with bool and 1 with true.

Please further take the opportunity and use ...

> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -36,6 +36,8 @@ static struct radix_tree_root ivrs_maps;
>  LIST_HEAD_READ_MOSTLY(amd_iommu_head);
>  bool_t iommuv2_enabled;
>  
> +bool __read_mostly amd_iommu_perdev_intremap = true;

... __ro_after_init here.

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -104,7 +104,9 @@ static inline void clear_iommu_hap_pt_share(void)
>  }
>  
>  extern bool_t iommu_debug;
> -extern bool_t amd_iommu_perdev_intremap;
> +#ifdef CONFIG_AMD_IOMMU
> +extern bool amd_iommu_perdev_intremap;
> +#endif

We try to avoid such #ifdef-ary: There's no real harm from the declaration
being in scope; in the worst case the build will fail not at the compile,
but at the linking stage.

Jan

Re: [RFC 0/7] Proposal to make x86 IOMMU driver support configurable

[Xenia Ragiadakou]

Hi,

On 12/19/22 20:28, Andrew Cooper wrote:
> On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
>> This series aims to provide a means to render the iommu driver support for x86
>> configurable. Currently, irrespectively of the target platform, both AMD and
>> Intel iommu drivers are built. This is the case because the existent Kconfig
>> infrastructure does not provide any facilities for finer-grained configuration.
>>
>> The series adds two new Kconfig options, AMD_IOMMU and INTEL_VTD, that can be
>> used to generate a tailored iommu configuration for a given platform.
>>
>> This series will be part of a broader effort to separate platform specific
>> code and it is sent as an RFC to gather feedback regarding the way the
>> separation should be performed.
> 
> Hello,
> 
> Thanks for the series.
> 
>  From discussions in the past, we do want CONFIG_INTEL/AMD/etc and we do
> want these to be selectable and available for randconfig to test.
> 
> Some sub-features are more complicated, because e.g. Centaur have CPUs
> with a VT-x implementation.  This will need expressing in whatever
> Kconfig scheme we end up with.
> 

What about having configuration per cpu vendor, per virtualization 
technology and per IOMMU technology? I mean sth like [CPU_AMD, 
CPU_HYGON, CPU_INTEL, CPU_SHANGHAI, CPU_CENTAUR], [AMD_SVM, INTEL_VMX] 
and [AMD_IOMMU, INTEL_IOMMU], respectively?

> IOMMUs are more tricky still.  They are (for most intents and purposes)
> mandatory on systems with >254 CPUs.  We could in principle start
> supporting asymmetric IRQ routing on large systems, but Xen doesn't
> currently and it would be a lot work that's definitely not high on the
> priority list.  At a minimum, this will need expressing in the Kconfig
> help text.
>

Ok.

> We need to decide whether it is sensible to allow users to turn off
> IOMMU support.  It probably is, because you could trivially do this by
> selecting CONFIG_INTEL only, and booting the result on an AMD system.
> 

I cannot understand. I guess that if the code for the target system is 
disabled, Xen won't boot ... hopefully :).

If users are not allowed to turn off the IOMMU support, it can be always 
enabled unconditionally via Kconfig select based on the virtualization 
technology or other.

> 
> For the names, I don't think AMD_IOMMU vs INTEL_VTD is a sensible.
> Probably wants to be INTEL_IOMMU to match.
> 

Ok.

> ~Andrew

-- 
Xenia

Re: [RFC 0/7] Proposal to make x86 IOMMU driver support configurable

[Xenia Ragiadakou]

On 12/20/22 12:09, Jan Beulich wrote:
> On 19.12.2022 19:28, Andrew Cooper wrote:
>> IOMMUs are more tricky still.  They are (for most intents and purposes)
>> mandatory on systems with >254 CPUs.  We could in principle start
>> supporting asymmetric IRQ routing on large systems, but Xen doesn't
>> currently and it would be a lot work that's definitely not high on the
>> priority list.  At a minimum, this will need expressing in the Kconfig
>> help text.
>>
>> We need to decide whether it is sensible to allow users to turn off
>> IOMMU support.  It probably is, because you could trivially do this by
>> selecting CONFIG_INTEL only, and booting the result on an AMD system.
> 
> One other thing Andrew and I have been talking about: We probably do
> not want to tie the IOMMU vendor control to CPU vendor one. IOW we'd
> like to be able to e.g. build a hypervisor supporting Intel (only) as
> the CPU vendor, but at the same time having support for an AMD IOMMU.
> 

I totally understand. I am not aiming to tie the AMD/INTEL IOMMU support 
to any particular CPU vendor.

>> For the names, I don't think AMD_IOMMU vs INTEL_VTD is a sensible.
>> Probably wants to be INTEL_IOMMU to match.
> 
> Or simply VTD, covering the case than some other vendor comes up with a
> clone. But of course we have that same issue with "AMD" and Hygon ...
> 

I prefer INTEL_IOMMU over VTD, I think.

> Jan

-- 
Xenia

Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Xenia Ragiadakou]

On 12/20/22 12:26, Andrew Cooper wrote:
> On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
>> diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
>> index 479d7de57a..82465aa627 100644
>> --- a/xen/drivers/passthrough/Kconfig
>> +++ b/xen/drivers/passthrough/Kconfig
>> @@ -37,6 +37,22 @@ config IPMMU_VMSA
>>   
>>   endif
>>   
>> +config AMD_IOMMU
>> +	bool "AMD IOMMU"
>> +	depends on X86
>> +	default y
>> +	---help---
> 
> We're trying to phase out ---help---, so please just use help.

Ok.

> 
> ~Andrew

-- 
Xenia

Re: [RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific

[Xenia Ragiadakou]

On 12/20/22 12:31, Andrew Cooper wrote:
> On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
>> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
>> index 5e2a720d29..1a02fdc453 100644
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -58,7 +58,6 @@ bool __read_mostly iommu_hap_pt_share = true;
>>   #endif
>>   
>>   bool_t __read_mostly iommu_debug;
>> -bool_t __read_mostly amd_iommu_perdev_intremap = 1;
>>   
>>   DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>>   
>> @@ -115,8 +114,10 @@ static int __init cf_check parse_iommu_param(const char *s)
>>               if ( val )
>>                   iommu_verbose = 1;
>>           }
>> +#ifdef CONFIG_AMD_IOMMU
>>           else if ( (val = parse_boolean("amd-iommu-perdev-intremap", s, ss)) >= 0 )
>>               amd_iommu_perdev_intremap = val;
>> +#endif
> 
> See parse_cet() and the use of no_config_param() so users get a bit of a
> hint as to why the option they specified is getting ignored.

Ah, ok I see. Thx for pointing that out.

> 
> ~Andrew

-- 
Xenia

Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Xenia Ragiadakou]

On 12/20/22 19:01, Jan Beulich wrote:
> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>> Currently, for x86 platforms, Xen does not provide to the users any
>> configuration control over the IOMMU support and can only be built with
>> both AMD and Intel IOMMU drivers enabled.
>> However, there are use cases, e.g in safety-critical systems, that require
>> Xen to be able to be configured to exclude unused code. A smaller tailored
>> configuration would help Xen to meet faster certification requirements for
>> individual platforms.
>>
>> Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow code
>> specific to each IOMMU technology to be separated and, when not required,
>> stripped. AMD_IOMMU enables IOMMU support for platforms that implement the
>> AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for
>> platforms that implement the Intel Virtualization Technology for Directed I/O.
>>
>> Since no functional change is intended regarding the default configuration
>> of an x86 system, both options depend on x86 and default to 'y'.
> 
> But do things also build successfully when one or both options are disabled?
> I have to say that I would be quite surprised if that worked without further
> adjustments. In which case initially these options want to be prompt-less,
> with the prompts only added once 'n' also works.

Without applying the whole series, disabling any of them or both won't 
work. Ok.

> 
> Jan

-- 
Xenia

Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Andrew Cooper]

On 20/12/2022 8:57 pm, Xenia Ragiadakou wrote:
>
> On 12/20/22 19:01, Jan Beulich wrote:
>> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>>> Currently, for x86 platforms, Xen does not provide to the users any
>>> configuration control over the IOMMU support and can only be built with
>>> both AMD and Intel IOMMU drivers enabled.
>>> However, there are use cases, e.g in safety-critical systems, that
>>> require
>>> Xen to be able to be configured to exclude unused code. A smaller
>>> tailored
>>> configuration would help Xen to meet faster certification
>>> requirements for
>>> individual platforms.
>>>
>>> Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow
>>> code
>>> specific to each IOMMU technology to be separated and, when not
>>> required,
>>> stripped. AMD_IOMMU enables IOMMU support for platforms that
>>> implement the
>>> AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for
>>> platforms that implement the Intel Virtualization Technology for
>>> Directed I/O.
>>>
>>> Since no functional change is intended regarding the default
>>> configuration
>>> of an x86 system, both options depend on x86 and default to 'y'.
>>
>> But do things also build successfully when one or both options are
>> disabled?
>> I have to say that I would be quite surprised if that worked without
>> further
>> adjustments. In which case initially these options want to be
>> prompt-less,
>> with the prompts only added once 'n' also works.
>
> Without applying the whole series, disabling any of them or both won't
> work. Ok.

To do a multi-step implementation, you start with

config FOO
    bool y

then rearrange them main code to use CONFIG_FOO as appropriate, then
have a final patch that adds a Kconfig name, help text, etc which is
what makes the config option user selectable and able to be turned off.

~Andrew

Re: [RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific

[Xenia Ragiadakou]

On 12/20/22 19:04, Jan Beulich wrote:
> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>> Move its definition to the AMD-Vi driver and use CONFIG_AMD_IOMMU
>> to guard its usage in common code.
>>
>> Take the opportunity to replace bool_t with bool and 1 with true.
> 
> Please further take the opportunity and use ...
> 
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -36,6 +36,8 @@ static struct radix_tree_root ivrs_maps;
>>   LIST_HEAD_READ_MOSTLY(amd_iommu_head);
>>   bool_t iommuv2_enabled;
>>   
>> +bool __read_mostly amd_iommu_perdev_intremap = true;
> 
> ... __ro_after_init here.
> 
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -104,7 +104,9 @@ static inline void clear_iommu_hap_pt_share(void)
>>   }
>>   
>>   extern bool_t iommu_debug;
>> -extern bool_t amd_iommu_perdev_intremap;
>> +#ifdef CONFIG_AMD_IOMMU
>> +extern bool amd_iommu_perdev_intremap;
>> +#endif
> 
> We try to avoid such #ifdef-ary: There's no real harm from the declaration
> being in scope; in the worst case the build will fail not at the compile,
> but at the linking stage.

That's true. I will leave it as it is.

> 
> Jan

-- 
Xenia

Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Xenia Ragiadakou]

On 12/20/22 23:00, Andrew Cooper wrote:
> On 20/12/2022 8:57 pm, Xenia Ragiadakou wrote:
>>
>> On 12/20/22 19:01, Jan Beulich wrote:
>>> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>>>> Currently, for x86 platforms, Xen does not provide to the users any
>>>> configuration control over the IOMMU support and can only be built with
>>>> both AMD and Intel IOMMU drivers enabled.
>>>> However, there are use cases, e.g in safety-critical systems, that
>>>> require
>>>> Xen to be able to be configured to exclude unused code. A smaller
>>>> tailored
>>>> configuration would help Xen to meet faster certification
>>>> requirements for
>>>> individual platforms.
>>>>
>>>> Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow
>>>> code
>>>> specific to each IOMMU technology to be separated and, when not
>>>> required,
>>>> stripped. AMD_IOMMU enables IOMMU support for platforms that
>>>> implement the
>>>> AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for
>>>> platforms that implement the Intel Virtualization Technology for
>>>> Directed I/O.
>>>>
>>>> Since no functional change is intended regarding the default
>>>> configuration
>>>> of an x86 system, both options depend on x86 and default to 'y'.
>>>
>>> But do things also build successfully when one or both options are
>>> disabled?
>>> I have to say that I would be quite surprised if that worked without
>>> further
>>> adjustments. In which case initially these options want to be
>>> prompt-less,
>>> with the prompts only added once 'n' also works.
>>
>> Without applying the whole series, disabling any of them or both won't
>> work. Ok.
> 
> To do a multi-step implementation, you start with
> 
> config FOO
>      bool y

Here, I think you mean def_bool y

> 
> then rearrange them main code to use CONFIG_FOO as appropriate, then
> have a final patch that adds a Kconfig name, help text, etc which is
> what makes the config option user selectable and able to be turned off.

Thank you both, for pointing that out. I will fix it.

> 
> ~Andrew

-- 
Xenia

Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Jan Beulich]

On 19.12.2022 07:34, Xenia Ragiadakou wrote:
> --- a/xen/drivers/passthrough/Kconfig
> +++ b/xen/drivers/passthrough/Kconfig
> @@ -37,6 +37,22 @@ config IPMMU_VMSA
>  
>  endif
>  
> +config AMD_IOMMU
> +	bool "AMD IOMMU"
> +	depends on X86
> +	default y
> +	---help---
> +	  Enables I/O virtualization on platforms that implement the
> +	  AMD I/O Virtualization Technology (IOMMU).
> +
> +config INTEL_VTD
> +	bool "Intel VT-d"
> +	depends on X86
> +	default y
> +	---help---
> +	  Enables I/O virtualization on platforms that implement the
> +	  Intel Virtualization Technology for Directed I/O (Intel VT-d).

One more thing Andrew and I have been talking about: As he has mentioned
elsewhere, IOMMU support is needed to boot systems with more than 254
CPUs (depending on APIC ID layout the boundary may actually be lower).
Hence it needs to at least be considered to make the prompts here (to
be precise: in the much later patch adding the prompts) dependent on
EXPERT, to prevent people from unknowingly building a non-functioning
(on some systems) hypervisor.

Jan

Re: [RFC 4/7] x86/acpi: separate AMD-Vi and VT-d specific functions

[Jan Beulich]

On 19.12.2022 07:34, Xenia Ragiadakou wrote:
> The functions acpi_dmar_init() and acpi_dmar_zap/reinstate() are
> VT-d specific while the function acpi_ivrs_init() is AMD-Vi specific.
> To eliminate dead code, they need to be guarded under CONFIG_INTEL_VTD
> and CONFIG_AMD_IOMMU, respectively.
> 
> Instead of adding #ifdef guards around the function calls, implement them
> as empty static inline functions.
> 
> Take the opportunity to move the declarations of acpi_dmar_zap/reinstate() to
> the arch specific header.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [RFC 6/7] x86/iommu: call pi_update_irte through an hvm_function callback

[Jan Beulich]

On 19.12.2022 07:34, Xenia Ragiadakou wrote:
> @@ -774,6 +779,16 @@ static inline void hvm_set_nonreg_state(struct vcpu *v,
>          alternative_vcall(hvm_funcs.set_nonreg_state, v, nrs);
>  }
>  
> +static inline int hvm_pi_update_irte(const struct vcpu *v,
> +                                     const struct pirq *pirq, uint8_t gvec)

Why "int" as return type when both call sites ignore the return value?

> @@ -893,6 +908,13 @@ static inline void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>      ASSERT_UNREACHABLE();
>  }
>  
> +static inline int hvm_pi_update_irte(const struct vcpu *v,
> +                                     const struct pirq *pirq, uint8_t gvec)
> +{
> +    ASSERT_UNREACHABLE();
> +    return -EOPNOTSUPP;
> +}

I don't think you need this stub - both callers live in a file which
is built only for HVM=y anyway.

Jan

Re: [RFC 7/7] x86/dpci: move hvm_dpci_isairq_eoi() to generic HVM code

[Jan Beulich]

On 19.12.2022 07:34, Xenia Ragiadakou wrote:
> The function hvm_dpci_isairq_eoi() has no dependencies on VT-d driver code
> and can be moved from xen/drivers/passthrough/vtd/x86/hvm.c to
> xen/drivers/passthrough/x86/hvm.c.
> 
> Remove the now empty xen/drivers/passthrough/vtd/x86/hvm.c.
> 
> Since the funcion is hvm specific, move its declaration from xen/iommu.h
> to asm/hvm/io.h.

While everything else looks good here, I question this part: The function
is both HVM- and IOMMU-specific. However, by moving the definition ...

> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
> 
> I was not sure how to handle the copyright. I assume that I have to
> retain the copyright of Weidong Han <weidong.han@intel.com>, right?
> 
>  xen/arch/x86/include/asm/hvm/io.h        |  1 +
>  xen/drivers/passthrough/vtd/x86/Makefile |  1 -
>  xen/drivers/passthrough/vtd/x86/hvm.c    | 64 ------------------------
>  xen/drivers/passthrough/x86/hvm.c        | 42 ++++++++++++++++

... here, you don't need a declaration anymore anyway - the function can
simply become static then, as its only caller lives in this very file.

Jan

Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Xenia Ragiadakou]

On 12/21/22 09:51, Jan Beulich wrote:
> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>> --- a/xen/drivers/passthrough/Kconfig
>> +++ b/xen/drivers/passthrough/Kconfig
>> @@ -37,6 +37,22 @@ config IPMMU_VMSA
>>   
>>   endif
>>   
>> +config AMD_IOMMU
>> +	bool "AMD IOMMU"
>> +	depends on X86
>> +	default y
>> +	---help---
>> +	  Enables I/O virtualization on platforms that implement the
>> +	  AMD I/O Virtualization Technology (IOMMU).
>> +
>> +config INTEL_VTD
>> +	bool "Intel VT-d"
>> +	depends on X86
>> +	default y
>> +	---help---
>> +	  Enables I/O virtualization on platforms that implement the
>> +	  Intel Virtualization Technology for Directed I/O (Intel VT-d).
> 
> One more thing Andrew and I have been talking about: As he has mentioned
> elsewhere, IOMMU support is needed to boot systems with more than 254
> CPUs (depending on APIC ID layout the boundary may actually be lower).
> Hence it needs to at least be considered to make the prompts here (to
> be precise: in the much later patch adding the prompts) dependent on
> EXPERT, to prevent people from unknowingly building a non-functioning
> (on some systems) hypervisor.

I will mention it in help as Andrew suggested and I will make it visible 
only if EXPERT.

> 
> Jan

-- 
Xenia

Re: [RFC 6/7] x86/iommu: call pi_update_irte through an hvm_function callback

[Xenia Ragiadakou]

On 12/21/22 12:13, Jan Beulich wrote:
> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>> @@ -774,6 +779,16 @@ static inline void hvm_set_nonreg_state(struct vcpu *v,
>>           alternative_vcall(hvm_funcs.set_nonreg_state, v, nrs);
>>   }
>>   
>> +static inline int hvm_pi_update_irte(const struct vcpu *v,
>> +                                     const struct pirq *pirq, uint8_t gvec)
> 
> Why "int" as return type when both call sites ignore the return value?

Because the original function returned int. I 'm not sure though why the 
returned value is ignored.

> 
>> @@ -893,6 +908,13 @@ static inline void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>>       ASSERT_UNREACHABLE();
>>   }
>>   
>> +static inline int hvm_pi_update_irte(const struct vcpu *v,
>> +                                     const struct pirq *pirq, uint8_t gvec)
>> +{
>> +    ASSERT_UNREACHABLE();
>> +    return -EOPNOTSUPP;
>> +}
> 
> I don't think you need this stub - both callers live in a file which
> is built only for HVM=y anyway.

That's true. I will remove it.

> 
> Jan

-- 
Xenia

Re: [RFC 7/7] x86/dpci: move hvm_dpci_isairq_eoi() to generic HVM code

[Xenia Ragiadakou]

On 12/21/22 12:19, Jan Beulich wrote:
> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>> The function hvm_dpci_isairq_eoi() has no dependencies on VT-d driver code
>> and can be moved from xen/drivers/passthrough/vtd/x86/hvm.c to
>> xen/drivers/passthrough/x86/hvm.c.
>>
>> Remove the now empty xen/drivers/passthrough/vtd/x86/hvm.c.
>>
>> Since the funcion is hvm specific, move its declaration from xen/iommu.h
>> to asm/hvm/io.h.
> 
> While everything else looks good here, I question this part: The function
> is both HVM- and IOMMU-specific. However, by moving the definition ...
> 
>> No functional change intended.
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>> ---
>>
>> I was not sure how to handle the copyright. I assume that I have to
>> retain the copyright of Weidong Han <weidong.han@intel.com>, right?
>>
>>   xen/arch/x86/include/asm/hvm/io.h        |  1 +
>>   xen/drivers/passthrough/vtd/x86/Makefile |  1 -
>>   xen/drivers/passthrough/vtd/x86/hvm.c    | 64 ------------------------
>>   xen/drivers/passthrough/x86/hvm.c        | 42 ++++++++++++++++
> 
> ... here, you don't need a declaration anymore anyway - the function can
> simply become static then, as its only caller lives in this very file.

I will change it to static.

Regarding the copyright, shall I add the copyright of Weidong Han 
<weidong.han@intel.com>, that was in the deleted file?

> 
> Jan

-- 
Xenia

Re: [RFC 6/7] x86/iommu: call pi_update_irte through an hvm_function callback

[Jan Beulich]

On 21.12.2022 12:09, Xenia Ragiadakou wrote:
> 
> On 12/21/22 12:13, Jan Beulich wrote:
>> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>>> @@ -774,6 +779,16 @@ static inline void hvm_set_nonreg_state(struct vcpu *v,
>>>           alternative_vcall(hvm_funcs.set_nonreg_state, v, nrs);
>>>   }
>>>   
>>> +static inline int hvm_pi_update_irte(const struct vcpu *v,
>>> +                                     const struct pirq *pirq, uint8_t gvec)
>>
>> Why "int" as return type when both call sites ignore the return value?
> 
> Because the original function returned int.

Hmm, indeed - looking more closely there can actually be errors, and
those shouldn't really be ignored in all cases. At the very least an
assertion would seem on order.

> I 'm not sure though why the returned value is ignored.

Kevin, thoughts?

Jan

Re: [RFC 7/7] x86/dpci: move hvm_dpci_isairq_eoi() to generic HVM code

[Jan Beulich]

On 21.12.2022 12:13, Xenia Ragiadakou wrote:
> 
> On 12/21/22 12:19, Jan Beulich wrote:
>> On 19.12.2022 07:34, Xenia Ragiadakou wrote:
>>> The function hvm_dpci_isairq_eoi() has no dependencies on VT-d driver code
>>> and can be moved from xen/drivers/passthrough/vtd/x86/hvm.c to
>>> xen/drivers/passthrough/x86/hvm.c.
>>>
>>> Remove the now empty xen/drivers/passthrough/vtd/x86/hvm.c.
>>>
>>> Since the funcion is hvm specific, move its declaration from xen/iommu.h
>>> to asm/hvm/io.h.
>>
>> While everything else looks good here, I question this part: The function
>> is both HVM- and IOMMU-specific. However, by moving the definition ...
>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>> ---
>>>
>>> I was not sure how to handle the copyright. I assume that I have to
>>> retain the copyright of Weidong Han <weidong.han@intel.com>, right?
>>>
>>>   xen/arch/x86/include/asm/hvm/io.h        |  1 +
>>>   xen/drivers/passthrough/vtd/x86/Makefile |  1 -
>>>   xen/drivers/passthrough/vtd/x86/hvm.c    | 64 ------------------------
>>>   xen/drivers/passthrough/x86/hvm.c        | 42 ++++++++++++++++
>>
>> ... here, you don't need a declaration anymore anyway - the function can
>> simply become static then, as its only caller lives in this very file.
> 
> I will change it to static.
> 
> Regarding the copyright, shall I add the copyright of Weidong Han 
> <weidong.han@intel.com>, that was in the deleted file?

Strictly speaking you probably ought to, yes.

Jan

Re: [RFC 5/7] x86/iommu: the code addressing CVE-2011-1898 is VT-d specific

[Xenia Ragiadakou]

On 12/20/22 13:09, Andrew Cooper wrote:
> On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
>> The variable untrusted_msi indicates whether the system is vulnerable to
>> CVE-2011-1898. This vulnerablity is VT-d specific.
>> Place the code that addresses the issue under CONFIG_INTEL_VTD.
>>
>> No functional change intended.
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> 
> Actually, this variable is pretty bogus.  I think I'd like to delete it
> entirely.

Nevertheless, I don't think that it would be appropriate to be done as 
part of this series.

> 
> There are systems with no IOMMU at all, and we certainly used to let PV
> Passthrough go ahead.  (Not sure we do any more.)
> 
> There are systems with DMA remapping only, but no interrupt remapping.
> These are known insecure.  I'm honestly not convinced that an ISR read
> and crash is useful when the user has already constructed an
> known-unsafe configuration, because a malicious guest in that case can
> still fully mess with dom0 by sending vectors other than 0x80 and 0x82.
> 
> In particular, this option does not get activated on AMD when the user
> elects to disable interrupt remapping, and I'm disinclined to wire it up
> in that case too.
> 
> ~Andrew
> 
> P.S. It occurs to me that FRED obsoletes the need for this anyway,
> seeing as it does properly distinguish the source of an event.

-- 
Xenia

Re: [RFC 0/7] Proposal to make x86 IOMMU driver support configurable

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On Tue, 20 Dec 2022, Xenia Ragiadakou wrote:
> > We need to decide whether it is sensible to allow users to turn off
> > IOMMU support.  It probably is, because you could trivially do this by
> > selecting CONFIG_INTEL only, and booting the result on an AMD system.
> > 
> 
> I cannot understand. I guess that if the code for the target system is
> disabled, Xen won't boot ... hopefully :).
> 
> If users are not allowed to turn off the IOMMU support, it can be always
> enabled unconditionally via Kconfig select based on the virtualization
> technology or other.

Just wanted to say that it should be fine either way. If we don't want
to provide an option to disable the IOMMU, then it could be handled at
the kconfig level.

Re: [RFC 5/7] x86/iommu: the code addressing CVE-2011-1898 is VT-d specific

[Jan Beulich]

On 21.12.2022 16:22, Xenia Ragiadakou wrote:
> 
> On 12/20/22 13:09, Andrew Cooper wrote:
>> On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
>>> The variable untrusted_msi indicates whether the system is vulnerable to
>>> CVE-2011-1898. This vulnerablity is VT-d specific.
>>> Place the code that addresses the issue under CONFIG_INTEL_VTD.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>
>> Actually, this variable is pretty bogus.  I think I'd like to delete it
>> entirely.

The important difference between Intel and AMD was that Intel initially
supplied DMA-remap-only IOMMUs, while AMD had intremap from the beginning.
Hence Intel hardware could be unsafe by default, whereas on AMD an admin
would need to come and turn off intremap. Deleting the variable would be
okay only if we declared Xen security-unsupported on inremap-less Intel
hardware. Extending coverage to AMD wouldn't seem unreasonable to me, if
we knew that there were people turning off intremap _and_ caring about
this particular class of attack. With no-one having complained in over
10 years, perhaps there's no-one of this kind ...

> Nevertheless, I don't think that it would be appropriate to be done as 
> part of this series.

I agree, but I'll want to comment on v2 nevertheless, rather than simply
ack-ing it.

Jan