Message-ID: <CALRxmdD6PTVvk2PpcPPEqh74E-=qTivmS7Gr8Mxeobd2Jgso0A@mail.gmail.com>
Date:	Wed, 27 Feb 2013 18:03:41 -0600
From:	Stuart Yoder <b08248@...il.com>
To:	Varun Sethi <Varun.Sethi@...escale.com>
Cc:	iommu@...ts.linux-foundation.org, linuxppc-dev@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org, Scott Wood <scottwood@...escale.com>,
	Joerg Roedel <joro@...tes.org>,
	Stuart Yoder <stuart.yoder@...escale.com>
Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.

Some more comments...

On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi <Varun.Sethi@...escale.com> wrote:
> +/* Handling access violations */
> +#define make64(high, low) (((u64)(high) << 32) | (low))
> +
> +struct pamu_isr_data {
> +       void __iomem *pamu_reg_base;    /* Base address of PAMU regs*/
> +       unsigned int count;             /* The number of PAMUs */
> +};
> +
> +static struct paace *ppaact;
> +static struct paace *spaact;
> +static struct ome *omt;
> +
> +/* maximum subwindows permitted per liodn */
> +unsigned int max_subwindow_count;
> +/* Number of SPAACT entries */
> +unsigned long max_subwins;

I don't like that these variables are not static... and they are
referenced directly
from code in fsl_pamu_domain.c.  It would be better if fsl_pamu_domain.c
called an accessor function-- like pamu_get_max_subwins.

> +/* Pool for fspi allocation */
> +struct gen_pool *spaace_pool;

spaace_pool should be static?

I'm wondering if you should change pamu_isr_data into a more
general struct analagous to struct intel_iommu.   You could
put in there the max # of subwins, etc.   You could then
provide an accessor to get at that data.

[cut]
> +/**
> + * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves subwindows
> + *                                required for primary PAACE in the secondary
> + *                                PAACE table.
> + * @subwin_cnt: Number of subwindows to be reserved.
> + *
> + * A PPAACE entry may have a number of associated subwindows. A subwindow
> + * corresponds to a SPAACE entry in the SPAACT table. Each PAACE entry stores
> + * the index (fspi) of the first SPAACE entry in the SPAACT table. This
> + * function returns the index of the first SPAACE entry. The remaining
> + * SPAACE entries are reserved contiguously from that index.
> + *
> + * Returns a valid fspi index in the range of 0 - max_subwins on success.
> + * If no SPAACE entry is available or the allocator can not reserve the required
> + * number of contiguous entries function returns ULONG_MAX indicating a failure.
> + *
> +*/
> +static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
> +{
> +       unsigned long spaace_addr;
> +
> +       spaace_addr = gen_pool_alloc(spaace_pool, subwin_cnt * sizeof(struct paace));
> +       if (!spaace_addr)
> +               return ULONG_MAX;
> +
> +       return (spaace_addr - (unsigned long)spaact) / (sizeof(struct paace));
> +}

In order to keep things symmetric (with the free function) can we just
call the above
function:

   pamu_alloc_subwins()

> +/* Release the subwindows reserved for a particular LIODN */
> +void pamu_free_subwins(int liodn)
> +{
> +       struct paace *ppaace;
> +       u32 subwin_cnt, size;
> +
> +       ppaace = pamu_get_ppaace(liodn);
> +       if (!ppaace) {
> +               pr_err("Invalid liodn entry\n");
> +               return;
> +       }
> +
> +       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(struct paace);
> +               gen_pool_free(spaace_pool, (unsigned long)&spaact[ppaace->fspi], size);
> +               set_bf(ppaace->addr_bitfields, PPAACE_AF_MW, 0);
> +       }
> +}

[cut]

> +/**
> + * get_stash_id - Returns stash destination id corresponding to a
> + *                cache type and vcpu.
> + * @stash_dest_hint: L1, L2 or L3
> + * @vcpu: vpcu target for a particular cache type.
> + *
> + * Returs stash on success or ~(u32)0 on failure.
> + *
> + */
> +u32 get_stash_id(u32 stash_dest_hint, u32 vcpu)
> +{

The stash dest is really not a hint, right?  It's the requested stash
destination.  So maybe just drop 'hint' from the name.

The CPU here is really a physical CPU number and has nothing to do
with vcpus I think.  vcpu implies the index is inside a virtual machine...but
this API is generic and may or may not be used with KVM.

> +
> +/*
> + * Get the maximum number of PAACT table entries
> + * and subwindows supported by PAMU
> + */
> +static void get_pamu_cap_values(unsigned long pamu_reg_base)
> +{
> +       u32 pc_val;
> +
> +       pc_val = in_be32((u32 *)(pamu_reg_base + PAMU_PC3));
> +       /* Maximum number of subwindows per liodn */
> +       max_subwindow_count = 1 << (1 + PAMU_PC3_MWCE(pc_val));
> +       /* Total number of SPACCT entries */
> +       max_subwins = PAACE_NUMBER_ENTRIES * max_subwindow_count;
> +}

If you follow the suggestion at the top of this file, this function
would become something like--  init_pamu_capabilities().   And then
create an accessor function to access max subwins, etc.

Also, BTW, I don't see any support for the DOMAIN_ATTR_WINDOWS
attribute in your patch.  Was that coming in a later patch?

[cut

> +static int __init fsl_pamu_probe(struct platform_device *pdev)
> +{
> +       void __iomem *pamu_regs = NULL;
> +       struct ccsr_guts __iomem *guts_regs = NULL;
> +       u32 pamubypenr, pamu_counter;
> +       unsigned long pamu_reg_off;
> +       unsigned long pamu_reg_base;
> +       struct pamu_isr_data *data;
> +       struct device_node *guts_node;
> +       u64 size;
> +       struct page *p;
> +       int ret = 0;
> +       int irq;
> +       phys_addr_t ppaact_phys;
> +       phys_addr_t spaact_phys;
> +       phys_addr_t omt_phys;
> +       size_t mem_size = 0;
> +       unsigned int order = 0;
> +       u32 csd_port_id = 0;
> +       unsigned i;
> +       /*
> +        * enumerate all PAMUs and allocate and setup PAMU tables
> +        * for each of them,
> +        * NOTE : All PAMUs share the same LIODN tables.
> +        */
> +
> +       pamu_regs = of_iomap(pdev->dev.of_node, 0);
> +       if (!pamu_regs) {
> +               dev_err(&pdev->dev, "ioremap of PAMU node failed\n");
> +               return -ENOMEM;

Any reason not to be consistent with the other error handling-- set ret and
goto error"?

> +       }
> +       of_get_address(pdev->dev.of_node, 0, &size, NULL);
> +
> +       irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +       if (irq == NO_IRQ) {
> +               dev_warn(&pdev->dev, "no interrupts listed in PAMU node\n");
> +               goto error;
> +       }
> +
> +       data = kzalloc(sizeof(struct pamu_isr_data), GFP_KERNEL);
> +       if (!data) {
> +               iounmap(pamu_regs);
> +               return -ENOMEM;

Any reason not to be consistent with the other error handling-- set ret and
goto error"?


Stuart
--
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