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 <andrew.cooper3@citrix.com>
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
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, &old, &ldte.raw128[0]);
+ res = cmpxchg16b(dte, &old, &ldte.raw128[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, &old, &lctxt.full);
-
- /*
- * 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",
- &PCI_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, &old, &lctxt.full);
- write_atomic(&context->hi, lctxt.hi);
- /* No barrier should be needed between these two. */
- write_atomic(&context->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",
- &PCI_SBDF(seg, bus, devfn), prev_dom);
-
- write_atomic(&context->lo, lctxt.lo);
- /* No barrier should be needed between these two. */
- write_atomic(&context->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",
+ &PCI_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(&acpi_drhd_units) )
{
ret = -ENODEV;
--
2.44.0
Teddy Astie | Vates XCP-ng Intern
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.techOn 18.04.2024 13:57, Teddy Astie wrote:
> --- 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;
> + }
One additional nit: Here you neatly have just AMD-Vi: as a prefix for the
log message. Why ...
> @@ -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;
> + }
... both VTDPREFIX and "IOMMU:" here?
Jan
On 18.04.2024 13:57, Teddy Astie wrote:
> 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.
As before: What about Xen itself running virtualized, and the underlying
hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
IOMMU in such an event, but by not mentioning the case you give the
appearance of not having considered it at all.
> --- 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 )
> {
I did previously point out (and that's even visible in patch context here)
that the per-vendor .setup() hooks aren't necessarily the first thing to
run. Please can you make sure you address (verbally or by code) prior to
submitting new versions?
Jan
On Wed, Apr 24, 2024 at 04:27:10PM +0200, Jan Beulich wrote:
> On 18.04.2024 13:57, Teddy Astie wrote:
> > 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.
>
> As before: What about Xen itself running virtualized, and the underlying
> hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
> IOMMU in such an event, but by not mentioning the case you give the
> appearance of not having considered it at all.
>
> > --- 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 )
> > {
>
> I did previously point out (and that's even visible in patch context here)
> that the per-vendor .setup() hooks aren't necessarily the first thing to
> run. Please can you make sure you address (verbally or by code) prior to
> submitting new versions?
I've re-visiting this as part of my other IOMMU IRTE atomic update
fix.
Would you prefer the check for CX16 be in the common x86
iommu_hardware_setup()? That would be kind of layering violation, as
in principle a further IOMMU implementation on x86 might not require
CX16. However I find it very unlikely, and hence I would be fine in
placing the check in iommu_hardware_setup() if you prefer it there.
Thanks, Roger.
On 21.01.2025 12:09, Roger Pau Monné wrote:
> On Wed, Apr 24, 2024 at 04:27:10PM +0200, Jan Beulich wrote:
>> On 18.04.2024 13:57, Teddy Astie wrote:
>>> 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.
>>
>> As before: What about Xen itself running virtualized, and the underlying
>> hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
>> IOMMU in such an event, but by not mentioning the case you give the
>> appearance of not having considered it at all.
>>
>>> --- 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 )
>>> {
>>
>> I did previously point out (and that's even visible in patch context here)
>> that the per-vendor .setup() hooks aren't necessarily the first thing to
>> run. Please can you make sure you address (verbally or by code) prior to
>> submitting new versions?
>
> I've re-visiting this as part of my other IOMMU IRTE atomic update
> fix.
>
> Would you prefer the check for CX16 be in the common x86
> iommu_hardware_setup()? That would be kind of layering violation, as
> in principle a further IOMMU implementation on x86 might not require
> CX16. However I find it very unlikely, and hence I would be fine in
> placing the check in iommu_hardware_setup() if you prefer it there.
No. The check needs replicating into the other hook that may run first,
->supports_x2apic(). And the ->enable_x2apic() hooks may want to gain
respective assertions then.
Jan
On Tue, Jan 21, 2025 at 12:20:17PM +0100, Jan Beulich wrote:
> On 21.01.2025 12:09, Roger Pau Monné wrote:
> > On Wed, Apr 24, 2024 at 04:27:10PM +0200, Jan Beulich wrote:
> >> On 18.04.2024 13:57, Teddy Astie wrote:
> >>> 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.
> >>
> >> As before: What about Xen itself running virtualized, and the underlying
> >> hypervisor surfacing an IOMMU but not CX16? It may be okay to ignore the
> >> IOMMU in such an event, but by not mentioning the case you give the
> >> appearance of not having considered it at all.
> >>
> >>> --- 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 )
> >>> {
> >>
> >> I did previously point out (and that's even visible in patch context here)
> >> that the per-vendor .setup() hooks aren't necessarily the first thing to
> >> run. Please can you make sure you address (verbally or by code) prior to
> >> submitting new versions?
> >
> > I've re-visiting this as part of my other IOMMU IRTE atomic update
> > fix.
> >
> > Would you prefer the check for CX16 be in the common x86
> > iommu_hardware_setup()? That would be kind of layering violation, as
> > in principle a further IOMMU implementation on x86 might not require
> > CX16. However I find it very unlikely, and hence I would be fine in
> > placing the check in iommu_hardware_setup() if you prefer it there.
>
> No. The check needs replicating into the other hook that may run first,
> ->supports_x2apic(). And the ->enable_x2apic() hooks may want to gain
> respective assertions then.
Oh, I see. So you either want patch 3 ahead of this, or both patches
merged into a single one.
Thanks, Roger.
© 2016 - 2026 Red Hat, Inc.