Re: [PATCH v3 03/14] iommu/mediatek: Add device_link between the consumer and the larb devices
On Tue, Sep 3, 2019 at 5:38 PM Yong Wu wrote:
>
> MediaTek IOMMU don't have its power-domain. all the consumer connect
> with smi-larb, then connect with smi-common.
>
> M4U
> |
> smi-common
> |
> -
> | |...
> | |
> larb1 larb2
> | |
> vdec venc
>
> When the consumer works, it should enable the smi-larb's power which
> also need enable the smi-common's power firstly.
>
> Thus, First of all, use the device link connect the consumer and the
> smi-larbs. then add device link between the smi-larb and smi-common.
>
> This patch adds device_link between the consumer and the larbs.
>
> When device_link_add, I add the flag DL_FLAG_STATELESS to avoid calling
> pm_runtime_xx to keep the original status of clocks. It can avoid two
> issues:
> 1) Display HW show fastlogo abnormally reported in [1]. At the beggining,
> all the clocks are enabled before entering kernel, but the clocks for
> display HW(always in larb0) will be gated after clk_enable and clk_disable
> called from device_link_add(->pm_runtime_resume) and rpm_idle. The clock
> operation happened before display driver probe. At that time, the display
> HW will be abnormal.
>
> 2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
> pm_runtime_xx to avoid the deadlock.
>
> Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
> device_link_removed should be added explicitly.
>
> [1] http://lists.infradead.org/pipermail/linux-mediatek/2019-July/
> 021500.html
> [2] https://lore.kernel.org/patchwork/patch/1086569/
>
> Suggested-by: Tomasz Figa
> Signed-off-by: Yong Wu
> ---
> drivers/iommu/mtk_iommu.c| 17 +
> drivers/iommu/mtk_iommu_v1.c | 18 +-
> 2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index b138b94..2511b3c 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -450,6 +450,9 @@ static int mtk_iommu_add_device(struct device *dev)
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct mtk_iommu_data *data;
> struct iommu_group *group;
> + struct device_link *link;
> + struct device *larbdev;
> + unsigned int larbid;
>
> if (!fwspec || fwspec->ops != _iommu_ops)
> return -ENODEV; /* Not a iommu client device */
> @@ -461,6 +464,14 @@ static int mtk_iommu_add_device(struct device *dev)
> if (IS_ERR(group))
> return PTR_ERR(group);
>
> + /* Link the consumer device with the smi-larb device(supplier) */
> + larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
I'll mirror the comment I made on gerrit
(https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1361013):
Maybe I'm missing something here, but for example, on MT8173,
vcodec_enc: vcodec@18002000 needs to use both larb3 and larb5, isn't
the code below just adding a link for larb3?
Do we need to iterate over all fwspecs->ids to figure out which larbs
we need to add links to each of them?
> + larbdev = data->larb_imu[larbid].dev;
> + link = device_link_add(dev, larbdev,
> + DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
> + if (!link)
> + dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
> +
> iommu_group_put(group);
> return 0;
> }
> @@ -469,6 +480,8 @@ static void mtk_iommu_remove_device(struct device *dev)
> {
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct mtk_iommu_data *data;
> + struct device *larbdev;
> + unsigned int larbid;
>
> if (!fwspec || fwspec->ops != _iommu_ops)
> return;
> @@ -476,6 +489,10 @@ static void mtk_iommu_remove_device(struct device *dev)
> data = fwspec->iommu_priv;
> iommu_device_unlink(>iommu, dev);
>
> + larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> + larbdev = data->larb_imu[larbid].dev;
> + device_link_remove(dev, larbdev);
> +
> iommu_group_remove_device(dev);
> iommu_fwspec_free(dev);
> }
> diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
> index 2034d72..a7f22a2 100644
> --- a/drivers/iommu/mtk_iommu_v1.c
> +++ b/drivers/iommu/mtk_iommu_v1.c
> @@ -423,7 +423,9 @@ static int mtk_iommu_add_device(struct device *dev)
> struct of_phandle_iterator it;
> struct mtk_iommu_data *data;
> struct iommu_group *group;
> - int err;
> + struct device_link *link;
> + struct device *larbdev;
> + int err, larbid;
>
> of_for_each_phandle(, err, dev->of_node, "iommus",
> "#iommu-cells", 0) {
> @@ -466,6 +468,14 @@ static int mtk_iommu_add_device(struct device *dev)
> return err;
> }
>
> + /* Link the consumer device with the smi-larb device(supplier) */
> + larbid =
[iommu:arm/omap 4/4] drivers/gpu/drm/rockchip/rockchip_drm_gem.c:134:20: error: implicit declaration of function 'vmap'; did you mean 'bmap'?
tree: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git arm/omap
head: e93a1695d7fb551376b1c1220a267d032b6ad159
commit: e93a1695d7fb551376b1c1220a267d032b6ad159 [4/4] iommu: Enable compile
testing for some of drivers
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.5.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout e93a1695d7fb551376b1c1220a267d032b6ad159
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
drivers/gpu/drm/rockchip/rockchip_drm_gem.c: In function
'rockchip_gem_alloc_iommu':
>> drivers/gpu/drm/rockchip/rockchip_drm_gem.c:134:20: error: implicit
>> declaration of function 'vmap'; did you mean 'bmap'?
>> [-Werror=implicit-function-declaration]
rk_obj->kvaddr = vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
^~~~
bmap
>> drivers/gpu/drm/rockchip/rockchip_drm_gem.c:134:59: error: 'VM_MAP'
>> undeclared (first use in this function); did you mean 'VM_MPX'?
rk_obj->kvaddr = vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
^~
VM_MPX
drivers/gpu/drm/rockchip/rockchip_drm_gem.c:134:59: note: each undeclared
identifier is reported only once for each function it appears in
drivers/gpu/drm/rockchip/rockchip_drm_gem.c: In function
'rockchip_gem_free_iommu':
>> drivers/gpu/drm/rockchip/rockchip_drm_gem.c:190:2: error: implicit
>> declaration of function 'vunmap'; did you mean 'iounmap'?
>> [-Werror=implicit-function-declaration]
vunmap(rk_obj->kvaddr);
^~
iounmap
drivers/gpu/drm/rockchip/rockchip_drm_gem.c: In function
'rockchip_gem_prime_vmap':
drivers/gpu/drm/rockchip/rockchip_drm_gem.c:547:49: error: 'VM_MAP'
undeclared (first use in this function); did you mean 'VM_MPX'?
return vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
^~
VM_MPX
cc1: some warnings being treated as errors
vim +134 drivers/gpu/drm/rockchip/rockchip_drm_gem.c
38f993b7c59e261 Tomasz Figa 2016-06-24 119
38f993b7c59e261 Tomasz Figa 2016-06-24 120 static int
rockchip_gem_alloc_iommu(struct rockchip_gem_object *rk_obj,
38f993b7c59e261 Tomasz Figa 2016-06-24 121
bool alloc_kmap)
38f993b7c59e261 Tomasz Figa 2016-06-24 122 {
38f993b7c59e261 Tomasz Figa 2016-06-24 123 int ret;
38f993b7c59e261 Tomasz Figa 2016-06-24 124
38f993b7c59e261 Tomasz Figa 2016-06-24 125 ret =
rockchip_gem_get_pages(rk_obj);
38f993b7c59e261 Tomasz Figa 2016-06-24 126 if (ret < 0)
38f993b7c59e261 Tomasz Figa 2016-06-24 127 return ret;
38f993b7c59e261 Tomasz Figa 2016-06-24 128
38f993b7c59e261 Tomasz Figa 2016-06-24 129 ret =
rockchip_gem_iommu_map(rk_obj);
38f993b7c59e261 Tomasz Figa 2016-06-24 130 if (ret < 0)
38f993b7c59e261 Tomasz Figa 2016-06-24 131 goto err_free;
38f993b7c59e261 Tomasz Figa 2016-06-24 132
38f993b7c59e261 Tomasz Figa 2016-06-24 133 if (alloc_kmap) {
38f993b7c59e261 Tomasz Figa 2016-06-24 @134 rk_obj->kvaddr
= vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
38f993b7c59e261 Tomasz Figa 2016-06-24 135
pgprot_writecombine(PAGE_KERNEL));
38f993b7c59e261 Tomasz Figa 2016-06-24 136 if
(!rk_obj->kvaddr) {
38f993b7c59e261 Tomasz Figa 2016-06-24 137
DRM_ERROR("failed to vmap() buffer\n");
38f993b7c59e261 Tomasz Figa 2016-06-24 138 ret =
-ENOMEM;
38f993b7c59e261 Tomasz Figa 2016-06-24 139 goto
err_unmap;
38f993b7c59e261 Tomasz Figa 2016-06-24 140 }
38f993b7c59e261 Tomasz Figa 2016-06-24 141 }
38f993b7c59e261 Tomasz Figa 2016-06-24 142
38f993b7c59e261 Tomasz Figa 2016-06-24 143 return 0;
38f993b7c59e261 Tomasz Figa 2016-06-24 144
38f993b7c59e261 Tomasz Figa 2016-06-24 145 err_unmap:
38f993b7c59e261 Tomasz Figa 2016-06-24 146
rockchip_gem_iommu_unmap(rk_obj);
38f993b7c59e261 Tomasz Figa 2016-06-24 147 err_free:
38f993b7c59e261 Tomasz Figa 2016-06-24 148
rockchip_gem_put_pages(rk_obj);
38f993b7c59e261 Tomasz Figa 2016-06-24 149
38f993b7c59e261 Tomasz Figa 2016-06-24 150 return ret;
38f993b7c59e261 Tomasz Figa 2016-06-24 151 }
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
On Wed, Mar 04, 2020 at 01:37:44PM -0800, Jacob Pan (Jun) wrote: > + Mike and Erik who work closely on UEFI and ACPICA. > > Copy paste Erik's initial response below on how to get a new table, > seems to confirm with the process you stated above. > > "Fairly easy. You reserve a 4-letter symbol by sending a message > requesting to reserve the signature to Mike or the ASWG mailing list or > i...@acpi.info Great! I think this is going to be the path forward here. Regards, Joerg > > There is also another option. You can have ASWG own this new table so > that not one entity or company owns the new table." ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
On Wed, Mar 04, 2020 at 02:34:33PM -0500, Michael S. Tsirkin wrote: > All these extra levels of indirection is one of the reasons > hypervisors such as kata try to avoid ACPI. Platforms that don't use ACPI need another hardware detection mechanism, which can also be supported. But the first step here is to enable the general case, and for x86 platforms this means ACPI. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
On Wed, 4 Mar 2020 18:40:46 +0100 Joerg Roedel wrote: > On Wed, Mar 04, 2020 at 04:38:21PM +0100, Jean-Philippe Brucker wrote: > > I agree with this. The problem is I don't know how to get a new > > ACPI table or change an existing one. It needs to go through the > > UEFI forum in order to be accepted, and I don't have any weight > > there. I've been trying to get the tiny change into IORT for ages. > > I haven't been given any convincing reason against it or offered > > any alternative, it's just stalled. The topology description > > introduced here wasn't my first choice either but unless someone > > can help finding another way into ACPI, I don't have a better > > idea. > > A quote from the ACPI Specification (Version 6.3, Section 5.2.6, > Page 119): > > Table signatures will be reserved by the ACPI promoters and > posted independently of this specification in ACPI errata and > clarification documents on the ACPI web site. Requests to > reserve a 4-byte alphanumeric table signature should be sent > to the email address i...@acpi.info and should include the purpose > of the table and reference URL to a document that describes > the table format. Tables defined outside of the ACPI specification > may define data value encodings in either little endian or big > endian format. For the purpose of clarity, external table > definition documents should include the endian-ness of their > data value encodings. > > So it sounds like you need to specifiy the table format and send a > request to i...@acpi.info to get a table signature for it. > + Mike and Erik who work closely on UEFI and ACPICA. Copy paste Erik's initial response below on how to get a new table, seems to confirm with the process you stated above. "Fairly easy. You reserve a 4-letter symbol by sending a message requesting to reserve the signature to Mike or the ASWG mailing list or i...@acpi.info There is also another option. You can have ASWG own this new table so that not one entity or company owns the new table." > Regards, > > Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
On Wed, Mar 04, 2020 at 02:37:08PM +0100, Joerg Roedel wrote: > Hi Michael, > > On Tue, Mar 03, 2020 at 11:09:41AM -0500, Michael S. Tsirkin wrote: > > No. It's coded into the hardware. Which might even be practical > > for bare-metal (e.g. on-board flash), but is very practical > > when the device is part of a hypervisor. > > If its that way on PPC, than fine for them. But since this is enablement > for x86, it should follow the x86 platform best practices, and that > means describing hardware through ACPI. For hardware, sure. Hypervisors aren't hardware though and a bunch of hypervisors don't use ACPI. > > This "hardware" is actually part of hypervisor so there's no > > reason it can't be completely self-descriptive. It's specified > > by the same entity as the "firmware". > > That is just an implementation detail. Yes, QEMU emulates the hardware > and builds the ACPI tables. But it could also be implemented in a way > where the ACPI tables are build by guest firmware. All these extra levels of indirection is one of the reasons hypervisors such as kata try to avoid ACPI. > > I don't see why it would be much faster. The interface isn't that > > different from command queues of VTD. > > VirtIO IOMMU doesn't need to build page-tables that the hypervisor then > has to shadow, which makes things much faster. If you emulate one of the > other IOMMUs (VT-d or AMD-Vi) the code has to shadow the full page-table > at once when device passthrough is used. VirtIO-IOMMU doesn't need that, > and that makes it much faster and efficient. IIUC VT-d at least supports range invalidations. > > > Making ACPI meet the goals of embedded projects such as kata containers > > would be a gigantic task with huge stability implications. By > > comparison this 400-line parser is well contained and does the job. I > > didn't yet see compelling reasons not to merge this, but I'll be > > interested to see some more specific concerns. > > An ACPI table parse wouldn't need more lines of code. It realies on ACPI OSPM itself to handle ACPI bytecode, which is huge. > For embedded > systems there is still the DT way of describing things. For some embedded systems. > Regards, > > Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
On Wed, Mar 04, 2020 at 04:38:21PM +0100, Jean-Philippe Brucker wrote: > I agree with this. The problem is I don't know how to get a new ACPI table > or change an existing one. It needs to go through the UEFI forum in order > to be accepted, and I don't have any weight there. I've been trying to get > the tiny change into IORT for ages. I haven't been given any convincing > reason against it or offered any alternative, it's just stalled. The > topology description introduced here wasn't my first choice either but > unless someone can help finding another way into ACPI, I don't have a > better idea. A quote from the ACPI Specification (Version 6.3, Section 5.2.6, Page 119): Table signatures will be reserved by the ACPI promoters and posted independently of this specification in ACPI errata and clarification documents on the ACPI web site. Requests to reserve a 4-byte alphanumeric table signature should be sent to the email address i...@acpi.info and should include the purpose of the table and reference URL to a document that describes the table format. Tables defined outside of the ACPI specification may define data value encodings in either little endian or big endian format. For the purpose of clarity, external table definition documents should include the endian-ness of their data value encodings. So it sounds like you need to specifiy the table format and send a request to i...@acpi.info to get a table signature for it. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
On Wed, Mar 04, 2020 at 07:48:54AM -0800, Jacob Pan wrote: > For emulated VT-d IOMMU, GIOVA can also be build as first level page > tables then pass to the host IOMMU to bind. There is no need to shadow > in this case, pIOMMU will do nested translation and walk guest page > tables. Right, but that requires hardware support. A pure software emulation of VT-d requires the full shadow of the guest io-page-table. > I thought we have the universal device properties to abstract DT and > ACPI (via _DSD). Is that an option? I don't know whether this was considered, Jean-Philippe? Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
On Wed, 4 Mar 2020 14:37:08 +0100 Joerg Roedel wrote: > Hi Michael, > > On Tue, Mar 03, 2020 at 11:09:41AM -0500, Michael S. Tsirkin wrote: > > No. It's coded into the hardware. Which might even be practical > > for bare-metal (e.g. on-board flash), but is very practical > > when the device is part of a hypervisor. > > If its that way on PPC, than fine for them. But since this is > enablement for x86, it should follow the x86 platform best practices, > and that means describing hardware through ACPI. > > > This "hardware" is actually part of hypervisor so there's no > > reason it can't be completely self-descriptive. It's specified > > by the same entity as the "firmware". > > That is just an implementation detail. Yes, QEMU emulates the hardware > and builds the ACPI tables. But it could also be implemented in a way > where the ACPI tables are build by guest firmware. > > > I don't see why it would be much faster. The interface isn't that > > different from command queues of VTD. > > VirtIO IOMMU doesn't need to build page-tables that the hypervisor > then has to shadow, which makes things much faster. If you emulate > one of the other IOMMUs (VT-d or AMD-Vi) the code has to shadow the > full page-table at once when device passthrough is used. VirtIO-IOMMU > doesn't need that, and that makes it much faster and efficient. > For emulated VT-d IOMMU, GIOVA can also be build as first level page tables then pass to the host IOMMU to bind. There is no need to shadow in this case, pIOMMU will do nested translation and walk guest page tables. > > Making ACPI meet the goals of embedded projects such as kata > > containers would be a gigantic task with huge stability > > implications. By comparison this 400-line parser is well contained > > and does the job. I didn't yet see compelling reasons not to merge > > this, but I'll be interested to see some more specific concerns. > > An ACPI table parse wouldn't need more lines of code. For embedded > systems there is still the DT way of describing things. > I thought we have the universal device properties to abstract DT and ACPI (via _DSD). Is that an option? > Regards, > > Joerg > ___ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu [Jacob Pan] ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
On Wed, Mar 04, 2020 at 02:37:08PM +0100, Joerg Roedel wrote: > Hi Michael, > > On Tue, Mar 03, 2020 at 11:09:41AM -0500, Michael S. Tsirkin wrote: > > No. It's coded into the hardware. Which might even be practical > > for bare-metal (e.g. on-board flash), but is very practical > > when the device is part of a hypervisor. > > If its that way on PPC, than fine for them. But since this is enablement > for x86, it should follow the x86 platform best practices, and that > means describing hardware through ACPI. I agree with this. The problem is I don't know how to get a new ACPI table or change an existing one. It needs to go through the UEFI forum in order to be accepted, and I don't have any weight there. I've been trying to get the tiny change into IORT for ages. I haven't been given any convincing reason against it or offered any alternative, it's just stalled. The topology description introduced here wasn't my first choice either but unless someone can help finding another way into ACPI, I don't have a better idea. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RESEND PATCH 1/4] iommu/omap: Fix pointer cast -Wpointer-to-int-cast warnings on 64 bit
On Tue, Mar 03, 2020 at 09:27:48PM +0100, Krzysztof Kozlowski wrote: > pointers should be casted to unsigned long to avoid > -Wpointer-to-int-cast warnings when compiling on 64-bit platform (e.g. > with COMPILE_TEST): > > drivers/iommu/omap-iommu.c: In function ‘omap2_iommu_enable’: > drivers/iommu/omap-iommu.c:170:25: warning: > cast from pointer to integer of different size [-Wpointer-to-int-cast] > if (!obj->iopgd || !IS_ALIGNED((u32)obj->iopgd, SZ_16K)) > ^ > > Signed-off-by: Krzysztof Kozlowski Applied all, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 07/26] arm64: mm: Pin down ASIDs for sharing mm with devices
On Thu, Feb 27, 2020 at 05:43:51PM +, Jonathan Cameron wrote: > On Mon, 24 Feb 2020 19:23:42 +0100 > Jean-Philippe Brucker wrote: > > > From: Jean-Philippe Brucker > > > > To enable address space sharing with the IOMMU, introduce mm_context_get() > > and mm_context_put(), that pin down a context and ensure that it will keep > > its ASID after a rollover. Export the symbols to let the modular SMMUv3 > > driver use them. > > > > Pinning is necessary because a device constantly needs a valid ASID, > > unlike tasks that only require one when running. Without pinning, we would > > need to notify the IOMMU when we're about to use a new ASID for a task, > > and it would get complicated when a new task is assigned a shared ASID. > > Consider the following scenario with no ASID pinned: > > > > 1. Task t1 is running on CPUx with shared ASID (gen=1, asid=1) > > 2. Task t2 is scheduled on CPUx, gets ASID (1, 2) > > 3. Task tn is scheduled on CPUy, a rollover occurs, tn gets ASID (2, 1) > >We would now have to immediately generate a new ASID for t1, notify > >the IOMMU, and finally enable task tn. We are holding the lock during > >all that time, since we can't afford having another CPU trigger a > >rollover. The IOMMU issues invalidation commands that can take tens of > >milliseconds. > > > > It gets needlessly complicated. All we wanted to do was schedule task tn, > > that has no business with the IOMMU. By letting the IOMMU pin tasks when > > needed, we avoid stalling the slow path, and let the pinning fail when > > we're out of shareable ASIDs. > > > > After a rollover, the allocator expects at least one ASID to be available > > in addition to the reserved ones (one per CPU). So (NR_ASIDS - NR_CPUS - > > 1) is the maximum number of ASIDs that can be shared with the IOMMU. > > > > Signed-off-by: Jean-Philippe Brucker > A few more trivial points. I'll fix those, thanks Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 23/26] iommu/arm-smmu-v3: Add stall support for platform devices
On Wed, Feb 26, 2020 at 04:44:53PM +0800, Xu Zaibo wrote:
> Hi,
>
>
> On 2020/2/25 2:23, Jean-Philippe Brucker wrote:
> > From: Jean-Philippe Brucker
> >
> > The SMMU provides a Stall model for handling page faults in platform
> > devices. It is similar to PCI PRI, but doesn't require devices to have
> > their own translation cache. Instead, faulting transactions are parked and
> > the OS is given a chance to fix the page tables and retry the transaction.
> >
> > Enable stall for devices that support it (opt-in by firmware). When an
> > event corresponds to a translation error, call the IOMMU fault handler. If
> > the fault is recoverable, it will call us back to terminate or continue
> > the stall.
> >
> > Signed-off-by: Jean-Philippe Brucker
> > ---
> > drivers/iommu/arm-smmu-v3.c | 271 ++--
> > drivers/iommu/of_iommu.c| 5 +-
> > include/linux/iommu.h | 2 +
> > 3 files changed, 269 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index 6a5987cce03f..da5dda5ba26a 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -374,6 +374,13 @@
> > #define CMDQ_PRI_1_GRPID GENMASK_ULL(8, 0)
> > #define CMDQ_PRI_1_RESP GENMASK_ULL(13, 12)
> [...]
> > +static int arm_smmu_page_response(struct device *dev,
> > + struct iommu_fault_event *unused,
> > + struct iommu_page_response *resp)
> > +{
> > + struct arm_smmu_cmdq_ent cmd = {0};
> > + struct arm_smmu_master *master = dev_iommu_fwspec_get(dev)->iommu_priv;
> Here can use 'dev_to_master' ?
Certainly, good catch
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 23/26] iommu/arm-smmu-v3: Add stall support for platform devices
On Thu, Feb 27, 2020 at 06:17:26PM +, Jonathan Cameron wrote:
> On Mon, 24 Feb 2020 19:23:58 +0100
> Jean-Philippe Brucker wrote:
>
> > From: Jean-Philippe Brucker
> >
> > The SMMU provides a Stall model for handling page faults in platform
> > devices. It is similar to PCI PRI, but doesn't require devices to have
> > their own translation cache. Instead, faulting transactions are parked and
> > the OS is given a chance to fix the page tables and retry the transaction.
> >
> > Enable stall for devices that support it (opt-in by firmware). When an
> > event corresponds to a translation error, call the IOMMU fault handler. If
> > the fault is recoverable, it will call us back to terminate or continue
> > the stall.
> >
> > Signed-off-by: Jean-Philippe Brucker
> One question inline.
>
> Thanks,
>
> > ---
> > drivers/iommu/arm-smmu-v3.c | 271 ++--
> > drivers/iommu/of_iommu.c| 5 +-
> > include/linux/iommu.h | 2 +
> > 3 files changed, 269 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index 6a5987cce03f..da5dda5ba26a 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -374,6 +374,13 @@
>
>
> > +/*
> > + * arm_smmu_flush_evtq - wait until all events currently in the queue have
> > been
> > + * consumed.
> > + *
> > + * Wait until the evtq thread finished a batch, or until the queue is
> > empty.
> > + * Note that we don't handle overflows on q->batch. If it occurs, just
> > wait for
> > + * the queue to be empty.
> > + */
> > +static int arm_smmu_flush_evtq(void *cookie, struct device *dev, int pasid)
> > +{
> > + int ret;
> > + u64 batch;
> > + struct arm_smmu_device *smmu = cookie;
> > + struct arm_smmu_queue *q = >evtq.q;
> > +
> > + spin_lock(>wq.lock);
> > + if (queue_sync_prod_in(q) == -EOVERFLOW)
> > + dev_err(smmu->dev, "evtq overflow detected -- requests lost\n");
> > +
> > + batch = q->batch;
>
> So this is trying to be sure we have advanced the queue 2 spots?
So we call arm_smmu_flush_evtq() before decommissioning a PASID, to make
sure that there aren't any pending event for this PASID languishing in the
fault queues.
The main test is queue_empty(). If that succeeds then we know that there
aren't any pending event (and the PASID is safe to reuse). But if new
events are constantly added to the queue then we wait for the evtq thread
to handle a full batch, where one batch corresponds to the queue size. For
that we take the batch number when entering flush(), and wait for the evtq
thread to increment it twice.
> Is there a potential race here? q->batch could have updated before we take
> a local copy.
Yes we're just checking on the progress of the evtq thread. All accesses
to batch are made while holding the wq lock.
Flush is a rare event so the lock isn't contended, but the wake_up() that
this patch introduces in arm_smmu_evtq_thread() does add some overhead
(0.85% of arm_smmu_evtq_thread(), according to perf). It would be nice to
get rid of it but I haven't found anything clever yet.
Thanks,
Jean
>
> > + ret = wait_event_interruptible_locked(q->wq, queue_empty(>llq) ||
> > + q->batch >= batch + 2);
> > + spin_unlock(>wq.lock);
> > +
> > + return ret;
> > +}
> > +
> ...
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
Hello, On 28.02.2020 04:57, Bjorn Andersson wrote: On Mon 09 Dec 07:07 PST 2019, Thierry Reding wrote: From: Thierry Reding Sorry for the slow response on this, finally got the time to go through this in detail and try it out on some Qualcomm boards. On some platforms, the firmware will setup hardware to read from a given region of memory. One such example is a display controller that is scanning out a splash screen from physical memory. This particular use case is the one that we need to figure out for Qualcomm devices as well; on some devices it's a simple splash screen (that on many devices can be disabled), but for others we have EFIFB on the display and no (sane) means to disable this. During Linux' boot process, the ARM SMMU will configure all contexts to fault by default. This means that memory accesses that happen by an SMMU master before its driver has had a chance to properly set up the IOMMU will cause a fault. This is especially annoying for something like the display controller scanning out a splash screen because the faults will result in the display controller getting bogus data (all-ones on Tegra) and since it repeatedly scans that framebuffer, it will keep triggering such faults and spam the boot log with them. As my proposed patches indicated, the Qualcomm platform boots with stream mapping setup for the hardware used by the bootloader, but relying on the associated context banks not being enabled. USFCFG in SCR0 is set and any faults resulting of this will trap into secure world and the device will be reset. In order to work around such problems, scan the device tree for IOMMU masters and set up a special identity domain that will map 1:1 all of the reserved regions associated with them. This happens before the SMMU is enabled, so that the mappings are already set up before translations begin. One thing that was pointed out earlier, and which I don't have a good idea on how to solve it, is that the early identity domain is not discarded. The assumption is that the standard direct mappings code of the IOMMU framework will replace the early identity domain once devices are properly attached to domains, but we don't have a good point in time when it would be safe to remove the early identity domain. One option that I can think of would be to create an early identity domain for each master and inherit it when that master is attached to the domain later on, but that seems rather complicated from an book- keeping point of view and tricky because we need to be careful not to map regions twice, etc. The one concern I ran into with this approach (after resolving below issues) is that when the display driver probes a new domain will be created automatically and I get a stream of "Unhandled context fault" in the log until the driver has mapped the framebuffer in the newly allocated context. This is normally not a problem, as we seem to be able to do this initialization in a few frames, but for the cases where the display driver probe defer this is a problem. Also gave this a go on one of NXP's layerscape platforms, and encountered the same issue. However, given that in our case it's not about a framebuffer device but a firmware, it cause it to crash. :-( Another apparent problem is that in the current implementation only one memory-region per device is supported. Actually it appears that this is a limitation of the DT reservation binding - it doesn't seem to allow specifying multiple regions per device. In our firmware case we would need support for multiple reserved regions (FW memory, FW i/o registers a.s.o). --- Best Regards, Laurentiu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
Hi Michael, On Tue, Mar 03, 2020 at 11:09:41AM -0500, Michael S. Tsirkin wrote: > No. It's coded into the hardware. Which might even be practical > for bare-metal (e.g. on-board flash), but is very practical > when the device is part of a hypervisor. If its that way on PPC, than fine for them. But since this is enablement for x86, it should follow the x86 platform best practices, and that means describing hardware through ACPI. > This "hardware" is actually part of hypervisor so there's no > reason it can't be completely self-descriptive. It's specified > by the same entity as the "firmware". That is just an implementation detail. Yes, QEMU emulates the hardware and builds the ACPI tables. But it could also be implemented in a way where the ACPI tables are build by guest firmware. > I don't see why it would be much faster. The interface isn't that > different from command queues of VTD. VirtIO IOMMU doesn't need to build page-tables that the hypervisor then has to shadow, which makes things much faster. If you emulate one of the other IOMMUs (VT-d or AMD-Vi) the code has to shadow the full page-table at once when device passthrough is used. VirtIO-IOMMU doesn't need that, and that makes it much faster and efficient. > Making ACPI meet the goals of embedded projects such as kata containers > would be a gigantic task with huge stability implications. By > comparison this 400-line parser is well contained and does the job. I > didn't yet see compelling reasons not to merge this, but I'll be > interested to see some more specific concerns. An ACPI table parse wouldn't need more lines of code. For embedded systems there is still the DT way of describing things. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/14] iommu: Move iommu_fwspec out of 'struct device'
Hi Will, On Tue, Mar 03, 2020 at 07:16:25PM +, Will Deacon wrote: > I haven't had a chance to review this properly yet, but I did take it > for a spin on my Seattle board with MMU-400 (arm-smmu) and it seems to > work the same as before, so: > > Tested-by: Will Deacon # arm-smmu > > I'll try to review the patches soon. Thanks for testing it! I will send out a new version probably beginning of next week (I am travelling this week) to fix the kbuild issue and anything you might find. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/mediatek: add pdata member for legacy ivrp paddr
On Wed, 2020-03-04 at 20:23 +0800, Yong Wu wrote:
> On Mon, 2020-03-02 at 12:21 +0100, Fabien Parent wrote:
> > Add a new platform data member in order to select which IVRP_PADDR
> > format is used by an SoC.
> >
> > Signed-off-by: Fabien Parent
> > ---
> >
> > v2: new patch
> >
> > ---
> > drivers/iommu/mtk_iommu.c | 3 ++-
> > drivers/iommu/mtk_iommu.h | 1 +
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 95945f467c03..78cb14ab7dd0 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -569,7 +569,7 @@ static int mtk_iommu_hw_init(const struct
> > mtk_iommu_data *data)
> > F_INT_PRETETCH_TRANSATION_FIFO_FAULT;
> > writel_relaxed(regval, data->base + REG_MMU_INT_MAIN_CONTROL);
> >
> > - if (data->plat_data->m4u_plat == M4U_MT8173)
> > + if (data->plat_data->has_legacy_ivrp_paddr)
> > regval = (data->protect_base >> 1) | (data->enable_4GB << 31);
> > else
> > regval = lower_32_bits(data->protect_base) |
> > @@ -786,6 +786,7 @@ static const struct mtk_iommu_plat_data mt8173_data = {
> > .m4u_plat = M4U_MT8173,
> > .has_4gb_mode = true,
> > .has_bclk = true,
> > + .has_legacy_ivrp_paddr = true;
,
> > .reset_axi= true,
> > .larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
> > };
> > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> > index ea949a324e33..4696ba027a71 100644
> > --- a/drivers/iommu/mtk_iommu.h
> > +++ b/drivers/iommu/mtk_iommu.h
> > @@ -42,6 +42,7 @@ struct mtk_iommu_plat_data {
> > boolhas_bclk;
> > boolhas_vld_pa_rng;
> > boolreset_axi;
> > + boolhas_legacy_ivrp_paddr;
>
> I'd like to put this before "has_vld_pa_rng" alphabetically.
>
> Other than this,
>
> Reviewed-by: Yong Wu
>
> > unsigned char larbid_remap[MTK_LARB_NR_MAX];
> > };
> >
>
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/3] iommu/mediatek: add support for MT8167
On Mon, 2020-03-02 at 12:21 +0100, Fabien Parent wrote:
> Add support for the IOMMU on MT8167
>
> Signed-off-by: Fabien Parent
> ---
>
> V2:
> * removed if based on m4u_plat, and using instead the new
> has_legacy_ivrp_paddr member that was introduced in patch 2.
>
> ---
> drivers/iommu/mtk_iommu.c | 9 +
> drivers/iommu/mtk_iommu.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 78cb14ab7dd0..25b7ad1647ba 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -782,6 +782,14 @@ static const struct mtk_iommu_plat_data mt2712_data = {
> .larbid_remap = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
> };
>
> +static const struct mtk_iommu_plat_data mt8167_data = {
> + .m4u_plat = M4U_MT8167,
> + .has_4gb_mode = true,
> + .has_legacy_ivrp_paddr = true;
> + .reset_axi= true,
> + .larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
Normally we put the file include/dt-bindings/memory/mt8167-larb-port.h
into the first binding patch.
If we have that file, we will know there is only 3 larbs in mt8167.
thus, here should be: larbid_remap = {0, 1, 2}
Other than this,
Reviewed-by: Yong Wu
> +};
> +
> static const struct mtk_iommu_plat_data mt8173_data = {
> .m4u_plat = M4U_MT8173,
> .has_4gb_mode = true,
> @@ -799,6 +807,7 @@ static const struct mtk_iommu_plat_data mt8183_data = {
>
> static const struct of_device_id mtk_iommu_of_ids[] = {
> { .compatible = "mediatek,mt2712-m4u", .data = _data},
> + { .compatible = "mediatek,mt8167-m4u", .data = _data},
> { .compatible = "mediatek,mt8173-m4u", .data = _data},
> { .compatible = "mediatek,mt8183-m4u", .data = _data},
> {}
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index 4696ba027a71..72f874ec9e9c 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -30,6 +30,7 @@ struct mtk_iommu_suspend_reg {
> enum mtk_iommu_plat {
> M4U_MT2701,
> M4U_MT2712,
> + M4U_MT8167,
> M4U_MT8173,
> M4U_MT8183,
> };
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/mediatek: add pdata member for legacy ivrp paddr
On Mon, 2020-03-02 at 12:21 +0100, Fabien Parent wrote:
> Add a new platform data member in order to select which IVRP_PADDR
> format is used by an SoC.
>
> Signed-off-by: Fabien Parent
> ---
>
> v2: new patch
>
> ---
> drivers/iommu/mtk_iommu.c | 3 ++-
> drivers/iommu/mtk_iommu.h | 1 +
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 95945f467c03..78cb14ab7dd0 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -569,7 +569,7 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data
> *data)
> F_INT_PRETETCH_TRANSATION_FIFO_FAULT;
> writel_relaxed(regval, data->base + REG_MMU_INT_MAIN_CONTROL);
>
> - if (data->plat_data->m4u_plat == M4U_MT8173)
> + if (data->plat_data->has_legacy_ivrp_paddr)
> regval = (data->protect_base >> 1) | (data->enable_4GB << 31);
> else
> regval = lower_32_bits(data->protect_base) |
> @@ -786,6 +786,7 @@ static const struct mtk_iommu_plat_data mt8173_data = {
> .m4u_plat = M4U_MT8173,
> .has_4gb_mode = true,
> .has_bclk = true,
> + .has_legacy_ivrp_paddr = true;
> .reset_axi= true,
> .larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
> };
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index ea949a324e33..4696ba027a71 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -42,6 +42,7 @@ struct mtk_iommu_plat_data {
> boolhas_bclk;
> boolhas_vld_pa_rng;
> boolreset_axi;
> + boolhas_legacy_ivrp_paddr;
I'd like to put this before "has_vld_pa_rng" alphabetically.
Other than this,
Reviewed-by: Yong Wu
> unsigned char larbid_remap[MTK_LARB_NR_MAX];
> };
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 3/5] iommu: Add support to change default domain of an iommu_group
Hi Joerg, On 2020/3/3 21:13, Joerg Roedel wrote: Hi Baolu, On Tue, Mar 03, 2020 at 02:47:02PM +0800, Lu Baolu wrote: Theoretically speaking, per-device default domain is impractical. PCI aliased devices (PCI bridge and all devices beneath it, VMD devices and various devices quirked with pci_add_dma_alias()) must use the same domain. It's likely that we have to introduce something like a sub-group with all PCI aliased devices staying in it. Current private-domain implementation in the vt-d driver was introduced for compatible purpose and I wanted to abandon it from the first day. :-) What hinders you from removing it now? I looked a bit closer into these private default domain implementation and it is very fragile. If it can be removed, then better do so sooner than later. I will soon send out the patches for review. Probably, we are able to configure per-group default domain type with below two interfaces. - (ops->)dev_def_domain_type: Return the required default domain type for a device. It returns - IOMMU_DOMAIN_DMA (device must use a DMA domain), unlikely - IOMMU_DOMAIN_IDENTITY (device must use an Identity domain), unlikely - 0 (both are okay), likely If we stay at the group level, this interface should work on the group level too, and not on the device level. - iommu_group_change_def_domain: Change the default domain of a group Works only when all devices have no driver bond. Btw, I have no objections about the concept of private default domains for devices, but the implementation should be moved to generic IOMMU code so that the behavior is consistent accross differnet IOMMU platforms, and of course be robust. Yes. I agree with you. Regards, Joerg Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: silence iommu group prints
On 04.03.2020 12:52, Russell King - ARM Linux admin wrote: On Wed, Mar 04, 2020 at 12:33:14PM +0200, Laurentiu Tudor wrote: On 04.03.2020 12:07, Russell King - ARM Linux admin wrote: On Wed, Mar 04, 2020 at 11:56:53AM +0200, Laurentiu Tudor wrote: From 44ae46501b5379bd0890df87fd3827248626ed03 Mon Sep 17 00:00:00 2001 From: Laurentiu Tudor Date: Tue, 1 Oct 2019 16:22:48 +0300 Subject: [PATCH 1/6] bus: fsl-mc: make mc work with smmu disable bypass on Content-Type: text/plain; charset="us-ascii" Since this commit [1] booting kernel on MC based chips started to fail because this firmware starts before the kernel and as soon as SMMU is probed it starts to trigger contiguous faults. I think you mean "continuous" here. Yes, thanks. This is a workaround that allows MC firmware to run with SMMU in disable bypass mode. It consists of the following steps: 1. pause the firmware at early boot to get a chance to setup SMMU 2. request direct mapping for MC device 3. resume the firmware The workaround relies on the fact that no state is lost when pausing / resuming firmware, as per the docs. With this patch, platforms with MC firmware can now boot without having to change the default config to set: CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n This alone is a definite improvement, and has been needed for a while. Please submit this patch with an appropriate Fixes: tag so stable trees can pick it up. The thing is that probably this workaround will never make it in the kernel because it questionable to say the least, e.g. see [1]. My plan is to give this approach [2] a try sometime soon. So, if we want to reduce the iommu noise, we need to completely break the ability to use a mainline kernel on the LX2160A. This doesn't seem practical nor sensible. Someone has to give. Well, I think it's a bit too early for such conclusions. I'd consider this stuff early / experimental work, probably will take quite a while for the dust to settle. Anyway, I'll take care not to break the kernel when I'll start submitting more official patches. For now, I'm just hoping that this stuff helps fixing your local tree. --- Best Regards, Laurentiu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: silence iommu group prints
On Wed, Mar 04, 2020 at 12:33:14PM +0200, Laurentiu Tudor wrote: > > > On 04.03.2020 12:07, Russell King - ARM Linux admin wrote: > > On Wed, Mar 04, 2020 at 11:56:53AM +0200, Laurentiu Tudor wrote: > > > From 44ae46501b5379bd0890df87fd3827248626ed03 Mon Sep 17 00:00:00 2001 > > > From: Laurentiu Tudor > > > Date: Tue, 1 Oct 2019 16:22:48 +0300 > > > Subject: [PATCH 1/6] bus: fsl-mc: make mc work with smmu disable bypass on > > > Content-Type: text/plain; charset="us-ascii" > > > > > > Since this commit [1] booting kernel on MC based chips started to > > > fail because this firmware starts before the kernel and as soon as > > > SMMU is probed it starts to trigger contiguous faults. > > > > I think you mean "continuous" here. > > Yes, thanks. > > > > This is a workaround that allows MC firmware to run with SMMU > > > in disable bypass mode. It consists of the following steps: > > > 1. pause the firmware at early boot to get a chance to setup SMMU > > > 2. request direct mapping for MC device > > > 3. resume the firmware > > > The workaround relies on the fact that no state is lost when > > > pausing / resuming firmware, as per the docs. > > > With this patch, platforms with MC firmware can now boot without > > > having to change the default config to set: > > >CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n > > > > This alone is a definite improvement, and has been needed for a while. > > Please submit this patch with an appropriate Fixes: tag so stable trees > > can pick it up. > > The thing is that probably this workaround will never make it in the kernel > because it questionable to say the least, e.g. see [1]. My plan is to give > this approach [2] a try sometime soon. > > [1] https://patchwork.kernel.org/comment/23149049/ > [2] https://patchwork.kernel.org/cover/11279577/ So, if we want to reduce the iommu noise, we need to completely break the ability to use a mainline kernel on the LX2160A. This doesn't seem practical nor sensible. Someone has to give. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: silence iommu group prints
On 04.03.2020 12:07, Russell King - ARM Linux admin wrote:
On Wed, Mar 04, 2020 at 11:56:53AM +0200, Laurentiu Tudor wrote:
From 44ae46501b5379bd0890df87fd3827248626ed03 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor
Date: Tue, 1 Oct 2019 16:22:48 +0300
Subject: [PATCH 1/6] bus: fsl-mc: make mc work with smmu disable bypass on
Content-Type: text/plain; charset="us-ascii"
Since this commit [1] booting kernel on MC based chips started to
fail because this firmware starts before the kernel and as soon as
SMMU is probed it starts to trigger contiguous faults.
I think you mean "continuous" here.
Yes, thanks.
This is a workaround that allows MC firmware to run with SMMU
in disable bypass mode. It consists of the following steps:
1. pause the firmware at early boot to get a chance to setup SMMU
2. request direct mapping for MC device
3. resume the firmware
The workaround relies on the fact that no state is lost when
pausing / resuming firmware, as per the docs.
With this patch, platforms with MC firmware can now boot without
having to change the default config to set:
CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n
This alone is a definite improvement, and has been needed for a while.
Please submit this patch with an appropriate Fixes: tag so stable trees
can pick it up.
The thing is that probably this workaround will never make it in the
kernel because it questionable to say the least, e.g. see [1]. My plan
is to give this approach [2] a try sometime soon.
[1] https://patchwork.kernel.org/comment/23149049/
[2] https://patchwork.kernel.org/cover/11279577/
---
Best Regards, Laurentiu
[1] 954a03be033 ("iommu/arm-smmu: Break insecure users by disabling bypass by
default")
Please put this where you're referencing it above - it's fine to wrap
the description of the commit when using it in the body of the commit
message. However, that should _never_ when providing a Fixes: tag
(linux-next has a script which will detect and complain about broken
Fixes: tags.)
Thanks.
Signed-off-by: Laurentiu Tudor
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 53 +
1 file changed, 53 insertions(+)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index a0f8854acb3a..683a6401ffe8 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -18,6 +18,8 @@
#include
#include
#include
+#include
+#include
#include "fsl-mc-private.h"
@@ -889,6 +891,12 @@ static int get_mc_addr_translation_ranges(struct device *dev,
return 0;
}
+#define FSL_MC_GCR1 0x0
+#define GCR1_P1_STOP BIT(31)
+
+static u32 boot_gcr1;
+static void __iomem *fsl_mc_regs;
+
/**
* fsl_mc_bus_probe - callback invoked when the root MC bus is being
* added
@@ -906,6 +914,21 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
struct mc_version mc_version;
struct resource res;
+ /*
+* The MC firmware requires full access to the whole address space plus
+* it has no way of dealing with any kind of address translation, so
+* request direct mapping for it.
+*/
+ error = iommu_request_dm_for_dev(>dev);
+ if (error)
+ dev_warn(>dev, "iommu_request_dm_for_dev() failed: %d\n",
+error);
+
+ if (fsl_mc_regs) {
+ /* Resume the firmware */
+ writel(boot_gcr1 & ~GCR1_P1_STOP, fsl_mc_regs + FSL_MC_GCR1);
+ }
+
mc = devm_kzalloc(>dev, sizeof(*mc), GFP_KERNEL);
if (!mc)
return -ENOMEM;
@@ -990,6 +1013,13 @@ static int fsl_mc_bus_remove(struct platform_device *pdev)
if (!fsl_mc_is_root_dprc(>root_mc_bus_dev->dev))
return -EINVAL;
+ /*
+* Pause back the firmware so that it doesn't trigger faults by the
+* time SMMU gets brought down.
+*/
+ writel(boot_gcr1 | GCR1_P1_STOP, fsl_mc_regs + FSL_MC_GCR1);
+ iounmap(fsl_mc_regs);
+
fsl_mc_device_remove(mc->root_mc_bus_dev);
fsl_destroy_mc_io(mc->root_mc_bus_dev->mc_io);
@@ -1018,6 +1048,8 @@ static struct platform_driver fsl_mc_bus_driver = {
static int __init fsl_mc_bus_driver_init(void)
{
int error;
+ struct device_node *np;
+ struct resource res;
error = bus_register(_mc_bus_type);
if (error < 0) {
@@ -1039,9 +1071,30 @@ static int __init fsl_mc_bus_driver_init(void)
if (error < 0)
goto error_cleanup_dprc_driver;
+ np = of_find_matching_node(NULL, fsl_mc_bus_match_table);
+ if (np && of_device_is_available(np)) {
+ error = of_address_to_resource(np, 1, );
+ if (error)
+ goto error_cleanup_dprc_driver;
+ fsl_mc_regs = ioremap(res.start, resource_size());
+ if (!fsl_mc_regs) {
+ error = -ENXIO;
+ goto error_cleanup_dprc_driver;
+
Re: [PATCH] iommu: silence iommu group prints
On 04.03.2020 11:51, Russell King - ARM Linux admin wrote:
On Wed, Mar 04, 2020 at 11:42:16AM +0200, Laurentiu Tudor wrote:
On 04.03.2020 11:33, Russell King - ARM Linux admin wrote:
On Wed, Mar 04, 2020 at 10:56:06AM +0200, Laurentiu Tudor wrote:
On 04.03.2020 00:17, Russell King - ARM Linux admin wrote:
On Tue, Mar 03, 2020 at 05:55:05PM +0200, Laurentiu Tudor wrote:
From c98dc05cdd45ae923654f2427985bd28bcde4bb2 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor
Date: Thu, 13 Feb 2020 11:59:12 +0200
Subject: [PATCH 1/4] bus: fsl-mc: add custom .dma_configure implementation
Content-Type: text/plain; charset="us-ascii"
The devices on this bus are not discovered by way of device tree
but by queries to the firmware. It makes little sense to trick the
generic of layer into thinking that these devices are of related so
that we can get our dma configuration. Instead of doing that, add
our custom dma configuration implementation.
Firstly, applying this to v5.5 results in a build failure, due to a
missing linux/iommu.h include.
Secondly, this on its own appears to make the DPAA2 network interfaces
completely disappear. Looking in /sys/bus/fsl-mc/drivers/*, none of
the DPAA2 drivers are bound to anything, and looking in
/sys/bus/fsl-mc/devices/, there is:
lrwxrwxrwx 1 root root 0 Mar 3 22:06 dprc.1 ->
../../../devices/platform/soc/80c00.fsl-mc/dprc.1
This is booting with u-boot, so using DT rather than ACPI.
Signed-off-by: Laurentiu Tudor
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 42 -
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 36eb25f82c8e..3df015eedae4 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -132,11 +132,51 @@ static int fsl_mc_bus_uevent(struct device *dev, struct
kobj_uevent_env *env)
static int fsl_mc_dma_configure(struct device *dev)
{
struct device *dma_dev = dev;
+ struct iommu_fwspec *fwspec;
+ const struct iommu_ops *iommu_ops;
+ struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+ int ret;
+ u32 icid;
+
+ /* Skip DMA setup for devices that are not DMA masters */
+ if (dev->type == _mc_bus_dpmcp_type ||
+ dev->type == _mc_bus_dpbp_type ||
+ dev->type == _mc_bus_dpcon_type ||
+ dev->type == _mc_bus_dpio_type)
+ return 0;
while (dev_is_fsl_mc(dma_dev))
dma_dev = dma_dev->parent;
- return of_dma_configure(dev, dma_dev->of_node, 0);
+ fwspec = dev_iommu_fwspec_get(dma_dev);
+ if (!fwspec)
+ return -ENODEV;
The problem appears to be here - fwspec is NULL for dprc.1.
Ok, that's because the iommu config is missing from the DT node that's
exposing the MC firmware. I've attached a fresh set of patches that include
on top the missing config and a workaround that makes MC work over SMMU.
Also added the missing #include, thanks for pointing out.
Let me know how it goes.
Shouldn't patch 6 should be first in the series, so that patch 1 does
not cause a regression and bisectability damage?
Correct, width one comment: both 5 and 6 should go first. Once iommu-map is
added in the device tree the workaround for MC with the
arm-smmu.disable_bypass boot arg will no longer work.
So, wouldn't it be reasonable to arrange the patch series like that?
Sure, please see attached.
---
Best Regards, Laurentiu
>From 44ae46501b5379bd0890df87fd3827248626ed03 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor
Date: Tue, 1 Oct 2019 16:22:48 +0300
Subject: [PATCH 1/6] bus: fsl-mc: make mc work with smmu disable bypass on
Content-Type: text/plain; charset="us-ascii"
Since this commit [1] booting kernel on MC based chips started to
fail because this firmware starts before the kernel and as soon as
SMMU is probed it starts to trigger contiguous faults.
This is a workaround that allows MC firmware to run with SMMU
in disable bypass mode. It consists of the following steps:
1. pause the firmware at early boot to get a chance to setup SMMU
2. request direct mapping for MC device
3. resume the firmware
The workaround relies on the fact that no state is lost when
pausing / resuming firmware, as per the docs.
With this patch, platforms with MC firmware can now boot without
having to change the default config to set:
CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n
[1] 954a03be033 ("iommu/arm-smmu: Break insecure users by disabling bypass by default")
Signed-off-by: Laurentiu Tudor
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 53 +
1 file changed, 53 insertions(+)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index a0f8854acb3a..683a6401ffe8 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -18,6 +18,8 @@
#include
#include
#include
+#include
+#include
#include
Re: [PATCH] iommu: silence iommu group prints
On Wed, Mar 04, 2020 at 11:56:53AM +0200, Laurentiu Tudor wrote:
> From 44ae46501b5379bd0890df87fd3827248626ed03 Mon Sep 17 00:00:00 2001
> From: Laurentiu Tudor
> Date: Tue, 1 Oct 2019 16:22:48 +0300
> Subject: [PATCH 1/6] bus: fsl-mc: make mc work with smmu disable bypass on
> Content-Type: text/plain; charset="us-ascii"
>
> Since this commit [1] booting kernel on MC based chips started to
> fail because this firmware starts before the kernel and as soon as
> SMMU is probed it starts to trigger contiguous faults.
I think you mean "continuous" here.
> This is a workaround that allows MC firmware to run with SMMU
> in disable bypass mode. It consists of the following steps:
> 1. pause the firmware at early boot to get a chance to setup SMMU
> 2. request direct mapping for MC device
> 3. resume the firmware
> The workaround relies on the fact that no state is lost when
> pausing / resuming firmware, as per the docs.
> With this patch, platforms with MC firmware can now boot without
> having to change the default config to set:
> CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n
This alone is a definite improvement, and has been needed for a while.
Please submit this patch with an appropriate Fixes: tag so stable trees
can pick it up.
> [1] 954a03be033 ("iommu/arm-smmu: Break insecure users by disabling bypass by
> default")
Please put this where you're referencing it above - it's fine to wrap
the description of the commit when using it in the body of the commit
message. However, that should _never_ when providing a Fixes: tag
(linux-next has a script which will detect and complain about broken
Fixes: tags.)
Thanks.
>
> Signed-off-by: Laurentiu Tudor
> ---
> drivers/bus/fsl-mc/fsl-mc-bus.c | 53 +
> 1 file changed, 53 insertions(+)
>
> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index a0f8854acb3a..683a6401ffe8 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -18,6 +18,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include "fsl-mc-private.h"
>
> @@ -889,6 +891,12 @@ static int get_mc_addr_translation_ranges(struct device
> *dev,
> return 0;
> }
>
> +#define FSL_MC_GCR1 0x0
> +#define GCR1_P1_STOP BIT(31)
> +
> +static u32 boot_gcr1;
> +static void __iomem *fsl_mc_regs;
> +
> /**
> * fsl_mc_bus_probe - callback invoked when the root MC bus is being
> * added
> @@ -906,6 +914,21 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
> struct mc_version mc_version;
> struct resource res;
>
> + /*
> + * The MC firmware requires full access to the whole address space plus
> + * it has no way of dealing with any kind of address translation, so
> + * request direct mapping for it.
> + */
> + error = iommu_request_dm_for_dev(>dev);
> + if (error)
> + dev_warn(>dev, "iommu_request_dm_for_dev() failed: %d\n",
> + error);
> +
> + if (fsl_mc_regs) {
> + /* Resume the firmware */
> + writel(boot_gcr1 & ~GCR1_P1_STOP, fsl_mc_regs + FSL_MC_GCR1);
> + }
> +
> mc = devm_kzalloc(>dev, sizeof(*mc), GFP_KERNEL);
> if (!mc)
> return -ENOMEM;
> @@ -990,6 +1013,13 @@ static int fsl_mc_bus_remove(struct platform_device
> *pdev)
> if (!fsl_mc_is_root_dprc(>root_mc_bus_dev->dev))
> return -EINVAL;
>
> + /*
> + * Pause back the firmware so that it doesn't trigger faults by the
> + * time SMMU gets brought down.
> + */
> + writel(boot_gcr1 | GCR1_P1_STOP, fsl_mc_regs + FSL_MC_GCR1);
> + iounmap(fsl_mc_regs);
> +
> fsl_mc_device_remove(mc->root_mc_bus_dev);
>
> fsl_destroy_mc_io(mc->root_mc_bus_dev->mc_io);
> @@ -1018,6 +1048,8 @@ static struct platform_driver fsl_mc_bus_driver = {
> static int __init fsl_mc_bus_driver_init(void)
> {
> int error;
> + struct device_node *np;
> + struct resource res;
>
> error = bus_register(_mc_bus_type);
> if (error < 0) {
> @@ -1039,9 +1071,30 @@ static int __init fsl_mc_bus_driver_init(void)
> if (error < 0)
> goto error_cleanup_dprc_driver;
>
> + np = of_find_matching_node(NULL, fsl_mc_bus_match_table);
> + if (np && of_device_is_available(np)) {
> + error = of_address_to_resource(np, 1, );
> + if (error)
> + goto error_cleanup_dprc_driver;
> + fsl_mc_regs = ioremap(res.start, resource_size());
> + if (!fsl_mc_regs) {
> + error = -ENXIO;
> + goto error_cleanup_dprc_driver;
> + }
> +
> + boot_gcr1 = readl(fsl_mc_regs + FSL_MC_GCR1);
> + /*
> + * If found running, pause MC firmware in order to get a chance
> + * to setup SMMU for it.
> + */
> + if (!(boot_gcr1 & GCR1_P1_STOP))
Re: [PATCH] iommu: silence iommu group prints
On Wed, Mar 04, 2020 at 11:42:16AM +0200, Laurentiu Tudor wrote:
> On 04.03.2020 11:33, Russell King - ARM Linux admin wrote:
> > On Wed, Mar 04, 2020 at 10:56:06AM +0200, Laurentiu Tudor wrote:
> > >
> > > On 04.03.2020 00:17, Russell King - ARM Linux admin wrote:
> > > > On Tue, Mar 03, 2020 at 05:55:05PM +0200, Laurentiu Tudor wrote:
> > > > > From c98dc05cdd45ae923654f2427985bd28bcde4bb2 Mon Sep 17 00:00:00
> > > > > 2001
> > > > > From: Laurentiu Tudor
> > > > > Date: Thu, 13 Feb 2020 11:59:12 +0200
> > > > > Subject: [PATCH 1/4] bus: fsl-mc: add custom .dma_configure
> > > > > implementation
> > > > > Content-Type: text/plain; charset="us-ascii"
> > > > >
> > > > > The devices on this bus are not discovered by way of device tree
> > > > > but by queries to the firmware. It makes little sense to trick the
> > > > > generic of layer into thinking that these devices are of related so
> > > > > that we can get our dma configuration. Instead of doing that, add
> > > > > our custom dma configuration implementation.
> > > >
> > > > Firstly, applying this to v5.5 results in a build failure, due to a
> > > > missing linux/iommu.h include.
> > > >
> > > > Secondly, this on its own appears to make the DPAA2 network interfaces
> > > > completely disappear. Looking in /sys/bus/fsl-mc/drivers/*, none of
> > > > the DPAA2 drivers are bound to anything, and looking in
> > > > /sys/bus/fsl-mc/devices/, there is:
> > > >
> > > > lrwxrwxrwx 1 root root 0 Mar 3 22:06 dprc.1 ->
> > > > ../../../devices/platform/soc/80c00.fsl-mc/dprc.1
> > > >
> > > > This is booting with u-boot, so using DT rather than ACPI.
> > > >
> > > > > Signed-off-by: Laurentiu Tudor
> > > > > ---
> > > > >drivers/bus/fsl-mc/fsl-mc-bus.c | 42
> > > > > -
> > > > >1 file changed, 41 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > > > b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > > > index 36eb25f82c8e..3df015eedae4 100644
> > > > > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > > > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > > > @@ -132,11 +132,51 @@ static int fsl_mc_bus_uevent(struct device
> > > > > *dev, struct kobj_uevent_env *env)
> > > > >static int fsl_mc_dma_configure(struct device *dev)
> > > > >{
> > > > > struct device *dma_dev = dev;
> > > > > + struct iommu_fwspec *fwspec;
> > > > > + const struct iommu_ops *iommu_ops;
> > > > > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
> > > > > + int ret;
> > > > > + u32 icid;
> > > > > +
> > > > > + /* Skip DMA setup for devices that are not DMA masters */
> > > > > + if (dev->type == _mc_bus_dpmcp_type ||
> > > > > + dev->type == _mc_bus_dpbp_type ||
> > > > > + dev->type == _mc_bus_dpcon_type ||
> > > > > + dev->type == _mc_bus_dpio_type)
> > > > > + return 0;
> > > > > while (dev_is_fsl_mc(dma_dev))
> > > > > dma_dev = dma_dev->parent;
> > > > > - return of_dma_configure(dev, dma_dev->of_node, 0);
> > > > > + fwspec = dev_iommu_fwspec_get(dma_dev);
> > > > > + if (!fwspec)
> > > > > + return -ENODEV;
> > > >
> > > > The problem appears to be here - fwspec is NULL for dprc.1.
> > >
> > > Ok, that's because the iommu config is missing from the DT node that's
> > > exposing the MC firmware. I've attached a fresh set of patches that
> > > include
> > > on top the missing config and a workaround that makes MC work over SMMU.
> > > Also added the missing #include, thanks for pointing out.
> > > Let me know how it goes.
> >
> > Shouldn't patch 6 should be first in the series, so that patch 1 does
> > not cause a regression and bisectability damage?
> >
>
> Correct, width one comment: both 5 and 6 should go first. Once iommu-map is
> added in the device tree the workaround for MC with the
> arm-smmu.disable_bypass boot arg will no longer work.
So, wouldn't it be reasonable to arrange the patch series like that?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: silence iommu group prints
On 04.03.2020 11:33, Russell King - ARM Linux admin wrote:
On Wed, Mar 04, 2020 at 10:56:06AM +0200, Laurentiu Tudor wrote:
On 04.03.2020 00:17, Russell King - ARM Linux admin wrote:
On Tue, Mar 03, 2020 at 05:55:05PM +0200, Laurentiu Tudor wrote:
From c98dc05cdd45ae923654f2427985bd28bcde4bb2 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor
Date: Thu, 13 Feb 2020 11:59:12 +0200
Subject: [PATCH 1/4] bus: fsl-mc: add custom .dma_configure implementation
Content-Type: text/plain; charset="us-ascii"
The devices on this bus are not discovered by way of device tree
but by queries to the firmware. It makes little sense to trick the
generic of layer into thinking that these devices are of related so
that we can get our dma configuration. Instead of doing that, add
our custom dma configuration implementation.
Firstly, applying this to v5.5 results in a build failure, due to a
missing linux/iommu.h include.
Secondly, this on its own appears to make the DPAA2 network interfaces
completely disappear. Looking in /sys/bus/fsl-mc/drivers/*, none of
the DPAA2 drivers are bound to anything, and looking in
/sys/bus/fsl-mc/devices/, there is:
lrwxrwxrwx 1 root root 0 Mar 3 22:06 dprc.1 ->
../../../devices/platform/soc/80c00.fsl-mc/dprc.1
This is booting with u-boot, so using DT rather than ACPI.
Signed-off-by: Laurentiu Tudor
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 42 -
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 36eb25f82c8e..3df015eedae4 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -132,11 +132,51 @@ static int fsl_mc_bus_uevent(struct device *dev, struct
kobj_uevent_env *env)
static int fsl_mc_dma_configure(struct device *dev)
{
struct device *dma_dev = dev;
+ struct iommu_fwspec *fwspec;
+ const struct iommu_ops *iommu_ops;
+ struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+ int ret;
+ u32 icid;
+
+ /* Skip DMA setup for devices that are not DMA masters */
+ if (dev->type == _mc_bus_dpmcp_type ||
+ dev->type == _mc_bus_dpbp_type ||
+ dev->type == _mc_bus_dpcon_type ||
+ dev->type == _mc_bus_dpio_type)
+ return 0;
while (dev_is_fsl_mc(dma_dev))
dma_dev = dma_dev->parent;
- return of_dma_configure(dev, dma_dev->of_node, 0);
+ fwspec = dev_iommu_fwspec_get(dma_dev);
+ if (!fwspec)
+ return -ENODEV;
The problem appears to be here - fwspec is NULL for dprc.1.
Ok, that's because the iommu config is missing from the DT node that's
exposing the MC firmware. I've attached a fresh set of patches that include
on top the missing config and a workaround that makes MC work over SMMU.
Also added the missing #include, thanks for pointing out.
Let me know how it goes.
Shouldn't patch 6 should be first in the series, so that patch 1 does
not cause a regression and bisectability damage?
Correct, width one comment: both 5 and 6 should go first. Once iommu-map
is added in the device tree the workaround for MC with the
arm-smmu.disable_bypass boot arg will no longer work.
---
Best Regards, Laurentiu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: silence iommu group prints
On Wed, Mar 04, 2020 at 10:56:06AM +0200, Laurentiu Tudor wrote:
>
> On 04.03.2020 00:17, Russell King - ARM Linux admin wrote:
> > On Tue, Mar 03, 2020 at 05:55:05PM +0200, Laurentiu Tudor wrote:
> > > From c98dc05cdd45ae923654f2427985bd28bcde4bb2 Mon Sep 17 00:00:00 2001
> > > From: Laurentiu Tudor
> > > Date: Thu, 13 Feb 2020 11:59:12 +0200
> > > Subject: [PATCH 1/4] bus: fsl-mc: add custom .dma_configure implementation
> > > Content-Type: text/plain; charset="us-ascii"
> > >
> > > The devices on this bus are not discovered by way of device tree
> > > but by queries to the firmware. It makes little sense to trick the
> > > generic of layer into thinking that these devices are of related so
> > > that we can get our dma configuration. Instead of doing that, add
> > > our custom dma configuration implementation.
> >
> > Firstly, applying this to v5.5 results in a build failure, due to a
> > missing linux/iommu.h include.
> >
> > Secondly, this on its own appears to make the DPAA2 network interfaces
> > completely disappear. Looking in /sys/bus/fsl-mc/drivers/*, none of
> > the DPAA2 drivers are bound to anything, and looking in
> > /sys/bus/fsl-mc/devices/, there is:
> >
> > lrwxrwxrwx 1 root root 0 Mar 3 22:06 dprc.1 ->
> > ../../../devices/platform/soc/80c00.fsl-mc/dprc.1
> >
> > This is booting with u-boot, so using DT rather than ACPI.
> >
> > > Signed-off-by: Laurentiu Tudor
> > > ---
> > > drivers/bus/fsl-mc/fsl-mc-bus.c | 42 -
> > > 1 file changed, 41 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > index 36eb25f82c8e..3df015eedae4 100644
> > > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > @@ -132,11 +132,51 @@ static int fsl_mc_bus_uevent(struct device *dev,
> > > struct kobj_uevent_env *env)
> > > static int fsl_mc_dma_configure(struct device *dev)
> > > {
> > > struct device *dma_dev = dev;
> > > + struct iommu_fwspec *fwspec;
> > > + const struct iommu_ops *iommu_ops;
> > > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
> > > + int ret;
> > > + u32 icid;
> > > +
> > > + /* Skip DMA setup for devices that are not DMA masters */
> > > + if (dev->type == _mc_bus_dpmcp_type ||
> > > + dev->type == _mc_bus_dpbp_type ||
> > > + dev->type == _mc_bus_dpcon_type ||
> > > + dev->type == _mc_bus_dpio_type)
> > > + return 0;
> > > while (dev_is_fsl_mc(dma_dev))
> > > dma_dev = dma_dev->parent;
> > > - return of_dma_configure(dev, dma_dev->of_node, 0);
> > > + fwspec = dev_iommu_fwspec_get(dma_dev);
> > > + if (!fwspec)
> > > + return -ENODEV;
> >
> > The problem appears to be here - fwspec is NULL for dprc.1.
>
> Ok, that's because the iommu config is missing from the DT node that's
> exposing the MC firmware. I've attached a fresh set of patches that include
> on top the missing config and a workaround that makes MC work over SMMU.
> Also added the missing #include, thanks for pointing out.
> Let me know how it goes.
Shouldn't patch 6 should be first in the series, so that patch 1 does
not cause a regression and bisectability damage?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: silence iommu group prints
On 04.03.2020 00:17, Russell King - ARM Linux admin wrote:
On Tue, Mar 03, 2020 at 05:55:05PM +0200, Laurentiu Tudor wrote:
From c98dc05cdd45ae923654f2427985bd28bcde4bb2 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor
Date: Thu, 13 Feb 2020 11:59:12 +0200
Subject: [PATCH 1/4] bus: fsl-mc: add custom .dma_configure implementation
Content-Type: text/plain; charset="us-ascii"
The devices on this bus are not discovered by way of device tree
but by queries to the firmware. It makes little sense to trick the
generic of layer into thinking that these devices are of related so
that we can get our dma configuration. Instead of doing that, add
our custom dma configuration implementation.
Firstly, applying this to v5.5 results in a build failure, due to a
missing linux/iommu.h include.
Secondly, this on its own appears to make the DPAA2 network interfaces
completely disappear. Looking in /sys/bus/fsl-mc/drivers/*, none of
the DPAA2 drivers are bound to anything, and looking in
/sys/bus/fsl-mc/devices/, there is:
lrwxrwxrwx 1 root root 0 Mar 3 22:06 dprc.1 ->
../../../devices/platform/soc/80c00.fsl-mc/dprc.1
This is booting with u-boot, so using DT rather than ACPI.
Signed-off-by: Laurentiu Tudor
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 42 -
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 36eb25f82c8e..3df015eedae4 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -132,11 +132,51 @@ static int fsl_mc_bus_uevent(struct device *dev, struct
kobj_uevent_env *env)
static int fsl_mc_dma_configure(struct device *dev)
{
struct device *dma_dev = dev;
+ struct iommu_fwspec *fwspec;
+ const struct iommu_ops *iommu_ops;
+ struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+ int ret;
+ u32 icid;
+
+ /* Skip DMA setup for devices that are not DMA masters */
+ if (dev->type == _mc_bus_dpmcp_type ||
+ dev->type == _mc_bus_dpbp_type ||
+ dev->type == _mc_bus_dpcon_type ||
+ dev->type == _mc_bus_dpio_type)
+ return 0;
while (dev_is_fsl_mc(dma_dev))
dma_dev = dma_dev->parent;
- return of_dma_configure(dev, dma_dev->of_node, 0);
+ fwspec = dev_iommu_fwspec_get(dma_dev);
+ if (!fwspec)
+ return -ENODEV;
The problem appears to be here - fwspec is NULL for dprc.1.
Ok, that's because the iommu config is missing from the DT node that's
exposing the MC firmware. I've attached a fresh set of patches that
include on top the missing config and a workaround that makes MC work
over SMMU. Also added the missing #include, thanks for pointing out.
Let me know how it goes.
---
Best Regards, Laurentiu
>From 3d418f04fed9c10b64b3b861d66378a8f7514cc4 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor
Date: Thu, 13 Feb 2020 11:59:12 +0200
Subject: [PATCH 1/6] bus: fsl-mc: add custom .dma_configure implementation
Content-Type: text/plain; charset="us-ascii"
The devices on this bus are not discovered by way of device tree
but by queries to the firmware. It makes little sense to trick the
generic of layer into thinking that these devices are of related so
that we can get our dma configuration. Instead of doing that, add
our custom dma configuration implementation.
Signed-off-by: Laurentiu Tudor
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 43 -
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index a0f8854acb3a..e2682fbefb42 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include "fsl-mc-private.h"
@@ -130,11 +131,51 @@ static int fsl_mc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
static int fsl_mc_dma_configure(struct device *dev)
{
struct device *dma_dev = dev;
+ struct iommu_fwspec *fwspec;
+ const struct iommu_ops *iommu_ops;
+ struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+ int ret;
+ u32 icid;
+
+ /* Skip DMA setup for devices that are not DMA masters */
+ if (dev->type == _mc_bus_dpmcp_type ||
+ dev->type == _mc_bus_dpbp_type ||
+ dev->type == _mc_bus_dpcon_type ||
+ dev->type == _mc_bus_dpio_type)
+ return 0;
while (dev_is_fsl_mc(dma_dev))
dma_dev = dma_dev->parent;
- return of_dma_configure(dev, dma_dev->of_node, 0);
+ fwspec = dev_iommu_fwspec_get(dma_dev);
+ if (!fwspec)
+ return -ENODEV;
+ iommu_ops = iommu_ops_from_fwnode(fwspec->iommu_fwnode);
+ if (!iommu_ops)
+ return -ENODEV;
+
+ ret = iommu_fwspec_init(dev, fwspec->iommu_fwnode, iommu_ops);
+ if (ret)
+ return ret;
+
+ icid = mc_dev->icid;
+ ret = iommu_fwspec_add_ids(dev, , 1);
+ if (ret) {
+ iommu_fwspec_free(dev);
+ return ret;
+ }
+
+ if (!device_iommu_mapped(dev)) {
+ ret =
