[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[marek.vasut]

From: Marek Vasut <marek.vasut+renesas@gmail.com>

Since commit 6c2fb2ea7636 ("of/device: Set bus DMA mask as appropriate"),
the upstream bus can constraint device DMA range. Respect that constraint
and do not change the device DMA masks if they were already set.

This is applicable e.g. on systems where the PCIe controller cannot expose
the full address space range. Such a system may have a 64bit CPU with DRAM
mapped both below and above the 32bit address space, yet the PCIe devices
can not perform DMA directly to/from the DRAM range above the 32bit limit.
Hence, for such setup to work, all the buffers must exist below the 32bit
limit.

Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Geert Uytterhoeven <geert+renesas at glider.be>
Cc: Jens Axboe <axboe at fb.com>
Cc: Jens Axboe <axboe at kernel.dk>
Cc: Keith Busch <keith.busch at intel.com>
Cc: Robin Murphy <robin.murphy at arm.com>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Wolfram Sang <wsa+renesas at sang-engineering.com>
Cc: linux-renesas-soc at vger.kernel.org
To: linux-ide at vger.kernel.org
To: linux-nvme at lists.infradead.org
---
 drivers/ata/ahci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 021ce46e2e57..2acce056dd8c 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -926,6 +926,13 @@ static int ahci_configure_dma_masks(struct pci_dev *pdev, int using_dac)
 	if (pdev->dma_mask && pdev->dma_mask < DMA_BIT_MASK(32))
 		return 0;
 
+	/*
+	 * The upstream device could have applied DMA constraints already,
+	 * respect those and do not change the DMA masks.
+	 */
+	if (pdev->dev.dma_mask && pdev->dev.coherent_dma_mask)
+		return 0;
+
 	if (using_dac &&
 	    !dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
 		rc = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
-- 
2.20.1

[PATCH 2/2] [RFC] nvme-pci: Respect bus DMA constraints

[marek.vasut]

From: Marek Vasut <marek.vasut+renesas@gmail.com>

Since commit 6c2fb2ea7636 ("of/device: Set bus DMA mask as appropriate"),
the upstream bus can constraint device DMA range. Respect that constraint
and do not change the device DMA masks if they were already set.

This is applicable e.g. on systems where the PCIe controller cannot expose
the full address space range. Such a system may have a 64bit CPU with DRAM
mapped both below and above the 32bit address space, yet the PCIe devices
can not perform DMA directly to/from the DRAM range above the 32bit limit.
Hence, for such setup to work, all the buffers must exist below the 32bit
limit.

Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Geert Uytterhoeven <geert+renesas at glider.be>
Cc: Jens Axboe <axboe at fb.com>
Cc: Jens Axboe <axboe at kernel.dk>
Cc: Keith Busch <keith.busch at intel.com>
Cc: Robin Murphy <robin.murphy at arm.com>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Wolfram Sang <wsa+renesas at sang-engineering.com>
Cc: linux-renesas-soc at vger.kernel.org
To: linux-ide at vger.kernel.org
To: linux-nvme at lists.infradead.org
---
 drivers/nvme/host/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 92bad1c810ac..8ec873fb86b1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2323,7 +2323,8 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 
 	pci_set_master(pdev);
 
-	if (dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(64)) &&
+	if (!dev->dev->coherent_dma_mask &&
+	    dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(64)) &&
 	    dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(32)))
 		goto disable;
 
-- 
2.20.1

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Robin Murphy]

On 2019-03-07 12:04 am, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas at gmail.com>
> 
> Since commit 6c2fb2ea7636 ("of/device: Set bus DMA mask as appropriate"),
> the upstream bus can constraint device DMA range. Respect that constraint
> and do not change the device DMA masks if they were already set.
> 
> This is applicable e.g. on systems where the PCIe controller cannot expose
> the full address space range. Such a system may have a 64bit CPU with DRAM
> mapped both below and above the 32bit address space, yet the PCIe devices
> can not perform DMA directly to/from the DRAM range above the 32bit limit.
> Hence, for such setup to work, all the buffers must exist below the 32bit
> limit.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Geert Uytterhoeven <geert+renesas at glider.be>
> Cc: Jens Axboe <axboe at fb.com>
> Cc: Jens Axboe <axboe at kernel.dk>
> Cc: Keith Busch <keith.busch at intel.com>
> Cc: Robin Murphy <robin.murphy at arm.com>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> Cc: Wolfram Sang <wsa+renesas at sang-engineering.com>
> Cc: linux-renesas-soc at vger.kernel.org
> To: linux-ide at vger.kernel.org
> To: linux-nvme at lists.infradead.org
> ---
>   drivers/ata/ahci.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 021ce46e2e57..2acce056dd8c 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -926,6 +926,13 @@ static int ahci_configure_dma_masks(struct pci_dev *pdev, int using_dac)
>   	if (pdev->dma_mask && pdev->dma_mask < DMA_BIT_MASK(32))
>   		return 0;
>   
> +	/*
> +	 * The upstream device could have applied DMA constraints already,
> +	 * respect those and do not change the DMA masks.
> +	 */
> +	if (pdev->dev.dma_mask && pdev->dev.coherent_dma_mask)
> +		return 0;

At least for DT platforms, the device masks are always going to be set 
to some initial value, which will most commonly just be the 32-bit 
default - that should not prevent the driver from setting wider masks if 
that's what the device really supports (in fact there are some patches 
queued in which we're now starting to formalise that properly).

Are you seeing a problem with a DMA API backend failing to respect 
bus_dma_mask?

Robin.

> +
>   	if (using_dac &&
>   	    !dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
>   		rc = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
> 

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/7/19 10:32 AM, Robin Murphy wrote:
> On 2019-03-07 12:04 am, marek.vasut@gmail.com wrote:
>> From: Marek Vasut <marek.vasut+renesas at gmail.com>
>>
>> Since commit 6c2fb2ea7636 ("of/device: Set bus DMA mask as appropriate"),
>> the upstream bus can constraint device DMA range. Respect that constraint
>> and do not change the device DMA masks if they were already set.
>>
>> This is applicable e.g. on systems where the PCIe controller cannot
>> expose
>> the full address space range. Such a system may have a 64bit CPU with
>> DRAM
>> mapped both below and above the 32bit address space, yet the PCIe devices
>> can not perform DMA directly to/from the DRAM range above the 32bit
>> limit.
>> Hence, for such setup to work, all the buffers must exist below the 32bit
>> limit.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
>> Cc: Christoph Hellwig <hch at lst.de>
>> Cc: Geert Uytterhoeven <geert+renesas at glider.be>
>> Cc: Jens Axboe <axboe at fb.com>
>> Cc: Jens Axboe <axboe at kernel.dk>
>> Cc: Keith Busch <keith.busch at intel.com>
>> Cc: Robin Murphy <robin.murphy at arm.com>
>> Cc: Sagi Grimberg <sagi at grimberg.me>
>> Cc: Wolfram Sang <wsa+renesas at sang-engineering.com>
>> Cc: linux-renesas-soc at vger.kernel.org
>> To: linux-ide at vger.kernel.org
>> To: linux-nvme at lists.infradead.org
>> ---
>> ? drivers/ata/ahci.c | 7 +++++++
>> ? 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 021ce46e2e57..2acce056dd8c 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -926,6 +926,13 @@ static int ahci_configure_dma_masks(struct
>> pci_dev *pdev, int using_dac)
>> ????? if (pdev->dma_mask && pdev->dma_mask < DMA_BIT_MASK(32))
>> ????????? return 0;
>> ? +??? /*
>> +???? * The upstream device could have applied DMA constraints already,
>> +???? * respect those and do not change the DMA masks.
>> +???? */
>> +??? if (pdev->dev.dma_mask && pdev->dev.coherent_dma_mask)
>> +??????? return 0;
> 
> At least for DT platforms, the device masks are always going to be set
> to some initial value, which will most commonly just be the 32-bit
> default - that should not prevent the driver from setting wider masks if
> that's what the device really supports (in fact there are some patches
> queued in which we're now starting to formalise that properly).
> 
> Are you seeing a problem with a DMA API backend failing to respect
> bus_dma_mask?

Yes, the DMA mask gets overridden here to 64bit one, which on the R-Car
Gen3 with PCI with 32bit addressing limitation makes the AHCI driver
fail (and NVMe driver, and xHCI PCI etc). All those PCI devices fail the
same way because they override the DMA mask.

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Robin Murphy]

On 2019-03-07 9:37 am, Marek Vasut wrote:
> On 3/7/19 10:32 AM, Robin Murphy wrote:
>> On 2019-03-07 12:04 am, marek.vasut@gmail.com wrote:
>>> From: Marek Vasut <marek.vasut+renesas at gmail.com>
>>>
>>> Since commit 6c2fb2ea7636 ("of/device: Set bus DMA mask as appropriate"),
>>> the upstream bus can constraint device DMA range. Respect that constraint
>>> and do not change the device DMA masks if they were already set.
>>>
>>> This is applicable e.g. on systems where the PCIe controller cannot
>>> expose
>>> the full address space range. Such a system may have a 64bit CPU with
>>> DRAM
>>> mapped both below and above the 32bit address space, yet the PCIe devices
>>> can not perform DMA directly to/from the DRAM range above the 32bit
>>> limit.
>>> Hence, for such setup to work, all the buffers must exist below the 32bit
>>> limit.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
>>> Cc: Christoph Hellwig <hch at lst.de>
>>> Cc: Geert Uytterhoeven <geert+renesas at glider.be>
>>> Cc: Jens Axboe <axboe at fb.com>
>>> Cc: Jens Axboe <axboe at kernel.dk>
>>> Cc: Keith Busch <keith.busch at intel.com>
>>> Cc: Robin Murphy <robin.murphy at arm.com>
>>> Cc: Sagi Grimberg <sagi at grimberg.me>
>>> Cc: Wolfram Sang <wsa+renesas at sang-engineering.com>
>>> Cc: linux-renesas-soc at vger.kernel.org
>>> To: linux-ide at vger.kernel.org
>>> To: linux-nvme at lists.infradead.org
>>> ---
>>>  ? drivers/ata/ahci.c | 7 +++++++
>>>  ? 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>> index 021ce46e2e57..2acce056dd8c 100644
>>> --- a/drivers/ata/ahci.c
>>> +++ b/drivers/ata/ahci.c
>>> @@ -926,6 +926,13 @@ static int ahci_configure_dma_masks(struct
>>> pci_dev *pdev, int using_dac)
>>>  ????? if (pdev->dma_mask && pdev->dma_mask < DMA_BIT_MASK(32))
>>>  ????????? return 0;
>>>  ? +??? /*
>>> +???? * The upstream device could have applied DMA constraints already,
>>> +???? * respect those and do not change the DMA masks.
>>> +???? */
>>> +??? if (pdev->dev.dma_mask && pdev->dev.coherent_dma_mask)
>>> +??????? return 0;
>>
>> At least for DT platforms, the device masks are always going to be set
>> to some initial value, which will most commonly just be the 32-bit
>> default - that should not prevent the driver from setting wider masks if
>> that's what the device really supports (in fact there are some patches
>> queued in which we're now starting to formalise that properly).
>>
>> Are you seeing a problem with a DMA API backend failing to respect
>> bus_dma_mask?
> 
> Yes, the DMA mask gets overridden here to 64bit one, which on the R-Car
> Gen3 with PCI with 32bit addressing limitation makes the AHCI driver
> fail (and NVMe driver, and xHCI PCI etc). All those PCI devices fail the
> same way because they override the DMA mask.

Right, but whoever *interprets* the device masks after the driver has 
overridden them should be taking the (smaller) bus mask into account as 
well, so the question is where is *that* not being done correctly?

Robin.

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/7/19 10:48 AM, Robin Murphy wrote:
> On 2019-03-07 9:37 am, Marek Vasut wrote:
>> On 3/7/19 10:32 AM, Robin Murphy wrote:
>>> On 2019-03-07 12:04 am, marek.vasut@gmail.com wrote:
>>>> From: Marek Vasut <marek.vasut+renesas at gmail.com>
>>>>
>>>> Since commit 6c2fb2ea7636 ("of/device: Set bus DMA mask as
>>>> appropriate"),
>>>> the upstream bus can constraint device DMA range. Respect that
>>>> constraint
>>>> and do not change the device DMA masks if they were already set.
>>>>
>>>> This is applicable e.g. on systems where the PCIe controller cannot
>>>> expose
>>>> the full address space range. Such a system may have a 64bit CPU with
>>>> DRAM
>>>> mapped both below and above the 32bit address space, yet the PCIe
>>>> devices
>>>> can not perform DMA directly to/from the DRAM range above the 32bit
>>>> limit.
>>>> Hence, for such setup to work, all the buffers must exist below the
>>>> 32bit
>>>> limit.
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
>>>> Cc: Christoph Hellwig <hch at lst.de>
>>>> Cc: Geert Uytterhoeven <geert+renesas at glider.be>
>>>> Cc: Jens Axboe <axboe at fb.com>
>>>> Cc: Jens Axboe <axboe at kernel.dk>
>>>> Cc: Keith Busch <keith.busch at intel.com>
>>>> Cc: Robin Murphy <robin.murphy at arm.com>
>>>> Cc: Sagi Grimberg <sagi at grimberg.me>
>>>> Cc: Wolfram Sang <wsa+renesas at sang-engineering.com>
>>>> Cc: linux-renesas-soc at vger.kernel.org
>>>> To: linux-ide at vger.kernel.org
>>>> To: linux-nvme at lists.infradead.org
>>>> ---
>>>> ?? drivers/ata/ahci.c | 7 +++++++
>>>> ?? 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>> index 021ce46e2e57..2acce056dd8c 100644
>>>> --- a/drivers/ata/ahci.c
>>>> +++ b/drivers/ata/ahci.c
>>>> @@ -926,6 +926,13 @@ static int ahci_configure_dma_masks(struct
>>>> pci_dev *pdev, int using_dac)
>>>> ?????? if (pdev->dma_mask && pdev->dma_mask < DMA_BIT_MASK(32))
>>>> ?????????? return 0;
>>>> ?? +??? /*
>>>> +???? * The upstream device could have applied DMA constraints already,
>>>> +???? * respect those and do not change the DMA masks.
>>>> +???? */
>>>> +??? if (pdev->dev.dma_mask && pdev->dev.coherent_dma_mask)
>>>> +??????? return 0;
>>>
>>> At least for DT platforms, the device masks are always going to be set
>>> to some initial value, which will most commonly just be the 32-bit
>>> default - that should not prevent the driver from setting wider masks if
>>> that's what the device really supports (in fact there are some patches
>>> queued in which we're now starting to formalise that properly).
>>>
>>> Are you seeing a problem with a DMA API backend failing to respect
>>> bus_dma_mask?
>>
>> Yes, the DMA mask gets overridden here to 64bit one, which on the R-Car
>> Gen3 with PCI with 32bit addressing limitation makes the AHCI driver
>> fail (and NVMe driver, and xHCI PCI etc). All those PCI devices fail the
>> same way because they override the DMA mask.
> 
> Right, but whoever *interprets* the device masks after the driver has
> overridden them should be taking the (smaller) bus mask into account as
> well, so the question is where is *that* not being done correctly?

Do you have a hint where I should look for that ?

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Christoph Hellwig]

On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
> > Right, but whoever *interprets* the device masks after the driver has
> > overridden them should be taking the (smaller) bus mask into account as
> > well, so the question is where is *that* not being done correctly?
> 
> Do you have a hint where I should look for that ?

If this a 32-bit ARM platform it might the complete lack of support
for bus_dma_mask in arch/arm/mm/dma-mapping.c..

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/8/19 8:18 AM, Christoph Hellwig wrote:
> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
>>> Right, but whoever *interprets* the device masks after the driver has
>>> overridden them should be taking the (smaller) bus mask into account as
>>> well, so the question is where is *that* not being done correctly?
>>
>> Do you have a hint where I should look for that ?
> 
> If this a 32-bit ARM platform it might the complete lack of support
> for bus_dma_mask in arch/arm/mm/dma-mapping.c..

It's an ARM 64bit platform, just the PCIe controller is limited to 32bit
address range, so the devices on the PCIe bus cannot read the host's
DRAM above the 32bit limit.

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Christoph Hellwig]

On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
> > On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
> >>> Right, but whoever *interprets* the device masks after the driver has
> >>> overridden them should be taking the (smaller) bus mask into account as
> >>> well, so the question is where is *that* not being done correctly?
> >>
> >> Do you have a hint where I should look for that ?
> > 
> > If this a 32-bit ARM platform it might the complete lack of support
> > for bus_dma_mask in arch/arm/mm/dma-mapping.c..
> 
> It's an ARM 64bit platform, just the PCIe controller is limited to 32bit
> address range, so the devices on the PCIe bus cannot read the host's
> DRAM above the 32bit limit.

arm64 should take the mask into account both for the swiotlb and
iommu case.  What are the exact symptoms you see?  Does it involve
swiotlb not kicking in, or iommu issues?  What is the exact kernel
version?

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/13/19 7:30 PM, Christoph Hellwig wrote:
> On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
>> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
>>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
>>>>> Right, but whoever *interprets* the device masks after the driver has
>>>>> overridden them should be taking the (smaller) bus mask into account as
>>>>> well, so the question is where is *that* not being done correctly?
>>>>
>>>> Do you have a hint where I should look for that ?
>>>
>>> If this a 32-bit ARM platform it might the complete lack of support
>>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
>>
>> It's an ARM 64bit platform, just the PCIe controller is limited to 32bit
>> address range, so the devices on the PCIe bus cannot read the host's
>> DRAM above the 32bit limit.
> 
> arm64 should take the mask into account both for the swiotlb and
> iommu case.  What are the exact symptoms you see?

With the nvme, the device is recognized, but cannot be used.
It boils down to PCI BAR access being possible, since that's all below
the 32bit boundary, but when the device tries to do any sort of DMA,
that transfer returns nonsense data.

But when I call dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(32) in
the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
drivers), it all starts to work fine.

Could it be that the driver overwrites the (coherent_)dma_mask and
that's why the swiotlb/iommu code cannot take this into account ?

> Does it involve
> swiotlb not kicking in, or iommu issues?

How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.

> What is the exact kernel version?

next/master from 20190306 (5.0.0 + next patches)

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/16/19 10:25 PM, Marek Vasut wrote:
> On 3/13/19 7:30 PM, Christoph Hellwig wrote:
>> On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
>>> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
>>>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
>>>>>> Right, but whoever *interprets* the device masks after the driver has
>>>>>> overridden them should be taking the (smaller) bus mask into account as
>>>>>> well, so the question is where is *that* not being done correctly?
>>>>>
>>>>> Do you have a hint where I should look for that ?
>>>>
>>>> If this a 32-bit ARM platform it might the complete lack of support
>>>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
>>>
>>> It's an ARM 64bit platform, just the PCIe controller is limited to 32bit
>>> address range, so the devices on the PCIe bus cannot read the host's
>>> DRAM above the 32bit limit.
>>
>> arm64 should take the mask into account both for the swiotlb and
>> iommu case.  What are the exact symptoms you see?
> 
> With the nvme, the device is recognized, but cannot be used.
> It boils down to PCI BAR access being possible, since that's all below
> the 32bit boundary, but when the device tries to do any sort of DMA,
> that transfer returns nonsense data.
> 
> But when I call dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(32) in
> the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
> drivers), it all starts to work fine.
> 
> Could it be that the driver overwrites the (coherent_)dma_mask and
> that's why the swiotlb/iommu code cannot take this into account ?
> 
>> Does it involve
>> swiotlb not kicking in, or iommu issues?
> 
> How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
> drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.

Digging further ...

drivers/nvme/host/pci.c nvme_map_data() calls dma_map_sg_attrs() and the
resulting sglist contains entry with >32bit PA. This is because
dma_map_sg_attrs() calls dma_direct_map_sg(), which in turn calls
dma_direct_map_sg(), then dma_direct_map_page() and that's where it goes
weird.

dma_direct_map_page() does a dma_direct_possible() check before
triggering swiotlb_map(). The check succeeds, so the later isn't executed.

dma_direct_possible() calls dma_capable() with dev->dma_mask =
DMA_BIT_MASK(64) and dev->dma_bus_mask = 0, so
min_not_zero(*dev->dma_mask, dev->bus_dma_mask) returns DMA_BIT_MASK(64).

Surely enough, if I hack dma_direct_possible() to return 0,
swiotlb_map() kicks in and the nvme driver starts working fine.

I presume the question here is, why is dev->bus_dma_mask = 0 ?

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Geert Uytterhoeven]

Hi Marek,

On Sun, Mar 17, 2019@12:04 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 3/16/19 10:25 PM, Marek Vasut wrote:
> > On 3/13/19 7:30 PM, Christoph Hellwig wrote:
> >> On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
> >>> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
> >>>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
> >>>>>> Right, but whoever *interprets* the device masks after the driver has
> >>>>>> overridden them should be taking the (smaller) bus mask into account as
> >>>>>> well, so the question is where is *that* not being done correctly?
> >>>>>
> >>>>> Do you have a hint where I should look for that ?
> >>>>
> >>>> If this a 32-bit ARM platform it might the complete lack of support
> >>>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
> >>>
> >>> It's an ARM 64bit platform, just the PCIe controller is limited to 32bit
> >>> address range, so the devices on the PCIe bus cannot read the host's
> >>> DRAM above the 32bit limit.
> >>
> >> arm64 should take the mask into account both for the swiotlb and
> >> iommu case.  What are the exact symptoms you see?
> >
> > With the nvme, the device is recognized, but cannot be used.
> > It boils down to PCI BAR access being possible, since that's all below
> > the 32bit boundary, but when the device tries to do any sort of DMA,
> > that transfer returns nonsense data.
> >
> > But when I call dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(32) in
> > the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
> > drivers), it all starts to work fine.
> >
> > Could it be that the driver overwrites the (coherent_)dma_mask and
> > that's why the swiotlb/iommu code cannot take this into account ?
> >
> >> Does it involve
> >> swiotlb not kicking in, or iommu issues?
> >
> > How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
> > drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.
>
> Digging further ...
>
> drivers/nvme/host/pci.c nvme_map_data() calls dma_map_sg_attrs() and the
> resulting sglist contains entry with >32bit PA. This is because
> dma_map_sg_attrs() calls dma_direct_map_sg(), which in turn calls
> dma_direct_map_sg(), then dma_direct_map_page() and that's where it goes
> weird.
>
> dma_direct_map_page() does a dma_direct_possible() check before
> triggering swiotlb_map(). The check succeeds, so the later isn't executed.
>
> dma_direct_possible() calls dma_capable() with dev->dma_mask =
> DMA_BIT_MASK(64) and dev->dma_bus_mask = 0, so
> min_not_zero(*dev->dma_mask, dev->bus_dma_mask) returns DMA_BIT_MASK(64).
>
> Surely enough, if I hack dma_direct_possible() to return 0,
> swiotlb_map() kicks in and the nvme driver starts working fine.
>
> I presume the question here is, why is dev->bus_dma_mask = 0 ?

Because that's the default, and almost no code overrides that?

$ git grep "\<bus_dma_mask ="
arch/mips/pci/fixup-sb1250.c:           dev->dev.bus_dma_mask =
DMA_BIT_MASK(32);
arch/x86/kernel/pci-dma.c:      pdev->dev.bus_dma_mask = DMA_BIT_MASK(32);
drivers/acpi/arm64/iort.c:              dev->bus_dma_mask = mask;
drivers/of/device.c:            dev->bus_dma_mask = mask;

dev is the nvme PCI device, I assume? So you can ignore the last match.

The first two seem to be related to platforms that cannot do >32 bit DMA
on PCI. So that's a hint on how to fix this...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/17/19 11:29 AM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Sun, Mar 17, 2019@12:04 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 3/16/19 10:25 PM, Marek Vasut wrote:
>>> On 3/13/19 7:30 PM, Christoph Hellwig wrote:
>>>> On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
>>>>> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
>>>>>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
>>>>>>>> Right, but whoever *interprets* the device masks after the driver has
>>>>>>>> overridden them should be taking the (smaller) bus mask into account as
>>>>>>>> well, so the question is where is *that* not being done correctly?
>>>>>>>
>>>>>>> Do you have a hint where I should look for that ?
>>>>>>
>>>>>> If this a 32-bit ARM platform it might the complete lack of support
>>>>>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
>>>>>
>>>>> It's an ARM 64bit platform, just the PCIe controller is limited to 32bit
>>>>> address range, so the devices on the PCIe bus cannot read the host's
>>>>> DRAM above the 32bit limit.
>>>>
>>>> arm64 should take the mask into account both for the swiotlb and
>>>> iommu case.  What are the exact symptoms you see?
>>>
>>> With the nvme, the device is recognized, but cannot be used.
>>> It boils down to PCI BAR access being possible, since that's all below
>>> the 32bit boundary, but when the device tries to do any sort of DMA,
>>> that transfer returns nonsense data.
>>>
>>> But when I call dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(32) in
>>> the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
>>> drivers), it all starts to work fine.
>>>
>>> Could it be that the driver overwrites the (coherent_)dma_mask and
>>> that's why the swiotlb/iommu code cannot take this into account ?
>>>
>>>> Does it involve
>>>> swiotlb not kicking in, or iommu issues?
>>>
>>> How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
>>> drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.
>>
>> Digging further ...
>>
>> drivers/nvme/host/pci.c nvme_map_data() calls dma_map_sg_attrs() and the
>> resulting sglist contains entry with >32bit PA. This is because
>> dma_map_sg_attrs() calls dma_direct_map_sg(), which in turn calls
>> dma_direct_map_sg(), then dma_direct_map_page() and that's where it goes
>> weird.
>>
>> dma_direct_map_page() does a dma_direct_possible() check before
>> triggering swiotlb_map(). The check succeeds, so the later isn't executed.
>>
>> dma_direct_possible() calls dma_capable() with dev->dma_mask =
>> DMA_BIT_MASK(64) and dev->dma_bus_mask = 0, so
>> min_not_zero(*dev->dma_mask, dev->bus_dma_mask) returns DMA_BIT_MASK(64).
>>
>> Surely enough, if I hack dma_direct_possible() to return 0,
>> swiotlb_map() kicks in and the nvme driver starts working fine.
>>
>> I presume the question here is, why is dev->bus_dma_mask = 0 ?
> 
> Because that's the default, and almost no code overrides that?

But shouldn't drivers/of/device.c set that for the PCIe controller ?

> $ git grep "\<bus_dma_mask ="
> arch/mips/pci/fixup-sb1250.c:           dev->dev.bus_dma_mask =
> DMA_BIT_MASK(32);
> arch/x86/kernel/pci-dma.c:      pdev->dev.bus_dma_mask = DMA_BIT_MASK(32);
> drivers/acpi/arm64/iort.c:              dev->bus_dma_mask = mask;
> drivers/of/device.c:            dev->bus_dma_mask = mask;
> 
> dev is the nvme PCI device, I assume? So you can ignore the last match.
> 
> The first two seem to be related to platforms that cannot do >32 bit DMA
> on PCI. So that's a hint on how to fix this...

That doesn't feel right, it's not a platform limitation, but a PCIe IP
limitation, so this fix should live somewhere in drivers/ I think ?

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Robin Murphy]

On 17/03/2019 23:36, Marek Vasut wrote:
> On 3/17/19 11:29 AM, Geert Uytterhoeven wrote:
>> Hi Marek,
> 
> Hi,
> 
>> On Sun, Mar 17, 2019@12:04 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 3/16/19 10:25 PM, Marek Vasut wrote:
>>>> On 3/13/19 7:30 PM, Christoph Hellwig wrote:
>>>>> On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
>>>>>> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
>>>>>>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
>>>>>>>>> Right, but whoever *interprets* the device masks after the driver has
>>>>>>>>> overridden them should be taking the (smaller) bus mask into account as
>>>>>>>>> well, so the question is where is *that* not being done correctly?
>>>>>>>>
>>>>>>>> Do you have a hint where I should look for that ?
>>>>>>>
>>>>>>> If this a 32-bit ARM platform it might the complete lack of support
>>>>>>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
>>>>>>
>>>>>> It's an ARM 64bit platform, just the PCIe controller is limited to 32bit
>>>>>> address range, so the devices on the PCIe bus cannot read the host's
>>>>>> DRAM above the 32bit limit.
>>>>>
>>>>> arm64 should take the mask into account both for the swiotlb and
>>>>> iommu case.  What are the exact symptoms you see?
>>>>
>>>> With the nvme, the device is recognized, but cannot be used.
>>>> It boils down to PCI BAR access being possible, since that's all below
>>>> the 32bit boundary, but when the device tries to do any sort of DMA,
>>>> that transfer returns nonsense data.
>>>>
>>>> But when I call dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(32) in
>>>> the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
>>>> drivers), it all starts to work fine.
>>>>
>>>> Could it be that the driver overwrites the (coherent_)dma_mask and
>>>> that's why the swiotlb/iommu code cannot take this into account ?
>>>>
>>>>> Does it involve
>>>>> swiotlb not kicking in, or iommu issues?
>>>>
>>>> How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
>>>> drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.
>>>
>>> Digging further ...
>>>
>>> drivers/nvme/host/pci.c nvme_map_data() calls dma_map_sg_attrs() and the
>>> resulting sglist contains entry with >32bit PA. This is because
>>> dma_map_sg_attrs() calls dma_direct_map_sg(), which in turn calls
>>> dma_direct_map_sg(), then dma_direct_map_page() and that's where it goes
>>> weird.
>>>
>>> dma_direct_map_page() does a dma_direct_possible() check before
>>> triggering swiotlb_map(). The check succeeds, so the later isn't executed.
>>>
>>> dma_direct_possible() calls dma_capable() with dev->dma_mask =
>>> DMA_BIT_MASK(64) and dev->dma_bus_mask = 0, so
>>> min_not_zero(*dev->dma_mask, dev->bus_dma_mask) returns DMA_BIT_MASK(64).
>>>
>>> Surely enough, if I hack dma_direct_possible() to return 0,
>>> swiotlb_map() kicks in and the nvme driver starts working fine.
>>>
>>> I presume the question here is, why is dev->bus_dma_mask = 0 ?
>>
>> Because that's the default, and almost no code overrides that?
> 
> But shouldn't drivers/of/device.c set that for the PCIe controller ?

Urgh, I really should have spotted the significance of "NVMe", but 
somehow it failed to click :(

Of course the existing code works fine for everything *except* PCI 
devices on DT-based systems... That's because of_dma_get_range() has 
never been made to work correctly with the trick we play of passing the 
host bridge of_node through of_dma_configure(). I've got at least 2 or 3 
half-finished attempts at improving that, but they keep getting 
sidetracked into trying to clean up the various new of_dma_configure() 
hacks I find in drivers and/or falling down the rabbit-hole of starting 
to redesign the whole dma_pfn_offset machinery entirely. Let me dig one 
up and try to constrain it to solve just this most common "one single 
limited range" condition for the sake of making actual progress...

Robin.

>> $ git grep "\<bus_dma_mask ="
>> arch/mips/pci/fixup-sb1250.c:           dev->dev.bus_dma_mask =
>> DMA_BIT_MASK(32);
>> arch/x86/kernel/pci-dma.c:      pdev->dev.bus_dma_mask = DMA_BIT_MASK(32);
>> drivers/acpi/arm64/iort.c:              dev->bus_dma_mask = mask;
>> drivers/of/device.c:            dev->bus_dma_mask = mask;
>>
>> dev is the nvme PCI device, I assume? So you can ignore the last match.
>>
>> The first two seem to be related to platforms that cannot do >32 bit DMA
>> on PCI. So that's a hint on how to fix this...
> 
> That doesn't feel right, it's not a platform limitation, but a PCIe IP
> limitation, so this fix should live somewhere in drivers/ I think ? >

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/18/19 2:14 PM, Robin Murphy wrote:
> On 17/03/2019 23:36, Marek Vasut wrote:
>> On 3/17/19 11:29 AM, Geert Uytterhoeven wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Sun, Mar 17, 2019 at 12:04 AM Marek Vasut <marek.vasut at gmail.com>
>>> wrote:
>>>> On 3/16/19 10:25 PM, Marek Vasut wrote:
>>>>> On 3/13/19 7:30 PM, Christoph Hellwig wrote:
>>>>>> On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
>>>>>>> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
>>>>>>>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
>>>>>>>>>> Right, but whoever *interprets* the device masks after the
>>>>>>>>>> driver has
>>>>>>>>>> overridden them should be taking the (smaller) bus mask into
>>>>>>>>>> account as
>>>>>>>>>> well, so the question is where is *that* not being done
>>>>>>>>>> correctly?
>>>>>>>>>
>>>>>>>>> Do you have a hint where I should look for that ?
>>>>>>>>
>>>>>>>> If this a 32-bit ARM platform it might the complete lack of support
>>>>>>>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
>>>>>>>
>>>>>>> It's an ARM 64bit platform, just the PCIe controller is limited
>>>>>>> to 32bit
>>>>>>> address range, so the devices on the PCIe bus cannot read the host's
>>>>>>> DRAM above the 32bit limit.
>>>>>>
>>>>>> arm64 should take the mask into account both for the swiotlb and
>>>>>> iommu case.? What are the exact symptoms you see?
>>>>>
>>>>> With the nvme, the device is recognized, but cannot be used.
>>>>> It boils down to PCI BAR access being possible, since that's all below
>>>>> the 32bit boundary, but when the device tries to do any sort of DMA,
>>>>> that transfer returns nonsense data.
>>>>>
>>>>> But when I call dma_set_mask_and_coherent(dev->dev,
>>>>> DMA_BIT_MASK(32) in
>>>>> the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
>>>>> drivers), it all starts to work fine.
>>>>>
>>>>> Could it be that the driver overwrites the (coherent_)dma_mask and
>>>>> that's why the swiotlb/iommu code cannot take this into account ?
>>>>>
>>>>>> Does it involve
>>>>>> swiotlb not kicking in, or iommu issues?
>>>>>
>>>>> How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
>>>>> drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.
>>>>
>>>> Digging further ...
>>>>
>>>> drivers/nvme/host/pci.c nvme_map_data() calls dma_map_sg_attrs() and
>>>> the
>>>> resulting sglist contains entry with >32bit PA. This is because
>>>> dma_map_sg_attrs() calls dma_direct_map_sg(), which in turn calls
>>>> dma_direct_map_sg(), then dma_direct_map_page() and that's where it
>>>> goes
>>>> weird.
>>>>
>>>> dma_direct_map_page() does a dma_direct_possible() check before
>>>> triggering swiotlb_map(). The check succeeds, so the later isn't
>>>> executed.
>>>>
>>>> dma_direct_possible() calls dma_capable() with dev->dma_mask =
>>>> DMA_BIT_MASK(64) and dev->dma_bus_mask = 0, so
>>>> min_not_zero(*dev->dma_mask, dev->bus_dma_mask) returns
>>>> DMA_BIT_MASK(64).
>>>>
>>>> Surely enough, if I hack dma_direct_possible() to return 0,
>>>> swiotlb_map() kicks in and the nvme driver starts working fine.
>>>>
>>>> I presume the question here is, why is dev->bus_dma_mask = 0 ?
>>>
>>> Because that's the default, and almost no code overrides that?
>>
>> But shouldn't drivers/of/device.c set that for the PCIe controller ?
> 
> Urgh, I really should have spotted the significance of "NVMe", but
> somehow it failed to click :(

Good thing it did now :-)

> Of course the existing code works fine for everything *except* PCI
> devices on DT-based systems... That's because of_dma_get_range() has
> never been made to work correctly with the trick we play of passing the
> host bridge of_node through of_dma_configure(). I've got at least 2 or 3
> half-finished attempts at improving that, but they keep getting
> sidetracked into trying to clean up the various new of_dma_configure()
> hacks I find in drivers and/or falling down the rabbit-hole of starting
> to redesign the whole dma_pfn_offset machinery entirely. Let me dig one
> up and try to constrain it to solve just this most common "one single
> limited range" condition for the sake of making actual progress...

That'd be nice, thank you. I'm happy to test it on various devices here.

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/19/19 12:25 AM, Marek Vasut wrote:
> On 3/18/19 2:14 PM, Robin Murphy wrote:
>> On 17/03/2019 23:36, Marek Vasut wrote:
>>> On 3/17/19 11:29 AM, Geert Uytterhoeven wrote:
>>>> Hi Marek,
>>>
>>> Hi,
>>>
>>>> On Sun, Mar 17, 2019 at 12:04 AM Marek Vasut <marek.vasut at gmail.com>
>>>> wrote:
>>>>> On 3/16/19 10:25 PM, Marek Vasut wrote:
>>>>>> On 3/13/19 7:30 PM, Christoph Hellwig wrote:
>>>>>>> On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
>>>>>>>> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
>>>>>>>>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
>>>>>>>>>>> Right, but whoever *interprets* the device masks after the
>>>>>>>>>>> driver has
>>>>>>>>>>> overridden them should be taking the (smaller) bus mask into
>>>>>>>>>>> account as
>>>>>>>>>>> well, so the question is where is *that* not being done
>>>>>>>>>>> correctly?
>>>>>>>>>>
>>>>>>>>>> Do you have a hint where I should look for that ?
>>>>>>>>>
>>>>>>>>> If this a 32-bit ARM platform it might the complete lack of support
>>>>>>>>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
>>>>>>>>
>>>>>>>> It's an ARM 64bit platform, just the PCIe controller is limited
>>>>>>>> to 32bit
>>>>>>>> address range, so the devices on the PCIe bus cannot read the host's
>>>>>>>> DRAM above the 32bit limit.
>>>>>>>
>>>>>>> arm64 should take the mask into account both for the swiotlb and
>>>>>>> iommu case.? What are the exact symptoms you see?
>>>>>>
>>>>>> With the nvme, the device is recognized, but cannot be used.
>>>>>> It boils down to PCI BAR access being possible, since that's all below
>>>>>> the 32bit boundary, but when the device tries to do any sort of DMA,
>>>>>> that transfer returns nonsense data.
>>>>>>
>>>>>> But when I call dma_set_mask_and_coherent(dev->dev,
>>>>>> DMA_BIT_MASK(32) in
>>>>>> the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
>>>>>> drivers), it all starts to work fine.
>>>>>>
>>>>>> Could it be that the driver overwrites the (coherent_)dma_mask and
>>>>>> that's why the swiotlb/iommu code cannot take this into account ?
>>>>>>
>>>>>>> Does it involve
>>>>>>> swiotlb not kicking in, or iommu issues?
>>>>>>
>>>>>> How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
>>>>>> drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.
>>>>>
>>>>> Digging further ...
>>>>>
>>>>> drivers/nvme/host/pci.c nvme_map_data() calls dma_map_sg_attrs() and
>>>>> the
>>>>> resulting sglist contains entry with >32bit PA. This is because
>>>>> dma_map_sg_attrs() calls dma_direct_map_sg(), which in turn calls
>>>>> dma_direct_map_sg(), then dma_direct_map_page() and that's where it
>>>>> goes
>>>>> weird.
>>>>>
>>>>> dma_direct_map_page() does a dma_direct_possible() check before
>>>>> triggering swiotlb_map(). The check succeeds, so the later isn't
>>>>> executed.
>>>>>
>>>>> dma_direct_possible() calls dma_capable() with dev->dma_mask =
>>>>> DMA_BIT_MASK(64) and dev->dma_bus_mask = 0, so
>>>>> min_not_zero(*dev->dma_mask, dev->bus_dma_mask) returns
>>>>> DMA_BIT_MASK(64).
>>>>>
>>>>> Surely enough, if I hack dma_direct_possible() to return 0,
>>>>> swiotlb_map() kicks in and the nvme driver starts working fine.
>>>>>
>>>>> I presume the question here is, why is dev->bus_dma_mask = 0 ?
>>>>
>>>> Because that's the default, and almost no code overrides that?
>>>
>>> But shouldn't drivers/of/device.c set that for the PCIe controller ?
>>
>> Urgh, I really should have spotted the significance of "NVMe", but
>> somehow it failed to click :(
> 
> Good thing it did now :-)
> 
>> Of course the existing code works fine for everything *except* PCI
>> devices on DT-based systems... That's because of_dma_get_range() has
>> never been made to work correctly with the trick we play of passing the
>> host bridge of_node through of_dma_configure(). I've got at least 2 or 3
>> half-finished attempts at improving that, but they keep getting
>> sidetracked into trying to clean up the various new of_dma_configure()
>> hacks I find in drivers and/or falling down the rabbit-hole of starting
>> to redesign the whole dma_pfn_offset machinery entirely. Let me dig one
>> up and try to constrain it to solve just this most common "one single
>> limited range" condition for the sake of making actual progress...
> 
> That'd be nice, thank you. I'm happy to test it on various devices here.

Just curious, no stress, did you get anywhere with this patch(set) yet?

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Marek Vasut]

On 3/28/19 4:25 AM, Marek Vasut wrote:
> On 3/19/19 12:25 AM, Marek Vasut wrote:
>> On 3/18/19 2:14 PM, Robin Murphy wrote:
>>> On 17/03/2019 23:36, Marek Vasut wrote:
>>>> On 3/17/19 11:29 AM, Geert Uytterhoeven wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On Sun, Mar 17, 2019 at 12:04 AM Marek Vasut <marek.vasut at gmail.com>
>>>>> wrote:
>>>>>> On 3/16/19 10:25 PM, Marek Vasut wrote:
>>>>>>> On 3/13/19 7:30 PM, Christoph Hellwig wrote:
>>>>>>>> On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
>>>>>>>>> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
>>>>>>>>>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
>>>>>>>>>>>> Right, but whoever *interprets* the device masks after the
>>>>>>>>>>>> driver has
>>>>>>>>>>>> overridden them should be taking the (smaller) bus mask into
>>>>>>>>>>>> account as
>>>>>>>>>>>> well, so the question is where is *that* not being done
>>>>>>>>>>>> correctly?
>>>>>>>>>>>
>>>>>>>>>>> Do you have a hint where I should look for that ?
>>>>>>>>>>
>>>>>>>>>> If this a 32-bit ARM platform it might the complete lack of support
>>>>>>>>>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
>>>>>>>>>
>>>>>>>>> It's an ARM 64bit platform, just the PCIe controller is limited
>>>>>>>>> to 32bit
>>>>>>>>> address range, so the devices on the PCIe bus cannot read the host's
>>>>>>>>> DRAM above the 32bit limit.
>>>>>>>>
>>>>>>>> arm64 should take the mask into account both for the swiotlb and
>>>>>>>> iommu case.? What are the exact symptoms you see?
>>>>>>>
>>>>>>> With the nvme, the device is recognized, but cannot be used.
>>>>>>> It boils down to PCI BAR access being possible, since that's all below
>>>>>>> the 32bit boundary, but when the device tries to do any sort of DMA,
>>>>>>> that transfer returns nonsense data.
>>>>>>>
>>>>>>> But when I call dma_set_mask_and_coherent(dev->dev,
>>>>>>> DMA_BIT_MASK(32) in
>>>>>>> the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
>>>>>>> drivers), it all starts to work fine.
>>>>>>>
>>>>>>> Could it be that the driver overwrites the (coherent_)dma_mask and
>>>>>>> that's why the swiotlb/iommu code cannot take this into account ?
>>>>>>>
>>>>>>>> Does it involve
>>>>>>>> swiotlb not kicking in, or iommu issues?
>>>>>>>
>>>>>>> How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
>>>>>>> drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.
>>>>>>
>>>>>> Digging further ...
>>>>>>
>>>>>> drivers/nvme/host/pci.c nvme_map_data() calls dma_map_sg_attrs() and
>>>>>> the
>>>>>> resulting sglist contains entry with >32bit PA. This is because
>>>>>> dma_map_sg_attrs() calls dma_direct_map_sg(), which in turn calls
>>>>>> dma_direct_map_sg(), then dma_direct_map_page() and that's where it
>>>>>> goes
>>>>>> weird.
>>>>>>
>>>>>> dma_direct_map_page() does a dma_direct_possible() check before
>>>>>> triggering swiotlb_map(). The check succeeds, so the later isn't
>>>>>> executed.
>>>>>>
>>>>>> dma_direct_possible() calls dma_capable() with dev->dma_mask =
>>>>>> DMA_BIT_MASK(64) and dev->dma_bus_mask = 0, so
>>>>>> min_not_zero(*dev->dma_mask, dev->bus_dma_mask) returns
>>>>>> DMA_BIT_MASK(64).
>>>>>>
>>>>>> Surely enough, if I hack dma_direct_possible() to return 0,
>>>>>> swiotlb_map() kicks in and the nvme driver starts working fine.
>>>>>>
>>>>>> I presume the question here is, why is dev->bus_dma_mask = 0 ?
>>>>>
>>>>> Because that's the default, and almost no code overrides that?
>>>>
>>>> But shouldn't drivers/of/device.c set that for the PCIe controller ?
>>>
>>> Urgh, I really should have spotted the significance of "NVMe", but
>>> somehow it failed to click :(
>>
>> Good thing it did now :-)
>>
>>> Of course the existing code works fine for everything *except* PCI
>>> devices on DT-based systems... That's because of_dma_get_range() has
>>> never been made to work correctly with the trick we play of passing the
>>> host bridge of_node through of_dma_configure(). I've got at least 2 or 3
>>> half-finished attempts at improving that, but they keep getting
>>> sidetracked into trying to clean up the various new of_dma_configure()
>>> hacks I find in drivers and/or falling down the rabbit-hole of starting
>>> to redesign the whole dma_pfn_offset machinery entirely. Let me dig one
>>> up and try to constrain it to solve just this most common "one single
>>> limited range" condition for the sake of making actual progress...
>>
>> That'd be nice, thank you. I'm happy to test it on various devices here.
> 
> Just curious, no stress, did you get anywhere with this patch(set) yet?

Bump ?

-- 
Best regards,
Marek Vasut

[PATCH 1/2] [RFC] ata: ahci: Respect bus DMA constraints

[Geert Uytterhoeven]

Hi Marek,

On Sat, Mar 16, 2019@10:26 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 3/13/19 7:30 PM, Christoph Hellwig wrote:
> > On Sat, Mar 09, 2019@12:23:15AM +0100, Marek Vasut wrote:
> >> On 3/8/19 8:18 AM, Christoph Hellwig wrote:
> >>> On Thu, Mar 07, 2019@12:14:06PM +0100, Marek Vasut wrote:
> >>>>> Right, but whoever *interprets* the device masks after the driver has
> >>>>> overridden them should be taking the (smaller) bus mask into account as
> >>>>> well, so the question is where is *that* not being done correctly?
> >>>>
> >>>> Do you have a hint where I should look for that ?
> >>>
> >>> If this a 32-bit ARM platform it might the complete lack of support
> >>> for bus_dma_mask in arch/arm/mm/dma-mapping.c..
> >>
> >> It's an ARM 64bit platform, just the PCIe controller is limited to 32bit
> >> address range, so the devices on the PCIe bus cannot read the host's
> >> DRAM above the 32bit limit.
> >
> > arm64 should take the mask into account both for the swiotlb and
> > iommu case.  What are the exact symptoms you see?
>
> With the nvme, the device is recognized, but cannot be used.
> It boils down to PCI BAR access being possible, since that's all below
> the 32bit boundary, but when the device tries to do any sort of DMA,
> that transfer returns nonsense data.
>
> But when I call dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(32) in
> the affected driver (thus far I tried this nvme, xhci-pci and ahci-pci
> drivers), it all starts to work fine.
>
> Could it be that the driver overwrites the (coherent_)dma_mask and
> that's why the swiotlb/iommu code cannot take this into account ?
>
> > Does it involve
> > swiotlb not kicking in, or iommu issues?
>
> How can I check ? I added printks into arch/arm64/mm/dma-mapping.c and
> drivers/iommu/dma-iommu.c , but I suspect I need to look elsewhere.

So far the IOMMU is disabled in upstream, as no devices are whitelisted
in drivers/iommu/ipmmu-vmsa.c.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds