Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Julien Grall <julien@xxxxxxx>]

Hi,

On 06/02/2021 11:09, Julien Grall wrote:
> Hi Stefano,
>
> On 06/02/2021 00:38, Stefano Stabellini wrote:
> > Commit 91d4eca7add broke gnttab_need_iommu_mapping on ARM.
> Doh :/.
>
> > 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,
> This is not entirely correct, we only need gnttab_need_iommu_mapping() 
> to return true when the domain is direct mapped **and** the IOMMU is 
> enabled for the domain.
> > 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.
> need_sync means that you have a separate IOMMU page-table and they need 
> to be updated for every change.
> hap_pt means the page-table used by the IOMMU is the P2M.
>
> For Arm, we always shared the P2M with the IOMMU.
>
> > 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
> >
> > 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.)
> gnttab_need_iommu_mapping() doesn't only gate the 
> iommu_legacy_{,un}map() call but also decides whether we need to held 
> both the local and remote grant-table write lock for the duration of the 
> operation (see double_gt_lock()).
> I'd like to avoid the requirement to held the double_gt_lock() if there  > is 
> the domain is going to use the IOMMU.
It looks like I didn't convey the right message here. What I meant is we 
should avoid to held both grant-table locks if dom0 is not going to be 
use the IOMMU.
Cheers,

--
Julien Grall