Re: [PATCH v2] xen/arm: fix arm_iommu_map_page after f9f6b22ab

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 7/12/25 06:08, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/07/2025 01:25, Stefano Stabellini wrote:
>> diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c 
>> b/xen/drivers/passthrough/arm/iommu_helpers.c
>> index 5cb1987481..dae5fc0caa 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));
>> -
> 
> Shouldn't arm_iommu_unmap_page() also be updated? It would not result to a 
> crash, but we would not be able to
> remove the mapping.

f9f6b22abf1d didn't add any calls to iommu_unmap(). As this is still
only hwdom for now, hwdom is not expected to be destroyed, so the
mapping is not expected to be removed for now.

With that said, in the future when we expose vITS to domU, you'd be
right. In the xlnx downstream we have something like this:

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ef8bd4b6ab33..2f5b79279ff9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -133,7 +133,8 @@ static inline int p2m_remove_mapping(struct domain *d,
         mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i), &t, NULL,
                                          &cur_order, NULL);
 
-        if ( p2m_is_any_ram(t) &&
+        if ( !mfn_eq(mfn, INVALID_MFN) &&
+             p2m_is_any_ram(t) &&
              (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) )
         {
             rc = -EILSEQ;
diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c 
b/xen/drivers/passthrough/arm/iommu_helpers.c
index 04b7e98e4aae..d59a9cf5a48b 100644
--- a/xen/drivers/passthrough/arm/iommu_helpers.c
+++ b/xen/drivers/passthrough/arm/iommu_helpers.c
@@ -63,14 +63,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) )
-        return -EINVAL;
-
-    return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),
+    return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), INVALID_MFN,
                                      order);
 }