Re: [XEN PATCH] x86/iommu: Conditionally compile platform-specific union entries
Le 23/05/2024 à 11:52, Roger Pau Monné a écrit :
> The #ifdef and #endif processor directives shouldn't be indented.
>
> Would you mind adding /* CONFIG_{AMD,INTEL}_IOMMU */ comments in the
> #endif directives?
>
Sure, will change it for v2.
> I wonder if we could move the definitions of those structures to the
> vendor specific headers, but that's more convoluted, and would require
> including the iommu headers in pci.h
Do you mean moving the vtd/amd union entries to separate structures (e.g
vtd_arch_iommu) and put them into another file (I don't see any
vendor-specific headers for this, perhaps create ones ?).
>
> Thanks, Roger.
Teddy
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH] x86/iommu: Conditionally compile platform-specific union entries
If some platform driver isn't compiled in, remove its related union
entries as they are not used.
Signed-off-by Teddy Astie
---
xen/arch/x86/include/asm/iommu.h | 4
xen/arch/x86/include/asm/pci.h | 4
2 files changed, 8 insertions(+)
diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index 8dc464fbd3..99180940c4 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -42,17 +42,21 @@ struct arch_iommu
struct list_head identity_maps;
union {
+#ifdef CONFIG_INTEL_IOMMU
/* Intel VT-d */
struct {
uint64_t pgd_maddr; /* io page directory machine address */
unsigned int agaw; /* adjusted guest address width, 0 is level 2
30-bit */
unsigned long *iommu_bitmap; /* bitmap of iommu(s) that the domain
uses */
} vtd;
+#endif
+#ifdef CONFIG_AMD_IOMMU
/* AMD IOMMU */
struct {
unsigned int paging_mode;
struct page_info *root_table;
} amd;
+#endif
};
};
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index fd5480d67d..842710f0dc 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -22,12 +22,16 @@ struct arch_pci_dev {
*/
union {
/* Subset of struct arch_iommu's fields, to be used in dom_io. */
+#ifdef CONFIG_INTEL_IOMMU
struct {
uint64_t pgd_maddr;
} vtd;
+#endif
+#ifdef CONFIG_AMD_IOMMU
struct {
struct page_info *root_table;
} amd;
+#endif
};
domid_t pseudo_domid;
mfn_t leaf_mfn;
--
2.45.1
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
Re: [XEN PATCH 2/2] x86/msr: add suffix 'U' to MSR_AMD_CSTATE_CFG macro.
Le 24/04/2024 à 14:11, Alessandro Zucchelli a écrit : > This addresses violations of MISRA C:2012 Rule 7.2 which states as > following: A “u” or “U” suffix shall be applied to all integer constants > that are represented in an unsigned type. > > No functional change. > > Signed-off-by: Alessandro Zucchelli > --- > xen/arch/x86/include/asm/msr-index.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/x86/include/asm/msr-index.h > b/xen/arch/x86/include/asm/msr-index.h > index 92dd9fa496..9cdb5b2625 100644 > --- a/xen/arch/x86/include/asm/msr-index.h > +++ b/xen/arch/x86/include/asm/msr-index.h > @@ -236,7 +236,7 @@ > > #define MSR_VIRT_SPEC_CTRL _AC(0xc001011f, U) /* Layout > matches MSR_SPEC_CTRL */ > > -#define MSR_AMD_CSTATE_CFG 0xc0010296 > +#define MSR_AMD_CSTATE_CFG 0xc0010296U > > /* >* Legacy MSR constants in need of cleanup. No new MSRs below this comment. Hello, thanks for the patches I wonder if the same approach should be also taken for all the other MSR constants of this file that are similar ? Teddy Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[RFC XEN PATCH v1] xen/public: Add initial files for PV-IOMMU
g these informations, the Dom0 may decide whether to use or not
+the PV-IOMMU interface.
+
+## Page pool for contexts
+
+In order to prevent unexpected starving on the hypervisor memory with a
+buggy Dom0. We can preallocate the pages the contexts will use and make
+map/unmap use these pages instead of allocating them dynamically.
+
diff --git a/xen/include/public/pv-iommu.h b/xen/include/public/pv-iommu.h
new file mode 100644
index 00..45f9c44eb1
--- /dev/null
+++ b/xen/include/public/pv-iommu.h
@@ -0,0 +1,114 @@
+/* SPDX-License-Identifier: MIT */
+/**
+ * pv-iommu.h
+ *
+ * Paravirtualized IOMMU driver interface.
+ *
+ * Copyright (c) 2024 Teddy Astie
+ */
+
+#ifndef __XEN_PUBLIC_PV_IOMMU_H__
+#define __XEN_PUBLIC_PV_IOMMU_H__
+
+#include "xen.h"
+#include "physdev.h"
+
+#define IOMMU_DEFAULT_CONTEXT (0)
+
+/**
+ * Query PV-IOMMU capabilities for this domain.
+ */
+#define IOMMUOP_query_capabilities1
+
+/**
+ * Allocate an IOMMU context, the new context handle will be written to ctx_no.
+ */
+#define IOMMUOP_alloc_context 2
+
+/**
+ * Destroy a IOMMU context.
+ * All devices attached to this context are reattached to default context.
+ *
+ * The default context can't be destroyed (0).
+ */
+#define IOMMUOP_free_context 3
+
+/**
+ * Reattach the device to IOMMU context.
+ */
+#define IOMMUOP_reattach_device 4
+
+#define IOMMUOP_map_pages 5
+#define IOMMUOP_unmap_pages 6
+
+/**
+ * Get the GFN associated to a specific DFN.
+ */
+#define IOMMUOP_lookup_page 7
+
+struct pv_iommu_op {
+uint16_t subop_id;
+uint16_t ctx_no;
+
+/**
+ * Create a context that is cloned from default.
+ * The new context will be populated with 1:1 mappings covering the entire
guest memory.
+ */
+#define IOMMU_CREATE_clone (1 << 0)
+
+#define IOMMU_OP_readable (1 << 0)
+#define IOMMU_OP_writeable (1 << 1)
+uint32_t flags;
+
+union {
+struct {
+uint64_t gfn;
+uint64_t dfn;
+/* Number of pages to map */
+uint32_t nr_pages;
+/* Number of pages actually mapped after sub-op */
+uint32_t mapped;
+} map_pages;
+
+struct {
+uint64_t dfn;
+/* Number of pages to unmap */
+uint32_t nr_pages;
+/* Number of pages actually unmapped after sub-op */
+uint32_t unmapped;
+} unmap_pages;
+
+struct {
+struct physdev_pci_device dev;
+} reattach_device;
+
+struct {
+uint64_t gfn;
+uint64_t dfn;
+} lookup_page;
+
+struct {
+/* Maximum number of IOMMU context this domain can use. */
+uint16_t max_ctx_no;
+/* Maximum number of pages that can be modified in a single
map/unmap operation. */
+uint32_t max_nr_pages;
+/* Maximum device address (iova) that the guest can use for
mappings. */
+uint64_t max_iova_addr;
+} cap;
+};
+};
+
+typedef struct pv_iommu_op pv_iommu_op_t;
+DEFINE_XEN_GUEST_HANDLE(pv_iommu_op_t);
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
\ No newline at end of file
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[RFC XEN v1] docs/designs: Introduce IOMMU context management
+- on the default context, reuse the domain_id (the default context is unique per domain) +- on non-default context, use a id allocated in the pseudo_domid map, (actually used by +quarantine) which is a DID outside of Xen domain_id range + +### AMD-Vi + +TODO + +## Device-tree platforms + +### SMMU and SMMUv3 + +TODO + +* * * + +[1] See pv-iommu.md + +[2] pci: phantom functions assigned to incorrect contexts +https://xenbits.xen.org/xsa/advisory-449.html -- 2.44.0 Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[XEN PATCH v5 3/3] x86/iommu: Disable intrerrupt remapping if cx16 is not supported
All hardware with VT-d/AMD-Vi has CMPXCHG16B support. Check this at
initialisation time, and remove the effectively-dead logic for the non-cx16
case.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/amd/iommu_intr.c | 6 ++
xen/drivers/passthrough/vtd/intremap.c | 70 +++-
xen/drivers/passthrough/vtd/iommu.c | 7 +--
3 files changed, 27 insertions(+), 56 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c
b/xen/drivers/passthrough/amd/iommu_intr.c
index 7fc796dec2..9ab7c68749 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -649,6 +649,12 @@ bool __init cf_check iov_supports_xt(void)
if ( !iommu_enable || !iommu_intremap )
return false;
+if ( unlikely(!cpu_has_cx16) )
+{
+AMD_IOMMU_WARN("CPU doesn't support CMPXCHG16B, disable interrupt
remapping\n");
+return false;
+}
+
if ( amd_iommu_prepare(true) )
return false;
diff --git a/xen/drivers/passthrough/vtd/intremap.c
b/xen/drivers/passthrough/vtd/intremap.c
index c504852eb8..7d4d907b4f 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -150,6 +150,13 @@ bool __init cf_check intel_iommu_supports_eim(void)
if ( !iommu_qinval || !iommu_intremap || list_empty(_drhd_units) )
return false;
+if ( unlikely(!cpu_has_cx16) )
+{
+printk(XENLOG_WARNING VTDPREFIX
+ "CPU doesn't support CMPXCHG16B, disable interrupt
remapping\n");
+return false;
+}
+
/* We MUST have a DRHD unit for each IOAPIC. */
for ( apic = 0; apic < nr_ioapics; apic++ )
if ( !ioapic_to_drhd(IO_APIC_ID(apic)) )
@@ -173,47 +180,26 @@ bool __init cf_check intel_iommu_supports_eim(void)
* Assume iremap_lock has been acquired. It is to make sure software will not
* change the same IRTE behind us. With this assumption, if only high qword or
* low qword in IRTE is to be updated, this function's atomic variant can
- * present an atomic update to VT-d hardware even when cmpxchg16b
- * instruction is not supported.
+ * present an atomic update to VT-d hardware.
*/
static void update_irte(struct vtd_iommu *iommu, struct iremap_entry *entry,
const struct iremap_entry *new_ire, bool atomic)
{
-ASSERT(spin_is_locked(>intremap.lock));
+__uint128_t ret;
+struct iremap_entry old_ire;
-if ( cpu_has_cx16 )
-{
-__uint128_t ret;
-struct iremap_entry old_ire;
+ASSERT(spin_is_locked(>intremap.lock));
-old_ire = *entry;
-ret = cmpxchg16b(entry, _ire, new_ire);
+old_ire = *entry;
+ret = cmpxchg16b(entry, _ire, new_ire);
-/*
- * In the above, we use cmpxchg16 to atomically update the 128-bit
- * IRTE, and the hardware cannot update the IRTE behind us, so
- * the return value of cmpxchg16 should be the same as old_ire.
- * This ASSERT validate it.
- */
-ASSERT(ret == old_ire.val);
-}
-else
-{
-/*
- * VT-d hardware doesn't update IRTEs behind us, nor the software
- * since we hold iremap_lock. If the caller wants VT-d hardware to
- * always see a consistent entry, but we can't meet it, a bug will
- * be raised.
- */
-if ( entry->lo == new_ire->lo )
-write_atomic(>hi, new_ire->hi);
-else if ( entry->hi == new_ire->hi )
-write_atomic(>lo, new_ire->lo);
-else if ( !atomic )
-*entry = *new_ire;
-else
-BUG();
-}
+/*
+ * In the above, we use cmpxchg16 to atomically update the 128-bit
+ * IRTE, and the hardware cannot update the IRTE behind us, so
+ * the return value of cmpxchg16 should be the same as old_ire.
+ * This ASSERT validate it.
+ */
+ASSERT(ret == old_ire.val);
}
/* Mark specified intr remap entry as free */
@@ -395,7 +381,6 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu
*iommu,
/* Indicate remap format. */
remap_rte->format = 1;
-/* If cmpxchg16b is not available the caller must mask the IO-APIC pin. */
update_irte(iommu, iremap_entry, _ire, !init && !masked);
iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
iommu_flush_iec_index(iommu, 0, index);
@@ -437,21 +422,6 @@ void cf_check io_apic_write_remap_rte(
bool masked = true;
int rc;
-if ( !cpu_has_cx16 )
-{
- /*
-* Cannot atomically update the IRTE entry: mask the IO-APIC pin to
-* avoid interrupts seeing an inconsistent IRTE entry.
-*/
-old_rte = __ioapic_read_entry(apic, pin, true);
-if ( !old_rte.mask )
-{
-masked = false;
-old_rte.mask = 1;
-__ioapic_write_entry(apic, pin, true, old_rte);
-
[XEN PATCH v5 0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported
All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside specifically crafted virtual machines). Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported while cx16 isn't, those paths may be bugged and are barely tested, dead code in practice. Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup no-cx16 handling logic from VT-d and AMD-Vi drivers. Also disable interrupt remapping that also relies on cx16. Teddy --- Changed in v2: * Added cleanup no-cx16 code for x2APIC * Fixed commit and code formatting * Added missing Suggested-by note Changed in v3: * Use -ENODEV instead of -ENOSYS. Changed in v4: * Reworded "Disable IOMMU if cx16 isn't supported" * Moved interrupt remapping cleanup in separate patches * Check cx16 for interrupt remapping in driver's callbacks rather than in x2apic_bsp_setup Changed in v5: * Folded VT-d/AMD-Vi cx16 cleanup patches Teddy Astie (3): x86/iommu: Disable IOMMU if cx16 isn't supported VT-d: Cleanup MAP_SINGLE_DEVICE and related code x86/iommu: Disable intrerrupt remapping if cx16 is not supported xen/drivers/passthrough/amd/iommu_intr.c| 6 ++ xen/drivers/passthrough/amd/iommu_map.c | 42 -- xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 ++ xen/drivers/passthrough/vtd/intremap.c | 70 +--- xen/drivers/passthrough/vtd/iommu.c | 92 +++-- xen/drivers/passthrough/vtd/vtd.h | 5 +- 6 files changed, 77 insertions(+), 144 deletions(-) -- 2.44.0 Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[XEN PATCH v5 2/3] VT-d: Cleanup MAP_SINGLE_DEVICE and related code
This flag was only used in case cx16 is not available, as those code paths no
longer exist, this flag now does basically nothing.
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/vtd/iommu.c | 12 +++-
xen/drivers/passthrough/vtd/vtd.h | 5 ++---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 9c787ba9eb..7c6bae0256 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1692,15 +1692,9 @@ static int domain_context_mapping(struct domain *domain,
u8 devfn,
break;
}
-if ( domain != pdev->domain && pdev->domain != dom_io )
-{
-if ( pdev->domain->is_dying )
-mode |= MAP_OWNER_DYING;
-else if ( drhd &&
- !any_pdev_behind_iommu(pdev->domain, pdev, drhd->iommu) &&
- !pdev->phantom_stride )
-mode |= MAP_SINGLE_DEVICE;
-}
+if ( domain != pdev->domain && pdev->domain != dom_io &&
+ pdev->domain->is_dying )
+mode |= MAP_OWNER_DYING;
switch ( pdev->type )
{
diff --git a/xen/drivers/passthrough/vtd/vtd.h
b/xen/drivers/passthrough/vtd/vtd.h
index cb2df76eed..43f06a353d 100644
--- a/xen/drivers/passthrough/vtd/vtd.h
+++ b/xen/drivers/passthrough/vtd/vtd.h
@@ -28,9 +28,8 @@
*/
#define MAP_WITH_RMRR (1u << 0)
#define MAP_OWNER_DYING (1u << 1)
-#define MAP_SINGLE_DEVICE (1u << 2)
-#define MAP_ERROR_RECOVERY(1u << 3)
-#define UNMAP_ME_PHANTOM_FUNC (1u << 4)
+#define MAP_ERROR_RECOVERY(1u << 2)
+#define UNMAP_ME_PHANTOM_FUNC (1u << 3)
/* Allow for both IOAPIC and IOSAPIC. */
#define IO_xAPIC_route_entry IO_APIC_route_entry
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH v5 1/3] x86/iommu: Disable IOMMU if cx16 isn't supported
All hardware with VT-d/AMD-Vi has CMPXCHG16B support. Check this at
initialisation time, and remove the effectively-dead logic for the
non-cx16 case.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/amd/iommu_map.c | 42
xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 ++
xen/drivers/passthrough/vtd/iommu.c | 73 +++--
3 files changed, 45 insertions(+), 76 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c
b/xen/drivers/passthrough/amd/iommu_map.c
index e0f4fe736a..f67975e700 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -167,15 +167,14 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
{
bool valid = flags & SET_ROOT_VALID;
-if ( dte->v && dte->tv &&
- (cpu_has_cx16 || (flags & SET_ROOT_WITH_UNITY_MAP)) )
+if ( dte->v && dte->tv )
{
union {
struct amd_iommu_dte dte;
uint64_t raw64[4];
__uint128_t raw128[2];
} ldte = { .dte = *dte };
-__uint128_t old = ldte.raw128[0];
+__uint128_t res, old = ldte.raw128[0];
int ret = 0;
ldte.dte.domain_id = domain_id;
@@ -185,33 +184,20 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
ldte.dte.paging_mode = paging_mode;
ldte.dte.v = valid;
-if ( cpu_has_cx16 )
-{
-__uint128_t res = cmpxchg16b(dte, , [0]);
+res = cmpxchg16b(dte, , [0]);
-/*
- * Hardware does not update the DTE behind our backs, so the
- * return value should match "old".
- */
-if ( res != old )
-{
-printk(XENLOG_ERR
- "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
- domain_id,
- (uint64_t)(res >> 64), (uint64_t)res,
- (uint64_t)(old >> 64), (uint64_t)old);
-ret = -EILSEQ;
-}
-}
-else /* Best effort, updating domain_id last. */
+/*
+ * Hardware does not update the DTE behind our backs, so the
+ * return value should match "old".
+ */
+if ( res != old )
{
-uint64_t *ptr = (void *)dte;
-
-write_atomic(ptr + 0, ldte.raw64[0]);
-/* No barrier should be needed between these two. */
-write_atomic(ptr + 1, ldte.raw64[1]);
-
-ret = 1;
+printk(XENLOG_ERR
+ "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
+ domain_id,
+ (uint64_t)(res >> 64), (uint64_t)res,
+ (uint64_t)(old >> 64), (uint64_t)old);
+ret = -EILSEQ;
}
return ret;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index f6efd88e36..3a6a23f706 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
if ( !iommu_enable && !iommu_intremap )
return 0;
+if ( unlikely(!cpu_has_cx16) )
+{
+printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
+return -ENODEV;
+}
+
if ( (init_done ? amd_iommu_init_late()
: amd_iommu_init(false)) != 0 )
{
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index c7110af7c9..9c787ba9eb 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1482,7 +1482,7 @@ int domain_context_mapping_one(
{
struct domain_iommu *hd = dom_iommu(domain);
struct context_entry *context, *context_entries, lctxt;
-__uint128_t old;
+__uint128_t res, old;
uint64_t maddr;
uint16_t seg = iommu->drhd->segment, prev_did = 0;
struct domain *prev_dom = NULL;
@@ -1580,55 +1580,23 @@ int domain_context_mapping_one(
ASSERT(!context_fault_disable(lctxt));
}
-if ( cpu_has_cx16 )
-{
-__uint128_t res = cmpxchg16b(context, , );
-
-/*
- * Hardware does not update the context entry behind our backs,
- * so the return value should match "old".
- */
-if ( res != old )
-{
-if ( pdev )
-check_cleanup_domid_map(domain, pdev, iommu);
-printk(XENLOG_ERR
- "%pp: unexpected context entry %016lx_%016lx (expected
%016lx_%016lx)\n",
- _SBDF(seg, bus, devfn),
- (uint64_t)(res >> 64), (uint64_t)res,
- (uint64_t)(old >> 64), (uint64_t)old);
-
Re: [XEN PATCH v1 15/15] x86/hvm: make AMD-V and Intel VT-x support configurable
Hello Sergiy, > Also make INTEL_IOMMU/AMD_IOMMU options dependant on VMX/SVM options. The discussion in the RFC series stated the IOMMU may be used with PV guests, and doesn't rely on VMX/SVM support (in fact, it can be used without HVM support in Xen). However, in the discussion, posted interrupts were supposed to be dependent on VMX/SVM instead. I suppose this is a leftover from the original RFC ? > https://lore.kernel.org/xen-devel/e29e375f-3d30-0eb1-7e28-b93f2d831...@suse.com/ Teddy --- Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [XEN PATCH v4 0/5] x86/iommu: Drop IOMMU support when cx16 isn't supported
Le 15/04/2024 à 14:15, Teddy Astie a écrit : > All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside > specifically crafted virtual machines). > > Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported > while cx16 isn't, those paths may be bugged and are barely tested, dead code > in practice. > > Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup > no-cx16 handling logic from VT-d and AMD-Vi drivers. Also disable > interrupt remapping that also relies on cx16. > > Teddy Astie (5): >VT-d: Disable IOMMU if cx16 isn't supported >AMD-Vi: Disable IOMMU if cx16 isn't supported >VT-d: Cleanup MAP_SINGLE_DEVICE and related code >VT-d: Disable intrerrupt remapping if cx16 is not supported >AMD-Vi: Disable intrerrupt remapping if cx16 is not supported > > xen/drivers/passthrough/amd/iommu_intr.c| 6 ++ > xen/drivers/passthrough/amd/iommu_map.c | 42 -- > xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 ++ > xen/drivers/passthrough/vtd/intremap.c | 70 +--- > xen/drivers/passthrough/vtd/iommu.c | 92 +++-- > xen/drivers/passthrough/vtd/vtd.h | 5 +- > 6 files changed, 77 insertions(+), 144 deletions(-) > Here is the patch history that got lost for some reason in this cover. Changed in v2: * Added cleanup no-cx16 code for x2APIC * Fixed commit and code formatting * Added missing Suggested-by note Changed in v3: * Use -ENODEV instead of -ENOSYS. Changed in v4: * Reworded "Disable IOMMU if cx16 isn't supported" * Moved interrupt remapping cleanup in separate patches * Check cx16 for interrupt remapping in driver's callbacks rather than in x2apic_bsp_setup Teddy --- Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[XEN PATCH v4 2/5] AMD-Vi: Disable IOMMU if cx16 isn't supported
All hardware with AMD-Vi has CMPXCHG16 support. Check this at initialisation
time, and remove the effectively-dead logic for the non-cx16 case.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/amd/iommu_map.c | 42 +++--
xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 +++
2 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c
b/xen/drivers/passthrough/amd/iommu_map.c
index e0f4fe736a..f67975e700 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -167,15 +167,14 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
{
bool valid = flags & SET_ROOT_VALID;
-if ( dte->v && dte->tv &&
- (cpu_has_cx16 || (flags & SET_ROOT_WITH_UNITY_MAP)) )
+if ( dte->v && dte->tv )
{
union {
struct amd_iommu_dte dte;
uint64_t raw64[4];
__uint128_t raw128[2];
} ldte = { .dte = *dte };
-__uint128_t old = ldte.raw128[0];
+__uint128_t res, old = ldte.raw128[0];
int ret = 0;
ldte.dte.domain_id = domain_id;
@@ -185,33 +184,20 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
ldte.dte.paging_mode = paging_mode;
ldte.dte.v = valid;
-if ( cpu_has_cx16 )
-{
-__uint128_t res = cmpxchg16b(dte, , [0]);
+res = cmpxchg16b(dte, , [0]);
-/*
- * Hardware does not update the DTE behind our backs, so the
- * return value should match "old".
- */
-if ( res != old )
-{
-printk(XENLOG_ERR
- "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
- domain_id,
- (uint64_t)(res >> 64), (uint64_t)res,
- (uint64_t)(old >> 64), (uint64_t)old);
-ret = -EILSEQ;
-}
-}
-else /* Best effort, updating domain_id last. */
+/*
+ * Hardware does not update the DTE behind our backs, so the
+ * return value should match "old".
+ */
+if ( res != old )
{
-uint64_t *ptr = (void *)dte;
-
-write_atomic(ptr + 0, ldte.raw64[0]);
-/* No barrier should be needed between these two. */
-write_atomic(ptr + 1, ldte.raw64[1]);
-
-ret = 1;
+printk(XENLOG_ERR
+ "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
+ domain_id,
+ (uint64_t)(res >> 64), (uint64_t)res,
+ (uint64_t)(old >> 64), (uint64_t)old);
+ret = -EILSEQ;
}
return ret;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index f6efd88e36..3a6a23f706 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -305,6 +305,12 @@ static int __init cf_check iov_detect(void)
if ( !iommu_enable && !iommu_intremap )
return 0;
+if ( unlikely(!cpu_has_cx16) )
+{
+printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
+return -ENODEV;
+ }
+
if ( (init_done ? amd_iommu_init_late()
: amd_iommu_init(false)) != 0 )
{
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH v4 1/5] VT-d: Disable IOMMU if cx16 isn't supported
All hardware with VT-d has CMPXCHG16B support. Check this at initialisation
time, and remove the effectively-dead logic for the non-cx16 case.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/vtd/iommu.c | 73 ++---
1 file changed, 25 insertions(+), 48 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index c7110af7c9..9c787ba9eb 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1482,7 +1482,7 @@ int domain_context_mapping_one(
{
struct domain_iommu *hd = dom_iommu(domain);
struct context_entry *context, *context_entries, lctxt;
-__uint128_t old;
+__uint128_t res, old;
uint64_t maddr;
uint16_t seg = iommu->drhd->segment, prev_did = 0;
struct domain *prev_dom = NULL;
@@ -1580,55 +1580,23 @@ int domain_context_mapping_one(
ASSERT(!context_fault_disable(lctxt));
}
-if ( cpu_has_cx16 )
-{
-__uint128_t res = cmpxchg16b(context, , );
-
-/*
- * Hardware does not update the context entry behind our backs,
- * so the return value should match "old".
- */
-if ( res != old )
-{
-if ( pdev )
-check_cleanup_domid_map(domain, pdev, iommu);
-printk(XENLOG_ERR
- "%pp: unexpected context entry %016lx_%016lx (expected
%016lx_%016lx)\n",
- _SBDF(seg, bus, devfn),
- (uint64_t)(res >> 64), (uint64_t)res,
- (uint64_t)(old >> 64), (uint64_t)old);
-rc = -EILSEQ;
-goto unlock;
-}
-}
-else if ( !prev_dom || !(mode & MAP_WITH_RMRR) )
-{
-context_clear_present(*context);
-iommu_sync_cache(context, sizeof(*context));
+res = cmpxchg16b(context, , );
-write_atomic(>hi, lctxt.hi);
-/* No barrier should be needed between these two. */
-write_atomic(>lo, lctxt.lo);
-}
-else /* Best effort, updating DID last. */
+/*
+ * Hardware does not update the context entry behind our backs,
+ * so the return value should match "old".
+ */
+if ( res != old )
{
- /*
- * By non-atomically updating the context entry's DID field last,
- * during a short window in time TLB entries with the old domain ID
- * but the new page tables may be inserted. This could affect I/O
- * of other devices using this same (old) domain ID. Such updating
- * therefore is not a problem if this was the only device associated
- * with the old domain ID. Diverting I/O of any of a dying domain's
- * devices to the quarantine page tables is intended anyway.
- */
-if ( !(mode & (MAP_OWNER_DYING | MAP_SINGLE_DEVICE)) )
-printk(XENLOG_WARNING VTDPREFIX
- " %pp: reassignment may cause %pd data corruption\n",
- _SBDF(seg, bus, devfn), prev_dom);
-
-write_atomic(>lo, lctxt.lo);
-/* No barrier should be needed between these two. */
-write_atomic(>hi, lctxt.hi);
+if ( pdev )
+check_cleanup_domid_map(domain, pdev, iommu);
+printk(XENLOG_ERR
+"%pp: unexpected context entry %016lx_%016lx (expected
%016lx_%016lx)\n",
+_SBDF(seg, bus, devfn),
+(uint64_t)(res >> 64), (uint64_t)res,
+(uint64_t)(old >> 64), (uint64_t)old);
+rc = -EILSEQ;
+goto unlock;
}
iommu_sync_cache(context, sizeof(struct context_entry));
@@ -2630,6 +2598,15 @@ static int __init cf_check vtd_setup(void)
int ret;
bool reg_inval_supported = true;
+if ( unlikely(!cpu_has_cx16) )
+{
+printk(XENLOG_ERR VTDPREFIX
+ "IOMMU: CPU doesn't support CMPXCHG16B, disabling\n");
+
+ret = -ENODEV;
+ goto error;
+}
+
if ( list_empty(_drhd_units) )
{
ret = -ENODEV;
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH v4 3/5] VT-d: Cleanup MAP_SINGLE_DEVICE and related code
This flag was only used in case cx16 is not available, as those code paths no
longer exist, this flag now does basically nothing.
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/vtd/iommu.c | 12 +++-
xen/drivers/passthrough/vtd/vtd.h | 5 ++---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 9c787ba9eb..7c6bae0256 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1692,15 +1692,9 @@ static int domain_context_mapping(struct domain *domain,
u8 devfn,
break;
}
-if ( domain != pdev->domain && pdev->domain != dom_io )
-{
-if ( pdev->domain->is_dying )
-mode |= MAP_OWNER_DYING;
-else if ( drhd &&
- !any_pdev_behind_iommu(pdev->domain, pdev, drhd->iommu) &&
- !pdev->phantom_stride )
-mode |= MAP_SINGLE_DEVICE;
-}
+if ( domain != pdev->domain && pdev->domain != dom_io &&
+ pdev->domain->is_dying )
+mode |= MAP_OWNER_DYING;
switch ( pdev->type )
{
diff --git a/xen/drivers/passthrough/vtd/vtd.h
b/xen/drivers/passthrough/vtd/vtd.h
index cb2df76eed..43f06a353d 100644
--- a/xen/drivers/passthrough/vtd/vtd.h
+++ b/xen/drivers/passthrough/vtd/vtd.h
@@ -28,9 +28,8 @@
*/
#define MAP_WITH_RMRR (1u << 0)
#define MAP_OWNER_DYING (1u << 1)
-#define MAP_SINGLE_DEVICE (1u << 2)
-#define MAP_ERROR_RECOVERY(1u << 3)
-#define UNMAP_ME_PHANTOM_FUNC (1u << 4)
+#define MAP_ERROR_RECOVERY(1u << 2)
+#define UNMAP_ME_PHANTOM_FUNC (1u << 3)
/* Allow for both IOAPIC and IOSAPIC. */
#define IO_xAPIC_route_entry IO_APIC_route_entry
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH v4 5/5] AMD-Vi: Disable intrerrupt remapping if cx16 is not supported
All hardware with AMD-Vi has CMPXCHG16 support. Check this at initialisation
time, and remove the effectively-dead logic for the non-cx16 case.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/amd/iommu_intr.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c
b/xen/drivers/passthrough/amd/iommu_intr.c
index 7fc796dec2..9ab7c68749 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -649,6 +649,12 @@ bool __init cf_check iov_supports_xt(void)
if ( !iommu_enable || !iommu_intremap )
return false;
+if ( unlikely(!cpu_has_cx16) )
+{
+AMD_IOMMU_WARN("CPU doesn't support CMPXCHG16B, disable interrupt
remapping\n");
+return false;
+}
+
if ( amd_iommu_prepare(true) )
return false;
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH v4 0/5] x86/iommu: Drop IOMMU support when cx16 isn't supported
All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside specifically crafted virtual machines). Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported while cx16 isn't, those paths may be bugged and are barely tested, dead code in practice. Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup no-cx16 handling logic from VT-d and AMD-Vi drivers. Also disable interrupt remapping that also relies on cx16. Teddy Astie (5): VT-d: Disable IOMMU if cx16 isn't supported AMD-Vi: Disable IOMMU if cx16 isn't supported VT-d: Cleanup MAP_SINGLE_DEVICE and related code VT-d: Disable intrerrupt remapping if cx16 is not supported AMD-Vi: Disable intrerrupt remapping if cx16 is not supported xen/drivers/passthrough/amd/iommu_intr.c| 6 ++ xen/drivers/passthrough/amd/iommu_map.c | 42 -- xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 ++ xen/drivers/passthrough/vtd/intremap.c | 70 +--- xen/drivers/passthrough/vtd/iommu.c | 92 +++-- xen/drivers/passthrough/vtd/vtd.h | 5 +- 6 files changed, 77 insertions(+), 144 deletions(-) -- 2.44.0 Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[XEN PATCH v4 4/5] VT-d: Disable intrerrupt remapping if cx16 is not supported
All hardware with VT-d has CMPXCHG16B support. Check this at initialisation
time, and remove the effectively-dead logic for the non-cx16 case.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/vtd/intremap.c | 70 --
xen/drivers/passthrough/vtd/iommu.c| 7 +--
2 files changed, 21 insertions(+), 56 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/intremap.c
b/xen/drivers/passthrough/vtd/intremap.c
index c504852eb8..7d4d907b4f 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -150,6 +150,13 @@ bool __init cf_check intel_iommu_supports_eim(void)
if ( !iommu_qinval || !iommu_intremap || list_empty(_drhd_units) )
return false;
+if ( unlikely(!cpu_has_cx16) )
+{
+printk(XENLOG_WARNING VTDPREFIX
+ "CPU doesn't support CMPXCHG16B, disable interrupt
remapping\n");
+return false;
+}
+
/* We MUST have a DRHD unit for each IOAPIC. */
for ( apic = 0; apic < nr_ioapics; apic++ )
if ( !ioapic_to_drhd(IO_APIC_ID(apic)) )
@@ -173,47 +180,26 @@ bool __init cf_check intel_iommu_supports_eim(void)
* Assume iremap_lock has been acquired. It is to make sure software will not
* change the same IRTE behind us. With this assumption, if only high qword or
* low qword in IRTE is to be updated, this function's atomic variant can
- * present an atomic update to VT-d hardware even when cmpxchg16b
- * instruction is not supported.
+ * present an atomic update to VT-d hardware.
*/
static void update_irte(struct vtd_iommu *iommu, struct iremap_entry *entry,
const struct iremap_entry *new_ire, bool atomic)
{
-ASSERT(spin_is_locked(>intremap.lock));
+__uint128_t ret;
+struct iremap_entry old_ire;
-if ( cpu_has_cx16 )
-{
-__uint128_t ret;
-struct iremap_entry old_ire;
+ASSERT(spin_is_locked(>intremap.lock));
-old_ire = *entry;
-ret = cmpxchg16b(entry, _ire, new_ire);
+old_ire = *entry;
+ret = cmpxchg16b(entry, _ire, new_ire);
-/*
- * In the above, we use cmpxchg16 to atomically update the 128-bit
- * IRTE, and the hardware cannot update the IRTE behind us, so
- * the return value of cmpxchg16 should be the same as old_ire.
- * This ASSERT validate it.
- */
-ASSERT(ret == old_ire.val);
-}
-else
-{
-/*
- * VT-d hardware doesn't update IRTEs behind us, nor the software
- * since we hold iremap_lock. If the caller wants VT-d hardware to
- * always see a consistent entry, but we can't meet it, a bug will
- * be raised.
- */
-if ( entry->lo == new_ire->lo )
-write_atomic(>hi, new_ire->hi);
-else if ( entry->hi == new_ire->hi )
-write_atomic(>lo, new_ire->lo);
-else if ( !atomic )
-*entry = *new_ire;
-else
-BUG();
-}
+/*
+ * In the above, we use cmpxchg16 to atomically update the 128-bit
+ * IRTE, and the hardware cannot update the IRTE behind us, so
+ * the return value of cmpxchg16 should be the same as old_ire.
+ * This ASSERT validate it.
+ */
+ASSERT(ret == old_ire.val);
}
/* Mark specified intr remap entry as free */
@@ -395,7 +381,6 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu
*iommu,
/* Indicate remap format. */
remap_rte->format = 1;
-/* If cmpxchg16b is not available the caller must mask the IO-APIC pin. */
update_irte(iommu, iremap_entry, _ire, !init && !masked);
iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
iommu_flush_iec_index(iommu, 0, index);
@@ -437,21 +422,6 @@ void cf_check io_apic_write_remap_rte(
bool masked = true;
int rc;
-if ( !cpu_has_cx16 )
-{
- /*
-* Cannot atomically update the IRTE entry: mask the IO-APIC pin to
-* avoid interrupts seeing an inconsistent IRTE entry.
-*/
-old_rte = __ioapic_read_entry(apic, pin, true);
-if ( !old_rte.mask )
-{
-masked = false;
-old_rte.mask = 1;
-__ioapic_write_entry(apic, pin, true, old_rte);
-}
-}
-
/* Not the initializer, for old gcc to cope. */
new_rte.raw = rte;
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 7c6bae0256..a1bd3c5ff6 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2663,12 +2663,7 @@ static int __init cf_check vtd_setup(void)
iommu_intremap = iommu_intremap_off;
#ifndef iommu_intpost
-/*
- * We cannot use posted interrupt if X86_FEATURE_CX16 is
- * not supported, since we count on this feature to
- * atomically update 16-byte IRTE in posted format
Re: [XEN PATCH v3 0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported
Le 12/04/2024 à 16:49, Andrew Cooper a écrit : > 1) You introduced trailing whitespace in patch 1 in the middle line here: > >> + ASSERT(spin_is_locked(>intremap.lock)); + + old_ire = *entry; Good catch, will fix > 2) In your commit messages, the grammar is a bit awkward. It would be > clearer to say: > > "All hardware with VT-d/AMD-Vi has CMPXCHG16 support. Check this at > initialisation time, and remove the effectively-dead logic for the > non-cx16 case." > > just as you've done in the cover letter. Yes > 3) In patch 1, you shouldn't modify x2apic_bsp_setup() like that. > x2APIC && no-IOMMU is a legal combination. > > Instead, you should put a cx16 check in both driver's supports_x2apic() > hooks. In this case, you mean both intel_iommu_supports_eim and iov_supports_xt (AMD) ? > > 4) In patch 3, you should drop the Suggested-by me. You found that one > all yourself. > Will change this. Teddy --- Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[XEN PATCH v3 3/3] VT-d: Cleanup MAP_SINGLE_DEVICE and related code
This flag was only used in case cx16 is not available, as those code paths no
longer exist, this flag now does basically nothing.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/vtd/iommu.c | 12 +++-
xen/drivers/passthrough/vtd/vtd.h | 5 ++---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index ef9380ed6a..a1bd3c5ff6 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1692,15 +1692,9 @@ static int domain_context_mapping(struct domain *domain,
u8 devfn,
break;
}
-if ( domain != pdev->domain && pdev->domain != dom_io )
-{
-if ( pdev->domain->is_dying )
-mode |= MAP_OWNER_DYING;
-else if ( drhd &&
- !any_pdev_behind_iommu(pdev->domain, pdev, drhd->iommu) &&
- !pdev->phantom_stride )
-mode |= MAP_SINGLE_DEVICE;
-}
+if ( domain != pdev->domain && pdev->domain != dom_io &&
+ pdev->domain->is_dying )
+mode |= MAP_OWNER_DYING;
switch ( pdev->type )
{
diff --git a/xen/drivers/passthrough/vtd/vtd.h
b/xen/drivers/passthrough/vtd/vtd.h
index cb2df76eed..43f06a353d 100644
--- a/xen/drivers/passthrough/vtd/vtd.h
+++ b/xen/drivers/passthrough/vtd/vtd.h
@@ -28,9 +28,8 @@
*/
#define MAP_WITH_RMRR (1u << 0)
#define MAP_OWNER_DYING (1u << 1)
-#define MAP_SINGLE_DEVICE (1u << 2)
-#define MAP_ERROR_RECOVERY(1u << 3)
-#define UNMAP_ME_PHANTOM_FUNC (1u << 4)
+#define MAP_ERROR_RECOVERY(1u << 2)
+#define UNMAP_ME_PHANTOM_FUNC (1u << 3)
/* Allow for both IOAPIC and IOSAPIC. */
#define IO_xAPIC_route_entry IO_APIC_route_entry
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH v3 1/3] VT-d: Disable IOMMU if cx16 isn't supported
No hardware has VT-d support while not having cx16 support, disable IOMMU in
this case to avoid potentially buggy code.
Now that IOMMU is only enabled if cx16 is supported, drop dead code that
handles cases where cx16 isn't supported.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/arch/x86/apic.c| 6 ++
xen/drivers/passthrough/vtd/intremap.c | 65 +
xen/drivers/passthrough/vtd/iommu.c| 80 +-
3 files changed, 46 insertions(+), 105 deletions(-)
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 592b78e11e..91d7f2b248 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -836,6 +836,12 @@ void __init x2apic_bsp_setup(void)
if ( !cpu_has_x2apic )
return;
+if ( unlikely(!cpu_has_cx16) )
+{
+printk("x2APIC: CPU doesn't support CMPXCHG16B, disabling\n");
+return;
+}
+
if ( !opt_x2apic )
{
if ( !x2apic_enabled )
diff --git a/xen/drivers/passthrough/vtd/intremap.c
b/xen/drivers/passthrough/vtd/intremap.c
index c504852eb8..b0a0dbdbc2 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -173,47 +173,26 @@ bool __init cf_check intel_iommu_supports_eim(void)
* Assume iremap_lock has been acquired. It is to make sure software will not
* change the same IRTE behind us. With this assumption, if only high qword or
* low qword in IRTE is to be updated, this function's atomic variant can
- * present an atomic update to VT-d hardware even when cmpxchg16b
- * instruction is not supported.
+ * present an atomic update to VT-d hardware.
*/
static void update_irte(struct vtd_iommu *iommu, struct iremap_entry *entry,
const struct iremap_entry *new_ire, bool atomic)
{
-ASSERT(spin_is_locked(>intremap.lock));
-
-if ( cpu_has_cx16 )
-{
-__uint128_t ret;
-struct iremap_entry old_ire;
+__uint128_t ret;
+struct iremap_entry old_ire;
-old_ire = *entry;
-ret = cmpxchg16b(entry, _ire, new_ire);
+ASSERT(spin_is_locked(>intremap.lock));
+
+old_ire = *entry;
+ret = cmpxchg16b(entry, _ire, new_ire);
-/*
- * In the above, we use cmpxchg16 to atomically update the 128-bit
- * IRTE, and the hardware cannot update the IRTE behind us, so
- * the return value of cmpxchg16 should be the same as old_ire.
- * This ASSERT validate it.
- */
-ASSERT(ret == old_ire.val);
-}
-else
-{
-/*
- * VT-d hardware doesn't update IRTEs behind us, nor the software
- * since we hold iremap_lock. If the caller wants VT-d hardware to
- * always see a consistent entry, but we can't meet it, a bug will
- * be raised.
- */
-if ( entry->lo == new_ire->lo )
-write_atomic(>hi, new_ire->hi);
-else if ( entry->hi == new_ire->hi )
-write_atomic(>lo, new_ire->lo);
-else if ( !atomic )
-*entry = *new_ire;
-else
-BUG();
-}
+/*
+ * In the above, we use cmpxchg16 to atomically update the 128-bit
+ * IRTE, and the hardware cannot update the IRTE behind us, so
+ * the return value of cmpxchg16 should be the same as old_ire.
+ * This ASSERT validate it.
+ */
+ASSERT(ret == old_ire.val);
}
/* Mark specified intr remap entry as free */
@@ -395,7 +374,6 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu
*iommu,
/* Indicate remap format. */
remap_rte->format = 1;
-/* If cmpxchg16b is not available the caller must mask the IO-APIC pin. */
update_irte(iommu, iremap_entry, _ire, !init && !masked);
iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
iommu_flush_iec_index(iommu, 0, index);
@@ -437,21 +415,6 @@ void cf_check io_apic_write_remap_rte(
bool masked = true;
int rc;
-if ( !cpu_has_cx16 )
-{
- /*
-* Cannot atomically update the IRTE entry: mask the IO-APIC pin to
-* avoid interrupts seeing an inconsistent IRTE entry.
-*/
-old_rte = __ioapic_read_entry(apic, pin, true);
-if ( !old_rte.mask )
-{
-masked = false;
-old_rte.mask = 1;
-__ioapic_write_entry(apic, pin, true, old_rte);
-}
-}
-
/* Not the initializer, for old gcc to cope. */
new_rte.raw = rte;
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index c7110af7c9..ef9380ed6a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1482,7 +1482,7 @@ int domain_context_mapping_one(
{
struct domain_iommu *hd = dom_iommu(domain);
struct context_entry *context, *context_entries, lctxt;
-__uint128_t old;
+__uint128_t res, old;
uint64_t maddr;
uint16_t seg = iom
[XEN PATCH v3 2/3] AMD-Vi: Disable IOMMU if cx16 isn't supported
No hardware has AMD-Vi support while not having cx16 support, disable IOMMU
in this case to avoid potentially buggy code.
Now that IOMMU is only enabled if cx16 is supported, drop dead code that
handles cases where cx16 isn't supported.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/amd/iommu_map.c | 42 +++--
xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 +++
2 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c
b/xen/drivers/passthrough/amd/iommu_map.c
index e0f4fe736a..f67975e700 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -167,15 +167,14 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
{
bool valid = flags & SET_ROOT_VALID;
-if ( dte->v && dte->tv &&
- (cpu_has_cx16 || (flags & SET_ROOT_WITH_UNITY_MAP)) )
+if ( dte->v && dte->tv )
{
union {
struct amd_iommu_dte dte;
uint64_t raw64[4];
__uint128_t raw128[2];
} ldte = { .dte = *dte };
-__uint128_t old = ldte.raw128[0];
+__uint128_t res, old = ldte.raw128[0];
int ret = 0;
ldte.dte.domain_id = domain_id;
@@ -185,33 +184,20 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
ldte.dte.paging_mode = paging_mode;
ldte.dte.v = valid;
-if ( cpu_has_cx16 )
-{
-__uint128_t res = cmpxchg16b(dte, , [0]);
+res = cmpxchg16b(dte, , [0]);
-/*
- * Hardware does not update the DTE behind our backs, so the
- * return value should match "old".
- */
-if ( res != old )
-{
-printk(XENLOG_ERR
- "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
- domain_id,
- (uint64_t)(res >> 64), (uint64_t)res,
- (uint64_t)(old >> 64), (uint64_t)old);
-ret = -EILSEQ;
-}
-}
-else /* Best effort, updating domain_id last. */
+/*
+ * Hardware does not update the DTE behind our backs, so the
+ * return value should match "old".
+ */
+if ( res != old )
{
-uint64_t *ptr = (void *)dte;
-
-write_atomic(ptr + 0, ldte.raw64[0]);
-/* No barrier should be needed between these two. */
-write_atomic(ptr + 1, ldte.raw64[1]);
-
-ret = 1;
+printk(XENLOG_ERR
+ "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
+ domain_id,
+ (uint64_t)(res >> 64), (uint64_t)res,
+ (uint64_t)(old >> 64), (uint64_t)old);
+ret = -EILSEQ;
}
return ret;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index f6efd88e36..a213dbea0b 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -312,6 +312,12 @@ static int __init cf_check iov_detect(void)
return -ENODEV;
}
+if ( unlikely(!cpu_has_cx16) )
+{
+printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
+ return -ENODEV;
+}
+
init_done = 1;
if ( !amd_iommu_perdev_intremap )
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH v3 0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported
All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside specifically crafted virtual machines). Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported while cx16 isn't, those paths may be bugged and are barely tested, dead code in practice. Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup no-cx16 handling logic from VT-d and AMD-Vi drivers. Teddy Changed in v2: * Added cleanup no-cx16 code for x2APIC * Fixed commit and code formatting * Added missing Suggested-by note Changed in v3: * Use -ENODEV instead of -ENOSYS. Teddy Astie (3): VT-d: Disable IOMMU if cx16 isn't supported AMD-Vi: Disable IOMMU if cx16 isn't supported VT-d: Cleanup MAP_SINGLE_DEVICE and related code xen/arch/x86/apic.c | 6 ++ xen/drivers/passthrough/amd/iommu_map.c | 42 -- xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 ++ xen/drivers/passthrough/vtd/intremap.c | 65 --- xen/drivers/passthrough/vtd/iommu.c | 92 +++-- xen/drivers/passthrough/vtd/vtd.h | 5 +- 6 files changed, 71 insertions(+), 145 deletions(-) -- 2.44.0 Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [XEN PATCH v2 0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported
Le 11/04/2024 à 22:05, Andrew Cooper a écrit : > On 08/04/2024 2:02 pm, Teddy Astie wrote: >> All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside >> specifically crafted virtual machines). >> >> Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported >> while cx16 isn't, those paths may be bugged and are barely tested, dead code >> in practice. >> >> Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup >> no-cx16 handling logic from VT-d and AMD-Vi drivers. >> >> Teddy >> >> Changed in v2: >> >> * Added cleanup no-cx16 code for x2APIC >> * Fixed commit and code formatting >> * Added missing Suggested-by note >> >> Teddy Astie (3): >>VT-d: Disable IOMMU if cx16 isn't supported >>AMD-Vi: Disable IOMMU if cx16 isn't supported >>VT-d: Cleanup MAP_SINGLE_DEVICE and related code >> >> xen/arch/x86/apic.c | 6 ++ >> xen/drivers/passthrough/amd/iommu_map.c | 42 -- >> xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 ++ >> xen/drivers/passthrough/vtd/intremap.c | 65 --- >> xen/drivers/passthrough/vtd/iommu.c | 92 +++-- >> xen/drivers/passthrough/vtd/vtd.h | 5 +- >> 6 files changed, 71 insertions(+), 145 deletions(-) >> > > Sorry, but you've sent out two copies of each patch in this series, and > it's not clear if they're identical or not. > > Please could you send out another version, making sure there's only one > of each patch. > > Also, you need to swap ENOSYS with ENODEV, as per Jan's review on v1. > > Thanks, > > ~Andrew Hello, Not entirely sure why it got sent twice, as marek said he only received it once. Will double-check next time to avoid this issue in case I wrongfully sent it twice. Will also swap ENOSYS with ENODEV in the next version. Thanks, Teddy Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[XEN PATCH v2 3/3] VT-d: Cleanup MAP_SINGLE_DEVICE and related code
This flag was only used in case cx16 is not available, as those code paths no
longer exist, this flag now does basically nothing.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/vtd/iommu.c | 12 +++-
xen/drivers/passthrough/vtd/vtd.h | 5 ++---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 47b56f37a9..4b15e6da79 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1692,15 +1692,9 @@ static int domain_context_mapping(struct domain *domain,
u8 devfn,
break;
}
-if ( domain != pdev->domain && pdev->domain != dom_io )
-{
-if ( pdev->domain->is_dying )
-mode |= MAP_OWNER_DYING;
-else if ( drhd &&
- !any_pdev_behind_iommu(pdev->domain, pdev, drhd->iommu) &&
- !pdev->phantom_stride )
-mode |= MAP_SINGLE_DEVICE;
-}
+if ( domain != pdev->domain && pdev->domain != dom_io &&
+ pdev->domain->is_dying )
+mode |= MAP_OWNER_DYING;
switch ( pdev->type )
{
diff --git a/xen/drivers/passthrough/vtd/vtd.h
b/xen/drivers/passthrough/vtd/vtd.h
index cb2df76eed..43f06a353d 100644
--- a/xen/drivers/passthrough/vtd/vtd.h
+++ b/xen/drivers/passthrough/vtd/vtd.h
@@ -28,9 +28,8 @@
*/
#define MAP_WITH_RMRR (1u << 0)
#define MAP_OWNER_DYING (1u << 1)
-#define MAP_SINGLE_DEVICE (1u << 2)
-#define MAP_ERROR_RECOVERY(1u << 3)
-#define UNMAP_ME_PHANTOM_FUNC (1u << 4)
+#define MAP_ERROR_RECOVERY(1u << 2)
+#define UNMAP_ME_PHANTOM_FUNC (1u << 3)
/* Allow for both IOAPIC and IOSAPIC. */
#define IO_xAPIC_route_entry IO_APIC_route_entry
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH v2 1/3] VT-d: Disable IOMMU if cx16 isn't supported
No hardware has VT-d support while not having cx16 support, disable IOMMU in
this case to avoid potentially buggy code.
Now that IOMMU is only enabled if cx16 is supported, drop dead code that
handles cases where cx16 isn't supported.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/arch/x86/apic.c| 6 ++
xen/drivers/passthrough/vtd/intremap.c | 65 +
xen/drivers/passthrough/vtd/iommu.c| 80 +-
3 files changed, 46 insertions(+), 105 deletions(-)
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 592b78e11e..91d7f2b248 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -836,6 +836,12 @@ void __init x2apic_bsp_setup(void)
if ( !cpu_has_x2apic )
return;
+if ( unlikely(!cpu_has_cx16) )
+{
+printk("x2APIC: CPU doesn't support CMPXCHG16B, disabling\n");
+return;
+}
+
if ( !opt_x2apic )
{
if ( !x2apic_enabled )
diff --git a/xen/drivers/passthrough/vtd/intremap.c
b/xen/drivers/passthrough/vtd/intremap.c
index c504852eb8..b0a0dbdbc2 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -173,47 +173,26 @@ bool __init cf_check intel_iommu_supports_eim(void)
* Assume iremap_lock has been acquired. It is to make sure software will not
* change the same IRTE behind us. With this assumption, if only high qword or
* low qword in IRTE is to be updated, this function's atomic variant can
- * present an atomic update to VT-d hardware even when cmpxchg16b
- * instruction is not supported.
+ * present an atomic update to VT-d hardware.
*/
static void update_irte(struct vtd_iommu *iommu, struct iremap_entry *entry,
const struct iremap_entry *new_ire, bool atomic)
{
-ASSERT(spin_is_locked(>intremap.lock));
-
-if ( cpu_has_cx16 )
-{
-__uint128_t ret;
-struct iremap_entry old_ire;
+__uint128_t ret;
+struct iremap_entry old_ire;
-old_ire = *entry;
-ret = cmpxchg16b(entry, _ire, new_ire);
+ASSERT(spin_is_locked(>intremap.lock));
+
+old_ire = *entry;
+ret = cmpxchg16b(entry, _ire, new_ire);
-/*
- * In the above, we use cmpxchg16 to atomically update the 128-bit
- * IRTE, and the hardware cannot update the IRTE behind us, so
- * the return value of cmpxchg16 should be the same as old_ire.
- * This ASSERT validate it.
- */
-ASSERT(ret == old_ire.val);
-}
-else
-{
-/*
- * VT-d hardware doesn't update IRTEs behind us, nor the software
- * since we hold iremap_lock. If the caller wants VT-d hardware to
- * always see a consistent entry, but we can't meet it, a bug will
- * be raised.
- */
-if ( entry->lo == new_ire->lo )
-write_atomic(>hi, new_ire->hi);
-else if ( entry->hi == new_ire->hi )
-write_atomic(>lo, new_ire->lo);
-else if ( !atomic )
-*entry = *new_ire;
-else
-BUG();
-}
+/*
+ * In the above, we use cmpxchg16 to atomically update the 128-bit
+ * IRTE, and the hardware cannot update the IRTE behind us, so
+ * the return value of cmpxchg16 should be the same as old_ire.
+ * This ASSERT validate it.
+ */
+ASSERT(ret == old_ire.val);
}
/* Mark specified intr remap entry as free */
@@ -395,7 +374,6 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu
*iommu,
/* Indicate remap format. */
remap_rte->format = 1;
-/* If cmpxchg16b is not available the caller must mask the IO-APIC pin. */
update_irte(iommu, iremap_entry, _ire, !init && !masked);
iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
iommu_flush_iec_index(iommu, 0, index);
@@ -437,21 +415,6 @@ void cf_check io_apic_write_remap_rte(
bool masked = true;
int rc;
-if ( !cpu_has_cx16 )
-{
- /*
-* Cannot atomically update the IRTE entry: mask the IO-APIC pin to
-* avoid interrupts seeing an inconsistent IRTE entry.
-*/
-old_rte = __ioapic_read_entry(apic, pin, true);
-if ( !old_rte.mask )
-{
-masked = false;
-old_rte.mask = 1;
-__ioapic_write_entry(apic, pin, true, old_rte);
-}
-}
-
/* Not the initializer, for old gcc to cope. */
new_rte.raw = rte;
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index c7110af7c9..47b56f37a9 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1482,7 +1482,7 @@ int domain_context_mapping_one(
{
struct domain_iommu *hd = dom_iommu(domain);
struct context_entry *context, *context_entries, lctxt;
-__uint128_t old;
+__uint128_t res, old;
uint64_t maddr;
uint16_t seg = iom
[XEN PATCH v2 0/3] x86/iommu: Drop IOMMU support when cx16 isn't supported
All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside specifically crafted virtual machines). Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported while cx16 isn't, those paths may be bugged and are barely tested, dead code in practice. Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup no-cx16 handling logic from VT-d and AMD-Vi drivers. Teddy Changed in v2: * Added cleanup no-cx16 code for x2APIC * Fixed commit and code formatting * Added missing Suggested-by note Teddy Astie (3): VT-d: Disable IOMMU if cx16 isn't supported AMD-Vi: Disable IOMMU if cx16 isn't supported VT-d: Cleanup MAP_SINGLE_DEVICE and related code xen/arch/x86/apic.c | 6 ++ xen/drivers/passthrough/amd/iommu_map.c | 42 -- xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 ++ xen/drivers/passthrough/vtd/intremap.c | 65 --- xen/drivers/passthrough/vtd/iommu.c | 92 +++-- xen/drivers/passthrough/vtd/vtd.h | 5 +- 6 files changed, 71 insertions(+), 145 deletions(-) -- 2.44.0 Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[XEN PATCH v2 2/3] AMD-Vi: Disable IOMMU if cx16 isn't supported
No hardware has AMD-Vi support while not having cx16 support, disable IOMMU
in this case to avoid potentially buggy code.
Now that IOMMU is only enabled if cx16 is supported, drop dead code that
handles cases where cx16 isn't supported.
Suggested-by: Andrew Cooper
Signed-off-by: Teddy Astie
---
xen/drivers/passthrough/amd/iommu_map.c | 42 +++--
xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 +++
2 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c
b/xen/drivers/passthrough/amd/iommu_map.c
index e0f4fe736a..f67975e700 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -167,15 +167,14 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
{
bool valid = flags & SET_ROOT_VALID;
-if ( dte->v && dte->tv &&
- (cpu_has_cx16 || (flags & SET_ROOT_WITH_UNITY_MAP)) )
+if ( dte->v && dte->tv )
{
union {
struct amd_iommu_dte dte;
uint64_t raw64[4];
__uint128_t raw128[2];
} ldte = { .dte = *dte };
-__uint128_t old = ldte.raw128[0];
+__uint128_t res, old = ldte.raw128[0];
int ret = 0;
ldte.dte.domain_id = domain_id;
@@ -185,33 +184,20 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
ldte.dte.paging_mode = paging_mode;
ldte.dte.v = valid;
-if ( cpu_has_cx16 )
-{
-__uint128_t res = cmpxchg16b(dte, , [0]);
+res = cmpxchg16b(dte, , [0]);
-/*
- * Hardware does not update the DTE behind our backs, so the
- * return value should match "old".
- */
-if ( res != old )
-{
-printk(XENLOG_ERR
- "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
- domain_id,
- (uint64_t)(res >> 64), (uint64_t)res,
- (uint64_t)(old >> 64), (uint64_t)old);
-ret = -EILSEQ;
-}
-}
-else /* Best effort, updating domain_id last. */
+/*
+ * Hardware does not update the DTE behind our backs, so the
+ * return value should match "old".
+ */
+if ( res != old )
{
-uint64_t *ptr = (void *)dte;
-
-write_atomic(ptr + 0, ldte.raw64[0]);
-/* No barrier should be needed between these two. */
-write_atomic(ptr + 1, ldte.raw64[1]);
-
-ret = 1;
+printk(XENLOG_ERR
+ "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
+ domain_id,
+ (uint64_t)(res >> 64), (uint64_t)res,
+ (uint64_t)(old >> 64), (uint64_t)old);
+ret = -EILSEQ;
}
return ret;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index f6efd88e36..656c5eda5d 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -312,6 +312,12 @@ static int __init cf_check iov_detect(void)
return -ENODEV;
}
+if ( unlikely(!cpu_has_cx16) )
+{
+printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
+ return -ENOSYS;
+}
+
init_done = 1;
if ( !amd_iommu_perdev_intremap )
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH 3/3] VT-d: Cleanup MAP_SINGLE_DEVICE and related dead code.
This flag was only used in case cx16 is not available, as those code paths no
longer exist, this flag now does basically nothing.
Signed-off-by Teddy Astie
---
xen/drivers/passthrough/vtd/iommu.c | 12 +++-
xen/drivers/passthrough/vtd/vtd.h | 5 ++---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 47b56f37a9..4b15e6da79 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1692,15 +1692,9 @@ static int domain_context_mapping(struct domain *domain,
u8 devfn,
break;
}
-if ( domain != pdev->domain && pdev->domain != dom_io )
-{
-if ( pdev->domain->is_dying )
-mode |= MAP_OWNER_DYING;
-else if ( drhd &&
- !any_pdev_behind_iommu(pdev->domain, pdev, drhd->iommu) &&
- !pdev->phantom_stride )
-mode |= MAP_SINGLE_DEVICE;
-}
+if ( domain != pdev->domain && pdev->domain != dom_io &&
+ pdev->domain->is_dying )
+mode |= MAP_OWNER_DYING;
switch ( pdev->type )
{
diff --git a/xen/drivers/passthrough/vtd/vtd.h
b/xen/drivers/passthrough/vtd/vtd.h
index cb2df76eed..43f06a353d 100644
--- a/xen/drivers/passthrough/vtd/vtd.h
+++ b/xen/drivers/passthrough/vtd/vtd.h
@@ -28,9 +28,8 @@
*/
#define MAP_WITH_RMRR (1u << 0)
#define MAP_OWNER_DYING (1u << 1)
-#define MAP_SINGLE_DEVICE (1u << 2)
-#define MAP_ERROR_RECOVERY(1u << 3)
-#define UNMAP_ME_PHANTOM_FUNC (1u << 4)
+#define MAP_ERROR_RECOVERY(1u << 2)
+#define UNMAP_ME_PHANTOM_FUNC (1u << 3)
/* Allow for both IOAPIC and IOSAPIC. */
#define IO_xAPIC_route_entry IO_APIC_route_entry
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH 1/3] VT-d: Disable IOMMU if cx16 isn't supported
No hardware has VT-d support while not having cx16 support, consider disabling
IOMMU in this case to avoid potentially buggy code.
Now that IOMMU is only enabled if cx16 is supported, drop dead code that
handles cases where cx16 isn't supported.
Signed-off-by Teddy Astie
---
xen/drivers/passthrough/vtd/intremap.c | 67 +
xen/drivers/passthrough/vtd/iommu.c| 80 +-
2 files changed, 41 insertions(+), 106 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/intremap.c
b/xen/drivers/passthrough/vtd/intremap.c
index c504852eb8..312b73e693 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -173,47 +173,26 @@ bool __init cf_check intel_iommu_supports_eim(void)
* Assume iremap_lock has been acquired. It is to make sure software will not
* change the same IRTE behind us. With this assumption, if only high qword or
* low qword in IRTE is to be updated, this function's atomic variant can
- * present an atomic update to VT-d hardware even when cmpxchg16b
- * instruction is not supported.
+ * present an atomic update to VT-d hardware.
*/
static void update_irte(struct vtd_iommu *iommu, struct iremap_entry *entry,
const struct iremap_entry *new_ire, bool atomic)
{
-ASSERT(spin_is_locked(>intremap.lock));
-
-if ( cpu_has_cx16 )
-{
-__uint128_t ret;
-struct iremap_entry old_ire;
+__uint128_t ret;
+struct iremap_entry old_ire;
-old_ire = *entry;
-ret = cmpxchg16b(entry, _ire, new_ire);
+ASSERT(spin_is_locked(>intremap.lock));
+
+old_ire = *entry;
+ret = cmpxchg16b(entry, _ire, new_ire);
-/*
- * In the above, we use cmpxchg16 to atomically update the 128-bit
- * IRTE, and the hardware cannot update the IRTE behind us, so
- * the return value of cmpxchg16 should be the same as old_ire.
- * This ASSERT validate it.
- */
-ASSERT(ret == old_ire.val);
-}
-else
-{
-/*
- * VT-d hardware doesn't update IRTEs behind us, nor the software
- * since we hold iremap_lock. If the caller wants VT-d hardware to
- * always see a consistent entry, but we can't meet it, a bug will
- * be raised.
- */
-if ( entry->lo == new_ire->lo )
-write_atomic(>hi, new_ire->hi);
-else if ( entry->hi == new_ire->hi )
-write_atomic(>lo, new_ire->lo);
-else if ( !atomic )
-*entry = *new_ire;
-else
-BUG();
-}
+/*
+ * In the above, we use cmpxchg16 to atomically update the 128-bit
+ * IRTE, and the hardware cannot update the IRTE behind us, so
+ * the return value of cmpxchg16 should be the same as old_ire.
+ * This ASSERT validate it.
+ */
+ASSERT(ret == old_ire.val);
}
/* Mark specified intr remap entry as free */
@@ -394,8 +373,7 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu
*iommu,
remap_rte->reserved = 0;
/* Indicate remap format. */
remap_rte->format = 1;
-
-/* If cmpxchg16b is not available the caller must mask the IO-APIC pin. */
+
update_irte(iommu, iremap_entry, _ire, !init && !masked);
iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
iommu_flush_iec_index(iommu, 0, index);
@@ -437,21 +415,6 @@ void cf_check io_apic_write_remap_rte(
bool masked = true;
int rc;
-if ( !cpu_has_cx16 )
-{
- /*
-* Cannot atomically update the IRTE entry: mask the IO-APIC pin to
-* avoid interrupts seeing an inconsistent IRTE entry.
-*/
-old_rte = __ioapic_read_entry(apic, pin, true);
-if ( !old_rte.mask )
-{
-masked = false;
-old_rte.mask = 1;
-__ioapic_write_entry(apic, pin, true, old_rte);
-}
-}
-
/* Not the initializer, for old gcc to cope. */
new_rte.raw = rte;
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index c7110af7c9..47b56f37a9 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1482,7 +1482,7 @@ int domain_context_mapping_one(
{
struct domain_iommu *hd = dom_iommu(domain);
struct context_entry *context, *context_entries, lctxt;
-__uint128_t old;
+__uint128_t res, old;
uint64_t maddr;
uint16_t seg = iommu->drhd->segment, prev_did = 0;
struct domain *prev_dom = NULL;
@@ -1580,55 +1580,23 @@ int domain_context_mapping_one(
ASSERT(!context_fault_disable(lctxt));
}
-if ( cpu_has_cx16 )
-{
-__uint128_t res = cmpxchg16b(context, , );
+res = cmpxchg16b(context, , );
-/*
- * Hardware does not update the context entry behind our backs,
- * so the return value should match "old".
- */
[XEN PATCH 2/3] AMD-Vi: Disable IOMMU if cx16 isn't supported
No hardware has VT-d support while not having cx16 support, consider disabling
IOMMU in this case to avoid potentially buggy code.
Now that IOMMU is only enabled if cx16 is supported, drop dead code that
handles cases where cx16 isn't supported.
Signed-off-by Teddy Astie
---
xen/drivers/passthrough/amd/iommu_map.c | 43 +++--
xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 +++
2 files changed, 21 insertions(+), 28 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c
b/xen/drivers/passthrough/amd/iommu_map.c
index e0f4fe736a..c8c1c0cfae 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -167,15 +167,14 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
{
bool valid = flags & SET_ROOT_VALID;
-if ( dte->v && dte->tv &&
- (cpu_has_cx16 || (flags & SET_ROOT_WITH_UNITY_MAP)) )
+if ( dte->v && dte->tv )
{
union {
struct amd_iommu_dte dte;
uint64_t raw64[4];
__uint128_t raw128[2];
} ldte = { .dte = *dte };
-__uint128_t old = ldte.raw128[0];
+__uint128_t res, old = ldte.raw128[0];
int ret = 0;
ldte.dte.domain_id = domain_id;
@@ -185,33 +184,21 @@ int amd_iommu_set_root_page_table(struct amd_iommu_dte
*dte,
ldte.dte.paging_mode = paging_mode;
ldte.dte.v = valid;
-if ( cpu_has_cx16 )
-{
-__uint128_t res = cmpxchg16b(dte, , [0]);
+
+res = cmpxchg16b(dte, , [0]);
-/*
- * Hardware does not update the DTE behind our backs, so the
- * return value should match "old".
- */
-if ( res != old )
-{
-printk(XENLOG_ERR
- "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
- domain_id,
- (uint64_t)(res >> 64), (uint64_t)res,
- (uint64_t)(old >> 64), (uint64_t)old);
-ret = -EILSEQ;
-}
-}
-else /* Best effort, updating domain_id last. */
+/*
+ * Hardware does not update the DTE behind our backs, so the
+ * return value should match "old".
+ */
+if ( res != old )
{
-uint64_t *ptr = (void *)dte;
-
-write_atomic(ptr + 0, ldte.raw64[0]);
-/* No barrier should be needed between these two. */
-write_atomic(ptr + 1, ldte.raw64[1]);
-
-ret = 1;
+printk(XENLOG_ERR
+ "Dom%d: unexpected DTE %016lx_%016lx (expected
%016lx_%016lx)\n",
+ domain_id,
+ (uint64_t)(res >> 64), (uint64_t)res,
+ (uint64_t)(old >> 64), (uint64_t)old);
+ret = -EILSEQ;
}
return ret;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index f6efd88e36..656c5eda5d 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -312,6 +312,12 @@ static int __init cf_check iov_detect(void)
return -ENODEV;
}
+if ( unlikely(!cpu_has_cx16) )
+{
+printk("AMD-Vi: CPU doesn't support CMPXCHG16B, disabling\n");
+ return -ENOSYS;
+}
+
init_done = 1;
if ( !amd_iommu_perdev_intremap )
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[XEN PATCH 0/3] x86/iommu: Drop IOMMU support when cpu doesn't support cx16.
All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside specifically crafted virtual machines). Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported while cx16 isn't, those paths may be bugged and are barely tested, dead code in practice. Consider disabling IOMMU in case we have IOMMU hardware but no cx16, then cleanup no-cx16 handling logic from VT-d and AMD-Vi drivers. Teddy Astie (3): VT-d: Disable IOMMU if cx16 isn't supported AMD-Vi: Disable IOMMU if cx16 isn't supported VT-d: Cleanup MAP_SINGLE_DEVICE and related dead code. xen/drivers/passthrough/amd/iommu_map.c | 43 -- xen/drivers/passthrough/amd/pci_amd_iommu.c | 6 ++ xen/drivers/passthrough/vtd/intremap.c | 67 --- xen/drivers/passthrough/vtd/iommu.c | 92 +++-- xen/drivers/passthrough/vtd/vtd.h | 5 +- 5 files changed, 67 insertions(+), 146 deletions(-) -- 2.44.0 Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
