[PATCH V4 5/8] dt-bindings: Add xen, grant-dma IOMMU description for xen-grant DMA ops
From: Oleksandr Tyshchenko
The main purpose of this binding is to communicate Xen specific
information using generic IOMMU device tree bindings (which is
a good fit here) rather than introducing a custom property.
Introduce Xen specific IOMMU for the virtualized device (e.g. virtio)
to be used by Xen grant DMA-mapping layer in the subsequent commit.
The reference to Xen specific IOMMU node using "iommus" property
indicates that Xen grant mappings need to be enabled for the device,
and it specifies the ID of the domain where the corresponding backend
resides. The domid (domain ID) is used as an argument to the Xen grant
mapping APIs.
This is needed for the option to restrict memory access using Xen grant
mappings to work which primary goal is to enable using virtio devices
in Xen guests.
Signed-off-by: Oleksandr Tyshchenko
Reviewed-by: Stefano Stabellini
---
Changes RFC -> V1:
- update commit subject/description and text in description
- move to devicetree/bindings/arm/
Changes V1 -> V2:
- update text in description
- change the maintainer of the binding
- fix validation issue
- reference xen,dev-domid.yaml schema from virtio/mmio.yaml
Change V2 -> V3:
- Stefano already gave his Reviewed-by, I dropped it due to the changes
(significant)
- use generic IOMMU device tree bindings instead of custom property
"xen,dev-domid"
- change commit subject and description, was
"dt-bindings: Add xen,dev-domid property description for xen-grant DMA ops"
Changes V3 -> V4:
- add Stefano's R-b
- remove underscore in iommu node name
- remove consumer example virtio@3000
- update text for two descriptions
---
.../devicetree/bindings/iommu/xen,grant-dma.yaml | 39 ++
1 file changed, 39 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
diff --git a/Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
b/Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
new file mode 100644
index ..be1539d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/xen,grant-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xen specific IOMMU for virtualized devices (e.g. virtio)
+
+maintainers:
+ - Stefano Stabellini
+
+description:
+ The Xen IOMMU represents the Xen grant table interface. Grant mappings
+ are to be used with devices connected to the Xen IOMMU using the "iommus"
+ property, which also specifies the ID of the backend domain.
+ The binding is required to restrict memory access using Xen grant mappings.
+
+properties:
+ compatible:
+const: xen,grant-dma
+
+ '#iommu-cells':
+const: 1
+description:
+ The single cell is the domid (domain ID) of the domain where the backend
+ is running.
+
+required:
+ - compatible
+ - "#iommu-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+iommu {
+compatible = "xen,grant-dma";
+#iommu-cells = <1>;
+};
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 20/21] PCI/P2PDMA: Introduce pci_mmap_p2pmem()
On 2022-06-02 11:28, Jason Gunthorpe wrote: > On Thu, Jun 02, 2022 at 10:49:15AM -0600, Logan Gunthorpe wrote: >> >> >> On 2022-06-02 10:30, Jason Gunthorpe wrote: >>> On Thu, Jun 02, 2022 at 10:16:10AM -0600, Logan Gunthorpe wrote: >>> > Just stuff the pages into the mmap, and your driver unprobe will > automatically block until all the mmaps are closed - no different than > having an open file descriptor or something. Oh is that what we want? >>> >>> Yes, it is the typical case - eg if you have a sysfs file open unbind >>> hangs indefinitely. Many drivers can't unbind while they have open file >>> descriptors/etc. >>> >>> A couple drivers go out of their way to allow unbinding while a live >>> userspace exists but this can get complicated. Usually there should be >>> a good reason. >>> >>> The module will already be refcounted anyhow because the mmap points >>> to a char file which holds a module reference - meaning a simple rmmod >>> of the driver shouldn't work already.. >> >> Also, I just tried it... If I open a sysfs file for an nvme device (ie. >> /sys/class/nvme/nvme4/cntlid) and unbind the device, it does not block. >> A subsequent read on that file descriptor returns ENODEV. Which is what >> I would have expected. > > Oh interesting, this has been changed since years ago when I last > looked, the kernfs_get_active() is now more narrowed than it once > was. So manybe sysfs isn't the same concern it used to be! Yeah, so I really think *not* blocking unbind indefinitely is the better approach here. It's what has always been done with device dax, etc. mmaps in userspace processes get unmapped and will fault with SIGBUS on next access and unbind will actually unbind the device relatively promptly. Userspace processes can fail or try to handle the device going away gracefully. Logan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 20/21] PCI/P2PDMA: Introduce pci_mmap_p2pmem()
On Thu, Jun 02, 2022 at 10:49:15AM -0600, Logan Gunthorpe wrote: > > > On 2022-06-02 10:30, Jason Gunthorpe wrote: > > On Thu, Jun 02, 2022 at 10:16:10AM -0600, Logan Gunthorpe wrote: > > > >>> Just stuff the pages into the mmap, and your driver unprobe will > >>> automatically block until all the mmaps are closed - no different than > >>> having an open file descriptor or something. > >> > >> Oh is that what we want? > > > > Yes, it is the typical case - eg if you have a sysfs file open unbind > > hangs indefinitely. Many drivers can't unbind while they have open file > > descriptors/etc. > > > > A couple drivers go out of their way to allow unbinding while a live > > userspace exists but this can get complicated. Usually there should be > > a good reason. > > > > The module will already be refcounted anyhow because the mmap points > > to a char file which holds a module reference - meaning a simple rmmod > > of the driver shouldn't work already.. > > Also, I just tried it... If I open a sysfs file for an nvme device (ie. > /sys/class/nvme/nvme4/cntlid) and unbind the device, it does not block. > A subsequent read on that file descriptor returns ENODEV. Which is what > I would have expected. Oh interesting, this has been changed since years ago when I last looked, the kernfs_get_active() is now more narrowed than it once was. So manybe sysfs isn't the same concern it used to be! Thanks, Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 20/21] PCI/P2PDMA: Introduce pci_mmap_p2pmem()
On Thu, Jun 02, 2022 at 10:45:55AM -0600, Logan Gunthorpe wrote: > > > > On 2022-06-02 10:30, Jason Gunthorpe wrote: > > On Thu, Jun 02, 2022 at 10:16:10AM -0600, Logan Gunthorpe wrote: > > > >>> Just stuff the pages into the mmap, and your driver unprobe will > >>> automatically block until all the mmaps are closed - no different than > >>> having an open file descriptor or something. > >> > >> Oh is that what we want? > > > > Yes, it is the typical case - eg if you have a sysfs file open unbind > > hangs indefinitely. Many drivers can't unbind while they have open file > > descriptors/etc. > > > > A couple drivers go out of their way to allow unbinding while a live > > userspace exists but this can get complicated. Usually there should be > > a good reason. > > This is not my experience. All the drivers I've worked with do not block > unbind with open file descriptors (at least for char devices). I know, > for example, that having a file descriptor open of /dev/nvmeX does not > cause unbinding to block. So there are lots of bugs in the kernel, and I've seen many drivers that think calling cdev_device_del() is all they need to do - and then happily allow cdev ioctl's/etc on a de-initialized driver struct. Drivers that do take care of this usually have to put a lock around all their fops to serialize against unbind. RDMA uses SRCU, iirc TPM used a rwlock. But this is tricky and hurts fops performance. I don't know what nvme did to protect against this, I didn't notice an obvious lock. > I figured this was the expectation as the userspace process doing > the unbind won't be able to be interrupted seeing there's no way to > fail on that path. Though, it certainly would make things a lot > easier if the unbind can block indefinitely as it usually requires > some complicated locking. As I said, this is what sysfs does today and I don't see that ever changing. If you userspace has a sysfs file open then the driver unbind hangs until the file is closed. So, doing as bad as sysfs seems like a reasonable baseline to me. > Do you have an example of this? What mechanisms are developers using to > block unbind with open file descriptors? Sysfs maintains a refcount with a bias that is basically a fancied rwlock. Most places use some kind of refcount triggering a completion. Sleep on the completion until refcount is 0 on unbind kind of thing. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 20/21] PCI/P2PDMA: Introduce pci_mmap_p2pmem()
On 2022-06-02 10:30, Jason Gunthorpe wrote: > On Thu, Jun 02, 2022 at 10:16:10AM -0600, Logan Gunthorpe wrote: > >>> Just stuff the pages into the mmap, and your driver unprobe will >>> automatically block until all the mmaps are closed - no different than >>> having an open file descriptor or something. >> >> Oh is that what we want? > > Yes, it is the typical case - eg if you have a sysfs file open unbind > hangs indefinitely. Many drivers can't unbind while they have open file > descriptors/etc. > > A couple drivers go out of their way to allow unbinding while a live > userspace exists but this can get complicated. Usually there should be > a good reason. > > The module will already be refcounted anyhow because the mmap points > to a char file which holds a module reference - meaning a simple rmmod > of the driver shouldn't work already.. Also, I just tried it... If I open a sysfs file for an nvme device (ie. /sys/class/nvme/nvme4/cntlid) and unbind the device, it does not block. A subsequent read on that file descriptor returns ENODEV. Which is what I would have expected. Logan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 20/21] PCI/P2PDMA: Introduce pci_mmap_p2pmem()
On 2022-06-02 10:30, Jason Gunthorpe wrote: > On Thu, Jun 02, 2022 at 10:16:10AM -0600, Logan Gunthorpe wrote: > >>> Just stuff the pages into the mmap, and your driver unprobe will >>> automatically block until all the mmaps are closed - no different than >>> having an open file descriptor or something. >> >> Oh is that what we want? > > Yes, it is the typical case - eg if you have a sysfs file open unbind > hangs indefinitely. Many drivers can't unbind while they have open file > descriptors/etc. > > A couple drivers go out of their way to allow unbinding while a live > userspace exists but this can get complicated. Usually there should be > a good reason. This is not my experience. All the drivers I've worked with do not block unbind with open file descriptors (at least for char devices). I know, for example, that having a file descriptor open of /dev/nvmeX does not cause unbinding to block. I figured this was the expectation as the userspace process doing the unbind won't be able to be interrupted seeing there's no way to fail on that path. Though, it certainly would make things a lot easier if the unbind can block indefinitely as it usually requires some complicated locking. Do you have an example of this? What mechanisms are developers using to block unbind with open file descriptors? Logan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 20/21] PCI/P2PDMA: Introduce pci_mmap_p2pmem()
On Thu, Jun 02, 2022 at 10:16:10AM -0600, Logan Gunthorpe wrote: > > Just stuff the pages into the mmap, and your driver unprobe will > > automatically block until all the mmaps are closed - no different than > > having an open file descriptor or something. > > Oh is that what we want? Yes, it is the typical case - eg if you have a sysfs file open unbind hangs indefinitely. Many drivers can't unbind while they have open file descriptors/etc. A couple drivers go out of their way to allow unbinding while a live userspace exists but this can get complicated. Usually there should be a good reason. The module will already be refcounted anyhow because the mmap points to a char file which holds a module reference - meaning a simple rmmod of the driver shouldn't work already.. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 20/21] PCI/P2PDMA: Introduce pci_mmap_p2pmem()
On 2022-06-01 18:00, Jason Gunthorpe wrote: > On Fri, May 27, 2022 at 04:41:08PM -0600, Logan Gunthorpe wrote: >>> >>> IIRC this is the last part: >>> >>> https://lore.kernel.org/linux-mm/20220524190632.3304-1-alex.sie...@amd.com/ >>> >>> And the earlier bit with Christoph's pieces looks like it might get >>> merged to v5.19.. >>> >>> The general idea is once pte_devmap is not set then all the >>> refcounting works the way it should. This is what all new ZONE_DEVICE >>> users should do.. >> >> Ok, I don't actually follow how those patches relate to this. >> >> Based on your description I guess I don't need to set PFN_DEV and > > Yes > >> perhaps not use vmf_insert_mixed()? And then just use vm_normal_page()? > > I'm not sure ATM the best function to use, but yes, a function that > doesn't set PFN_DEV is needed here. > >> But the refcounting of the pages seemed like it was already sane to me, >> unless you mean that the code no longer has to synchronize_rcu() before >> returning the pages... > > Right. It also doesn't need to call unmap range or keep track of the > inode, or do any of that stuff unless it really needs mmap revokation > semantics (which I doubt this use case does) > > unmap range was only necessary because the refcounting is wrong - > since the pte's don't hold a ref on the page in PFN_DEV mode it is > necessary to wipe all the PTE explicitly before going ahead to > decrement the refcount on this path. > > Just stuff the pages into the mmap, and your driver unprobe will > automatically block until all the mmaps are closed - no different than > having an open file descriptor or something. Oh is that what we want? With the current method the mmaps are unmapped on unbind so that it doesn't block indefinitely. It seems more typical for resources to be dropped quickly on unbind and processes that are using them will get an error on next use. Logan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address
Hi Robin, On 23.05.2022 19:30, Robin Murphy wrote: > On 2022-05-11 13:15, Ajay Kumar wrote: >> From: Marek Szyprowski >> >> Zero is a valid DMA and IOVA address on many architectures, so adjust >> the >> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR >> (~0UL) is introduced as a generic value for the error case. Adjust all >> callers of the alloc_iova_fast() function for the new return value. > > And when does anything actually need this? In fact if you were to stop > iommu-dma from reserving IOVA 0 - which you don't - it would only show > how patch #3 is broken. I don't get why you says that patch #3 is broken. Them main issue with address 0 being reserved appears when one uses first fit allocation algorithm. In such case the first allocation will be at the 0 address, what causes some issues. This patch simply makes the whole iova related code capable of such case. This mimics the similar code already used on ARM 32bit. There are drivers that rely on the way the IOVAs are allocated. This is especially important for the drivers for devices with limited addressing capabilities. And there exists such and they can be even behind the IOMMU. Lets focus on the s5p-mfc driver and the way one of the supported hardware does the DMA. The hardware has very limited DMA window (256M) and addresses all memory buffers as an offset from the firmware base. Currently it has been assumed that the first allocated buffer will be on the beginning of the IOVA space. This worked fine on ARM 32bit and supporting such case is a target of this patchset. > Also note that it's really nothing to do with architecture either way; > iommu-dma simply chooses to reserve IOVA 0 for its own convenience, > mostly because it can. Much the same way that 0 is typically a valid > CPU VA, but mapping something meaningful there is just asking for a > world of pain debugging NULL-dereference bugs. Right that it is not directly related to the architecture, but it is much more common case for some architectures where DMA (IOVA) address = physical address + some offset. The commit message can be rephrased, though. I see no reason to forbid the 0 as a valid IOVA. The DMA-mapping also uses DMA_MAPPING_ERROR as ~0. It looks that when last fit allocation algorithm is used no one has ever need to consider such case so far. Best regards -- Marek Szyprowski, PhD Samsung R Institute Poland ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/6] iommu/qcom: Add support for AArch64 IOMMU pagetables
On Fri, May 27, 2022 at 11:28:59PM +0200, Konrad Dybcio wrote: > From: AngeloGioacchino Del Regno > > Some IOMMUs associated with some TZ firmwares may support switching > to the AArch64 pagetable format by sending a "set pagetable format" > scm command indicating the IOMMU secure ID and the context number > to switch. > > Add a DT property "qcom,use-aarch64-pagetables" for this driver to > send this command to the secure world and to switch the pagetable > format to benefit of the ARM64 IOMMU pagetables, where possible. > > Note that, even though the command should be valid to switch each > context, the property is made global because: > 1. It doesn't make too much sense to switch only one or two >context(s) to AA64 instead of just the entire thing > 2. Some IOMMUs will go crazy and produce spectacular results when >trying to mix up the pagetables on a per-context basis. > > Signed-off-by: AngeloGioacchino Del Regno > > Signed-off-by: Marijn Suijten > Signed-off-by: Konrad Dybcio > --- > .../devicetree/bindings/iommu/qcom,iommu.txt | 2 + Bindings should be separate patch. As you are making multiple changes, please convert this to DT schema first. > drivers/iommu/arm/arm-smmu/qcom_iommu.c | 54 +++ > 2 files changed, 47 insertions(+), 9 deletions(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] iommu: bindings: Add binding documentation for Toshiba Visconti5 IOMMU device
On Wed, May 25, 2022 at 10:31:46AM +0900, Nobuhiro Iwamatsu wrote:
> Add documentation for the binding of Toshiba Visconti5 SoC's IOMMU.
>
> Signed-off-by: Nobuhiro Iwamatsu
> ---
> .../bindings/iommu/toshiba,visconti-atu.yaml | 62 +++
> 1 file changed, 62 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iommu/toshiba,visconti-atu.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/iommu/toshiba,visconti-atu.yaml
> b/Documentation/devicetree/bindings/iommu/toshiba,visconti-atu.yaml
> new file mode 100644
> index ..af8d6688fa70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/toshiba,visconti-atu.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: GPL-2.0-only
Dual license: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/toshiba,visconti-atu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Toshiba ARM SoC Visconti5 IOMMU (ATU)
> +
> +maintainers:
> + - Nobuhiro Iwamatsu
> +
> +description: |+
> + IOMMU (ATU) driver can bse used for Visconti5's multimedia IPs, such as
Bindings are for hardware, not drivers.
> + DCNN (Deep Convolutional Neural Network), VIIF(Video Input), VOIF(Video
> + output), and others.
> +
> +properties:
> + compatible:
> +const: toshiba,visconti-atu
> +
> + reg:
> +maxItems: 1
> +
> + "#iommu-cells":
> +const: 0
> +
> + toshiba,max-entry:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +description: The size of entry for address
> +enum:
> + - 16
> + - 32
> +
> + toshiba,reserved-entry:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +description: The reserve number of entry address.
> +default: 0
> +minimum: 0
> +maximum: 32
These 2 need a better description of what they are for.
> +
> +required:
> + - compatible
> + - reg
> + - "#iommu-cells"
> + - toshiba,max-entry
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +soc {
> +#address-cells = <2>;
> +#size-cells = <2>;
> +
> +atu_affine0: iommu@1400f000 {
Drop unused labels.
> +compatible = "toshiba,visconti-atu";
> +reg = <0 0x1400F000 0 0x1000>;
> +toshiba,max-entry = <16>;
> +toshiba,reserved-entry = <1>;
> +#iommu-cells = <0>;
> +};
> +};
> --
> 2.36.0
>
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RESEND PATCH v2] iommu/omap: Fix missing put_device() call in omap_iommu_probe_device
The reference taken by 'of_find_device_by_node()' must be released when
not needed anymore.
Add the corresponding 'put_device()' in the error handling path and
the regular path.
Fixes: ede1c2e7d4dc ("iommu/omap: Store iommu_dev pointer in arch_data")
Signed-off-by: Miaoqian Lin
---
changes in v2:
- move put_device() before of_node_put().
- add put_device() in the regular path.
v1 Link: https://lore.kernel.org/r/20220107080428.10873-1-linmq...@gmail.com
v2 Link: https://lore.kernel.org/r/20220301063326.18120-1-linmq...@gmail.com
---
drivers/iommu/omap-iommu.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index d9cf2820c02e..58f3efdac3f7 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1683,6 +1683,7 @@ static struct iommu_device
*omap_iommu_probe_device(struct device *dev)
oiommu = platform_get_drvdata(pdev);
if (!oiommu) {
+ put_device(>dev);
of_node_put(np);
kfree(arch_data);
return ERR_PTR(-EINVAL);
@@ -1691,6 +1692,7 @@ static struct iommu_device
*omap_iommu_probe_device(struct device *dev)
tmp->iommu_dev = oiommu;
tmp->dev = >dev;
+ put_device(>dev);
of_node_put(np);
}
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] dt-bindings: iommu: mediatek: add binding documentation for MT8365 SoC
On Thu, 2022-06-02 at 16:42 +0800, Macpaul Lin wrote: > On 6/2/22 4:27 PM, Macpaul Lin wrote: > > On 6/2/22 2:18 PM, Yong Wu wrote: > > > On Mon, 2022-05-30 at 20:03 +0200, Fabien Parent wrote: > > > > Add IOMMU binding documentation for the MT8365 SoC. > > > > > > > > Signed-off-by: Fabien Parent > > > > --- > > > > .../bindings/iommu/mediatek,iommu.yaml| 2 + > > > > include/dt-bindings/memory/mt8365-larb-port.h | 96 > > > > +++ > > > > 2 files changed, 98 insertions(+) > > > > create mode 100644 include/dt-bindings/memory/mt8365-larb- > > > > port.h > > > > > > [snip...] > > > > > > > +#ifndef _DT_BINDINGS_MEMORY_MT8365_LARB_PORT_H_ > > > > +#define _DT_BINDINGS_MEMORY_MT8365_LARB_PORT_H_ > > > > + > > > > +#include > > > > + > > > > +#define M4U_LARB0_ID0 > > > > +#define M4U_LARB1_ID1 > > > > +#define M4U_LARB2_ID2 > > > > +#define M4U_LARB3_ID3 > > > > +#define M4U_LARB4_ID4 > > > > +#define M4U_LARB5_ID5 > > > > +#define M4U_LARB6_ID6 > > > > +#define M4U_LARB7_ID7 > > > > > > Remove these. they are no used, right? > > > > AIOT and customers are using the modules and their related IOMMU > > modules. > > DISP0, VENC, VDEC, ISP (CAMSYS), and APU (as far as I know, which > > should > > be VP6?) were all supported. > > Dear Yong, > How about to replace the following definitions? > > For example, replace > #define M4U_PORT_DISP_OVL0MTK_M4U_ID(0, 0) > to > #define M4U_PORT_DISP_OVL0 MTK_M4U_ID(M4U_LARB0_ID , 0) Yes. It is ok. > > > > > > > > + > > > > +/* larb0 */ > > > > +#define M4U_PORT_DISP_OVL0MTK_M4U_ID(0, 0) > > > > +#define M4U_PORT_DISP_OVL0_2LMTK_M4U_ID(0, 1) > > > > > > [...] > > > > > > > > > > > +/* larb4 */ > > > > +#define M4U_PORT_APU_READMTK_M4U_ID(0, 0) > > > > +#define M4U_PORT_APU_WRITEMTK_M4U_ID(0, 1) > > > > > > Please remove these two APU definitions. currently these are not > > > supported. > > > > Kidd, please help to check if APU use these definitions with Yong. > > However, I think these are all available to the customers. > > > > Thanks > > Macpaul Lin > > Thanks > Macpaul Lin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] dt-bindings: iommu: mediatek: add binding documentation for MT8365 SoC
On 6/2/22 4:27 PM, Macpaul Lin wrote: On 6/2/22 2:18 PM, Yong Wu wrote: On Mon, 2022-05-30 at 20:03 +0200, Fabien Parent wrote: Add IOMMU binding documentation for the MT8365 SoC. Signed-off-by: Fabien Parent --- .../bindings/iommu/mediatek,iommu.yaml | 2 + include/dt-bindings/memory/mt8365-larb-port.h | 96 +++ 2 files changed, 98 insertions(+) create mode 100644 include/dt-bindings/memory/mt8365-larb-port.h [snip...] +#ifndef _DT_BINDINGS_MEMORY_MT8365_LARB_PORT_H_ +#define _DT_BINDINGS_MEMORY_MT8365_LARB_PORT_H_ + +#include + +#define M4U_LARB0_ID 0 +#define M4U_LARB1_ID 1 +#define M4U_LARB2_ID 2 +#define M4U_LARB3_ID 3 +#define M4U_LARB4_ID 4 +#define M4U_LARB5_ID 5 +#define M4U_LARB6_ID 6 +#define M4U_LARB7_ID 7 Remove these. they are no used, right? AIOT and customers are using the modules and their related IOMMU modules. DISP0, VENC, VDEC, ISP (CAMSYS), and APU (as far as I know, which should be VP6?) were all supported. Dear Yong, How about to replace the following definitions? For example, replace #define M4U_PORT_DISP_OVL0 MTK_M4U_ID(0, 0) to #define M4U_PORT_DISP_OVL0 MTK_M4U_ID(M4U_LARB0_ID , 0) + +/* larb0 */ +#define M4U_PORT_DISP_OVL0 MTK_M4U_ID(0, 0) +#define M4U_PORT_DISP_OVL0_2L MTK_M4U_ID(0, 1) [...] +/* larb4 */ +#define M4U_PORT_APU_READ MTK_M4U_ID(0, 0) +#define M4U_PORT_APU_WRITE MTK_M4U_ID(0, 1) Please remove these two APU definitions. currently these are not supported. Kidd, please help to check if APU use these definitions with Yong. However, I think these are all available to the customers. Thanks Macpaul Lin Thanks Macpaul Lin___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] dt-bindings: iommu: mediatek: add binding documentation for MT8365 SoC
On 6/2/22 2:18 PM, Yong Wu wrote: On Mon, 2022-05-30 at 20:03 +0200, Fabien Parent wrote: Add IOMMU binding documentation for the MT8365 SoC. Signed-off-by: Fabien Parent --- .../bindings/iommu/mediatek,iommu.yaml| 2 + include/dt-bindings/memory/mt8365-larb-port.h | 96 +++ 2 files changed, 98 insertions(+) create mode 100644 include/dt-bindings/memory/mt8365-larb-port.h [snip...] +#ifndef _DT_BINDINGS_MEMORY_MT8365_LARB_PORT_H_ +#define _DT_BINDINGS_MEMORY_MT8365_LARB_PORT_H_ + +#include + +#define M4U_LARB0_ID 0 +#define M4U_LARB1_ID 1 +#define M4U_LARB2_ID 2 +#define M4U_LARB3_ID 3 +#define M4U_LARB4_ID 4 +#define M4U_LARB5_ID 5 +#define M4U_LARB6_ID 6 +#define M4U_LARB7_ID 7 Remove these. they are no used, right? AIOT and customers are using the modules and their related IOMMU modules. DISP0, VENC, VDEC, ISP (CAMSYS), and APU (as far as I know, which should be VP6?) were all supported. + +/* larb0 */ +#define M4U_PORT_DISP_OVL0 MTK_M4U_ID(0, 0) +#define M4U_PORT_DISP_OVL0_2L MTK_M4U_ID(0, 1) [...] +/* larb4 */ +#define M4U_PORT_APU_READ MTK_M4U_ID(0, 0) +#define M4U_PORT_APU_WRITE MTK_M4U_ID(0, 1) Please remove these two APU definitions. currently these are not supported. Kidd, please help to check if APU use these definitions with Yong. However, I think these are all available to the customers. Thanks Macpaul Lin___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v7 06/10] iommu/sva: Refactoring iommu_sva_bind/unbind_device()
> From: Jean-Philippe Brucker
> Sent: Wednesday, May 25, 2022 3:30 PM
>
> On Wed, May 25, 2022 at 02:04:49AM +, Tian, Kevin wrote:
> > > From: Jean-Philippe Brucker
> > > Sent: Tuesday, May 24, 2022 6:58 PM
> > >
> > > On Tue, May 24, 2022 at 10:22:28AM +, Tian, Kevin wrote:
> > > > > From: Lu Baolu
> > > > > Sent: Thursday, May 19, 2022 3:21 PM
> > > > >
> > > > > The existing iommu SVA interfaces are implemented by calling the SVA
> > > > > specific iommu ops provided by the IOMMU drivers. There's no need
> for
> > > > > any SVA specific ops in iommu_ops vector anymore as we can achieve
> > > > > this through the generic attach/detach_dev_pasid domain ops.
> > > >
> > > > set/block_pasid_dev, to be consistent.
> > > >
> > > > > +
> > > > > + mutex_lock(_sva_lock);
> > > > > + /* Search for an existing domain. */
> > > > > + domain = iommu_get_domain_for_dev_pasid(dev, mm-
> >pasid);
> > > > > + if (domain) {
> > > > > + sva_domain = to_sva_domain(domain);
> > > > > + refcount_inc(_domain->bond.users);
> > > > > + goto out_success;
> > > > > + }
> > > > > +
> > > >
> > > > why would one device/pasid be bound to a mm more than once?
> > >
> > > Device drivers can call bind() multiple times for the same device and mm,
> > > for example if one process wants to open multiple accelerator queues.
> > >
> >
> > Is it clearer to have a sva_bond_get/put() pair instead of calling
> > bind() multiple times here?
>
> I don't think it's clearer, and it would force device drivers to keep
> track of {dev, mm} pairs, when the IOMMU subsystem already does that.
> At the moment a device driver calls
>
> bond = iommu_sva_bind_device(dev, mm)
>
> for each ADI that it wants to assign to userspace. If a process happens to
> want multiple ADIs on one device, then the {dev, mm} parameters are the
> same and bind() returns the same bond. Since the IOMMU driver needs to
> track these anyway, it might as well refcount them.
>
My impression was that when an interface returns an object
then further reference to this object is usually directly acquired
on the object hence requires the caller to track it. But not a
strong opinion here if others all agree to favor simplicity for
the caller side.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 09/12] iommu/vt-d: Check device list of domain in domain free path
> From: Baolu Lu
> Sent: Wednesday, June 1, 2022 7:02 PM
>
> On 2022/6/1 17:28, Tian, Kevin wrote:
> >> From: Lu Baolu
> >> Sent: Friday, May 27, 2022 2:30 PM
> >>
> >> When the IOMMU domain is about to be freed, it should not be set on
> any
> >> device. Instead of silently dealing with some bug cases, it's better to
> >> trigger a warning to report and fix any potential bugs at the first time.
> >>
> >
> >
> >> static void domain_exit(struct dmar_domain *domain)
> >> {
> >> -
> >> - /* Remove associated devices and clear attached or cached domains
> >> */
> >> - domain_remove_dev_info(domain);
> >> + if (WARN_ON(!list_empty(>devices)))
> >> + return;
> >>
> >
> > warning is good but it doesn't mean the driver shouldn't deal with
> > that situation to make it safer e.g. blocking DMA from all attached
> > device...
>
> I have ever thought the same thing. :-)
>
> Blocking DMA from attached device should be done when setting blocking
> domain to the device. It should not be part of freeing a domain.
yes but here we are talking about some bug scenario.
>
> Here, the caller asks the driver to free the domain, but the driver
> finds that something is wrong. Therefore, it warns and returns directly.
> The domain will still be there in use until the next set_domain().
>
at least it'd look safer if we always try to unmap the entire domain i.e.:
static void domain_exit(struct dmar_domain *domain)
{
-
- /* Remove associated devices and clear attached or cached domains */
- domain_remove_dev_info(domain);
if (domain->pgd) {
LIST_HEAD(freelist);
domain_unmap(domain, 0, DOMAIN_MAX_PFN(domain->gaw), );
put_pages_list();
}
+ if (WARN_ON(!list_empty(>devices)))
+ return;
kfree(domain);
}
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] iommu: mtk_iommu: add support for 6-bit encoded port IDs
Hi Fabien,
Thanks for very much for this patch.
Retitle to iommu/mediatek: Xxx
On Mon, 2022-05-30 at 20:03 +0200, Fabien Parent wrote:
> Until now the port ID was always encoded as a 5-bit data. On MT8365,
> the port ID is encoded as a 6-bit data. This requires to rework the
> macros F_MMU_INT_ID_LARB_ID, and F_MMU_INT_ID_PORT_ID in order
> to support 5-bit and 6-bit encoded port IDs.
>
> Signed-off-by: Fabien Parent
> ---
> drivers/iommu/mtk_iommu.c | 17 +
> drivers/iommu/mtk_iommu.h | 1 +
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 6fd75a60abd6..b692347d8d56 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -103,8 +103,10 @@
> #define REG_MMU1_INT_ID 0x154
> #define F_MMU_INT_ID_COMM_ID(a) (((a) >> 9) &
> 0x7)
> #define F_MMU_INT_ID_SUB_COMM_ID(a) (((a) >> 7) & 0x3)
> -#define F_MMU_INT_ID_LARB_ID(a) (((a) >> 7) &
> 0x7)
> -#define F_MMU_INT_ID_PORT_ID(a) (((a) >> 2) &
> 0x1f)
> +#define F_MMU_INT_ID_LARB_ID(a, port_width) \
> + ((a) >> ((port_width + 2) & 0x7))
> +#define F_MMU_INT_ID_PORT_ID(a, port_width) \
> + (((a) >> 2) & GENMASK(port_width - 1,
> 0))
Add () for port_width.
>
> #define MTK_PROTECT_PA_ALIGN 256
>
> @@ -291,12 +293,13 @@ static irqreturn_t mtk_iommu_isr(int irq, void
> *dev_id)
> fault_pa |= (u64)pa34_32 << 32;
> }
>
> - fault_port = F_MMU_INT_ID_PORT_ID(regval);
> + fault_port = F_MMU_INT_ID_PORT_ID(regval, data->plat_data-
> >port_width);
> if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_SUB_COMM)) {
> fault_larb = F_MMU_INT_ID_COMM_ID(regval);
> sub_comm = F_MMU_INT_ID_SUB_COMM_ID(regval);
> } else {
> - fault_larb = F_MMU_INT_ID_LARB_ID(regval);
> + fault_larb = F_MMU_INT_ID_LARB_ID(regval,
> + data->plat_data-
> >port_width);
> }
> fault_larb = data->plat_data-
> >larbid_remap[fault_larb][sub_comm];
>
> @@ -1034,6 +1037,7 @@ static const struct mtk_iommu_plat_data
> mt2712_data = {
> .iova_region = single_domain,
> .iova_region_nr = ARRAY_SIZE(single_domain),
> .larbid_remap = {{0}, {1}, {2}, {3}, {4}, {5}, {6}, {7}},
> + .port_width = 5,
> };
>
> static const struct mtk_iommu_plat_data mt6779_data = {
> @@ -1043,6 +1047,7 @@ static const struct mtk_iommu_plat_data
> mt6779_data = {
> .iova_region = single_domain,
> .iova_region_nr = ARRAY_SIZE(single_domain),
> .larbid_remap = {{0}, {1}, {2}, {3}, {5}, {7, 8}, {10}, {9}},
> + .port_width= 5,
> };
>
> static const struct mtk_iommu_plat_data mt8167_data = {
> @@ -1052,6 +1057,7 @@ static const struct mtk_iommu_plat_data
> mt8167_data = {
> .iova_region = single_domain,
> .iova_region_nr = ARRAY_SIZE(single_domain),
> .larbid_remap = {{0}, {1}, {2}}, /* Linear mapping. */
> + .port_width = 5,
> };
>
> static const struct mtk_iommu_plat_data mt8173_data = {
> @@ -1062,6 +1068,7 @@ static const struct mtk_iommu_plat_data
> mt8173_data = {
> .iova_region = single_domain,
> .iova_region_nr = ARRAY_SIZE(single_domain),
> .larbid_remap = {{0}, {1}, {2}, {3}, {4}, {5}}, /* Linear
> mapping. */
> + .port_width = 5,
> };
>
> static const struct mtk_iommu_plat_data mt8183_data = {
> @@ -1071,6 +1078,7 @@ static const struct mtk_iommu_plat_data
> mt8183_data = {
> .iova_region = single_domain,
> .iova_region_nr = ARRAY_SIZE(single_domain),
> .larbid_remap = {{0}, {4}, {5}, {6}, {7}, {2}, {3}, {1}},
> + .port_width = 5,
> };
>
> static const struct mtk_iommu_plat_data mt8192_data = {
> @@ -1082,6 +1090,7 @@ static const struct mtk_iommu_plat_data
> mt8192_data = {
> .iova_region_nr = ARRAY_SIZE(mt8192_multi_dom),
> .larbid_remap = {{0}, {1}, {4, 5}, {7}, {2}, {9, 11, 19, 20},
> {0, 14, 16}, {0, 13, 18, 17}},
> + .port_width = 5,
> };
>
> static const struct of_device_id mtk_iommu_of_ids[] = {
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index b742432220c5..84cecaf6d61c 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -54,6 +54,7 @@ struct mtk_iommu_plat_data {
> enum mtk_iommu_plat m4u_plat;
> u32 flags;
> u32 inv_sel_reg;
> + u8 port_width;
Please help rename to int_id_port_width for more detailed from the
register name (REG_MMU0_INT_ID).
>
> unsigned intiova_region_nr;
> const struct mtk_iommu_iova_region *iova_region;
> --
> 2.36.1
>
___
iommu mailing
Re: [PATCH 1/3] dt-bindings: iommu: mediatek: add binding documentation for MT8365 SoC
On Mon, 2022-05-30 at 20:03 +0200, Fabien Parent wrote: > Add IOMMU binding documentation for the MT8365 SoC. > > Signed-off-by: Fabien Parent > --- > .../bindings/iommu/mediatek,iommu.yaml| 2 + > include/dt-bindings/memory/mt8365-larb-port.h | 96 > +++ > 2 files changed, 98 insertions(+) > create mode 100644 include/dt-bindings/memory/mt8365-larb-port.h [snip...] > +#ifndef _DT_BINDINGS_MEMORY_MT8365_LARB_PORT_H_ > +#define _DT_BINDINGS_MEMORY_MT8365_LARB_PORT_H_ > + > +#include > + > +#define M4U_LARB0_ID 0 > +#define M4U_LARB1_ID 1 > +#define M4U_LARB2_ID 2 > +#define M4U_LARB3_ID 3 > +#define M4U_LARB4_ID 4 > +#define M4U_LARB5_ID 5 > +#define M4U_LARB6_ID 6 > +#define M4U_LARB7_ID 7 Remove these. they are no used, right? > + > +/* larb0 */ > +#define M4U_PORT_DISP_OVL0 MTK_M4U_ID(0, 0) > +#define M4U_PORT_DISP_OVL0_2LMTK_M4U_ID(0, 1) [...] > > +/* larb4 */ > +#define M4U_PORT_APU_READMTK_M4U_ID(0, 0) > +#define M4U_PORT_APU_WRITE MTK_M4U_ID(0, 1) Please remove these two APU definitions. currently these are not supported. Thanks. > + > +#endif ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
