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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2025 20:13, Stewart Hildebrand wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> 
> 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@xxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> 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