Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Vinod Koul]

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> Hi,
> 
> This series tries to solve the problem with DMA with device registers
> (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> recent patch '9575632 (dmaengine: make slave address physical)'
> clarifies that DMA slave address provided by clients is the physical
> address. This puts the task of mapping the DMA slave address from a
> phys_addr_t to a dma_addr_t on the DMA engine.
> 
> Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> the same and no special care is needed. However if you have a IOMMU you
> need to map the DMA slave phys_addr_t to a dma_addr_t using something
> like this.
> 
> This series is based on top of v4.8-rc1. And I'm hoping to be able to collect 
> a
> Ack from Russell King on patch 4/6 that adds the ARM specific part and then be
> able to take the whole series through the dmaengine tree. If this is not the
> best route I'm more then happy to do it another way.
> 
> It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
> ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with
> /dev/mmcblk1, i2c and the serial console which are devices behind the
> iommu.
> 
> Furthermore I have audited to the best of my ability all call paths
> involved to make sure that the dma_addr_t obtained from
> dma_map_resource() to is not used in a way where it would be expected
> for the mapping to be RAM (have a struct page). Many thanks to Christoph
> Hellwig and Laurent Pinchart for there input in this effort.

Applied, thanks

-- 
~Vinod

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Niklas Söderlund]

Hi Vinod,

On 2016-09-15 21:56:51 +0530, Vinod Koul wrote:
> On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
> > On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> > > Hi,
> > > 
> > > This series tries to solve the problem with DMA with device registers
> > > (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> > > recent patch '9575632 (dmaengine: make slave address physical)'
> > > clarifies that DMA slave address provided by clients is the physical
> > > address. This puts the task of mapping the DMA slave address from a
> > > phys_addr_t to a dma_addr_t on the DMA engine.
> > > 
> > > Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> > > the same and no special care is needed. However if you have a IOMMU you
> > > need to map the DMA slave phys_addr_t to a dma_addr_t using something
> > > like this.
> > > 
> > > This series is based on top of v4.8-rc1. And I'm hoping to be able to 
> > > collect a
> > > Ack from Russell King on patch 4/6 that adds the ARM specific part and 
> > > then be
> > > able to take the whole series through the dmaengine tree. If this is not 
> > > the
> > > best route I'm more then happy to do it another way.
> > > 
> > > It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
> > > ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with
> > > /dev/mmcblk1, i2c and the serial console which are devices behind the
> > > iommu.
> > 
> > As I said in last one, the dmaengine parts look fine to me. But to go thru
> > dmaengine tree I would need ACK on non dmaengine patches.
> 
> I havent heard back from this one and I am inclined to merge this one now.
> If anyone has any objects, please speak up now...

I'm just curios, do you plan to merge this series with Arnds Ack? If not 
is there anything I can do to help move the series in the right 
direction?

> 
> Also ACKs welcome...
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Laurent Pinchart]

Hi Robin,

On Friday 16 Sep 2016 13:49:21 Robin Murphy wrote:
> On 16/09/16 13:05, Laurent Pinchart wrote:
> [...]
> 
>  One concern I have is that we might get an awkward situation if we ever
>  encounter one DMA engine hardware that is used in different systems
>  that all have an IOMMU, but on some of them the connection between the
>  DMA master and the slave FIFO bypasses the IOMMU while on others the
>  IOMMU is required.
> >>> 
> >>> Do you mean systems where some of the channels of a specific DMA engine
> >>> go through the IOMMU while others do not ? We indeed have no solution
> >>> today for such a situation.
> >>> 
> >>> The problem is a bit broader than that, we'll also have an issue with
> >>> DMA engines that have different channels served by different IOMMUs. I
> >>> recall discussing this in the past with you, and the solution you
> >>> proposed was to add a channel index to struct dma_attrs seems good to
> >>> me. To support the case where some channels don't go through an IOMMU we
> >>> would only need support for null entries in the IOMMUs list associated
> >>> with a device (for instance in the DT case null entries in the iommus
> >>> property).
> >> 
> >> I think at that point we just create the channels as child devices of
> >> the main dmaengine device so they each get their own DMA ops, and can do
> >> whatever. The Qualcomm HIDMA driver already does that for a very similar
> >> reason (so that the IOMMU can map individual channels into different
> >> guest VMs).
> > 
> > That's another option, but it seems more like a workaround to me, instead
> > of a proper solution to fix the more global problem of multiple memory
> > paths within a single device. I have other hardware devices that can act
> > as bus masters through different paths (for instance a display-related
> > device that fetches data and commands through different paths). Luckily
> > so far all those paths are served by the same IOMMU, but there's no
> > guarantee this will remain true in the future. Furthermore, even today,
> > the IOMMU connected to that device has the ability to selectively enable
> > and disable its ports. I have to keep them all enabled due to the lack of
> > channel information in the DMA mapping and IOMMU APIs, leading to
> > increased power consumption.
> 
> Indeed, I think both the Exynos and Rockchip IOMMU drivers already do
> cater for a device mastering though multiple discrete IOMMUs, not being
> the fancy multi-port multi-context ones like yours and mine.
> 
> I guess what we could really do with is a decent abstraction of
> multi-master peripherals at the device level; a "threads within the same
> process" sort of granularity, as it were. I'd envisage it more along the
> lines of how we handle NUMA, i.e. dma_map_page_attrs(...) becomes a
> wrapper for dma_map_page_attrs_multi(..., CHANNEL_ALL), and trickier
> users can call the latter with the a more specific channel(s) argument
> (maybe it's a bitmask rather than an index).

That's pretty much what I've discussed with Arnd in the past, except that we 
were planning to add the channel to struct dma_attrs. Hence my disappointment 
seeing the structure go away.

> Meanwhile, dev->archdata.dma_ops may point to a device-specific array of
> dma_map_ops, which the DMA API backend iterates over if necessary.
> 
> Strangely, that doesn't actually sound too horrible.

-- 
Regards,

Laurent Pinchart

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Laurent Pinchart]

Hi Arnd,

On Friday 16 Sep 2016 14:22:31 Arnd Bergmann wrote:
> On Friday, September 16, 2016 3:09:29 PM CEST Laurent Pinchart wrote:
> >> I wasn't thinking quite that far, though that is also a theoretical
> >> problem. However, the simple solution would be to have a bit in the DMA
> >> specifier let the driver know whether translation is needed or not.
> >> 
> >> The simpler case I was thinking of is where the entire DMA engine
> >> either goes through an IOMMU or doesn't (depending on the integration
> >> into the SoC), so we'd have to find out through some DT property
> >> or compatible string in the DMA enginen driver.
> > 
> > Don't we already get that information from the iommus DT property ? If the
> > DMA engine goes through an IOMMU the property will be set, otherwise it
> > will not.
>
> It depends. A dmaengine typically at least has two DMA masters,
> possibly more. It's likely that some dmaengine implementations are
> connected to RAM through an IOMMU, but have direct access to an
> I/O bus for the slave FIFOs.

Sure, but I expect the DMA engine DT node to list all the relevant IOMMU(s) 
(if any) in the iommus property in a way that allows the DMA engine driver to 
know what IOMMU port is used for what purpose. It will then be up to the DMA 
engine driver to select the right port identifier to pass to the DMA mapping 
API.

I'm not sure how this would work with Robin's proposal of creating one device 
per channel though, as there would still be a single node in DT for the DMA 
engine device. Furthermore, a single channel might indeed have multiple DMA 
masters, not all of them being served by an IOMMU. We would thus still need 
memory port identifiers in the DMA mapping API.

> >>> The problem is a bit broader than that, we'll also have an issue with
> >>> DMA engines that have different channels served by different IOMMUs.
> >> 
> >> Do you mean a theoretical problem, or a chip that you already know
> >> exists?
> > 
> > That's theoretical. The problem I'm facing today is a DMA engine whose
> > channels are served by different ports of the same IOMMU. This works in a
> > suboptimal way because I have to keep all the IOMMU ports enabled
> > regardless of whether they're used or not, as the DMA engine and IOMMU
> > APIs don't carry channel information.
> 
> Ok

-- 
Regards,

Laurent Pinchart

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Robin Murphy]

On 16/09/16 13:05, Laurent Pinchart wrote:
[...]
 One concern I have is that we might get an awkward situation if we ever
 encounter one DMA engine hardware that is used in different systems that
 all have an IOMMU, but on some of them the connection between the DMA
 master and the slave FIFO bypasses the IOMMU while on others the IOMMU
 is required.
>>>
>>> Do you mean systems where some of the channels of a specific DMA engine go
>>> through the IOMMU while others do not ? We indeed have no solution today
>>> for such a situation.
>>>
>>> The problem is a bit broader than that, we'll also have an issue with DMA
>>> engines that have different channels served by different IOMMUs. I recall
>>> discussing this in the past with you, and the solution you proposed was to
>>> add a channel index to struct dma_attrs seems good to me. To support the
>>> case where some channels don't go through an IOMMU we would only need
>>> support for null entries in the IOMMUs list associated with a device (for
>>> instance in the DT case null entries in the iommus property).
>>
>> I think at that point we just create the channels as child devices of
>> the main dmaengine device so they each get their own DMA ops, and can do
>> whatever. The Qualcomm HIDMA driver already does that for a very similar
>> reason (so that the IOMMU can map individual channels into different
>> guest VMs).
> 
> That's another option, but it seems more like a workaround to me, instead of 
> a 
> proper solution to fix the more global problem of multiple memory paths 
> within 
> a single device. I have other hardware devices that can act as bus masters 
> through different paths (for instance a display-related device that fetches 
> data and commands through different paths). Luckily so far all those paths 
> are 
> served by the same IOMMU, but there's no guarantee this will remain true in 
> the future. Furthermore, even today, the IOMMU connected to that device has 
> the ability to selectively enable and disable its ports. I have to keep them 
> all enabled due to the lack of channel information in the DMA mapping and 
> IOMMU APIs, leading to increased power consumption.

Indeed, I think both the Exynos and Rockchip IOMMU drivers already do
cater for a device mastering though multiple discrete IOMMUs, not being
the fancy multi-port multi-context ones like yours and mine.

I guess what we could really do with is a decent abstraction of
multi-master peripherals at the device level; a "threads within the same
process" sort of granularity, as it were. I'd envisage it more along the
lines of how we handle NUMA, i.e. dma_map_page_attrs(...) becomes a
wrapper for dma_map_page_attrs_multi(..., CHANNEL_ALL), and trickier
users can call the latter with the a more specific channel(s) argument
(maybe it's a bitmask rather than an index). Meanwhile,
dev->archdata.dma_ops may point to a device-specific array of
dma_map_ops, which the DMA API backend iterates over if necessary.

Strangely, that doesn't actually sound too horrible.

Robin.

> 
>>> Now I see that struct dma_attrs has been replaced by unsigned long in
>>>
>>> commit 00085f1efa387a8ce100e3734920f7639c80caa3
>>> Author: Krzysztof Kozlowski 
>>> Date:   Wed Aug 3 13:46:00 2016 -0700
>>>
>>> dma-mapping: use unsigned long for dma_attrs
>>>
>>> We still have enough bits to reserve some of them for a channel number,
>>> but I'm not very happy with that patch as I can see how a future proposal
>>> to handle the channel number through the DMA attributes will get rejected
>>> on the grounds of bits starvation then :-(
>>>
 I don't have any idea for how this could be handled in a generic way, so
 my best answer here is to hope we never get there, and if we do, handle
 it using some local hack in the driver.
> 

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Arnd Bergmann]

On Friday, September 16, 2016 3:09:29 PM CEST Laurent Pinchart wrote:
> > I wasn't thinking quite that far, though that is also a theoretical
> > problem. However, the simple solution would be to have a bit in the DMA
> > specifier let the driver know whether translation is needed or not.
> > 
> > The simpler case I was thinking of is where the entire DMA engine
> > either goes through an IOMMU or doesn't (depending on the integration
> > into the SoC), so we'd have to find out through some DT property
> > or compatible string in the DMA enginen driver.
> 
> Don't we already get that information from the iommus DT property ? If the 
> DMA 
> engine goes through an IOMMU the property will be set, otherwise it will not.

It depends. A dmaengine typically at least has two DMA masters,
possibly more. It's likely that some dmaengine implementations are
connected to RAM through an IOMMU, but have direct access to an
I/O bus for the slave FIFOs.

> > > The problem is a bit broader than that, we'll also have an issue with DMA
> > > engines that have different channels served by different IOMMUs.
> > 
> > Do you mean a theoretical problem, or a chip that you already know exists?
> 
> That's theoretical. The problem I'm facing today is a DMA engine whose 
> channels are served by different ports of the same IOMMU. This works in a 
> suboptimal way because I have to keep all the IOMMU ports enabled regardless 
> of whether they're used or not, as the DMA engine and IOMMU APIs don't carry 
> channel information.

Ok

Arnd

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Laurent Pinchart]

Hi Arnd,

On Friday 16 Sep 2016 14:02:35 Arnd Bergmann wrote:
> On Friday, September 16, 2016 12:48:23 PM CEST Laurent Pinchart wrote:
> > On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
> >> On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:
> >>> On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
>  On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> >>
> >> I had not looked at the series earlier, but this version looks entirely
> >> reasonable to me, so
> >> 
> >> Acked-by: Arnd Bergmann 
> >> 
> >> 
> >> One concern I have is that we might get an awkward situation if we ever
> >> encounter one DMA engine hardware that is used in different systems that
> >> all have an IOMMU, but on some of them the connection between the DMA
> >> master and the slave FIFO bypasses the IOMMU while on others the IOMMU
> >> is required.
> >
> > Do you mean systems where some of the channels of a specific DMA engine go
> > through the IOMMU while others do not ? We indeed have no solution today
> > for such a situation.
> 
> I wasn't thinking quite that far, though that is also a theoretical
> problem. However, the simple solution would be to have a bit in the DMA
> specifier let the driver know whether translation is needed or not.
> 
> The simpler case I was thinking of is where the entire DMA engine
> either goes through an IOMMU or doesn't (depending on the integration
> into the SoC), so we'd have to find out through some DT property
> or compatible string in the DMA enginen driver.

Don't we already get that information from the iommus DT property ? If the DMA 
engine goes through an IOMMU the property will be set, otherwise it will not.

> > The problem is a bit broader than that, we'll also have an issue with DMA
> > engines that have different channels served by different IOMMUs.
> 
> Do you mean a theoretical problem, or a chip that you already know exists?

That's theoretical. The problem I'm facing today is a DMA engine whose 
channels are served by different ports of the same IOMMU. This works in a 
suboptimal way because I have to keep all the IOMMU ports enabled regardless 
of whether they're used or not, as the DMA engine and IOMMU APIs don't carry 
channel information.

> > I recall discussing this in the past with you, and the solution you
> > proposed was to add a channel index to struct dma_attrs seems good to me.
> > To support the case where some channels don't go through an IOMMU we would
> > only need support for null entries in the IOMMUs list associated with a
> > device (for instance in the DT case null entries in the iommus property).
> > 
> > Now I see that struct dma_attrs has been replaced by unsigned long in
> > 
> > commit 00085f1efa387a8ce100e3734920f7639c80caa3
> > Author: Krzysztof Kozlowski 
> > Date:   Wed Aug 3 13:46:00 2016 -0700
> > 
> > dma-mapping: use unsigned long for dma_attrs
> > 
> > We still have enough bits to reserve some of them for a channel number,
> > but I'm not very happy with that patch as I can see how a future proposal
> > to handle the channel number through the DMA attributes will get rejected
> > on the grounds of bits starvation then :-(
> 
> Agreed, that can become interesting.

Does the above-mentioned patch really fix a performance, memory consumption or 
other issue ?

-- 
Regards,

Laurent Pinchart

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Laurent Pinchart]

Hi Rubin,

On Friday 16 Sep 2016 11:36:29 Robin Murphy wrote:
> On 16/09/16 10:48, Laurent Pinchart wrote:
> > On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
> >> On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:
> >>> On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
>  On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> > Hi,
> > 
> > This series tries to solve the problem with DMA with device registers
> > (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> > recent patch '9575632 (dmaengine: make slave address physical)'
> > clarifies that DMA slave address provided by clients is the physical
> > address. This puts the task of mapping the DMA slave address from a
> > phys_addr_t to a dma_addr_t on the DMA engine.
> > 
> > Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> > the same and no special care is needed. However if you have a IOMMU
> > you need to map the DMA slave phys_addr_t to a dma_addr_t using
> > something like this.
> > 
> > This series is based on top of v4.8-rc1. And I'm hoping to be able to
> > collect a Ack from Russell King on patch 4/6 that adds the ARM
> > specific part and then be able to take the whole series through the
> > dmaengine tree. If this is not the best route I'm more then happy to
> > do it another way.
> > 
> > It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
> > ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting
> > with /dev/mmcblk1, i2c and the serial console which are devices behind
> > the iommu.
>  
>  As I said in last one, the dmaengine parts look fine to me. But to go
>  thru dmaengine tree I would need ACK on non dmaengine patches.
> >>> 
> >>> I havent heard back from this one and I am inclined to merge this one
> >>> now. If anyone has any objects, please speak up now...
> >>> 
> >>> Also ACKs welcome...
> >> 
> >> I had not looked at the series earlier, but this version looks entirely
> >> reasonable to me, so
> >> 
> >> Acked-by: Arnd Bergmann 
> >> 
> >> 
> >> One concern I have is that we might get an awkward situation if we ever
> >> encounter one DMA engine hardware that is used in different systems that
> >> all have an IOMMU, but on some of them the connection between the DMA
> >> master and the slave FIFO bypasses the IOMMU while on others the IOMMU
> >> is required.
> >
> > Do you mean systems where some of the channels of a specific DMA engine go
> > through the IOMMU while others do not ? We indeed have no solution today
> > for such a situation.
> > 
> > The problem is a bit broader than that, we'll also have an issue with DMA
> > engines that have different channels served by different IOMMUs. I recall
> > discussing this in the past with you, and the solution you proposed was to
> > add a channel index to struct dma_attrs seems good to me. To support the
> > case where some channels don't go through an IOMMU we would only need
> > support for null entries in the IOMMUs list associated with a device (for
> > instance in the DT case null entries in the iommus property).
> 
> I think at that point we just create the channels as child devices of
> the main dmaengine device so they each get their own DMA ops, and can do
> whatever. The Qualcomm HIDMA driver already does that for a very similar
> reason (so that the IOMMU can map individual channels into different
> guest VMs).

That's another option, but it seems more like a workaround to me, instead of a 
proper solution to fix the more global problem of multiple memory paths within 
a single device. I have other hardware devices that can act as bus masters 
through different paths (for instance a display-related device that fetches 
data and commands through different paths). Luckily so far all those paths are 
served by the same IOMMU, but there's no guarantee this will remain true in 
the future. Furthermore, even today, the IOMMU connected to that device has 
the ability to selectively enable and disable its ports. I have to keep them 
all enabled due to the lack of channel information in the DMA mapping and 
IOMMU APIs, leading to increased power consumption.

> > Now I see that struct dma_attrs has been replaced by unsigned long in
> > 
> > commit 00085f1efa387a8ce100e3734920f7639c80caa3
> > Author: Krzysztof Kozlowski 
> > Date:   Wed Aug 3 13:46:00 2016 -0700
> > 
> > dma-mapping: use unsigned long for dma_attrs
> > 
> > We still have enough bits to reserve some of them for a channel number,
> > but I'm not very happy with that patch as I can see how a future proposal
> > to handle the channel number through the DMA attributes will get rejected
> > on the grounds of bits starvation then :-(
> > 
> >> I don't have any idea for how this could be handled in a generic way, so
> >> my best answer here is to hope 

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Arnd Bergmann]

On Friday, September 16, 2016 12:48:23 PM CEST Laurent Pinchart wrote:
> On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
> > On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:
> > > On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
> > >> On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> > I had not looked at the series earlier, but this version looks entirely
> > reasonable to me, so
> > 
> > Acked-by: Arnd Bergmann 
> > 
> > 
> > One concern I have is that we might get an awkward situation if we ever
> > encounter one DMA engine hardware that is used in different systems that all
> > have an IOMMU, but on some of them the connection between the DMA master and
> > the slave FIFO bypasses the IOMMU while on others the IOMMU is required.
> 
> Do you mean systems where some of the channels of a specific DMA engine go 
> through the IOMMU while others do not ? We indeed have no solution today for 
> such a situation.

I wasn't thinking quite that far, though that is also a theoretical
problem. However, the simple solution would be to have a bit in the DMA
specifier let the driver know whether translation is needed or not.

The simpler case I was thinking of is where the entire DMA engine
either goes through an IOMMU or doesn't (depending on the integration
into the SoC), so we'd have to find out through some DT property
or compatible string in the DMA enginen driver.

> The problem is a bit broader than that, we'll also have an issue with DMA 
> engines that have different channels served by different IOMMUs.

Do you mean a theoretical problem, or a chip that you already know exists?

> I recall 
> discussing this in the past with you, and the solution you proposed was to 
> add 
> a channel index to struct dma_attrs seems good to me. To support the case 
> where some channels don't go through an IOMMU we would only need support for 
> null entries in the IOMMUs list associated with a device (for instance in the 
> DT case null entries in the iommus property).
> 
> Now I see that struct dma_attrs has been replaced by unsigned long in
> 
> commit 00085f1efa387a8ce100e3734920f7639c80caa3
> Author: Krzysztof Kozlowski 
> Date:   Wed Aug 3 13:46:00 2016 -0700
> 
> dma-mapping: use unsigned long for dma_attrs
> 
> We still have enough bits to reserve some of them for a channel number, but 
> I'm not very happy with that patch as I can see how a future proposal to 
> handle the channel number through the DMA attributes will get rejected on the 
> grounds of bits starvation then :-(

Agreed, that can become interesting.

Arnd

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Robin Murphy]

On 16/09/16 10:48, Laurent Pinchart wrote:
> Hi Arnd,
> 
> On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
>> On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:
>>> On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
 On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> Hi,
>
> This series tries to solve the problem with DMA with device registers
> (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> recent patch '9575632 (dmaengine: make slave address physical)'
> clarifies that DMA slave address provided by clients is the physical
> address. This puts the task of mapping the DMA slave address from a
> phys_addr_t to a dma_addr_t on the DMA engine.
>
> Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> the same and no special care is needed. However if you have a IOMMU
> you need to map the DMA slave phys_addr_t to a dma_addr_t using
> something like this.
>
> This series is based on top of v4.8-rc1. And I'm hoping to be able to
> collect a Ack from Russell King on patch 4/6 that adds the ARM
> specific part and then be able to take the whole series through the
> dmaengine tree. If this is not the best route I'm more then happy to
> do it another way.
>
> It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
> ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting
> with /dev/mmcblk1, i2c and the serial console which are devices behind
> the iommu.

 As I said in last one, the dmaengine parts look fine to me. But to go
 thru dmaengine tree I would need ACK on non dmaengine patches.
>>>
>>> I havent heard back from this one and I am inclined to merge this one now.
>>> If anyone has any objects, please speak up now...
>>>
>>> Also ACKs welcome...
>>
>> I had not looked at the series earlier, but this version looks entirely
>> reasonable to me, so
>>
>> Acked-by: Arnd Bergmann 
>>
>>
>> One concern I have is that we might get an awkward situation if we ever
>> encounter one DMA engine hardware that is used in different systems that all
>> have an IOMMU, but on some of them the connection between the DMA master and
>> the slave FIFO bypasses the IOMMU while on others the IOMMU is required.
> 
> Do you mean systems where some of the channels of a specific DMA engine go 
> through the IOMMU while others do not ? We indeed have no solution today for 
> such a situation.
> 
> The problem is a bit broader than that, we'll also have an issue with DMA 
> engines that have different channels served by different IOMMUs. I recall 
> discussing this in the past with you, and the solution you proposed was to 
> add 
> a channel index to struct dma_attrs seems good to me. To support the case 
> where some channels don't go through an IOMMU we would only need support for 
> null entries in the IOMMUs list associated with a device (for instance in the 
> DT case null entries in the iommus property).

I think at that point we just create the channels as child devices of
the main dmaengine device so they each get their own DMA ops, and can do
whatever. The Qualcomm HIDMA driver already does that for a very similar
reason (so that the IOMMU can map individual channels into different
guest VMs).

Robin.

> Now I see that struct dma_attrs has been replaced by unsigned long in
> 
> commit 00085f1efa387a8ce100e3734920f7639c80caa3
> Author: Krzysztof Kozlowski 
> Date:   Wed Aug 3 13:46:00 2016 -0700
> 
> dma-mapping: use unsigned long for dma_attrs
> 
> We still have enough bits to reserve some of them for a channel number, but 
> I'm not very happy with that patch as I can see how a future proposal to 
> handle the channel number through the DMA attributes will get rejected on the 
> grounds of bits starvation then :-(
> 
>> I don't have any idea for how this could be handled in a generic way, so my
>> best answer here is to hope we never get there, and if we do, handle it
>> using some local hack in the driver.
> 

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Laurent Pinchart]

Hi Arnd,

On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
> On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:
> > On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
> >> On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> >>> Hi,
> >>> 
> >>> This series tries to solve the problem with DMA with device registers
> >>> (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> >>> recent patch '9575632 (dmaengine: make slave address physical)'
> >>> clarifies that DMA slave address provided by clients is the physical
> >>> address. This puts the task of mapping the DMA slave address from a
> >>> phys_addr_t to a dma_addr_t on the DMA engine.
> >>> 
> >>> Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> >>> the same and no special care is needed. However if you have a IOMMU
> >>> you need to map the DMA slave phys_addr_t to a dma_addr_t using
> >>> something like this.
> >>> 
> >>> This series is based on top of v4.8-rc1. And I'm hoping to be able to
> >>> collect a Ack from Russell King on patch 4/6 that adds the ARM
> >>> specific part and then be able to take the whole series through the
> >>> dmaengine tree. If this is not the best route I'm more then happy to
> >>> do it another way.
> >>> 
> >>> It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
> >>> ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting
> >>> with /dev/mmcblk1, i2c and the serial console which are devices behind
> >>> the iommu.
> >> 
> >> As I said in last one, the dmaengine parts look fine to me. But to go
> >> thru dmaengine tree I would need ACK on non dmaengine patches.
> > 
> > I havent heard back from this one and I am inclined to merge this one now.
> > If anyone has any objects, please speak up now...
> > 
> > Also ACKs welcome...
> 
> I had not looked at the series earlier, but this version looks entirely
> reasonable to me, so
> 
> Acked-by: Arnd Bergmann 
> 
> 
> One concern I have is that we might get an awkward situation if we ever
> encounter one DMA engine hardware that is used in different systems that all
> have an IOMMU, but on some of them the connection between the DMA master and
> the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go 
through the IOMMU while others do not ? We indeed have no solution today for 
such a situation.

The problem is a bit broader than that, we'll also have an issue with DMA 
engines that have different channels served by different IOMMUs. I recall 
discussing this in the past with you, and the solution you proposed was to add 
a channel index to struct dma_attrs seems good to me. To support the case 
where some channels don't go through an IOMMU we would only need support for 
null entries in the IOMMUs list associated with a device (for instance in the 
DT case null entries in the iommus property).

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3
Author: Krzysztof Kozlowski 
Date:   Wed Aug 3 13:46:00 2016 -0700

dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but 
I'm not very happy with that patch as I can see how a future proposal to 
handle the channel number through the DMA attributes will get rejected on the 
grounds of bits starvation then :-(

> I don't have any idea for how this could be handled in a generic way, so my
> best answer here is to hope we never get there, and if we do, handle it
> using some local hack in the driver.

-- 
Regards,

Laurent Pinchart

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Arnd Bergmann]

On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:
> On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
> > On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> > > Hi,
> > > 
> > > This series tries to solve the problem with DMA with device registers
> > > (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> > > recent patch '9575632 (dmaengine: make slave address physical)'
> > > clarifies that DMA slave address provided by clients is the physical
> > > address. This puts the task of mapping the DMA slave address from a
> > > phys_addr_t to a dma_addr_t on the DMA engine.
> > > 
> > > Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> > > the same and no special care is needed. However if you have a IOMMU you
> > > need to map the DMA slave phys_addr_t to a dma_addr_t using something
> > > like this.
> > > 
> > > This series is based on top of v4.8-rc1. And I'm hoping to be able to 
> > > collect a
> > > Ack from Russell King on patch 4/6 that adds the ARM specific part and 
> > > then be
> > > able to take the whole series through the dmaengine tree. If this is not 
> > > the
> > > best route I'm more then happy to do it another way.
> > > 
> > > It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
> > > ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with
> > > /dev/mmcblk1, i2c and the serial console which are devices behind the
> > > iommu.
> > 
> > As I said in last one, the dmaengine parts look fine to me. But to go thru
> > dmaengine tree I would need ACK on non dmaengine patches.
> 
> I havent heard back from this one and I am inclined to merge this one now.
> If anyone has any objects, please speak up now...
> 
> Also ACKs welcome...
> 


I had not looked at the series earlier, but this version looks entirely
reasonable to me, so

Acked-by: Arnd Bergmann 


One concern I have is that we might get an awkward situation if we
ever encounter one DMA engine hardware that is used in different
systems that all have an IOMMU, but on some of them the connection
between the DMA master and the slave FIFO bypasses the IOMMU
while on others the IOMMU is required. I don't have any idea for
how this could be handled in a generic way, so my best answer
here is to hope we never get there, and if we do, handle it
using some local hack in the driver.

Arnd

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Vinod Koul]

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
> On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> > Hi,
> > 
> > This series tries to solve the problem with DMA with device registers
> > (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> > recent patch '9575632 (dmaengine: make slave address physical)'
> > clarifies that DMA slave address provided by clients is the physical
> > address. This puts the task of mapping the DMA slave address from a
> > phys_addr_t to a dma_addr_t on the DMA engine.
> > 
> > Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> > the same and no special care is needed. However if you have a IOMMU you
> > need to map the DMA slave phys_addr_t to a dma_addr_t using something
> > like this.
> > 
> > This series is based on top of v4.8-rc1. And I'm hoping to be able to 
> > collect a
> > Ack from Russell King on patch 4/6 that adds the ARM specific part and then 
> > be
> > able to take the whole series through the dmaengine tree. If this is not the
> > best route I'm more then happy to do it another way.
> > 
> > It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
> > ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with
> > /dev/mmcblk1, i2c and the serial console which are devices behind the
> > iommu.
> 
> As I said in last one, the dmaengine parts look fine to me. But to go thru
> dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now.
If anyone has any objects, please speak up now...

Also ACKs welcome...

-- 
~Vinod

Re: [PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Vinod Koul]

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> Hi,
> 
> This series tries to solve the problem with DMA with device registers
> (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> recent patch '9575632 (dmaengine: make slave address physical)'
> clarifies that DMA slave address provided by clients is the physical
> address. This puts the task of mapping the DMA slave address from a
> phys_addr_t to a dma_addr_t on the DMA engine.
> 
> Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> the same and no special care is needed. However if you have a IOMMU you
> need to map the DMA slave phys_addr_t to a dma_addr_t using something
> like this.
> 
> This series is based on top of v4.8-rc1. And I'm hoping to be able to collect 
> a
> Ack from Russell King on patch 4/6 that adds the ARM specific part and then be
> able to take the whole series through the dmaengine tree. If this is not the
> best route I'm more then happy to do it another way.
> 
> It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
> ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with
> /dev/mmcblk1, i2c and the serial console which are devices behind the
> iommu.

As I said in last one, the dmaengine parts look fine to me. But to go thru
dmaengine tree I would need ACK on non dmaengine patches.


-- 
~Vinod