| 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
| ||
|
Message-ID: <55B630CE.4050803@arm.com>
Date: Mon, 27 Jul 2015 14:23:26 +0100
From: Robin Murphy <robin.murphy@....com>
To: Yong Wu <yong.wu@...iatek.com>, Joerg Roedel <joro@...tes.org>,
Thierry Reding <treding@...dia.com>,
Mark Rutland <Mark.Rutland@....com>,
Matthias Brugger <matthias.bgg@...il.com>
CC: Will Deacon <Will.Deacon@....com>,
Daniel Kurtz <djkurtz@...gle.com>,
Tomasz Figa <tfiga@...gle.com>,
Lucas Stach <l.stach@...gutronix.de>,
Rob Herring <robh+dt@...nel.org>,
Catalin Marinas <Catalin.Marinas@....com>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
Sasha Hauer <kernel@...gutronix.de>,
"srv_heupstream@...iatek.com" <srv_heupstream@...iatek.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"pebolle@...cali.nl" <pebolle@...cali.nl>,
"arnd@...db.de" <arnd@...db.de>,
"mitchelh@...eaurora.org" <mitchelh@...eaurora.org>,
"cloud.chou@...iatek.com" <cloud.chou@...iatek.com>,
"frederic.chen@...iatek.com" <frederic.chen@...iatek.com>
Subject: Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
On 16/07/15 10:04, Yong Wu wrote:
> This patch adds support for mediatek m4u (MultiMedia Memory Management
> Unit).
>
> Signed-off-by: Yong Wu <yong.wu@...iatek.com>
[...]
> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> +{
> + struct mtk_iommu_domain *domain = cookie;
> + unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> +
> + dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> + size, DMA_TO_DEVICE);
Nit: this looks like it may as well be dma_map_single.
It would probably be worth following it with a matching unmap too, just
to avoid any possible leakage bugs (especially if this M4U ever appears
in a SoC supporting RAM above the 32-bit boundary).
> +}
> +
[...]
> +static int mtk_iommu_parse_dt(struct platform_device *pdev,
> + struct mtk_iommu_data *data)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *ofnode;
> + struct resource *res;
> + int i;
> +
> + ofnode = dev->of_node;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->base))
> + return PTR_ERR(data->base);
> +
> + data->irq = platform_get_irq(pdev, 0);
> + if (data->irq < 0)
> + return data->irq;
> +
> + data->bclk = devm_clk_get(dev, "bclk");
> + if (IS_ERR(data->bclk))
> + return PTR_ERR(data->bclk);
> +
> + data->larb_nr = of_count_phandle_with_args(
> + ofnode, "mediatek,larb", NULL);
> + if (data->larb_nr < 0)
> + return data->larb_nr;
> +
> + for (i = 0; i < data->larb_nr; i++) {
> + struct device_node *larbnode;
> + struct platform_device *plarbdev;
> +
> + larbnode = of_parse_phandle(ofnode, "mediatek,larb", i);
> + if (!larbnode)
> + return -EINVAL;
> +
> + plarbdev = of_find_device_by_node(larbnode);
> + of_node_put(larbnode);
> + if (!plarbdev)
> + return -EPROBE_DEFER;
> + data->larbdev[i] = &plarbdev->dev;
> + }
At a glance this seems like a job for of_parse_phandle_with_args, but I
may be missing something subtle.
> + return 0;
> +}
> +
[...]
> +static int mtk_iommu_init_domain_context(struct mtk_iommu_domain *dom)
> +{
> + int ret;
> +
> + if (dom->iop)
> + return 0;
> +
> + spin_lock_init(&dom->pgtlock);
> + dom->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS |
> + IO_PGTABLE_QUIRK_SHORT_SUPERSECTION |
> + IO_PGTABLE_QUIRK_SHORT_MTK;
> + dom->cfg.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
> + dom->cfg.ias = 32;
> + dom->cfg.oas = 32;
> + dom->cfg.tlb = &mtk_iommu_gather_ops;
> +
> + dom->iop = alloc_io_pgtable_ops(ARM_SHORT_DESC, &dom->cfg, dom);
> + if (!dom->iop) {
> + pr_err("Failed to alloc io pgtable\n");
> + return -EINVAL;
> + }
> +
> + /* Update our support page sizes bitmap */
> + mtk_iommu_ops.pgsize_bitmap = dom->cfg.pgsize_bitmap;
> +
> + ret = mtk_iommu_hw_init(dom);
> + if (ret)
> + free_io_pgtable_ops(dom->iop);
> +
> + return ret;
> +}
I don't see that you need the above function at all - since your
pagetable config is fixed and doesn't have any depency on which IOMMU
you're attaching to, can't you just do all of that straight away in
domain_alloc?
> +static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
> +{
> + struct mtk_iommu_domain *priv;
> +
> + /* We only support unmanaged domains for now */
> + if (type != IOMMU_DOMAIN_UNMANAGED)
> + return NULL;
Have you had a chance to play with the latest DMA mapping series now
that I've integrated the default domain changes? I think if you handled
IOMMU_DOMAIN_DMA requests here and made them always return the
(preallocated) private domain, you should be able to get rid of the
tricks in attach_device completely.
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return NULL;
> +
> + priv->domain.geometry.aperture_start = 0;
> + priv->domain.geometry.aperture_end = (1ULL << 32) - 1;
DMA_BIT_MASK(32) ?
> + priv->domain.geometry.force_aperture = true;
> +
> + return &priv->domain;
> +}
> +
[...]
> +static int mtk_iommu_add_device(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom;
> + struct iommu_group *group;
> + struct mtk_iommu_client_priv *devhead;
> + int ret;
> +
> + if (!dev->archdata.dma_ops)/* Not a iommu client device */
> + return -ENODEV;
I think archdata.dma_ops is the wrong thing to test here. It would be
better to use archdata.iommu, since you go on to dereference that
unconditionally anyway, plus then you only depend on your own of_xlate
behaviour, rather than some quirk of the arch code (which could quietly
change in future).
> + group = iommu_group_get(dev);
> + if (!group) {
> + group = iommu_group_alloc();
> + if (IS_ERR(group)) {
> + dev_err(dev, "Failed to allocate IOMMU group\n");
> + return PTR_ERR(group);
> + }
> + }
> +
> + ret = iommu_group_add_device(group, dev);
> + if (ret) {
> + dev_err(dev, "Failed to add IOMMU group\n");
> + goto err_group_put;
> + }
> +
> + /* get the mtk_iommu_domain from the iommu device */
> + devhead = dev->archdata.iommu;
> + mtkdom = devhead->imudev->archdata.iommu;
> + ret = iommu_attach_group(&mtkdom->domain, group);
> + if (ret)
> + dev_err(dev, "Failed to attach IOMMU group\n");
> +
> +err_group_put:
> + iommu_group_put(group);
> + return ret;
> +}
> +
[...]
> +static struct iommu_ops mtk_iommu_ops = {
> + .domain_alloc = mtk_iommu_domain_alloc,
> + .domain_free = mtk_iommu_domain_free,
> + .attach_dev = mtk_iommu_attach_device,
> + .detach_dev = mtk_iommu_detach_device,
> + .map = mtk_iommu_map,
> + .unmap = mtk_iommu_unmap,
> + .map_sg = default_iommu_map_sg,
> + .iova_to_phys = mtk_iommu_iova_to_phys,
> + .add_device = mtk_iommu_add_device,
> + .of_xlate = mtk_iommu_of_xlate,
> + .pgsize_bitmap = -1UL,
As above, if you know the hardware only supports a single descriptor
format with a fixed set of page sizes, why not just represent that directly?
> +};
> +
[...]
> +static int mtk_iommu_suspend(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
> + struct mtk_iommu_data *data = mtkdom->data;
> + void __iomem *base = data->base;
> + unsigned int i = 0;
> +
> + data->reg[i++] = readl(base + REG_MMU_PT_BASE_ADDR);
Redundancy nit: any particular reason for saving this here rather than
simply restoring it from mtkdom->cfg.arm_short_cfg.ttbr[0] on resume?
> + data->reg[i++] = readl(base + REG_MMU_STANDARD_AXI_MODE);
> + data->reg[i++] = readl(base + REG_MMU_DCM_DIS);
> + data->reg[i++] = readl(base + REG_MMU_CTRL_REG);
> + data->reg[i++] = readl(base + REG_MMU_IVRP_PADDR);
> + data->reg[i++] = readl(base + REG_MMU_INT_CONTROL0);
> + data->reg[i++] = readl(base + REG_MMU_INT_MAIN_CONTROL);
Nit: given that these are fairly arbitrary discontiguous registers so
you can't have a simple loop over the array, it might be clearer to
replace the array with an equivalent struct, e.g.:
struct mtk_iommu_suspend_regs {
u32 standard_axi_mode;
u32 dcm_dis;
...
}
...
data->reg.dcm_dis = readl(base + REG_MMU_DCM_DIS);
...
then since you refer to everything by name you don't have to worry about
the length and order of array elements if anything ever changes.
> + return 0;
> +}
> +
> +static int mtk_iommu_resume(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
> + struct mtk_iommu_data *data = mtkdom->data;
> + void __iomem *base = data->base;
> + unsigned int i = 0;
> +
> + writel(data->reg[i++], base + REG_MMU_PT_BASE_ADDR);
> + writel(data->reg[i++], base + REG_MMU_STANDARD_AXI_MODE);
> + writel(data->reg[i++], base + REG_MMU_DCM_DIS);
> + writel(data->reg[i++], base + REG_MMU_CTRL_REG);
> + writel(data->reg[i++], base + REG_MMU_IVRP_PADDR);
> + writel(data->reg[i++], base + REG_MMU_INT_CONTROL0);
> + writel(data->reg[i++], base + REG_MMU_INT_MAIN_CONTROL);
> + return 0;
> +}
Other than that though, modulo the issues with the pagetable code I
think this part of the driver is shaping up quite nicely.
Robin.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists