Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
flat view
From: Stuart Yoder <hidden>
Date: 2013-02-28 00:03:44
Also in:
linux-iommu, lkml
Some more comments... On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi [off-list ref] wrote:
quoted hunk ↗ jump to hunk
+/* 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]
quoted hunk ↗ jump to hunk
+/** + * 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()
quoted hunk ↗ jump to hunk
+/* 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]
quoted hunk ↗ jump to hunk
+/** + * 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.
quoted hunk ↗ jump to hunk
+ +/* + * 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
quoted hunk ↗ jump to hunk
+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"?
quoted hunk ↗ jump to hunk
+ } + 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