linux-kernel - Re: [PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory Controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180809111746.GG21639@ulmo>
Date:   Thu, 9 Aug 2018 13:17:46 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Joerg Roedel <joro@...tes.org>,
        Robin Murphy <robin.murphy@....com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        iommu@...ts.linux-foundation.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory
 Controller driver

On Sat, Aug 04, 2018 at 05:29:57PM +0300, Dmitry Osipenko wrote:
> GART contain registers needed by the Memory Controller driver, provide
> access to the MC driver by utilizing its GART-integration facility.
> 
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
>  drivers/iommu/tegra-gart.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index a004f6da35f2..f8b653e25914 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c
> @@ -31,6 +31,8 @@
>  #include <linux/iommu.h>
>  #include <linux/of.h>
>  
> +#include <soc/tegra/mc.h>
> +
>  #include <asm/cacheflush.h>
>  
>  /* bitmap of the page sizes currently supported */
> @@ -41,6 +43,8 @@
>  #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
>  #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
>  #define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
> +#define GART_ERROR_REQ		(0x30 - GART_REG_BASE)
> +#define GART_ERROR_ADDR		(0x34 - GART_REG_BASE)
>  
>  #define GART_PAGE_SHIFT		12
>  #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
> @@ -63,6 +67,8 @@ struct gart_device {
>  	struct device		*dev;
>  
>  	struct iommu_device	iommu;		/* IOMMU Core handle */
> +
> +	struct tegra_mc_gart_handle mc_gart_handle;
>  };
>  
>  struct gart_domain {
> @@ -408,6 +414,20 @@ static int tegra_gart_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static u32 tegra_gart_error_addr(struct tegra_mc_gart_handle *handle)
> +{
> +	struct gart_device *gart = container_of(handle, struct gart_device,
> +						mc_gart_handle);
> +	return readl_relaxed(gart->regs + GART_ERROR_ADDR);
> +}
> +
> +static u32 tegra_gart_error_req(struct tegra_mc_gart_handle *handle)
> +{
> +	struct gart_device *gart = container_of(handle, struct gart_device,
> +						mc_gart_handle);
> +	return readl_relaxed(gart->regs + GART_ERROR_REQ);
> +}
> +
>  static int tegra_gart_probe(struct platform_device *pdev)
>  {
>  	struct gart_device *gart;
> @@ -464,6 +484,8 @@ static int tegra_gart_probe(struct platform_device *pdev)
>  	gart->regs = gart_regs;
>  	gart->iovmm_base = (dma_addr_t)res_remap->start;
>  	gart->page_count = (resource_size(res_remap) >> GART_PAGE_SHIFT);
> +	gart->mc_gart_handle.error_addr = tegra_gart_error_addr;
> +	gart->mc_gart_handle.error_req = tegra_gart_error_req;
>  
>  	gart->savedata = vmalloc(array_size(sizeof(u32), gart->page_count));
>  	if (!gart->savedata) {
> @@ -475,6 +497,7 @@ static int tegra_gart_probe(struct platform_device *pdev)
>  	do_gart_setup(gart, NULL);
>  
>  	gart_handle = gart;
> +	tegra_mc_register_gart(&gart->mc_gart_handle);
>  
>  	return 0;
>  }

I see now why you've did it this way. We have separate devices unlike
with SMMU where it is properly modeled as part of the memory controller.
I think we should consider breaking ABI at this point and properly merge
both the memory controller and GART nodes. There's really no reason why
they should be separate and we're jumping through multiple hoops to do
what we need to do just because a few years back we made a mistake.

I know we're technically not supposed to break the DT ABI, but I think
in this particular case we can make a good case for it. The current DT
bindings are plainly broken, and obviously so. Also, we don't currently
use the GART upstream for anything, so we can't break any existing
systems either.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)