RE: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, October 23, 2012 5:23 AM
> To: Tabi Timur-B04825
> Cc: Sethi Varun-B16395; joerg.roe...@amd.com; iommu@lists.linux-
> foundation.org; linuxppc-...@lists.ozlabs.org; linux-
> ker...@vger.kernel.org
> Subject: 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
> > wrote:
> > > +}
> > > +
> > > +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?
>
> spaact offset (yes, this needs to be documented)
[Sethi Varun-B16395] Ok.
>
> > > +/* 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?
>
> 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 pgsize_bitmap
> will need to change as well.
>
[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).
> > > +static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
> > > +{
> > > + u32 subwin_cnt = dma_domain->subwin_cnt;
> > > + unsigned long rpn;
> > > + int ret = 0, i;
> > > +
> > > + if (subwin_cnt) {
> > > + struct dma_subwindow *sub_win_ptr =
> > > + &dma_domain->sub_win_arr[0];
> > > + for (i = 0; i < subwin_cnt; i++) {
> > > + if (sub_win_ptr[i].valid) {
> > > + rpn = sub_win_ptr[i].paddr >>
> > > +PAMU_PAGE_SHIFT,
> > > + spin_lock(&iommu_lock);
> > > + ret = pamu_config_spaace(liodn,
> > subwin_cnt, i,
> > > +
> > sub_win_ptr[i].size,
> > > +-1,
> > > +rpn,
> > > +
> > dma_domain->snoop_id,
> > > +
> > dma_domain->stash_id,
> > > +(i > 0) ?
> > 1 : 0,
> > > +
> > sub_win_ptr[i].prot);
> > > + spin_unlock(&iommu_lock);
> > > + if (ret) {
> > > + pr_err("PAMU SPAACE
> > configuration failed for liodn %d\n",
> > > +liodn);
> > > + return ret;
> > > + }
> > > + }
> > > + }
>
> Break up that nesting with some subfunctions.
>
> > > + while (!list_empty(&dma_domain->devices)) {
> > > + info = 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.
>
> The above is simpler if you're destroying the entire list.
>
> > > +}
> > > +
> > > +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.
>
> Me too.
[Sethi Varun-B16395] Sure.
-Varun
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
> -Original Message-
> From: Tabi Timur-B04825
> Sent: Tuesday, October 23, 2012 2:48 AM
> To: Sethi Varun-B16395
> Cc: joerg.roe...@amd.com; iommu@lists.linux-foundation.org; linuxppc-
> d...@lists.ozlabs.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU
> API implementation.
>
> On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi
> wrote:
>
> > + * Copyright (C) 2012 Freescale Semiconductor, Inc.
>
> Copyright 2012 Freescale Semiconductor, Inc.
>
[Sethi Varun-B16395] Have followed similar convention elsewhere i.e. added (C).
> > + *
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "fsl_pamu.h"
> > +
> > +/* PAMU bypass enbale register contains control bits for
> > + * enabling bypass mode on each PAMU.
> > + */
>
> /*
> * Linux multi-line comments
> * look like this.
> */
>
> > +#define PAMUBYPENR 0x604
>
> Update struct ccsr_guts instead.
>
> http://patchwork.ozlabs.org/patch/141649/
>
[Sethi Varun-B16395] Ok.
> > +
> > +/* define indexes for each operation mapping scenario */
> > +#define OMI_QMAN0x00
> > +#define OMI_FMAN0x01
> > +#define OMI_QMAN_PRIV 0x02
> > +#define OMI_CAAM0x03
> > +
> > +static paace_t *ppaact;
> > +static paace_t *spaact;
> > +static struct ome *omt;
> > +unsigned int max_subwindow_count;
> > +
> > +struct gen_pool *spaace_pool;
> > +
> > +static paace_t *pamu_get_ppaace(int liodn) {
> > + if (!ppaact) {
> > + pr_err("PPAACT doesn't exist\n");
>
> pr_err("fsl-pamu: PPAACT has not been initialized\n");
>
> Make sure ALL pr_xxx() messages in this file start with "fsl-pamu: "
>
> > + return NULL;
> > + }
> > +
> > + return &ppaact[liodn];
>
> Bounds checking?
>
[Sethi Varun-B16395] Ok.
> > +}
> > +
> > +/** Sets validation bit of PACCE
> > + *
> > + * @parm[in] liodn PAACT index for desired PAACE
> > + *
> > + * @return Returns 0 upon success else error code < 0 returned */
> > +int pamu_enable_liodn(int liodn) {
> > + paace_t *ppaace;
> > +
> > + ppaace = pamu_get_ppaace(liodn);
> > + if (!ppaace)
> > + return -ENOENT;
>
> error message?
[Sethi Varun-B16395] Have error messages at places from where the function is
invoked.
>
> > +
> > + if (!get_bf(ppaace->addr_bitfields, PPAACE_AF_WSE)) {
> > + pr_err("liodn %d not configured\n", liodn);
> > + return -EINVAL;
> > + }
> > +
> > + /* Ensure that all other stores to the ppaace complete first */
> > + mb();
> > +
> > + ppaace->addr_bitfields |= PAACE_V_VALID;
> > + mb();
> > +
> > + return 0;
> > +}
> > +
> > +/** Clears validation bit of PACCE
> > + *
> > + * @parm[in] liodn PAACT index for desired PAACE
> > + *
> > + * @return Returns 0 upon success else error code < 0 returned
>
> This is not proper kerneldoc format.
[Sethi Varun-B16395] What format must be used? Can you point me to a relevant
file.
>
> > + */
> > +int pamu_disable_liodn(int liodn)
> > +{
> > + paace_t *ppaace;
> > +
> > + ppaace = pamu_get_ppaace(liodn);
> > + if (!ppaace)
> > + return -ENOENT;
>
> error message?
[Sethi Varun-B16395] Error message at the point of invocation.
>
> > +
> > + set_bf(ppaace->addr_bitfields, PAACE_AF_V, PAACE_V_INVALID);
> > + mb();
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static unsigned int map_addrspace_size_to_wse(phys_addr_t
> > +addrspace_size) {
> > + BUG_ON((addrspace_size & (addrspace_size - 1)));
> > +
> > + /* window size is 2^(WSE+1) bytes */
> > + return __ffs(addrspace_size >> PAMU_PAGE_SHIFT) +
> > +PAMU_PAGE_SHIFT - 1; }
> > +
> > +static unsigned int map_subwindow_cnt_to_wce(u32 subwindow_cnt) {
> > + /* window count is 2^(WCE+1) bytes */
> > + return __ffs(subwindow_cnt) - 1; }
> > +
> > +static void pamu_setup_default_xfer_to_host_ppaace(paace_t *p
Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi wrote:
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "fsl_pamu.h"
> +
> +/* PAMU bypass enbale register contains control bits for
> + * enabling bypass mode on each PAMU.
> + */
/*
* Linux multi-line comments
* look like this.
*/
> +#define PAMUBYPENR 0x604
Update struct ccsr_guts instead.
http://patchwork.ozlabs.org/patch/141649/
> +
> +/* define indexes for each operation mapping scenario */
> +#define OMI_QMAN0x00
> +#define OMI_FMAN0x01
> +#define OMI_QMAN_PRIV 0x02
> +#define OMI_CAAM0x03
> +
> +static paace_t *ppaact;
> +static paace_t *spaact;
> +static struct ome *omt;
> +unsigned int max_subwindow_count;
> +
> +struct gen_pool *spaace_pool;
> +
> +static paace_t *pamu_get_ppaace(int liodn)
> +{
> + if (!ppaact) {
> + pr_err("PPAACT doesn't exist\n");
pr_err("fsl-pamu: PPAACT has not been initialized\n");
Make sure ALL pr_xxx() messages in this file start with "fsl-pamu: "
> + return NULL;
> + }
> +
> + return &ppaact[liodn];
Bounds checking?
> +}
> +
> +/** Sets validation bit of PACCE
> + *
> + * @parm[in] liodn PAACT index for desired PAACE
> + *
> + * @return Returns 0 upon success else error code < 0 returned
> + */
> +int pamu_enable_liodn(int liodn)
> +{
> + paace_t *ppaace;
> +
> + ppaace = pamu_get_ppaace(liodn);
> + if (!ppaace)
> + return -ENOENT;
error message?
> +
> + if (!get_bf(ppaace->addr_bitfields, PPAACE_AF_WSE)) {
> + pr_err("liodn %d not configured\n", liodn);
> + return -EINVAL;
> + }
> +
> + /* Ensure that all other stores to the ppaace complete first */
> + mb();
> +
> + ppaace->addr_bitfields |= PAACE_V_VALID;
> + mb();
> +
> + return 0;
> +}
> +
> +/** Clears validation bit of PACCE
> + *
> + * @parm[in] liodn PAACT index for desired PAACE
> + *
> + * @return Returns 0 upon success else error code < 0 returned
This is not proper kerneldoc format.
> + */
> +int pamu_disable_liodn(int liodn)
> +{
> + paace_t *ppaace;
> +
> + ppaace = pamu_get_ppaace(liodn);
> + if (!ppaace)
> + return -ENOENT;
error message?
> +
> + set_bf(ppaace->addr_bitfields, PAACE_AF_V, PAACE_V_INVALID);
> + mb();
> +
> + return 0;
> +}
> +
> +
> +static unsigned int map_addrspace_size_to_wse(phys_addr_t addrspace_size)
> +{
> + BUG_ON((addrspace_size & (addrspace_size - 1)));
> +
> + /* window size is 2^(WSE+1) bytes */
> + return __ffs(addrspace_size >> PAMU_PAGE_SHIFT) + PAMU_PAGE_SHIFT - 1;
> +}
> +
> +static unsigned int map_subwindow_cnt_to_wce(u32 subwindow_cnt)
> +{
> + /* window count is 2^(WCE+1) bytes */
> + return __ffs(subwindow_cnt) - 1;
> +}
> +
> +static void pamu_setup_default_xfer_to_host_ppaace(paace_t *ppaace)
> +{
> + set_bf(ppaace->addr_bitfields, PAACE_AF_PT, PAACE_PT_PRIMARY);
> +
> + set_bf(ppaace->domain_attr.to_host.coherency_required,
> PAACE_DA_HOST_CR,
> + PAACE_M_COHERENCE_REQ);
> +}
> +
> +static void pamu_setup_default_xfer_to_host_spaace(paace_t *spaace)
> +{
> + set_bf(spaace->addr_bitfields, PAACE_AF_PT, PAACE_PT_SECONDARY);
> + set_bf(spaace->domain_attr.to_host.coherency_required,
> PAACE_DA_HOST_CR,
> + PAACE_M_COHERENCE_REQ);
> +}
> +
> +static paace_t *pamu_get_spaace(u32 fspi_index, u32 wnum)
> +{
> + return &spaact[fspi_index + wnum];
bounds checking?
> +}
> +
> +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?
> + unsigned long spaace_addr;
> +
> + spaace_addr = gen_pool_alloc(spaace_pool, subwin_cnt *
> sizeof(paace_t));
> + if (!spaace_addr)
> + return ULONG_MAX;
What's wrong with returning 0 on error?
> +
> + return (spaace_addr - (unsigned long)spaact) / (sizeof(paace_t));
Is this supposed to be a virtual address? If so, then return void*
instead of an unsigned long.
> +}
> +
> +void pamu_free_subwins(int liodn)
> +{
> + paace_t *ppaace;
> + u32 subwin_cnt, size;
subwin_cnt should probably be an unsigned int.
> +
> + ppaace = pamu_get_ppaace(liodn);
> + if (!ppaace)
> + return;
error message
> +
> + if (get_bf(ppaace->addr_bitfields, PPAACE_AF_MW)) {
> + subwin_cnt = 1UL << (get_bf(ppaace->impl_attr, PAACE_IA_WCE)
> + 1);
> + size = (subwin_cnt - 1) * sizeof(paace_t);
> + gen_pool_free(spaace_pool, (unsigned
> long)&spaact[ppaace->fspi], size);
> +
