Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On 1/7/21 1:09 PM, Florian Fainelli wrote: On 1/7/21 9:57 AM, Konrad Rzeszutek Wilk wrote: On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote: Hi Greg and Konrad, This change is intended to be non-arch specific. Any arch that lacks DMA access control and has devices not behind an IOMMU can make use of it. Could you share why you think this should be arch specific? The idea behind non-arch specific code is it to be generic. The devicetree is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should be in arch specific code. In premise the same code could be used with an ACPI enabled system with an appropriate service to identify the restricted DMA regions and unlock them. More than 1 architecture requiring this function (ARM and ARM64 are the two I can think of needing this immediately) sort of calls for making the code architecture agnostic since past 2, you need something that scales. There is already code today under kernel/dma/contiguous.c that is only activated on a CONFIG_OF=y && CONFIG_OF_RESERVED_MEM=y system, this is no different. Just a note for history/archives that this approach would not be appropriate on general purpose Arm systems, such as SystemReady-ES edge/non-server platforms seeking to run general purpose distros. I want to have that in the record before someone at Arm (or NVidia, or a bunch of others that come to mind who have memory firewalls) gets an idea. If you're working at an Arm vendor and come looking at this later thinking "wow, what a great idea!", please fix your hardware to have a real IOMMU/SMMU and real PCIe. You'll be pointed at this reply. Jon. -- Computer Architect
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On Wed, Jan 13, 2021 at 8:42 PM Christoph Hellwig wrote:
>
> > +#ifdef CONFIG_SWIOTLB
> > + struct io_tlb_mem *dma_io_tlb_mem;
> > #endif
>
> Please add a new config option for this code instead of always building
> it when swiotlb is enabled.
>
> > +static int swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t
> > start,
> > +size_t size)
>
> Can you split the refactoring in swiotlb.c into one or more prep
> patches?
>
> > +static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
> > + struct device *dev)
> > +{
> > + struct io_tlb_mem *mem = rmem->priv;
> > + int ret;
> > +
> > + if (dev->dma_io_tlb_mem)
> > + return -EBUSY;
> > +
> > + if (!mem) {
> > + mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > + if (!mem)
> > + return -ENOMEM;
>
> What is the calling convention here that allows for a NULL and non-NULL
> private data?
Since multiple devices can share the same pool, the private data,
io_tlb_mem struct, will be initialized by the first device attached to
it.
This is similar to rmem_dma_device_init() in kernel/dma/coherent.c.
I'll add a comment for it in next version.
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On 2021-01-13 17:43, Florian Fainelli wrote:
On 1/13/21 7:27 AM, Robin Murphy wrote:
On 2021-01-13 13:59, Nicolas Saenz Julienne wrote:
Hi All,
On Tue, 2021-01-12 at 16:03 -0800, Florian Fainelli wrote:
On 1/5/21 7:41 PM, Claire Chang wrote:
Add the initialization function to create restricted DMA pools from
matching reserved-memory nodes in the device tree.
Signed-off-by: Claire Chang
---
include/linux/device.h | 4 ++
include/linux/swiotlb.h | 7 +-
kernel/dma/Kconfig | 1 +
kernel/dma/swiotlb.c | 144
++--
4 files changed, 131 insertions(+), 25 deletions(-)
diff --git a/include/linux/device.h b/include/linux/device.h
index 89bb8b84173e..ca6f71ec8871 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -413,6 +413,7 @@ struct dev_links_info {
* @dma_pools: Dma pools (if dma'ble device).
* @dma_mem: Internal for coherent mem override.
* @cma_area: Contiguous memory area for dma allocations
+ * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
* @archdata: For arch-specific additions.
* @of_node: Associated device tree node.
* @fwnode: Associated device node supplied by platform firmware.
@@ -515,6 +516,9 @@ struct device {
#ifdef CONFIG_DMA_CMA
struct cma *cma_area; /* contiguous memory area for dma
allocations */
+#endif
+#ifdef CONFIG_SWIOTLB
+ struct io_tlb_mem *dma_io_tlb_mem;
#endif
/* arch specific additions */
struct dev_archdata archdata;
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index dd8eb57cbb8f..a1bbd775 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -76,12 +76,13 @@ extern enum swiotlb_force swiotlb_force;
*
* @start: The start address of the swiotlb memory pool. Used
to do a quick
* range check to see if the memory was in fact allocated
by this
- * API.
+ * API. For restricted DMA pool, this is device tree
adjustable.
Maybe write it as this is "firmware adjustable" such that when/if ACPI
needs something like this, the description does not need updating.
TBH I really don't think this needs calling out at all. Even in the
regular case, the details of exactly how and where the pool is allocated
are beyond the scope of this code - architectures already have several
ways to control that and make their own decisions.
[snip]
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
+
+ if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
MEMREMAP_WB sounds appropriate as a default.
As per the binding 'no-map' has to be disabled here. So AFAIU, this
memory will
be part of the linear mapping. Is this really needed then?
More than that, I'd assume that we *have* to use the linear/direct map
address rather than anything that has any possibility of being a vmalloc
remap, otherwise we can no longer safely rely on
phys_to_dma/dma_to_phys, no?
I believe you are right, which means that if we want to make use of the
restricted DMA pool on a 32-bit architecture (and we do, at least, I do)
we should probably add some error checking/warning to ensure the
restricted DMA pool falls within the linear map.
Oh, good point - I'm so used to 64-bit that I instinctively just blanked
out the !PageHighMem() condition in try_ram_remap(). So maybe the
original intent here *was* to effectively just implement that check, but
if so it could still do with being a lot more explicit.
Cheers,
Robin.
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On 1/13/21 7:27 AM, Robin Murphy wrote:
> On 2021-01-13 13:59, Nicolas Saenz Julienne wrote:
>> Hi All,
>>
>> On Tue, 2021-01-12 at 16:03 -0800, Florian Fainelli wrote:
>>> On 1/5/21 7:41 PM, Claire Chang wrote:
Add the initialization function to create restricted DMA pools from
matching reserved-memory nodes in the device tree.
Signed-off-by: Claire Chang
---
include/linux/device.h | 4 ++
include/linux/swiotlb.h | 7 +-
kernel/dma/Kconfig | 1 +
kernel/dma/swiotlb.c | 144
++--
4 files changed, 131 insertions(+), 25 deletions(-)
diff --git a/include/linux/device.h b/include/linux/device.h
index 89bb8b84173e..ca6f71ec8871 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -413,6 +413,7 @@ struct dev_links_info {
* @dma_pools: Dma pools (if dma'ble device).
* @dma_mem: Internal for coherent mem override.
* @cma_area: Contiguous memory area for dma allocations
+ * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
* @archdata: For arch-specific additions.
* @of_node: Associated device tree node.
* @fwnode: Associated device node supplied by platform firmware.
@@ -515,6 +516,9 @@ struct device {
#ifdef CONFIG_DMA_CMA
struct cma *cma_area; /* contiguous memory area for dma
allocations */
+#endif
+#ifdef CONFIG_SWIOTLB
+ struct io_tlb_mem *dma_io_tlb_mem;
#endif
/* arch specific additions */
struct dev_archdata archdata;
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index dd8eb57cbb8f..a1bbd775 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -76,12 +76,13 @@ extern enum swiotlb_force swiotlb_force;
*
* @start: The start address of the swiotlb memory pool. Used
to do a quick
* range check to see if the memory was in fact allocated
by this
- * API.
+ * API. For restricted DMA pool, this is device tree
adjustable.
>>>
>>> Maybe write it as this is "firmware adjustable" such that when/if ACPI
>>> needs something like this, the description does not need updating.
>
> TBH I really don't think this needs calling out at all. Even in the
> regular case, the details of exactly how and where the pool is allocated
> are beyond the scope of this code - architectures already have several
> ways to control that and make their own decisions.
>
>>>
>>> [snip]
>>>
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
+
+ if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
>>>
>>> MEMREMAP_WB sounds appropriate as a default.
>>
>> As per the binding 'no-map' has to be disabled here. So AFAIU, this
>> memory will
>> be part of the linear mapping. Is this really needed then?
>
> More than that, I'd assume that we *have* to use the linear/direct map
> address rather than anything that has any possibility of being a vmalloc
> remap, otherwise we can no longer safely rely on
> phys_to_dma/dma_to_phys, no?
I believe you are right, which means that if we want to make use of the
restricted DMA pool on a 32-bit architecture (and we do, at least, I do)
we should probably add some error checking/warning to ensure the
restricted DMA pool falls within the linear map.
--
Florian
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On 2021-01-13 13:59, Nicolas Saenz Julienne wrote:
Hi All,
On Tue, 2021-01-12 at 16:03 -0800, Florian Fainelli wrote:
On 1/5/21 7:41 PM, Claire Chang wrote:
Add the initialization function to create restricted DMA pools from
matching reserved-memory nodes in the device tree.
Signed-off-by: Claire Chang
---
include/linux/device.h | 4 ++
include/linux/swiotlb.h | 7 +-
kernel/dma/Kconfig | 1 +
kernel/dma/swiotlb.c| 144 ++--
4 files changed, 131 insertions(+), 25 deletions(-)
diff --git a/include/linux/device.h b/include/linux/device.h
index 89bb8b84173e..ca6f71ec8871 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -413,6 +413,7 @@ struct dev_links_info {
* @dma_pools:Dma pools (if dma'ble device).
* @dma_mem: Internal for coherent mem override.
* @cma_area: Contiguous memory area for dma allocations
+ * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
* @archdata: For arch-specific additions.
* @of_node: Associated device tree node.
* @fwnode: Associated device node supplied by platform firmware.
@@ -515,6 +516,9 @@ struct device {
#ifdef CONFIG_DMA_CMA
struct cma *cma_area; /* contiguous memory area for dma
allocations */
+#endif
+#ifdef CONFIG_SWIOTLB
+ struct io_tlb_mem *dma_io_tlb_mem;
#endif
/* arch specific additions */
struct dev_archdata archdata;
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index dd8eb57cbb8f..a1bbd775 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -76,12 +76,13 @@ extern enum swiotlb_force swiotlb_force;
*
* @start:The start address of the swiotlb memory pool. Used to do a quick
*range check to see if the memory was in fact allocated by this
- * API.
+ * API. For restricted DMA pool, this is device tree adjustable.
Maybe write it as this is "firmware adjustable" such that when/if ACPI
needs something like this, the description does not need updating.
TBH I really don't think this needs calling out at all. Even in the
regular case, the details of exactly how and where the pool is allocated
are beyond the scope of this code - architectures already have several
ways to control that and make their own decisions.
[snip]
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
+
+ if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
MEMREMAP_WB sounds appropriate as a default.
As per the binding 'no-map' has to be disabled here. So AFAIU, this memory will
be part of the linear mapping. Is this really needed then?
More than that, I'd assume that we *have* to use the linear/direct map
address rather than anything that has any possibility of being a vmalloc
remap, otherwise we can no longer safely rely on
phys_to_dma/dma_to_phys, no?
That said, given that we're not actually using the returned address, I'm
not entirely sure what the point of this call is anyway. If we can
assume it's always going to return the linear map address via
try_ram_remap() then we can equally just go ahead and use the linear map
address straight away. I don't really see how we could ever hit the
"is_ram == REGION_MIXED" case in memremap() that would return NULL, if
we passed the memblock check earlier in __reserved_mem_alloc_size() such
that this rmem node ever got to be initialised at all.
Robin.
Documentation/devicetree/bindings/reserved-memory/ramoops.txt does
define an "unbuffered" property which in premise could be applied to the
generic reserved memory binding as well and that we may have to be
honoring here, if we were to make it more generic. Oh well, this does
not need to be addressed right now I guess.
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
Hi All,
On Tue, 2021-01-12 at 16:03 -0800, Florian Fainelli wrote:
> On 1/5/21 7:41 PM, Claire Chang wrote:
> > Add the initialization function to create restricted DMA pools from
> > matching reserved-memory nodes in the device tree.
> >
> > Signed-off-by: Claire Chang
> > ---
> > include/linux/device.h | 4 ++
> > include/linux/swiotlb.h | 7 +-
> > kernel/dma/Kconfig | 1 +
> > kernel/dma/swiotlb.c| 144 ++--
> > 4 files changed, 131 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 89bb8b84173e..ca6f71ec8871 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -413,6 +413,7 @@ struct dev_links_info {
> > * @dma_pools: Dma pools (if dma'ble device).
> > * @dma_mem: Internal for coherent mem override.
> > * @cma_area: Contiguous memory area for dma allocations
> > + * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
> > * @archdata: For arch-specific additions.
> > * @of_node: Associated device tree node.
> > * @fwnode:Associated device node supplied by platform firmware.
> > @@ -515,6 +516,9 @@ struct device {
> > #ifdef CONFIG_DMA_CMA
> > struct cma *cma_area; /* contiguous memory area for dma
> > allocations */
> > +#endif
> > +#ifdef CONFIG_SWIOTLB
> > + struct io_tlb_mem *dma_io_tlb_mem;
> > #endif
> > /* arch specific additions */
> > struct dev_archdata archdata;
> > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> > index dd8eb57cbb8f..a1bbd775 100644
> > --- a/include/linux/swiotlb.h
> > +++ b/include/linux/swiotlb.h
> > @@ -76,12 +76,13 @@ extern enum swiotlb_force swiotlb_force;
> > *
> > * @start: The start address of the swiotlb memory pool. Used to do a quick
> > * range check to see if the memory was in fact allocated by this
> > - * API.
> > + * API. For restricted DMA pool, this is device tree adjustable.
>
> Maybe write it as this is "firmware adjustable" such that when/if ACPI
> needs something like this, the description does not need updating.
>
> [snip]
>
> > +static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
> > + struct device *dev)
> > +{
> > + struct io_tlb_mem *mem = rmem->priv;
> > + int ret;
> > +
> > + if (dev->dma_io_tlb_mem)
> > + return -EBUSY;
> > +
> > + if (!mem) {
> > + mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > + if (!mem)
> > + return -ENOMEM;
> > +
> > + if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
>
> MEMREMAP_WB sounds appropriate as a default.
As per the binding 'no-map' has to be disabled here. So AFAIU, this memory will
be part of the linear mapping. Is this really needed then?
> Documentation/devicetree/bindings/reserved-memory/ramoops.txt does
> define an "unbuffered" property which in premise could be applied to the
> generic reserved memory binding as well and that we may have to be
> honoring here, if we were to make it more generic. Oh well, this does
> not need to be addressed right now I guess.
signature.asc
Description: This is a digitally signed message part
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
> +#ifdef CONFIG_SWIOTLB
> + struct io_tlb_mem *dma_io_tlb_mem;
> #endif
Please add a new config option for this code instead of always building
it when swiotlb is enabled.
> +static int swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
> +size_t size)
Can you split the refactoring in swiotlb.c into one or more prep
patches?
> +static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
> + struct device *dev)
> +{
> + struct io_tlb_mem *mem = rmem->priv;
> + int ret;
> +
> + if (dev->dma_io_tlb_mem)
> + return -EBUSY;
> +
> + if (!mem) {
> + mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> + if (!mem)
> + return -ENOMEM;
What is the calling convention here that allows for a NULL and non-NULL
private data?
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On Wed, Jan 13, 2021 at 01:29:05PM +0100, Greg KH wrote: > > > Why does this have to be added here? Shouldn't the platform-specific > > > code handle it instead? > > > > The whole code added here is pretty generic. What we need to eventually > > do, though is to add a separate dma_device instead of adding more and more > > bloat to struct device. > > I have no objections for that happening! I'm pretty sure you agreed to it before in fact. Now someone just needs to find the time to do this heavy lifting, where "someone" probably means me.
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On Wed, Jan 13, 2021 at 12:51:26PM +0100, Christoph Hellwig wrote:
> On Wed, Jan 06, 2021 at 08:50:03AM +0100, Greg KH wrote:
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -413,6 +413,7 @@ struct dev_links_info {
> > > * @dma_pools: Dma pools (if dma'ble device).
> > > * @dma_mem: Internal for coherent mem override.
> > > * @cma_area:Contiguous memory area for dma allocations
> > > + * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
> >
> > Why does this have to be added here? Shouldn't the platform-specific
> > code handle it instead?
>
> The whole code added here is pretty generic. What we need to eventually
> do, though is to add a separate dma_device instead of adding more and more
> bloat to struct device.
I have no objections for that happening!
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On Wed, Jan 06, 2021 at 08:50:03AM +0100, Greg KH wrote:
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -413,6 +413,7 @@ struct dev_links_info {
> > * @dma_pools: Dma pools (if dma'ble device).
> > * @dma_mem: Internal for coherent mem override.
> > * @cma_area: Contiguous memory area for dma allocations
> > + * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
>
> Why does this have to be added here? Shouldn't the platform-specific
> code handle it instead?
The whole code added here is pretty generic. What we need to eventually
do, though is to add a separate dma_device instead of adding more and more
bloat to struct device.
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On 2021-01-07 17:57, Konrad Rzeszutek Wilk wrote: On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote: Hi Greg and Konrad, This change is intended to be non-arch specific. Any arch that lacks DMA access control and has devices not behind an IOMMU can make use of it. Could you share why you think this should be arch specific? The idea behind non-arch specific code is it to be generic. The devicetree is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should be in arch specific code. Sorry, but that's an absurd argument. By the same token you'd equally have to claim that bits of, say, the Broadcom WiFi driver (not to mention dozens of others) should be split out into arch code, since not all platforms use the devicetree parts, nor the ACPI parts, nor the PCI parts... There is nothing architecture-specific about using devicetree as a system description - AFAIK there *are* a handful of x86 platforms that use it, besides even more architectures than you've listed above. It has long been the policy that devicetree-related code for a particular subsystem should just live within that subsystem. Sometimes if there's enough of it it gets collected together into its own file - e.g. drivers/pci/of.c - otherwise it tends to just get #ifdef'ed - e.g. of_spi_parse_dt(), or the other DMA reserved-memory consumers that already exist as Florian points out. Besides, there are far more platforms that enable CONFIG_OF than enable CONFIG_SWIOTLB, so by that metric the whole of the SWIOTLB code itself is even less "generic" than any DT parsing :P Robin.
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On 1/5/21 7:41 PM, Claire Chang wrote:
> Add the initialization function to create restricted DMA pools from
> matching reserved-memory nodes in the device tree.
>
> Signed-off-by: Claire Chang
> ---
> include/linux/device.h | 4 ++
> include/linux/swiotlb.h | 7 +-
> kernel/dma/Kconfig | 1 +
> kernel/dma/swiotlb.c| 144 ++--
> 4 files changed, 131 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 89bb8b84173e..ca6f71ec8871 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -413,6 +413,7 @@ struct dev_links_info {
> * @dma_pools: Dma pools (if dma'ble device).
> * @dma_mem: Internal for coherent mem override.
> * @cma_area:Contiguous memory area for dma allocations
> + * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
> * @archdata:For arch-specific additions.
> * @of_node: Associated device tree node.
> * @fwnode: Associated device node supplied by platform firmware.
> @@ -515,6 +516,9 @@ struct device {
> #ifdef CONFIG_DMA_CMA
> struct cma *cma_area; /* contiguous memory area for dma
> allocations */
> +#endif
> +#ifdef CONFIG_SWIOTLB
> + struct io_tlb_mem *dma_io_tlb_mem;
> #endif
> /* arch specific additions */
> struct dev_archdata archdata;
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index dd8eb57cbb8f..a1bbd775 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -76,12 +76,13 @@ extern enum swiotlb_force swiotlb_force;
> *
> * @start: The start address of the swiotlb memory pool. Used to do a quick
> * range check to see if the memory was in fact allocated by this
> - * API.
> + * API. For restricted DMA pool, this is device tree adjustable.
Maybe write it as this is "firmware adjustable" such that when/if ACPI
needs something like this, the description does not need updating.
[snip]
> +static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
> + struct device *dev)
> +{
> + struct io_tlb_mem *mem = rmem->priv;
> + int ret;
> +
> + if (dev->dma_io_tlb_mem)
> + return -EBUSY;
> +
> + if (!mem) {
> + mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> + if (!mem)
> + return -ENOMEM;
> +
> + if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
MEMREMAP_WB sounds appropriate as a default.
Documentation/devicetree/bindings/reserved-memory/ramoops.txt does
define an "unbuffered" property which in premise could be applied to the
generic reserved memory binding as well and that we may have to be
honoring here, if we were to make it more generic. Oh well, this does
not need to be addressed right now I guess.
--
Florian
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On 1/7/21 1:19 PM, Konrad Rzeszutek Wilk wrote: > On Thu, Jan 07, 2021 at 10:09:14AM -0800, Florian Fainelli wrote: >> On 1/7/21 9:57 AM, Konrad Rzeszutek Wilk wrote: >>> On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote: Hi Greg and Konrad, This change is intended to be non-arch specific. Any arch that lacks DMA access control and has devices not behind an IOMMU can make use of it. Could you share why you think this should be arch specific? >>> >>> The idea behind non-arch specific code is it to be generic. The devicetree >>> is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should >>> be in arch specific code. >> >> In premise the same code could be used with an ACPI enabled system with >> an appropriate service to identify the restricted DMA regions and unlock >> them. > > Which this patchset is not. ACPI is not included, but the comment about Device Tree being specific to PowerPC, SPARC and ARM is x86 is not quite correct. There is an architecture specific part to obtaining where the Device Tree lives in memory, but the implementation itself is architecture agnostic (with some early SPARC/OpenFirmware shenanigans), and x86 does, or rather did support Device Tree to a very small extent with the CE4100 platform. Would you prefer that an swiotlb_of.c file be created instead or something along those lines to better encapsulate where the OF specific code lives? > >> >> More than 1 architecture requiring this function (ARM and ARM64 are the >> two I can think of needing this immediately) sort of calls for making >> the code architecture agnostic since past 2, you need something that scales. > > I believe the use-case is for ARM64 at this moment. For the platforms that Claire uses, certainly for the ones we use, ARM and ARM64 are in scope. -- Florian
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On Thu, Jan 07, 2021 at 10:09:14AM -0800, Florian Fainelli wrote: > On 1/7/21 9:57 AM, Konrad Rzeszutek Wilk wrote: > > On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote: > >> Hi Greg and Konrad, > >> > >> This change is intended to be non-arch specific. Any arch that lacks DMA > >> access > >> control and has devices not behind an IOMMU can make use of it. Could you > >> share > >> why you think this should be arch specific? > > > > The idea behind non-arch specific code is it to be generic. The devicetree > > is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should > > be in arch specific code. > > In premise the same code could be used with an ACPI enabled system with > an appropriate service to identify the restricted DMA regions and unlock > them. Which this patchset is not. > > More than 1 architecture requiring this function (ARM and ARM64 are the > two I can think of needing this immediately) sort of calls for making > the code architecture agnostic since past 2, you need something that scales. I believe the use-case is for ARM64 at this moment. > > There is already code today under kernel/dma/contiguous.c that is only > activated on a CONFIG_OF=y && CONFIG_OF_RESERVED_MEM=y system, this is > no different. > -- > Florian
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On 1/7/21 9:57 AM, Konrad Rzeszutek Wilk wrote: > On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote: >> Hi Greg and Konrad, >> >> This change is intended to be non-arch specific. Any arch that lacks DMA >> access >> control and has devices not behind an IOMMU can make use of it. Could you >> share >> why you think this should be arch specific? > > The idea behind non-arch specific code is it to be generic. The devicetree > is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should > be in arch specific code. In premise the same code could be used with an ACPI enabled system with an appropriate service to identify the restricted DMA regions and unlock them. More than 1 architecture requiring this function (ARM and ARM64 are the two I can think of needing this immediately) sort of calls for making the code architecture agnostic since past 2, you need something that scales. There is already code today under kernel/dma/contiguous.c that is only activated on a CONFIG_OF=y && CONFIG_OF_RESERVED_MEM=y system, this is no different. -- Florian
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote: > Hi Greg and Konrad, > > This change is intended to be non-arch specific. Any arch that lacks DMA > access > control and has devices not behind an IOMMU can make use of it. Could you > share > why you think this should be arch specific? The idea behind non-arch specific code is it to be generic. The devicetree is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should be in arch specific code. > > Thanks!
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
Hi Greg and Konrad, This change is intended to be non-arch specific. Any arch that lacks DMA access control and has devices not behind an IOMMU can make use of it. Could you share why you think this should be arch specific? Thanks!
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
Hello!
In this file:
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index e4368159f88a..7fb2ac087d23 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
..
> +static const struct reserved_mem_ops rmem_swiotlb_ops = {
> + .device_init= rmem_swiotlb_device_init,
> + .device_release = rmem_swiotlb_device_release,
> +};
> +
> +static int __init rmem_swiotlb_setup(struct reserved_mem *rmem)
> +{
> + unsigned long node = rmem->fdt_node;
> +
> + if (of_get_flat_dt_prop(node, "reusable", NULL) ||
> + of_get_flat_dt_prop(node, "linux,cma-default", NULL) ||
> + of_get_flat_dt_prop(node, "linux,dma-default", NULL) ||
> + of_get_flat_dt_prop(node, "no-map", NULL))
> + return -EINVAL;
> +
> + rmem->ops = _swiotlb_ops;
> + pr_info("Reserved memory: created device swiotlb memory pool at %pa,
> size %ld MiB\n",
> + >base, (unsigned long)rmem->size / SZ_1M);
> + return 0;
> +}
> +
> +RESERVEDMEM_OF_DECLARE(dma, "restricted-dma-pool", rmem_swiotlb_setup);
The code should be as much as possible arch-agnostic. That is why there
are multiple -swiotlb files scattered in arch directories that own the
architecture specific code.
Would it be possible to move the code there and perhaps have a ARM
specific front-end for this DMA restricted pool there? See for example
the xen-swiotlb code.
Cheers!
Konrad
Re: [RFC PATCH v3 2/6] swiotlb: Add restricted DMA pool
On Wed, Jan 06, 2021 at 11:41:20AM +0800, Claire Chang wrote:
> Add the initialization function to create restricted DMA pools from
> matching reserved-memory nodes in the device tree.
>
> Signed-off-by: Claire Chang
> ---
> include/linux/device.h | 4 ++
> include/linux/swiotlb.h | 7 +-
> kernel/dma/Kconfig | 1 +
> kernel/dma/swiotlb.c| 144 ++--
> 4 files changed, 131 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 89bb8b84173e..ca6f71ec8871 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -413,6 +413,7 @@ struct dev_links_info {
> * @dma_pools: Dma pools (if dma'ble device).
> * @dma_mem: Internal for coherent mem override.
> * @cma_area:Contiguous memory area for dma allocations
> + * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
Why does this have to be added here? Shouldn't the platform-specific
code handle it instead?
thanks,
greg k-h
