Le 13/11/2025 à 12:38, Jan Beulich a écrit : > 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. >
I only expect to see this path in case of a bug (recoverable here), which is only checked in the res != old case. But if something similar occurs in the non-atomic path, then nothing good will happen as such checks cannot be implemented properly. Perhaps we want to emphasis this : "The race check is now always made instead of only being made for the atomic path. This specific path should be triggered under normal circumstances, and is very likely a bug." And wrap the res != old inside a unlikely() to insist on this aspect. > Jan > -- Teddy Astie | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
