Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.

On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote:

On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi =20
[off-list ref] wrote:

quoted

+}
+
+static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
+{

=20
subwin_cnt should probably be an unsigned int.
=20
This function needs to be documented.  What value is being returned?

spaact offset (yes, this needs to be documented)

quoted

+       unsigned long spaace_addr;
+
+       spaace_addr =3D gen_pool_alloc(spaace_pool, subwin_cnt * =20

sizeof(paace_t));

quoted

+       if (!spaace_addr)
+               return ULONG_MAX;

=20
What's wrong with returning 0 on error?

0 is a valid spaact offset

quoted

+
+       return (spaace_addr - (unsigned long)spaact) / =20

(sizeof(paace_t));
=20
Is this supposed to be a virtual address?  If so, then return void*
instead of an unsigned long.

It's not a virtual address.  How often does subtraction followed by =20
division result in a valid virtual address?

quoted

+int  pamu_update_paace_stash(int liodn, u32 subwin, u32 value)

Whitespace

quoted

+#define PAMU_PAGE_SHIFT 12
+#define PAMU_PAGE_SIZE  4096ULL

=20
4096ULL?  Why not just 4096?

This lets it be used in phys_addr_t expressions without needing casts
everywhere or dropping bits.

quoted

+/* This bitmap advertises the page sizes supported by PAMU hardware
+ * to the IOMMU API.
+ */
+#define FSL_PAMU_PGSIZES       (~0xFFFUL)

=20
There should be a better way to define this.  ~(PAMU_PAGE_SIZE-1) =20
maybe?

Is it even true?  We don't support IOMMU pages larger than the SoC can
address.

The (~0xFFFUL) version also discards some valid IOMMU page sizes on
32-bit kernels.  One use case for windows larger than the CPU virtual
address space is creating one big identity-map window to effectively
disable translation.  If we're to support that, the size of =20
pgsize_bitmap
will need to change as well.

quoted

+static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
+{
+       u32 subwin_cnt =3D dma_domain->subwin_cnt;
+       unsigned long rpn;
+       int ret =3D 0, i;
+
+       if (subwin_cnt) {
+               struct dma_subwindow *sub_win_ptr =3D
+                                       &dma_domain->sub_win_arr[0];
+               for (i =3D 0; i < subwin_cnt; i++) {
+                       if (sub_win_ptr[i].valid) {
+                               rpn =3D sub_win_ptr[i].paddr >>
+                                        PAMU_PAGE_SHIFT,
+                               spin_lock(&iommu_lock);
+                               ret =3D pamu_config_spaace(liodn, =20

subwin_cnt, i,

quoted

+                                                        =20

sub_win_ptr[i].size,

quoted

+                                                        -1,
+                                                        rpn,
+                                                        =20

dma_domain->snoop_id,

quoted

+                                                        =20

dma_domain->stash_id,

quoted

+                                                        (i > 0) ? =20

1 : 0,

quoted

+                                                        =20

sub_win_ptr[i].prot);

quoted

+                               spin_unlock(&iommu_lock);
+                               if (ret) {
+                                       pr_err("PAMU SPAACE =20

configuration failed for liodn %d\n",

quoted

+                                                liodn);
+                                       return ret;
+                               }
+                       }
+               }

Break up that nesting with some subfunctions.

quoted

+       while (!list_empty(&dma_domain->devices)) {
+               info =3D list_entry(dma_domain->devices.next,
+                       struct device_domain_info, link);
+               remove_domain_ref(info, dma_domain->subwin_cnt);
+       }

=20
I wonder if you should use list_for_each_safe() instead.

The above is simpler if you're destroying the entire list.

quoted

+}
+
+static int configure_domain_dma_state(struct fsl_dma_domain =20

*dma_domain, int enable)
=20
bool enable
=20
Finally, please CC: me on all IOMMU and PAMU patches you post =20
upstream.

Me too.

-Scott=

-----Original Message-----
From: Wood Scott-B07421
Sent: Tuesday, October 23, 2012 5:23 AM
To: Tabi Timur-B04825
Cc: Sethi Varun-B16395; joerg.roedel@amd.com; iommu@lists.linux-
foundation.org; linuxppc-dev@lists.ozlabs.org; linux-
kernel@vger.kernel.org
Subject: Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU
API implementation.
=20
On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote:

quoted

On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi
[off-list ref] wrote:

quoted

+}
+
+static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt) {

subwin_cnt should probably be an unsigned int.

This function needs to be documented.  What value is being returned?

=20
spaact offset (yes, this needs to be documented)

[Sethi Varun-B16395] Ok.

=20

quoted

quoted

+/* This bitmap advertises the page sizes supported by PAMU hardware
+ * to the IOMMU API.
+ */
+#define FSL_PAMU_PGSIZES       (~0xFFFUL)

There should be a better way to define this.  ~(PAMU_PAGE_SIZE-1)
maybe?

=20
Is it even true?  We don't support IOMMU pages larger than the SoC can
address.
=20
The (~0xFFFUL) version also discards some valid IOMMU page sizes on 32-
bit kernels.  One use case for windows larger than the CPU virtual
address space is creating one big identity-map window to effectively
disable translation.  If we're to support that, the size of pgsize_bitmap
will need to change as well.
=20

[Sethi Varun-B16395] Correct, this needs to be fixed. I will try to address=
 this
In a separate patch (would require changes to iommu_map).

quoted

quoted

+static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
+{
+       u32 subwin_cnt =3D dma_domain->subwin_cnt;
+       unsigned long rpn;
+       int ret =3D 0, i;
+
+       if (subwin_cnt) {
+               struct dma_subwindow *sub_win_ptr =3D
+                                       &dma_domain->sub_win_arr[0];
+               for (i =3D 0; i < subwin_cnt; i++) {
+                       if (sub_win_ptr[i].valid) {
+                               rpn =3D sub_win_ptr[i].paddr >>
+                                        PAMU_PAGE_SHIFT,
+                               spin_lock(&iommu_lock);
+                               ret =3D pamu_config_spaace(liodn,

subwin_cnt, i,

quoted

+

sub_win_ptr[i].size,

quoted

+                                                        -1,
+                                                        rpn,
+

dma_domain->snoop_id,

quoted

+

dma_domain->stash_id,

quoted

+                                                        (i > 0) ?

1 : 0,

quoted

+

sub_win_ptr[i].prot);

quoted

+                               spin_unlock(&iommu_lock);
+                               if (ret) {
+                                       pr_err("PAMU SPAACE

configuration failed for liodn %d\n",

quoted

+                                                liodn);
+                                       return ret;
+                               }
+                       }
+               }

=20
Break up that nesting with some subfunctions.
=20

quoted

quoted

+       while (!list_empty(&dma_domain->devices)) {
+               info =3D list_entry(dma_domain->devices.next,
+                       struct device_domain_info, link);
+               remove_domain_ref(info, dma_domain->subwin_cnt);
+       }

I wonder if you should use list_for_each_safe() instead.

=20
The above is simpler if you're destroying the entire list.
=20

quoted

quoted

+}
+
+static int configure_domain_dma_state(struct fsl_dma_domain

*dma_domain, int enable)

bool enable

Finally, please CC: me on all IOMMU and PAMU patches you post
upstream.

=20
Me too.

[Sethi Varun-B16395] Sure.

-Varun