iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: Jordan Crouse <jcrouse@codeaurora.org>
To: Rob Clark <robdclark@gmail.com>
Cc: freedreno <freedreno@lists.freedesktop.org>,
	Joerg Roedel <jroedel@suse.de>, Will Deacon <will@kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,
	" <iommu@lists.linux-foundation.org>,
	John Stultz <john.stultz@linaro.org>,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [Freedreno] [PATCH v2 2/6] iommu/io-pgtable: Allow a pgtable implementation to skip TLB operations
Date: Wed, 8 Jul 2020 13:19:23 -0600	[thread overview]
Message-ID: <20200708191923.GA21059@jcrouse1-lnx.qualcomm.com> (raw)
In-Reply-To: <CAF6AEGttgRQ3Ug2Nw_vJgjjjUOc0OLPSs+xLv-XFHNrHTOL8sg@mail.gmail.com>

On Tue, Jul 07, 2020 at 07:58:18AM -0700, Rob Clark wrote:
> On Tue, Jul 7, 2020 at 7:25 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Tue, Jul 7, 2020 at 4:34 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > >
> > > On 2020-06-26 21:04, Jordan Crouse wrote:
> > > > Allow a io-pgtable implementation to skip TLB operations by checking for
> > > > NULL pointers in the helper functions. It will be up to to the owner
> > > > of the io-pgtable instance to make sure that they independently handle
> > > > the TLB correctly.
> > >
> > > I don't really understand what this is for - tricking the IOMMU driver
> > > into not performing its TLB maintenance at points when that maintenance
> > > has been deemed necessary doesn't seem like the appropriate way to
> > > achieve anything good :/
> >
> > No, for triggering the io-pgtable helpers into not performing TLB
> > maintenance.  But seriously, since we are creating pgtables ourselves,
> > and we don't want to be ioremap'ing the GPU's SMMU instance, the
> > alternative is plugging in no-op helpers.  Which amounts to the same
> > thing.
> 
> Hmm, that said, since we are just memcpy'ing the io_pgtable_cfg from
> arm-smmu, it will already be populated with arm-smmu's fxn ptrs.  I
> guess we could maybe make it work without no-op helpers, although in
> that case it looks like we need to fix something about aux-domain vs
> tlb helpers:

I had a change that handled these correctly but I abandoned it because the
TLB functions didn't kick the power and I didn't think that would be desirable
at the generic level for performance reasons. Since the GPU SMMU is on the same
power domain as the GMU we could enable it in the GPU driver before calling
the TLB operations but we would need to be clever about it to prevent bringing
up the GMU just to unmap memory.

Jordan

> [  +0.004373] Unable to handle kernel NULL pointer dereference at
> virtual address 0000000000000019
> [  +0.004086] Mem abort info:
> [  +0.004319]   ESR = 0x96000004
> [  +0.003462]   EC = 0x25: DABT (current EL), IL = 32 bits
> [  +0.003494]   SET = 0, FnV = 0
> [  +0.002812]   EA = 0, S1PTW = 0
> [  +0.002873] Data abort info:
> [  +0.003031]   ISV = 0, ISS = 0x00000004
> [  +0.003785]   CM = 0, WnR = 0
> [  +0.003641] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000261d65000
> [  +0.003383] [0000000000000019] pgd=0000000000000000, p4d=0000000000000000
> [  +0.003715] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [  +0.002744] Modules linked in: xt_CHECKSUM xt_MASQUERADE
> xt_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp ip6table_mangle
> ip6table_nat iptable_mangle iptable_nat nf_nat nf_conntrack
> nf_defrag_ipv4 libcrc32c bridge stp llc ip6table_filter ip6_tables
> iptable_filter ax88179_178a usbnet uvcvideo videobuf2_vmalloc
> videobuf2_memops videobuf2_v4l2 videobuf2_common videodev mc
> hid_multitouch i2c_hid some_battery ti_sn65dsi86 hci_uart btqca btbcm
> qcom_spmi_adc5 bluetooth qcom_spmi_temp_alarm qcom_vadc_common
> ecdh_generic ecc snd_soc_sdm845 snd_soc_rt5663 snd_soc_qcom_common
> ath10k_snoc ath10k_core crct10dif_ce ath mac80211 snd_soc_rl6231
> soundwire_bus i2c_qcom_geni libarc4 qcom_rng msm phy_qcom_qusb2
> reset_qcom_pdc drm_kms_helper cfg80211 rfkill qcom_q6v5_mss
> qcom_q6v5_ipa_notify socinfo qrtr ns panel_simple qcom_q6v5_pas
> qcom_common qcom_glink_smem slim_qcom_ngd_ctrl qcom_sysmon drm
> qcom_q6v5 slimbus qmi_helpers qcom_wdt mdt_loader rmtfs_mem be2iscsi
> bnx2i cnic uio cxgb4i cxgb4 cxgb3i cxgb3 mdio
> [  +0.000139]  libcxgbi libcxgb qla4xxx iscsi_boot_sysfs iscsi_tcp
> libiscsi_tcp libiscsi scsi_transport_iscsi fuse ip_tables x_tables
> ipv6 nf_defrag_ipv6
> [  +0.020933] CPU: 3 PID: 168 Comm: kworker/u16:7 Not tainted
> 5.8.0-rc1-c630+ #31
> [  +0.003828] Hardware name: LENOVO 81JL/LNVNB161216, BIOS
> 9UCN33WW(V2.06) 06/ 4/2019
> [  +0.004039] Workqueue: msm msm_gem_free_work [msm]
> [  +0.003885] pstate: 60c00005 (nZCv daif +PAN +UAO BTYPE=--)
> [  +0.003859] pc : arm_smmu_tlb_inv_range_s1+0x30/0x148
> [  +0.003742] lr : arm_smmu_tlb_add_page_s1+0x1c/0x28
> [  +0.003887] sp : ffff800011cdb970
> [  +0.003868] x29: ffff800011cdb970 x28: 0000000000000003
> [  +0.003930] x27: ffff0001f1882f80 x26: 0000000000000001
> [  +0.003886] x25: 0000000000000003 x24: 0000000000000620
> [  +0.003932] x23: 0000000000000000 x22: 0000000000001000
> [  +0.003886] x21: 0000000000001000 x20: ffff0001cf857300
> [  +0.003916] x19: 0000000000000001 x18: 00000000ffffffff
> [  +0.003921] x17: ffffd9e6a24ae0e8 x16: 0000000000012577
> [  +0.003843] x15: 0000000000012578 x14: 0000000000000000
> [  +0.003884] x13: 0000000000012574 x12: ffffd9e6a2550180
> [  +0.003834] x11: 0000000000083f80 x10: 0000000000000000
> [  +0.003889] x9 : 0000000000000000 x8 : ffff0001f1882f80
> [  +0.003812] x7 : 0000000000000001 x6 : 0000000000000048
> [  +0.003807] x5 : ffff0001c86e1000 x4 : 0000000000000620
> [  +0.003802] x3 : ffff0001ddb57700 x2 : 0000000000001000
> [  +0.003809] x1 : 0000000000001000 x0 : 0000000101048000
> [  +0.003768] Call trace:
> [  +0.003665]  arm_smmu_tlb_inv_range_s1+0x30/0x148
> [  +0.003769]  arm_smmu_tlb_add_page_s1+0x1c/0x28
> [  +0.003760]  __arm_lpae_unmap+0x3c4/0x498
> [  +0.003821]  __arm_lpae_unmap+0xfc/0x498
> [  +0.003693]  __arm_lpae_unmap+0xfc/0x498
> [  +0.003704]  __arm_lpae_unmap+0xfc/0x498
> [  +0.003608]  arm_lpae_unmap+0x60/0x78
> [  +0.003653]  msm_iommu_pagetable_unmap+0x5c/0xa0 [msm]
> [  +0.003711]  msm_gem_purge_vma+0x48/0x70 [msm]
> [  +0.003716]  put_iova+0x68/0xc8 [msm]
> [  +0.003792]  msm_gem_free_work+0x118/0x190 [msm]
> [  +0.003739]  process_one_work+0x28c/0x6e8
> [  +0.003595]  worker_thread+0x4c/0x420
> [  +0.003546]  kthread+0x148/0x168
> [  +0.003675]  ret_from_fork+0x10/0x1c
> [  +0.003596] Code: 2a0403f8 a9046bf9 f9400073 39406077 (b9401a61)
> 
> BR,
> -R
> 
> >
> > Currently (in a later patch in the series) we are using
> > iommu_flush_tlb_all() when unmapping, which is a bit of a big hammer.
> > Although I think we could be a bit more clever and do the TLB ops on
> > the GPU (since the GPU knows if pagetables we are unmapping from are
> > in-use and could skip the TLB ops otherwise).
> >
> > On the topic, if we are using unique ASID values per set of
> > pagetables, how expensive is tlb invalidate for an ASID that has no
> > entries in the TLB?
> >
> > BR,
> > -R
> >
> > >
> > > Robin.
> > >
> > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > > > ---
> > > >
> > > >   include/linux/io-pgtable.h | 11 +++++++----
> > > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> > > > index 53d53c6c2be9..bbed1d3925ba 100644
> > > > --- a/include/linux/io-pgtable.h
> > > > +++ b/include/linux/io-pgtable.h
> > > > @@ -210,21 +210,24 @@ struct io_pgtable {
> > > >
> > > >   static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
> > > >   {
> > > > -     iop->cfg.tlb->tlb_flush_all(iop->cookie);
> > > > +     if (iop->cfg.tlb)
> > > > +             iop->cfg.tlb->tlb_flush_all(iop->cookie);
> > > >   }
> > > >
> > > >   static inline void
> > > >   io_pgtable_tlb_flush_walk(struct io_pgtable *iop, unsigned long iova,
> > > >                         size_t size, size_t granule)
> > > >   {
> > > > -     iop->cfg.tlb->tlb_flush_walk(iova, size, granule, iop->cookie);
> > > > +     if (iop->cfg.tlb)
> > > > +             iop->cfg.tlb->tlb_flush_walk(iova, size, granule, iop->cookie);
> > > >   }
> > > >
> > > >   static inline void
> > > >   io_pgtable_tlb_flush_leaf(struct io_pgtable *iop, unsigned long iova,
> > > >                         size_t size, size_t granule)
> > > >   {
> > > > -     iop->cfg.tlb->tlb_flush_leaf(iova, size, granule, iop->cookie);
> > > > +     if (iop->cfg.tlb)
> > > > +             iop->cfg.tlb->tlb_flush_leaf(iova, size, granule, iop->cookie);
> > > >   }
> > > >
> > > >   static inline void
> > > > @@ -232,7 +235,7 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop,
> > > >                       struct iommu_iotlb_gather * gather, unsigned long iova,
> > > >                       size_t granule)
> > > >   {
> > > > -     if (iop->cfg.tlb->tlb_add_page)
> > > > +     if (iop->cfg.tlb && iop->cfg.tlb->tlb_add_page)
> > > >               iop->cfg.tlb->tlb_add_page(gather, iova, granule, iop->cookie);
> > > >   }
> > > >
> > > >
> > > _______________________________________________
> > > Freedreno mailing list
> > > Freedreno@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/freedreno

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply	other threads:[~2020-07-08 19:19 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-26 20:04 [PATCH v2 0/6] iommu-arm-smmu: Add auxiliary domains and per-instance pagetables Jordan Crouse
2020-06-26 20:04 ` [PATCH v2 1/6] iommu/arm-smmu: Add auxiliary domain support for arm-smmuv2 Jordan Crouse
2020-07-07 10:48   ` Jean-Philippe Brucker
2020-07-07 12:34   ` Robin Murphy
2020-07-07 15:09     ` [Freedreno] " Rob Clark
2020-07-13 17:35       ` Jordan Crouse
2020-06-26 20:04 ` [PATCH v2 2/6] iommu/io-pgtable: Allow a pgtable implementation to skip TLB operations Jordan Crouse
2020-07-07 11:34   ` Robin Murphy
2020-07-07 14:25     ` [Freedreno] " Rob Clark
2020-07-07 14:58       ` Rob Clark
2020-07-08 19:19         ` Jordan Crouse [this message]
2020-06-26 20:04 ` [PATCH v2 3/6] iommu/arm-smmu: Add a domain attribute to pass the pagetable config Jordan Crouse
2020-06-26 20:04 ` [PATCH v2 4/6] drm/msm: Add support to create a local pagetable Jordan Crouse
2020-07-07 11:36   ` Robin Murphy
2020-07-07 14:41     ` [Freedreno] " Rob Clark
2020-07-08 19:35     ` Jordan Crouse
2020-06-26 20:04 ` [PATCH v2 5/6] drm/msm: Add support for address space instances Jordan Crouse
2020-06-26 20:04 ` [PATCH v2 6/6] drm/msm/a6xx: Add support for per-instance pagetables Jordan Crouse
2020-06-27 19:56   ` Rob Clark
2020-06-27 20:11     ` Rob Clark
2020-06-29 14:56       ` [Freedreno] " Jordan Crouse

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200708191923.GA21059@jcrouse1-lnx.qualcomm.com \
    --to=jcrouse@codeaurora.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=john.stultz@linaro.org \
    --cc=jroedel@suse.de \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robdclark@gmail.com \
    --cc=robin.murphy@arm.com \
    --cc=will@kernel.org \
    --subject='Re: [Freedreno] [PATCH v2 2/6] iommu/io-pgtable: Allow a pgtable implementation to skip TLB operations' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).