[PATCH v3 0/2] Avoid using Xen DMA ops when the device is protected by an IOMMU

[PATCH v3 0/2] Avoid using Xen DMA ops when the device is protected by an IOMMU

[Sergiy Kibrik]

At the moment, dom0 can't distinguish which devices are protected by
IOMMU and which are not. In some cases, this can cause swiotlb bounce
buffer use for DMA addresses above 32 bits, which in turn can lead
to poor performance.

Previous discussions at [1,2] agreed upon passing info about IOMMU via
device tree. This series does that in a way consistent with existing iommu bindings.

[1] https://lore.kernel.org/all/1392913234-25429-1-git-send-email-julien.grall__16109.9684810781$1392913341$gmane$org@linaro.org/
[2] https://lore.kernel.org/all/cover.1633106362.git.roman_skakun@epam.com/

Sergiy Kibrik (2):
  dt-bindings: arm: xen: document Xen iommu device
  arm/xen: don't use xen DMA ops when the device is protected by an
    IOMMU

 Documentation/devicetree/bindings/arm/xen.txt | 26 +++++++++++++++++++
 arch/arm/mm/dma-mapping.c                     |  2 +-
 arch/arm/xen/enlighten.c                      |  9 +++++++
 arch/arm64/mm/dma-mapping.c                   |  2 +-
 include/xen/swiotlb-xen.h                     |  1 +
 5 files changed, 38 insertions(+), 2 deletions(-)

Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien@xen.org>
Cc: Oleksandr Tyshchenko <olekstysh@gmail.com>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

-- 
2.25.1

[PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Sergiy Kibrik]

In IOMMU-capable system hypervisor usually takes over IOMMU control.
Generally guest domains don't need to care about IOMMU management and take any
extra actions. Yet in some cases a knowledge about which device is protected
may be useful for privileged domain.

In compliance with iommu bindings this can be achieved with device-level
iommus property specified with dummy Xen iommu device.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 Documentation/devicetree/bindings/arm/xen.txt | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
index db5c56db30ec..98efa95c0d1b 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt
@@ -58,3 +58,29 @@ Documentation/arm/uefi.rst, which are provided by the regular UEFI stub. However
 they differ because they are provided by the Xen hypervisor, together with a set
 of UEFI runtime services implemented via hypercalls, see
 http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,platform.h.html.
+
+* XEN IOMMU device
+
+In compliance with iommu bindings Xen virtual IOMMU device node represents
+hypervisor-managed IOMMU [1]. Platform devices specified as IOMMU masters of
+this xen-iommu device are protected by hypervisor-managed platform IOMMU.
+
+Required properties:
+
+- compatible:	Should be "xen,iommu-el2-v1"
+- #iommu-cells: must be 0
+
+Example:
+
+xen-iommu {
+	compatible = "xen,iommu-el2-v1";
+	#iommu-cells = <0>;
+};
+
+video@fe001000 {
+	...
+	/* this platform device is IOMMU-protected by hypervisor */
+	iommus = <&xen-iommu 0x0>;
+};
+
+[1] Documentation/devicetree/bindings/iommu/iommu.txt
-- 
2.25.1

Re: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Robin Murphy]

On 2022-01-17 12:32, Sergiy Kibrik wrote:
> In IOMMU-capable system hypervisor usually takes over IOMMU control.
> Generally guest domains don't need to care about IOMMU management and take any
> extra actions. Yet in some cases a knowledge about which device is protected
> may be useful for privileged domain.
> 
> In compliance with iommu bindings this can be achieved with device-level
> iommus property specified with dummy Xen iommu device.

This could break Linux guests, since depending on the deferred probe 
timeout setting it could lead to drivers never probing because the 
"IOMMU" never becomes available.

Unless you intend to expose actual paravirtualised IOMMU translation 
functionality to guests (in which case virtio-iommu would be highly 
preferable anyway), I don't think this is the right approach. If there's 
no better alternative to using DT to communicate Xen-specific policy, 
then at least it should logically be via a Xen-specific DT property.

Thanks,
Robin.

> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> ---
>   Documentation/devicetree/bindings/arm/xen.txt | 26 +++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
> index db5c56db30ec..98efa95c0d1b 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -58,3 +58,29 @@ Documentation/arm/uefi.rst, which are provided by the regular UEFI stub. However
>   they differ because they are provided by the Xen hypervisor, together with a set
>   of UEFI runtime services implemented via hypercalls, see
>   http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,platform.h.html.
> +
> +* XEN IOMMU device
> +
> +In compliance with iommu bindings Xen virtual IOMMU device node represents
> +hypervisor-managed IOMMU [1]. Platform devices specified as IOMMU masters of
> +this xen-iommu device are protected by hypervisor-managed platform IOMMU.
> +
> +Required properties:
> +
> +- compatible:	Should be "xen,iommu-el2-v1"
> +- #iommu-cells: must be 0
> +
> +Example:
> +
> +xen-iommu {
> +	compatible = "xen,iommu-el2-v1";
> +	#iommu-cells = <0>;
> +};
> +
> +video@fe001000 {
> +	...
> +	/* this platform device is IOMMU-protected by hypervisor */
> +	iommus = <&xen-iommu 0x0>;
> +};
> +
> +[1] Documentation/devicetree/bindings/iommu/iommu.txt

Re: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Stefano Stabellini]

On Mon, 17 Jan 2022, Robin Murphy wrote:
> On 2022-01-17 12:32, Sergiy Kibrik wrote:
> > In IOMMU-capable system hypervisor usually takes over IOMMU control.
> > Generally guest domains don't need to care about IOMMU management and take
> > any
> > extra actions. Yet in some cases a knowledge about which device is protected
> > may be useful for privileged domain.
> > 
> > In compliance with iommu bindings this can be achieved with device-level
> > iommus property specified with dummy Xen iommu device.
> 
> This could break Linux guests, since depending on the deferred probe timeout
> setting it could lead to drivers never probing because the "IOMMU" never
> becomes available.
> 
> Unless you intend to expose actual paravirtualised IOMMU translation
> functionality to guests (in which case virtio-iommu would be highly preferable
> anyway), I don't think this is the right approach. If there's no better
> alternative to using DT to communicate Xen-specific policy, then at least it
> should logically be via a Xen-specific DT property.

Hi Robin,

Let me explain why this is useful and how it differs from something like
virtio-iommu.

When Linux is running as dom0 it uses a swiotlb-based driver to ensure
that DMA operations involving foreign (foreign == owned by other VMs)
pages can work successfully. The driver is drivers/xen/swiotlb-xen.c.
For this discussion the interesting thing about swiotlb-xen is that it
is complex, it can be slow, and it is not actually always needed. You
can imagine that somebody might want to disable swiotlb-xen whenever
possible.

Specifically, if a DMA-capable device is behind an IOMMU, then there is
no need for Linux to use swiotlb-xen.

The problem is that Linux doesn't know when a device is protected by an
IOMMU because the IOMMU is used by Xen and not exposed to Linux.

This bindings is a way for Xen to provide information to Linux, so that
Linux knows when to safely skip swiotlb-xen. It is telling Linux that
the device is protected by an IOMMU, even if Linux is not expected to
use it or do anything with it.

The initial idea was actually to write a Xen-specific DT property for
this. Then a member of the community suggested to re-use the IOMMU
bindings given that after all this is an IOMMU; the only thing special
about it is that the Linux driver is not supposed to do anything with
it. In my mind there should actually be a driver for "xen,iommu-el2-v1"
in Linux, it is just that the driver would "return 0" without doing
anything. (Note that the current patch series doesn't do this as it
turned out not to be currently necessary.)

This is a long way to say that:

1) from my point of view it is also OK to introduce a Xen-specific
   property for this if you think it is best (it doesn't look like it
   would make the code more complex either)
2) at the same time I think that reusing the IOMMU bindings seems fit
   for purpose


In regards to the "deferred probe timeout", I take it wouldn't be an
issue if we use a Xen-specific property instead? Also, would the
presence of an empty IOMMU driver for it (one that just return success
from the probing function without doing anything) solve the problem too?

Thanks for taking a look.

Cheers,

Stefano


> > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> > ---
> >   Documentation/devicetree/bindings/arm/xen.txt | 26 +++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/xen.txt
> > b/Documentation/devicetree/bindings/arm/xen.txt
> > index db5c56db30ec..98efa95c0d1b 100644
> > --- a/Documentation/devicetree/bindings/arm/xen.txt
> > +++ b/Documentation/devicetree/bindings/arm/xen.txt
> > @@ -58,3 +58,29 @@ Documentation/arm/uefi.rst, which are provided by the
> > regular UEFI stub. However
> >   they differ because they are provided by the Xen hypervisor, together with
> > a set
> >   of UEFI runtime services implemented via hypercalls, see
> >   http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,platform.h.html.
> > +
> > +* XEN IOMMU device
> > +
> > +In compliance with iommu bindings Xen virtual IOMMU device node represents
> > +hypervisor-managed IOMMU [1]. Platform devices specified as IOMMU masters
> > of
> > +this xen-iommu device are protected by hypervisor-managed platform IOMMU.
> > +
> > +Required properties:
> > +
> > +- compatible:	Should be "xen,iommu-el2-v1"
> > +- #iommu-cells: must be 0
> > +
> > +Example:
> > +
> > +xen-iommu {
> > +	compatible = "xen,iommu-el2-v1";
> > +	#iommu-cells = <0>;
> > +};
> > +
> > +video@fe001000 {
> > +	...
> > +	/* this platform device is IOMMU-protected by hypervisor */
> > +	iommus = <&xen-iommu 0x0>;
> > +};
> > +
> > +[1] Documentation/devicetree/bindings/iommu/iommu.txt
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

RE: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Sergiy Kibrik]

Hi Robin,

> 
> This could break Linux guests, since depending on the deferred probe
> timeout setting it could lead to drivers never probing because the "IOMMU"
> never becomes available.
> 

I've noticed no deferred probe timeouts when booting with this patch. Could you please explain more on how this would break guests?
Thank you!

 -- Sergiy

Re: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Robin Murphy]

On 2022-01-26 15:09, Sergiy Kibrik wrote:
> Hi Robin,
> 
>>
>> This could break Linux guests, since depending on the deferred probe
>> timeout setting it could lead to drivers never probing because the "IOMMU"
>> never becomes available.
>>
> 
> I've noticed no deferred probe timeouts when booting with this patch. Could you please explain more on how this would break guests?

Right now I think it would actually require command-line intervention, 
e.g. "fw_devlink=on" or "deferred_probe_timeout=3600" (with modules 
enabled for the latter to take full effect), but I'm wary of the 
potential for future config options to control those behaviours by default.

Robin.

> Thank you!
> 
>   -- Sergiy

Re: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Stefano Stabellini]

On Wed, 26 Jan 2022, Robin Murphy wrote:
> On 2022-01-26 15:09, Sergiy Kibrik wrote:
> > Hi Robin,
> > 
> > > 
> > > This could break Linux guests, since depending on the deferred probe
> > > timeout setting it could lead to drivers never probing because the "IOMMU"
> > > never becomes available.
> > > 
> > 
> > I've noticed no deferred probe timeouts when booting with this patch. Could
> > you please explain more on how this would break guests?
> 
> Right now I think it would actually require command-line intervention, e.g.
> "fw_devlink=on" or "deferred_probe_timeout=3600" (with modules enabled for the
> latter to take full effect), but I'm wary of the potential for future config
> options to control those behaviours by default.

If deferred_probe_timeout=3600 was specified, we would just need an
IOMMU driver in Linux for the "xen,iommu-el2-v1" node to solve the
problem, right? I guess I am trying to say that it wouldn't be a device
tree interface problem but rather a Linux implementation discussion.

Re: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Rob Herring]

On Wed, Jan 26, 2022 at 10:56:39AM -0800, Stefano Stabellini wrote:
> On Wed, 26 Jan 2022, Robin Murphy wrote:
> > On 2022-01-26 15:09, Sergiy Kibrik wrote:
> > > Hi Robin,
> > > 
> > > > 
> > > > This could break Linux guests, since depending on the deferred probe
> > > > timeout setting it could lead to drivers never probing because the "IOMMU"
> > > > never becomes available.
> > > > 
> > > 
> > > I've noticed no deferred probe timeouts when booting with this patch. Could
> > > you please explain more on how this would break guests?
> > 
> > Right now I think it would actually require command-line intervention, e.g.
> > "fw_devlink=on" or "deferred_probe_timeout=3600" (with modules enabled for the
> > latter to take full effect), but I'm wary of the potential for future config
> > options to control those behaviours by default.

fw_devlink=on is now the default (for at least a couple of cycles).

> 
> If deferred_probe_timeout=3600 was specified, we would just need an
> IOMMU driver in Linux for the "xen,iommu-el2-v1" node to solve the
> problem, right? I guess I am trying to say that it wouldn't be a device
> tree interface problem but rather a Linux implementation discussion.

You would have to add that IOMMU driver to old, existing kernels if you 
want compatibility with a new DT. Otherwise, that kernel would stop 
booting with a new DT.

Rob

Re: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Stefano Stabellini]

On Fri, 4 Feb 2022, Rob Herring wrote:
> On Wed, Jan 26, 2022 at 10:56:39AM -0800, Stefano Stabellini wrote:
> > On Wed, 26 Jan 2022, Robin Murphy wrote:
> > > On 2022-01-26 15:09, Sergiy Kibrik wrote:
> > > > Hi Robin,
> > > > 
> > > > > 
> > > > > This could break Linux guests, since depending on the deferred probe
> > > > > timeout setting it could lead to drivers never probing because the "IOMMU"
> > > > > never becomes available.
> > > > > 
> > > > 
> > > > I've noticed no deferred probe timeouts when booting with this patch. Could
> > > > you please explain more on how this would break guests?
> > > 
> > > Right now I think it would actually require command-line intervention, e.g.
> > > "fw_devlink=on" or "deferred_probe_timeout=3600" (with modules enabled for the
> > > latter to take full effect), but I'm wary of the potential for future config
> > > options to control those behaviours by default.
> 
> fw_devlink=on is now the default (for at least a couple of cycles).
> 
> > 
> > If deferred_probe_timeout=3600 was specified, we would just need an
> > IOMMU driver in Linux for the "xen,iommu-el2-v1" node to solve the
> > problem, right? I guess I am trying to say that it wouldn't be a device
> > tree interface problem but rather a Linux implementation discussion.
> 
> You would have to add that IOMMU driver to old, existing kernels if you 
> want compatibility with a new DT. Otherwise, that kernel would stop 
> booting with a new DT.

The tiny "xen,iommu-el2-v1" driver could be backported to the stable
trees, I would imagine. Otherwise, do you have another suggestion?

It looks like fw_devlink=on applies to supplier/consumer interfaces.
If that is the problem, then maybe we should avoid supplier/consumer
interfaces altogether. Instead, we could add a new Xen specific
property, e.g.:

    device@ff0000 {
        compatible = "this,device";

        xen,iommu = "on";
    };

Re: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device

[Oleksii Moisieiev]

Hi Stefano,

 > The tiny "xen,iommu-el2-v1" driver could be backported to the stable
 > trees, I would imagine. Otherwise, do you have another suggestion?

There is stub IOMMU driver already merged to the Linux Kernel:
Commit 1ca55d50e50c74747a7b8846dac306fbe5ac4cf5 ("xen/grant-dma-iommu: 
Introduce stub IOMMU driver" added by Oleksandr Tyshchenko.

I was able to use it as an empty IOMMU driver on my test setup:
Made the following device-tree changes:

xen_iommu: xen-iommu {
	compatible = "xen,grant-dma";
	iommu-cells = <0>;
};

i2c@e60b0000 {
	iommus = <&xen_iommu 0x0>;
};

Maybe this driver can be used to solve the deferred problem with some 
modifications?
What is your opinion?

Best regards,
Oleksii.

[PATCH v3 2/2] arm/xen: don't use xen DMA ops when the device is protected by an IOMMU

[Sergiy Kibrik]

Only Xen is able to know if a device can safely avoid to use xen-swiotlb.
However since Xen links FDT nodes of protected devices to special dummy
xen-iommu node we can use that information to decide whether
xen-swiotlb is needed.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
Changelog:

v3: rebased over master & documented DT binding
    https://lists.xenproject.org/archives/html/xen-devel/2021-12/msg01755.html

v2: re-use common iommu dt bindings to let guests know which devices are protected:
    https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00073.html

 arch/arm/mm/dma-mapping.c   | 2 +-
 arch/arm/xen/enlighten.c    | 9 +++++++++
 arch/arm64/mm/dma-mapping.c | 2 +-
 include/xen/swiotlb-xen.h   | 1 +
 4 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 4b61541853ea..73495f0b0a4d 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2286,7 +2286,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	set_dma_ops(dev, dma_ops);
 
 #ifdef CONFIG_XEN
-	if (xen_initial_domain())
+	if (xen_initial_domain() && !xen_is_protected_device(dev))
 		dev->dma_ops = &xen_swiotlb_dma_ops;
 #endif
 	dev->archdata.dma_ops_setup = true;
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 7619fbffcea2..0c2f0b77c8b9 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -63,6 +63,15 @@ static __read_mostly unsigned int xen_events_irq;
 uint32_t xen_start_flags;
 EXPORT_SYMBOL(xen_start_flags);
 
+bool xen_is_protected_device(struct device *dev)
+{
+	struct fwnode_handle *fwnode =
+		fwnode_find_reference(dev_fwnode(dev), "iommus", 0) ;
+	if (IS_ERR(fwnode))
+		return false;
+	return of_device_is_compatible(to_of_node(fwnode), "xen,iommu-el2-v1");
+}
+
 int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
 			       int nr, struct page **pages)
 {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 6719f9efea09..2f2b5921bf68 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -53,7 +53,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		iommu_setup_dma_ops(dev, dma_base, dma_base + size - 1);
 
 #ifdef CONFIG_XEN
-	if (xen_swiotlb_detect())
+	if (xen_swiotlb_detect() && !xen_is_protected_device(dev))
 		dev->dma_ops = &xen_swiotlb_dma_ops;
 #endif
 }
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index b3e647f86e3e..f3d805f2246f 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -9,6 +9,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
 			  size_t size, enum dma_data_direction dir);
 void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
 			     size_t size, enum dma_data_direction dir);
+bool xen_is_protected_device(struct device *dev);
 
 int xen_swiotlb_init(void);
 void __init xen_swiotlb_init_early(void);
-- 
2.25.1