Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Stefano,

> On 6 Feb 2021, at 12:38 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> Commit 91d4eca7add broke gnttab_need_iommu_mapping on ARM.
> The offending chunk is:
> 
> #define gnttab_need_iommu_mapping(d)                    \
> -    (is_domain_direct_mapped(d) && need_iommu(d))
> +    (is_domain_direct_mapped(d) && need_iommu_pt_sync(d))
> 
> On ARM we need gnttab_need_iommu_mapping to be true for dom0 when it is
> directly mapped, like the old check did, but the new check is always
> false.
> 
> In fact, need_iommu_pt_sync is defined as dom_iommu(d)->need_sync and
> need_sync is set as:
> 
>    if ( !is_hardware_domain(d) || iommu_hwdom_strict )
>        hd->need_sync = !iommu_use_hap_pt(d);
> 
> iommu_hwdom_strict is actually supposed to be ignored on ARM, see the
> definition in docs/misc/xen-command-line.pandoc:
> 
>    This option is hardwired to true for x86 PVH dom0's (as RAM belonging to
>    other domains in the system don't live in a compatible address space), and
>    is ignored for ARM.
> 
> But aside from that, the issue is that iommu_use_hap_pt(d) is true,
> hence, hd->need_sync is false, and gnttab_need_iommu_mapping(d) is false
> too.
> 
> As a consequence, when using PV network from a domU on a system where
> IOMMU is on from Dom0, I get:
> 
> (XEN) smmu: /smmu@fd800000: Unhandled context fault: fsr=0x402, 
> iova=0x8424cb148, fsynr=0xb0001, cb=0
> [   68.290307] macb ff0e0000.ethernet eth0: DMA bus error: HRESP not OK

I also observed the IOMMU fault when DOMU guest is created and great table is 
used when IOMMU is enabled. I fixed the error in different way but I am not 
sure if you also observing the same error. I submitted the patch to 
pci-passthrough integration branch. Please have a look once if that make sense.

https://gitlab.com/xen-project/fusa/xen-integration/-/commit/43a1a6ec91c4e3e28fa54dcbecdc8a2917836765

Regards,
Rahul
> 
> The fix is to go back to the old implementation of
> gnttab_need_iommu_mapping.  However, we don't even need to specify &&
> need_iommu(d) since we don't actually need to check for the IOMMU to be
> enabled (iommu_map does it for us at the beginning of the function.)
> 
> This fix is preferrable to changing the implementation of need_sync or
> iommu_use_hap_pt because "need_sync" is not really the reason why we
> want gnttab_need_iommu_mapping to return true.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> Backport: 4.12+ 
> 
> ---
> 
> It is incredible that it was missed for this long, but it takes a full
> PV drivers test using DMA from a non-coherent device to trigger it, e.g.
> wget from a domU to an HTTP server on a different machine, ping or
> connections to dom0 won't trigger the bug.
> 
> It is interesting that given that IOMMU is on for dom0, Linux could
> have just avoided using swiotlb-xen and everything would have just
> worked. It is worth considering introducing a feature flag (e.g.
> XENFEAT_ARM_dom0_iommu) to let dom0 know that the IOMMU is on and
> swiotlb-xen is not necessary. Then Linux can avoid initializing
> swiotlb-xen and just rely on the IOMMU for translation.
> 
> diff --git a/xen/include/asm-arm/grant_table.h 
> b/xen/include/asm-arm/grant_table.h
> index 6f585b1538..2a154d1851 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -88,8 +88,7 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t 
> mfn,
> #define gnttab_status_gfn(d, t, i)                                       \
>     (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
> 
> -#define gnttab_need_iommu_mapping(d)                    \
> -    (is_domain_direct_mapped(d) && need_iommu_pt_sync(d))
> +#define gnttab_need_iommu_mapping(d)  (is_domain_direct_mapped(d))
> 
> #endif /* __ASM_GRANT_TABLE_H__ */
> /*
>