| Message ID | 20170306125739.19445-9-rrichter@cavium.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Robert, I don't see anywhere in this patch, code calls explicitly CMA API to allocate memory for device table. The CMA feature is an optional in kernel, and will be handled transparently inside the the DMA layer. It would be nicer to not mention CMA word in the commit subject. On 03/06/2017 06:57 AM, Robert Richter wrote: > The gicv3-its device table may have a size of up to 16MB. With 4k > pagesize the maximum size of memory allocation is 4MB. Use CMA for > allocation of large tables. Just say use devm_alloc_coherent() to allocate memory. > We use the device managed version of dma_alloc_coherent(). Thus, we > don't need to release it manually on device removal. > > Signed-off-by: Robert Richter <rrichter@cavium.com> > --- > drivers/irqchip/irq-gic-v3-its.c | 69 +++++++++++++++++++++++----------------- > 1 file changed, 40 insertions(+), 29 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index 6625b3a505f0..6d293a0165b0 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -21,6 +21,7 @@ > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/dma-iommu.h> > +#include <linux/dma-mapping.h> > #include <linux/interrupt.h> > #include <linux/irqdomain.h> > #include <linux/acpi_iort.h> > @@ -864,6 +865,7 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > u64 type = GITS_BASER_TYPE(val); > u32 alloc_pages; > void *base; > + dma_addr_t dma_handle; > u64 tmp; > > retry_alloc_baser: > @@ -876,13 +878,26 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > order = get_order(GITS_BASER_PAGES_MAX * psz); > } > > - base = (void *)devm_get_free_pages(&its->dev, GFP_KERNEL | __GFP_ZERO, > - order); > - if (!base) > + base = dmam_alloc_coherent(&its->dev, > + PAGE_ORDER_TO_SIZE(order), > + &dma_handle, > + GFP_KERNEL | __GFP_ZERO); Not just for 1st level device table, you have do a similar code change when allocating memory for 2nd level device table. > + > + if (!base && order >= MAX_ORDER) { > + order = MAX_ORDER - 1; > + dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u, no CMA memory available\n", > + its->device_ids, > + ilog2(PAGE_ORDER_TO_SIZE(order) / (int)esz)); > + goto retry_alloc_baser; > + } > + > + if (!base) { > + dev_err(&its->dev, "Failed to allocate device table\n"); > return -ENOMEM; > + } > > retry_baser: > - val = (virt_to_phys(base) | > + val = (dma_handle | > (type << GITS_BASER_TYPE_SHIFT) | > ((esz - 1) << GITS_BASER_ENTRY_SIZE_SHIFT) | > ((alloc_pages - 1) << GITS_BASER_PAGES_SHIFT) | > @@ -923,29 +938,28 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > goto retry_baser; > } > > - if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) { > - /* > - * Page size didn't stick. Let's try a smaller > - * size and retry. If we reach 4K, then > - * something is horribly wrong... > - */ > - devm_free_pages(&its->dev, (unsigned long)base); > - baser->base = NULL; > - > - switch (psz) { > - case SZ_16K: > - psz = SZ_4K; > - goto retry_alloc_baser; > - case SZ_64K: > - psz = SZ_16K; > - goto retry_alloc_baser; > + if (val != tmp) { > + dmam_free_coherent(&its->dev, PAGE_ORDER_TO_SIZE(order), > + base, dma_handle); > + > + if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) { > + /* > + * Page size didn't stick. Let's try a smaller > + * size and retry. If we reach 4K, then > + * something is horribly wrong... > + */ > + switch (psz) { > + case SZ_16K: > + psz = SZ_4K; > + goto retry_alloc_baser; > + case SZ_64K: > + psz = SZ_16K; > + goto retry_alloc_baser; > + } > } > - } > > - if (val != tmp) { > dev_err(&its->dev, "%s doesn't stick: %llx %llx\n", > its_base_type_string[type], val, tmp); > - devm_free_pages(&its->dev, (unsigned long)base); > return -ENXIO; > } > > @@ -1003,12 +1017,6 @@ static bool its_parse_baser_device(struct its_node *its, struct its_baser *baser > * feature is not supported by hardware. > */ > new_order = max_t(u32, get_order(esz << ids), new_order); > - if (new_order >= MAX_ORDER) { > - new_order = MAX_ORDER - 1; > - ids = ilog2(PAGE_ORDER_TO_SIZE(new_order) / (int)esz); > - dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u\n", > - its->device_ids, ids); > - } > > *order = new_order; > > @@ -1698,6 +1706,9 @@ static int __init its_init_one(struct its_node *its) > return err; > } > > + /* Setup dma_ops for dmam_alloc_coherent() */ > + arch_setup_dma_ops(&its->dev, 0, 0, NULL, true); > + Why you are hard-coding DMA coherent property to true here ? It breaks the MSI(x) functionally on systems where ITS hardware doesn't support coherency. > its_base = devm_ioremap(&its->dev, its->phys_base, its->phys_size); > if (!its_base) { > dev_warn(&its->dev, "Unable to map ITS registers\n");
On 14.03.17 12:40:45, Shanker Donthineni wrote: > I don't see anywhere in this patch, code calls explicitly CMA API to > allocate memory for device table. The CMA feature is an optional in > kernel, and will be handled transparently inside the the DMA > layer. It would be nicer to not mention CMA word in the commit > subject. Still CMA is *essential* and used for large tables. IMO this needs to be emphasized. That's the reason for using devm_alloc_coherent(). > On 03/06/2017 06:57 AM, Robert Richter wrote: > > The gicv3-its device table may have a size of up to 16MB. With 4k > > pagesize the maximum size of memory allocation is 4MB. Use CMA for > > allocation of large tables. > Just say use devm_alloc_coherent() to allocate memory. > > > We use the device managed version of dma_alloc_coherent(). Thus, we > > don't need to release it manually on device removal. > > > > Signed-off-by: Robert Richter <rrichter@cavium.com> > > --- > > drivers/irqchip/irq-gic-v3-its.c | 69 +++++++++++++++++++++++----------------- > > 1 file changed, 40 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > > index 6625b3a505f0..6d293a0165b0 100644 > > --- a/drivers/irqchip/irq-gic-v3-its.c > > +++ b/drivers/irqchip/irq-gic-v3-its.c > > @@ -876,13 +878,26 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > > order = get_order(GITS_BASER_PAGES_MAX * psz); > > } > > > > - base = (void *)devm_get_free_pages(&its->dev, GFP_KERNEL | __GFP_ZERO, > > - order); > > - if (!base) > > + base = dmam_alloc_coherent(&its->dev, > > + PAGE_ORDER_TO_SIZE(order), > > + &dma_handle, > > + GFP_KERNEL | __GFP_ZERO); > Not just for 1st level device table, you have do a similar code > change when allocating memory for 2nd level device table. The 2nd level tables are much smaller, so no need for dmam_alloc_coherent() there. Though, we could use device managed devm_get_free_pages() there too. > > + > > + if (!base && order >= MAX_ORDER) { > > + order = MAX_ORDER - 1; > > + dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u, no CMA memory available\n", > > + its->device_ids, > > + ilog2(PAGE_ORDER_TO_SIZE(order) / (int)esz)); > > + goto retry_alloc_baser; > > + } > > + > > + if (!base) { > > + dev_err(&its->dev, "Failed to allocate device table\n"); > > return -ENOMEM; > > + } > > @@ -1698,6 +1706,9 @@ static int __init its_init_one(struct its_node *its) > > return err; > > } > > > > + /* Setup dma_ops for dmam_alloc_coherent() */ > > + arch_setup_dma_ops(&its->dev, 0, 0, NULL, true); > > + > Why you are hard-coding DMA coherent property to true here ? It > breaks the MSI(x) functionally on systems where ITS hardware doesn't > support coherency. Aren't current ITS tables coherent only? Thanks, -Robert
On 15/03/17 18:37, Robert Richter wrote: > On 14.03.17 12:40:45, Shanker Donthineni wrote: > >> I don't see anywhere in this patch, code calls explicitly CMA API to >> allocate memory for device table. The CMA feature is an optional in >> kernel, and will be handled transparently inside the the DMA >> layer. It would be nicer to not mention CMA word in the commit >> subject. > > Still CMA is *essential* and used for large tables. IMO this needs to > be emphasized. That's the reason for using devm_alloc_coherent(). > >> On 03/06/2017 06:57 AM, Robert Richter wrote: >>> The gicv3-its device table may have a size of up to 16MB. With 4k >>> pagesize the maximum size of memory allocation is 4MB. Use CMA for >>> allocation of large tables. >> Just say use devm_alloc_coherent() to allocate memory. >> >>> We use the device managed version of dma_alloc_coherent(). Thus, we >>> don't need to release it manually on device removal. >>> >>> Signed-off-by: Robert Richter <rrichter@cavium.com> >>> --- >>> drivers/irqchip/irq-gic-v3-its.c | 69 +++++++++++++++++++++++----------------- >>> 1 file changed, 40 insertions(+), 29 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c >>> index 6625b3a505f0..6d293a0165b0 100644 >>> --- a/drivers/irqchip/irq-gic-v3-its.c >>> +++ b/drivers/irqchip/irq-gic-v3-its.c > >>> @@ -876,13 +878,26 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, >>> order = get_order(GITS_BASER_PAGES_MAX * psz); >>> } >>> >>> - base = (void *)devm_get_free_pages(&its->dev, GFP_KERNEL | __GFP_ZERO, >>> - order); >>> - if (!base) >>> + base = dmam_alloc_coherent(&its->dev, >>> + PAGE_ORDER_TO_SIZE(order), >>> + &dma_handle, >>> + GFP_KERNEL | __GFP_ZERO); > >> Not just for 1st level device table, you have do a similar code >> change when allocating memory for 2nd level device table. > > The 2nd level tables are much smaller, so no need for > dmam_alloc_coherent() there. > > Though, we could use device managed devm_get_free_pages() there too. > >>> + >>> + if (!base && order >= MAX_ORDER) { >>> + order = MAX_ORDER - 1; >>> + dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u, no CMA memory available\n", >>> + its->device_ids, >>> + ilog2(PAGE_ORDER_TO_SIZE(order) / (int)esz)); >>> + goto retry_alloc_baser; >>> + } >>> + >>> + if (!base) { >>> + dev_err(&its->dev, "Failed to allocate device table\n"); >>> return -ENOMEM; >>> + } > >>> @@ -1698,6 +1706,9 @@ static int __init its_init_one(struct its_node *its) >>> return err; >>> } >>> >>> + /* Setup dma_ops for dmam_alloc_coherent() */ >>> + arch_setup_dma_ops(&its->dev, 0, 0, NULL, true); >>> + > >> Why you are hard-coding DMA coherent property to true here ? It >> breaks the MSI(x) functionally on systems where ITS hardware doesn't >> support coherency. > > Aren't current ITS tables coherent only? No, there is no such guarantee. Actually, there is strictly no need for coherency, as the ITS tables are only written by the ITS itself, for its own purpose. The only things that may benefit from coherency are the property table and the command queue. Thanks, M.
On 15.03.17 18:46:22, Marc Zyngier wrote: > On 15/03/17 18:37, Robert Richter wrote: > > On 14.03.17 12:40:45, Shanker Donthineni wrote: > >>> @@ -1698,6 +1706,9 @@ static int __init its_init_one(struct its_node *its) > >>> return err; > >>> } > >>> > >>> + /* Setup dma_ops for dmam_alloc_coherent() */ > >>> + arch_setup_dma_ops(&its->dev, 0, 0, NULL, true); > >>> + > > > >> Why you are hard-coding DMA coherent property to true here ? It > >> breaks the MSI(x) functionally on systems where ITS hardware doesn't > >> support coherency. > > > > Aren't current ITS tables coherent only? > > No, there is no such guarantee. Actually, there is strictly no need for > coherency, as the ITS tables are only written by the ITS itself, for its > own purpose. So no need to change that, right? Thanks, -Robert
On 16/03/17 13:31, Robert Richter wrote: > On 15.03.17 18:46:22, Marc Zyngier wrote: >> On 15/03/17 18:37, Robert Richter wrote: >>> On 14.03.17 12:40:45, Shanker Donthineni wrote: > >>>>> @@ -1698,6 +1706,9 @@ static int __init its_init_one(struct its_node *its) >>>>> return err; >>>>> } >>>>> >>>>> + /* Setup dma_ops for dmam_alloc_coherent() */ >>>>> + arch_setup_dma_ops(&its->dev, 0, 0, NULL, true); >>>>> + >>> >>>> Why you are hard-coding DMA coherent property to true here ? It >>>> breaks the MSI(x) functionally on systems where ITS hardware doesn't >>>> support coherency. >>> >>> Aren't current ITS tables coherent only? >> >> No, there is no such guarantee. Actually, there is strictly no need for >> coherency, as the ITS tables are only written by the ITS itself, for its >> own purpose. > > So no need to change that, right? I don't think there is any. We just need to allocate memory with the relevant constraints (alignment and zeroing, mostly), and make sure we never access it directly. Of course, property tables and command queues would benefit from being allocated as DMA buffers, which would allow the cache flush to be dealt with at the DMA level. Thanks, M.
Hi Marc/Robert, On 03/16/2017 08:41 AM, Marc Zyngier wrote: > On 16/03/17 13:31, Robert Richter wrote: >> On 15.03.17 18:46:22, Marc Zyngier wrote: >>> On 15/03/17 18:37, Robert Richter wrote: >>>> On 14.03.17 12:40:45, Shanker Donthineni wrote: >>>>>> @@ -1698,6 +1706,9 @@ static int __init its_init_one(struct its_node *its) >>>>>> return err; >>>>>> } >>>>>> >>>>>> + /* Setup dma_ops for dmam_alloc_coherent() */ >>>>>> + arch_setup_dma_ops(&its->dev, 0, 0, NULL, true); >>>>>> + >>>>> Why you are hard-coding DMA coherent property to true here ? It >>>>> breaks the MSI(x) functionally on systems where ITS hardware doesn't >>>>> support coherency. >>>> Aren't current ITS tables coherent only? >>> No, there is no such guarantee. Actually, there is strictly no need for >>> coherency, as the ITS tables are only written by the ITS itself, for its >>> own purpose. >> So no need to change that, right? > I don't think there is any. We just need to allocate memory with the > relevant constraints (alignment and zeroing, mostly), and make sure we > never access it directly. Of course, property tables and command queues > would benefit from being allocated as DMA buffers, which would allow the > cache flush to be dealt with at the DMA level. Agree with Marc, only PROP tables and CMD queue buffers are touched by CPU during runtime. That means DMA coherent property set to true only when both the CMD_BASE (ITS_FLAGS_CMDQ_NEEDS_FLUSHING) and PROP_BASE (RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING) tables support coherency. > Thanks, > > M.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 6625b3a505f0..6d293a0165b0 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -21,6 +21,7 @@ #include <linux/delay.h> #include <linux/device.h> #include <linux/dma-iommu.h> +#include <linux/dma-mapping.h> #include <linux/interrupt.h> #include <linux/irqdomain.h> #include <linux/acpi_iort.h> @@ -864,6 +865,7 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, u64 type = GITS_BASER_TYPE(val); u32 alloc_pages; void *base; + dma_addr_t dma_handle; u64 tmp; retry_alloc_baser: @@ -876,13 +878,26 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, order = get_order(GITS_BASER_PAGES_MAX * psz); } - base = (void *)devm_get_free_pages(&its->dev, GFP_KERNEL | __GFP_ZERO, - order); - if (!base) + base = dmam_alloc_coherent(&its->dev, + PAGE_ORDER_TO_SIZE(order), + &dma_handle, + GFP_KERNEL | __GFP_ZERO); + + if (!base && order >= MAX_ORDER) { + order = MAX_ORDER - 1; + dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u, no CMA memory available\n", + its->device_ids, + ilog2(PAGE_ORDER_TO_SIZE(order) / (int)esz)); + goto retry_alloc_baser; + } + + if (!base) { + dev_err(&its->dev, "Failed to allocate device table\n"); return -ENOMEM; + } retry_baser: - val = (virt_to_phys(base) | + val = (dma_handle | (type << GITS_BASER_TYPE_SHIFT) | ((esz - 1) << GITS_BASER_ENTRY_SIZE_SHIFT) | ((alloc_pages - 1) << GITS_BASER_PAGES_SHIFT) | @@ -923,29 +938,28 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, goto retry_baser; } - if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) { - /* - * Page size didn't stick. Let's try a smaller - * size and retry. If we reach 4K, then - * something is horribly wrong... - */ - devm_free_pages(&its->dev, (unsigned long)base); - baser->base = NULL; - - switch (psz) { - case SZ_16K: - psz = SZ_4K; - goto retry_alloc_baser; - case SZ_64K: - psz = SZ_16K; - goto retry_alloc_baser; + if (val != tmp) { + dmam_free_coherent(&its->dev, PAGE_ORDER_TO_SIZE(order), + base, dma_handle); + + if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) { + /* + * Page size didn't stick. Let's try a smaller + * size and retry. If we reach 4K, then + * something is horribly wrong... + */ + switch (psz) { + case SZ_16K: + psz = SZ_4K; + goto retry_alloc_baser; + case SZ_64K: + psz = SZ_16K; + goto retry_alloc_baser; + } } - } - if (val != tmp) { dev_err(&its->dev, "%s doesn't stick: %llx %llx\n", its_base_type_string[type], val, tmp); - devm_free_pages(&its->dev, (unsigned long)base); return -ENXIO; } @@ -1003,12 +1017,6 @@ static bool its_parse_baser_device(struct its_node *its, struct its_baser *baser * feature is not supported by hardware. */ new_order = max_t(u32, get_order(esz << ids), new_order); - if (new_order >= MAX_ORDER) { - new_order = MAX_ORDER - 1; - ids = ilog2(PAGE_ORDER_TO_SIZE(new_order) / (int)esz); - dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u\n", - its->device_ids, ids); - } *order = new_order; @@ -1698,6 +1706,9 @@ static int __init its_init_one(struct its_node *its) return err; } + /* Setup dma_ops for dmam_alloc_coherent() */ + arch_setup_dma_ops(&its->dev, 0, 0, NULL, true); + its_base = devm_ioremap(&its->dev, its->phys_base, its->phys_size); if (!its_base) { dev_warn(&its->dev, "Unable to map ITS registers\n");
The gicv3-its device table may have a size of up to 16MB. With 4k pagesize the maximum size of memory allocation is 4MB. Use CMA for allocation of large tables. We use the device managed version of dma_alloc_coherent(). Thus, we don't need to release it manually on device removal. Signed-off-by: Robert Richter <rrichter@cavium.com> --- drivers/irqchip/irq-gic-v3-its.c | 69 +++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 29 deletions(-)