[PATCH 0/3] drivers: rpmsg: make rpmsg bus DMA capable

[PATCH 0/3] drivers: rpmsg: make rpmsg bus DMA capable

[srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A]

From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

On Qualcomm SoCs, DSP exposes many functionalities like audio,
as these drivers are child of rpmsg bus. Any dma allocations
or iommu operations on these devices would fail as the rpmsg
bus is not marked as dma capable bus.

This patchset makes the rpmsg bus dma capable.

This patchset is tested on DB410c and DB820c boards.

Thanks,
srini

Srinivas Kandagatla (3):
  rpmsg: core: export rpmsg bus type
  rpmsg: core: make rpmsg bus DMA capable
  iommu: armsmmu: set iommu ops for rpmsg bus

 drivers/iommu/arm-smmu.c   | 5 +++++
 drivers/rpmsg/rpmsg_core.c | 4 +++-
 include/linux/rpmsg.h      | 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.15.1

[PATCH 1/3] rpmsg: core: export rpmsg bus type

[srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A]

From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Export rpmsg bus type so that iommu drivers iommu ops to
rpmsg bus type.

On Qualcomm SoCs ADSP exposes functions like audio and
other which need iommu access, as these drivers are part
of rpmsg bus, able to allocate memory from iommus is basic
requirement. So expose this bus so that iommu drivers can
add ops to this.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/rpmsg/rpmsg_core.c | 3 ++-
 include/linux/rpmsg.h      | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 5a081762afcc..e84c71f8d6ab 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -465,7 +465,7 @@ static int rpmsg_dev_remove(struct device *dev)
 	return err;
 }
 
-static struct bus_type rpmsg_bus = {
+struct bus_type rpmsg_bus = {
 	.name		= "rpmsg",
 	.match		= rpmsg_dev_match,
 	.dev_groups	= rpmsg_dev_groups,
@@ -473,6 +473,7 @@ static struct bus_type rpmsg_bus = {
 	.probe		= rpmsg_dev_probe,
 	.remove		= rpmsg_dev_remove,
 };
+EXPORT_SYMBOL(rpmsg_bus);
 
 int rpmsg_register_device(struct rpmsg_device *rpdev)
 {
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index ca07366c4c33..869e5946b7df 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -49,7 +49,7 @@ struct rpmsg_device;
 struct rpmsg_endpoint;
 struct rpmsg_device_ops;
 struct rpmsg_endpoint_ops;
-
+extern struct bus_type rpmsg_bus;
 /**
  * struct rpmsg_channel_info - channel info representation
  * @name: name of service
-- 
2.15.1

Re: [PATCH 1/3] rpmsg: core: export rpmsg bus type

[Robin Murphy]

On 02/03/18 14:55, srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Export rpmsg bus type so that iommu drivers iommu ops to
> rpmsg bus type.
> 
> On Qualcomm SoCs ADSP exposes functions like audio and
> other which need iommu access, as these drivers are part
> of rpmsg bus, able to allocate memory from iommus is basic
> requirement. So expose this bus so that iommu drivers can
> add ops to this.

IOMMU drivers are built-in while rpmsg can be built as a module, so that 
isn't always going to work.

Robin.

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>   drivers/rpmsg/rpmsg_core.c | 3 ++-
>   include/linux/rpmsg.h      | 2 +-
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 5a081762afcc..e84c71f8d6ab 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -465,7 +465,7 @@ static int rpmsg_dev_remove(struct device *dev)
>   	return err;
>   }
>   
> -static struct bus_type rpmsg_bus = {
> +struct bus_type rpmsg_bus = {
>   	.name		= "rpmsg",
>   	.match		= rpmsg_dev_match,
>   	.dev_groups	= rpmsg_dev_groups,
> @@ -473,6 +473,7 @@ static struct bus_type rpmsg_bus = {
>   	.probe		= rpmsg_dev_probe,
>   	.remove		= rpmsg_dev_remove,
>   };
> +EXPORT_SYMBOL(rpmsg_bus);
>   
>   int rpmsg_register_device(struct rpmsg_device *rpdev)
>   {
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index ca07366c4c33..869e5946b7df 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -49,7 +49,7 @@ struct rpmsg_device;
>   struct rpmsg_endpoint;
>   struct rpmsg_device_ops;
>   struct rpmsg_endpoint_ops;
> -
> +extern struct bus_type rpmsg_bus;
>   /**
>    * struct rpmsg_channel_info - channel info representation
>    * @name: name of service
> 

[PATCH 2/3] rpmsg: core: make rpmsg bus DMA capable

[srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A]

From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Many of the rpmsg clients like audio drivers need to allocate
dma memory. Make this bus DMA capable so that the child devices
can use dma apis.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/rpmsg/rpmsg_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index e84c71f8d6ab..540a3f3567b8 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -472,6 +472,7 @@ struct bus_type rpmsg_bus = {
 	.uevent		= rpmsg_uevent,
 	.probe		= rpmsg_dev_probe,
 	.remove		= rpmsg_dev_remove,
+	.force_dma	= true,
 };
 EXPORT_SYMBOL(rpmsg_bus);
 
-- 
2.15.1

Re: [PATCH 2/3] rpmsg: core: make rpmsg bus DMA capable

[Robin Murphy]

On 02/03/18 14:55, srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Many of the rpmsg clients like audio drivers need to allocate
> dma memory. Make this bus DMA capable so that the child devices
> can use dma apis.

AFAICS after 15 minutes in the docs and code, the rpmsg "bus" is a 
virtual one based around shared-memory mailbox communication, so I don't 
really see how DMA exists in that context - I think maybe that 
abstraction needs looking at.

However, from grepping through the DTs it seems at first glance like the 
non-trivial things under the "qcom,smd" bus mostly map to actual 
platform devices via the "qcom,smd-edge" property - if those platform 
devices are the physical DMA masters, they should be the ones used for 
DMA API operations.

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>   drivers/rpmsg/rpmsg_core.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index e84c71f8d6ab..540a3f3567b8 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -472,6 +472,7 @@ struct bus_type rpmsg_bus = {
>   	.uevent		= rpmsg_uevent,
>   	.probe		= rpmsg_dev_probe,
>   	.remove		= rpmsg_dev_remove,
> +	.force_dma	= true,

Regardless of the above, would you really need to use this brute force 
hack instead of just fixing the DTs? I'm struggling to find which 
drivers might currently be relying on this :/

Robin.

>   };
>   EXPORT_SYMBOL(rpmsg_bus);
>   
> 

Re: [PATCH 2/3] rpmsg: core: make rpmsg bus DMA capable

[Srinivas Kandagatla]

Thanks for your time,

On 02/03/18 16:14, Robin Murphy wrote:
> On 02/03/18 14:55, srinivas.kandagatla@linaro.org wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> Many of the rpmsg clients like audio drivers need to allocate
>> dma memory. Make this bus DMA capable so that the child devices
>> can use dma apis.
> 
> AFAICS after 15 minutes in the docs and code, the rpmsg "bus" is a 
> virtual one based around shared-memory mailbox communication, so I don't 
> really see how DMA exists in that context - I think maybe that 
> abstraction needs looking at.
> 
> However, from grepping through the DTs it seems at first glance like the 
> non-trivial things under the "qcom,smd" bus mostly map to actual 
> platform devices via the "qcom,smd-edge" property - if those platform 
> devices are the physical DMA masters, they should be the ones used for 
> DMA API operations.

Currently there are very limited rpmsg devices in the mainline that use 
dma. Only one I can think of is wcnss WIFI driver which models up itself 
into another layer of platform device. Not sure if the DMA was the 
reason to do that.

However am working on audio drivers [1] which I modeled up as children 
of the rpmsg bus, so the problem started. There is an IOMMU in between 
APPs and DSP which provides audio services.
There are also other projects like FastRPC which have used similar 
driver model which ended up with same issues.

It all depends on how you model your driver. Audio case we have a rpmsg 
channel which exposes audio functionality. so If we want to use the 
iommu/dma operations we have to add another layer of platform device.
Which also means that rpmsg channel notifications have to be passed to 
these platform devices in some way.

Am not 100% sure if this correct way to fix the issue.

> 
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/rpmsg/rpmsg_core.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index e84c71f8d6ab..540a3f3567b8 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -472,6 +472,7 @@ struct bus_type rpmsg_bus = {
>>       .uevent        = rpmsg_uevent,
>>       .probe        = rpmsg_dev_probe,
>>       .remove        = rpmsg_dev_remove,
>> +    .force_dma    = true,
> 
> Regardless of the above, would you really need to use this brute force 
> hack instead of just fixing the DTs? I'm struggling to find which 
> drivers might currently be relying on this :/

This is one of the two issues. dma-ranges might work in this case, but 
we still have iommu case.

> 
> Robin.
> 
>>   };
>>   EXPORT_SYMBOL(rpmsg_bus);
>>

thanks,
srini
[1]: https://lkml.org/lkml/2018/2/13/719

Re: [PATCH 2/3] rpmsg: core: make rpmsg bus DMA capable

[Bjorn Andersson]

On Fri 02 Mar 08:14 PST 2018, Robin Murphy wrote:

> On 02/03/18 14:55, srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
> > From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > 
> > Many of the rpmsg clients like audio drivers need to allocate
> > dma memory. Make this bus DMA capable so that the child devices
> > can use dma apis.
> 
> AFAICS after 15 minutes in the docs and code, the rpmsg "bus" is a virtual
> one based around shared-memory mailbox communication, so I don't really see
> how DMA exists in that context - I think maybe that abstraction needs
> looking at.
> 

That's right, rpmsg shuffles messages back and forth to some coprocessor
over shared memory, the contexts generating and receiving these messages
are "rpmsg devices".

The problem Srinivas is facing is that one of these rpmsg devices is
trying to allocate and map a larger chunk of memory to be shared with
the coprocessor, which is then going to be referenced in the messages
being passed in rpmsg.

> However, from grepping through the DTs it seems at first glance like the
> non-trivial things under the "qcom,smd" bus mostly map to actual platform
> devices via the "qcom,smd-edge" property - if those platform devices are the
> physical DMA masters, they should be the ones used for DMA API operations.
> 

One of the rpmsg implementations is virtio based and have a similar
problem, there dma_alloc*() is called with dev->parent->parent as
device, but this causes issues as dev->parent might not be what the
original author expected it to -- so this needs to be reworked as well.

> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >   drivers/rpmsg/rpmsg_core.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> > index e84c71f8d6ab..540a3f3567b8 100644
> > --- a/drivers/rpmsg/rpmsg_core.c
> > +++ b/drivers/rpmsg/rpmsg_core.c
> > @@ -472,6 +472,7 @@ struct bus_type rpmsg_bus = {
> >   	.uevent		= rpmsg_uevent,
> >   	.probe		= rpmsg_dev_probe,
> >   	.remove		= rpmsg_dev_remove,
> > +	.force_dma	= true,
> 
> Regardless of the above, would you really need to use this brute force hack
> instead of just fixing the DTs? I'm struggling to find which drivers might
> currently be relying on this :/
> 

The rpmsg devices, described as child nodes of rpmsg bus relates to
specific functions in the coprocessor firmware. The fact that the
firmware can be started and stopped dynamically makes the current layout
quite convenient (in comparison to e.g. how we would describe a
mailbox).

We know for these cases that dev->parent->parent is a remoteproc
instance representing the coprocessor that sits on the other side of the
communication channel. So we did investigate if we could just have that
to allocate and map buffers. The problem with this is that these
functions has multiple iommu contexts.

Regards,
Bjorn

[PATCH 3/3] iommu: armsmmu: set iommu ops for rpmsg bus

[srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A]

From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

On Qualcomm SoCs, ADSP exposes many functions like audio and
others. These services need iommu access to allocate any
memory for the DSP. As these drivers are childeren of
rpmsg bus, able to allocate memory from iommus is basic
requirement. So set arm smmu iommu ops for this bus type.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index e6920d32ac9e..9b63489af15c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -53,6 +53,7 @@
 #include <linux/spinlock.h>
 
 #include <linux/amba/bus.h>
+#include <linux/rpmsg.h>
 
 #include "io-pgtable.h"
 #include "arm-smmu-regs.h"
@@ -2168,6 +2169,10 @@ static void arm_smmu_bus_init(void)
 		bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
 	}
 #endif
+#ifdef CONFIG_RPMSG
+	if (!iommu_present(&rpmsg_bus))
+		bus_set_iommu(&rpmsg_bus, &arm_smmu_ops);
+#endif
 }
 
 static int arm_smmu_device_probe(struct platform_device *pdev)
-- 
2.15.1

Re: [PATCH 3/3] iommu: armsmmu: set iommu ops for rpmsg bus

[Robin Murphy]

On 02/03/18 14:55, srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> On Qualcomm SoCs, ADSP exposes many functions like audio and
> others. These services need iommu access to allocate any
> memory for the DSP. As these drivers are childeren of
> rpmsg bus, able to allocate memory from iommus is basic
> requirement. So set arm smmu iommu ops for this bus type.

Documentation/rpmsg.txt: "Every rpmsg device is a communication channel 
with a remote processor (thus rpmsg devices are called channels)."

I'd instinctively assume that a remote processor already has its own 
memory, and that a communication channel doesn't somehow go directly 
through an IOMMU, so that "basic requirement" seems like a pretty big 
assumption.

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>   drivers/iommu/arm-smmu.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index e6920d32ac9e..9b63489af15c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -53,6 +53,7 @@
>   #include <linux/spinlock.h>
>   
>   #include <linux/amba/bus.h>
> +#include <linux/rpmsg.h>
>   
>   #include "io-pgtable.h"
>   #include "arm-smmu-regs.h"
> @@ -2168,6 +2169,10 @@ static void arm_smmu_bus_init(void)
>   		bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>   	}
>   #endif
> +#ifdef CONFIG_RPMSG

Ah, so this will at least build OK with RPMSG=m, but I doubt it does 
what you want it to in that case.

Robin.

> +	if (!iommu_present(&rpmsg_bus))
> +		bus_set_iommu(&rpmsg_bus, &arm_smmu_ops);
> +#endif
>   }
>   
>   static int arm_smmu_device_probe(struct platform_device *pdev)
> 

Re: [PATCH 3/3] iommu: armsmmu: set iommu ops for rpmsg bus

[Bjorn Andersson]

On Fri, Mar 2, 2018 at 8:59 AM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> On 02/03/18 14:55, srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
>>
>> From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> On Qualcomm SoCs, ADSP exposes many functions like audio and
>> others. These services need iommu access to allocate any
>> memory for the DSP. As these drivers are childeren of
>> rpmsg bus, able to allocate memory from iommus is basic
>> requirement. So set arm smmu iommu ops for this bus type.
>

Forgot to answer this and the dma_ops patch seems to be going in the
right direction.

>
> Documentation/rpmsg.txt: "Every rpmsg device is a communication channel with
> a remote processor (thus rpmsg devices are called channels)."
>
> I'd instinctively assume that a remote processor already has its own memory,
> and that a communication channel doesn't somehow go directly through an
> IOMMU, so that "basic requirement" seems like a pretty big assumption.
>

As of today rpmsg exclusively uses system memory for implementing the
communication fifos, but this memory is owned/handled by the rpmsg
bus. The need here is for drivers on top of the rpmsg_bus,
implementing some application-level protocol that requires indirection
buffers; e.g. to achieve zero copy of audio or image buffers that the
remote processor is expected to operate on. In this case the device
sitting on top of the rpmsg bus will have to map the buffer to the
appropriate context and can then send application specific control
requests referencing this mapping.

As different parts of the firmware might operate in different contexts
it's not feasible to utilize the parent's (the rpmsg_bus) context for
these indirection buffers.

>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   drivers/iommu/arm-smmu.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index e6920d32ac9e..9b63489af15c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -53,6 +53,7 @@
>>   #include <linux/spinlock.h>
>>     #include <linux/amba/bus.h>
>> +#include <linux/rpmsg.h>
>>     #include "io-pgtable.h"
>>   #include "arm-smmu-regs.h"
>> @@ -2168,6 +2169,10 @@ static void arm_smmu_bus_init(void)
>>                 bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>>         }
>>   #endif
>> +#ifdef CONFIG_RPMSG
>
>
> Ah, so this will at least build OK with RPMSG=m, but I doubt it does what
> you want it to in that case.
>

Things have been refactored but the core has remained tristate,
causing extra head aches in various areas. I think it's very
reasonable to review the rpmsg config options and make CONFIG_RPMSG
bool.

So with the addition of making CONFIG_RPMSG bool the patch has my Acked-by.

That said I'm generally concerned about the first probed iommu
implementation assigning itself as the sole iommu implementation for
all busses, but I guess we haven't yet hit the point where there are
different iommu implementations in a single SoC?

Regards,
Bjorn

Re: [PATCH 3/3] iommu: armsmmu: set iommu ops for rpmsg bus

[Robin Murphy]

On 07/05/18 20:28, Bjorn Andersson wrote:
> On Fri, Mar 2, 2018 at 8:59 AM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>> On 02/03/18 14:55, srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
>>>
>>> From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>
>>> On Qualcomm SoCs, ADSP exposes many functions like audio and
>>> others. These services need iommu access to allocate any
>>> memory for the DSP. As these drivers are childeren of
>>> rpmsg bus, able to allocate memory from iommus is basic
>>> requirement. So set arm smmu iommu ops for this bus type.
>>
> 
> Forgot to answer this and the dma_ops patch seems to be going in the
> right direction.
> 
>>
>> Documentation/rpmsg.txt: "Every rpmsg device is a communication channel with
>> a remote processor (thus rpmsg devices are called channels)."
>>
>> I'd instinctively assume that a remote processor already has its own memory,
>> and that a communication channel doesn't somehow go directly through an
>> IOMMU, so that "basic requirement" seems like a pretty big assumption.
>>
> 
> As of today rpmsg exclusively uses system memory for implementing the
> communication fifos, but this memory is owned/handled by the rpmsg
> bus. The need here is for drivers on top of the rpmsg_bus,
> implementing some application-level protocol that requires indirection
> buffers; e.g. to achieve zero copy of audio or image buffers that the
> remote processor is expected to operate on. In this case the device
> sitting on top of the rpmsg bus will have to map the buffer to the
> appropriate context and can then send application specific control
> requests referencing this mapping.

Right, but that's more or less what I was getting at - rpmsg can be used 
as a means to signal some DMA master device to start doing a thing, but 
that thing itself is unrelated to rpmsg, and it by no means implies that 
everything which rpmsg can talk to is always capable of system-wide DMA. 
It's no different if that communication channel is a hardware mailbox or 
an I2C/SPI/USB/etc. link, rather than virtio; we wouldn't automatically 
consider devices on the other end of those to be directly connected to 
an IOMMU either.

IOMMU and DMA operations are highly dependent on the physical hardware 
topology, which is why I really don't like trying to shoehorn them into 
software constructs without modelling the actual hardware reasonably 
accurately. For instance it's not unheard of for remote processors in a 
SoC to see a different physical memory map from the main application 
processors - how would rpmsg try to describe that? What even is the 
address space of the rpmsg "bus"?

> As different parts of the firmware might operate in different contexts
> it's not feasible to utilize the parent's (the rpmsg_bus) context for
> these indirection buffers.

Indeed, and I maintain that that wouldn't be the right thing to do 
anyway. As before, I think the most accurate way to model the situation 
with the tools we have available is to have the actual hardware function 
represented by a platform device, which is associated with a 
corresponding rpmsg endpoint. Then the driver can manage communication 
in the rpmsg context, and physical DMA setup in the 'real' hardware 
context, and everything looks sane without questionable abstraction 
breakage. Since this looked to be more or less what is actually 
implemented anyway, it doesn't seem all that hard to refine; if there 
are multiple DMA master functions identified distinctly to the IOMMU, 
then they could either be represented as separate platform devices with 
explicit IOMMU specifiers, or you could model the actual DSP subsystem 
hardware as its own bus-like arrangement with an iommu-map arrangement 
translating function identifiers to IOMMU identifiers.

What I don't like is forcing IOMMU drivers to pretend that some data in 
a shared memory buffer is itself directly capable of generating 
transactions on the interconnect. If other 'indirect' bus abstractions 
like CoreSight can get this right, I don't see why rpmsg deserves to be 
special.

>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> ---
>>>    drivers/iommu/arm-smmu.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index e6920d32ac9e..9b63489af15c 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -53,6 +53,7 @@
>>>    #include <linux/spinlock.h>
>>>      #include <linux/amba/bus.h>
>>> +#include <linux/rpmsg.h>
>>>      #include "io-pgtable.h"
>>>    #include "arm-smmu-regs.h"
>>> @@ -2168,6 +2169,10 @@ static void arm_smmu_bus_init(void)
>>>                  bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>>>          }
>>>    #endif
>>> +#ifdef CONFIG_RPMSG
>>
>>
>> Ah, so this will at least build OK with RPMSG=m, but I doubt it does what
>> you want it to in that case.
>>
> 
> Things have been refactored but the core has remained tristate,
> causing extra head aches in various areas. I think it's very
> reasonable to review the rpmsg config options and make CONFIG_RPMSG
> bool.
> 
> So with the addition of making CONFIG_RPMSG bool the patch has my Acked-by.
> 
> That said I'm generally concerned about the first probed iommu
> implementation assigning itself as the sole iommu implementation for
> all busses, but I guess we haven't yet hit the point where there are
> different iommu implementations in a single SoC?

As it happens I do know of one such SoC already - Rockchip RK3288 seems 
to have an undocumented Arm SMMU alongside all the rockchip-iommu 
instances, but it's not used by Linux (and I have no idea what it was 
intended for; I just went and poked the intriguing "peripheral MMU" 
region of the memory map and found what looks an awful lot like an Arm 
MMU-400). More realistically, I also know of folks using the Arm Juno 
dev board with an MMU-600 in the add-on FPGA tile, which would have that 
driver-probe-order fight with the MMU-401 instances in the SoC, but I 
figure they were either using an older firmware which didn't enable the 
latter or just got lucky with not having the SMMUv2 driver enabled.

But yes, the per-bus ops thing is awful and I've been complaining about 
it for years now. Since iommu_fwspec we at least have the foundations 
for per-device ops in place now, but as is often the case, getting 80% 
of the way there is simple[1], whilst the last 20% (like replacing 
iommu_domain_alloc(), and where to call iommu_{add,remove}_device() 
from) is really hard.

Robin.

[1] 
https://www.mail-archive.com/iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org/msg14576.html