[PATCH v3 0/8] Make x86 IOMMU driver support configurable

[PATCH v3 0/8] 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_IOMMU, that can be
used to generate a tailored iommu configuration for a given platform.

This version of the series is rebased on top of the current staging and
addresses the comments made on version 2.
Patch "[v2] x86/iommu: introduce AMD-Vi and Intel VT-d Kconfig options"
is not included in this series because it has been already merged, and patch
"[v2] x86/iommu: iommu_igfx, iommu_qinval and iommu_snoop are VT-d specific"
has been splitted up into two separate patches.

Xenia Ragiadakou (8):
  x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific
  x86/iommu: iommu_igfx and iommu_qinval are Intel VT-d specific
  x86/iommu: snoop control is allowed only by Intel VT-d
  x86/acpi: separate AMD-Vi and VT-d specific functions
  x86/iommu: make code addressing CVE-2011-1898 no 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
  x86/iommu: make AMD-Vi and Intel VT-d support configurable

 xen/arch/x86/hvm/vmx/vmx.c               | 41 +++++++++++++++
 xen/arch/x86/include/asm/acpi.h          |  6 ++-
 xen/arch/x86/include/asm/hvm/hvm.h       | 10 ++++
 xen/arch/x86/include/asm/iommu.h         |  3 --
 xen/drivers/passthrough/Kconfig          | 22 +++++++-
 xen/drivers/passthrough/amd/iommu_init.c |  2 +
 xen/drivers/passthrough/iommu.c          | 15 +++++-
 xen/drivers/passthrough/vtd/intremap.c   | 36 -------------
 xen/drivers/passthrough/vtd/iommu.c      |  3 --
 xen/drivers/passthrough/vtd/x86/Makefile |  1 -
 xen/drivers/passthrough/vtd/x86/hvm.c    | 64 ------------------------
 xen/drivers/passthrough/x86/hvm.c        | 50 ++++++++++++++++--
 xen/drivers/passthrough/x86/iommu.c      |  5 ++
 xen/include/xen/acpi.h                   |  7 +++
 xen/include/xen/iommu.h                  |  8 ++-
 15 files changed, 156 insertions(+), 117 deletions(-)
 delete mode 100644 xen/drivers/passthrough/vtd/x86/hvm.c

-- 
2.37.2

[PATCH v3 1/8] 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>
---

Changes in v3:
  - the second arg of no_config_param() should have been the parameter name,
    i.e "iommu", and not the boolean suboption "amd-iommu-perdev-intremap"

 xen/drivers/passthrough/amd/iommu_init.c | 2 ++
 xen/drivers/passthrough/iommu.c          | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 1f14aaf49e..9773ccfcb4 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 __ro_after_init 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..9d95fb27d0 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);
 
@@ -116,7 +115,11 @@ static int __init cf_check parse_iommu_param(const char *s)
                 iommu_verbose = 1;
         }
         else if ( (val = parse_boolean("amd-iommu-perdev-intremap", s, ss)) >= 0 )
+#ifdef CONFIG_AMD_IOMMU
             amd_iommu_perdev_intremap = val;
+#else
+            no_config_param("AMD_IOMMU", "iommu", s, ss);
+#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 )
-- 
2.37.2

[PATCH v3 2/8] x86/iommu: iommu_igfx and iommu_qinval are Intel VT-d specific

[Xenia Ragiadakou]

Use CONFIG_INTEL_IOMMU to guard the usage of iommu_igfx and iommu_qinval
in common code.

No functional change intended.

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

Changes in v3:
  - handle iommu_snoop case in a different patch and update commit msg
  - use no_config_param() to print a warning when the user sets an INTEL_IOMMU
    specific string in the iommu boot parameter and INTEL_IOMMU is disabled

 xen/drivers/passthrough/iommu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 9d95fb27d0..b4dfa95dfd 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -82,11 +82,19 @@ 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
         else if ( (val = parse_boolean("igfx", s, ss)) >= 0 )
+#ifdef CONFIG_INTEL_IOMMU
             iommu_igfx = val;
+#else
+            no_config_param("INTEL_IOMMU", "iommu", s, ss);
+#endif
         else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
+#ifdef CONFIG_INTEL_IOMMU
             iommu_qinval = val;
+#else
+            no_config_param("INTEL_IOMMU", "iommu", s, ss);
+#endif
+#ifdef CONFIG_X86
         else if ( (val = parse_boolean("superpages", s, ss)) >= 0 )
             iommu_superpages = val;
 #endif
-- 
2.37.2

[PATCH v3 3/8] x86/iommu: snoop control is allowed only by Intel VT-d

[Xenia Ragiadakou]

The AMD-Vi driver forces coherent accesses by hardwiring the FC bit to 1.
Therefore, given that iommu_snoop is used only when the iommu is enabled,
when Xen is configured with only the AMD iommu enabled, iommu_snoop can be
reduced to a #define to true.

No functional change intended.

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

Changes in v3:
  - new patch
    This patch depends on Jan's patch "x86/shadow: sanitize iommu_snoop usage"
    to ensure that iommu_snoop is used only when the iommu is enabled

 xen/include/xen/iommu.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 4f22fc1bed..626731941b 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -74,7 +74,12 @@ extern enum __packed iommu_intremap {
    iommu_intremap_restricted,
    iommu_intremap_full,
 } iommu_intremap;
-extern bool iommu_igfx, iommu_qinval, iommu_snoop;
+extern bool iommu_igfx, iommu_qinval;
+#ifdef CONFIG_INTEL_IOMMU
+extern bool iommu_snoop;
+#else
+# define iommu_snoop true
+#endif /* CONFIG_INTEL_IOMMU */
 #else
 # define iommu_intremap false
 # define iommu_snoop false
-- 
2.37.2

[PATCH v3 4/8] 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_IOMMU
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 declaration of acpi_dmar_init from the
x86 arch-specific header to the common header, since Intel VT-d has been
also used on IA-64 platforms.

No functional change intended.

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

Changes in v3:
  - move the declarations of Intel VT-d functions to the common header,
    because Intel VT-d has been also used on IA-64 platforms, and update
    the commit log accordingly

 xen/arch/x86/include/asm/acpi.h | 6 +++++-
 xen/include/xen/acpi.h          | 7 +++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/acpi.h b/xen/arch/x86/include/asm/acpi.h
index c453450a74..6ce79ce465 100644
--- a/xen/arch/x86/include/asm/acpi.h
+++ b/xen/arch/x86/include/asm/acpi.h
@@ -140,8 +140,12 @@ extern u32 pmtmr_ioport;
 extern unsigned int pmtmr_width;
 
 void acpi_iommu_init(void);
-int acpi_dmar_init(void);
+
+#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..352f27f6a7 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -206,8 +206,15 @@ static inline int acpi_get_pxm(acpi_handle handle)
 
 void acpi_reboot(void);
 
+#ifdef CONFIG_INTEL_IOMMU
+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
 
 #endif /* __ASSEMBLY__ */
 
-- 
2.37.2

[PATCH v3 5/8] x86/iommu: make code addressing CVE-2011-1898 no VT-d specific

[Xenia Ragiadakou]

The variable untrusted_msi indicates whether the system is vulnerable to
CVE-2011-1898 due to the absence of interrupt remapping  support.
AMD iommus with interrupt remapping disabled are also exposed.
Therefore move the definition of the variable to the common x86 iommu code.

Also, since the current implementation assumes that only PV guests are prone
to this attack, take the opportunity to define untrusted_msi only when PV is
enabled.

No functional change intended.

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

Changes in v3:
  - change untrusted_msi from being VT-d specific to PV specific and
    update commit log accordingly
  - remove unnecessary #ifdef guard from its declaration

 xen/drivers/passthrough/vtd/iommu.c | 3 ---
 xen/drivers/passthrough/x86/iommu.c | 5 +++++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 62e143125d..dae2426cb9 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -54,9 +54,6 @@
                                  ? dom_iommu(d)->arch.vtd.pgd_maddr \
                                  : (pdev)->arch.vtd.pgd_maddr)
 
-/* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
-bool __read_mostly untrusted_msi;
-
 bool __read_mostly iommu_igfx = true;
 bool __read_mostly iommu_qinval = true;
 #ifndef iommu_snoop
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index f671b0f2bb..c5021ea023 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -36,6 +36,11 @@ bool __initdata iommu_superpages = true;
 
 enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
 
+#ifdef CONFIG_PV
+/* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
+bool __read_mostly untrusted_msi;
+#endif
+
 #ifndef iommu_intpost
 /*
  * In the current implementation of VT-d posted interrupts, in some extreme
-- 
2.37.2

[PATCH v3 6/8] 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.

Since the posted interrupt descriptor is Intel VT-x specific while
msi_msg_write_remap_rte is iommu specific, open code pi_update_irte() inside
vmx_pi_update_irte() but replace msi_msg_write_remap_rte() with generic
iommu_update_ire_from_msi(). That way vmx_pi_update_irte() is not bound to
Intel VT-d anymore.

Remove the now unused pi_update_irte() implementation.

No functional change intended.

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

Changes in v3:
  - open code pi_update_irte() in vmx_pi_update_irte() but instead of using
    the VT-d specific function msi_msg_write_remap_rte() use the generic
    iommu_update_ire_from_msi()
  - delete pi_update_irte() from vtd/intremap.c
  - move the initialization of vmx pi_update_irte stub to start_vmx() and
    perform it only if iommu_intpost is enabled.
  - move pi_update_irte right after handle_eoi

 xen/arch/x86/hvm/vmx/vmx.c             | 41 ++++++++++++++++++++++++++
 xen/arch/x86/include/asm/hvm/hvm.h     | 10 +++++++
 xen/arch/x86/include/asm/iommu.h       |  3 --
 xen/drivers/passthrough/vtd/intremap.c | 36 ----------------------
 xen/drivers/passthrough/x86/hvm.c      |  5 ++--
 5 files changed, 53 insertions(+), 42 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 15a07933ee..a5355dbac2 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -396,6 +396,43 @@ void vmx_pi_hooks_deassign(struct domain *d)
     domain_unpause(d);
 }
 
+/*
+ * This function is used to update the IRTE for posted-interrupt
+ * when guest changes MSI/MSI-X information.
+ */
+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;
+    struct irq_desc *desc;
+    struct msi_desc *msi_desc;
+    int rc;
+
+    desc = pirq_spin_lock_irq_desc(pirq, NULL);
+    if ( !desc )
+        return -EINVAL;
+
+    msi_desc = desc->msi_desc;
+    if ( !msi_desc )
+    {
+        rc = -ENODEV;
+        goto unlock_out;
+    }
+    msi_desc->pi_desc = pi_desc;
+    msi_desc->gvec = gvec;
+
+    spin_unlock_irq(&desc->lock);
+
+    ASSERT(pcidevs_locked());
+
+    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
+
+ unlock_out:
+    spin_unlock_irq(&desc->lock);
+
+    return rc;
+}
+
 static const struct lbr_info {
     u32 base, count;
 } p4_lbr[] = {
@@ -2969,8 +3006,12 @@ const struct hvm_function_table * __init start_vmx(void)
     {
         alloc_direct_apic_vector(&posted_intr_vector, pi_notification_interrupt);
         if ( iommu_intpost )
+        {
             alloc_direct_apic_vector(&pi_wakeup_vector, pi_wakeup_interrupt);
 
+            vmx_function_table.pi_update_irte = vmx_pi_update_irte;
+        }
+
         vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
         vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
         vmx_function_table.test_pir            = vmx_test_pir;
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 93254651f2..b1990a047c 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;
@@ -213,6 +215,8 @@ struct hvm_function_table {
     void (*sync_pir_to_irr)(struct vcpu *v);
     bool (*test_pir)(const struct vcpu *v, uint8_t vector);
     void (*handle_eoi)(uint8_t vector, int isr);
+    int (*pi_update_irte)(const struct vcpu *v, const struct pirq *pirq,
+                          uint8_t gvec);
 
     /*Walk nested p2m  */
     int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,
@@ -774,6 +778,12 @@ 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)
+{
+    return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec);
+}
+
 #else  /* CONFIG_HVM */
 
 #define hvm_enabled false
diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index fc0afe35bf..4794e72cf1 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -129,9 +129,6 @@ void iommu_identity_map_teardown(struct domain *d);
 
 extern bool untrusted_msi;
 
-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/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index 1512e4866b..b39bc83282 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -866,39 +866,3 @@ void cf_check intel_iommu_disable_eim(void)
     for_each_drhd_unit ( drhd )
         disable_qinval(drhd->iommu);
 }
-
-/*
- * This function is used to update the IRTE for posted-interrupt
- * when guest changes MSI/MSI-X information.
- */
-int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
-    const uint8_t gvec)
-{
-    struct irq_desc *desc;
-    struct msi_desc *msi_desc;
-    int rc;
-
-    desc = pirq_spin_lock_irq_desc(pirq, NULL);
-    if ( !desc )
-        return -EINVAL;
-
-    msi_desc = desc->msi_desc;
-    if ( !msi_desc )
-    {
-        rc = -ENODEV;
-        goto unlock_out;
-    }
-    msi_desc->pi_desc = pi_desc;
-    msi_desc->gvec = gvec;
-
-    spin_unlock_irq(&desc->lock);
-
-    ASSERT(pcidevs_locked());
-
-    return msi_msg_write_remap_rte(msi_desc, &msi_desc->msg);
-
- unlock_out:
-    spin_unlock_irq(&desc->lock);
-
-    return rc;
-}
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

[PATCH v3 7/8] 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, along with the corresponding copyrights.

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

Since the function is used only in this file, declare it static.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---

Changes in v3:
  - in _hvm_dpci_isairq_eoi(), make hvm_irq pointer to const
  - in hvm_dpci_isairq_eoi(), make dpci pointer to const, add a blank line
    after ASSERT(isairq < NR_ISAIRQS), add a blank line before write_unlock()
  - fix typo in commit log, s/funcion/function/
  - add Jan's Reviewed-by tag

 xen/drivers/passthrough/vtd/x86/Makefile |  1 -
 xen/drivers/passthrough/vtd/x86/hvm.c    | 64 ------------------------
 xen/drivers/passthrough/x86/hvm.c        | 45 +++++++++++++++++
 xen/include/xen/iommu.h                  |  1 -
 4 files changed, 45 insertions(+), 66 deletions(-)
 delete mode 100644 xen/drivers/passthrough/vtd/x86/hvm.c

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..6bbd04bf3d 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -14,6 +14,7 @@
  * 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>
  * Copyright (C) Xiaohui Xin <xiaohui.xin@intel.com>
  */
 
@@ -924,6 +925,50 @@ 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)
+{
+    const 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;
+}
+
+static void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq)
+{
+    const 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 626731941b..405db59971 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -201,7 +201,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

[PATCH v3 8/8] x86/iommu: make AMD-Vi and Intel VT-d support configurable

[Xenia Ragiadakou]

Provide the user with configuration control over the IOMMU support by making
AMD_IOMMU and INTEL_IOMMU options user selectable and able to be turned off.

However, there are cases where the IOMMU support is required, for instance for
a system with more than 254 CPUs. In order to prevent users from unknowingly
disabling it and ending up with a broken hypervisor, make the support user
selectable only if EXPERT is enabled.

To preserve the current default configuration of an x86 system, both options
depend on X86 and default to Y.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---

Changes in v3:
  - add Jan's Acked-by tag

 xen/drivers/passthrough/Kconfig | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
index 5c65567744..864fcf3b0c 100644
--- a/xen/drivers/passthrough/Kconfig
+++ b/xen/drivers/passthrough/Kconfig
@@ -38,10 +38,28 @@ config IPMMU_VMSA
 endif
 
 config AMD_IOMMU
-	def_bool y if X86
+	bool "AMD IOMMU" if EXPERT
+	depends on X86
+	default y
+	help
+	  Enables I/O virtualization on platforms that implement the
+	  AMD I/O Virtualization Technology (IOMMU).
+
+	  If your system includes an IOMMU implementing AMD-Vi, say Y.
+	  This is required if your system has more than 254 CPUs.
+	  If in doubt, say Y.
 
 config INTEL_IOMMU
-	def_bool y if X86
+	bool "Intel VT-d" if EXPERT
+	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).
+
+	  If your system includes an IOMMU implementing Intel VT-d, say Y.
+	  This is required if your system has more than 254 CPUs.
+	  If in doubt, say Y.
 
 config IOMMU_FORCE_PT_SHARE
 	bool
-- 
2.37.2

Re: [PATCH v3 5/8] x86/iommu: make code addressing CVE-2011-1898 no VT-d specific

[Xenia Ragiadakou]

On 1/16/23 09:04, Xenia Ragiadakou wrote:
> The variable untrusted_msi indicates whether the system is vulnerable to
> CVE-2011-1898 due to the absence of interrupt remapping  support.
> AMD iommus with interrupt remapping disabled are also exposed.
> Therefore move the definition of the variable to the common x86 iommu code.
> 
> Also, since the current implementation assumes that only PV guests are prone
> to this attack, take the opportunity to define untrusted_msi only when PV is
> enabled.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
> 
> Changes in v3:
>    - change untrusted_msi from being VT-d specific to PV specific and
>      update commit log accordingly
>    - remove unnecessary #ifdef guard from its declaration
> 
>   xen/drivers/passthrough/vtd/iommu.c | 3 ---
>   xen/drivers/passthrough/x86/iommu.c | 5 +++++
>   2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 62e143125d..dae2426cb9 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -54,9 +54,6 @@
>                                    ? dom_iommu(d)->arch.vtd.pgd_maddr \
>                                    : (pdev)->arch.vtd.pgd_maddr)
>   
> -/* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
> -bool __read_mostly untrusted_msi;
> -
>   bool __read_mostly iommu_igfx = true;
>   bool __read_mostly iommu_qinval = true;
>   #ifndef iommu_snoop
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index f671b0f2bb..c5021ea023 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -36,6 +36,11 @@ bool __initdata iommu_superpages = true;
>   
>   enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
>   
> +#ifdef CONFIG_PV
> +/* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
> +bool __read_mostly untrusted_msi;
> +#endif
> +
>   #ifndef iommu_intpost
>   /*
>    * In the current implementation of VT-d posted interrupts, in some extreme

Here, somehow I missed the part:
diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index dae2426cb9..e97b1fe8cd 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2767,6 +2767,7 @@ static int cf_check reassign_device_ownership(
          if ( !has_arch_pdevs(target) )
              vmx_pi_hooks_assign(target);

+#ifdef CONFIG_PV
          /*
           * Devices assigned to untrusted domains (here assumed to be 
any domU)
           * can attempt to send arbitrary LAPIC/MSI messages. We are 
unprotected
@@ -2775,6 +2776,7 @@ static int cf_check reassign_device_ownership(
          if ( !iommu_intremap && !is_hardware_domain(target) &&
               !is_system_domain(target) )
              untrusted_msi = true;
+#endif

          ret = domain_context_mapping(target, devfn, pdev);

I will fix it in the next version.

-- 
Xenia

Re: [PATCH v3 6/8] x86/iommu: call pi_update_irte through an hvm_function callback

[Xenia Ragiadakou]

On 1/16/23 09:04, Xenia Ragiadakou wrote:
> 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.
> 
> Since the posted interrupt descriptor is Intel VT-x specific while
> msi_msg_write_remap_rte is iommu specific, open code pi_update_irte() inside
> vmx_pi_update_irte() but replace msi_msg_write_remap_rte() with generic
> iommu_update_ire_from_msi(). That way vmx_pi_update_irte() is not bound to
> Intel VT-d anymore.
> 
> Remove the now unused pi_update_irte() implementation.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
> 
> Changes in v3:
>    - open code pi_update_irte() in vmx_pi_update_irte() but instead of using
>      the VT-d specific function msi_msg_write_remap_rte() use the generic
>      iommu_update_ire_from_msi()
>    - delete pi_update_irte() from vtd/intremap.c
>    - move the initialization of vmx pi_update_irte stub to start_vmx() and
>      perform it only if iommu_intpost is enabled.
>    - move pi_update_irte right after handle_eoi
> 
>   xen/arch/x86/hvm/vmx/vmx.c             | 41 ++++++++++++++++++++++++++
>   xen/arch/x86/include/asm/hvm/hvm.h     | 10 +++++++
>   xen/arch/x86/include/asm/iommu.h       |  3 --
>   xen/drivers/passthrough/vtd/intremap.c | 36 ----------------------
>   xen/drivers/passthrough/x86/hvm.c      |  5 ++--
>   5 files changed, 53 insertions(+), 42 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 15a07933ee..a5355dbac2 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -396,6 +396,43 @@ void vmx_pi_hooks_deassign(struct domain *d)
>       domain_unpause(d);
>   }
>   
> +/*
> + * This function is used to update the IRTE for posted-interrupt
> + * when guest changes MSI/MSI-X information.
> + */
> +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;
> +    struct irq_desc *desc;
> +    struct msi_desc *msi_desc;
> +    int rc;
> +
> +    desc = pirq_spin_lock_irq_desc(pirq, NULL);
> +    if ( !desc )
> +        return -EINVAL;
> +
> +    msi_desc = desc->msi_desc;
> +    if ( !msi_desc )
> +    {
> +        rc = -ENODEV;
> +        goto unlock_out;
> +    }
> +    msi_desc->pi_desc = pi_desc;
> +    msi_desc->gvec = gvec;
> +
> +    spin_unlock_irq(&desc->lock);
> +
> +    ASSERT(pcidevs_locked());
> +
> +    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
> +
> + unlock_out:
> +    spin_unlock_irq(&desc->lock);
> +
> +    return rc;
> +}
> +
>   static const struct lbr_info {
>       u32 base, count;
>   } p4_lbr[] = {
> @@ -2969,8 +3006,12 @@ const struct hvm_function_table * __init start_vmx(void)
>       {
>           alloc_direct_apic_vector(&posted_intr_vector, pi_notification_interrupt);
>           if ( iommu_intpost )
> +        {
>               alloc_direct_apic_vector(&pi_wakeup_vector, pi_wakeup_interrupt);
>   
> +            vmx_function_table.pi_update_irte = vmx_pi_update_irte;
> +        }
> +
>           vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
>           vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
>           vmx_function_table.test_pir            = vmx_test_pir;
> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
> index 93254651f2..b1990a047c 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;
> @@ -213,6 +215,8 @@ struct hvm_function_table {
>       void (*sync_pir_to_irr)(struct vcpu *v);
>       bool (*test_pir)(const struct vcpu *v, uint8_t vector);
>       void (*handle_eoi)(uint8_t vector, int isr);
> +    int (*pi_update_irte)(const struct vcpu *v, const struct pirq *pirq,
> +                          uint8_t gvec);
>   
>       /*Walk nested p2m  */
>       int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,
> @@ -774,6 +778,12 @@ 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)
> +{
> +    return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec);
> +}
> +
>   #else  /* CONFIG_HVM */
>   
>   #define hvm_enabled false
> diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
> index fc0afe35bf..4794e72cf1 100644
> --- a/xen/arch/x86/include/asm/iommu.h
> +++ b/xen/arch/x86/include/asm/iommu.h
> @@ -129,9 +129,6 @@ void iommu_identity_map_teardown(struct domain *d);
>   
>   extern bool untrusted_msi;
>   
> -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)

Here, I forgot to remove the #include <asm/hvm/vmx/vmcs.h>. I will fix 
it in the next version.

> diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
> index 1512e4866b..b39bc83282 100644
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -866,39 +866,3 @@ void cf_check intel_iommu_disable_eim(void)
>       for_each_drhd_unit ( drhd )
>           disable_qinval(drhd->iommu);
>   }
> -
> -/*
> - * This function is used to update the IRTE for posted-interrupt
> - * when guest changes MSI/MSI-X information.
> - */
> -int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
> -    const uint8_t gvec)
> -{
> -    struct irq_desc *desc;
> -    struct msi_desc *msi_desc;
> -    int rc;
> -
> -    desc = pirq_spin_lock_irq_desc(pirq, NULL);
> -    if ( !desc )
> -        return -EINVAL;
> -
> -    msi_desc = desc->msi_desc;
> -    if ( !msi_desc )
> -    {
> -        rc = -ENODEV;
> -        goto unlock_out;
> -    }
> -    msi_desc->pi_desc = pi_desc;
> -    msi_desc->gvec = gvec;
> -
> -    spin_unlock_irq(&desc->lock);
> -
> -    ASSERT(pcidevs_locked());
> -
> -    return msi_msg_write_remap_rte(msi_desc, &msi_desc->msg);
> -
> - unlock_out:
> -    spin_unlock_irq(&desc->lock);
> -
> -    return rc;
> -}
> 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) )

-- 
Xenia

Re: [PATCH v3 1/8] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific

[Jan Beulich]

On 16.01.2023 08:04, 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.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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

Re: [PATCH v3 2/8] x86/iommu: iommu_igfx and iommu_qinval are Intel VT-d specific

[Jan Beulich]

On 16.01.2023 08:04, Xenia Ragiadakou wrote:
> Use CONFIG_INTEL_IOMMU to guard the usage of iommu_igfx and iommu_qinval
> in common code.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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

Re: [PATCH v3 3/8] x86/iommu: snoop control is allowed only by Intel VT-d

[Jan Beulich]

On 16.01.2023 08:04, Xenia Ragiadakou wrote:
> The AMD-Vi driver forces coherent accesses by hardwiring the FC bit to 1.
> Therefore, given that iommu_snoop is used only when the iommu is enabled,
> when Xen is configured with only the AMD iommu enabled, iommu_snoop can be
> reduced to a #define to true.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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

Re: [PATCH v3 4/8] x86/acpi: separate AMD-Vi and VT-d specific functions

[Jan Beulich]

On 16.01.2023 08:04, 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_IOMMU
> 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 declaration of acpi_dmar_init from the
> x86 arch-specific header to the common header, since Intel VT-d has been
> also used on IA-64 platforms.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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

Re: [PATCH v3 5/8] x86/iommu: make code addressing CVE-2011-1898 no VT-d specific

[Jan Beulich]

On 16.01.2023 08:21, Xenia Ragiadakou wrote:
> On 1/16/23 09:04, Xenia Ragiadakou wrote:
>> The variable untrusted_msi indicates whether the system is vulnerable to
>> CVE-2011-1898 due to the absence of interrupt remapping  support.
>> AMD iommus with interrupt remapping disabled are also exposed.

It would probably help if you mentioned here explicitly that, while affected,
we don't handle that yet (the code setting the flag would either need to
move out of VT-d specific space, or be cloned accordingly).

>> Therefore move the definition of the variable to the common x86 iommu code.
>>
>> Also, since the current implementation assumes that only PV guests are prone
>> to this attack, take the opportunity to define untrusted_msi only when PV is
>> enabled.
>>
>> No functional change intended.
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>[...]
> 
> Here, somehow I missed the part:
> diff --git a/xen/drivers/passthrough/vtd/iommu.c 
> b/xen/drivers/passthrough/vtd/iommu.c
> index dae2426cb9..e97b1fe8cd 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2767,6 +2767,7 @@ static int cf_check reassign_device_ownership(
>           if ( !has_arch_pdevs(target) )
>               vmx_pi_hooks_assign(target);
> 
> +#ifdef CONFIG_PV
>           /*
>            * Devices assigned to untrusted domains (here assumed to be 
> any domU)
>            * can attempt to send arbitrary LAPIC/MSI messages. We are 
> unprotected
> @@ -2775,6 +2776,7 @@ static int cf_check reassign_device_ownership(
>           if ( !iommu_intremap && !is_hardware_domain(target) &&
>                !is_system_domain(target) )
>               untrusted_msi = true;
> +#endif
> 
>           ret = domain_context_mapping(target, devfn, pdev);
> 
> I will fix it in the next version.
> 

With that added (and preferably the description clarified)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v3 5/8] x86/iommu: make code addressing CVE-2011-1898 no VT-d specific

[Xenia Ragiadakou]

On 1/16/23 18:49, Jan Beulich wrote:
> On 16.01.2023 08:21, Xenia Ragiadakou wrote:
>> On 1/16/23 09:04, Xenia Ragiadakou wrote:
>>> The variable untrusted_msi indicates whether the system is vulnerable to
>>> CVE-2011-1898 due to the absence of interrupt remapping  support.
>>> AMD iommus with interrupt remapping disabled are also exposed.
> 
> It would probably help if you mentioned here explicitly that, while affected,
> we don't handle that yet (the code setting the flag would either need to
> move out of VT-d specific space, or be cloned accordingly).

Sure. I will update the comment.

> 
>>> Therefore move the definition of the variable to the common x86 iommu code.
>>>
>>> Also, since the current implementation assumes that only PV guests are prone
>>> to this attack, take the opportunity to define untrusted_msi only when PV is
>>> enabled.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>> [...]
>>
>> Here, somehow I missed the part:
>> diff --git a/xen/drivers/passthrough/vtd/iommu.c
>> b/xen/drivers/passthrough/vtd/iommu.c
>> index dae2426cb9..e97b1fe8cd 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -2767,6 +2767,7 @@ static int cf_check reassign_device_ownership(
>>            if ( !has_arch_pdevs(target) )
>>                vmx_pi_hooks_assign(target);
>>
>> +#ifdef CONFIG_PV
>>            /*
>>             * Devices assigned to untrusted domains (here assumed to be
>> any domU)
>>             * can attempt to send arbitrary LAPIC/MSI messages. We are
>> unprotected
>> @@ -2775,6 +2776,7 @@ static int cf_check reassign_device_ownership(
>>            if ( !iommu_intremap && !is_hardware_domain(target) &&
>>                 !is_system_domain(target) )
>>                untrusted_msi = true;
>> +#endif
>>
>>            ret = domain_context_mapping(target, devfn, pdev);
>>
>> I will fix it in the next version.
>>
> 
> With that added (and preferably the description clarified)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Jan

-- 
Xenia