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

[Julien Grall <julien@xxxxxxx>]

Hi Stewards,

On 14/07/2025 22:12, Stewart Hildebrand wrote:
> 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.
I already gathered that by looking at the fixes tag in my previous 
answer. Maybe I should have been clearer at that point. Even though 
iommu_unmap() is not called today, this is meant to be the reverse of 
what was done by iommu_map(). So it looks very odd to update one but not 
the other.
Furthermore, AFAIU, this patch is going a bit further than just fixing 
the bug introduced by f9f6b22abf1d. At which point, we should properly
fix it in the same patch rather than hoping that someone else will 
remember that this needed be updated.
> 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) &&
I don't quite understand why you need to update this function. Can you 
clarify?
>                (!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);
>   }
Cheers,

--
Julien Grall