Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1693001 > unrolled thread

[PATCH 0/4] ACPI: DMA ranges management

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] ACPI: DMA ranges management Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2017-07-20 16:50 +0200
    Re: [PATCH 0/4] ACPI: DMA ranges management Nate Watterson <nwatters@codeaurora.org> - 2017-07-26 16:50 +0200
      Re: [PATCH 0/4] ACPI: DMA ranges management Robin Murphy <robin.murphy@arm.com> - 2017-07-26 17:10 +0200
        Re: [PATCH 0/4] ACPI: DMA ranges management Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2017-07-26 17:40 +0200
          Re: [PATCH 0/4] ACPI: DMA ranges management Nate Watterson <nwatters@codeaurora.org> - 2017-07-26 18:50 +0200
          Re: [PATCH 0/4] ACPI: DMA ranges management Robin Murphy <robin.murphy@arm.com> - 2017-07-28 15:10 +0200
            Re: [PATCH 0/4] ACPI: DMA ranges management Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2017-07-28 16:10 +0200
              Re: [PATCH 0/4] ACPI: DMA ranges management Robin Murphy <robin.murphy@arm.com> - 2017-07-28 18:00 +0200
                Re: [PATCH 0/4] ACPI: DMA ranges management Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2017-07-31 11:00 +0200

#1693001 — [PATCH 0/4] ACPI: DMA ranges management

As reported in:

http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com

the bus connecting devices to an IOMMU bus can be smaller in size than
the IOMMU input address bits which results in devices DMA HW bugs in
particular related to IOVA allocation (ie chopping of higher address
bits owing to system bus HW capabilities mismatch with the IOMMU).

Fortunately this problem can be solved through an already present but never
used ACPI 6.2 firmware bindings (ie _DMA object) allowing to define the DMA
window for a specific bus in ACPI and therefore all upstream devices
connected to it.

This small patch series enables _DMA parsing in ACPI core code and
use it in ACPI IORT code in order to detect DMA ranges for devices and
update their data structures to make them work with their related DMA
addressing restrictions.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Feng Kan <fkan@apm.com>
Cc: Jon Masters <jcm@redhat.com>
Cc: Robert Moore <robert.moore@intel.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

Lorenzo Pieralisi (4):
  ACPI: Allow _DMA method in walk resources
  ACPI: Make acpi_dev_get_resources() method agnostic
  ACPI: Introduce DMA ranges parsing
  ACPI: Make acpi_dma_configure() DMA regions aware

 drivers/acpi/acpica/rsxface.c |  7 ++--
 drivers/acpi/arm64/iort.c     | 27 +++++++++++-
 drivers/acpi/resource.c       | 83 ++++++++++++++++++++++++++++---------
 drivers/acpi/scan.c           | 95 +++++++++++++++++++++++++++++++++++++++----
 include/acpi/acnames.h        |  1 +
 include/acpi/acpi_bus.h       |  2 +
 include/linux/acpi.h          |  8 ++++
 include/linux/acpi_iort.h     |  5 ++-
 8 files changed, 194 insertions(+), 34 deletions(-)

-- 
2.10.0

[toc] | [next] | [standalone]

#1697247

Hi Lorenzo,

On 7/20/2017 10:45 AM, Lorenzo Pieralisi wrote:
> As reported in:
> 
> http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com
> 
> the bus connecting devices to an IOMMU bus can be smaller in size than
> the IOMMU input address bits which results in devices DMA HW bugs in
> particular related to IOVA allocation (ie chopping of higher address
> bits owing to system bus HW capabilities mismatch with the IOMMU).
> 
> Fortunately this problem can be solved through an already present but never
> used ACPI 6.2 firmware bindings (ie _DMA object) allowing to define the DMA
> window for a specific bus in ACPI and therefore all upstream devices
> connected to it.
> 
> This small patch series enables _DMA parsing in ACPI core code and
> use it in ACPI IORT code in order to detect DMA ranges for devices and
> update their data structures to make them work with their related DMA
> addressing restrictions.

I tested the patches and unfortunately it seems like the DMA addressing
restrictions are not really enforced for devices that attempt to set
their own dma_mask based on controller capabilities. For instance,
consider the following from the ahci_platform driver:

	if (hpriv->cap & HOST_CAP_64) {
		rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
		[...]
	}

Prior to the check, I can see that the device dma_mask respects the
limits enumerated in the _DMA object, however it is then clobbered by
the call to dma_coerce_mask_and_coherent(). Interestingly, if
HOST_CAP_64 was not set and the _DMA object for the device (or its
parent) indicated support for > 32-bit addrs, the host controller
could end up getting programmed with addresses beyond what it actually
supports. That is more a bug with the ahci_platform driver assuming a
default 32-bit dma_mask, but I would not be surprised to find other
drivers that rely on the same assumption.

To ensure that dma_set_mask() and friends actually respect _DMA, would
you consider introducing a dma_supported() callback to check the input
dma_mask against the FW defined limits? This would end up aggressively
clipping the dma_mask to 32-bits for devices like the above if the _DMA
limit was less than 64-bits, but that is probably preferable to the
controller accessing unintended addresses.

Also, how would you feel about adding support for the IORT named_node
memory_address_limit field?

-Nate
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Feng Kan <fkan@apm.com>
> Cc: Jon Masters <jcm@redhat.com>
> Cc: Robert Moore <robert.moore@intel.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> 
> Lorenzo Pieralisi (4):
>    ACPI: Allow _DMA method in walk resources
>    ACPI: Make acpi_dev_get_resources() method agnostic
>    ACPI: Introduce DMA ranges parsing
>    ACPI: Make acpi_dma_configure() DMA regions aware
> 
>   drivers/acpi/acpica/rsxface.c |  7 ++--
>   drivers/acpi/arm64/iort.c     | 27 +++++++++++-
>   drivers/acpi/resource.c       | 83 ++++++++++++++++++++++++++++---------
>   drivers/acpi/scan.c           | 95 +++++++++++++++++++++++++++++++++++++++----
>   include/acpi/acnames.h        |  1 +
>   include/acpi/acpi_bus.h       |  2 +
>   include/linux/acpi.h          |  8 ++++
>   include/linux/acpi_iort.h     |  5 ++-
>   8 files changed, 194 insertions(+), 34 deletions(-)
> 

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1697269

Hi Nate,

On 26/07/17 15:46, Nate Watterson wrote:
> Hi Lorenzo,
> 
> On 7/20/2017 10:45 AM, Lorenzo Pieralisi wrote:
>> As reported in:
>>
>> http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com
>>
>>
>> the bus connecting devices to an IOMMU bus can be smaller in size than
>> the IOMMU input address bits which results in devices DMA HW bugs in
>> particular related to IOVA allocation (ie chopping of higher address
>> bits owing to system bus HW capabilities mismatch with the IOMMU).
>>
>> Fortunately this problem can be solved through an already present but
>> never
>> used ACPI 6.2 firmware bindings (ie _DMA object) allowing to define
>> the DMA
>> window for a specific bus in ACPI and therefore all upstream devices
>> connected to it.
>>
>> This small patch series enables _DMA parsing in ACPI core code and
>> use it in ACPI IORT code in order to detect DMA ranges for devices and
>> update their data structures to make them work with their related DMA
>> addressing restrictions.
> 
> I tested the patches and unfortunately it seems like the DMA addressing
> restrictions are not really enforced for devices that attempt to set
> their own dma_mask based on controller capabilities. For instance,
> consider the following from the ahci_platform driver:
> 
>     if (hpriv->cap & HOST_CAP_64) {
>         rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
>         [...]
>     }
> 
> Prior to the check, I can see that the device dma_mask respects the
> limits enumerated in the _DMA object, however it is then clobbered by
> the call to dma_coerce_mask_and_coherent(). Interestingly, if
> HOST_CAP_64 was not set and the _DMA object for the device (or its
> parent) indicated support for > 32-bit addrs, the host controller
> could end up getting programmed with addresses beyond what it actually
> supports. That is more a bug with the ahci_platform driver assuming a
> default 32-bit dma_mask, but I would not be surprised to find other
> drivers that rely on the same assumption.

Yup, you've hit upon the more general problem, which applies equally to
DT "dma-ranges" too. I'm working on arm64 DMA stuff at the moment, and
have the patch to actually enforce the firmware-described limit when
drivers update their masks, but that depends on everyone passing the
correct information to arch_setup_dma_ops() in the first place (I think
DT needs more fixing than ACPI does).

> To ensure that dma_set_mask() and friends actually respect _DMA, would
> you consider introducing a dma_supported() callback to check the input
> dma_mask against the FW defined limits? This would end up aggressively
> clipping the dma_mask to 32-bits for devices like the above if the _DMA
> limit was less than 64-bits, but that is probably preferable to the
> controller accessing unintended addresses.
> 
> Also, how would you feel about adding support for the IORT named_node
> memory_address_limit field?

We will certainly need that for some platform devices, so if you fancy
giving it a go before Lorenzo or I get there, feel free!

Robin.

> -Nate
>>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>> Cc: Feng Kan <fkan@apm.com>
>> Cc: Jon Masters <jcm@redhat.com>
>> Cc: Robert Moore <robert.moore@intel.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>
>> Lorenzo Pieralisi (4):
>>    ACPI: Allow _DMA method in walk resources
>>    ACPI: Make acpi_dev_get_resources() method agnostic
>>    ACPI: Introduce DMA ranges parsing
>>    ACPI: Make acpi_dma_configure() DMA regions aware
>>
>>   drivers/acpi/acpica/rsxface.c |  7 ++--
>>   drivers/acpi/arm64/iort.c     | 27 +++++++++++-
>>   drivers/acpi/resource.c       | 83
>> ++++++++++++++++++++++++++++---------
>>   drivers/acpi/scan.c           | 95
>> +++++++++++++++++++++++++++++++++++++++----
>>   include/acpi/acnames.h        |  1 +
>>   include/acpi/acpi_bus.h       |  2 +
>>   include/linux/acpi.h          |  8 ++++
>>   include/linux/acpi_iort.h     |  5 ++-
>>   8 files changed, 194 insertions(+), 34 deletions(-)
>>
> 

[toc] | [prev] | [next] | [standalone]

#1697313

On Wed, Jul 26, 2017 at 04:05:55PM +0100, Robin Murphy wrote:
> Hi Nate,
> 
> On 26/07/17 15:46, Nate Watterson wrote:
> > Hi Lorenzo,
> > 
> > On 7/20/2017 10:45 AM, Lorenzo Pieralisi wrote:
> >> As reported in:
> >>
> >> http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com
> >>
> >>
> >> the bus connecting devices to an IOMMU bus can be smaller in size than
> >> the IOMMU input address bits which results in devices DMA HW bugs in
> >> particular related to IOVA allocation (ie chopping of higher address
> >> bits owing to system bus HW capabilities mismatch with the IOMMU).
> >>
> >> Fortunately this problem can be solved through an already present but
> >> never
> >> used ACPI 6.2 firmware bindings (ie _DMA object) allowing to define
> >> the DMA
> >> window for a specific bus in ACPI and therefore all upstream devices
> >> connected to it.
> >>
> >> This small patch series enables _DMA parsing in ACPI core code and
> >> use it in ACPI IORT code in order to detect DMA ranges for devices and
> >> update their data structures to make them work with their related DMA
> >> addressing restrictions.
> > 
> > I tested the patches and unfortunately it seems like the DMA addressing
> > restrictions are not really enforced for devices that attempt to set
> > their own dma_mask based on controller capabilities. For instance,
> > consider the following from the ahci_platform driver:
> > 
> >     if (hpriv->cap & HOST_CAP_64) {
> >         rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
> >         [...]
> >     }
> > 
> > Prior to the check, I can see that the device dma_mask respects the
> > limits enumerated in the _DMA object, however it is then clobbered by
> > the call to dma_coerce_mask_and_coherent(). Interestingly, if
> > HOST_CAP_64 was not set and the _DMA object for the device (or its
> > parent) indicated support for > 32-bit addrs, the host controller
> > could end up getting programmed with addresses beyond what it actually
> > supports. That is more a bug with the ahci_platform driver assuming a
> > default 32-bit dma_mask, but I would not be surprised to find other
> > drivers that rely on the same assumption.
> 
> Yup, you've hit upon the more general problem, which applies equally to
> DT "dma-ranges" too. I'm working on arm64 DMA stuff at the moment, and
> have the patch to actually enforce the firmware-described limit when
> drivers update their masks, but that depends on everyone passing the
> correct information to arch_setup_dma_ops() in the first place (I think
> DT needs more fixing than ACPI does).
> 
> > To ensure that dma_set_mask() and friends actually respect _DMA, would
> > you consider introducing a dma_supported() callback to check the input
> > dma_mask against the FW defined limits? This would end up aggressively
> > clipping the dma_mask to 32-bits for devices like the above if the _DMA
> > limit was less than 64-bits, but that is probably preferable to the
> > controller accessing unintended addresses.
> > 
> > Also, how would you feel about adding support for the IORT named_node
> > memory_address_limit field?
> 
> We will certainly need that for some platform devices, so if you fancy
> giving it a go before Lorenzo or I get there, feel free!

I can do it for v2 but I would like to understand why using _DMA is
not good enough for named components - having two bindings describing
the same thing is not ideal and I'd rather avoid it - if there is
a reason I am happy to add the necessary code.

Thanks,
Lorenzo

> Robin.
> 
> > -Nate
> >>
> >> Cc: Will Deacon <will.deacon@arm.com>
> >> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> >> Cc: Feng Kan <fkan@apm.com>
> >> Cc: Jon Masters <jcm@redhat.com>
> >> Cc: Robert Moore <robert.moore@intel.com>
> >> Cc: Robin Murphy <robin.murphy@arm.com>
> >> Cc: Zhang Rui <rui.zhang@intel.com>
> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >>
> >> Lorenzo Pieralisi (4):
> >>    ACPI: Allow _DMA method in walk resources
> >>    ACPI: Make acpi_dev_get_resources() method agnostic
> >>    ACPI: Introduce DMA ranges parsing
> >>    ACPI: Make acpi_dma_configure() DMA regions aware
> >>
> >>   drivers/acpi/acpica/rsxface.c |  7 ++--
> >>   drivers/acpi/arm64/iort.c     | 27 +++++++++++-
> >>   drivers/acpi/resource.c       | 83
> >> ++++++++++++++++++++++++++++---------
> >>   drivers/acpi/scan.c           | 95
> >> +++++++++++++++++++++++++++++++++++++++----
> >>   include/acpi/acnames.h        |  1 +
> >>   include/acpi/acpi_bus.h       |  2 +
> >>   include/linux/acpi.h          |  8 ++++
> >>   include/linux/acpi_iort.h     |  5 ++-
> >>   8 files changed, 194 insertions(+), 34 deletions(-)
> >>
> > 
> 

[toc] | [prev] | [next] | [standalone]

#1697351

On 7/26/2017 11:35 AM, Lorenzo Pieralisi wrote:
> On Wed, Jul 26, 2017 at 04:05:55PM +0100, Robin Murphy wrote:
>> Hi Nate,
>>
>> On 26/07/17 15:46, Nate Watterson wrote:
>>> Hi Lorenzo,
>>>
>>> On 7/20/2017 10:45 AM, Lorenzo Pieralisi wrote:
>>>> As reported in:
>>>>
>>>> http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com
>>>>
>>>>
>>>> the bus connecting devices to an IOMMU bus can be smaller in size than
>>>> the IOMMU input address bits which results in devices DMA HW bugs in
>>>> particular related to IOVA allocation (ie chopping of higher address
>>>> bits owing to system bus HW capabilities mismatch with the IOMMU).
>>>>
>>>> Fortunately this problem can be solved through an already present but
>>>> never
>>>> used ACPI 6.2 firmware bindings (ie _DMA object) allowing to define
>>>> the DMA
>>>> window for a specific bus in ACPI and therefore all upstream devices
>>>> connected to it.
>>>>
>>>> This small patch series enables _DMA parsing in ACPI core code and
>>>> use it in ACPI IORT code in order to detect DMA ranges for devices and
>>>> update their data structures to make them work with their related DMA
>>>> addressing restrictions.
>>>
>>> I tested the patches and unfortunately it seems like the DMA addressing
>>> restrictions are not really enforced for devices that attempt to set
>>> their own dma_mask based on controller capabilities. For instance,
>>> consider the following from the ahci_platform driver:
>>>
>>>      if (hpriv->cap & HOST_CAP_64) {
>>>          rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
>>>          [...]
>>>      }
>>>
>>> Prior to the check, I can see that the device dma_mask respects the
>>> limits enumerated in the _DMA object, however it is then clobbered by
>>> the call to dma_coerce_mask_and_coherent(). Interestingly, if
>>> HOST_CAP_64 was not set and the _DMA object for the device (or its
>>> parent) indicated support for > 32-bit addrs, the host controller
>>> could end up getting programmed with addresses beyond what it actually
>>> supports. That is more a bug with the ahci_platform driver assuming a
>>> default 32-bit dma_mask, but I would not be surprised to find other
>>> drivers that rely on the same assumption.
>>
>> Yup, you've hit upon the more general problem, which applies equally to
>> DT "dma-ranges" too. I'm working on arm64 DMA stuff at the moment, and
>> have the patch to actually enforce the firmware-described limit when
>> drivers update their masks, but that depends on everyone passing the
>> correct information to arch_setup_dma_ops() in the first place (I think
>> DT needs more fixing than ACPI does).
>>
>>> To ensure that dma_set_mask() and friends actually respect _DMA, would
>>> you consider introducing a dma_supported() callback to check the input
>>> dma_mask against the FW defined limits? This would end up aggressively
>>> clipping the dma_mask to 32-bits for devices like the above if the _DMA
>>> limit was less than 64-bits, but that is probably preferable to the
>>> controller accessing unintended addresses.
>>>
>>> Also, how would you feel about adding support for the IORT named_node
>>> memory_address_limit field?
>>
>> We will certainly need that for some platform devices, so if you fancy
>> giving it a go before Lorenzo or I get there, feel free!
> 
> I can do it for v2 but I would like to understand why using _DMA is
> not good enough for named components - having two bindings describing
> the same thing is not ideal and I'd rather avoid it - if there is
> a reason I am happy to add the necessary code.

My primary reason for requesting this is that I had already configured
the memory_address_limit field for the address challenged platform
devices in our (QDF2400) IORT under the assumption it would eventually
be supported.

Tested-by: Nate Watterson <nwatters@codeaurora.org>
> 
> Thanks,
> Lorenzo
> 
>> Robin.
>>
>>> -Nate
>>>>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>>>> Cc: Feng Kan <fkan@apm.com>
>>>> Cc: Jon Masters <jcm@redhat.com>
>>>> Cc: Robert Moore <robert.moore@intel.com>
>>>> Cc: Robin Murphy <robin.murphy@arm.com>
>>>> Cc: Zhang Rui <rui.zhang@intel.com>
>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>>
>>>> Lorenzo Pieralisi (4):
>>>>     ACPI: Allow _DMA method in walk resources
>>>>     ACPI: Make acpi_dev_get_resources() method agnostic
>>>>     ACPI: Introduce DMA ranges parsing
>>>>     ACPI: Make acpi_dma_configure() DMA regions aware
>>>>
>>>>    drivers/acpi/acpica/rsxface.c |  7 ++--
>>>>    drivers/acpi/arm64/iort.c     | 27 +++++++++++-
>>>>    drivers/acpi/resource.c       | 83
>>>> ++++++++++++++++++++++++++++---------
>>>>    drivers/acpi/scan.c           | 95
>>>> +++++++++++++++++++++++++++++++++++++++----
>>>>    include/acpi/acnames.h        |  1 +
>>>>    include/acpi/acpi_bus.h       |  2 +
>>>>    include/linux/acpi.h          |  8 ++++
>>>>    include/linux/acpi_iort.h     |  5 ++-
>>>>    8 files changed, 194 insertions(+), 34 deletions(-)
>>>>
>>>
>>

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1698759

On 26/07/17 16:35, Lorenzo Pieralisi wrote:
> On Wed, Jul 26, 2017 at 04:05:55PM +0100, Robin Murphy wrote:
>> Hi Nate,
>>
>> On 26/07/17 15:46, Nate Watterson wrote:
>>> Hi Lorenzo,
>>>
>>> On 7/20/2017 10:45 AM, Lorenzo Pieralisi wrote:
>>>> As reported in:
>>>>
>>>> http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com
>>>>
>>>>
>>>> the bus connecting devices to an IOMMU bus can be smaller in size than
>>>> the IOMMU input address bits which results in devices DMA HW bugs in
>>>> particular related to IOVA allocation (ie chopping of higher address
>>>> bits owing to system bus HW capabilities mismatch with the IOMMU).
>>>>
>>>> Fortunately this problem can be solved through an already present but
>>>> never
>>>> used ACPI 6.2 firmware bindings (ie _DMA object) allowing to define
>>>> the DMA
>>>> window for a specific bus in ACPI and therefore all upstream devices
>>>> connected to it.
>>>>
>>>> This small patch series enables _DMA parsing in ACPI core code and
>>>> use it in ACPI IORT code in order to detect DMA ranges for devices and
>>>> update their data structures to make them work with their related DMA
>>>> addressing restrictions.
>>>
>>> I tested the patches and unfortunately it seems like the DMA addressing
>>> restrictions are not really enforced for devices that attempt to set
>>> their own dma_mask based on controller capabilities. For instance,
>>> consider the following from the ahci_platform driver:
>>>
>>>     if (hpriv->cap & HOST_CAP_64) {
>>>         rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
>>>         [...]
>>>     }
>>>
>>> Prior to the check, I can see that the device dma_mask respects the
>>> limits enumerated in the _DMA object, however it is then clobbered by
>>> the call to dma_coerce_mask_and_coherent(). Interestingly, if
>>> HOST_CAP_64 was not set and the _DMA object for the device (or its
>>> parent) indicated support for > 32-bit addrs, the host controller
>>> could end up getting programmed with addresses beyond what it actually
>>> supports. That is more a bug with the ahci_platform driver assuming a
>>> default 32-bit dma_mask, but I would not be surprised to find other
>>> drivers that rely on the same assumption.
>>
>> Yup, you've hit upon the more general problem, which applies equally to
>> DT "dma-ranges" too. I'm working on arm64 DMA stuff at the moment, and
>> have the patch to actually enforce the firmware-described limit when
>> drivers update their masks, but that depends on everyone passing the
>> correct information to arch_setup_dma_ops() in the first place (I think
>> DT needs more fixing than ACPI does).
>>
>>> To ensure that dma_set_mask() and friends actually respect _DMA, would
>>> you consider introducing a dma_supported() callback to check the input
>>> dma_mask against the FW defined limits? This would end up aggressively
>>> clipping the dma_mask to 32-bits for devices like the above if the _DMA
>>> limit was less than 64-bits, but that is probably preferable to the
>>> controller accessing unintended addresses.
>>>
>>> Also, how would you feel about adding support for the IORT named_node
>>> memory_address_limit field?
>>
>> We will certainly need that for some platform devices, so if you fancy
>> giving it a go before Lorenzo or I get there, feel free!
> 
> I can do it for v2 but I would like to understand why using _DMA is
> not good enough for named components - having two bindings describing
> the same thing is not ideal and I'd rather avoid it - if there is
> a reason I am happy to add the necessary code.

My interpretation of "_DMA is only defined under devices that represent
buses." (ACPI 6.0, section 6.2.4) is that "devices that represent buses"
are those that have other device objects as children. In other words
(excuse my novice pseudo-ASL), this would be valid:

Scope(_SB)
{
	Device (Bus)
	{
		...
		Method (_DMA ... )
		Device (Dev1)
		{
			...
		}
	}
}

but this should be invalid:

Scope(_SB)
{
	Device (Dev2)
	{
		...
		Method (_DMA ... )
	}
}

Thus in the case where Dev2 is wired directly to an SMMU input, but
fewer address bits are wired up between the two than both the device and
SMMU interfaces are capable of, memory address limit is enough to
describe that without having to insert a fake "bus" object above it just
to hold the _DMA method.

Robin.

[toc] | [prev] | [next] | [standalone]

#1698816

On Fri, Jul 28, 2017 at 02:08:01PM +0100, Robin Murphy wrote:

[...]

> >>> To ensure that dma_set_mask() and friends actually respect _DMA, would
> >>> you consider introducing a dma_supported() callback to check the input
> >>> dma_mask against the FW defined limits? This would end up aggressively
> >>> clipping the dma_mask to 32-bits for devices like the above if the _DMA
> >>> limit was less than 64-bits, but that is probably preferable to the
> >>> controller accessing unintended addresses.
> >>>
> >>> Also, how would you feel about adding support for the IORT named_node
> >>> memory_address_limit field?
> >>
> >> We will certainly need that for some platform devices, so if you fancy
> >> giving it a go before Lorenzo or I get there, feel free!
> > 
> > I can do it for v2 but I would like to understand why using _DMA is
> > not good enough for named components - having two bindings describing
> > the same thing is not ideal and I'd rather avoid it - if there is
> > a reason I am happy to add the necessary code.
> 
> My interpretation of "_DMA is only defined under devices that represent
> buses." (ACPI 6.0, section 6.2.4) is that "devices that represent buses"
> are those that have other device objects as children.

Well if that was the case we would not be able to use _DMA for
eg PNP0A03 PCI host bridges that have no child ACPI devices, which
defeats the whole purpose of what I am doing.

The question here is what the _DMA object binding exactly means when
it refers to a "bus" and that's something I will figure out (and possibly
change) ASAP.

> In other words (excuse my novice pseudo-ASL), this would be valid:
> 
> Scope(_SB)
> {
> 	Device (Bus)
> 	{
> 		...
> 		Method (_DMA ... )
> 		Device (Dev1)
> 		{
> 			...
> 		}
> 	}
> }
> 
> but this should be invalid:
> 
> Scope(_SB)
> {
> 	Device (Dev2)
> 	{
> 		...
> 		Method (_DMA ... )
> 	}
> }

Not sure about that (see above) and I agree that's what needs
clarification.

> Thus in the case where Dev2 is wired directly to an SMMU input, but
> fewer address bits are wired up between the two than both the device and
> SMMU interfaces are capable of, memory address limit is enough to
> describe that without having to insert a fake "bus" object above it just
> to hold the _DMA method.

BTW, how would you describe that in DT ? A "dma-ranges" property in the
device DT node right ? Arguably "dma-ranges" was not meant to be used
like that either ;-)

Long and short of it is: I do not like having two ways of describing
the same thing. I agree that the _DMA object usage requires
clarifications from a spec point of view but I want to do that before
plugging in code that may use bindings inconsistently.

I will flag this up at ACPI spec level as soon as possible and get this
sorted.

Thanks,
Lorenzo

[toc] | [prev] | [next] | [standalone]

#1698916

On 28/07/17 15:09, Lorenzo Pieralisi wrote:
> On Fri, Jul 28, 2017 at 02:08:01PM +0100, Robin Murphy wrote:
> 
> [...]
> 
>>>>> To ensure that dma_set_mask() and friends actually respect _DMA, would
>>>>> you consider introducing a dma_supported() callback to check the input
>>>>> dma_mask against the FW defined limits? This would end up aggressively
>>>>> clipping the dma_mask to 32-bits for devices like the above if the _DMA
>>>>> limit was less than 64-bits, but that is probably preferable to the
>>>>> controller accessing unintended addresses.
>>>>>
>>>>> Also, how would you feel about adding support for the IORT named_node
>>>>> memory_address_limit field?
>>>>
>>>> We will certainly need that for some platform devices, so if you fancy
>>>> giving it a go before Lorenzo or I get there, feel free!
>>>
>>> I can do it for v2 but I would like to understand why using _DMA is
>>> not good enough for named components - having two bindings describing
>>> the same thing is not ideal and I'd rather avoid it - if there is
>>> a reason I am happy to add the necessary code.
>>
>> My interpretation of "_DMA is only defined under devices that represent
>> buses." (ACPI 6.0, section 6.2.4) is that "devices that represent buses"
>> are those that have other device objects as children.
> 
> Well if that was the case we would not be able to use _DMA for
> eg PNP0A03 PCI host bridges that have no child ACPI devices, which
> defeats the whole purpose of what I am doing.
> 
> The question here is what the _DMA object binding exactly means when
> it refers to a "bus" and that's something I will figure out (and possibly
> change) ASAP.
> 
>> In other words (excuse my novice pseudo-ASL), this would be valid:
>>
>> Scope(_SB)
>> {
>> 	Device (Bus)
>> 	{
>> 		...
>> 		Method (_DMA ... )
>> 		Device (Dev1)
>> 		{
>> 			...
>> 		}
>> 	}
>> }
>>
>> but this should be invalid:
>>
>> Scope(_SB)
>> {
>> 	Device (Dev2)
>> 	{
>> 		...
>> 		Method (_DMA ... )
>> 	}
>> }
> 
> Not sure about that (see above) and I agree that's what needs
> clarification.
> 
>> Thus in the case where Dev2 is wired directly to an SMMU input, but
>> fewer address bits are wired up between the two than both the device and
>> SMMU interfaces are capable of, memory address limit is enough to
>> describe that without having to insert a fake "bus" object above it just
>> to hold the _DMA method.
> 
> BTW, how would you describe that in DT ? A "dma-ranges" property in the
> device DT node right ? Arguably "dma-ranges" was not meant to be used
> like that either ;-)

I believe that in real Open Firmware, the full PCI hierarchy is
described in the device tree - I had assumed that ACPI expected the
equivalent (i.e. the firmware probes PCI and assigns resources, so
bridges/endpoints/etc. would be represented in the namespace with
appropriate _CRS), thus the "bus with invisible children" case would
only need to apply to DT. In terms of DTspec, it does not say that
"dma-ranges" cannot be present on nodes without children, but that *is*
implied of #address-cells and #size cells, so there does exist a similar
ambiguity about what exactly counts as "a memory-mapped bus whose
devicetree parent can be accessed from DMA operations originating from
the bus". Certainly in the current Linux code, of_dma_configure()
*doesn't* parse "dma-ranges" on leaf nodes (which is an open problem for
some PCI host bridges in extant FDTs).

As for the case of straightforward interconnect widths/offsets (rather
than potentially arbitrary windows), the 'fake bus' notion is already
alive and well:

$ git grep 'soc {' arch/arm*/boot/dts

and the current "dma-ranges" users thankfully have consistent-enough
topologies that they don't need to get much crazier than that.

(side note: up at the other end, I'm not entirely convinced that what I
did for Juno is actually legal either)

> Long and short of it is: I do not like having two ways of describing
> the same thing. I agree that the _DMA object usage requires
> clarifications from a spec point of view but I want to do that before
> plugging in code that may use bindings inconsistently.

I'd still argue that they are describing different things, just that one
(the number of address bits wired up between a device and an SMMU)
happens to be possible to describe as a subset of the other (an
arbitrary mapping between two address spaces). The use-cases don't
entirely overlap either - the information in _DMA is also likely to be
wanted by non-ECAM PCI host controller drivers to configure their
inbound windows, irrespective of anything to do with IOMMUs, whereas
IORT code in hypervisors or other situations without a full ACPI
namespace available may need to make decisions that the device memory
address size limit is necessary for (well, that's the argument I've
heard anyway).
> I will flag this up at ACPI spec level as soon as possible and get this
> sorted.

Agreed.

Robin.

[toc] | [prev] | [next] | [standalone]

#1699833

On Fri, Jul 28, 2017 at 04:55:36PM +0100, Robin Murphy wrote:
> On 28/07/17 15:09, Lorenzo Pieralisi wrote:
> > On Fri, Jul 28, 2017 at 02:08:01PM +0100, Robin Murphy wrote:
> > 
> > [...]
> > 
> >>>>> To ensure that dma_set_mask() and friends actually respect _DMA, would
> >>>>> you consider introducing a dma_supported() callback to check the input
> >>>>> dma_mask against the FW defined limits? This would end up aggressively
> >>>>> clipping the dma_mask to 32-bits for devices like the above if the _DMA
> >>>>> limit was less than 64-bits, but that is probably preferable to the
> >>>>> controller accessing unintended addresses.
> >>>>>
> >>>>> Also, how would you feel about adding support for the IORT named_node
> >>>>> memory_address_limit field?
> >>>>
> >>>> We will certainly need that for some platform devices, so if you fancy
> >>>> giving it a go before Lorenzo or I get there, feel free!
> >>>
> >>> I can do it for v2 but I would like to understand why using _DMA is
> >>> not good enough for named components - having two bindings describing
> >>> the same thing is not ideal and I'd rather avoid it - if there is
> >>> a reason I am happy to add the necessary code.
> >>
> >> My interpretation of "_DMA is only defined under devices that represent
> >> buses." (ACPI 6.0, section 6.2.4) is that "devices that represent buses"
> >> are those that have other device objects as children.
> > 
> > Well if that was the case we would not be able to use _DMA for
> > eg PNP0A03 PCI host bridges that have no child ACPI devices, which
> > defeats the whole purpose of what I am doing.
> > 
> > The question here is what the _DMA object binding exactly means when
> > it refers to a "bus" and that's something I will figure out (and possibly
> > change) ASAP.
> > 
> >> In other words (excuse my novice pseudo-ASL), this would be valid:
> >>
> >> Scope(_SB)
> >> {
> >> 	Device (Bus)
> >> 	{
> >> 		...
> >> 		Method (_DMA ... )
> >> 		Device (Dev1)
> >> 		{
> >> 			...
> >> 		}
> >> 	}
> >> }
> >>
> >> but this should be invalid:
> >>
> >> Scope(_SB)
> >> {
> >> 	Device (Dev2)
> >> 	{
> >> 		...
> >> 		Method (_DMA ... )
> >> 	}
> >> }
> > 
> > Not sure about that (see above) and I agree that's what needs
> > clarification.
> > 
> >> Thus in the case where Dev2 is wired directly to an SMMU input, but
> >> fewer address bits are wired up between the two than both the device and
> >> SMMU interfaces are capable of, memory address limit is enough to
> >> describe that without having to insert a fake "bus" object above it just
> >> to hold the _DMA method.
> > 
> > BTW, how would you describe that in DT ? A "dma-ranges" property in the
> > device DT node right ? Arguably "dma-ranges" was not meant to be used
> > like that either ;-)
> 
> I believe that in real Open Firmware, the full PCI hierarchy is
> described in the device tree - I had assumed that ACPI expected the
> equivalent (i.e. the firmware probes PCI and assigns resources, so
> bridges/endpoints/etc. would be represented in the namespace with
> appropriate _CRS), thus the "bus with invisible children" case would
> only need to apply to DT. In terms of DTspec, it does not say that
> "dma-ranges" cannot be present on nodes without children, but that *is*
> implied of #address-cells and #size cells, so there does exist a similar
> ambiguity about what exactly counts as "a memory-mapped bus whose
> devicetree parent can be accessed from DMA operations originating from
> the bus". Certainly in the current Linux code, of_dma_configure()
> *doesn't* parse "dma-ranges" on leaf nodes (which is an open problem for
> some PCI host bridges in extant FDTs).
> 
> As for the case of straightforward interconnect widths/offsets (rather
> than potentially arbitrary windows), the 'fake bus' notion is already
> alive and well:
> 
> $ git grep 'soc {' arch/arm*/boot/dts
> 
> and the current "dma-ranges" users thankfully have consistent-enough
> topologies that they don't need to get much crazier than that.
> 
> (side note: up at the other end, I'm not entirely convinced that what I
> did for Juno is actually legal either)
> 
> > Long and short of it is: I do not like having two ways of describing
> > the same thing. I agree that the _DMA object usage requires
> > clarifications from a spec point of view but I want to do that before
> > plugging in code that may use bindings inconsistently.
> 
> I'd still argue that they are describing different things, just that one
> (the number of address bits wired up between a device and an SMMU)
> happens to be possible to describe as a subset of the other (an
> arbitrary mapping between two address spaces). The use-cases don't
> entirely overlap either - the information in _DMA is also likely to be
> wanted by non-ECAM PCI host controller drivers to configure their
> inbound windows, irrespective of anything to do with IOMMUs, whereas
> IORT code in hypervisors or other situations without a full ACPI
> namespace available may need to make decisions that the device memory
> address size limit is necessary for (well, that's the argument I've
> heard anyway).

Ok, I thought about it a bit more and I think the points you raised
should be tackled, certainly using _DMA objects on devices that
are not PCI host bridges (probably the only devices _DMA object was
devised to apply to) is moot, to say the least, whereas IORT address
limits are well defined for named components.

I will rework the code to use _DMA object (and its ranges) for PCI
devices, IORT named components address capabilities for anything else.

I will seek ACPI specs clarification anyway to make sure that the _DMA
usage is solid for PCI devices, if you have more comments just let me
know please.

Thanks,
Lorenzo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web