[PATCH v4 1/2] xen/arm: fix arm_iommu_map_page after f9f6b22abf1d

[PATCH v4 1/2] xen/arm: fix arm_iommu_map_page after f9f6b22abf1d

[Stewart Hildebrand]

From: Stefano Stabellini <stefano.stabellini@amd.com>

Up until f9f6b22abf1d "xen/arm: Map ITS doorbell register to IOMMU page
tables" the only caller of iommu_map on ARM was grant_table.c which has
a specific usage model and restrictions as described by the in-code
comment in arm_iommu_map_page.

f9f6b22abf1d introduced a second caller to iommu_map on ARM:
vgic_v3_its_init_virtual. This specific statement in the
f9f6b22abf1d commit message is partially wrong:

"Note that the 1:1 check in arm_iommu_map_page remains for now, as
virtual ITSes are currently only created for hwdom where the doorbell
mapping is always 1:1."

Leading to crashes any time the hardware domain is not direct-mapped
(e.g. cache coloring and non-Dom0 hardware domain):

(XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:47
[...]
(XEN) Xen call trace:
(XEN)    [<00000a000024c758>] arm_iommu_map_page+0x80/0x90 (PC)
(XEN)    [<00000a000024c750>] arm_iommu_map_page+0x78/0x90 (LR)
(XEN)    [<00000a0000250884>] iommu_map+0xcc/0x29c
(XEN)    [<00000a0000288024>] vgic_v3_its_init_domain+0x18c/0x1e8
(XEN)    [<00000a0000285228>] vgic-v3.c#vgic_v3_domain_init+0x168/0x21c
(XEN)    [<00000a0000281dcc>] domain_vgic_init+0x14c/0x210
(XEN)    [<00000a00002705a4>] arch_domain_create+0x150/0x1f0
(XEN)    [<00000a00002055e8>] domain_create+0x47c/0x6c0
(XEN)    [<00000a00002cf090>] create_domUs+0x7f8/0x8cc
(XEN)    [<00000a00002eb588>] start_xen+0x8f4/0x998
(XEN)    [<00000a000020018c>] head.o#primary_switched+0x4/0x10

Specifically, non-1:1 hardware domain exists with cache coloring
enabled. For that, is_domain_direct_mapped(d) is false but
domain_use_host_layout(d) is true.

Change the is_domain_direct_mapped(d) checks in arm_iommu_map_page and
arm_iommu_unmap_page into domain_use_host_layout(d) checks.

Move the in-code comment specific to the grant table to grant-table.c
and adjust to be architecture-neutral.

Fixes: f9f6b22abf1d ("xen/arm: Map ITS doorbell register to IOMMU page tables")
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* adjust comment to be architecture-neutral

v2->v3:
* split change into 2 patches
* 12-character commit IDs
* change both arm_iommu_map_page and arm_iommu_unmap_page
* s/IPA/GFN/ in comment
---
 xen/common/grant_table.c                    |  5 +++++
 xen/drivers/passthrough/arm/iommu_helpers.c | 16 ++--------------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index cf131c43a1f1..1b7f63a448a7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1274,6 +1274,11 @@ map_grant_ref(
         }
 
         /*
+         * Grant mappings can be used for DMA requests. The dev_bus_addr
+         * returned by the hypercall is the MFN (not the GFN). For device
+         * protected by an IOMMU, Xen needs to add a 1:1 mapping in the domain
+         * p2m to allow DMA request to work.
+         *
          * We're not translated, so we know that dfns and mfns are
          * the same things, so the IOMMU entry is always 1-to-1.
          */
diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c b/xen/drivers/passthrough/arm/iommu_helpers.c
index 5cb198748193..bdb271584b0d 100644
--- a/xen/drivers/passthrough/arm/iommu_helpers.c
+++ b/xen/drivers/passthrough/arm/iommu_helpers.c
@@ -36,15 +36,7 @@ int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
 {
     p2m_type_t t;
 
-    /*
-     * Grant mappings can be used for DMA requests. The dev_bus_addr
-     * returned by the hypercall is the MFN (not the IPA). For device
-     * protected by an IOMMU, Xen needs to add a 1:1 mapping in the domain
-     * p2m to allow DMA request to work.
-     * This is only valid when the domain is directed mapped. Hence this
-     * function should only be used by gnttab code with gfn == mfn == dfn.
-     */
-    BUG_ON(!is_domain_direct_mapped(d));
+    BUG_ON(!domain_use_host_layout(d));
     BUG_ON(mfn_x(mfn) != dfn_x(dfn));
 
     /* We only support readable and writable flags */
@@ -66,11 +58,7 @@ int __must_check arm_iommu_unmap_page(struct domain *d, dfn_t dfn,
                                       unsigned int order,
                                       unsigned int *flush_flags)
 {
-    /*
-     * This function should only be used by gnttab code when the domain
-     * is direct mapped (i.e. gfn == mfn == dfn).
-     */
-    if ( !is_domain_direct_mapped(d) )
+    if ( !domain_use_host_layout(d) )
         return -EINVAL;
 
     return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),

base-commit: 5c798ac8854af3528a78ca5a622c9ea68399809b
-- 
2.50.1

[PATCH v4 2/2] xen/arm: allow translated iommu mappings

[Stewart Hildebrand]

From: Stefano Stabellini <stefano.stabellini@amd.com>

In preparation of exposing vITS to domUs, generalize arm_iommu_map_page
and arm_iommu_unmap_page to allow ITS doorbell mappings with dfn != mfn.

The mfn does not need to be passed to guest_physmap_remove_page since
there is no mfn checking on the p2m_iommu_map_{rw,ro} p2m types during
unmap. Pass INVALID_MFN to guest_physmap_remove_page.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* no change

v2->v3:
* split change from ("xen/arm: fix arm_iommu_map_page after f9f6b22ab")
* both map and unmap
---
 xen/drivers/passthrough/arm/iommu_helpers.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c b/xen/drivers/passthrough/arm/iommu_helpers.c
index bdb271584b0d..a3e3cafb8705 100644
--- a/xen/drivers/passthrough/arm/iommu_helpers.c
+++ b/xen/drivers/passthrough/arm/iommu_helpers.c
@@ -36,9 +36,6 @@ int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
 {
     p2m_type_t t;
 
-    BUG_ON(!domain_use_host_layout(d));
-    BUG_ON(mfn_x(mfn) != dfn_x(dfn));
-
     /* We only support readable and writable flags */
     if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
         return -EINVAL;
@@ -49,7 +46,7 @@ int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
      * The function guest_physmap_add_entry replaces the current mapping
      * if there is already one...
      */
-    return guest_physmap_add_entry(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),
+    return guest_physmap_add_entry(d, _gfn(dfn_x(dfn)), mfn,
                                    IOMMUF_order(flags), t);
 }
 
@@ -58,11 +55,7 @@ int __must_check arm_iommu_unmap_page(struct domain *d, dfn_t dfn,
                                       unsigned int order,
                                       unsigned int *flush_flags)
 {
-    if ( !domain_use_host_layout(d) )
-        return -EINVAL;
-
-    return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),
-                                     order);
+    return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), INVALID_MFN, order);
 }
 
 /*
-- 
2.50.1

Re: [PATCH v4 1/2] xen/arm: fix arm_iommu_map_page after f9f6b22abf1d

[Jan Beulich]

On 23.07.2025 20:13, Stewart Hildebrand wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> Up until f9f6b22abf1d "xen/arm: Map ITS doorbell register to IOMMU page
> tables" the only caller of iommu_map on ARM was grant_table.c which has
> a specific usage model and restrictions as described by the in-code
> comment in arm_iommu_map_page.
> 
> f9f6b22abf1d introduced a second caller to iommu_map on ARM:
> vgic_v3_its_init_virtual. This specific statement in the
> f9f6b22abf1d commit message is partially wrong:
> 
> "Note that the 1:1 check in arm_iommu_map_page remains for now, as
> virtual ITSes are currently only created for hwdom where the doorbell
> mapping is always 1:1."
> 
> Leading to crashes any time the hardware domain is not direct-mapped
> (e.g. cache coloring and non-Dom0 hardware domain):
> 
> (XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:47
> [...]
> (XEN) Xen call trace:
> (XEN)    [<00000a000024c758>] arm_iommu_map_page+0x80/0x90 (PC)
> (XEN)    [<00000a000024c750>] arm_iommu_map_page+0x78/0x90 (LR)
> (XEN)    [<00000a0000250884>] iommu_map+0xcc/0x29c
> (XEN)    [<00000a0000288024>] vgic_v3_its_init_domain+0x18c/0x1e8
> (XEN)    [<00000a0000285228>] vgic-v3.c#vgic_v3_domain_init+0x168/0x21c
> (XEN)    [<00000a0000281dcc>] domain_vgic_init+0x14c/0x210
> (XEN)    [<00000a00002705a4>] arch_domain_create+0x150/0x1f0
> (XEN)    [<00000a00002055e8>] domain_create+0x47c/0x6c0
> (XEN)    [<00000a00002cf090>] create_domUs+0x7f8/0x8cc
> (XEN)    [<00000a00002eb588>] start_xen+0x8f4/0x998
> (XEN)    [<00000a000020018c>] head.o#primary_switched+0x4/0x10
> 
> Specifically, non-1:1 hardware domain exists with cache coloring
> enabled. For that, is_domain_direct_mapped(d) is false but
> domain_use_host_layout(d) is true.
> 
> Change the is_domain_direct_mapped(d) checks in arm_iommu_map_page and
> arm_iommu_unmap_page into domain_use_host_layout(d) checks.
> 
> Move the in-code comment specific to the grant table to grant-table.c
> and adjust to be architecture-neutral.
> 
> Fixes: f9f6b22abf1d ("xen/arm: Map ITS doorbell register to IOMMU page tables")
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v3->v4:
> * adjust comment to be architecture-neutral

Hmm, it's now arch-neutral, but still not quite correct.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1274,6 +1274,11 @@ map_grant_ref(
>          }
>  
>          /*
> +         * Grant mappings can be used for DMA requests. The dev_bus_addr
> +         * returned by the hypercall is the MFN (not the GFN). For device
> +         * protected by an IOMMU, Xen needs to add a 1:1 mapping in the domain
> +         * p2m to allow DMA request to work.
> +         *
>           * We're not translated, so we know that dfns and mfns are
>           * the same things, so the IOMMU entry is always 1-to-1.
>           */

The original comment, for a reason, talks about DFN, not GFN. The relationship
to P2M (where GFNs might indeed matter) also isn't quite clear to me:
iommu_legacy_map() alters IOMMU mappings. Which may or may not be shared with
CPU ones.

Fundamental question: What exactly is insufficient in the comment that's there
already?

Jan

Re: [PATCH v4 1/2] xen/arm: fix arm_iommu_map_page after f9f6b22abf1d

[Stewart Hildebrand]

On 7/24/25 04:07, Jan Beulich wrote:
> On 23.07.2025 20:13, Stewart Hildebrand wrote:
>> From: Stefano Stabellini <stefano.stabellini@amd.com>
>>
>> Up until f9f6b22abf1d "xen/arm: Map ITS doorbell register to IOMMU page
>> tables" the only caller of iommu_map on ARM was grant_table.c which has
>> a specific usage model and restrictions as described by the in-code
>> comment in arm_iommu_map_page.
>>
>> f9f6b22abf1d introduced a second caller to iommu_map on ARM:
>> vgic_v3_its_init_virtual. This specific statement in the
>> f9f6b22abf1d commit message is partially wrong:
>>
>> "Note that the 1:1 check in arm_iommu_map_page remains for now, as
>> virtual ITSes are currently only created for hwdom where the doorbell
>> mapping is always 1:1."
>>
>> Leading to crashes any time the hardware domain is not direct-mapped
>> (e.g. cache coloring and non-Dom0 hardware domain):
>>
>> (XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:47
>> [...]
>> (XEN) Xen call trace:
>> (XEN)    [<00000a000024c758>] arm_iommu_map_page+0x80/0x90 (PC)
>> (XEN)    [<00000a000024c750>] arm_iommu_map_page+0x78/0x90 (LR)
>> (XEN)    [<00000a0000250884>] iommu_map+0xcc/0x29c
>> (XEN)    [<00000a0000288024>] vgic_v3_its_init_domain+0x18c/0x1e8
>> (XEN)    [<00000a0000285228>] vgic-v3.c#vgic_v3_domain_init+0x168/0x21c
>> (XEN)    [<00000a0000281dcc>] domain_vgic_init+0x14c/0x210
>> (XEN)    [<00000a00002705a4>] arch_domain_create+0x150/0x1f0
>> (XEN)    [<00000a00002055e8>] domain_create+0x47c/0x6c0
>> (XEN)    [<00000a00002cf090>] create_domUs+0x7f8/0x8cc
>> (XEN)    [<00000a00002eb588>] start_xen+0x8f4/0x998
>> (XEN)    [<00000a000020018c>] head.o#primary_switched+0x4/0x10
>>
>> Specifically, non-1:1 hardware domain exists with cache coloring
>> enabled. For that, is_domain_direct_mapped(d) is false but
>> domain_use_host_layout(d) is true.
>>
>> Change the is_domain_direct_mapped(d) checks in arm_iommu_map_page and
>> arm_iommu_unmap_page into domain_use_host_layout(d) checks.
>>
>> Move the in-code comment specific to the grant table to grant-table.c
>> and adjust to be architecture-neutral.
>>
>> Fixes: f9f6b22abf1d ("xen/arm: Map ITS doorbell register to IOMMU page tables")
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> v3->v4:
>> * adjust comment to be architecture-neutral
> 
> Hmm, it's now arch-neutral, but still not quite correct.
> 
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -1274,6 +1274,11 @@ map_grant_ref(
>>          }
>>  
>>          /*
>> +         * Grant mappings can be used for DMA requests. The dev_bus_addr
>> +         * returned by the hypercall is the MFN (not the GFN). For device
>> +         * protected by an IOMMU, Xen needs to add a 1:1 mapping in the domain
>> +         * p2m to allow DMA request to work.
>> +         *
>>           * We're not translated, so we know that dfns and mfns are
>>           * the same things, so the IOMMU entry is always 1-to-1.
>>           */
> 
> The original comment, for a reason, talks about DFN, not GFN. The relationship
> to P2M (where GFNs might indeed matter) also isn't quite clear to me:
> iommu_legacy_map() alters IOMMU mappings. Which may or may not be shared with
> CPU ones.

Ah, you're right, I assumed iommu page tables are always shared with
cpu... A bad assumption, sorry about that.

> 
> Fundamental question: What exactly is insufficient in the comment that's there
> already?

Nothing. It was nothing more than trying to find a new home for the
comment in xen/drivers/passthrough/arm/iommu_helpers.c, but perhaps it's
better to drop the comment altogether and leave xen/common/grant_table.c
unchanged.