[PATCH] xen/arm: fix gnttab_need_iommu_mapping

[PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Stefano Stabellini]

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

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@xilinx.com>
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__ */
 /*

Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Julien Grall]

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.

> 
> 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.

In 4.13, we introduced is_iommu_enabled() (see commit c45f59292367 
"domain: introduce XEN_DOMCTL_CDF_iommu flag") that should do the job 
for this patch.

For 4.12, we could use iommu_enabled as in general dom0 will use an 
IOMMU if Xen enable it globally. Note that 4.12 is only security 
supported since last October (see [1]). So this would be up to patch 
there tree.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Backport: 4.12+

I would suggest to use Fixes: tag if you know the exact commit. This 
would make easier for downstream users if they backported the offending 
patch.

> 
> ---
> 
> 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.

Great finding!

> 
> 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.

The presence of an IOMMU on the system doesn't necessarily indicate that 
all the devices will be protected by an IOMMU. We can only turn off the 
swiotlb-xen when we know that **all** the devices are protected.

Therefore a simple feature flag is not going to do the job. Instead, we 
need to tell Linux which devices has been protected by an IOMMU. This is 
something I attempted to do a few years ago (see [2]).

In addition to that, we also need to know whether Linux is capable to 
disable swiotlb-xen. This would allow us to turn off all the mitigation 
we introduced in Xen for direct mapped domain. One possibility would be 
to introduce ELF note like for Arm (see [3]).

> 
> 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__ */
>   /*
> 

Cheers,

[1] https://xenbits.xen.org/docs/unstable/support-matrix.html
[2] 
https://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/234523.html
[3] 
https://patchwork.kernel.org/project/linux-arm-kernel/patch/5342AF59.3030405@linaro.org/


-- 
Julien Grall

Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Julien Grall]

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

Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Stefano Stabellini]

On Sat, 6 Feb 2021, 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.
> 
> > 
> > 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.
> 
> In 4.13, we introduced is_iommu_enabled() (see commit c45f59292367 "domain:
> introduce XEN_DOMCTL_CDF_iommu flag") that should do the job for this patch.
> 
> For 4.12, we could use iommu_enabled as in general dom0 will use an IOMMU if
> Xen enable it globally. Note that 4.12 is only security supported since last
> October (see [1]). So this would be up to patch there tree.

I'll make some commit message improvements based on your reply and also
add "is_iommu_enable(d)" to the check for this patch, with the
understanding that in 4.12 it would have to be different.

Speaking of 4.12, this bug is so severe that I would consider asking for
a backport even if technically the tree is only open for security fixes.



> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > Backport: 4.12+
> 
> I would suggest to use Fixes: tag if you know the exact commit. This would
> make easier for downstream users if they backported the offending patch.

I'll add a Fixes tag


> > ---
> > 
> > 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.
> 
> Great finding!
> 
> > 
> > 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.
> 
> The presence of an IOMMU on the system doesn't necessarily indicate that all
> the devices will be protected by an IOMMU. We can only turn off the
> swiotlb-xen when we know that **all** the devices are protected.
> 
> Therefore a simple feature flag is not going to do the job. Instead, we need
> to tell Linux which devices has been protected by an IOMMU. This is something
> I attempted to do a few years ago (see [2]).
> 
> In addition to that, we also need to know whether Linux is capable to disable
> swiotlb-xen. This would allow us to turn off all the mitigation we introduced
> in Xen for direct mapped domain. One possibility would be to introduce ELF
> note like for Arm (see [3]).

Thanks for your feedback, I'll mull over it a bit more and then start a
separate email thread on this topic.

 
> > 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__ */
> >   /*
> > 
> 
> Cheers,
> 
> [1] https://xenbits.xen.org/docs/unstable/support-matrix.html
> [2]
> https://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/234523.html
> [3]
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/5342AF59.3030405@linaro.org/
> 
> 
> -- 
> Julien Grall
> 

Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Rahul Singh]

Hello Stefano,

> On 6 Feb 2021, at 12:38 am, Stefano Stabellini <sstabellini@kernel.org> 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@xilinx.com>
> 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__ */
> /*
> 

Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Julien Grall]

On 08/02/2021 18:06, Rahul Singh wrote:
>> On 6 Feb 2021, at 12:38 am, Stefano Stabellini <sstabellini@kernel.org> 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.

I belive this is the same error as Stefano has observed. However, your 
patch will unfortunately not work if you have a system with a mix of 
protected and non-protected DMA-capable devices.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Rahul Singh]

Hello Julien,

> On 8 Feb 2021, at 6:11 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 08/02/2021 18:06, Rahul Singh wrote:
>>> On 6 Feb 2021, at 12:38 am, Stefano Stabellini <sstabellini@kernel.org> 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.
> 
> I belive this is the same error as Stefano has observed. However, your patch will unfortunately not work if you have a system with a mix of protected and non-protected DMA-capable devices.

Yes you are right thats what I though when I fixed the error but then I thought in different direction if IOMMU is enabled system wise every device should be protected by IOMMU. My understanding might be wrong.

Regards,
Rahul

> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Julien Grall]

On 08/02/2021 18:19, Rahul Singh wrote:
> Hello Julien,

Hi Rahul,

> 
>> On 8 Feb 2021, at 6:11 pm, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 08/02/2021 18:06, Rahul Singh wrote:
>>>> On 6 Feb 2021, at 12:38 am, Stefano Stabellini <sstabellini@kernel.org> 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.
>>
>> I belive this is the same error as Stefano has observed. However, your patch will unfortunately not work if you have a system with a mix of protected and non-protected DMA-capable devices.
> 
> Yes you are right thats what I though when I fixed the error but then I thought in different direction if IOMMU is enabled system wise every device should be protected by IOMMU.
I am not aware of any rule preventing a mix of protected and unprotected 
DMA-capable devices.

However, even if they are all protected by an IOMMU, some of the IOMMUs 
may have been disabled by the firmware tables for various reasons (e.g. 
performance, buggy SMMU...). For instance, this is the case on Juno 
where 2 out of 3 SMMUs are disabled in the Linux upstream DT.

As we don't know which device will use the grant for DMA, we always need 
to return the machine physical address.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: fix gnttab_need_iommu_mapping

[Rahul Singh]

Hello Julien,

> On 8 Feb 2021, at 6:49 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 08/02/2021 18:19, Rahul Singh wrote:
>> Hello Julien,
> 
> Hi Rahul,
> 
>>> On 8 Feb 2021, at 6:11 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 08/02/2021 18:06, Rahul Singh wrote:
>>>>> On 6 Feb 2021, at 12:38 am, Stefano Stabellini <sstabellini@kernel.org> 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.
>>> 
>>> I belive this is the same error as Stefano has observed. However, your patch will unfortunately not work if you have a system with a mix of protected and non-protected DMA-capable devices.
>> Yes you are right thats what I though when I fixed the error but then I thought in different direction if IOMMU is enabled system wise every device should be protected by IOMMU.
> I am not aware of any rule preventing a mix of protected and unprotected DMA-capable devices.
> 
> However, even if they are all protected by an IOMMU, some of the IOMMUs may have been disabled by the firmware tables for various reasons (e.g. performance, buggy SMMU...). For instance, this is the case on Juno where 2 out of 3 SMMUs are disabled in the Linux upstream DT.
> 
> As we don't know which device will use the grant for DMA, we always need to return the machine physical address.

Thanks for the information for clearing my doubts. 

Now I understand that we need to return the machine physical address. I fixed the issue when there is no IOMMU mapping call for grant pages. I thought if page tables is not shared between IOMMU and CPU, in that scenario only we can add the mapping for the grant table in IOMMU page tables by calling
iommu_map/iommu_unmap functions. That’s why I fixed the issue to return IPA as there is no (mfn -> mfn) mapping in IOMMU page table for DMA. After this patch we don’t need to return the IPA as (mfn -> mfn) mapping will be present in P2M.

Regards,
Rahul

> 
> Cheers,
> 
> -- 
> Julien Grall