On 12.11.2025 16:37, Teddy Astie wrote: > amd_iommu_set_root_page_table chooses between updating atomically > and non-atomically depending on whether the DTE is active or not. > > This choice existed mostly because cx16 wasn't supposed always available > until [1]. Thus we don't need to threat the non-atomic path in a special > way anymore. > > By rearranging slightly the atomic path, we can make it cover all the cases > which improves the code generation at the expense of systematically performing > cmpxchg16b. > > Also remove unused raw64 fields of ldte, and the deprecated comment as the > function actually behaves in a more usual way and can't return >0. > > [1] 2636fcdc15c7 "x86/iommu: check for CMPXCHG16B when enabling IOMMU" > > Signed-off-by: Teddy Astie <[email protected]> > --- > xen/drivers/passthrough/amd/iommu_map.c | 78 ++++++++----------------- > 1 file changed, 25 insertions(+), 53 deletions(-) > > diff --git a/xen/drivers/passthrough/amd/iommu_map.c > b/xen/drivers/passthrough/amd/iommu_map.c > index 320a2dc64c..e3165d93aa 100644 > --- a/xen/drivers/passthrough/amd/iommu_map.c > +++ b/xen/drivers/passthrough/amd/iommu_map.c > @@ -154,69 +154,41 @@ static void set_iommu_ptes_present(unsigned long pt_mfn, > unmap_domain_page(table); > } > > -/* > - * This function returns > - * - -errno for errors, > - * - 0 for a successful update, atomic when necessary > - * - 1 for a successful but non-atomic update, which may need to be warned > - * about by the caller. > - */ > int amd_iommu_set_root_page_table(struct amd_iommu_dte *dte, > uint64_t root_ptr, uint16_t domain_id, > uint8_t paging_mode, unsigned int flags) > { > bool valid = flags & SET_ROOT_VALID; > > - if ( dte->v && dte->tv ) > - { > - union { > - struct amd_iommu_dte dte; > - uint64_t raw64[4]; > - __uint128_t raw128[2]; > - } ldte = { .dte = *dte }; > - __uint128_t res, old = ldte.raw128[0]; > - int ret = 0; > - > - ldte.dte.domain_id = domain_id; > - ldte.dte.pt_root = paddr_to_pfn(root_ptr); > - ldte.dte.iw = true; > - ldte.dte.ir = true; > - ldte.dte.paging_mode = paging_mode; > - ldte.dte.v = valid; > - > - 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; > - } > + union { > + struct amd_iommu_dte dte; > + __uint128_t raw128[2]; > + } ldte = { .dte = *dte }; > + __uint128_t res, old = ldte.raw128[0]; > > - return ret; > - } > + ldte.dte.domain_id = domain_id; > + ldte.dte.pt_root = paddr_to_pfn(root_ptr); > + ldte.dte.iw = true; > + ldte.dte.ir = true; > + ldte.dte.paging_mode = paging_mode; > + ldte.dte.tv = true; > + ldte.dte.v = valid; > + > + res = cmpxchg16b(dte, &old, &ldte.raw128[0]); > > - if ( valid || dte->v ) > + /* > + * Hardware does not update the DTE behind our backs, so the > + * return value should match "old". > + */ > + if ( res != old ) > { > - dte->tv = false; > - dte->v = true; > - smp_wmb(); > + 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);
Indentation is now off by 1 here. > + return -EILSEQ; The downside of this is that all updates can now take this path. Yes, this shouldn't be possible to be taken, but it's a (minor) concern nevertheless. At the very least such a downside wants, imo, mentioning in the description, even if for nothing else than to make clear that it was a deliberate choice. Jan
