Re: [PATCH 1/2] amd/iommu: Always atomically update DTE

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <teddy.astie@xxxxxxxxxx>
> ---
>  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