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: <16855111.9YhuyP46zb@dimapc>
Date:   Thu, 09 Aug 2018 14:39:03 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...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 Thursday, 9 August 2018 14:17:46 MSK Thierry Reding wrote:
> 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.

IIUC, that will require to break the stable DT ABI of the tegra20-mc, which is 
working fine and does its job. I'm personally not seeing the slight lameness 
of the current DT as a good excuse to break the ABI. Let's then break DT ABI 
on all Tegra's and convert them all to genpd and other goodies like assigned 
clock parents and clock rate.