Re: [RFC 1/6] omap: iommu: generic iommu api migration

Hi Ohad,

Thanks for the patch.

On Friday 03 June 2011 00:27:38 Ohad Ben-Cohen wrote:

Migrate OMAP's iommu to the generic iommu api, so users can stay
generic, and non-omap-specific code can be removed and eventually
consolidated into a generic framework.

Tested on both OMAP3 and OMAP4.

Signed-off-by: Ohad Ben-Cohen <redacted>

[snip]

[snip]

+static int omap_iommu_domain_init(struct iommu_domain *domain)
+{
+	struct omap_iommu_domain *omap_domain;
+
+	omap_domain = kzalloc(sizeof(*omap_domain), GFP_KERNEL);
+	if (!omap_domain) {
+		pr_err("kzalloc failed\n");
+		goto fail_nomem;
+	}
+
+	omap_domain->pgtable = (u32 *)__get_free_pages(GFP_KERNEL,
+					get_order(IOPGD_TABLE_SIZE));
+	if (!omap_domain->pgtable) {
+		pr_err("__get_free_pages failed\n");
+		goto fail_nomem;
+	}
+
+	BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));

Either __get_free_pages() guarantees that the allocated memory will be aligned 
on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is unnecessary, or 
doesn't offer such guarantee, in which case the BUG_ON() will oops randomly. 
In both cases BUG_ON() should probably be avoided.

+	memset(omap_domain->pgtable, 0, IOPGD_TABLE_SIZE);
+	clean_dcache_area(omap_domain->pgtable, IOPGD_TABLE_SIZE);
+	mutex_init(&omap_domain->lock);
+
+	domain->priv = omap_domain;
+
+	return 0;
+
+fail_nomem:
+	kfree(omap_domain);
+	return -ENOMEM;
+}
+
+/* assume device was already detached */
+static void omap_iommu_domain_destroy(struct iommu_domain *domain)
+{
+	struct omap_iommu_domain *omap_domain = domain->priv;
+
+	domain->priv = NULL;
+
+	kfree(omap_domain);

This leaks omap_domain->pgtable.

The free_pages() call in omap_iommu_remove() should be removed, as 
omap_iommu_probe() doesn't allocate the pages table anymore. You can also 
remove the the struct iommu::iopgd field.

+}

+
+static phys_addr_t omap_iommu_iova_to_phys(struct iommu_domain *domain,
+					  unsigned long da)
+{
+	struct omap_iommu_domain *omap_domain = domain->priv;
+	struct iommu *oiommu = omap_domain->iommu_dev;
+	struct device *dev = oiommu->dev;
+	u32 *pgd, *pte;
+	phys_addr_t ret = 0;
+
+	iopgtable_lookup_entry(oiommu, da, &pgd, &pte);
+
+	if (pte) {
+		if (iopte_is_small(*pte))
+			ret = omap_iommu_translate(*pte, da, IOPTE_MASK);
+		else if (iopte_is_large(*pte))
+			ret = omap_iommu_translate(*pte, da, IOLARGE_MASK);
+		else
+			dev_err(dev, "bogus pte 0x%x", *pte);
+	} else {
+		if (iopgd_is_section(*pgd))
+			ret = omap_iommu_translate(*pgd, da, IOSECTION_MASK);
+		else if (iopgd_is_super(*pgd))
+			ret = omap_iommu_translate(*pgd, da, IOSUPER_MASK);
+		else
+			dev_err(dev, "bogus pgd 0x%x", *pgd);
+	}
+
+	return ret;

You return 0 in the bogus pte/pgd cases. Is that intentional ?

+}

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On Tue, Jun 7, 2011 at 12:22 PM, Laurent Pinchart
[off-list ref] wrote:

quoted

+ ? ? BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));

Either __get_free_pages() guarantees that the allocated memory will be aligned
on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is unnecessary, or
doesn't offer such guarantee, in which case the BUG_ON() will oops randomly.

Curious, does it oops randomly today ?
(i just copied this from omap_iommu_probe, where it always existed).

It is a bit ugly though, and thinking on it again, 16KB is not that
big. We can just use kmalloc here, which does ensure the alignment
(or, better yet, kzalloc, and then ditch the memset).

In both cases BUG_ON() should probably be avoided.

I disagree; we must check this so user data won't be harmed (hardware
requirement), and if a memory allocation API fails to meet its
requirements - that's really bad and user data is again at stake (much
more will break, not only the iommu driver).

This leaks omap_domain->pgtable.

The free_pages() call in omap_iommu_remove() should be removed, as
omap_iommu_probe() doesn't allocate the pages table anymore.

thanks !

You can also remove the the struct iommu::iopgd field.

No, I can't; it's used when the device is attached to an address space domain.

You return 0 in the bogus pte/pgd cases. Is that intentional ?

Yes, that's probably the most (if any) reasonable value to return here
(all other iommu implementations are doing so too).

Thanks,
Ohad.

Hi Ohad,

On Tuesday 07 June 2011 13:19:05 Ohad Ben-Cohen wrote:

On Tue, Jun 7, 2011 at 12:22 PM, Laurent Pinchart wrote:

quoted

quoted

+     BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));

Either __get_free_pages() guarantees that the allocated memory will be
aligned on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is
unnecessary, or doesn't offer such guarantee, in which case the BUG_ON()
will oops randomly.

Curious, does it oops randomly today ?
(i just copied this from omap_iommu_probe, where it always existed).

No that I know of :-)

It is a bit ugly though, and thinking on it again, 16KB is not that
big. We can just use kmalloc here, which does ensure the alignment
(or, better yet, kzalloc, and then ditch the memset).

quoted

In both cases BUG_ON() should probably be avoided.

I disagree; we must check this so user data won't be harmed (hardware
requirement), and if a memory allocation API fails to meet its
requirements - that's really bad and user data is again at stake (much
more will break, not only the iommu driver).

My point is that if the allocator guarantees the alignment (not as a side 
effect of the implementation, but per its API) there's no need to check it 
again. As the alignement is required, we need an allocator that guarantees it 
anyway.

quoted

This leaks omap_domain->pgtable.

The free_pages() call in omap_iommu_remove() should be removed, as
omap_iommu_probe() doesn't allocate the pages table anymore.

thanks !

quoted

You can also remove the the struct iommu::iopgd field.

No, I can't; it's used when the device is attached to an address space
domain.

Right, my bad.

quoted

You return 0 in the bogus pte/pgd cases. Is that intentional ?

Yes, that's probably the most (if any) reasonable value to return here
(all other iommu implementations are doing so too).

-- 
Regards,

Laurent Pinchart

On Tue, Jun 7, 2011 at 2:40 PM, Laurent Pinchart
[off-list ref] wrote:

My point is that if the allocator guarantees the alignment (not as a side
effect of the implementation, but per its API) there's no need to check it
again. As the alignement is required, we need an allocator that guarantees it
anyway.

I understand, but I'd still prefer to have an explicit check that the
hardware alignment requirement is met.

There's no cost in doing that (it's a cold path), and even if it would
only fail once and with an extremely broken kernel - it's worth it.
Will save huge amount of debugging pain (think of the poor guy that
will have to debug this...).

Thanks,
Ohad.