Re: [RFC] xen/arm: fix arm_iommu_map_page after f9f6b22ab

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 7/9/25 19:28, Stefano Stabellini wrote:
> Hi all,
> 
> Up until f9f6b22ab "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.
> 
> f9f6b22ab introduced a second caller to iommu_map on ARM:
> vgic_v3_its_init_virtual. This specific statement in the
> f9f6b22ab commit message is 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."

The doorbell mapping is still expected to be DFN == MFN for non-direct
map hwdom as far as I understand. In other words,
domain_use_host_layout(d) is true for non-direct-map hwdom, but not
is_domain_direct_mapped(d).

> Leading to crashes any time the hardware domain is not direct-mapped
> (e.g. cache coloring and non-Dom0 hardware domain):
> 
> (XEN) Loading d1 initrd from 0000000045800000 to 
> 0x0000000048200000-0x000000004bdcd615
> (XEN) Loading d1 DTB to 0x0000000048000000-0x00000000480005c4
> (XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:49
> (XEN) ----[ Xen-4.21-unstable  arm64  debug=y  Tainted: I      ]----
> (XEN) CPU:    0
> (XEN) PC:     00000a000024c758 arm_iommu_map_page+0x80/0x90
> (XEN) LR:     00000a000024c750
> (XEN) SP:     00000a000032fb70
> (XEN) CPSR:   0000000080000249 MODE:64-bit EL2h (Hypervisor, handler)
> (XEN)      X0: 0000000000000008  X1: 0000000000000000  X2: 0000000000000000
> (XEN)      X3: 0000000000000001  X4: 00000a00002c98e0  X5: 0000000000000000
> (XEN)      X6: 00008000ffffa710  X7: 00008000ffffa710  X8: 7f7f7f7f7f7f7f7f
> (XEN)      X9: 0000000000000080 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
> (XEN)     X12: 0000000000000008 X13: 0000000000000020 X14: 0000000000000000
> (XEN)     X15: ffffffffffffffff X16: 00000a000033cce8 X17: 0000000000000000
> (XEN)     X18: 000000013e688d90 X19: 0000000000008090 X20: 0000000000000080
> (XEN)     X21: 00008000f3a01000 X22: 0000000000008090 X23: 0000000000000080
> (XEN)     X24: 0000000000008090 X25: 0000000000008090 X26: 0000000000000001
> (XEN)     X27: 0000000000008090 X28: 0000000000000000  FP: 00000a000032fb70
> (XEN)
> (XEN)   VTCR_EL2: 0000000080043594
> (XEN)  VTTBR_EL2: 000000013fdcb000
> (XEN)
> (XEN)  SCTLR_EL2: 0000000030cd183d
> (XEN)    HCR_EL2: 0000000000000039
> (XEN)  TTBR0_EL2: 000000004dd50000
> (XEN)
> (XEN)    ESR_EL2: 00000000f2000001
> (XEN)  HPFAR_EL2: 0000000000000000
> (XEN)    FAR_EL2: 0000000000000000
> (XEN)
> (XEN) Xen stack trace from sp=00000a000032fb70:
> (XEN)    00000a000032fba0 00000a0000250884 0000000000000000 0000000000000000
> (XEN)    00008000ffffa410 00008000f3a01000 00000a000032fc30 00000a0000288024
> (XEN)    00008000f3a07010 00008000f3a01000 00008000ffffa410 0000000008080000
> (XEN)    0000000008090040 00008000f3a01570 0000000000000010 0000000000000010
> (XEN)    0000000000010040 3800000000000400 0000000000000010 00000a000032fc9c
> (XEN)    0000000000008090 00008000f3a01150 00000a0000339388 00000a000033df80
> (XEN)    00000a000032fca0 00000a0000285228 00008000f3a01000 0000000000000001
> (XEN)    0000000000003800 0000000000000008 0000000000000000 0000000000000000
> (XEN)    0000000000000001 0000000000000003 000000013e6c9950 000000013e6c9920
> (XEN)    00008000f3a01000 0000000000000008 00000a000032fce0 00000a0000281dcc
> (XEN)    00008000f3a01000 0000000000000100 00000a000032fce0 00000a0000281d40
> (XEN)    0000000000000000 00000000000000ff 00000a000032fd10 00000a00002705a4
> (XEN)    0000000000000000 00008000f3a01000 00000a000032fe08 00000a0000270578
> (XEN)    00000a000032fd50 00000a00002055e8 00008000f3a01000 00000a000032fe08
> (XEN)    0000000000000000 00000a00002053a8 00008000f3a01000 000000030032fda0
> (XEN)    00000a000032fda0 00000a00002cf090 00008000fffed4c8 0000000000000003
> (XEN)    0000000000000008 00000a00002f1bb8 0000000000000004 0000000000000048
> (XEN)    19170c1f5f0cee0e 0000000000000002 00000a000032fe60 00000a00002eb588
> (XEN)    0000000000000004 00000a00002bc000 00000a00002bd900 00000a00002b7c28
> (XEN)    0000000000000004 0000000000000000 0000000000000000 0000000000000000
> (XEN)    00000a000032fe20 00000000ffffffc8 000000030032fe60 0000000000000000
> (XEN)    0000000000000000 0000012300000000 0000000100000000 00000040ffffffff
> (XEN)    00000001ffffffff 0000000000000000 0000000200000000 0000000000000100
> (XEN)    0000000000000000 00008000ffffa710 000000013e588620 00000a000020018c
> (XEN)    000000004dc00000 fffff6004da00000 000000004de00000 0000000000001710
> (XEN)    000000013e5887a0 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000
> (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
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:49
> (XEN) ****************************************
> (XEN)
> (XEN) Reboot in five seconds...
> 
> Credits go to Jason for spotting this problem.
> 
> It makes sense to generalize the arm_iommu_map_page function to be more
> generic and unopinionated. The comment specific to the grant table can
> live in grant-table.c instead.
> 
> Opinions?
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index cf131c43a1..5c841d4f17 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1274,8 +1274,14 @@ map_grant_ref(
>          }
>  
>          /*
> -         * We're not translated, so we know that dfns and mfns are
> -         * the same things, so the IOMMU entry is always 1-to-1.
> +         * 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.
> +         * 
> +         * We're not translated, so we know that dfns and mfns are the
> +         * same things, so the IOMMU entry is always 1-to-1.
>           */
>          if ( !(op->flags & GNTMAP_readonly) && node.cnt.wr == 1 )
>              kind = IOMMUF_readable | IOMMUF_writable;
> diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c 
> b/xen/drivers/passthrough/arm/iommu_helpers.c
> index 5cb1987481..2585f2e526 100644
> --- a/xen/drivers/passthrough/arm/iommu_helpers.c
> +++ b/xen/drivers/passthrough/arm/iommu_helpers.c
> @@ -36,17 +36,6 @@ 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(mfn_x(mfn) != dfn_x(dfn));

Removing both of these checks is taking it further that minimally
necessary to address the reported issue. It would suffice to replace
is_domain_direct_mapped with domain_use_host_layout. However, removing
both can still be considered desirable as preparation for exposing a
vITS to domUs where the doorbell mapping will be DFN != MFN, but this is
not yet the case upstream. If removing both, I would just ask that it be
described so as to not conflate fixing the reported issue with the
preparation for future work.

> -
>      /* We only support readable and writable flags */
>      if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
>          return -EINVAL;
> @@ -57,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(dfn_x(mfn)),

This can be simplified to:

    return guest_physmap_add_entry(d, _gfn(dfn_x(dfn)), mfn,

>                                     IOMMUF_order(flags), t);
>  }
>