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

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

[Teddy Astie]

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@vates.tech>
---
 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);
+        return -EILSEQ;
     }
-    dte->domain_id = domain_id;
-    dte->pt_root = paddr_to_pfn(root_ptr);
-    dte->iw = true;
-    dte->ir = true;
-    dte->paging_mode = paging_mode;
-    smp_wmb();
-    dte->tv = true;
-    dte->v = valid;
 
     return 0;
 }
-- 
2.51.2



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 0/2] AMD-Vi cleanups around domain device setup

[Teddy Astie]

Various simplifications (redundant logic merging and dead code removal)
with no expected impact on effective behavior.

Teddy Astie (2):
  amd/iommu: Always atomically update DTE
  amd/iommu: Remove dead non-atomic update checking

 xen/drivers/passthrough/amd/iommu_map.c     | 78 +++++++--------------
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 18 -----
 2 files changed, 25 insertions(+), 71 deletions(-)

-- 
2.51.2



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 2/2] amd/iommu: Remove dead non-atomic update checking

[Teddy Astie]

When updating a DTE, amd_iommu_setup_domain_device() would check if
the update had been non-atomic (i.e rc > 0) and throw a warning if
such non-atomic update could be dangerous.  However since commit
3fc44151d83d, rc can no longer be positive, making this branch
unreachable code.

No functional change intended.

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 3a14770855..02eee4e658 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -225,24 +225,6 @@ static int __must_check amd_iommu_setup_domain_device(
             spin_unlock_irqrestore(&iommu->lock, flags);
             return rc;
         }
-        if ( rc &&
-             domain != pdev->domain &&
-             /*
-              * By non-atomically updating the DTE's domain ID field last,
-              * during a short window in time TLB entries with the old domain
-              * ID but the new page tables may have been 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.
-              */
-             !pdev->domain->is_dying &&
-             pdev->domain != dom_io &&
-             (any_pdev_behind_iommu(pdev->domain, pdev, iommu) ||
-              pdev->phantom_stride) )
-            AMD_IOMMU_WARN(" %pp: reassignment may cause %pd data corruption\n",
-                           &PCI_SBDF(pdev->seg, bus, devfn), pdev->domain);
 
         /*
          * Check remaining settings are still in place from an earlier call
-- 
2.51.2



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Jan Beulich]

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@vates.tech>
> ---
>  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

Re: [PATCH 2/2] amd/iommu: Remove dead non-atomic update checking

[Jan Beulich]

On 12.11.2025 16:37, Teddy Astie wrote:
> When updating a DTE, amd_iommu_setup_domain_device() would check if
> the update had been non-atomic (i.e rc > 0) and throw a warning if
> such non-atomic update could be dangerous.  However since commit
> 3fc44151d83d, rc can no longer be positive, making this branch
> unreachable code.

I.e. it addresses a Misra concern and hence ...

> No functional change intended.
> 
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>

... wants at least an Amends: tag, likely a Fixes: one. Then:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 2/2] amd/iommu: Remove dead non-atomic update checking

[Jason Andryuk]

On 2025-11-13 06:37, Jan Beulich wrote:
> On 12.11.2025 16:37, Teddy Astie wrote:
>> When updating a DTE, amd_iommu_setup_domain_device() would check if
>> the update had been non-atomic (i.e rc > 0) and throw a warning if
>> such non-atomic update could be dangerous.  However since commit
>> 3fc44151d83d, rc can no longer be positive, making this branch
>> unreachable code.
> 
> I.e. it addresses a Misra concern and hence ...
> 
>> No functional change intended.
>>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> 
> ... wants at least an Amends: tag, likely a Fixes: one. Then:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

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

[Teddy Astie]

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

Apparently it doesn't build (debian-bookworm-gcc-arm32-randconfig
catched it).
ARM does provide MAX_VIRT_CPUS and GUEST_MAX_VCPUS which is 128 or
lower, but that doesn't map (or not properly) with what we have in x86
(MAX_VIRT_CPUS=8192 is PV-specific, and GUEST_MAX_VCPUS doesn't exist).

I am not sure what to do, looks like many things are redundant here.

> Jan
>



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Teddy Astie]

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

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

[Teddy Astie]

Le 17/11/2025 à 14:05, Teddy Astie a écrit :
> 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 <teddy.astie@vates.tech>
>>> ---
>>>   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.
>>
>
> Apparently it doesn't build (debian-bookworm-gcc-arm32-randconfig
> catched it).
> ARM does provide MAX_VIRT_CPUS and GUEST_MAX_VCPUS which is 128 or
> lower, but that doesn't map (or not properly) with what we have in x86
> (MAX_VIRT_CPUS=8192 is PV-specific, and GUEST_MAX_VCPUS doesn't exist).
>
> I am not sure what to do, looks like many things are redundant here.
>
>> Jan
>>
>

Oops, sent the wrong message. Please ignore.


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Jan Beulich]

On 17.11.2025 14:49, Teddy Astie wrote:
> 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 <teddy.astie@vates.tech>
>>> ---
>>>   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."

s/should/shouldn't/ I suppose?

Jan

> And wrap the res != old inside a unlikely() to insist on this aspect.
> --
> Teddy Astie | Vates XCP-ng Developer
> 
> XCP-ng & Xen Orchestra - Vates solutions
> 
> web: https://vates.tech
> 
> 

Re: [PATCH 2/2] amd/iommu: Remove dead non-atomic update checking

[Teddy Astie]

Le 13/11/2025 à 12:39, Jan Beulich a écrit :
> On 12.11.2025 16:37, Teddy Astie wrote:
>> When updating a DTE, amd_iommu_setup_domain_device() would check if
>> the update had been non-atomic (i.e rc > 0) and throw a warning if
>> such non-atomic update could be dangerous.  However since commit
>> 3fc44151d83d, rc can no longer be positive, making this branch
>> unreachable code.
>
> I.e. it addresses a Misra concern and hence ...
>
>> No functional change intended.
>>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>
> ... wants at least an Amends: tag, likely a Fixes: one. Then:

With :

Fixes: 3fc44151d83d ("x86/iommu: remove non-CX16 logic from DMA remapping")

?

As it is the commit that removed the rc > 0 case.

> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
>



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH 2/2] amd/iommu: Remove dead non-atomic update checking

[Jan Beulich]

On 19.11.2025 11:22, Teddy Astie wrote:
> Le 13/11/2025 à 12:39, Jan Beulich a écrit :
>> On 12.11.2025 16:37, Teddy Astie wrote:
>>> When updating a DTE, amd_iommu_setup_domain_device() would check if
>>> the update had been non-atomic (i.e rc > 0) and throw a warning if
>>> such non-atomic update could be dangerous.  However since commit
>>> 3fc44151d83d, rc can no longer be positive, making this branch
>>> unreachable code.
>>
>> I.e. it addresses a Misra concern and hence ...
>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>
>> ... wants at least an Amends: tag, likely a Fixes: one. Then:
> 
> With :
> 
> Fixes: 3fc44151d83d ("x86/iommu: remove non-CX16 logic from DMA remapping")
> 
> ?
> 
> As it is the commit that removed the rc > 0 case.

Yes, that looks to be the one.

Jan