Re: [PATCH v3 00/21] iommu: Refactoring domain allocation interface
On Thu, Jul 04, 2024 at 10:24:52PM +0800, Baolu Lu wrote: > On 2024/7/4 22:18, Will Deacon wrote: > > On Mon, 10 Jun 2024 16:55:34 +0800, Lu Baolu wrote: > > > The IOMMU subsystem has undergone some changes, including the removal > > > of iommu_ops from the bus structure. Consequently, the existing domain > > > allocation interface, which relies on a bus type argument, is no longer > > > relevant: > > > > > > struct iommu_domain *iommu_domain_alloc(struct bus_type *bus) > > > > > > [...] > > Applied a few of these to iommu (iommufd/paging-domain-alloc), thanks! > > > > [01/21] iommu: Add iommu_paging_domain_alloc() interface > > https://git.kernel.org/iommu/c/a27bf2743cb8 > > [02/21] iommufd: Use iommu_paging_domain_alloc() > > https://git.kernel.org/iommu/c/26a581606fab > > [03/21] vfio/type1: Use iommu_paging_domain_alloc() > > https://git.kernel.org/iommu/c/60ffc4501722 > > [04/21] vhost-vdpa: Use iommu_paging_domain_alloc() > > https://git.kernel.org/iommu/c/9c159f6de1ae > > [05/21] drm/msm: Use iommu_paging_domain_alloc() > > https://git.kernel.org/iommu/c/45acf35af200 > > > > [10/21] wifi: ath10k: Use iommu_paging_domain_alloc() > > https://git.kernel.org/iommu/c/d5b7485588df > > [11/21] wifi: ath11k: Use iommu_paging_domain_alloc() > > https://git.kernel.org/iommu/c/ef50d41fbf1c > > > > [14/21] RDMA/usnic: Use iommu_paging_domain_alloc() > > https://git.kernel.org/iommu/c/3b10f25704be > > [15/21] iommu/vt-d: Add helper to allocate paging domain > > https://git.kernel.org/iommu/c/9e9ba576c259 > > Will, the patch [15/21] has already been included in my VT-d update pull > request. I have also addressed Yi's comment in that patch. So can you > please remove it from this branch? Heh, was just about to say the same thing as I noticed when I was re-creating 'next'. Yes, I'll drop it now. Since it's on the end, the other SHAs will remain the same. Sorry about that, Will
Re: [PATCH v3 00/21] iommu: Refactoring domain allocation interface
On Mon, 10 Jun 2024 16:55:34 +0800, Lu Baolu wrote: > The IOMMU subsystem has undergone some changes, including the removal > of iommu_ops from the bus structure. Consequently, the existing domain > allocation interface, which relies on a bus type argument, is no longer > relevant: > > struct iommu_domain *iommu_domain_alloc(struct bus_type *bus) > > [...] Applied a few of these to iommu (iommufd/paging-domain-alloc), thanks! [01/21] iommu: Add iommu_paging_domain_alloc() interface https://git.kernel.org/iommu/c/a27bf2743cb8 [02/21] iommufd: Use iommu_paging_domain_alloc() https://git.kernel.org/iommu/c/26a581606fab [03/21] vfio/type1: Use iommu_paging_domain_alloc() https://git.kernel.org/iommu/c/60ffc4501722 [04/21] vhost-vdpa: Use iommu_paging_domain_alloc() https://git.kernel.org/iommu/c/9c159f6de1ae [05/21] drm/msm: Use iommu_paging_domain_alloc() https://git.kernel.org/iommu/c/45acf35af200 [10/21] wifi: ath10k: Use iommu_paging_domain_alloc() https://git.kernel.org/iommu/c/d5b7485588df [11/21] wifi: ath11k: Use iommu_paging_domain_alloc() https://git.kernel.org/iommu/c/ef50d41fbf1c [14/21] RDMA/usnic: Use iommu_paging_domain_alloc() https://git.kernel.org/iommu/c/3b10f25704be [15/21] iommu/vt-d: Add helper to allocate paging domain https://git.kernel.org/iommu/c/9e9ba576c259 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev
Re: [PATCH v3 04/21] vhost-vdpa: Use iommu_paging_domain_alloc()
On Wed, Jul 03, 2024 at 12:32:06PM -0400, Michael S. Tsirkin wrote: > On Mon, Jun 10, 2024 at 04:55:38PM +0800, Lu Baolu wrote: > > Replace iommu_domain_alloc() with iommu_paging_domain_alloc(). > > > > Signed-off-by: Lu Baolu > > Acked-by: Michael S. Tsirkin > > > I assume it's merged with the rest of the stuff, right? Sure, I can grab this one in the iommu tree. It has to go along with the patch adding iommu_paging_domain_alloc() anyway. Will
Re: [PATCH v2 0/5] Support for Adreno X1-85 GPU
On Sat, 29 Jun 2024 07:19:33 +0530, Akhil P Oommen wrote: > This series adds support for the Adreno X1-85 GPU found in Qualcomm's > compute series chipset, Snapdragon X1 Elite (x1e80100). In this new > naming scheme for Adreno GPU, 'X' stands for compute series, '1' denotes > 1st generation and '8' & '5' denotes the tier and the SKU which it > belongs. > > X1-85 has major focus on doubling core clock frequency and bandwidth > throughput. It has a dedicated collapsible Graphics MX rail (gmxc) to > power the memories and double the number of data channels to improve > bandwidth to DDR. > > [...] Applied SMMU bindings change to will (for-joerg/arm-smmu/bindings), thanks! [4/5] dt-bindings: arm-smmu: Add X1E80100 GPU SMMU https://git.kernel.org/will/c/d6c102881b30 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev
Re: [PATCH v4 1/2] iommu/io-pgtable-arm: Add way to debug pgtable walk
On Mon, Jun 24, 2024 at 08:37:26AM -0700, Rob Clark wrote:
> On Mon, Jun 24, 2024 at 8:14 AM Will Deacon wrote:
> >
> > On Thu, May 23, 2024 at 10:52:21AM -0700, Rob Clark wrote:
> > > From: Rob Clark
> > >
> > > Add an io-pgtable method to walk the pgtable returning the raw PTEs that
> > > would be traversed for a given iova access.
> > >
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/iommu/io-pgtable-arm.c | 51 --
> > > include/linux/io-pgtable.h | 4 +++
> > > 2 files changed, 46 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/iommu/io-pgtable-arm.c
> > > b/drivers/iommu/io-pgtable-arm.c
> > > index f7828a7aad41..f47a0e64bb35 100644
> > > --- a/drivers/iommu/io-pgtable-arm.c
> > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > @@ -693,17 +693,19 @@ static size_t arm_lpae_unmap_pages(struct
> > > io_pgtable_ops *ops, unsigned long iov
> > > data->start_level, ptep);
> > > }
> > >
> > > -static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
> > > - unsigned long iova)
> > > +static int arm_lpae_pgtable_walk(struct io_pgtable_ops *ops, unsigned
> > > long iova,
> > > + int (*cb)(void *cb_data, void *pte, int level),
> > > + void *cb_data)
> > > {
> > > struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > > arm_lpae_iopte pte, *ptep = data->pgd;
> > > int lvl = data->start_level;
> > > + int ret;
> > >
> > > do {
> > > /* Valid IOPTE pointer? */
> > > if (!ptep)
> > > - return 0;
> > > + return -EFAULT;
> >
> > nit: -ENOENT might be a little better, as we're only checking against a
> > NULL entry rather than strictly any faulting entry.
> >
> > > /* Grab the IOPTE we're interested in */
> > > ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
> > > @@ -711,22 +713,52 @@ static phys_addr_t arm_lpae_iova_to_phys(struct
> > > io_pgtable_ops *ops,
> > >
> > > /* Valid entry? */
> > > if (!pte)
> > > - return 0;
> > > + return -EFAULT;
> >
> > Same here (and at the end of the function).
> >
> > > +
> > > + ret = cb(cb_data, &pte, lvl);
> >
> > Since pte is on the stack, rather than pointing into the actual pgtable,
> > I think it would be clearer to pass it by value to the callback.
>
> fwiw, I passed it as a void* to avoid the pte size.. although I guess
> it could be a union of all the possible pte types
Can you just get away with a u64?
Will
Re: [PATCH v4 1/2] iommu/io-pgtable-arm: Add way to debug pgtable walk
On Thu, May 23, 2024 at 10:52:21AM -0700, Rob Clark wrote:
> From: Rob Clark
>
> Add an io-pgtable method to walk the pgtable returning the raw PTEs that
> would be traversed for a given iova access.
>
> Signed-off-by: Rob Clark
> ---
> drivers/iommu/io-pgtable-arm.c | 51 --
> include/linux/io-pgtable.h | 4 +++
> 2 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index f7828a7aad41..f47a0e64bb35 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -693,17 +693,19 @@ static size_t arm_lpae_unmap_pages(struct
> io_pgtable_ops *ops, unsigned long iov
> data->start_level, ptep);
> }
>
> -static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
> - unsigned long iova)
> +static int arm_lpae_pgtable_walk(struct io_pgtable_ops *ops, unsigned long
> iova,
> + int (*cb)(void *cb_data, void *pte, int level),
> + void *cb_data)
> {
> struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> arm_lpae_iopte pte, *ptep = data->pgd;
> int lvl = data->start_level;
> + int ret;
>
> do {
> /* Valid IOPTE pointer? */
> if (!ptep)
> - return 0;
> + return -EFAULT;
nit: -ENOENT might be a little better, as we're only checking against a
NULL entry rather than strictly any faulting entry.
> /* Grab the IOPTE we're interested in */
> ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
> @@ -711,22 +713,52 @@ static phys_addr_t arm_lpae_iova_to_phys(struct
> io_pgtable_ops *ops,
>
> /* Valid entry? */
> if (!pte)
> - return 0;
> + return -EFAULT;
Same here (and at the end of the function).
> +
> + ret = cb(cb_data, &pte, lvl);
Since pte is on the stack, rather than pointing into the actual pgtable,
I think it would be clearer to pass it by value to the callback.
> + if (ret)
> + return ret;
>
> - /* Leaf entry? */
> + /* Leaf entry? If so, we've found the translation */
> if (iopte_leaf(pte, lvl, data->iop.fmt))
> - goto found_translation;
> + return 0;
>
> /* Take it to the next level */
> ptep = iopte_deref(pte, data);
> } while (++lvl < ARM_LPAE_MAX_LEVELS);
>
> /* Ran out of page tables to walk */
> + return -EFAULT;
> +}
> +
> +struct iova_to_phys_walk_data {
> + arm_lpae_iopte pte;
> + int level;
> +};
Expanding a little on Robin's suggestion, why don't we drop this structure
in favour of something more generic:
struct arm_lpae_walk_data {
arm_lpae_iopte ptes[ARM_LPAE_MAX_LEVELS];
};
and then do something in the walker like:
if (cb && !cb(pte, lvl))
walk_data->ptes[lvl] = pte;
which could return the physical address at the end, if it reaches a leaf
entry. That way arm_lpae_iova_to_phys() is just passing a NULL callback
to the walker and your debug callback just needs to return 0 (i.e. the
callback is basically just saying whether or not to continue the walk).
Will
Re: [PATCH] drm/msm/adreno: De-spaghettify the use of memory barriers
On Tue, Jun 18, 2024 at 09:41:58PM +0530, Akhil P Oommen wrote:
> On Tue, Jun 04, 2024 at 03:40:56PM +0100, Will Deacon wrote:
> > On Thu, May 16, 2024 at 01:55:26PM -0500, Andrew Halaney wrote:
> > > On Thu, May 16, 2024 at 08:20:05PM GMT, Akhil P Oommen wrote:
> > > > On Thu, May 16, 2024 at 08:15:34AM -0500, Andrew Halaney wrote:
> > > > > If I understand correctly, you don't need any memory barrier.
> > > > > writel()/readl()'s are ordered to the same endpoint. That goes for all
> > > > > the reordering/barrier comments mentioned below too.
> > > > >
> > > > > device-io.rst:
> > > > >
> > > > > The read and write functions are defined to be ordered. That is
> > > > > the
> > > > > compiler is not permitted to reorder the I/O sequence. When the
> > > > > ordering
> > > > > can be compiler optimised, you can use __readb() and friends to
> > > > > indicate the relaxed ordering. Use this with care.
> > > > >
> > > > > memory-barriers.txt:
> > > > >
> > > > > (*) readX(), writeX():
> > > > >
> > > > > The readX() and writeX() MMIO accessors take a pointer to
> > > > > the
> > > > > peripheral being accessed as an __iomem * parameter. For
> > > > > pointers
> > > > > mapped with the default I/O attributes (e.g. those returned
> > > > > by
> > > > > ioremap()), the ordering guarantees are as follows:
> > > > >
> > > > > 1. All readX() and writeX() accesses to the same peripheral
> > > > > are ordered
> > > > > with respect to each other. This ensures that MMIO
> > > > > register accesses
> > > > > by the same CPU thread to a particular device will
> > > > > arrive in program
> > > > > order.
> > > > >
> > > >
> > > > In arm64, a writel followed by readl translates to roughly the following
> > > > sequence: dmb_wmb(), __raw_writel(), __raw_readl(), dmb_rmb(). I am not
> > > > sure what is stopping compiler from reordering __raw_writel() and
> > > > __raw_readl()
> > > > above? I am assuming iomem cookie is ignored during compilation.
> > >
> > > It seems to me that is due to some usage of volatile there in
> > > __raw_writel() etc, but to be honest after reading about volatile and
> > > some threads from gcc mailing lists, I don't have a confident answer :)
> > >
> > > >
> > > > Added Will to this thread if he can throw some light on this.
> > >
> > > Hopefully Will can school us.
> >
> > The ordering in this case is ensured by the memory attributes used for
> > ioremap(). When an MMIO region is mapped using Device-nGnRE attributes
> > (as it the case for ioremap()), the "nR" part means "no reordering", so
> > readX() and writeX() to that region are ordered wrt each other.
>
> But that avoids only HW reordering, doesn't it? What about *compiler
> reordering* in the
> case of a writel following by a readl which translates to:
> 1: dmb_wmb()
> 2: __raw_writel() -> roughly "asm volatile('str')
> 3: __raw_readl() -> roughly "asm volatile('ldr')
> 4: dmb_rmb()
>
> Is the 'volatile' keyword sufficient to avoid reordering between (2) and (3)?
> Or
> do we need a "memory" clobber to inhibit reordering?
>
> This is still not clear to me even after going through some compiler
> documentions.
I don't think the compiler should reorder volatile asm blocks wrt each
other.
Will
Re: [PATCH] drm/msm/adreno: De-spaghettify the use of memory barriers
On Thu, Jun 06, 2024 at 02:03:24PM +0200, Konrad Dybcio wrote: > On 4.06.2024 4:40 PM, Will Deacon wrote: > > On Thu, May 16, 2024 at 01:55:26PM -0500, Andrew Halaney wrote: > >> On Thu, May 16, 2024 at 08:20:05PM GMT, Akhil P Oommen wrote: > >>> On Thu, May 16, 2024 at 08:15:34AM -0500, Andrew Halaney wrote: > >>>> If I understand correctly, you don't need any memory barrier. > >>>> writel()/readl()'s are ordered to the same endpoint. That goes for all > >>>> the reordering/barrier comments mentioned below too. > >>>> > >>>> device-io.rst: > >>>> > >>>> The read and write functions are defined to be ordered. That is the > >>>> compiler is not permitted to reorder the I/O sequence. When the > >>>> ordering > >>>> can be compiler optimised, you can use __readb() and friends to > >>>> indicate the relaxed ordering. Use this with care. > >>>> > >>>> memory-barriers.txt: > >>>> > >>>> (*) readX(), writeX(): > >>>> > >>>> The readX() and writeX() MMIO accessors take a pointer to the > >>>> peripheral being accessed as an __iomem * parameter. For pointers > >>>> mapped with the default I/O attributes (e.g. those returned by > >>>> ioremap()), the ordering guarantees are as follows: > >>>> > >>>> 1. All readX() and writeX() accesses to the same peripheral are > >>>> ordered > >>>> with respect to each other. This ensures that MMIO register > >>>> accesses > >>>> by the same CPU thread to a particular device will arrive in > >>>> program > >>>> order. > >>>> > >>> > >>> In arm64, a writel followed by readl translates to roughly the following > >>> sequence: dmb_wmb(), __raw_writel(), __raw_readl(), dmb_rmb(). I am not > >>> sure what is stopping compiler from reordering __raw_writel() and > >>> __raw_readl() > >>> above? I am assuming iomem cookie is ignored during compilation. > >> > >> It seems to me that is due to some usage of volatile there in > >> __raw_writel() etc, but to be honest after reading about volatile and > >> some threads from gcc mailing lists, I don't have a confident answer :) > >> > >>> > >>> Added Will to this thread if he can throw some light on this. > >> > >> Hopefully Will can school us. > > > > The ordering in this case is ensured by the memory attributes used for > > ioremap(). When an MMIO region is mapped using Device-nGnRE attributes > > (as it the case for ioremap()), the "nR" part means "no reordering", so > > readX() and writeX() to that region are ordered wrt each other. > > > > Note that guarantee _doesn't_ apply to other flavours of ioremap(), so > > e.g. ioremap_wc() won't give you the ordering. > > > > Hope that helps, > > Just to make sure I'm following, would mapping things as nGnRnE effectively > get rid of write buffering, perhaps being a way of debugging whether that > in particular is causing issues (at the cost of speed)? I think the "nE" part is just a hint, so it will depend on how the hardware has been built. On top of that, you'll still need something like a DSB to force the CPU to wait for the write response. Will
Re: [PATCH] drm/msm/adreno: De-spaghettify the use of memory barriers
On Thu, May 16, 2024 at 01:55:26PM -0500, Andrew Halaney wrote: > On Thu, May 16, 2024 at 08:20:05PM GMT, Akhil P Oommen wrote: > > On Thu, May 16, 2024 at 08:15:34AM -0500, Andrew Halaney wrote: > > > If I understand correctly, you don't need any memory barrier. > > > writel()/readl()'s are ordered to the same endpoint. That goes for all > > > the reordering/barrier comments mentioned below too. > > > > > > device-io.rst: > > > > > > The read and write functions are defined to be ordered. That is the > > > compiler is not permitted to reorder the I/O sequence. When the > > > ordering > > > can be compiler optimised, you can use __readb() and friends to > > > indicate the relaxed ordering. Use this with care. > > > > > > memory-barriers.txt: > > > > > > (*) readX(), writeX(): > > > > > > The readX() and writeX() MMIO accessors take a pointer to the > > > peripheral being accessed as an __iomem * parameter. For pointers > > > mapped with the default I/O attributes (e.g. those returned by > > > ioremap()), the ordering guarantees are as follows: > > > > > > 1. All readX() and writeX() accesses to the same peripheral are > > > ordered > > > with respect to each other. This ensures that MMIO register > > > accesses > > > by the same CPU thread to a particular device will arrive in > > > program > > > order. > > > > > > > In arm64, a writel followed by readl translates to roughly the following > > sequence: dmb_wmb(), __raw_writel(), __raw_readl(), dmb_rmb(). I am not > > sure what is stopping compiler from reordering __raw_writel() and > > __raw_readl() > > above? I am assuming iomem cookie is ignored during compilation. > > It seems to me that is due to some usage of volatile there in > __raw_writel() etc, but to be honest after reading about volatile and > some threads from gcc mailing lists, I don't have a confident answer :) > > > > > Added Will to this thread if he can throw some light on this. > > Hopefully Will can school us. The ordering in this case is ensured by the memory attributes used for ioremap(). When an MMIO region is mapped using Device-nGnRE attributes (as it the case for ioremap()), the "nR" part means "no reordering", so readX() and writeX() to that region are ordered wrt each other. Note that guarantee _doesn't_ apply to other flavours of ioremap(), so e.g. ioremap_wc() won't give you the ordering. Hope that helps, Will
Re: [PATCH v2 0/7] A702 support
On Fri, Feb 23, 2024 at 10:21:36PM +0100, Konrad Dybcio wrote: > Bit of a megaseries, bunched together for your testing convenience.. > Needs mesa!27665 [1] on the userland part, kmscube happily spins. > > I'm feeling quite lukewarm about the memory barriers in patch 3.. > > Patch 1 for Will/smmu, 5-6 for drm/msm, rest for qcom I'm guessing you don't really expect me to take the clock bindings?! Will
Re: [PATCH v3 0/7] drm/msm: Add support for the A750 GPU found on the SM8650 platform
On Fri, 16 Feb 2024 12:03:47 +0100, Neil Armstrong wrote: > Unlike the the very close A740 GPU on the SM8550 SoC, the A750 GPU > doesn't have an HWCFG block but a separate register set. > > The missing registers are added in the a6xx.xml.h file that would > require a subsequent sync and the non-existent hwcfg is handled > in a6xx_set_hwcg(). > > [...] Applied SMMU binding changes to will (for-joerg/arm-smmu/bindings), thanks! [2/7] dt-bindings: arm-smmu: Fix SM8[45]50 GPU SMMU 'if' condition https://git.kernel.org/will/c/281ca9b6 [3/7] dt-bindings: arm-smmu: Document SM8650 GPU SMMU https://git.kernel.org/will/c/8a05f74d567a Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev
Re: [PATCH 0/8] A702 support
On Mon, 19 Feb 2024 14:35:45 +0100, Konrad Dybcio wrote: > Bit of a megaseries, bunched together for your testing convenience.. > Needs mesa!27665 [1] on the userland part, kmscube happily spins. > > I'm feeling quite lukewarm about the memory barriers in patch 3.. > > Patch 1 for Will/smmu, 5-6 for drm/msm, rest for qcom > > [...] Applied SMMU bindings patch to will (for-joerg/arm-smmu/bindings), thanks! [1/8] dt-bindings: arm-smmu: Add QCM2290 GPU SMMU https://git.kernel.org/will/c/0eca305f8e0d Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev
Re: [PATCH v3 00/12] RB1/QCM2290 features
On Wed, 29 Nov 2023 15:43:57 +0100, Konrad Dybcio wrote: > This series brings: > - interconnect plumbing > - display setup > > for QCM2290/QRB2210 and > > - CAN bus controller > - HDMI display > - wifi fw variant name > > [...] Applied SMMU update to will (for-joerg/arm-smmu/updates), thanks! [05/12] iommu/arm-smmu-qcom: Add QCM2290 MDSS compatible https://git.kernel.org/will/c/28af105cb650 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev
Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
On Fri, Sep 29, 2023 at 06:25:21PM +0100, Robin Murphy wrote: > On 29/09/2023 4:45 pm, Will Deacon wrote: > > On Mon, Sep 25, 2023 at 06:54:42PM +0100, Robin Murphy wrote: > > > On 2023-04-10 19:52, Dmitry Baryshkov wrote: > > > > If the Adreno SMMU is dma-coherent, allocation will fail unless we > > > > disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the > > > > coherent SMMUs (like we have on sm8350 platform). > > > > > > Hmm, but is it right that it should fail in the first place? The fact is > > > that if the SMMU is coherent then walks *will* be outer-WBWA, so I > > > honestly > > > can't see why the io-pgtable code is going out of its way to explicitly > > > reject a request to give them the same attribute it's already giving then > > > anyway :/ > > > > > > Even if the original intent was for the quirk to have an over-specific > > > implication of representing inner-NC as well, that hardly seems useful if > > > what we've ended up with in practice is a nonsensical-looking check in one > > > place and then a weird hacky bodge in another purely to work around it. > > > > > > Does anyone know a good reason why this is the way it is? > > > > I think it was mainly because the quick doesn't make sense for a coherent > > page-table walker and we could in theory use that bit for something else > > in that case. > > Yuck, even if we did want some horrible notion of quirks being conditional > on parts of the config rather than just the format, then the users would > need to be testing for the same condition as the pagetable code itself (i.e. > cfg->coherent_walk), rather than hoping some other property of something > else indirectly reflects the right information - e.g. there'd be no hope of > backporting this particular bodge before 5.19 where the old > iommu_capable(IOMMU_CAP_CACHE_COHERENCY) always returned true, and in future > we could conceivably support coherent SMMUs being configured for > non-coherent walks on a per-domain basis. That doesn't sound like an insurmountable problem to me. Either a bunch of other stuff has to be backported as well, or the msm_iommu driver can fish the pgtable configuration out of the SMMU, like it already does elsewhere. > Furthermore, if we did overload a flag to have multiple meanings, then we'd > have no way of knowing which one the caller was actually expecting, thus the > illusion of being able to validate calls in the meantime isn't necessarily > as helpful as it seems, particularly in a case where the "wrong" > interpretation would be to have no effect anyway. Mostly though I'd hope > that if we ever got anywhere near the point of running out of quirk bits > we'd have already realised that it's time for a better interface :( Although I agree that practically I can't see us reusing quirk bits, I do much prefer that we reject quirks that don't make sense. Yes, in this case it happens that the quirk is expressing something which is already true for the coherent walker, but that feels like a special case to me rather than something which is likely to be true in general, for example, the system cache quirk proposed by Qualcomm to expose the unused inner-NC-outer-WBWRA MAIR configuration. Implicitly accepting quirks also makes it more difficult if we wanted to change the default configuration in future; for example if we wanted to adjust the default allocation hints. So I'd prefer to leave the page-table code as-is. Will
Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
On Mon, Sep 25, 2023 at 06:54:42PM +0100, Robin Murphy wrote: > On 2023-04-10 19:52, Dmitry Baryshkov wrote: > > If the Adreno SMMU is dma-coherent, allocation will fail unless we > > disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the > > coherent SMMUs (like we have on sm8350 platform). > > Hmm, but is it right that it should fail in the first place? The fact is > that if the SMMU is coherent then walks *will* be outer-WBWA, so I honestly > can't see why the io-pgtable code is going out of its way to explicitly > reject a request to give them the same attribute it's already giving then > anyway :/ > > Even if the original intent was for the quirk to have an over-specific > implication of representing inner-NC as well, that hardly seems useful if > what we've ended up with in practice is a nonsensical-looking check in one > place and then a weird hacky bodge in another purely to work around it. > > Does anyone know a good reason why this is the way it is? I think it was mainly because the quick doesn't make sense for a coherent page-table walker and we could in theory use that bit for something else in that case. Will
Re: [PATCH v6 12/12] iommu/arm-smmu-qcom: Add SM6350 DPU compatible
On Tue, Jun 06, 2023 at 02:44:03PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio
>
> Add the SM6350 DPU compatible to clients compatible list, as it also
> needs the workarounds.
>
> Signed-off-by: Konrad Dybcio
> Acked-by: Dmitry Baryshkov
> Signed-off-by: Konrad Dybcio
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index cc574928c707..bdeb587552c0 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -253,6 +253,7 @@ static const struct of_device_id
> qcom_smmu_client_of_match[] __maybe_unused = {
> { .compatible = "qcom,sc8280xp-mdss" },
> { .compatible = "qcom,sdm845-mdss" },
> { .compatible = "qcom,sdm845-mss-pil" },
> + { .compatible = "qcom,sm6350-mdss" },
> { .compatible = "qcom,sm6375-mdss" },
> { .compatible = "qcom,sm8150-mdss" },
> { .compatible = "qcom,sm8250-mdss" },
Acked-by: Will Deacon
Will
Re: [PATCH v6 11/12] iommu/arm-smmu-qcom: Add SM6375 DPU compatible
On Tue, Jun 06, 2023 at 02:44:02PM +0200, Konrad Dybcio wrote:
> Add the SM6375 DPU compatible to clients compatible list, as it also
> needs the workarounds.
>
> Acked-by: Dmitry Baryshkov
> Signed-off-by: Konrad Dybcio
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 3800ab478216..cc574928c707 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -253,6 +253,7 @@ static const struct of_device_id
> qcom_smmu_client_of_match[] __maybe_unused = {
> { .compatible = "qcom,sc8280xp-mdss" },
> { .compatible = "qcom,sdm845-mdss" },
> { .compatible = "qcom,sdm845-mss-pil" },
> + { .compatible = "qcom,sm6375-mdss" },
> { .compatible = "qcom,sm8150-mdss" },
> { .compatible = "qcom,sm8250-mdss" },
> { }
Acked-by: Will Deacon
Will
Re: [PATCH v6 10/12] iommu/arm-smmu-qcom: Sort the compatible list alphabetically
On Tue, Jun 06, 2023 at 02:44:01PM +0200, Konrad Dybcio wrote:
> It got broken at some point, fix it up.
>
> Reviewed-by: Dmitry Baryshkov
> Signed-off-by: Konrad Dybcio
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index c71afda79d64..3800ab478216 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -251,10 +251,10 @@ static const struct of_device_id
> qcom_smmu_client_of_match[] __maybe_unused = {
> { .compatible = "qcom,sc7280-mss-pil" },
> { .compatible = "qcom,sc8180x-mdss" },
> { .compatible = "qcom,sc8280xp-mdss" },
> - { .compatible = "qcom,sm8150-mdss" },
> - { .compatible = "qcom,sm8250-mdss" },
> { .compatible = "qcom,sdm845-mdss" },
> { .compatible = "qcom,sdm845-mss-pil" },
> + { .compatible = "qcom,sm8150-mdss" },
> + { .compatible = "qcom,sm8250-mdss" },
> { }
> };
Acked-by: Will Deacon
Will
Re: [PATCH v5 00/12] SM63(50|75) DPU support
On Thu, Jun 01, 2023 at 03:16:52AM +0300, Dmitry Baryshkov wrote: > On 23/05/2023 10:46, Konrad Dybcio wrote: > > [skipped the changelog] > > > --- > > Konrad Dybcio (12): > >dt-bindings: display/msm: dsi-controller-main: Add SM6350 > >dt-bindings: display/msm: dsi-controller-main: Add SM6375 > >dt-bindings: display/msm: sc7180-dpu: Describe SM6350 and SM6375 > >dt-bindings: display/msm: Add SM6350 MDSS > >dt-bindings: display/msm: Add SM6375 MDSS > >drm/msm/dpu: Add SM6350 support > >drm/msm: mdss: Add SM6350 support > >drm/msm/dpu: Add SM6375 support > >drm/msm: mdss: Add SM6375 support > >iommu/arm-smmu-qcom: Sort the compatible list alphabetically > >iommu/arm-smmu-qcom: Add SM6375 DPU compatible > >iommu/arm-smmu-qcom: Add SM6350 DPU compatible > > As we are now nearly ready to merge this series, Will, Robin, what should be > the merge strategy for these three patches? Would you take them through the > arm-smmu/iommu tree? I'm happy to take the three IOMMU changes, but the bulk of this series is replated to display and GPU so I don't think it makes sense for me to take those. Will
Re: [PATCH v3 1/2] iommu/arm-smmu-qcom: Fix missing adreno_smmu's
On Thu, May 11, 2023 at 07:59:05AM -0700, Rob Clark wrote:
> From: Rob Clark
>
> When the special handling of qcom,adreno-smmu was moved into
> qcom_smmu_create(), it was overlooked that we didn't have all the
> required entries in qcom_smmu_impl_of_match. So we stopped getting
> adreno_smmu_priv on sc7180, breaking per-process pgtables.
>
> Fixes: 30b912a03d91 ("iommu/arm-smmu-qcom: Move the qcom,adreno-smmu check
> into qcom_smmu_create")
> Suggested-by: Lepton Wu
> Signed-off-by: Rob Clark
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index d1b296b95c86..66e191773099 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -496,20 +496,21 @@ static const struct qcom_smmu_match_data
> qcom_smmu_500_impl0_data = {
> /*
> * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they
> need
> * special handling and can not be covered by the qcom,smmu-500 entry.
> */
> static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
> { .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
> { .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_v2_data },
> { .compatible = "qcom,qcm2290-smmu-500", .data =
> &qcom_smmu_500_impl0_data },
> { .compatible = "qcom,qdu1000-smmu-500", .data =
> &qcom_smmu_500_impl0_data },
> { .compatible = "qcom,sc7180-smmu-500", .data =
> &qcom_smmu_500_impl0_data },
> + { .compatible = "qcom,sc7180-smmu-v2", .data = &qcom_smmu_v2_data },
> { .compatible = "qcom,sc7280-smmu-500", .data =
> &qcom_smmu_500_impl0_data },
> { .compatible = "qcom,sc8180x-smmu-500", .data =
> &qcom_smmu_500_impl0_data },
> { .compatible = "qcom,sc8280xp-smmu-500", .data =
> &qcom_smmu_500_impl0_data },
> { .compatible = "qcom,sdm630-smmu-v2", .data = &qcom_smmu_v2_data },
> { .compatible = "qcom,sdm845-smmu-v2", .data = &qcom_smmu_v2_data },
> { .compatible = "qcom,sdm845-smmu-500", .data = &sdm845_smmu_500_data },
> { .compatible = "qcom,sm6115-smmu-500", .data =
> &qcom_smmu_500_impl0_data},
> { .compatible = "qcom,sm6125-smmu-500", .data =
> &qcom_smmu_500_impl0_data },
> { .compatible = "qcom,sm6350-smmu-v2", .data = &qcom_smmu_v2_data },
> { .compatible = "qcom,sm6350-smmu-500", .data =
> &qcom_smmu_500_impl0_data },
> @@ -540,12 +541,18 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct
> arm_smmu_device *smmu)
> /* Match platform for ACPI boot */
> if (acpi_match_platform_list(qcom_acpi_platlist) >= 0)
> return qcom_smmu_create(smmu,
> &qcom_smmu_500_impl0_data);
> }
> #endif
>
> match = of_match_node(qcom_smmu_impl_of_match, np);
> if (match)
> return qcom_smmu_create(smmu, match->data);
>
> + /* If you hit this WARN_ON() you are missing an entry in the
> + * qcom_smmu_impl_of_match[] table, and GPU per-process page-
> + * tables will be broken.
> + */
> + WARN_ON(of_device_is_compatible(np, "qcom,adreno-smmu"));
Wouldn't it be better to print the information from the comment, rather
than force the user to diagnose a WARN_ON() back to the source?
Will
Re: next-20230110: arm64: defconfig+kselftest config boot failed - Unable to handle kernel paging request at virtual address fffffffffffffff8
[+ James and Nathan]
On Tue, Jan 10, 2023 at 09:44:40PM +0530, Naresh Kamboju wrote:
> [ please ignore this email if this regression already reported ]
>
> Today's Linux next tag next-20230110 boot passes with defconfig but
> boot fails with
> defconfig + kselftest merge config on arm64 devices and qemu-arm64.
>
> Reported-by: Linux Kernel Functional Testing
>
> We are bisecting this problem and get back to you shortly.
>
> GOOD: next-20230109 (defconfig + kselftests configs)
> BAD: next-20230110 (defconfig + kselftests configs)
I couldn't find a kselftests .config in the tree (assumedly I'm just ont
looking hard enough), but does that happen to enable CONFIG_STACK_TRACER=y?
If so, since you're using clang, I wonder if this is an issue with
68a63a412d18 ("arm64: Fix build with CC=clang, CONFIG_FTRACE=y and
CONFIG_STACK_TRACER=y")?
Please let us know how the bisection goes...
Will
> kernel crash log [1]:
>
> [ 15.302140] Unable to handle kernel paging request at virtual
> address fff8
> [ 15.309906] Mem abort info:
> [ 15.312659] ESR = 0x9604
> [ 15.316365] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 15.321626] SET = 0, FnV = 0
> [ 15.324644] EA = 0, S1PTW = 0
> [ 15.327744] FSC = 0x04: level 0 translation fault
> [ 15.332619] Data abort info:
> [ 15.335422] ISV = 0, ISS = 0x0004
> [ 15.339226] CM = 0, WnR = 0
> [ 15.342154] swapper pgtable: 4k pages, 48-bit VAs, pgdp=1496c000
> [ 15.348795] [fff8] pgd=, p4d=
> [ 15.355524] Internal error: Oops: 9604 [#1] PREEMPT SMP
> [ 15.361729] Modules linked in: meson_gxl dwmac_generic
> snd_soc_meson_gx_sound_card snd_soc_meson_card_utils lima gpu_sched
> drm_shmem_helper meson_drm drm_dma_helper crct10dif_ce meson_ir
> rc_core meson_dw_hdmi dw_hdmi meson_canvas dwmac_meson8b
> stmmac_platform meson_rng stmmac rng_core cec meson_gxbb_wdt
> drm_display_helper snd_soc_meson_aiu snd_soc_meson_codec_glue pcs_xpcs
> snd_soc_meson_t9015 amlogic_gxl_crypto crypto_engine display_connector
> snd_soc_simple_amplifier drm_kms_helper drm nvmem_meson_efuse
> [ 15.405976] CPU: 1 PID: 9 Comm: kworker/u8:0 Not tainted
> 6.2.0-rc3-next-20230110 #1
> [ 15.413563] Hardware name: Libre Computer AML-S905X-CC (DT)
> [ 15.419086] Workqueue: events_unbound deferred_probe_work_func
> [ 15.424863] pstate: 0005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 15.431762] pc : of_drm_find_bridge+0x38/0x70 [drm]
> [ 15.436594] lr : of_drm_find_bridge+0x20/0x70 [drm]
> [ 15.441423] sp : 8a04b9b0
> [ 15.444700] x29: 8a04b9b0 x28: 08de5810 x27:
> 08de5808
> [ 15.451772] x26: 08de5800 x25: 084cb8b0 x24:
> 01223c00
> [ 15.458844] x23: x22: 0001 x21:
> 7fa61a28
> [ 15.465917] x20: 084ca080 x19: 7fa61a28 x18:
> 019bd700
> [ 15.472989] x17: 6d64685f77645f6e x16: x15:
> 0004
> [ 15.480062] x14: 89bab410 x13: x12:
> 0003
> [ 15.487135] x11: x10: x9 :
>
> [ 15.494207] x8 : 810a70a0 x7 : 64410079616b6f01 x6 :
> 80416403
> [ 15.501279] x5 : 03644100 x4 : 0080 x3 :
> 00416400
> [ 15.508352] x2 : 01128000 x1 : x0 :
>
> [ 15.515426] Call trace:
> [ 15.517863] Insufficient stack space to handle exception!
> [ 15.517867] ESR: 0x9647 -- DABT (current EL)
> [ 15.517871] FAR: 0x8a047ff0
> [ 15.517873] Task stack: [0x8a048000..0x8a04c000]
> [ 15.517877] IRQ stack: [0x88008000..0x8800c000]
> [ 15.517880] Overflow stack: [0x7d9c1320..0x7d9c2320]
> [ 15.517884] CPU: 1 PID: 9 Comm: kworker/u8:0 Not tainted
> 6.2.0-rc3-next-20230110 #1
> [ 15.517890] Hardware name: Libre Computer AML-S905X-CC (DT)
> [ 15.517895] Workqueue: events_unbound deferred_probe_work_func
> [ 15.517915] pstate: 83c5 (Nzcv DAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 15.517923] pc : el1_abort+0x4/0x5c
> [ 15.517932] lr : el1h_64_sync_handler+0x60/0xac
> [ 15.517939] sp : 8a048020
> [ 15.517941] x29: 8a048020 x28: 01128000 x27:
> 08de5808
> [ 15.517950] x26: 08de5800 x25: 8a04b608 x24:
> 01128000
> [ 15.517957] x23: a0c5 x22: 880321dc x21:
> 8a048180
> [ 15.517965] x20: 898e1000 x19: 8a048290 x18:
> 019bd700
> [ 15.517972] x17: 0011 x16: x15:
> 0004
> [ 15.517979] x14: 89bab410 x13: x12:
>
> [ 15.517986] x11: 0030 x10: 89013a1c x9 :
> 890401a0
> [ 15.517994] x8 : 0025 x7 : 205d363234353
Re: [PATCH 1/2] iommu: arm-smmu-impl: Add 8250 display compatible to the client list.
On Tue, 14 Jun 2022 16:01:35 -0700, Emma Anholt wrote: > Required for turning on per-process page tables for the GPU. > > Applied to will (for-joerg/arm-smmu/updates), thanks! [1/2] iommu: arm-smmu-impl: Add 8250 display compatible to the client list. https://git.kernel.org/will/c/3482c0b73073 [2/2] arm64: dts: qcom: sm8250: Enable per-process page tables. (no commit info) Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev
Re: [PATCH v5 5/9] iommu/arm-smmu: Attach to host1x context device bus
On Mon, May 16, 2022 at 11:52:54AM +0300, cyn...@kapsi.fi wrote:
> From: Mikko Perttunen
>
> Set itself as the IOMMU for the host1x context device bus, containing
> "dummy" devices used for Host1x context isolation.
>
> Signed-off-by: Mikko Perttunen
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 568cce590ccc..9ff54eaecf81 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -39,6 +39,7 @@
>
> #include
> #include
> +#include
>
> #include "arm-smmu.h"
>
> @@ -2053,8 +2054,20 @@ static int arm_smmu_bus_init(struct iommu_ops *ops)
> goto err_reset_pci_ops;
> }
> #endif
> +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS
> + if (!iommu_present(&host1x_context_device_bus_type)) {
> + err = bus_set_iommu(&host1x_context_device_bus_type, ops);
> + if (err)
> + goto err_reset_fsl_mc_ops;
> + }
> +#endif
> +
> return 0;
>
> +err_reset_fsl_mc_ops: __maybe_unused;
> +#ifdef CONFIG_FSL_MC_BUS
> + bus_set_iommu(&fsl_mc_bus_type, NULL);
> +#endif
bus_set_iommu() is going away:
https://lore.kernel.org/r/cover.1650890638.git.robin.mur...@arm.com
Will
>
Re: [PATCH v2 1/3] iommu/io-pgtable-arm: Add way to debug pgtable walk
On Tue, Oct 05, 2021 at 08:16:25AM -0700, Rob Clark wrote:
> From: Rob Clark
>
> Add an io-pgtable method to retrieve the raw PTEs that would be
> traversed for a given iova access.
>
> Signed-off-by: Rob Clark
> ---
> drivers/iommu/io-pgtable-arm.c | 40 +++---
> include/linux/io-pgtable.h | 9
> 2 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index dd9e47189d0d..c470fc0b3c2b 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -700,38 +700,61 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops
> *ops, unsigned long iova,
> return arm_lpae_unmap_pages(ops, iova, size, 1, gather);
> }
>
> -static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
> - unsigned long iova)
> +static int arm_lpae_pgtable_walk(struct io_pgtable_ops *ops, unsigned long
> iova,
> + void *_ptes, int *num_ptes)
> {
> struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> arm_lpae_iopte pte, *ptep = data->pgd;
> + arm_lpae_iopte *ptes = _ptes;
> + int max_ptes = *num_ptes;
> int lvl = data->start_level;
>
> + *num_ptes = 0;
> +
> do {
> + if (*num_ptes >= max_ptes)
> + return -ENOSPC;
> +
> /* Valid IOPTE pointer? */
> if (!ptep)
> - return 0;
> + return -EFAULT;
>
> /* Grab the IOPTE we're interested in */
> ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
> pte = READ_ONCE(*ptep);
>
> + ptes[(*num_ptes)++] = pte;
> +
> /* Valid entry? */
> if (!pte)
> - return 0;
> + return -EFAULT;
>
> /* Leaf entry? */
> if (iopte_leaf(pte, lvl, data->iop.fmt))
> - goto found_translation;
> + return 0;
>
> /* Take it to the next level */
> ptep = iopte_deref(pte, data);
> } while (++lvl < ARM_LPAE_MAX_LEVELS);
>
> - /* Ran out of page tables to walk */
> - return 0;
> + return -EFAULT;
> +}
> +
> +static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
> + unsigned long iova)
> +{
> + struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> + arm_lpae_iopte pte, ptes[ARM_LPAE_MAX_LEVELS];
> + int lvl, num_ptes = ARM_LPAE_MAX_LEVELS;
> + int ret;
> +
> + ret = arm_lpae_pgtable_walk(ops, iova, ptes, &num_ptes);
> + if (ret)
> + return 0;
> +
> + pte = ptes[num_ptes - 1];
> + lvl = num_ptes - 1 + data->start_level;
>
> -found_translation:
> iova &= (ARM_LPAE_BLOCK_SIZE(lvl, data) - 1);
> return iopte_to_paddr(pte, data) | iova;
> }
> @@ -816,6 +839,7 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg)
> .unmap = arm_lpae_unmap,
> .unmap_pages= arm_lpae_unmap_pages,
> .iova_to_phys = arm_lpae_iova_to_phys,
> + .pgtable_walk = arm_lpae_pgtable_walk,
> };
>
> return data;
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index 86af6f0a00a2..501f362a929c 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -148,6 +148,13 @@ struct io_pgtable_cfg {
> * @unmap:Unmap a physically contiguous memory region.
> * @unmap_pages: Unmap a range of virtually contiguous pages of the same
> size.
> * @iova_to_phys: Translate iova to physical address.
> + * @pgtable_walk: Return details of a page table walk for a given iova.
> + *This returns the array of PTEs in a format that is
> + *specific to the page table format. The number of
> + *PTEs can be format specific. The num_ptes parameter
> + *on input specifies the size of the ptes array, and
> + *on output the number of PTEs filled in (which depends
> + *on the number of PTEs walked to resolve the iova)
I think this would be a fair bit cleaner if the interface instead took a
callback function to invoke at each page-table level. It would be invoked
with the pte value and the level. Depending on its return value the walk
could be terminated early. That would also potentially scale to walking
ranges of iovas as well if we ever need it and it may be more readily
implementable by other formats too.
> *
> * These functions map directly onto the iommu_ops member functions with
> * the same names.
This bit of the comment is no longer true with your change.
Will
Re: [PATCH] iommu/arm-smmu-qcom: Fix TTBR0 read
On Mon, 8 Nov 2021 09:17:23 -0800, Rob Clark wrote: > From: Rob Clark > > It is a 64b register, lets not lose the upper bits. > > Applied to will (for-joerg/arm-smmu/updates), thanks! [1/1] iommu/arm-smmu-qcom: Fix TTBR0 read https://git.kernel.org/will/c/c31112fbd407 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev
Re: [PATCH] [RFC] qcom_scm: hide Kconfig symbol
On Mon, Sep 27, 2021 at 05:22:13PM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann > > Now that SCM can be a loadable module, we have to add another > dependency to avoid link failures when ipa or adreno-gpu are > built-in: > > aarch64-linux-ld: drivers/net/ipa/ipa_main.o: in function `ipa_probe': > ipa_main.c:(.text+0xfc4): undefined reference to `qcom_scm_is_available' > > ld.lld: error: undefined symbol: qcom_scm_is_available > >>> referenced by adreno_gpu.c > >>> gpu/drm/msm/adreno/adreno_gpu.o:(adreno_zap_shader_load) in > >>> archive drivers/built-in.a > > This can happen when CONFIG_ARCH_QCOM is disabled and we don't select > QCOM_MDT_LOADER, but some other module selects QCOM_SCM. Ideally we'd > use a similar dependency here to what we have for QCOM_RPROC_COMMON, > but that causes dependency loops from other things selecting QCOM_SCM. > > This appears to be an endless problem, so try something different this > time: > > - CONFIG_QCOM_SCM becomes a hidden symbol that nothing 'depends on' >but that is simply selected by all of its users > > - All the stubs in include/linux/qcom_scm.h can go away > > - arm-smccc.h needs to provide a stub for __arm_smccc_smc() to >allow compile-testing QCOM_SCM on all architectures. > > - To avoid a circular dependency chain involving RESET_CONTROLLER >and PINCTRL_SUNXI, change the 'depends on RESET_CONTROLLER' in >the latter one to 'select'. > > The last bit is rather annoying, as drivers should generally never > 'select' another subsystem, and about half the users of the reset > controller interface do this anyway. > > Nevertheless, this version seems to pass all my randconfig tests > and is more robust than any of the prior versions. > > Comments? > > Signed-off-by: Arnd Bergmann > --- > drivers/firmware/Kconfig| 4 +- > drivers/gpu/drm/msm/Kconfig | 4 +- > drivers/iommu/Kconfig | 2 +- > drivers/media/platform/Kconfig | 2 +- > drivers/mmc/host/Kconfig| 2 +- > drivers/net/ipa/Kconfig | 1 + > drivers/net/wireless/ath/ath10k/Kconfig | 2 +- > drivers/pinctrl/qcom/Kconfig| 3 +- > drivers/pinctrl/sunxi/Kconfig | 6 +-- > include/linux/arm-smccc.h | 10 > include/linux/qcom_scm.h| 71 - > 11 files changed, 23 insertions(+), 84 deletions(-) > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > index 220a58cf0a44..f7dd82ef0b9c 100644 > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -203,9 +203,7 @@ config INTEL_STRATIX10_RSU > Say Y here if you want Intel RSU support. > > config QCOM_SCM > - tristate "Qcom SCM driver" > - depends on ARM || ARM64 > - depends on HAVE_ARM_SMCCC > + tristate > select RESET_CONTROLLER > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig > index e9c6af78b1d7..3ddf739a6f9b 100644 > --- a/drivers/gpu/drm/msm/Kconfig > +++ b/drivers/gpu/drm/msm/Kconfig > @@ -17,7 +17,7 @@ config DRM_MSM > select DRM_SCHED > select SHMEM > select TMPFS > - select QCOM_SCM if ARCH_QCOM > + select QCOM_SCM > select WANT_DEV_COREDUMP > select SND_SOC_HDMI_CODEC if SND_SOC > select SYNC_FILE > @@ -55,7 +55,7 @@ config DRM_MSM_GPU_SUDO > > config DRM_MSM_HDMI_HDCP > bool "Enable HDMI HDCP support in MSM DRM driver" > - depends on DRM_MSM && QCOM_SCM > + depends on DRM_MSM > default y > help > Choose this option to enable HDCP state machine > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 124c41adeca1..989c83acbfee 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -308,7 +308,7 @@ config APPLE_DART > config ARM_SMMU > tristate "ARM Ltd. System MMU (SMMU) Support" > depends on ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64) > - depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y > + select QCOM_SCM > select IOMMU_API > select IOMMU_IO_PGTABLE_LPAE > select ARM_DMA_USE_IOMMU if ARM I don't want to get in the way of this patch because I'm also tired of the randconfig failures caused by QCOM_SCM. However, ARM_SMMU is applicable to a wide variety of (non-qcom) SoCs and so it seems a shame to require the QCOM_SCM code to be included for all of those when it's not strictly needed at all. Will
Re: [Freedreno] [PATCH 0/3] iommu/drm/msm: Allow non-coherent masters to use system cache
On Mon, Aug 09, 2021 at 11:17:40PM +0530, Sai Prakash Ranjan wrote: > On 2021-08-09 23:10, Will Deacon wrote: > > On Mon, Aug 09, 2021 at 10:18:21AM -0700, Rob Clark wrote: > > > On Mon, Aug 9, 2021 at 10:05 AM Will Deacon wrote: > > > > On Mon, Aug 09, 2021 at 09:57:08AM -0700, Rob Clark wrote: > > > > > But I suppose we could call it instead IOMMU_QCOM_LLC or something > > > > > like that to make it more clear that it is not necessarily something > > > > > that would work with a different outer level cache implementation? > > > > > > > > ... or we could just deal with the problem so that other people can > > > > reuse > > > > the code. I haven't really understood the reluctance to solve this > > > > properly. > > > > > > > > Am I missing some reason this isn't solvable? > > > > > > Oh, was there another way to solve it (other than foregoing setting > > > INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a > > > corresponding setting on the MMU pgtables side of things? > > > > Right -- we just need to program the CPU's MMU with the matching memory > > attributes! It's a bit more fiddly if you're just using ioremap_wc() > > though, as it's usually the DMA API which handles the attributes under > > the > > hood. > > > > Anyway, sorry, I should've said that explicitly earlier on. We've done > > this > > sort of thing in the Android tree so I assumed Sai knew what needed to > > be > > done and then I didn't think to explain to you :( > > > > Right I was aware of that but even in the android tree there is no user :) I'm assuming there are vendor modules using it there, otherwise we wouldn't have been asked to put it in. Since you work at Qualcomm, maybe you could talk to your colleagues (Isaac and Patrick) directly? > I think we can't have a new memory type without any user right in upstream > like android tree? Correct. But I don't think we should be adding IOMMU_* anything upstream if we don't have a user. Will
Re: [Freedreno] [PATCH 0/3] iommu/drm/msm: Allow non-coherent masters to use system cache
On Mon, Aug 09, 2021 at 10:18:21AM -0700, Rob Clark wrote:
> On Mon, Aug 9, 2021 at 10:05 AM Will Deacon wrote:
> >
> > On Mon, Aug 09, 2021 at 09:57:08AM -0700, Rob Clark wrote:
> > > On Mon, Aug 9, 2021 at 7:56 AM Will Deacon wrote:
> > > > On Mon, Aug 02, 2021 at 06:36:04PM -0700, Rob Clark wrote:
> > > > > On Mon, Aug 2, 2021 at 8:14 AM Will Deacon wrote:
> > > > > > On Mon, Aug 02, 2021 at 08:08:07AM -0700, Rob Clark wrote:
> > > > > > > On Mon, Aug 2, 2021 at 3:55 AM Will Deacon
> > > > > > > wrote:
> > > > > > > > On Thu, Jul 29, 2021 at 10:08:22AM +0530, Sai Prakash Ranjan
> > > > > > > > wrote:
> > > > > > > > > On 2021-07-28 19:30, Georgi Djakov wrote:
> > > > > > > > > > On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash
> > > > > > > > > > Ranjan wrote:
> > > > > > > > > > > commit ecd7274fb4cd ("iommu: Remove unused
> > > > > > > > > > > IOMMU_SYS_CACHE_ONLY flag")
> > > > > > > > > > > removed unused IOMMU_SYS_CACHE_ONLY prot flag and along
> > > > > > > > > > > with it went
> > > > > > > > > > > the memory type setting required for the non-coherent
> > > > > > > > > > > masters to use
> > > > > > > > > > > system cache. Now that system cache support for GPU is
> > > > > > > > > > > added, we will
> > > > > > > > > > > need to set the right PTE attribute for GPU buffers to be
> > > > > > > > > > > sys cached.
> > > > > > > > > > > Without this, the system cache lines are not allocated
> > > > > > > > > > > for GPU.
> > > > > > > > > > >
> > > > > > > > > > > So the patches in this series introduces a new prot flag
> > > > > > > > > > > IOMMU_LLC,
> > > > > > > > > > > renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to
> > > > > > > > > > > IO_PGTABLE_QUIRK_PTW_LLC
> > > > > > > > > > > and makes GPU the user of this protection flag.
> > > > > > > > > >
> > > > > > > > > > Thank you for the patchset! Are you planning to refresh it,
> > > > > > > > > > as it does
> > > > > > > > > > not apply anymore?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I was waiting on Will's reply [1]. If there are no changes
> > > > > > > > > needed, then
> > > > > > > > > I can repost the patch.
> > > > > > > >
> > > > > > > > I still think you need to handle the mismatched alias, no?
> > > > > > > > You're adding
> > > > > > > > a new memory type to the SMMU which doesn't exist on the CPU
> > > > > > > > side. That
> > > > > > > > can't be right.
> > > > > > > >
> > > > > > >
> > > > > > > Just curious, and maybe this is a dumb question, but what is your
> > > > > > > concern about mismatched aliases? I mean the cache hierarchy on
> > > > > > > the
> > > > > > > GPU device side (anything beyond the LLC) is pretty different and
> > > > > > > doesn't really care about the smmu pgtable attributes..
> > > > > >
> > > > > > If the CPU accesses a shared buffer with different attributes to
> > > > > > those which
> > > > > > the device is using then you fall into the "mismatched memory
> > > > > > attributes"
> > > > > > part of the Arm architecture. It's reasonably unforgiving (you
> > > > > > should go and
> > > > > > read it) and in some cases can apply to speculative accesses as
> > > > > > well, but
> > > > > > the end result is typically loss of coherency.
> > > > >
> > > > > Ok, I might have a few other sections to read first to decipher the
> > >
Re: [Freedreno] [PATCH 0/3] iommu/drm/msm: Allow non-coherent masters to use system cache
On Mon, Aug 09, 2021 at 09:57:08AM -0700, Rob Clark wrote:
> On Mon, Aug 9, 2021 at 7:56 AM Will Deacon wrote:
> > On Mon, Aug 02, 2021 at 06:36:04PM -0700, Rob Clark wrote:
> > > On Mon, Aug 2, 2021 at 8:14 AM Will Deacon wrote:
> > > > On Mon, Aug 02, 2021 at 08:08:07AM -0700, Rob Clark wrote:
> > > > > On Mon, Aug 2, 2021 at 3:55 AM Will Deacon wrote:
> > > > > > On Thu, Jul 29, 2021 at 10:08:22AM +0530, Sai Prakash Ranjan wrote:
> > > > > > > On 2021-07-28 19:30, Georgi Djakov wrote:
> > > > > > > > On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash Ranjan
> > > > > > > > wrote:
> > > > > > > > > commit ecd7274fb4cd ("iommu: Remove unused
> > > > > > > > > IOMMU_SYS_CACHE_ONLY flag")
> > > > > > > > > removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with
> > > > > > > > > it went
> > > > > > > > > the memory type setting required for the non-coherent masters
> > > > > > > > > to use
> > > > > > > > > system cache. Now that system cache support for GPU is added,
> > > > > > > > > we will
> > > > > > > > > need to set the right PTE attribute for GPU buffers to be sys
> > > > > > > > > cached.
> > > > > > > > > Without this, the system cache lines are not allocated for
> > > > > > > > > GPU.
> > > > > > > > >
> > > > > > > > > So the patches in this series introduces a new prot flag
> > > > > > > > > IOMMU_LLC,
> > > > > > > > > renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to
> > > > > > > > > IO_PGTABLE_QUIRK_PTW_LLC
> > > > > > > > > and makes GPU the user of this protection flag.
> > > > > > > >
> > > > > > > > Thank you for the patchset! Are you planning to refresh it, as
> > > > > > > > it does
> > > > > > > > not apply anymore?
> > > > > > > >
> > > > > > >
> > > > > > > I was waiting on Will's reply [1]. If there are no changes
> > > > > > > needed, then
> > > > > > > I can repost the patch.
> > > > > >
> > > > > > I still think you need to handle the mismatched alias, no? You're
> > > > > > adding
> > > > > > a new memory type to the SMMU which doesn't exist on the CPU side.
> > > > > > That
> > > > > > can't be right.
> > > > > >
> > > > >
> > > > > Just curious, and maybe this is a dumb question, but what is your
> > > > > concern about mismatched aliases? I mean the cache hierarchy on the
> > > > > GPU device side (anything beyond the LLC) is pretty different and
> > > > > doesn't really care about the smmu pgtable attributes..
> > > >
> > > > If the CPU accesses a shared buffer with different attributes to those
> > > > which
> > > > the device is using then you fall into the "mismatched memory
> > > > attributes"
> > > > part of the Arm architecture. It's reasonably unforgiving (you should
> > > > go and
> > > > read it) and in some cases can apply to speculative accesses as well,
> > > > but
> > > > the end result is typically loss of coherency.
> > >
> > > Ok, I might have a few other sections to read first to decipher the
> > > terminology..
> > >
> > > But my understanding of LLC is that it looks just like system memory
> > > to the CPU and GPU (I think that would make it "the point of
> > > coherence" between the GPU and CPU?) If that is true, shouldn't it be
> > > invisible from the point of view of different CPU mapping options?
> >
> > You could certainly build a system where mismatched attributes don't cause
> > loss of coherence, but as it's not guaranteed by the architecture and the
> > changes proposed here affect APIs which are exposed across SoCs, then I
> > don't think it helps much.
> >
>
> Hmm, the description of the new mapping flag is that it applies only
> to transparent outer level cache:
>
> +/*
> + * Non-coherent masters can use this page protection flag to set cacheable
> + * memory attributes for only a transparent outer level of cache, also known
> as
> + * the last-level or system cache.
> + */
> +#define IOMMU_LLC (1 << 6)
>
> But I suppose we could call it instead IOMMU_QCOM_LLC or something
> like that to make it more clear that it is not necessarily something
> that would work with a different outer level cache implementation?
... or we could just deal with the problem so that other people can reuse
the code. I haven't really understood the reluctance to solve this properly.
Am I missing some reason this isn't solvable?
Will
Re: [Freedreno] [PATCH 0/3] iommu/drm/msm: Allow non-coherent masters to use system cache
On Mon, Aug 02, 2021 at 06:36:04PM -0700, Rob Clark wrote:
> On Mon, Aug 2, 2021 at 8:14 AM Will Deacon wrote:
> >
> > On Mon, Aug 02, 2021 at 08:08:07AM -0700, Rob Clark wrote:
> > > On Mon, Aug 2, 2021 at 3:55 AM Will Deacon wrote:
> > > >
> > > > On Thu, Jul 29, 2021 at 10:08:22AM +0530, Sai Prakash Ranjan wrote:
> > > > > On 2021-07-28 19:30, Georgi Djakov wrote:
> > > > > > On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash Ranjan wrote:
> > > > > > > commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY
> > > > > > > flag")
> > > > > > > removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it
> > > > > > > went
> > > > > > > the memory type setting required for the non-coherent masters to
> > > > > > > use
> > > > > > > system cache. Now that system cache support for GPU is added, we
> > > > > > > will
> > > > > > > need to set the right PTE attribute for GPU buffers to be sys
> > > > > > > cached.
> > > > > > > Without this, the system cache lines are not allocated for GPU.
> > > > > > >
> > > > > > > So the patches in this series introduces a new prot flag
> > > > > > > IOMMU_LLC,
> > > > > > > renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to
> > > > > > > IO_PGTABLE_QUIRK_PTW_LLC
> > > > > > > and makes GPU the user of this protection flag.
> > > > > >
> > > > > > Thank you for the patchset! Are you planning to refresh it, as it
> > > > > > does
> > > > > > not apply anymore?
> > > > > >
> > > > >
> > > > > I was waiting on Will's reply [1]. If there are no changes needed,
> > > > > then
> > > > > I can repost the patch.
> > > >
> > > > I still think you need to handle the mismatched alias, no? You're adding
> > > > a new memory type to the SMMU which doesn't exist on the CPU side. That
> > > > can't be right.
> > > >
> > >
> > > Just curious, and maybe this is a dumb question, but what is your
> > > concern about mismatched aliases? I mean the cache hierarchy on the
> > > GPU device side (anything beyond the LLC) is pretty different and
> > > doesn't really care about the smmu pgtable attributes..
> >
> > If the CPU accesses a shared buffer with different attributes to those which
> > the device is using then you fall into the "mismatched memory attributes"
> > part of the Arm architecture. It's reasonably unforgiving (you should go and
> > read it) and in some cases can apply to speculative accesses as well, but
> > the end result is typically loss of coherency.
>
> Ok, I might have a few other sections to read first to decipher the
> terminology..
>
> But my understanding of LLC is that it looks just like system memory
> to the CPU and GPU (I think that would make it "the point of
> coherence" between the GPU and CPU?) If that is true, shouldn't it be
> invisible from the point of view of different CPU mapping options?
You could certainly build a system where mismatched attributes don't cause
loss of coherence, but as it's not guaranteed by the architecture and the
changes proposed here affect APIs which are exposed across SoCs, then I
don't think it helps much.
Will
Re: [Freedreno] [PATCH 0/3] iommu/drm/msm: Allow non-coherent masters to use system cache
On Mon, Aug 02, 2021 at 08:08:07AM -0700, Rob Clark wrote:
> On Mon, Aug 2, 2021 at 3:55 AM Will Deacon wrote:
> >
> > On Thu, Jul 29, 2021 at 10:08:22AM +0530, Sai Prakash Ranjan wrote:
> > > On 2021-07-28 19:30, Georgi Djakov wrote:
> > > > On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash Ranjan wrote:
> > > > > commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag")
> > > > > removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went
> > > > > the memory type setting required for the non-coherent masters to use
> > > > > system cache. Now that system cache support for GPU is added, we will
> > > > > need to set the right PTE attribute for GPU buffers to be sys cached.
> > > > > Without this, the system cache lines are not allocated for GPU.
> > > > >
> > > > > So the patches in this series introduces a new prot flag IOMMU_LLC,
> > > > > renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC
> > > > > and makes GPU the user of this protection flag.
> > > >
> > > > Thank you for the patchset! Are you planning to refresh it, as it does
> > > > not apply anymore?
> > > >
> > >
> > > I was waiting on Will's reply [1]. If there are no changes needed, then
> > > I can repost the patch.
> >
> > I still think you need to handle the mismatched alias, no? You're adding
> > a new memory type to the SMMU which doesn't exist on the CPU side. That
> > can't be right.
> >
>
> Just curious, and maybe this is a dumb question, but what is your
> concern about mismatched aliases? I mean the cache hierarchy on the
> GPU device side (anything beyond the LLC) is pretty different and
> doesn't really care about the smmu pgtable attributes..
If the CPU accesses a shared buffer with different attributes to those which
the device is using then you fall into the "mismatched memory attributes"
part of the Arm architecture. It's reasonably unforgiving (you should go and
read it) and in some cases can apply to speculative accesses as well, but
the end result is typically loss of coherency.
Will
Re: [PATCH 0/3] iommu/drm/msm: Allow non-coherent masters to use system cache
On Thu, Jul 29, 2021 at 10:08:22AM +0530, Sai Prakash Ranjan wrote:
> On 2021-07-28 19:30, Georgi Djakov wrote:
> > On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash Ranjan wrote:
> > > commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag")
> > > removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went
> > > the memory type setting required for the non-coherent masters to use
> > > system cache. Now that system cache support for GPU is added, we will
> > > need to set the right PTE attribute for GPU buffers to be sys cached.
> > > Without this, the system cache lines are not allocated for GPU.
> > >
> > > So the patches in this series introduces a new prot flag IOMMU_LLC,
> > > renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC
> > > and makes GPU the user of this protection flag.
> >
> > Thank you for the patchset! Are you planning to refresh it, as it does
> > not apply anymore?
> >
>
> I was waiting on Will's reply [1]. If there are no changes needed, then
> I can repost the patch.
I still think you need to handle the mismatched alias, no? You're adding
a new memory type to the SMMU which doesn't exist on the CPU side. That
can't be right.
Will
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Tue, Jul 06, 2021 at 12:59:57PM -0400, Konrad Rzeszutek Wilk wrote: > On Tue, Jul 06, 2021 at 05:57:21PM +0100, Will Deacon wrote: > > On Tue, Jul 06, 2021 at 10:46:07AM -0400, Konrad Rzeszutek Wilk wrote: > > > On Tue, Jul 06, 2021 at 04:05:13PM +0200, Christoph Hellwig wrote: > > > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > > > FWIW I was pondering the question of whether to do something along > > > > > those > > > > > lines or just scrap the default assignment entirely, so since I > > > > > hadn't got > > > > > round to saying that I've gone ahead and hacked up the alternative > > > > > (similarly untested) for comparison :) > > > > > > > > > > TBH I'm still not sure which one I prefer... > > > > > > > > Claire did implement something like your suggestion originally, but > > > > I don't really like it as it doesn't scale for adding multiple global > > > > pools, e.g. for the 64-bit addressable one for the various encrypted > > > > secure guest schemes. > > > > > > Couple of things: > > > - I am not pushing to Linus the Claire's patchset until we have a > > >resolution on this. I hope you all agree that is a sensible way > > >forward as much as I hate doing that. > > > > Sure, it's a pity but we could clearly use a bit more time to get these > > just right and we've run out of time for 5.14. > > > > I think the main question I have is how would you like to see patches for > > 5.15? i.e. as patches on top of devel/for-linus-5.14 or something else? > > Yes that would be perfect. If there are any dependencies on the rc1, I > can rebase it on top of that. Yes, please, rebasing would be very helpful. The broader rework of 'io_tlb_default_mem' is going to conflict quite badly otherwise. Cheers, Will
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Tue, Jul 06, 2021 at 12:14:16PM -0700, Nathan Chancellor wrote: > On 7/6/2021 10:06 AM, Will Deacon wrote: > > On Tue, Jul 06, 2021 at 04:39:11PM +0100, Robin Murphy wrote: > > > On 2021-07-06 15:05, Christoph Hellwig wrote: > > > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > > > FWIW I was pondering the question of whether to do something along > > > > > those > > > > > lines or just scrap the default assignment entirely, so since I > > > > > hadn't got > > > > > round to saying that I've gone ahead and hacked up the alternative > > > > > (similarly untested) for comparison :) > > > > > > > > > > TBH I'm still not sure which one I prefer... > > > > > > > > Claire did implement something like your suggestion originally, but > > > > I don't really like it as it doesn't scale for adding multiple global > > > > pools, e.g. for the 64-bit addressable one for the various encrypted > > > > secure guest schemes. > > > > > > Ah yes, that had slipped my mind, and it's a fair point indeed. Since > > > we're > > > not concerned with a minimal fix for backports anyway I'm more than happy > > > to > > > focus on Will's approach. Another thing is that that looks to take us a > > > quiet step closer to the possibility of dynamically resizing a SWIOTLB > > > pool, > > > which is something that some of the hypervisor protection schemes looking > > > to > > > build on top of this series may want to explore at some point. > > > > Ok, I'll split that nasty diff I posted up into a reviewable series and we > > can take it from there. > > For what it's worth, I attempted to boot Will's diff on top of Konrad's > devel/for-linus-5.14 and it did not work; in fact, I got no output on my > monitor period, even with earlyprintk=, and I do not think this machine has > a serial console. Looking back at the diff, I completely messed up swiotlb_exit() by mixing up physical and virtual addresses. > Robin's fix does work, it survived ten reboots with no issues getting to X > and I do not see the KASAN and slub debug messages anymore but I understand > that this is not the preferred solution it seems (although Konrad did want > to know if it works). > > I am happy to test any further patches or follow ups as needed, just keep me > on CC. Cheers. Since this isn't 5.14 material any more, I'll CC you on a series next week. Will
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Tue, Jul 06, 2021 at 04:39:11PM +0100, Robin Murphy wrote: > On 2021-07-06 15:05, Christoph Hellwig wrote: > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > FWIW I was pondering the question of whether to do something along those > > > lines or just scrap the default assignment entirely, so since I hadn't got > > > round to saying that I've gone ahead and hacked up the alternative > > > (similarly untested) for comparison :) > > > > > > TBH I'm still not sure which one I prefer... > > > > Claire did implement something like your suggestion originally, but > > I don't really like it as it doesn't scale for adding multiple global > > pools, e.g. for the 64-bit addressable one for the various encrypted > > secure guest schemes. > > Ah yes, that had slipped my mind, and it's a fair point indeed. Since we're > not concerned with a minimal fix for backports anyway I'm more than happy to > focus on Will's approach. Another thing is that that looks to take us a > quiet step closer to the possibility of dynamically resizing a SWIOTLB pool, > which is something that some of the hypervisor protection schemes looking to > build on top of this series may want to explore at some point. Ok, I'll split that nasty diff I posted up into a reviewable series and we can take it from there. Will
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Tue, Jul 06, 2021 at 10:46:07AM -0400, Konrad Rzeszutek Wilk wrote: > On Tue, Jul 06, 2021 at 04:05:13PM +0200, Christoph Hellwig wrote: > > On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: > > > FWIW I was pondering the question of whether to do something along those > > > lines or just scrap the default assignment entirely, so since I hadn't > > > got > > > round to saying that I've gone ahead and hacked up the alternative > > > (similarly untested) for comparison :) > > > > > > TBH I'm still not sure which one I prefer... > > > > Claire did implement something like your suggestion originally, but > > I don't really like it as it doesn't scale for adding multiple global > > pools, e.g. for the 64-bit addressable one for the various encrypted > > secure guest schemes. > > Couple of things: > - I am not pushing to Linus the Claire's patchset until we have a >resolution on this. I hope you all agree that is a sensible way >forward as much as I hate doing that. Sure, it's a pity but we could clearly use a bit more time to get these just right and we've run out of time for 5.14. I think the main question I have is how would you like to see patches for 5.15? i.e. as patches on top of devel/for-linus-5.14 or something else? Cheers, Will
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Tue, Jul 06, 2021 at 06:48:48AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 05, 2021 at 08:03:52PM +0100, Will Deacon wrote:
> > So at this point, the AMD IOMMU driver does:
> >
> > swiotlb= (iommu_default_passthrough() || sme_me_mask) ? 1 : 0;
> >
> > where 'swiotlb' is a global variable indicating whether or not swiotlb
> > is in use. It's picked up a bit later on by pci_swiotlb_late_init(), which
> > will call swiotlb_exit() if 'swiotlb' is false.
> >
> > Now, that used to work fine, because swiotlb_exit() clears
> > 'io_tlb_default_mem' to NULL, but now with the restricted DMA changes, I
> > think that all the devices which have successfully probed beforehand will
> > have stale pointers to the freed structure in their 'dev->dma_io_tlb_mem'
> > field.
>
> Yeah. I don't think we can do that anymore, and I also think it is
> a bad idea to start with.
I've had a crack at reworking things along the following lines:
- io_tlb_default_mem now lives in the BSS, the flexible array member
is now a pointer and that part is allocated dynamically (downside of
this is an extra indirection to get at the slots).
- io_tlb_default_mem.nslabs tells you whether the thing is valid
- swiotlb_exit() frees the slots array and clears the rest of the
structure to 0. I also extended it to free the actual slabs, but I'm
not sure why it wasn't doing that before.
So a non-NULL dev->dma_io_tlb_mem should always be valid to follow.
Untested diff below... Nathan, it would be ace if you're brave enough
to give this a shot.
Will
--->8
diff --git a/drivers/base/core.c b/drivers/base/core.c
index bbad7c559901..9e1218f89e4b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2820,7 +2820,7 @@ void device_initialize(struct device *dev)
dev->dma_coherent = dma_default_coherent;
#endif
#ifdef CONFIG_SWIOTLB
- dev->dma_io_tlb_mem = io_tlb_default_mem;
+ dev->dma_io_tlb_mem = &io_tlb_default_mem;
#endif
}
EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 785ec7e8be01..f06d9b4f1e0f 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -164,7 +164,7 @@ int __ref xen_swiotlb_init(void)
int rc = -ENOMEM;
char *start;
- if (io_tlb_default_mem != NULL) {
+ if (io_tlb_default_mem.nslabs) {
pr_warn("swiotlb buffer already initialized\n");
return -EEXIST;
}
@@ -547,7 +547,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct
scatterlist *sgl,
static int
xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
- return xen_phys_to_dma(hwdev, io_tlb_default_mem->end - 1) <= mask;
+ return xen_phys_to_dma(hwdev, io_tlb_default_mem.end - 1) <= mask;
}
const struct dma_map_ops xen_swiotlb_dma_ops = {
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 39284ff2a6cd..b0cb2a9973f4 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -103,9 +103,9 @@ struct io_tlb_mem {
phys_addr_t orig_addr;
size_t alloc_size;
unsigned int list;
- } slots[];
+ } *slots;
};
-extern struct io_tlb_mem *io_tlb_default_mem;
+extern struct io_tlb_mem io_tlb_default_mem;
static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr)
{
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 0ffbaae9fba2..91cd1d413027 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -70,7 +70,7 @@
enum swiotlb_force swiotlb_force;
-struct io_tlb_mem *io_tlb_default_mem;
+struct io_tlb_mem io_tlb_default_mem;
/*
* Max segment that we can provide which (if pages are contingous) will
@@ -101,7 +101,7 @@ early_param("swiotlb", setup_io_tlb_npages);
unsigned int swiotlb_max_segment(void)
{
- return io_tlb_default_mem ? max_segment : 0;
+ return io_tlb_default_mem.nslabs ? max_segment : 0;
}
EXPORT_SYMBOL_GPL(swiotlb_max_segment);
@@ -134,9 +134,9 @@ void __init swiotlb_adjust_size(unsigned long size)
void swiotlb_print_info(void)
{
- struct io_tlb_mem *mem = io_tlb_default_mem;
+ struct io_tlb_mem *mem = &io_tlb_default_mem;
- if (!mem) {
+ if (!mem->nslabs) {
pr_warn("No low mem\n");
return;
}
@@ -163,11 +163,11 @@ static inline unsigned long nr_slots(u64 val)
*/
void __init swiotlb_update_mem_attributes(void)
{
- struct io_tlb_mem *mem = io_tlb_default_mem;
+ struct io_tlb_mem *mem = &io_tlb_default_mem;
void *vaddr;
unsigned long bytes;
- if (!mem || mem->late_alloc)
+ if (!mem->nslabs || mem->late_alloc)
ret
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
Hi Nathan, I may have just spotted something in these logs... On Fri, Jul 02, 2021 at 10:55:17PM -0700, Nathan Chancellor wrote: > [2.340956] pci :0c:00.1: Adding to iommu group 4 > [2.340996] pci :0c:00.2: Adding to iommu group 4 > [2.341038] pci :0c:00.3: Adding to iommu group 4 > [2.341078] pci :0c:00.4: Adding to iommu group 4 > [2.341122] pci :0c:00.6: Adding to iommu group 4 > [2.341163] pci :0d:00.0: Adding to iommu group 4 > [2.341203] pci :0d:00.1: Adding to iommu group 4 > [2.361821] pci :00:00.2: AMD-Vi: Found IOMMU cap 0x40 > [2.361839] pci :00:00.2: AMD-Vi: Extended features > (0x206d73ef22254ade): > [2.361846] PPR X2APIC NX GT IA GA PC GA_vAPIC > [2.361861] AMD-Vi: Interrupt remapping enabled > [2.361865] AMD-Vi: Virtual APIC enabled > [2.361870] AMD-Vi: X2APIC enabled > [2.362272] AMD-Vi: Lazy IO/TLB flushing enabled So at this point, the AMD IOMMU driver does: swiotlb= (iommu_default_passthrough() || sme_me_mask) ? 1 : 0; where 'swiotlb' is a global variable indicating whether or not swiotlb is in use. It's picked up a bit later on by pci_swiotlb_late_init(), which will call swiotlb_exit() if 'swiotlb' is false. Now, that used to work fine, because swiotlb_exit() clears 'io_tlb_default_mem' to NULL, but now with the restricted DMA changes, I think that all the devices which have successfully probed beforehand will have stale pointers to the freed structure in their 'dev->dma_io_tlb_mem' field. Will
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
Hi Nathan, On Thu, Jul 01, 2021 at 12:52:20AM -0700, Nathan Chancellor wrote: > On 7/1/2021 12:40 AM, Will Deacon wrote: > > On Wed, Jun 30, 2021 at 08:56:51AM -0700, Nathan Chancellor wrote: > > > On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: > > > > On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > > > > > `BUG: unable to handle page fault for address: 003a8290` and > > > > > the fact it crashed at `_raw_spin_lock_irqsave` look like the memory > > > > > (maybe dev->dma_io_tlb_mem) was corrupted? > > > > > The dev->dma_io_tlb_mem should be set here > > > > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > > > > > through device_initialize. > > > > > > > > I'm less sure about this. 'dma_io_tlb_mem' should be pointing at > > > > 'io_tlb_default_mem', which is a page-aligned allocation from memblock. > > > > The spinlock is at offset 0x24 in that structure, and looking at the > > > > register dump from the crash: > > > > > > > > Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:adb4013db9e8 EFLAGS: > > > > 00010006 > > > > Jun 29 18:28:42 hp-4300G kernel: RAX: 003a8290 RBX: > > > > RCX: 8900572ad580 > > > > Jun 29 18:28:42 hp-4300G kernel: RDX: 89005653f024 RSI: > > > > 000c RDI: 1d17 > > > > Jun 29 18:28:42 hp-4300G kernel: RBP: 0a20d000 R08: > > > > 000c R09: > > > > Jun 29 18:28:42 hp-4300G kernel: R10: 0a20d000 R11: > > > > 89005653f000 R12: 0212 > > > > Jun 29 18:28:42 hp-4300G kernel: R13: 1000 R14: > > > > 0002 R15: 0020 > > > > Jun 29 18:28:42 hp-4300G kernel: FS: 7f1f8898ea40() > > > > GS:89005728() knlGS: > > > > Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: ES: CR0: > > > > 80050033 > > > > Jun 29 18:28:42 hp-4300G kernel: CR2: 003a8290 CR3: > > > > 0001020d CR4: 00350ee0 > > > > Jun 29 18:28:42 hp-4300G kernel: Call Trace: > > > > Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 > > > > Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 > > > > > > > > Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and > > > > RDX pointing at the spinlock. Yet RAX is holding junk :/ > > > > > > > > I agree that enabling KASAN would be a good idea, but I also think we > > > > probably need to get some more information out of > > > > swiotlb_tbl_map_single() > > > > to see see what exactly is going wrong in there. > > > > > > I can certainly enable KASAN and if there is any debug print I can add > > > or dump anything, let me know! > > > > I bit the bullet and took v5.13 with swiotlb/for-linus-5.14 merged in, built > > x86 defconfig and ran it on my laptop. However, it seems to work fine! > > > > Please can you share your .config? > > Sure thing, it is attached. It is just Arch Linux's config run through > olddefconfig. The original is below in case you need to diff it. > > https://raw.githubusercontent.com/archlinux/svntogit-packages/9045405dc835527164f3034b3ceb9a67c7a53cd4/trunk/config > > If there is anything more that I can provide, please let me know. I eventually got this booting (for some reason it was causing LD to SEGV trying to link it for a while...) and sadly it works fine on my laptop. Hmm. Did you manage to try again with KASAN? It might also be worth taking the IOMMU out of the equation, since that interfaces differently with SWIOTLB and I couldn't figure out the code path from the log you provided. What happens if you boot with "amd_iommu=off swiotlb=force"? (although word of warning here: i915 dies horribly on my laptop if I pass swiotlb=force, even with the distro 5.10 kernel) Will
Re: [PATCH v15 12/12] of: Add plumbing for restricted DMA pool
On Fri, Jul 02, 2021 at 12:39:41PM +0100, Robin Murphy wrote:
> On 2021-07-02 04:08, Guenter Roeck wrote:
> > On Thu, Jun 24, 2021 at 11:55:26PM +0800, Claire Chang wrote:
> > > If a device is not behind an IOMMU, we look up the device node and set
> > > up the restricted DMA when the restricted-dma-pool is presented.
> > >
> > > Signed-off-by: Claire Chang
> > > Tested-by: Stefano Stabellini
> > > Tested-by: Will Deacon
> >
> > With this patch in place, all sparc and sparc64 qemu emulations
> > fail to boot. Symptom is that the root file system is not found.
> > Reverting this patch fixes the problem. Bisect log is attached.
>
> Ah, OF_ADDRESS depends on !SPARC, so of_dma_configure_id() is presumably
> returning an unexpected -ENODEV from the of_dma_set_restricted_buffer()
> stub. That should probably be returning 0 instead, since either way it's not
> an error condition for it to simply do nothing.
Something like below?
Will
--->8
>From 4d9dcb9210c1f37435b6088284e04b6b36ee8c4d Mon Sep 17 00:00:00 2001
From: Will Deacon
Date: Fri, 2 Jul 2021 14:13:28 +0100
Subject: [PATCH] of: Return success from of_dma_set_restricted_buffer() when
!OF_ADDRESS
When CONFIG_OF_ADDRESS=n, of_dma_set_restricted_buffer() returns -ENODEV
and breaks the boot for sparc[64] machines. Return 0 instead, since the
function is essentially a glorified NOP in this configuration.
Cc: Claire Chang
Cc: Konrad Rzeszutek Wilk
Reported-by: Guenter Roeck
Suggested-by: Robin Murphy
Link: https://lore.kernel.org/r/20210702030807.ga2685...@roeck-us.net
Signed-off-by: Will Deacon
---
drivers/of/of_private.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 8fde97565d11..34dd548c5eac 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -173,7 +173,8 @@ static inline int of_dma_get_range(struct device_node *np,
static inline int of_dma_set_restricted_buffer(struct device *dev,
struct device_node *np)
{
- return -ENODEV;
+ /* Do nothing, successfully. */
+ return 0;
}
#endif
--
2.32.0.93.g670b81a890-goog
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Wed, Jun 30, 2021 at 08:56:51AM -0700, Nathan Chancellor wrote: > On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: > > On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > > > `BUG: unable to handle page fault for address: 003a8290` and > > > the fact it crashed at `_raw_spin_lock_irqsave` look like the memory > > > (maybe dev->dma_io_tlb_mem) was corrupted? > > > The dev->dma_io_tlb_mem should be set here > > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > > > through device_initialize. > > > > I'm less sure about this. 'dma_io_tlb_mem' should be pointing at > > 'io_tlb_default_mem', which is a page-aligned allocation from memblock. > > The spinlock is at offset 0x24 in that structure, and looking at the > > register dump from the crash: > > > > Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:adb4013db9e8 EFLAGS: 00010006 > > Jun 29 18:28:42 hp-4300G kernel: RAX: 003a8290 RBX: > > RCX: 8900572ad580 > > Jun 29 18:28:42 hp-4300G kernel: RDX: 89005653f024 RSI: > > 000c RDI: 1d17 > > Jun 29 18:28:42 hp-4300G kernel: RBP: 0a20d000 R08: > > 000c R09: > > Jun 29 18:28:42 hp-4300G kernel: R10: 0a20d000 R11: > > 89005653f000 R12: 0212 > > Jun 29 18:28:42 hp-4300G kernel: R13: 1000 R14: > > 0002 R15: 0020 > > Jun 29 18:28:42 hp-4300G kernel: FS: 7f1f8898ea40() > > GS:89005728() knlGS: > > Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: ES: CR0: > > 80050033 > > Jun 29 18:28:42 hp-4300G kernel: CR2: 003a8290 CR3: > > 0001020d CR4: 00350ee0 > > Jun 29 18:28:42 hp-4300G kernel: Call Trace: > > Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 > > Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 > > > > Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and > > RDX pointing at the spinlock. Yet RAX is holding junk :/ > > > > I agree that enabling KASAN would be a good idea, but I also think we > > probably need to get some more information out of swiotlb_tbl_map_single() > > to see see what exactly is going wrong in there. > > I can certainly enable KASAN and if there is any debug print I can add > or dump anything, let me know! I bit the bullet and took v5.13 with swiotlb/for-linus-5.14 merged in, built x86 defconfig and ran it on my laptop. However, it seems to work fine! Please can you share your .config? Will
Re: [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote:
> On Wed, Jun 30, 2021 at 9:43 AM Nathan Chancellor wrote:
> >
> > On Thu, Jun 24, 2021 at 11:55:20PM +0800, Claire Chang wrote:
> > > Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
> > > use it to determine whether to bounce the data or not. This will be
> > > useful later to allow for different pools.
> > >
> > > Signed-off-by: Claire Chang
> > > Reviewed-by: Christoph Hellwig
> > > Tested-by: Stefano Stabellini
> > > Tested-by: Will Deacon
> > > Acked-by: Stefano Stabellini
> >
> > This patch as commit af452ec1b1a3 ("swiotlb: Use is_swiotlb_force_bounce
> > for swiotlb data bouncing") causes my Ryzen 3 4300G system to fail to
> > get to an X session consistently (although not every single time),
> > presumably due to a crash in the AMDGPU driver that I see in dmesg.
> >
> > I have attached logs at af452ec1b1a3 and f127c9556a8e and I am happy
> > to provide any further information, debug, or test patches as necessary.
>
> Are you using swiotlb=force? or the swiotlb_map is called because of
> !dma_capable?
> (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/dma/direct.h#n93)
The command line is in the dmesg:
| Kernel command line: initrd=\amd-ucode.img
initrd=\initramfs-linux-next-llvm.img
root=PARTUUID=8680aa0c-cf09-4a69-8cf3-970478040ee7 rw intel_pstate=no_hwp
irqpoll
but I worry that this looks _very_ similar to the issue reported by Qian
Cai which we thought we had fixed. Nathan -- is the failure deterministic?
> `BUG: unable to handle page fault for address: 003a8290` and
> the fact it crashed at `_raw_spin_lock_irqsave` look like the memory
> (maybe dev->dma_io_tlb_mem) was corrupted?
> The dev->dma_io_tlb_mem should be set here
> (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528)
> through device_initialize.
I'm less sure about this. 'dma_io_tlb_mem' should be pointing at
'io_tlb_default_mem', which is a page-aligned allocation from memblock.
The spinlock is at offset 0x24 in that structure, and looking at the
register dump from the crash:
Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:adb4013db9e8 EFLAGS: 00010006
Jun 29 18:28:42 hp-4300G kernel: RAX: 003a8290 RBX:
RCX: 8900572ad580
Jun 29 18:28:42 hp-4300G kernel: RDX: 89005653f024 RSI: 000c
RDI: 1d17
Jun 29 18:28:42 hp-4300G kernel: RBP: 0a20d000 R08: 000c
R09:
Jun 29 18:28:42 hp-4300G kernel: R10: 0a20d000 R11: 89005653f000
R12: 0212
Jun 29 18:28:42 hp-4300G kernel: R13: 1000 R14: 0002
R15: 0020
Jun 29 18:28:42 hp-4300G kernel: FS: 7f1f8898ea40()
GS:89005728() knlGS:
Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: ES: CR0:
80050033
Jun 29 18:28:42 hp-4300G kernel: CR2: 003a8290 CR3: 0001020d
CR4: 00350ee0
Jun 29 18:28:42 hp-4300G kernel: Call Trace:
Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50
Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0
Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and
RDX pointing at the spinlock. Yet RAX is holding junk :/
I agree that enabling KASAN would be a good idea, but I also think we
probably need to get some more information out of swiotlb_tbl_map_single()
to see see what exactly is going wrong in there.
Will
Re: [PATCH v15 00/12] Restricted DMA
On Thu, Jun 24, 2021 at 03:19:48PM -0400, Konrad Rzeszutek Wilk wrote: > On Thu, Jun 24, 2021 at 11:55:14PM +0800, Claire Chang wrote: > > This series implements mitigations for lack of DMA access control on > > systems without an IOMMU, which could result in the DMA accessing the > > system memory at unexpected times and/or unexpected addresses, possibly > > leading to data leakage or corruption. > > > > For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is > > not behind an IOMMU. As PCI-e, by design, gives the device full access to > > system memory, a vulnerability in the Wi-Fi firmware could easily escalate > > to a full system exploit (remote wifi exploits: [1a], [1b] that shows a > > full chain of exploits; [2], [3]). > > > > To mitigate the security concerns, we introduce restricted DMA. Restricted > > DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a > > specially allocated region and does memory allocation from the same region. > > The feature on its own provides a basic level of protection against the DMA > > overwriting buffer contents at unexpected times. However, to protect > > against general data leakage and system memory corruption, the system needs > > to provide a way to restrict the DMA to a predefined memory region (this is > > usually done at firmware level, e.g. MPU in ATF on some ARM platforms [4]). > > > > [1a] > > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_4.html > > [1b] > > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_11.html > > [2] https://blade.tencent.com/en/advisories/qualpwn/ > > [3] > > https://www.bleepingcomputer.com/news/security/vulnerabilities-found-in-highly-popular-firmware-for-wifi-chips/ > > [4] > > https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132 > > > > v15: > > - Apply Will's diff > > (https://lore.kernel.org/patchwork/patch/1448957/#1647521) > > to fix the crash reported by Qian. > > - Add Stefano's Acked-by tag for patch 01/12 from v14 > > That all should be now be on > > https://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git/ > devel/for-linus-5.14 (and linux-next) Thanks Konrad! Will
Re: [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Thu, Jun 24, 2021 at 12:34:09PM +0100, Robin Murphy wrote:
> On 2021-06-24 12:18, Will Deacon wrote:
> > On Thu, Jun 24, 2021 at 12:14:39PM +0100, Robin Murphy wrote:
> > > On 2021-06-24 07:05, Claire Chang wrote:
> > > > On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig wrote:
> > > > >
> > > > > On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
> > > > > > is_swiotlb_force_bounce at
> > > > > > /usr/src/linux-next/./include/linux/swiotlb.h:119
> > > > > >
> > > > > > is_swiotlb_force_bounce() was the new function introduced in this
> > > > > > patch here.
> > > > > >
> > > > > > +static inline bool is_swiotlb_force_bounce(struct device *dev)
> > > > > > +{
> > > > > > + return dev->dma_io_tlb_mem->force_bounce;
> > > > > > +}
> > > > >
> > > > > To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
> > > > > turn this into :
> > > > >
> > > > > return dev->dma_io_tlb_mem &&
> > > > > dev->dma_io_tlb_mem->force_bounce;
> > > > >
> > > > > for a quick debug check?
> > > >
> > > > I just realized that dma_io_tlb_mem might be NULL like Christoph
> > > > pointed out since swiotlb might not get initialized.
> > > > However, `Unable to handle kernel paging request at virtual address
> > > > dfff800e` looks more like the address is garbage rather than
> > > > NULL?
> > > > I wonder if that's because dev->dma_io_tlb_mem is not assigned
> > > > properly (which means device_initialize is not called?).
> > >
> > > What also looks odd is that the base "address" 0xdfff8000 is held
> > > in
> > > a couple of registers, but the offset 0xe looks too small to match up to
> > > any
> > > relevant structure member in that dereference chain :/
> >
> > FWIW, I've managed to trigger a NULL dereference locally when swiotlb hasn't
> > been initialised but we dereference 'dev->dma_io_tlb_mem', so I think
> > Christoph's suggestion is needed regardless.
>
> Ack to that - for SWIOTLB_NO_FORCE, io_tlb_default_mem will remain NULL. The
> massive jump in KernelCI baseline failures as of yesterday looks like every
> arm64 machine with less than 4GB of RAM blowing up...
Ok, diff below which attempts to tackle the offset issue I mentioned as
well. Qian Cai -- please can you try with these changes?
Will
--->8
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 175b6c113ed8..39284ff2a6cd 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -116,7 +116,9 @@ static inline bool is_swiotlb_buffer(struct device *dev,
phys_addr_t paddr)
static inline bool is_swiotlb_force_bounce(struct device *dev)
{
- return dev->dma_io_tlb_mem->force_bounce;
+ struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
+
+ return mem && mem->force_bounce;
}
void __init swiotlb_exit(void);
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 44be8258e27b..0ffbaae9fba2 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -449,6 +449,7 @@ static int swiotlb_find_slots(struct device *dev,
phys_addr_t orig_addr,
dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
unsigned int nslots = nr_slots(alloc_size), stride;
unsigned int index, wrap, count = 0, i;
+ unsigned int offset = swiotlb_align_offset(dev, orig_addr);
unsigned long flags;
BUG_ON(!nslots);
@@ -497,7 +498,7 @@ static int swiotlb_find_slots(struct device *dev,
phys_addr_t orig_addr,
for (i = index; i < index + nslots; i++) {
mem->slots[i].list = 0;
mem->slots[i].alloc_size =
- alloc_size - ((i - index) << IO_TLB_SHIFT);
+ alloc_size - (offset + ((i - index) << IO_TLB_SHIFT));
}
for (i = index - 1;
io_tlb_offset(i) != IO_TLB_SEGSIZE - 1 &&
Re: [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Thu, Jun 24, 2021 at 12:14:39PM +0100, Robin Murphy wrote:
> On 2021-06-24 07:05, Claire Chang wrote:
> > On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig wrote:
> > >
> > > On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
> > > > is_swiotlb_force_bounce at
> > > > /usr/src/linux-next/./include/linux/swiotlb.h:119
> > > >
> > > > is_swiotlb_force_bounce() was the new function introduced in this patch
> > > > here.
> > > >
> > > > +static inline bool is_swiotlb_force_bounce(struct device *dev)
> > > > +{
> > > > + return dev->dma_io_tlb_mem->force_bounce;
> > > > +}
> > >
> > > To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
> > > turn this into :
> > >
> > > return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
> > >
> > > for a quick debug check?
> >
> > I just realized that dma_io_tlb_mem might be NULL like Christoph
> > pointed out since swiotlb might not get initialized.
> > However, `Unable to handle kernel paging request at virtual address
> > dfff800e` looks more like the address is garbage rather than
> > NULL?
> > I wonder if that's because dev->dma_io_tlb_mem is not assigned
> > properly (which means device_initialize is not called?).
>
> What also looks odd is that the base "address" 0xdfff8000 is held in
> a couple of registers, but the offset 0xe looks too small to match up to any
> relevant structure member in that dereference chain :/
FWIW, I've managed to trigger a NULL dereference locally when swiotlb hasn't
been initialised but we dereference 'dev->dma_io_tlb_mem', so I think
Christoph's suggestion is needed regardless. But I agree that it won't help
with the issue reported by Qian Cai.
Qian Cai: please can you share your .config and your command line?
Thanks,
Will
Re: [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Wed, Jun 23, 2021 at 12:39:29PM -0400, Qian Cai wrote: > > > On 6/18/2021 11:40 PM, Claire Chang wrote: > > Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and > > use it to determine whether to bounce the data or not. This will be > > useful later to allow for different pools. > > > > Signed-off-by: Claire Chang > > Reviewed-by: Christoph Hellwig > > Tested-by: Stefano Stabellini > > Tested-by: Will Deacon > > Acked-by: Stefano Stabellini > > Reverting the rest of the series up to this patch fixed a boot crash with > NVMe on today's linux-next. Hmm, so that makes patch 7 the suspicious one, right? Looking at that one more closely, it looks like swiotlb_find_slots() takes 'alloc_size + offset' as its 'alloc_size' parameter from swiotlb_tbl_map_single() and initialises 'mem->slots[i].alloc_size' based on 'alloc_size + offset', which looks like a change in behaviour from the old code, which didn't include the offset there. swiotlb_release_slots() then adds the offset back on afaict, so we end up accounting for it twice and possibly unmap more than we're supposed to? Will
Re: [PATCH v12 00/12] Restricted DMA
Hi Claire, On Wed, Jun 16, 2021 at 02:21:45PM +0800, Claire Chang wrote: > This series implements mitigations for lack of DMA access control on > systems without an IOMMU, which could result in the DMA accessing the > system memory at unexpected times and/or unexpected addresses, possibly > leading to data leakage or corruption. > > For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is > not behind an IOMMU. As PCI-e, by design, gives the device full access to > system memory, a vulnerability in the Wi-Fi firmware could easily escalate > to a full system exploit (remote wifi exploits: [1a], [1b] that shows a > full chain of exploits; [2], [3]). > > To mitigate the security concerns, we introduce restricted DMA. Restricted > DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a > specially allocated region and does memory allocation from the same region. > The feature on its own provides a basic level of protection against the DMA > overwriting buffer contents at unexpected times. However, to protect > against general data leakage and system memory corruption, the system needs > to provide a way to restrict the DMA to a predefined memory region (this is > usually done at firmware level, e.g. MPU in ATF on some ARM platforms [4]). > > [1a] > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_4.html > [1b] > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_11.html > [2] https://blade.tencent.com/en/advisories/qualpwn/ > [3] > https://www.bleepingcomputer.com/news/security/vulnerabilities-found-in-highly-popular-firmware-for-wifi-chips/ > [4] > https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132 > > v12: > Split is_dev_swiotlb_force into is_swiotlb_force_bounce (patch 06/12) and > is_swiotlb_for_alloc (patch 09/12) I took this for a spin in an arm64 KVM guest with virtio devices using the DMA API and it works as expected on top of swiotlb devel/for-linus-5.14, so: Tested-by: Will Deacon Thanks! Will
Re: [PATCH 1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c.
On Fri, 26 Mar 2021 16:13:02 -0700, Eric Anholt wrote: > db820c wants to use the qcom smmu path to get HUPCF set (which keeps > the GPU from wedging and then sometimes wedging the kernel after a > page fault), but it doesn't have separate pagetables support yet in > drm/msm so we can't go all the way to the TTBR1 path. Applied to will (for-joerg/arm-smmu/updates), thanks! [1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c. https://git.kernel.org/will/c/a242f4297cfe [2/2] arm64: dts: msm8996: Mark the GPU's SMMU as an adreno one. https://git.kernel.org/will/c/19c07b91f85d Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev
Re: [RESEND PATCH v4 1/6] iommu/arm-smmu: Add support for driver IOMMU fault handlers
On Wed, Jun 02, 2021 at 09:52:44AM -0700, Rob Clark wrote: > From: Jordan Crouse > > Call report_iommu_fault() to allow upper-level drivers to register their > own fault handlers. > > Signed-off-by: Jordan Crouse > Signed-off-by: Rob Clark > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c > b/drivers/iommu/arm/arm-smmu/arm-smmu.c > index 6f72c4d208ca..b4b32d31fc06 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > @@ -408,6 +408,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void > *dev) > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_device *smmu = smmu_domain->smmu; > int idx = smmu_domain->cfg.cbndx; > + int ret; > > fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR); > if (!(fsr & ARM_SMMU_FSR_FAULT)) > @@ -417,8 +418,12 @@ static irqreturn_t arm_smmu_context_fault(int irq, void > *dev) > iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR); > cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx)); > > - dev_err_ratelimited(smmu->dev, > - "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, > cbfrsynra=0x%x, cb=%d\n", > + ret = report_iommu_fault(domain, NULL, iova, > + fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : > IOMMU_FAULT_READ); > + > + if (ret == -ENOSYS) > + dev_err_ratelimited(smmu->dev, > + "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, > cbfrsynra=0x%x, cb=%d\n", > fsr, iova, fsynr, cbfrsynra, idx); Acked-by: Will Deacon Will
Re: [PATCH] arm64: cache: Lower ARCH_DMA_MINALIGN to 64 (L1_CACHE_BYTES)
[Adding VC4 folks -- please see the KASAN splat below!] Background here is that reducing ARCH_DMA_MINALIGN to 64 on arm64 (queued in -next) is causing vc4 to hang on Rpi3b due to a probable driver bug. Will On Mon, Jun 07, 2021 at 02:08:59PM +0100, Mark Rutland wrote: > On Mon, Jun 07, 2021 at 01:01:18PM +0100, Mark Rutland wrote: > > On Mon, Jun 07, 2021 at 11:58:32AM +0200, Marek Szyprowski wrote: > > > I've just checked with the latest firmware from > > > https://github.com/raspberrypi/firmware (master branch, just copied > > > everything to /boot) and the issue is still there. > > > > > > If you start from arm64/defconfig without modules, please make sure you > > > have enabled all RPi drivers, otherwise VC4 DRM won't come up. I've > > > managed to reproduce the issue without the modules with the following > > > changes to arm64's defconfig: > > > > > > ./scripts/config -e DRM -e DRM_VC4 -e CONFIG_CLK_RASPBERRYPI -e > > > CONFIG_SENSORS_RASPBERRYPI_HWMON -e CONFIG_I2C_BCM2835 -e > > > CONFIG_ARM_RASPBERRYPI_CPUFREQ > > > > Thanks for this! > > > > With that config on commit 65688d2a05deb9f0 I also see a hang at the end > > of boot, but before reaching userspace, with the last messages in dmesg > > as below. > > > > I'll go check that the ARCH_DMA_MINALIGN affects this, then I'll go play > > with debug options. > > I can confirm that with the ARCH_DMA_MINALIGN change reverted, the hang > goes away. Running with that reverted andwith KASAN, I get the > slab-out-of-bounds splat below, which occurs at the time the hang would > otherwise occur, and is possibly the problem: > > [3.609515] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops) > [3.621451] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops) > [3.628344] vc4-drm soc:gpu: bound 3f40.hvs (ops vc4_hvs_ops) > [3.635904] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops) > [3.643351] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops) > [3.651238] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops) > [3.659167] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops) > [3.666499] vc4-drm soc:gpu: bound 3fc0.v3d (ops vc4_v3d_ops) > [3.688560] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0 > [3.728010] > == > [3.728042] BUG: KASAN: slab-out-of-bounds in > vc4_atomic_commit_tail+0x1cc/0x910 > [3.728123] Read of size 8 at addr 07360440 by task kworker/u8:0/7 > [3.728153] > [3.728169] CPU: 2 PID: 7 Comm: kworker/u8:0 Not tainted > 5.13.0-rc3-9-g694c523e7267 #3 > [3.728203] Hardware name: Raspberry Pi 3 Model B (DT) > [3.728225] Workqueue: events_unbound deferred_probe_work_func > [3.728290] Call trace: > [3.728301] dump_backtrace+0x0/0x2b4 > [3.728358] show_stack+0x1c/0x30 > [3.728407] dump_stack+0xfc/0x168 > [3.728445] print_address_description.constprop.0+0x2c/0x2c0 > [3.728495] kasan_report+0x1dc/0x240 > [3.728529] __asan_load8+0x98/0xd4 > [3.728565] vc4_atomic_commit_tail+0x1cc/0x910 > [3.728621] commit_tail+0x100/0x210 > [3.728675] drm_atomic_helper_commit+0x1c4/0x3dc > [3.728730] drm_atomic_commit+0x80/0x94 > [3.728768] drm_client_modeset_commit_atomic+0x2f4/0x3a0 > [3.728821] drm_client_modeset_commit_locked+0x8c/0x230 > [3.728872] drm_fb_helper_pan_display+0x164/0x3a0 > [3.728924] fb_pan_display+0x12c/0x1fc > [3.728963] bit_update_start+0x34/0xa0 > [3.729013] fbcon_switch+0x678/0x920 > [3.729058] redraw_screen+0x17c/0x35c > [3.729095] fbcon_prepare_logo+0x484/0x5bc > [3.729143] fbcon_init+0x77c/0x970 > [3.729187] visual_init+0x14c/0x1e4 > [3.729239] do_bind_con_driver.isra.0+0x2c4/0x530 > [3.729279] do_take_over_console+0x200/0x2e0 > [3.729317] do_fbcon_takeover+0x90/0x120 > [3.729363] fbcon_fb_registered+0x14c/0x164 > [3.729412] register_framebuffer+0x308/0x4e0 > [3.729451] __drm_fb_helper_initial_config_and_unlock+0x538/0x7d0 > [3.729506] drm_fbdev_client_hotplug+0x204/0x374 > [3.729556] drm_fbdev_generic_setup+0xf4/0x24c > [3.729604] vc4_drm_bind+0x1d4/0x1f0 > [3.729654] try_to_bring_up_master+0x254/0x2dc > [3.729709] __component_add+0x10c/0x240 > [3.729759] component_add+0x18/0x24 > [3.729807] vc4_v3d_dev_probe+0x20/0x30 > [3.729854] platform_probe+0x90/0x110 > [3.729907] really_probe+0x148/0x744 > [3.729952] driver_probe_device+0x8c/0xfc > [3.729998] __device_attach_driver+0x120/0x180 > [3.730048] bus_for_each_drv+0xf4/0x15c > [3.730091] __device_attach+0x168/0x250 > [3.730137] device_initial_probe+0x18/0x24 > [3.730186] bus_probe_device+0xec/0x100 > [3.730230] deferred_probe_work_func+0xe8/0x130 > [3.730279] process_one_work+0x3b8/0x650 > [3.730319] worker_thread+0x3cc/0x72c > [3.730356] kthread+0x21c/0x224
Re: [PATCH v8 00/15] Restricted DMA
Hi Claire, On Thu, May 27, 2021 at 08:58:30PM +0800, Claire Chang wrote: > This series implements mitigations for lack of DMA access control on > systems without an IOMMU, which could result in the DMA accessing the > system memory at unexpected times and/or unexpected addresses, possibly > leading to data leakage or corruption. > > For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is > not behind an IOMMU. As PCI-e, by design, gives the device full access to > system memory, a vulnerability in the Wi-Fi firmware could easily escalate > to a full system exploit (remote wifi exploits: [1a], [1b] that shows a > full chain of exploits; [2], [3]). > > To mitigate the security concerns, we introduce restricted DMA. Restricted > DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a > specially allocated region and does memory allocation from the same region. > The feature on its own provides a basic level of protection against the DMA > overwriting buffer contents at unexpected times. However, to protect > against general data leakage and system memory corruption, the system needs > to provide a way to restrict the DMA to a predefined memory region (this is > usually done at firmware level, e.g. MPU in ATF on some ARM platforms [4]). > > [1a] > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_4.html > [1b] > https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_11.html > [2] https://blade.tencent.com/en/advisories/qualpwn/ > [3] > https://www.bleepingcomputer.com/news/security/vulnerabilities-found-in-highly-popular-firmware-for-wifi-chips/ > [4] > https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132 > > v8: > - Fix reserved-memory.txt and add the reg property in example. > - Fix sizeof for of_property_count_elems_of_size in > drivers/of/address.c#of_dma_set_restricted_buffer. > - Apply Will's suggestion to try the OF node having DMA configuration in > drivers/of/address.c#of_dma_set_restricted_buffer. > - Fix typo in the comment of > drivers/of/address.c#of_dma_set_restricted_buffer. > - Add error message for PageHighMem in > kernel/dma/swiotlb.c#rmem_swiotlb_device_init and move it to > rmem_swiotlb_setup. > - Fix the message string in rmem_swiotlb_setup. Thanks for the v8. It works for me out of the box on arm64 under KVM, so: Tested-by: Will Deacon Note that something seems to have gone wrong with the mail threading, so the last 5 patches ended up as a separate thread for me. Probably worth posting again with all the patches in one place, if you can. Cheers, Will
Re: [PATCH v7 14/15] dt-bindings: of: Add restricted DMA pool
On Thu, May 27, 2021 at 08:48:59PM +0800, Claire Chang wrote:
> On Thu, May 27, 2021 at 7:35 PM Will Deacon wrote:
> >
> > On Thu, May 27, 2021 at 07:29:20PM +0800, Claire Chang wrote:
> > > On Wed, May 26, 2021 at 11:53 PM Will Deacon wrote:
> > > >
> > > > On Wed, May 26, 2021 at 01:13:22PM +0100, Will Deacon wrote:
> > > > > On Tue, May 18, 2021 at 02:42:14PM +0800, Claire Chang wrote:
> > > > > > @@ -138,4 +160,9 @@ one for multimedia processing (named
> > > > > > multimedia-memory@7700, 64MiB).
> > > > > > memory-region = <&multimedia_reserved>;
> > > > > > /* ... */
> > > > > > };
> > > > > > +
> > > > > > + pcie_device: pcie_device@0,0 {
> > > > > > + memory-region = <&restricted_dma_mem_reserved>;
> > > > > > + /* ... */
> > > > > > + };
> > > > >
> > > > > I still don't understand how this works for individual PCIe devices
> > > > > -- how
> > > > > is dev->of_node set to point at the node you have above?
> > > > >
> > > > > I tried adding the memory-region to the host controller instead, and
> > > > > then
> > > > > I see it crop up in dmesg:
> > > > >
> > > > > | pci-host-generic 4000.pci: assigned reserved memory node
> > > > > restricted_dma_mem_reserved
> > > > >
> > > > > but none of the actual PCI devices end up with 'dma_io_tlb_mem' set,
> > > > > and
> > > > > so the restricted DMA area is not used. In fact, swiotlb isn't used
> > > > > at all.
> > > > >
> > > > > What am I missing to make this work with PCIe devices?
> > > >
> > > > Aha, looks like we're just missing the logic to inherit the DMA
> > > > configuration. The diff below gets things working for me.
> > >
> > > I guess what was missing is the reg property in the pcie_device node.
> > > Will update the example dts.
> >
> > Thanks. I still think something like my diff makes sense, if you wouldn't
> > mind including
> > it, as it allows restricted DMA to be used for situations where the PCIe
> > topology is not static.
> >
> > Perhaps we should prefer dev->of_node if it exists, but then use the node
> > of the host bridge's parent node otherwise?
>
> Sure. Let me add in the next version.
Brill, thanks! I'll take it for a spin once it lands on the list.
Will
Re: [PATCH v7 14/15] dt-bindings: of: Add restricted DMA pool
On Thu, May 27, 2021 at 07:29:20PM +0800, Claire Chang wrote:
> On Wed, May 26, 2021 at 11:53 PM Will Deacon wrote:
> >
> > On Wed, May 26, 2021 at 01:13:22PM +0100, Will Deacon wrote:
> > > On Tue, May 18, 2021 at 02:42:14PM +0800, Claire Chang wrote:
> > > > @@ -138,4 +160,9 @@ one for multimedia processing (named
> > > > multimedia-memory@7700, 64MiB).
> > > > memory-region = <&multimedia_reserved>;
> > > > /* ... */
> > > > };
> > > > +
> > > > + pcie_device: pcie_device@0,0 {
> > > > + memory-region = <&restricted_dma_mem_reserved>;
> > > > + /* ... */
> > > > + };
> > >
> > > I still don't understand how this works for individual PCIe devices -- how
> > > is dev->of_node set to point at the node you have above?
> > >
> > > I tried adding the memory-region to the host controller instead, and then
> > > I see it crop up in dmesg:
> > >
> > > | pci-host-generic 4000.pci: assigned reserved memory node
> > > restricted_dma_mem_reserved
> > >
> > > but none of the actual PCI devices end up with 'dma_io_tlb_mem' set, and
> > > so the restricted DMA area is not used. In fact, swiotlb isn't used at
> > > all.
> > >
> > > What am I missing to make this work with PCIe devices?
> >
> > Aha, looks like we're just missing the logic to inherit the DMA
> > configuration. The diff below gets things working for me.
>
> I guess what was missing is the reg property in the pcie_device node.
> Will update the example dts.
Thanks. I still think something like my diff makes sense, if you wouldn't mind
including
it, as it allows restricted DMA to be used for situations where the PCIe
topology is not static.
Perhaps we should prefer dev->of_node if it exists, but then use the node
of the host bridge's parent node otherwise?
Will
Re: [PATCH v7 14/15] dt-bindings: of: Add restricted DMA pool
On Wed, May 26, 2021 at 01:13:22PM +0100, Will Deacon wrote:
> On Tue, May 18, 2021 at 02:42:14PM +0800, Claire Chang wrote:
> > @@ -138,4 +160,9 @@ one for multimedia processing (named
> > multimedia-memory@7700, 64MiB).
> > memory-region = <&multimedia_reserved>;
> > /* ... */
> > };
> > +
> > + pcie_device: pcie_device@0,0 {
> > + memory-region = <&restricted_dma_mem_reserved>;
> > + /* ... */
> > + };
>
> I still don't understand how this works for individual PCIe devices -- how
> is dev->of_node set to point at the node you have above?
>
> I tried adding the memory-region to the host controller instead, and then
> I see it crop up in dmesg:
>
> | pci-host-generic 4000.pci: assigned reserved memory node
> restricted_dma_mem_reserved
>
> but none of the actual PCI devices end up with 'dma_io_tlb_mem' set, and
> so the restricted DMA area is not used. In fact, swiotlb isn't used at all.
>
> What am I missing to make this work with PCIe devices?
Aha, looks like we're just missing the logic to inherit the DMA
configuration. The diff below gets things working for me.
Will
--->8
diff --git a/drivers/of/address.c b/drivers/of/address.c
index c562a9ff5f0b..bf499fdd6e93 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -1113,25 +1113,25 @@ bool of_dma_is_coherent(struct device_node *np)
}
EXPORT_SYMBOL_GPL(of_dma_is_coherent);
-int of_dma_set_restricted_buffer(struct device *dev)
+int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
{
- struct device_node *node;
int count, i;
- if (!dev->of_node)
+ if (!np)
return 0;
- count = of_property_count_elems_of_size(dev->of_node, "memory-region",
+ count = of_property_count_elems_of_size(np, "memory-region",
sizeof(phandle));
for (i = 0; i < count; i++) {
- node = of_parse_phandle(dev->of_node, "memory-region", i);
+ struct device_node *node;
+
+ node = of_parse_phandle(np, "memory-region", i);
/* There might be multiple memory regions, but only one
-* restriced-dma-pool region is allowed.
+* restricted-dma-pool region is allowed.
*/
if (of_device_is_compatible(node, "restricted-dma-pool") &&
of_device_is_available(node))
- return of_reserved_mem_device_init_by_idx(
- dev, dev->of_node, i);
+ return of_reserved_mem_device_init_by_idx(dev, np, i);
}
return 0;
diff --git a/drivers/of/device.c b/drivers/of/device.c
index d8d865223e51..2defdca418ec 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -166,7 +166,7 @@ int of_dma_configure_id(struct device *dev, struct
device_node *np,
arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
if (!iommu)
- return of_dma_set_restricted_buffer(dev);
+ return of_dma_set_restricted_buffer(dev, np);
return 0;
}
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 9fc874548528..8fde97565d11 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -163,14 +163,15 @@ struct bus_dma_region;
#if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map);
-int of_dma_set_restricted_buffer(struct device *dev);
+int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
#else
static inline int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map)
{
return -ENODEV;
}
-static inline int of_dma_set_restricted_buffer(struct device *dev)
+static inline int of_dma_set_restricted_buffer(struct device *dev,
+ struct device_node *np)
{
return -ENODEV;
}
Re: [PATCH v7 14/15] dt-bindings: of: Add restricted DMA pool
Hi Claire,
On Tue, May 18, 2021 at 02:42:14PM +0800, Claire Chang wrote:
> Introduce the new compatible string, restricted-dma-pool, for restricted
> DMA. One can specify the address and length of the restricted DMA memory
> region by restricted-dma-pool in the reserved-memory node.
>
> Signed-off-by: Claire Chang
> ---
> .../reserved-memory/reserved-memory.txt | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> index e8d3096d922c..284aea659015 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> @@ -51,6 +51,23 @@ compatible (optional) - standard definition
>used as a shared pool of DMA buffers for a set of devices. It can
>be used by an operating system to instantiate the necessary pool
>management subsystem if necessary.
> +- restricted-dma-pool: This indicates a region of memory meant to be
> + used as a pool of restricted DMA buffers for a set of devices. The
> + memory region would be the only region accessible to those devices.
> + When using this, the no-map and reusable properties must not be
> set,
> + so the operating system can create a virtual mapping that will be
> used
> + for synchronization. The main purpose for restricted DMA is to
> + mitigate the lack of DMA access control on systems without an
> IOMMU,
> + which could result in the DMA accessing the system memory at
> + unexpected times and/or unexpected addresses, possibly leading to
> data
> + leakage or corruption. The feature on its own provides a basic
> level
> + of protection against the DMA overwriting buffer contents at
> + unexpected times. However, to protect against general data leakage
> and
> + system memory corruption, the system needs to provide way to lock
> down
> + the memory access, e.g., MPU. Note that since coherent allocation
> + needs remapping, one must set up another device coherent pool by
> + shared-dma-pool and use dma_alloc_from_dev_coherent instead for
> atomic
> + coherent allocation.
> - vendor specific string in the form ,[-]
> no-map (optional) - empty property
> - Indicates the operating system must not create a virtual mapping
> @@ -120,6 +137,11 @@ one for multimedia processing (named
> multimedia-memory@7700, 64MiB).
> compatible = "acme,multimedia-memory";
> reg = <0x7700 0x400>;
> };
> +
> + restricted_dma_mem_reserved: restricted_dma_mem_reserved {
> + compatible = "restricted-dma-pool";
> + reg = <0x5000 0x40>;
> + };
nit: You need to update the old text that states "This example defines 3
contiguous regions ...".
> };
>
> /* ... */
> @@ -138,4 +160,9 @@ one for multimedia processing (named
> multimedia-memory@7700, 64MiB).
> memory-region = <&multimedia_reserved>;
> /* ... */
> };
> +
> + pcie_device: pcie_device@0,0 {
> + memory-region = <&restricted_dma_mem_reserved>;
> + /* ... */
> + };
I still don't understand how this works for individual PCIe devices -- how
is dev->of_node set to point at the node you have above?
I tried adding the memory-region to the host controller instead, and then
I see it crop up in dmesg:
| pci-host-generic 4000.pci: assigned reserved memory node
restricted_dma_mem_reserved
but none of the actual PCI devices end up with 'dma_io_tlb_mem' set, and
so the restricted DMA area is not used. In fact, swiotlb isn't used at all.
What am I missing to make this work with PCIe devices?
Thanks,
Will
Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
On Thu, Apr 01, 2021 at 11:59:45AM +0200, Christoph Hellwig wrote: > For now I'll just pass the iommu_domain to iommu_get_dma_strict, > so that we can check for it. We can do additional cleanups on top > of that later. Sounds good to me, cheers! Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
On Wed, Mar 31, 2021 at 02:09:37PM +0100, Robin Murphy wrote: > On 2021-03-31 12:49, Will Deacon wrote: > > On Tue, Mar 30, 2021 at 05:28:19PM +0100, Robin Murphy wrote: > > > On 2021-03-30 14:58, Will Deacon wrote: > > > > On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote: > > > > > On 2021-03-30 14:11, Will Deacon wrote: > > > > > > On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote: > > > > > > > From: Robin Murphy > > > > > > > > > > > > > > Instead make the global iommu_dma_strict paramete in iommu.c > > > > > > > canonical by > > > > > > > exporting helpers to get and set it and use those directly in the > > > > > > > drivers. > > > > > > > > > > > > > > This make sure that the iommu.strict parameter also works for the > > > > > > > AMD and > > > > > > > Intel IOMMU drivers on x86. As those default to lazy flushing a > > > > > > > new > > > > > > > IOMMU_CMD_LINE_STRICT is used to turn the value into a tristate to > > > > > > > represent the default if not overriden by an explicit parameter. > > > > > > > > > > > > > > Signed-off-by: Robin Murphy . > > > > > > > [ported on top of the other iommu_attr changes and added a few > > > > > > > small > > > > > > > missing bits] > > > > > > > Signed-off-by: Christoph Hellwig > > > > > > > --- > > > > > > > drivers/iommu/amd/iommu.c | 23 +--- > > > > > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 > > > > > > > +--- > > > > > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > > > > > > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 27 + > > > > > > > drivers/iommu/dma-iommu.c | 9 +-- > > > > > > > drivers/iommu/intel/iommu.c | 64 > > > > > > > - > > > > > > > drivers/iommu/iommu.c | 27 ++--- > > > > > > > include/linux/iommu.h | 4 +- > > > > > > > 8 files changed, 40 insertions(+), 165 deletions(-) > > > > > > > > > > > > I really like this cleanup, but I can't help wonder if it's going > > > > > > in the > > > > > > wrong direction. With SoCs often having multiple IOMMU instances > > > > > > and a > > > > > > distinction between "trusted" and "untrusted" devices, then having > > > > > > the > > > > > > flush-queue enabled on a per-IOMMU or per-domain basis doesn't sound > > > > > > unreasonable to me, but this change makes it a global property. > > > > > > > > > > The intent here was just to streamline the existing behaviour of > > > > > stuffing a > > > > > global property into a domain attribute then pulling it out again in > > > > > the > > > > > illusion that it was in any way per-domain. We're still checking > > > > > dev_is_untrusted() before making an actual decision, and it's not > > > > > like we > > > > > can't add more factors at that point if we want to. > > > > > > > > Like I say, the cleanup is great. I'm just wondering whether there's a > > > > better way to express the complicated logic to decide whether or not to > > > > use > > > > the flush queue than what we end up with: > > > > > > > > if (!cookie->fq_domain && (!dev || !dev_is_untrusted(dev)) && > > > > domain->ops->flush_iotlb_all && !iommu_get_dma_strict()) > > > > > > > > which is mixing up globals, device properties and domain properties. The > > > > result is that the driver code ends up just using the global to > > > > determine > > > > whether or not to pass IO_PGTABLE_QUIRK_NON_STRICT to the page-table > > > > code, > > > > which is a departure from the current way of doing things. > > > > > > But previously, SMMU only ever saw th
Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
On Tue, Mar 30, 2021 at 05:28:19PM +0100, Robin Murphy wrote: > On 2021-03-30 14:58, Will Deacon wrote: > > On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote: > > > On 2021-03-30 14:11, Will Deacon wrote: > > > > On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote: > > > > > From: Robin Murphy > > > > > > > > > > Instead make the global iommu_dma_strict paramete in iommu.c > > > > > canonical by > > > > > exporting helpers to get and set it and use those directly in the > > > > > drivers. > > > > > > > > > > This make sure that the iommu.strict parameter also works for the AMD > > > > > and > > > > > Intel IOMMU drivers on x86. As those default to lazy flushing a new > > > > > IOMMU_CMD_LINE_STRICT is used to turn the value into a tristate to > > > > > represent the default if not overriden by an explicit parameter. > > > > > > > > > > Signed-off-by: Robin Murphy . > > > > > [ported on top of the other iommu_attr changes and added a few small > > > > >missing bits] > > > > > Signed-off-by: Christoph Hellwig > > > > > --- > > > > >drivers/iommu/amd/iommu.c | 23 +--- > > > > >drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +--- > > > > >drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > > > > >drivers/iommu/arm/arm-smmu/arm-smmu.c | 27 + > > > > >drivers/iommu/dma-iommu.c | 9 +-- > > > > >drivers/iommu/intel/iommu.c | 64 > > > > > - > > > > >drivers/iommu/iommu.c | 27 ++--- > > > > >include/linux/iommu.h | 4 +- > > > > >8 files changed, 40 insertions(+), 165 deletions(-) > > > > > > > > I really like this cleanup, but I can't help wonder if it's going in the > > > > wrong direction. With SoCs often having multiple IOMMU instances and a > > > > distinction between "trusted" and "untrusted" devices, then having the > > > > flush-queue enabled on a per-IOMMU or per-domain basis doesn't sound > > > > unreasonable to me, but this change makes it a global property. > > > > > > The intent here was just to streamline the existing behaviour of stuffing > > > a > > > global property into a domain attribute then pulling it out again in the > > > illusion that it was in any way per-domain. We're still checking > > > dev_is_untrusted() before making an actual decision, and it's not like we > > > can't add more factors at that point if we want to. > > > > Like I say, the cleanup is great. I'm just wondering whether there's a > > better way to express the complicated logic to decide whether or not to use > > the flush queue than what we end up with: > > > > if (!cookie->fq_domain && (!dev || !dev_is_untrusted(dev)) && > > domain->ops->flush_iotlb_all && !iommu_get_dma_strict()) > > > > which is mixing up globals, device properties and domain properties. The > > result is that the driver code ends up just using the global to determine > > whether or not to pass IO_PGTABLE_QUIRK_NON_STRICT to the page-table code, > > which is a departure from the current way of doing things. > > But previously, SMMU only ever saw the global policy piped through the > domain attribute by iommu_group_alloc_default_domain(), so there's no > functional change there. For DMA domains sure, but I don't think that's the case for unmanaged domains such as those used by VFIO. > Obviously some of the above checks could be factored out into some kind of > iommu_use_flush_queue() helper that IOMMU drivers can also call if they need > to keep in sync. Or maybe we just allow iommu-dma to set > IO_PGTABLE_QUIRK_NON_STRICT directly via iommu_set_pgtable_quirks() if we're > treating that as a generic thing now. I think a helper that takes a domain would be a good starting point. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c.
On Tue, Mar 30, 2021 at 08:03:36AM -0700, Rob Clark wrote: > On Tue, Mar 30, 2021 at 2:34 AM Will Deacon wrote: > > > > On Mon, Mar 29, 2021 at 09:02:50PM -0700, Rob Clark wrote: > > > On Mon, Mar 29, 2021 at 7:47 AM Will Deacon wrote: > > > > > > > > On Fri, Mar 26, 2021 at 04:13:02PM -0700, Eric Anholt wrote: > > > > > db820c wants to use the qcom smmu path to get HUPCF set (which keeps > > > > > the GPU from wedging and then sometimes wedging the kernel after a > > > > > page fault), but it doesn't have separate pagetables support yet in > > > > > drm/msm so we can't go all the way to the TTBR1 path. > > > > > > > > What do you mean by "doesn't have separate pagetables support yet"? The > > > > compatible string doesn't feel like the right way to determine this. > > > > > > the compatible string identifies what it is, not what the sw > > > limitations are, so in that regard it seems right to me.. > > > > Well it depends on what "doesn't have separate pagetables support yet" > > means. I can't tell if it's a hardware issue, a firmware issue or a driver > > issue. > > Just a driver issue (and the fact that currently we don't have > physical access to a device... debugging a5xx per-process-pgtables by > pushing untested things to the CI farm is kind of a difficult way to > work) But then in that case, this is using the compatible string to identify a driver issue, no? Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote: > On 2021-03-30 14:11, Will Deacon wrote: > > On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote: > > > From: Robin Murphy > > > > > > Instead make the global iommu_dma_strict paramete in iommu.c canonical by > > > exporting helpers to get and set it and use those directly in the drivers. > > > > > > This make sure that the iommu.strict parameter also works for the AMD and > > > Intel IOMMU drivers on x86. As those default to lazy flushing a new > > > IOMMU_CMD_LINE_STRICT is used to turn the value into a tristate to > > > represent the default if not overriden by an explicit parameter. > > > > > > Signed-off-by: Robin Murphy . > > > [ported on top of the other iommu_attr changes and added a few small > > > missing bits] > > > Signed-off-by: Christoph Hellwig > > > --- > > > drivers/iommu/amd/iommu.c | 23 +--- > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +--- > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 27 + > > > drivers/iommu/dma-iommu.c | 9 +-- > > > drivers/iommu/intel/iommu.c | 64 - > > > drivers/iommu/iommu.c | 27 ++--- > > > include/linux/iommu.h | 4 +- > > > 8 files changed, 40 insertions(+), 165 deletions(-) > > > > I really like this cleanup, but I can't help wonder if it's going in the > > wrong direction. With SoCs often having multiple IOMMU instances and a > > distinction between "trusted" and "untrusted" devices, then having the > > flush-queue enabled on a per-IOMMU or per-domain basis doesn't sound > > unreasonable to me, but this change makes it a global property. > > The intent here was just to streamline the existing behaviour of stuffing a > global property into a domain attribute then pulling it out again in the > illusion that it was in any way per-domain. We're still checking > dev_is_untrusted() before making an actual decision, and it's not like we > can't add more factors at that point if we want to. Like I say, the cleanup is great. I'm just wondering whether there's a better way to express the complicated logic to decide whether or not to use the flush queue than what we end up with: if (!cookie->fq_domain && (!dev || !dev_is_untrusted(dev)) && domain->ops->flush_iotlb_all && !iommu_get_dma_strict()) which is mixing up globals, device properties and domain properties. The result is that the driver code ends up just using the global to determine whether or not to pass IO_PGTABLE_QUIRK_NON_STRICT to the page-table code, which is a departure from the current way of doing things. > > For example, see the recent patch from Lu Baolu: > > > > https://lore.kernel.org/r/20210225061454.2864009-1-baolu...@linux.intel.com > > Erm, this patch is based on that one, it's right there in the context :/ Ah, sorry, I didn't spot that! I was just trying to illustrate that this is per-device. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 18/18] iommu: remove iommu_domain_{get,set}_attr
On Tue, Mar 16, 2021 at 04:38:24PM +0100, Christoph Hellwig wrote: > Remove the now unused iommu attr infrastructure. > > Signed-off-by: Christoph Hellwig > --- > drivers/iommu/iommu.c | 26 -- > include/linux/iommu.h | 36 > 2 files changed, 62 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 17/18] iommu: remove DOMAIN_ATTR_IO_PGTABLE_CFG
On Tue, Mar 16, 2021 at 04:38:23PM +0100, Christoph Hellwig wrote: > Use an explicit set_pgtable_quirks method instead that just passes > the actual quirk bitmask instead. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 +- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 64 + > drivers/iommu/arm/arm-smmu/arm-smmu.h | 2 +- > drivers/iommu/iommu.c | 11 + > include/linux/io-pgtable.h | 4 -- > include/linux/iommu.h | 12 - > 6 files changed, 35 insertions(+), 63 deletions(-) I'm fine with this for now, although there has been talk about passing things other than boolean flags as page-table quirks. We can cross that bridge when we get there, so: Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote: > From: Robin Murphy > > Instead make the global iommu_dma_strict paramete in iommu.c canonical by > exporting helpers to get and set it and use those directly in the drivers. > > This make sure that the iommu.strict parameter also works for the AMD and > Intel IOMMU drivers on x86. As those default to lazy flushing a new > IOMMU_CMD_LINE_STRICT is used to turn the value into a tristate to > represent the default if not overriden by an explicit parameter. > > Signed-off-by: Robin Murphy . > [ported on top of the other iommu_attr changes and added a few small > missing bits] > Signed-off-by: Christoph Hellwig > --- > drivers/iommu/amd/iommu.c | 23 +--- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +--- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > drivers/iommu/arm/arm-smmu/arm-smmu.c | 27 + > drivers/iommu/dma-iommu.c | 9 +-- > drivers/iommu/intel/iommu.c | 64 - > drivers/iommu/iommu.c | 27 ++--- > include/linux/iommu.h | 4 +- > 8 files changed, 40 insertions(+), 165 deletions(-) I really like this cleanup, but I can't help wonder if it's going in the wrong direction. With SoCs often having multiple IOMMU instances and a distinction between "trusted" and "untrusted" devices, then having the flush-queue enabled on a per-IOMMU or per-domain basis doesn't sound unreasonable to me, but this change makes it a global property. For example, see the recent patch from Lu Baolu: https://lore.kernel.org/r/20210225061454.2864009-1-baolu...@linux.intel.com Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 15/18] iommu: remove iommu_set_cmd_line_dma_api and iommu_cmd_line_dma_api
On Tue, Mar 16, 2021 at 04:38:21PM +0100, Christoph Hellwig wrote: > Don't obsfucate the trivial bit flag check. > > Signed-off-by: Christoph Hellwig > --- > drivers/iommu/iommu.c | 23 +-- > 1 file changed, 5 insertions(+), 18 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 14/18] iommu: remove DOMAIN_ATTR_NESTING
On Tue, Mar 16, 2021 at 04:38:20PM +0100, Christoph Hellwig wrote: > Use an explicit enable_nesting method instead. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 43 - > drivers/iommu/arm/arm-smmu/arm-smmu.c | 30 +++--- > drivers/iommu/intel/iommu.c | 31 +-- > drivers/iommu/iommu.c | 10 + > drivers/vfio/vfio_iommu_type1.c | 5 +-- > include/linux/iommu.h | 4 +- > 6 files changed, 55 insertions(+), 68 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 13/18] iommu: remove DOMAIN_ATTR_GEOMETRY
On Tue, Mar 16, 2021 at 04:38:19PM +0100, Christoph Hellwig wrote: > The geometry information can be trivially queried from the iommu_domain > struture. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/iommu.c | 20 +++- > drivers/vfio/vfio_iommu_type1.c | 26 -- > drivers/vhost/vdpa.c| 10 +++--- > include/linux/iommu.h | 1 - > 4 files changed, 18 insertions(+), 39 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 12/18] iommu: remove DOMAIN_ATTR_PAGING
On Tue, Mar 16, 2021 at 04:38:18PM +0100, Christoph Hellwig wrote: > DOMAIN_ATTR_PAGING is never used. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/iommu.c | 5 - > include/linux/iommu.h | 1 - > 2 files changed, 6 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 11/18] iommu/fsl_pamu: remove the snoop_id field
On Tue, Mar 16, 2021 at 04:38:17PM +0100, Christoph Hellwig wrote: > The snoop_id is always set to ~(u32)0. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/fsl_pamu_domain.c | 5 ++--- > drivers/iommu/fsl_pamu_domain.h | 1 - > 2 files changed, 2 insertions(+), 4 deletions(-) pamu_config_ppaace() takes quite a few useless parameters at this stage, but anyway: Acked-by: Will Deacon Do you know if this driver is actually useful? Once the complexity has been stripped back, the stubs and default values really stand out. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 10/18] iommu/fsl_pamu: enable the liodn when attaching a device
On Tue, Mar 16, 2021 at 04:38:16PM +0100, Christoph Hellwig wrote: > Instead of a separate call to enable all devices from the list, just > enablde the liodn one the device is attached to the iommu domain. (typos: "enablde" and "one" probably needs to be "once"?) > This also remove the DOMAIN_ATTR_FSL_PAMU_ENABLE iommu_attr. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/fsl_pamu_domain.c | 47 ++--- > drivers/iommu/fsl_pamu_domain.h | 10 -- > drivers/soc/fsl/qbman/qman_portal.c | 11 --- > include/linux/iommu.h | 1 - > 4 files changed, 3 insertions(+), 66 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/18] iommu/fsl_pamu: merge handle_attach_device into fsl_pamu_attach_device
On Tue, Mar 16, 2021 at 04:38:15PM +0100, Christoph Hellwig wrote: > No good reason to split this functionality over two functions. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/fsl_pamu_domain.c | 59 +++-- > 1 file changed, 20 insertions(+), 39 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/18] iommu/fsl_pamu: merge pamu_set_liodn and map_liodn
On Tue, Mar 16, 2021 at 04:38:14PM +0100, Christoph Hellwig wrote:
> Merge the two fuctions that configure the ppaace into a single coherent
> function. I somehow doubt we need the two pamu_config_ppaace calls,
> but keep the existing behavior just to be on the safe side.
>
> Signed-off-by: Christoph Hellwig
> Acked-by: Li Yang
> ---
> drivers/iommu/fsl_pamu_domain.c | 65 +
> 1 file changed, 17 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index 40eff4b7bc5d42..4a4944332674f7 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -54,25 +54,6 @@ static int __init iommu_init_mempool(void)
> return 0;
> }
>
> -/* Map the DMA window corresponding to the LIODN */
> -static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
> -{
> - int ret;
> - struct iommu_domain_geometry *geom = &dma_domain->iommu_domain.geometry;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&iommu_lock, flags);
> - ret = pamu_config_ppaace(liodn, geom->aperture_start,
> - geom->aperture_end - 1, ~(u32)0,
> - 0, dma_domain->snoop_id, dma_domain->stash_id,
> - PAACE_AP_PERMS_QUERY | PAACE_AP_PERMS_UPDATE);
> - spin_unlock_irqrestore(&iommu_lock, flags);
> - if (ret)
> - pr_debug("PAACE configuration failed for liodn %d\n", liodn);
> -
> - return ret;
> -}
> -
> static int update_liodn_stash(int liodn, struct fsl_dma_domain *dma_domain,
> u32 val)
> {
> @@ -94,11 +75,11 @@ static int update_liodn_stash(int liodn, struct
> fsl_dma_domain *dma_domain,
> }
>
> /* Set the geometry parameters for a LIODN */
> -static int pamu_set_liodn(int liodn, struct device *dev,
> - struct fsl_dma_domain *dma_domain,
> - struct iommu_domain_geometry *geom_attr)
> +static int pamu_set_liodn(struct fsl_dma_domain *dma_domain, struct device
> *dev,
> + int liodn)
> {
> - phys_addr_t window_addr, window_size;
> + struct iommu_domain *domain = &dma_domain->iommu_domain;
> + struct iommu_domain_geometry *geom = &domain->geometry;
> u32 omi_index = ~(u32)0;
> unsigned long flags;
> int ret;
> @@ -110,22 +91,25 @@ static int pamu_set_liodn(int liodn, struct device *dev,
>*/
> get_ome_index(&omi_index, dev);
>
> - window_addr = geom_attr->aperture_start;
> - window_size = geom_attr->aperture_end + 1;
> -
> spin_lock_irqsave(&iommu_lock, flags);
> ret = pamu_disable_liodn(liodn);
> - if (!ret)
> - ret = pamu_config_ppaace(liodn, window_addr, window_size,
> omi_index,
> - 0, dma_domain->snoop_id,
> - dma_domain->stash_id, 0);
> + if (ret)
> + goto out_unlock;
> + ret = pamu_config_ppaace(liodn, geom->aperture_start,
> + geom->aperture_end - 1, omi_index, 0,
> + dma_domain->snoop_id, dma_domain->stash_id, 0);
> + if (ret)
> + goto out_unlock;
> + ret = pamu_config_ppaace(liodn, geom->aperture_start,
> + geom->aperture_end - 1, ~(u32)0,
> + 0, dma_domain->snoop_id, dma_domain->stash_id,
> + PAACE_AP_PERMS_QUERY | PAACE_AP_PERMS_UPDATE);
There's more '+1' / '-1' confusion here with aperture_end which I'm not
managing to follow. What am I missing?
Will
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/18] iommu/fsl_pamu: replace DOMAIN_ATTR_FSL_PAMU_STASH with a direct call
On Tue, Mar 16, 2021 at 04:38:13PM +0100, Christoph Hellwig wrote: > Add a fsl_pamu_configure_l1_stash API that qman_portal can call directly > instead of indirecting through the iommu attr API. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > arch/powerpc/include/asm/fsl_pamu_stash.h | 12 +++- > drivers/iommu/fsl_pamu_domain.c | 16 +++- > drivers/iommu/fsl_pamu_domain.h | 2 -- > drivers/soc/fsl/qbman/qman_portal.c | 18 +++--- > include/linux/iommu.h | 1 - > 5 files changed, 9 insertions(+), 40 deletions(-) Heh, this thing is so over-engineered. Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 06/18] iommu/fsl_pamu: remove ->domain_window_enable
On Tue, Mar 16, 2021 at 04:38:12PM +0100, Christoph Hellwig wrote:
> The only thing that fsl_pamu_window_enable does for the current caller
> is to fill in the prot value in the only dma_window structure, and to
> propagate a few values from the iommu_domain_geometry struture into the
> dma_window. Remove the dma_window entirely, hardcode the prot value and
> otherwise use the iommu_domain_geometry structure instead.
>
> Remove the now unused ->domain_window_enable iommu method.
>
> Signed-off-by: Christoph Hellwig
> Acked-by: Li Yang
> ---
> drivers/iommu/fsl_pamu_domain.c | 182 +++-
> drivers/iommu/fsl_pamu_domain.h | 17 ---
> drivers/iommu/iommu.c | 11 --
> drivers/soc/fsl/qbman/qman_portal.c | 7 --
> include/linux/iommu.h | 17 ---
> 5 files changed, 14 insertions(+), 220 deletions(-)
>
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index e6bdd38fc18409..fd2bc88b690465 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -54,34 +54,18 @@ static int __init iommu_init_mempool(void)
> return 0;
> }
>
> -static phys_addr_t get_phys_addr(struct fsl_dma_domain *dma_domain,
> dma_addr_t iova)
> -{
> - struct dma_window *win_ptr = &dma_domain->win_arr[0];
> - struct iommu_domain_geometry *geom;
> -
> - geom = &dma_domain->iommu_domain.geometry;
> -
> - if (win_ptr->valid)
> - return win_ptr->paddr + (iova & (win_ptr->size - 1));
> -
> - return 0;
> -}
> -
> /* Map the DMA window corresponding to the LIODN */
> static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
> {
> int ret;
> - struct dma_window *wnd = &dma_domain->win_arr[0];
> - phys_addr_t wnd_addr = dma_domain->iommu_domain.geometry.aperture_start;
> + struct iommu_domain_geometry *geom = &dma_domain->iommu_domain.geometry;
> unsigned long flags;
>
> spin_lock_irqsave(&iommu_lock, flags);
> - ret = pamu_config_ppaace(liodn, wnd_addr,
> - wnd->size,
> - ~(u32)0,
> - wnd->paddr >> PAMU_PAGE_SHIFT,
> - dma_domain->snoop_id, dma_domain->stash_id,
> - wnd->prot);
> + ret = pamu_config_ppaace(liodn, geom->aperture_start,
> + geom->aperture_end - 1, ~(u32)0,
You're passing 'geom->aperture_end - 1' as the size here, but the old code
seemed to _add_ 1:
> -static int fsl_pamu_window_enable(struct iommu_domain *domain, u32 wnd_nr,
> - phys_addr_t paddr, u64 size, int prot)
> -{
> - struct fsl_dma_domain *dma_domain = to_fsl_dma_domain(domain);
> - struct dma_window *wnd;
> - int pamu_prot = 0;
> - int ret;
> - unsigned long flags;
> - u64 win_size;
> -
> - if (prot & IOMMU_READ)
> - pamu_prot |= PAACE_AP_PERMS_QUERY;
> - if (prot & IOMMU_WRITE)
> - pamu_prot |= PAACE_AP_PERMS_UPDATE;
> -
> - spin_lock_irqsave(&dma_domain->domain_lock, flags);
> - if (wnd_nr > 0) {
> - pr_debug("Invalid window index\n");
> - spin_unlock_irqrestore(&dma_domain->domain_lock, flags);
> - return -EINVAL;
> - }
> -
> - win_size = (domain->geometry.aperture_end + 1) >> ilog2(1);
here ^^ when calculating the exclusive upper bound. In other words, I think
'1ULL << 36' used to get passed to pamu_config_ppaace(). Is that an
intentional change?
Will
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 05/18] iommu/fsl_pamu: remove support for multiple windows
On Tue, Mar 16, 2021 at 04:38:11PM +0100, Christoph Hellwig wrote:
> The only domains allocated forces use of a single window. Remove all
> the code related to multiple window support, as well as the need for
> qman_portal to force a single window.
>
> Remove the now unused DOMAIN_ATTR_WINDOWS iommu_attr.
>
> Signed-off-by: Christoph Hellwig
> Acked-by: Li Yang
> ---
> drivers/iommu/fsl_pamu.c| 264 +-
> drivers/iommu/fsl_pamu.h| 10 +-
> drivers/iommu/fsl_pamu_domain.c | 275 +---
> drivers/iommu/fsl_pamu_domain.h | 12 +-
> drivers/soc/fsl/qbman/qman_portal.c | 7 -
> include/linux/iommu.h | 1 -
> 6 files changed, 59 insertions(+), 510 deletions(-)
[...]
> + set_bf(ppaace->impl_attr, PAACE_IA_ATM, PAACE_ATM_WINDOW_XLATE);
> + ppaace->twbah = rpn >> 20;
> + set_bf(ppaace->win_bitfields, PAACE_WIN_TWBAL, rpn);
> + set_bf(ppaace->addr_bitfields, PAACE_AF_AP, prot);
> + set_bf(ppaace->impl_attr, PAACE_IA_WCE, 0);
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_MW, 0);
> mb();
(I wonder what on Earth that mb() is doing...)
> diff --git a/drivers/iommu/fsl_pamu_domain.h b/drivers/iommu/fsl_pamu_domain.h
> index 53d359d66fe577..b9236fb5a8f82e 100644
> --- a/drivers/iommu/fsl_pamu_domain.h
> +++ b/drivers/iommu/fsl_pamu_domain.h
> @@ -17,23 +17,13 @@ struct dma_window {
> };
>
> struct fsl_dma_domain {
> - /*
> - * Number of windows assocaited with this domain.
> - * During domain initialization, it is set to the
> - * the maximum number of subwindows allowed for a LIODN.
> - * Minimum value for this is 1 indicating a single PAMU
> - * window, without any sub windows. Value can be set/
> - * queried by set_attr/get_attr API for DOMAIN_ATTR_WINDOWS.
> - * Value can only be set once the geometry has been configured.
> - */
> - u32 win_cnt;
> /*
>* win_arr contains information of the configured
>* windows for a domain. This is allocated only
>* when the number of windows for the domain are
>* set.
>*/
The last part of this comment is now stale ^^
> - struct dma_window *win_arr;
> + struct dma_window win_arr[1];
> /* list of devices associated with the domain */
> struct list_headdevices;
> /* dma_domain states:
Acked-by: Will Deacon
Will
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 04/18] iommu/fsl_pamu: merge iommu_alloc_dma_domain into fsl_pamu_domain_alloc
On Tue, Mar 16, 2021 at 04:38:10PM +0100, Christoph Hellwig wrote: > Keep the functionality to allocate the domain together. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/fsl_pamu_domain.c | 34 ++--- > 1 file changed, 10 insertions(+), 24 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 03/18] iommu/fsl_pamu: remove support for setting DOMAIN_ATTR_GEOMETRY
On Tue, Mar 16, 2021 at 04:38:09PM +0100, Christoph Hellwig wrote: > The default geometry is the same as the one set by qman_port given > that FSL_PAMU depends on having 64-bit physical and thus DMA addresses. > > Remove the support to update the geometry and remove the now pointless > geom_size field. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/fsl_pamu_domain.c | 55 +++-- > drivers/iommu/fsl_pamu_domain.h | 6 > drivers/soc/fsl/qbman/qman_portal.c | 12 --- > 3 files changed, 5 insertions(+), 68 deletions(-) Took me a minute to track down the other magic '36' which ends up in aperture_end, but I found it eventually so: Acked-by: Will Deacon (It does make me wonder what all this glue was intended to be used for) Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 02/18] iommu/fsl_pamu: remove fsl_pamu_get_domain_attr
On Tue, Mar 16, 2021 at 04:38:08PM +0100, Christoph Hellwig wrote: > None of the values returned by this function are ever queried. Also > remove the DOMAIN_ATTR_FSL_PAMUV1 enum value that is not otherwise used. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/fsl_pamu_domain.c | 30 -- > include/linux/iommu.h | 4 > 2 files changed, 34 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/18] iommu: remove the unused domain_window_disable method
On Tue, Mar 16, 2021 at 04:38:07PM +0100, Christoph Hellwig wrote: > domain_window_disable is wired up by fsl_pamu, but never actually called. > > Signed-off-by: Christoph Hellwig > Acked-by: Li Yang > --- > drivers/iommu/fsl_pamu_domain.c | 48 - > include/linux/iommu.h | 2 -- > 2 files changed, 50 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c.
On Mon, Mar 29, 2021 at 09:02:50PM -0700, Rob Clark wrote: > On Mon, Mar 29, 2021 at 7:47 AM Will Deacon wrote: > > > > On Fri, Mar 26, 2021 at 04:13:02PM -0700, Eric Anholt wrote: > > > db820c wants to use the qcom smmu path to get HUPCF set (which keeps > > > the GPU from wedging and then sometimes wedging the kernel after a > > > page fault), but it doesn't have separate pagetables support yet in > > > drm/msm so we can't go all the way to the TTBR1 path. > > > > What do you mean by "doesn't have separate pagetables support yet"? The > > compatible string doesn't feel like the right way to determine this. > > the compatible string identifies what it is, not what the sw > limitations are, so in that regard it seems right to me.. Well it depends on what "doesn't have separate pagetables support yet" means. I can't tell if it's a hardware issue, a firmware issue or a driver issue. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c.
On Fri, Mar 26, 2021 at 04:13:02PM -0700, Eric Anholt wrote: > db820c wants to use the qcom smmu path to get HUPCF set (which keeps > the GPU from wedging and then sometimes wedging the kernel after a > page fault), but it doesn't have separate pagetables support yet in > drm/msm so we can't go all the way to the TTBR1 path. What do you mean by "doesn't have separate pagetables support yet"? The compatible string doesn't feel like the right way to determine this. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag
On Tue, Mar 09, 2021 at 12:10:44PM +0530, Sai Prakash Ranjan wrote: > On 2021-02-05 17:38, Sai Prakash Ranjan wrote: > > On 2021-02-04 03:16, Will Deacon wrote: > > > On Tue, Feb 02, 2021 at 11:56:27AM +0530, Sai Prakash Ranjan wrote: > > > > On 2021-02-01 23:50, Jordan Crouse wrote: > > > > > On Mon, Feb 01, 2021 at 08:20:44AM -0800, Rob Clark wrote: > > > > > > On Mon, Feb 1, 2021 at 3:16 AM Will Deacon wrote: > > > > > > > On Fri, Jan 29, 2021 at 03:12:59PM +0530, Sai Prakash Ranjan > > > > > > > wrote: > > > > > > > > On 2021-01-29 14:35, Will Deacon wrote: > > > > > > > > > On Mon, Jan 11, 2021 at 07:45:04PM +0530, Sai Prakash Ranjan > > > > > > > > > wrote: > > > > > > > > > > +#define IOMMU_LLC(1 << 6) > > > > > > > > > > > > > > > > > > On reflection, I'm a bit worried about exposing this because > > > > > > > > > I think it > > > > > > > > > will > > > > > > > > > introduce a mismatched virtual alias with the CPU (we don't > > > > > > > > > even have a > > > > > > > > > MAIR > > > > > > > > > set up for this memory type). Now, we also have that issue > > > > > > > > > for the PTW, > > > > > > > > > but > > > > > > > > > since we always use cache maintenance (i.e. the streaming > > > > > > > > > API) for > > > > > > > > > publishing the page-tables to a non-coheren walker, it works > > > > > > > > > out. > > > > > > > > > However, > > > > > > > > > if somebody expects IOMMU_LLC to be coherent with a DMA API > > > > > > > > > coherent > > > > > > > > > allocation, then they're potentially in for a nasty surprise > > > > > > > > > due to the > > > > > > > > > mismatched outer-cacheability attributes. > > > > > > > > > > > > > > > > > > > > > > > > > Can't we add the syscached memory type similar to what is done > > > > > > > > on android? > > > > > > > > > > > > > > Maybe. How does the GPU driver map these things on the CPU side? > > > > > > > > > > > > Currently we use writecombine mappings for everything, although > > > > > > there > > > > > > are some cases that we'd like to use cached (but have not merged > > > > > > patches that would give userspace a way to flush/invalidate) > > > > > > > > > > > > > > > > LLC/system cache doesn't have a relationship with the CPU cache. Its > > > > > just a > > > > > little accelerator that sits on the connection from the GPU to DDR and > > > > > caches > > > > > accesses. The hint that Sai is suggesting is used to mark the buffers > > > > > as > > > > > 'no-write-allocate' to prevent GPU write operations from being cached > > > > > in > > > > > the LLC > > > > > which a) isn't interesting and b) takes up cache space for read > > > > > operations. > > > > > > > > > > Its easiest to think of the LLC as a bonus accelerator that has no > > > > > cost > > > > > for > > > > > us to use outside of the unfortunate per buffer hint. > > > > > > > > > > We do have to worry about the CPU cache w.r.t I/O coherency (which is > > > > > a > > > > > different hint) and in that case we have all of concerns that Will > > > > > identified. > > > > > > > > > > > > > For mismatched outer cacheability attributes which Will > > > > mentioned, I was > > > > referring to [1] in android kernel. > > > > > > I've lost track of the conversation here :/ > > > > > > When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also > > > mapped > > > into the CPU and with what attributes? Rob said "writecombine for > > > everything" -- does that mean ioremap_wc() / MEMREMAP_WC? > > > > > > > Rob answered this. > > > > > Finally, we need to be careful when we use the word "hint" as > > > "allocation > > > hint" has a specific meaning in the architecture, and if we only > > > mismatch on > > > those then we're actually ok. But I think IOMMU_LLC is more than > > > just a > > > hint, since it actually drives eviction policy (i.e. it enables > > > writeback). > > > > > > Sorry for the pedantry, but I just want to make sure we're all talking > > > about the same things! > > > > > > > Sorry for the confusion which probably was caused by my mentioning of > > android, NWA(no write allocate) is an allocation hint which we can > > ignore > > for now as it is not introduced yet in upstream. > > > > Any chance of taking this forward? We do not want to miss out on small fps > gain when the product gets released. Do we have a solution to the mismatched virtual alias? Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [PATCH 16/17] iommu: remove DOMAIN_ATTR_IO_PGTABLE_CFG
On Wed, Mar 10, 2021 at 09:58:06AM +0100, Christoph Hellwig wrote: > On Fri, Mar 05, 2021 at 10:00:12AM +0000, Will Deacon wrote: > > > But one thing I'm not sure about is whether > > > IO_PGTABLE_QUIRK_ARM_OUTER_WBWA is something that other devices > > > *should* be using as well, but just haven't gotten around to yet. > > > > The intention is certainly that this would be a place to collate per-domain > > pgtable quirks, so I'd prefer not to tie that to the GPU. > > So the overall consensus is to just keep this as-is for now? Yes, please. If it doesn't see wider adoption then we can revisit it. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [PATCH 16/17] iommu: remove DOMAIN_ATTR_IO_PGTABLE_CFG
On Thu, Mar 04, 2021 at 03:11:08PM -0800, Rob Clark wrote: > On Thu, Mar 4, 2021 at 7:48 AM Robin Murphy wrote: > > > > On 2021-03-01 08:42, Christoph Hellwig wrote: > > > Signed-off-by: Christoph Hellwig > > > > Moreso than the previous patch, where the feature is at least relatively > > generic (note that there's a bunch of in-flight development around > > DOMAIN_ATTR_NESTING), I'm really not convinced that it's beneficial to > > bloat the generic iommu_ops structure with private driver-specific > > interfaces. The attribute interface is a great compromise for these > > kinds of things, and you can easily add type-checked wrappers around it > > for external callers (maybe even make the actual attributes internal > > between the IOMMU core and drivers) if that's your concern. > > I suppose if this is *just* for the GPU we could move it into > adreno_smmu_priv.. > > But one thing I'm not sure about is whether > IO_PGTABLE_QUIRK_ARM_OUTER_WBWA is something that other devices > *should* be using as well, but just haven't gotten around to yet. The intention is certainly that this would be a place to collate per-domain pgtable quirks, so I'd prefer not to tie that to the GPU. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag
On Tue, Feb 02, 2021 at 11:56:27AM +0530, Sai Prakash Ranjan wrote: > On 2021-02-01 23:50, Jordan Crouse wrote: > > On Mon, Feb 01, 2021 at 08:20:44AM -0800, Rob Clark wrote: > > > On Mon, Feb 1, 2021 at 3:16 AM Will Deacon wrote: > > > > On Fri, Jan 29, 2021 at 03:12:59PM +0530, Sai Prakash Ranjan wrote: > > > > > On 2021-01-29 14:35, Will Deacon wrote: > > > > > > On Mon, Jan 11, 2021 at 07:45:04PM +0530, Sai Prakash Ranjan wrote: > > > > > > > +#define IOMMU_LLC(1 << 6) > > > > > > > > > > > > On reflection, I'm a bit worried about exposing this because I > > > > > > think it > > > > > > will > > > > > > introduce a mismatched virtual alias with the CPU (we don't even > > > > > > have a > > > > > > MAIR > > > > > > set up for this memory type). Now, we also have that issue for the > > > > > > PTW, > > > > > > but > > > > > > since we always use cache maintenance (i.e. the streaming API) for > > > > > > publishing the page-tables to a non-coheren walker, it works out. > > > > > > However, > > > > > > if somebody expects IOMMU_LLC to be coherent with a DMA API coherent > > > > > > allocation, then they're potentially in for a nasty surprise due to > > > > > > the > > > > > > mismatched outer-cacheability attributes. > > > > > > > > > > > > > > > > Can't we add the syscached memory type similar to what is done on > > > > > android? > > > > > > > > Maybe. How does the GPU driver map these things on the CPU side? > > > > > > Currently we use writecombine mappings for everything, although there > > > are some cases that we'd like to use cached (but have not merged > > > patches that would give userspace a way to flush/invalidate) > > > > > > > LLC/system cache doesn't have a relationship with the CPU cache. Its > > just a > > little accelerator that sits on the connection from the GPU to DDR and > > caches > > accesses. The hint that Sai is suggesting is used to mark the buffers as > > 'no-write-allocate' to prevent GPU write operations from being cached in > > the LLC > > which a) isn't interesting and b) takes up cache space for read > > operations. > > > > Its easiest to think of the LLC as a bonus accelerator that has no cost > > for > > us to use outside of the unfortunate per buffer hint. > > > > We do have to worry about the CPU cache w.r.t I/O coherency (which is a > > different hint) and in that case we have all of concerns that Will > > identified. > > > > For mismatched outer cacheability attributes which Will mentioned, I was > referring to [1] in android kernel. I've lost track of the conversation here :/ When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC? Finally, we need to be careful when we use the word "hint" as "allocation hint" has a specific meaning in the architecture, and if we only mismatch on those then we're actually ok. But I think IOMMU_LLC is more than just a hint, since it actually drives eviction policy (i.e. it enables writeback). Sorry for the pedantry, but I just want to make sure we're all talking about the same things! Cheers, Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag
On Fri, Jan 29, 2021 at 03:12:59PM +0530, Sai Prakash Ranjan wrote: > On 2021-01-29 14:35, Will Deacon wrote: > > On Mon, Jan 11, 2021 at 07:45:04PM +0530, Sai Prakash Ranjan wrote: > > > Add a new page protection flag IOMMU_LLC which can be used > > > by non-coherent masters to set cacheable memory attributes > > > for an outer level of cache called as last-level cache or > > > system cache. Initial user of this page protection flag is > > > the adreno gpu and then can later be used by other clients > > > such as video where this can be used for per-buffer based > > > mapping. > > > > > > Signed-off-by: Sai Prakash Ranjan > > > --- > > > drivers/iommu/io-pgtable-arm.c | 3 +++ > > > include/linux/iommu.h | 6 ++ > > > 2 files changed, 9 insertions(+) > > > > > > diff --git a/drivers/iommu/io-pgtable-arm.c > > > b/drivers/iommu/io-pgtable-arm.c > > > index 7439ee7fdcdb..ebe653ef601b 100644 > > > --- a/drivers/iommu/io-pgtable-arm.c > > > +++ b/drivers/iommu/io-pgtable-arm.c > > > @@ -415,6 +415,9 @@ static arm_lpae_iopte > > > arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data, > > > else if (prot & IOMMU_CACHE) > > > pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE > > > << ARM_LPAE_PTE_ATTRINDX_SHIFT); > > > + else if (prot & IOMMU_LLC) > > > + pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE > > > + << ARM_LPAE_PTE_ATTRINDX_SHIFT); > > > } > > > > > > if (prot & IOMMU_CACHE) > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > index ffaa389ea128..1f82057df531 100644 > > > --- a/include/linux/iommu.h > > > +++ b/include/linux/iommu.h > > > @@ -31,6 +31,12 @@ > > > * if the IOMMU page table format is equivalent. > > > */ > > > #define IOMMU_PRIV (1 << 5) > > > +/* > > > + * Non-coherent masters can use this page protection flag to set > > > cacheable > > > + * memory attributes for only a transparent outer level of cache, > > > also known as > > > + * the last-level or system cache. > > > + */ > > > +#define IOMMU_LLC(1 << 6) > > > > On reflection, I'm a bit worried about exposing this because I think it > > will > > introduce a mismatched virtual alias with the CPU (we don't even have a > > MAIR > > set up for this memory type). Now, we also have that issue for the PTW, > > but > > since we always use cache maintenance (i.e. the streaming API) for > > publishing the page-tables to a non-coheren walker, it works out. > > However, > > if somebody expects IOMMU_LLC to be coherent with a DMA API coherent > > allocation, then they're potentially in for a nasty surprise due to the > > mismatched outer-cacheability attributes. > > > > Can't we add the syscached memory type similar to what is done on android? Maybe. How does the GPU driver map these things on the CPU side? Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag
On Mon, Jan 11, 2021 at 07:45:04PM +0530, Sai Prakash Ranjan wrote: > Add a new page protection flag IOMMU_LLC which can be used > by non-coherent masters to set cacheable memory attributes > for an outer level of cache called as last-level cache or > system cache. Initial user of this page protection flag is > the adreno gpu and then can later be used by other clients > such as video where this can be used for per-buffer based > mapping. > > Signed-off-by: Sai Prakash Ranjan > --- > drivers/iommu/io-pgtable-arm.c | 3 +++ > include/linux/iommu.h | 6 ++ > 2 files changed, 9 insertions(+) > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c > index 7439ee7fdcdb..ebe653ef601b 100644 > --- a/drivers/iommu/io-pgtable-arm.c > +++ b/drivers/iommu/io-pgtable-arm.c > @@ -415,6 +415,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct > arm_lpae_io_pgtable *data, > else if (prot & IOMMU_CACHE) > pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE > << ARM_LPAE_PTE_ATTRINDX_SHIFT); > + else if (prot & IOMMU_LLC) > + pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE > + << ARM_LPAE_PTE_ATTRINDX_SHIFT); > } > > if (prot & IOMMU_CACHE) > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index ffaa389ea128..1f82057df531 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -31,6 +31,12 @@ > * if the IOMMU page table format is equivalent. > */ > #define IOMMU_PRIV (1 << 5) > +/* > + * Non-coherent masters can use this page protection flag to set cacheable > + * memory attributes for only a transparent outer level of cache, also known > as > + * the last-level or system cache. > + */ > +#define IOMMU_LLC(1 << 6) On reflection, I'm a bit worried about exposing this because I think it will introduce a mismatched virtual alias with the CPU (we don't even have a MAIR set up for this memory type). Now, we also have that issue for the PTW, but since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However, if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes. So I can take patch (1) as a trivial rename, but unfortunately I think this needs more thought before exposing it beyond the PTW. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/7] drm/msm: Add dependency on io-pgtable-arm format module
On Mon, Jan 18, 2021 at 01:16:03PM -0800, Rob Clark wrote: > On Mon, Dec 21, 2020 at 4:44 PM Isaac J. Manjarres > wrote: > > > > The MSM DRM driver depends on the availability of the ARM LPAE io-pgtable > > format code to work properly. In preparation for having the io-pgtable > > formats as modules, add a "pre" dependency with MODULE_SOFTDEP() to > > ensure that the io-pgtable-arm format module is loaded before loading > > the MSM DRM driver module. > > > > Signed-off-by: Isaac J. Manjarres > > Thanks, I've queued this up locally I don't plan to make the io-pgtable code modular, so please drop this patch. https://lore.kernel.org/r/20210106123428.GA1798@willie-the-truck Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RESEND 0/7] iommu: Permit modular builds of io-pgtable drivers
On Mon, Jan 04, 2021 at 11:36:38PM -0800, Isaac J. Manjarres wrote: > The goal of the Generic Kernel Image (GKI) effort is to have a common > kernel image that works across multiple Android devices. This involves > generating a kernel image that has core features integrated into it, > while SoC specific functionality can be added to the kernel for the > device as a module. > > Along with modularizing IOMMU drivers, this also means building the > io-pgtable code as modules, which allows for SoC vendors to only include > the io-pgtable implementations that they use. For example, GKI for arm64 > must include support for both the IOMMU ARM LPAE/V7S formats at the > moment. Having the code for both formats as modules allows SoC vendors > to only provide the page table format that they use, along with their > IOMMU driver. Why is this desirable for upstream? This code isn't especially large, and the formats we support are largely architectural, meaning that they are shared between different IOMMU drivers. I think that making this modular just means that out-of-tree modifications are likely to generate page-tables which are specific to a particular IOMMU, and lead to horrible problems (crashes and data corruption) if another IOMMU driver tries to use them. Please can you upstream whatever changes you want to make instead? Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: Revert "add DCN support for aarch64"
On Mon, Jan 04, 2021 at 11:27:24AM -0500, Alex Deucher wrote: > On Tue, Dec 29, 2020 at 8:17 AM Ard Biesheuvel wrote: > > > > On Wed, 16 Dec 2020 at 23:26, Ard Biesheuvel wrote: > > > > > > On Wed, 16 Dec 2020 at 19:00, Alex Deucher wrote: > > > > > > > > On Mon, Dec 14, 2020 at 12:53 PM Ard Biesheuvel wrote: > > > > > > > > > > This reverts commit c38d444e44badc557cf29fdfdfb823604890ccfa. > > > > > > > > > > Simply disabling -mgeneral-regs-only left and right is risky, given > > > > > that > > > > > the standard AArch64 ABI permits the use of FP/SIMD registers > > > > > anywhere, > > > > > and GCC is known to use SIMD registers for spilling, and may invent > > > > > other uses of the FP/SIMD register file that have nothing to do with > > > > > the > > > > > floating point code in question. Note that putting kernel_neon_begin() > > > > > and kernel_neon_end() around the code that does use FP is not > > > > > sufficient > > > > > here, the problem is in all the other code that may be emitted with > > > > > references to SIMD registers in it. > > > > > > > > > > So the only way to do this properly is to put all floating point code > > > > > in > > > > > a separate compilation unit, and only compile that unit with > > > > > -mgeneral-regs-only. But perhaps the use of floating point here is > > > > > something that should be reconsidered entirely. > > > > > > > > > > Cc: Catalin Marinas > > > > > Cc: Will Deacon > > > > > Cc: Dave Martin > > > > > Cc: Rob Herring > > > > > Cc: Leo Li > > > > > Cc: Alex Deucher > > > > > Cc: "Christian König" > > > > > Cc: David Airlie > > > > > Cc: Daniel Vetter > > > > > Cc: Daniel Kolesa > > > > > Cc: amd-...@lists.freedesktop.org > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > Signed-off-by: Ard Biesheuvel > > > > > > > > Can rebase this on Linus' master branch? There were a number of new > > > > asics added which copy pasted the ARM64 support. > > > > > > > > > > Not sure what you are asking me here. Reverting commit c38d444e44badc5 > > > on top of mainline is not going to fix the other code that was added. > > > Or are you asking me to go and find the patches (how many?) that added > > > new ASICs and fix them for arm64? > > > > > > Note that this code is critically broken, as it may corrupt user > > > process state arbitrarily. So if new code was added that contains the > > > same bug, it should be reverted so that the respective authors can fix > > > it and resubmit. > > > > > > > Is this simply about dropping the newly added references to > > $(dml_rcflags) from the Makefile? Because that is quite trivial ... > > Yes, I was thinking something like the attached patch. > > Alex > From fbc93ca7d7739861ce63f6b483cf23d7cf1d69fb Mon Sep 17 00:00:00 2001 > From: Alex Deucher > Date: Mon, 4 Jan 2021 11:24:20 -0500 > Subject: [PATCH] drm/amdgpu/display: drop DCN support for aarch64 > > From Ard: > > "Simply disabling -mgeneral-regs-only left and right is risky, given that > the standard AArch64 ABI permits the use of FP/SIMD registers anywhere, > and GCC is known to use SIMD registers for spilling, and may invent > other uses of the FP/SIMD register file that have nothing to do with the > floating point code in question. Note that putting kernel_neon_begin() > and kernel_neon_end() around the code that does use FP is not sufficient > here, the problem is in all the other code that may be emitted with > references to SIMD registers in it. > > So the only way to do this properly is to put all floating point code in > a separate compilation unit, and only compile that unit with > -mgeneral-regs-only." > > Disable support until the code can be properly refactored to support this > properly on aarch64. > > Reported-by: Ard Biesheuvel > Signed-off-by: Alex Deucher > --- > drivers/gpu/drm/amd/display/Kconfig | 2 +- > drivers/gpu/drm/amd/display/dc/calcs/Makefile | 4 > .../gpu/drm/amd/display/dc/clk_mgr/Makefile | 21 --- > drivers/gpu/drm/amd/display/dc/dcn10/Makefile | 7 --- > .../drm/amd/display/dc/dcn10/dcn10_resource.c | 7 --- > drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 4 > drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 4 > drivers/gpu/drm/amd/display/dc/dcn30/Makefile | 5 - > .../gpu/drm/amd/display/dc/dcn301/Makefile| 4 > .../gpu/drm/amd/display/dc/dcn302/Makefile| 4 > drivers/gpu/drm/amd/display/dc/dml/Makefile | 4 > drivers/gpu/drm/amd/display/dc/dsc/Makefile | 4 > drivers/gpu/drm/amd/display/dc/os_types.h | 4 > 13 files changed, 1 insertion(+), 73 deletions(-) Acked-by: Will Deacon Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: Revert "add DCN support for aarch64"
On Mon, Dec 14, 2020 at 06:52:25PM +0100, Ard Biesheuvel wrote: > This reverts commit c38d444e44badc557cf29fdfdfb823604890ccfa. > > Simply disabling -mgeneral-regs-only left and right is risky, given that > the standard AArch64 ABI permits the use of FP/SIMD registers anywhere, > and GCC is known to use SIMD registers for spilling, and may invent > other uses of the FP/SIMD register file that have nothing to do with the > floating point code in question. Note that putting kernel_neon_begin() > and kernel_neon_end() around the code that does use FP is not sufficient > here, the problem is in all the other code that may be emitted with > references to SIMD registers in it. > > So the only way to do this properly is to put all floating point code in > a separate compilation unit, and only compile that unit with > -mgeneral-regs-only. But perhaps the use of floating point here is > something that should be reconsidered entirely. > > Cc: Catalin Marinas > Cc: Will Deacon > Cc: Dave Martin > Cc: Rob Herring > Cc: Leo Li > Cc: Alex Deucher > Cc: "Christian König" > Cc: David Airlie > Cc: Daniel Vetter > Cc: Daniel Kolesa > Cc: amd-...@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Ard Biesheuvel > --- > drivers/gpu/drm/amd/display/Kconfig | 2 +- > drivers/gpu/drm/amd/display/dc/calcs/Makefile | 7 -- > drivers/gpu/drm/amd/display/dc/clk_mgr/Makefile | 7 -- > drivers/gpu/drm/amd/display/dc/dcn10/Makefile | 7 -- > drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c | 81 > > drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 4 - > drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 4 - > drivers/gpu/drm/amd/display/dc/dml/Makefile | 13 > drivers/gpu/drm/amd/display/dc/dsc/Makefile | 5 -- > drivers/gpu/drm/amd/display/dc/os_types.h | 4 - > 10 files changed, 32 insertions(+), 102 deletions(-) I didn't notice we'd enabled this for arm64, but I agree with the reasoning in the commit message, so: Acked-by: Will Deacon The long and short of it is that it is not safe to compile kernel C code without -mgeneral-regs-only on arm64. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] iommu/io-pgtable: Remove tlb_flush_leaf
On Wed, 25 Nov 2020 17:29:39 +, Robin Murphy wrote: > The only user of tlb_flush_leaf is a particularly hairy corner of the > Arm short-descriptor code, which wants a synchronous invalidation to > minimise the races inherent in trying to split a large page mapping. > This is already far enough into "here be dragons" territory that no > sensible caller should ever hit it, and thus it really doesn't need > optimising. Although using tlb_flush_walk there may technically be > more heavyweight than needed, it does the job and saves everyone else > having to carry around useless baggage. Applied to arm64 (for-next/iommu/core), thanks! [1/1] iommu/io-pgtable: Remove tlb_flush_leaf https://git.kernel.org/arm64/c/fefe8527a1e0 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] iommu/io-pgtable: Remove tlb_flush_leaf
On Wed, Nov 25, 2020 at 05:29:39PM +, Robin Murphy wrote: > The only user of tlb_flush_leaf is a particularly hairy corner of the > Arm short-descriptor code, which wants a synchronous invalidation to > minimise the races inherent in trying to split a large page mapping. > This is already far enough into "here be dragons" territory that no > sensible caller should ever hit it, and thus it really doesn't need > optimising. Although using tlb_flush_walk there may technically be > more heavyweight than needed, it does the job and saves everyone else > having to carry around useless baggage. > > Signed-off-by: Robin Murphy > --- > > Reviewing the Mediatek TLB optimisation patches just left me thinking > "why do we even have this?"... Panfrost folks, this has zero functional > impact to you, merely wants an ack for straying outside drivers/iommu. Thanks, this looks good to me, but I'll defer queuing it until the last minute so that I can merge all the iommu component branches together first and then apply this on top. Should happen next week. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv10 0/9] System Cache support for GPU and required SMMU support
On Wed, 25 Nov 2020 12:30:09 +0530, Sai Prakash Ranjan wrote: > Some hardware variants contain a system cache or the last level > cache(llc). This cache is typically a large block which is shared > by multiple clients on the SOC. GPU uses the system cache to cache > both the GPU data buffers(like textures) as well the SMMU pagetables. > This helps with improved render performance as well as lower power > consumption by reducing the bus traffic to the system memory. > > [...] Applied first two patches on a shared branch for Rob: arm64 (for-next/iommu/io-pgtable-domain-attr), thanks! [1/9] iommu/io-pgtable: Add a domain attribute for pagetable configuration https://git.kernel.org/arm64/c/a7656ecf825a [2/9] iommu/io-pgtable-arm: Add support to use system cache https://git.kernel.org/arm64/c/e67890c97944 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv10 0/9] System Cache support for GPU and required SMMU support
On Wed, 25 Nov 2020 12:30:09 +0530, Sai Prakash Ranjan wrote: > Some hardware variants contain a system cache or the last level > cache(llc). This cache is typically a large block which is shared > by multiple clients on the SOC. GPU uses the system cache to cache > both the GPU data buffers(like textures) as well the SMMU pagetables. > This helps with improved render performance as well as lower power > consumption by reducing the bus traffic to the system memory. > > [...] Applied the SMMU bits to arm64 (for-next/iommu/arm-smmu), thanks! [3/9] iommu/arm-smmu: Add support for pagetable config domain attribute https://git.kernel.org/arm64/c/c99110a865a3 [4/9] iommu/arm-smmu: Move non-strict mode to use io_pgtable_domain_attr https://git.kernel.org/arm64/c/12bc36793fd6 [8/9] iommu: arm-smmu-impl: Use table to list QCOM implementations https://git.kernel.org/arm64/c/00597f9ff5ec [9/9] iommu: arm-smmu-impl: Add a space before open parenthesis https://git.kernel.org/arm64/c/7f575a6087f4 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv8 0/8] System Cache support for GPU and required SMMU support
On Tue, Nov 24, 2020 at 11:05:39AM -0800, Rob Clark wrote: > On Tue, Nov 24, 2020 at 3:10 AM Will Deacon wrote: > > On Tue, Nov 24, 2020 at 09:32:54AM +0530, Sai Prakash Ranjan wrote: > > > On 2020-11-24 00:52, Rob Clark wrote: > > > > On Mon, Nov 23, 2020 at 9:01 AM Sai Prakash Ranjan > > > > wrote: > > > > > On 2020-11-23 20:51, Will Deacon wrote: > > > > > > Modulo some minor comments I've made, this looks good to me. What is > > > > > > the > > > > > > plan for merging it? I can take the IOMMU parts, but patches 4-6 > > > > > > touch > > > > > > the > > > > > > MSM GPU driver and I'd like to avoid conflicts with that. > > > > > > > > > > > > > > > > SMMU bits are pretty much independent and GPU relies on the domain > > > > > attribute > > > > > and the quirk exposed, so as long as SMMU changes go in first it > > > > > should > > > > > be good. > > > > > Rob? > > > > > > > > I suppose one option would be to split out the patch that adds the > > > > attribute into it's own patch, and merge that both thru drm and iommu? > > > > > > > > > > Ok I can split out domain attr and quirk into its own patch if Will is > > > fine with that approach. > > > > Why don't I just queue the first two patches on their own branch and we > > both pull that? > > Ok, that works for me. I normally base msm-next on -rc1 but I guess > as long as we base the branch on the older or our two -next branches, > that should work out nicely Turns out we're getting a v10 of Sai's stuff, so I've asked him to split patch two up anyway. Then I'll make a branch based on -rc1 that we can both pull. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv9 2/8] iommu/arm-smmu: Add domain attribute for pagetable configuration
On Mon, Nov 23, 2020 at 10:35:55PM +0530, Sai Prakash Ranjan wrote: > Add iommu domain attribute for pagetable configuration which > initially will be used to set quirks like for system cache aka > last level cache to be used by client drivers like GPU to set > right attributes for caching the hardware pagetables into the > system cache and later can be extended to include other page > table configuration data. > > Signed-off-by: Sai Prakash Ranjan > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 20 > drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 + > include/linux/io-pgtable.h| 4 > include/linux/iommu.h | 1 + > 4 files changed, 26 insertions(+) Given that we're heading for a v10 to address my comments on patch 3, then I guess you may as well split this into two patches so that I can share just the atttibute with Rob rather than the driver parts. Please keep it all as one series though, with the common parts at the beginning, and I'll figure it out. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv9 3/8] iommu/arm-smmu: Move non-strict mode to use io_pgtable_domain_attr
On Mon, Nov 23, 2020 at 10:35:56PM +0530, Sai Prakash Ranjan wrote:
> Now that we have a struct io_pgtable_domain_attr with quirks,
> use that for non_strict mode as well thereby removing the need
> for more members of arm_smmu_domain in the future.
>
> Signed-off-by: Sai Prakash Ranjan
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 8 +++-
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 -
> 2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 4b9b10fe50ed..f56f266ebdf7 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -786,9 +786,6 @@ static int arm_smmu_init_domain_context(struct
> iommu_domain *domain,
> goto out_clear_smmu;
> }
>
> - if (smmu_domain->non_strict)
> - pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
> -
> if (smmu_domain->pgtbl_cfg.quirks)
> pgtbl_cfg.quirks |= smmu_domain->pgtbl_cfg.quirks;
>
> @@ -1527,7 +1524,8 @@ static int arm_smmu_domain_get_attr(struct iommu_domain
> *domain,
> case IOMMU_DOMAIN_DMA:
> switch (attr) {
> case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> - *(int *)data = smmu_domain->non_strict;
> + if (smmu_domain->pgtbl_cfg.quirks &
> IO_PGTABLE_QUIRK_NON_STRICT)
> + *(int *)data = smmu_domain->pgtbl_cfg.quirks;
I still don't think this is right :(
We need to set *data to 1 or 0 depending on whether or not the non-strict
quirk is set, i.e:
bool non_strict = smmu_domain->pgtbl_cfg.quirks &
IO_PGTABLE_QUIRK_NON_STRICT;
*(int *)data = non_strict;
Your code above leaves *data uninitialised if non_strict is not set.
> return 0;
> default:
> return -ENODEV;
> @@ -1578,7 +1576,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain
> *domain,
> case IOMMU_DOMAIN_DMA:
> switch (attr) {
> case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> - smmu_domain->non_strict = *(int *)data;
> + smmu_domain->pgtbl_cfg.quirks |=
> IO_PGTABLE_QUIRK_NON_STRICT;
And this is broken because if *data is 0, then you _set_ the quirk, which is
the opposite of what we should be doing.
In other words, although the implementation has changed, the semantics have
not.
Will
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv8 0/8] System Cache support for GPU and required SMMU support
On Tue, Nov 24, 2020 at 09:32:54AM +0530, Sai Prakash Ranjan wrote: > On 2020-11-24 00:52, Rob Clark wrote: > > On Mon, Nov 23, 2020 at 9:01 AM Sai Prakash Ranjan > > wrote: > > > > > > On 2020-11-23 20:51, Will Deacon wrote: > > > > On Tue, Nov 17, 2020 at 08:00:39PM +0530, Sai Prakash Ranjan wrote: > > > >> Some hardware variants contain a system cache or the last level > > > >> cache(llc). This cache is typically a large block which is shared > > > >> by multiple clients on the SOC. GPU uses the system cache to cache > > > >> both the GPU data buffers(like textures) as well the SMMU pagetables. > > > >> This helps with improved render performance as well as lower power > > > >> consumption by reducing the bus traffic to the system memory. > > > >> > > > >> The system cache architecture allows the cache to be split into slices > > > >> which then be used by multiple SOC clients. This patch series is an > > > >> effort to enable and use two of those slices preallocated for the GPU, > > > >> one for the GPU data buffers and another for the GPU SMMU hardware > > > >> pagetables. > > > >> > > > >> Patch 1 - Patch 6 adds system cache support in SMMU and GPU driver. > > > >> Patch 7 and 8 are minor cleanups for arm-smmu impl. > > > >> > > > >> Changes in v8: > > > >> * Introduce a generic domain attribute for pagetable config (Will) > > > >> * Rename quirk to more generic IO_PGTABLE_QUIRK_ARM_OUTER_WBWA (Will) > > > >> * Move non-strict mode to use new struct domain_attr_io_pgtbl_config > > > >> (Will) > > > > > > > > Modulo some minor comments I've made, this looks good to me. What is > > > > the > > > > plan for merging it? I can take the IOMMU parts, but patches 4-6 touch > > > > the > > > > MSM GPU driver and I'd like to avoid conflicts with that. > > > > > > > > > > SMMU bits are pretty much independent and GPU relies on the domain > > > attribute > > > and the quirk exposed, so as long as SMMU changes go in first it > > > should > > > be good. > > > Rob? > > > > I suppose one option would be to split out the patch that adds the > > attribute into it's own patch, and merge that both thru drm and iommu? > > > > Ok I can split out domain attr and quirk into its own patch if Will is > fine with that approach. Why don't I just queue the first two patches on their own branch and we both pull that? Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv8 0/8] System Cache support for GPU and required SMMU support
On Tue, Nov 17, 2020 at 08:00:39PM +0530, Sai Prakash Ranjan wrote: > Some hardware variants contain a system cache or the last level > cache(llc). This cache is typically a large block which is shared > by multiple clients on the SOC. GPU uses the system cache to cache > both the GPU data buffers(like textures) as well the SMMU pagetables. > This helps with improved render performance as well as lower power > consumption by reducing the bus traffic to the system memory. > > The system cache architecture allows the cache to be split into slices > which then be used by multiple SOC clients. This patch series is an > effort to enable and use two of those slices preallocated for the GPU, > one for the GPU data buffers and another for the GPU SMMU hardware > pagetables. > > Patch 1 - Patch 6 adds system cache support in SMMU and GPU driver. > Patch 7 and 8 are minor cleanups for arm-smmu impl. > > Changes in v8: > * Introduce a generic domain attribute for pagetable config (Will) > * Rename quirk to more generic IO_PGTABLE_QUIRK_ARM_OUTER_WBWA (Will) > * Move non-strict mode to use new struct domain_attr_io_pgtbl_config (Will) Modulo some minor comments I've made, this looks good to me. What is the plan for merging it? I can take the IOMMU parts, but patches 4-6 touch the MSM GPU driver and I'd like to avoid conflicts with that. Will ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv8 3/8] iommu/arm-smmu: Move non-strict mode to use domain_attr_io_pgtbl_cfg
On Tue, Nov 17, 2020 at 08:00:42PM +0530, Sai Prakash Ranjan wrote:
> Now that we have a struct domain_attr_io_pgtbl_cfg with quirks,
> use that for non_strict mode as well thereby removing the need
> for more members of arm_smmu_domain in the future.
>
> Signed-off-by: Sai Prakash Ranjan
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 7 ++-
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 -
> 2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 7b05782738e2..5f066a1b7221 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -786,9 +786,6 @@ static int arm_smmu_init_domain_context(struct
> iommu_domain *domain,
> goto out_clear_smmu;
> }
>
> - if (smmu_domain->non_strict)
> - pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
> -
> if (smmu_domain->pgtbl_cfg.quirks)
> pgtbl_cfg.quirks |= smmu_domain->pgtbl_cfg.quirks;
>
> @@ -1527,7 +1524,7 @@ static int arm_smmu_domain_get_attr(struct iommu_domain
> *domain,
> case IOMMU_DOMAIN_DMA:
> switch (attr) {
> case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> - *(int *)data = smmu_domain->non_strict;
> + *(int *)data = smmu_domain->pgtbl_cfg.quirks;
Probably better to compare with IO_PGTABLE_QUIRK_NON_STRICT here even though
we only support this one quirk for DMA domains atm.
Will
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
