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 <[email protected]>
Cc: Hanjun Guo <[email protected]>
Cc: Feng Kan <[email protected]>
Cc: Jon Masters <[email protected]>
Cc: Robert Moore <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Zhang Rui <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
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
The function acpi_dev_get_resources() is completely generic and
can be used to parse resource objects that are not necessarily
coming from the _CRS method but also from other objects eg _DMA
that have the same _CRS resource format.
Create an acpi_dev_get_resources() helper, internal to the ACPI
resources parsing compilation unit, acpi_dev_get_resources_method(),
that takes a char* parameter to detect which ACPI method should be
called to retrieve the resources list and make acpi_dev_get_resources()
call it with a method name _CRS leaving the API behaviour unchanged.
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
drivers/acpi/resource.c | 54 +++++++++++++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index cd4c427..2b20a09 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -573,6 +573,36 @@ static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
return AE_OK;
}
+static
+int acpi_dev_get_resources_method(struct acpi_device *adev,
+ struct list_head *list,
+ int (*preproc)(struct acpi_resource *, void *),
+ void *preproc_data, char *method)
+{
+ struct res_proc_context c;
+ acpi_status status;
+
+ if (!adev || !adev->handle || !list_empty(list))
+ return -EINVAL;
+
+ if (!acpi_has_method(adev->handle, method))
+ return 0;
+
+ c.list = list;
+ c.preproc = preproc;
+ c.preproc_data = preproc_data;
+ c.count = 0;
+ c.error = 0;
+ status = acpi_walk_resources(adev->handle, method,
+ acpi_dev_process_resource, &c);
+ if (ACPI_FAILURE(status)) {
+ acpi_dev_free_resource_list(list);
+ return c.error ? c.error : -EIO;
+ }
+
+ return c.count;
+}
+
/**
* acpi_dev_get_resources - Get current resources of a device.
* @adev: ACPI device node to get the resources for.
@@ -601,28 +631,8 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
int (*preproc)(struct acpi_resource *, void *),
void *preproc_data)
{
- struct res_proc_context c;
- acpi_status status;
-
- if (!adev || !adev->handle || !list_empty(list))
- return -EINVAL;
-
- if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
- return 0;
-
- c.list = list;
- c.preproc = preproc;
- c.preproc_data = preproc_data;
- c.count = 0;
- c.error = 0;
- status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
- acpi_dev_process_resource, &c);
- if (ACPI_FAILURE(status)) {
- acpi_dev_free_resource_list(list);
- return c.error ? c.error : -EIO;
- }
-
- return c.count;
+ return acpi_dev_get_resources_method(adev, list, preproc,
+ preproc_data, METHOD_NAME__CRS);
}
EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
--
2.10.0
Some devices have limited addressing capabilities and cannot
reference the whole memory address space while carrying out DMA
operations (eg some devices with bus address bits range smaller than
system bus - which prevents them from using bus addresses that are
otherwise valid for the system).
The ACPI _DMA object allows bus devices to define the DMA window that is
actually addressable by devices that sit upstream the bus, therefore
providing a means to parse and initialize the devices DMA masks and
addressable DMA range size.
By relying on the generic ACPI kernel layer to retrieve and parse
resources, introduce ACPI core code to parse the _DMA object.
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
drivers/acpi/resource.c | 35 +++++++++++++++++++++
drivers/acpi/scan.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 2 ++
include/linux/acpi.h | 8 +++++
4 files changed, 128 insertions(+)
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 2b20a09..9602248 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -636,6 +636,41 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
}
EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
+static int is_memory(struct acpi_resource *ares, void *not_used)
+{
+ struct resource_win win;
+ struct resource *res = &win.res;
+
+ memset(&win, 0, sizeof(win));
+
+ return !(acpi_dev_resource_memory(ares, res)
+ || acpi_dev_resource_address_space(ares, &win)
+ || acpi_dev_resource_ext_address_space(ares, &win));
+}
+
+/**
+ * acpi_dev_get_dma_resources - Get current DMA resources of a device.
+ * @adev: ACPI device node to get the resources for.
+ * @list: Head of the resultant list of resources (must be empty).
+ *
+ * Evaluate the _DMA method for the given device node and process its
+ * output.
+ *
+ * The resultant struct resource objects are put on the list pointed to
+ * by @list, that must be empty initially, as members of struct
+ * resource_entry objects. Callers of this routine should use
+ * %acpi_dev_free_resource_list() to free that list.
+ *
+ * The number of resources in the output list is returned on success,
+ * an error code reflecting the error condition is returned otherwise.
+ */
+int acpi_dev_get_dma_resources(struct acpi_device *adev, struct list_head *list)
+{
+ return acpi_dev_get_resources_method(adev, list, is_memory, NULL,
+ METHOD_NAME__DMA);
+}
+EXPORT_SYMBOL_GPL(acpi_dev_get_dma_resources);
+
/**
* acpi_dev_filter_resource_type - Filter ACPI resource according to resource
* types
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 3389729..eb493c2 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1360,6 +1360,89 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
}
/**
+ * acpi_dma_get_range() - Get device DMA parameters.
+ *
+ * @dev: device to configure
+ * @dma_addr: pointer device DMA address result
+ * @offset: pointer to the DMA offset result
+ * @size: pointer to DMA range size result
+ *
+ * Evaluate DMA regions and return respectively DMA region start, offset
+ * and size in dma_addr, offset and size on parsing success; it does not
+ * update the passed in values on failure.
+ *
+ * Return 0 on success, < 0 on failure.
+ */
+int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
+ u64 *size)
+{
+ struct acpi_device *adev;
+ LIST_HEAD(list);
+ struct resource_entry *rentry;
+ int ret;
+ struct device *dma_dev = dev;
+ struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
+
+ /*
+ * Walk the device tree chasing an ACPI companion with a _DMA
+ * object while we go. Stop if we find a device with an ACPI
+ * companion containing a _DMA method.
+ */
+ do {
+ if (has_acpi_companion(dma_dev)) {
+ adev = ACPI_COMPANION(dma_dev);
+
+ if (acpi_has_method(adev->handle, METHOD_NAME__DMA))
+ break;
+ }
+ } while ((dma_dev = dma_dev->parent));
+
+ if (!dma_dev)
+ return -ENODEV;
+
+ if (!acpi_has_method(adev->handle, METHOD_NAME__CRS)) {
+ acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, &name_buffer);
+ pr_warn(FW_BUG "%s: _DMA object only valid in object with valid _CRS\n",
+ (char *)name_buffer.pointer);
+ kfree(name_buffer.pointer);
+ return -EINVAL;
+ }
+
+ ret = acpi_dev_get_dma_resources(adev, &list);
+ if (ret > 0) {
+ list_for_each_entry(rentry, &list, node) {
+ if (dma_offset && rentry->offset != dma_offset) {
+ ret = -EINVAL;
+ pr_warn("Can't handle multiple windows with different offsets\n");
+ goto out;
+ }
+ dma_offset = rentry->offset;
+
+ /* Take lower and upper limits */
+ if (rentry->res->start < dma_start)
+ dma_start = rentry->res->start;
+ if (rentry->res->end > dma_end)
+ dma_end = rentry->res->end;
+ }
+
+ if (dma_start >= dma_end) {
+ ret = -EINVAL;
+ pr_warn("Invalid DMA regions configuration\n");
+ goto out;
+ }
+
+ *dma_addr = dma_start - dma_offset;
+ *size = dma_end - dma_start + 1;
+ *offset = dma_offset;
+ }
+ out:
+ acpi_dev_free_resource_list(&list);
+
+ return ret >= 0 ? 0 : ret;
+}
+
+/**
* acpi_dma_configure - Set-up DMA configuration for the device.
* @dev: The pointer to the device
* @attr: device dma attributes
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 68bc6be..07eb963 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -578,6 +578,8 @@ struct acpi_pci_root {
bool acpi_dma_supported(struct acpi_device *adev);
enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
+int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
+ u64 *size);
int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr);
void acpi_dma_deconfigure(struct device *dev);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c749eef..a5eaff9 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -427,6 +427,8 @@ void acpi_dev_free_resource_list(struct list_head *list);
int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
int (*preproc)(struct acpi_resource *, void *),
void *preproc_data);
+int acpi_dev_get_dma_resources(struct acpi_device *adev,
+ struct list_head *list);
int acpi_dev_filter_resource_type(struct acpi_resource *ares,
unsigned long types);
@@ -774,6 +776,12 @@ static inline enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
return DEV_DMA_NOT_SUPPORTED;
}
+static inline int acpi_dma_get_range(struct device *dev, u64 *dma_addr,
+ u64 *offset, u64 *size)
+{
+ return -ENODEV;
+}
+
static inline int acpi_dma_configure(struct device *dev,
enum dev_dma_attr attr)
{
--
2.10.0
Current ACPI DMA configuration set-up device DMA capabilities through
kernel defaults that do not take into account platform specific DMA
configurations reported by firmware.
By leveraging the ACPI acpi_dev_get_dma_resources() API, add code
in acpi_dma_configure() to retrieve the DMA regions to correctly
set-up devices DMA parameters.
Rework the ACPI IORT kernel API to make sure they can accommodate
the DMA set-up required by firmware. By making devices DMA set-up
ACPI IORT specific, the kernel is shielded from unwanted regressions
that could be triggered by parsing DMA resources on arches that were
previously ignoring them (ie x86/ia64), leaving kernel behaviour
unchanged on those arches.
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Hanjun Guo <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
drivers/acpi/arm64/iort.c | 27 +++++++++++++++++++++++++--
drivers/acpi/scan.c | 12 ++++--------
include/linux/acpi_iort.h | 5 +++--
3 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index a3215ee..9a7a65b 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -681,12 +681,17 @@ static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
}
/**
- * iort_set_dma_mask - Set-up dma mask for a device.
+ * iort_dma_setup() - Set-up device DMA parameters.
*
* @dev: device to configure
+ * @dma_addr: device DMA address result pointer
+ * @size: DMA range size result pointer
*/
-void iort_set_dma_mask(struct device *dev)
+void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
{
+ u64 mask, dmaaddr = 0, size = 0, offset = 0;
+ int ret;
+
/*
* Set default coherent_dma_mask to 32 bit. Drivers are expected to
* setup the correct supported mask.
@@ -700,6 +705,24 @@ void iort_set_dma_mask(struct device *dev)
*/
if (!dev->dma_mask)
dev->dma_mask = &dev->coherent_dma_mask;
+
+ ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size);
+ if (ret < 0)
+ size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
+
+ mask = __roundup_pow_of_two(dmaaddr + size) - 1;
+ /*
+ * Limit coherent and dma mask based on size and default mask
+ * set by the driver.
+ */
+ dev->coherent_dma_mask = mask;
+ *dev->dma_mask = mask;
+
+ *dma_addr = dmaaddr;
+ *dma_size = size;
+
+ dev->dma_pfn_offset = PFN_DOWN(offset);
+ dev_dbg(dev, "dma_pfn_offset(%#08llx)\n", offset);
}
/**
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index eb493c2..15d0059 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1450,20 +1450,16 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
{
const struct iommu_ops *iommu;
- u64 size;
+ u64 dma_addr = 0, size = 0;
- iort_set_dma_mask(dev);
+ iort_dma_setup(dev, &dma_addr, &size);
iommu = iort_iommu_configure(dev);
if (IS_ERR(iommu) && PTR_ERR(iommu) == -EPROBE_DEFER)
return -EPROBE_DEFER;
- size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
- /*
- * Assume dma valid range starts at 0 and covers the whole
- * coherent_dma_mask.
- */
- arch_setup_dma_ops(dev, 0, size, iommu, attr == DEV_DMA_COHERENT);
+ arch_setup_dma_ops(dev, dma_addr, size,
+ iommu, attr == DEV_DMA_COHERENT);
return 0;
}
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 8379d40..8d3f0bf 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -36,7 +36,7 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
void acpi_configure_pmsi_domain(struct device *dev);
int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
/* IOMMU interface */
-void iort_set_dma_mask(struct device *dev);
+void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *size);
const struct iommu_ops *iort_iommu_configure(struct device *dev);
#else
static inline void acpi_iort_init(void) { }
@@ -47,7 +47,8 @@ static inline struct irq_domain *iort_get_device_domain(struct device *dev,
{ return NULL; }
static inline void acpi_configure_pmsi_domain(struct device *dev) { }
/* IOMMU interface */
-static inline void iort_set_dma_mask(struct device *dev) { }
+static inline void iort_dma_setup(struct device *dev, u64 *dma_addr,
+ u64 *size) { }
static inline
const struct iommu_ops *iort_iommu_configure(struct device *dev)
{ return NULL; }
--
2.10.0
The _DMA object contains a resource template, this change adds support
for the walk resources function so that ACPI devices containing a _DMA
object can actually parse it to detect DMA ranges for the respective
bus.
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Robert Moore <[email protected]>
Cc: Zhang Rui <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
drivers/acpi/acpica/rsxface.c | 7 ++++---
include/acpi/acnames.h | 1 +
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/acpica/rsxface.c b/drivers/acpi/acpica/rsxface.c
index 59a4f9e..be65e65 100644
--- a/drivers/acpi/acpica/rsxface.c
+++ b/drivers/acpi/acpica/rsxface.c
@@ -615,7 +615,7 @@ ACPI_EXPORT_SYMBOL(acpi_walk_resource_buffer)
* device we are querying
* name - Method name of the resources we want.
* (METHOD_NAME__CRS, METHOD_NAME__PRS, or
- * METHOD_NAME__AEI)
+ * METHOD_NAME__AEI or METHOD_NAME__DMA)
* user_function - Called for each resource
* context - Passed to user_function
*
@@ -641,11 +641,12 @@ acpi_walk_resources(acpi_handle device_handle,
if (!device_handle || !user_function || !name ||
(!ACPI_COMPARE_NAME(name, METHOD_NAME__CRS) &&
!ACPI_COMPARE_NAME(name, METHOD_NAME__PRS) &&
- !ACPI_COMPARE_NAME(name, METHOD_NAME__AEI))) {
+ !ACPI_COMPARE_NAME(name, METHOD_NAME__AEI) &&
+ !ACPI_COMPARE_NAME(name, METHOD_NAME__DMA))) {
return_ACPI_STATUS(AE_BAD_PARAMETER);
}
- /* Get the _CRS/_PRS/_AEI resource list */
+ /* Get the _CRS/_PRS/_AEI/_DMA resource list */
buffer.length = ACPI_ALLOCATE_LOCAL_BUFFER;
status = acpi_rs_get_method_data(device_handle, name, &buffer);
diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h
index b421584..d8dd3bf 100644
--- a/include/acpi/acnames.h
+++ b/include/acpi/acnames.h
@@ -54,6 +54,7 @@
#define METHOD_NAME__CLS "_CLS"
#define METHOD_NAME__CRS "_CRS"
#define METHOD_NAME__DDN "_DDN"
+#define METHOD_NAME__DMA "_DMA"
#define METHOD_NAME__HID "_HID"
#define METHOD_NAME__INI "_INI"
#define METHOD_NAME__PLD "_PLD"
--
2.10.0
I think we can take this as-is into ACPICA.
Bob
> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:[email protected]]
> Sent: Thursday, July 20, 2017 7:45 AM
> To: [email protected]
> Cc: [email protected]; [email protected];
> Lorenzo Pieralisi <[email protected]>; Moore, Robert
> <[email protected]>; Zhang, Rui <[email protected]>; Rafael J.
> Wysocki <[email protected]>; Will Deacon <[email protected]>; Robin
> Murphy <[email protected]>; Hanjun Guo <[email protected]>; Feng
> Kan <[email protected]>; Jon Masters <[email protected]>; Nate Watterson
> <[email protected]>
> Subject: [PATCH 1/4] ACPI: Allow _DMA method in walk resources
>
> The _DMA object contains a resource template, this change adds support
> for the walk resources function so that ACPI devices containing a _DMA
> object can actually parse it to detect DMA ranges for the respective
> bus.
>
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Cc: Robert Moore <[email protected]>
> Cc: Zhang Rui <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> drivers/acpi/acpica/rsxface.c | 7 ++++---
> include/acpi/acnames.h | 1 +
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/acpica/rsxface.c
> b/drivers/acpi/acpica/rsxface.c index 59a4f9e..be65e65 100644
> --- a/drivers/acpi/acpica/rsxface.c
> +++ b/drivers/acpi/acpica/rsxface.c
> @@ -615,7 +615,7 @@ ACPI_EXPORT_SYMBOL(acpi_walk_resource_buffer)
> * device we are querying
> * name - Method name of the resources we want.
> * (METHOD_NAME__CRS, METHOD_NAME__PRS,
> or
> - * METHOD_NAME__AEI)
> + * METHOD_NAME__AEI or METHOD_NAME__DMA)
> * user_function - Called for each resource
> * context - Passed to user_function
> *
> @@ -641,11 +641,12 @@ acpi_walk_resources(acpi_handle device_handle,
> if (!device_handle || !user_function || !name ||
> (!ACPI_COMPARE_NAME(name, METHOD_NAME__CRS) &&
> !ACPI_COMPARE_NAME(name, METHOD_NAME__PRS) &&
> - !ACPI_COMPARE_NAME(name, METHOD_NAME__AEI))) {
> + !ACPI_COMPARE_NAME(name, METHOD_NAME__AEI) &&
> + !ACPI_COMPARE_NAME(name, METHOD_NAME__DMA))) {
> return_ACPI_STATUS(AE_BAD_PARAMETER);
> }
>
> - /* Get the _CRS/_PRS/_AEI resource list */
> + /* Get the _CRS/_PRS/_AEI/_DMA resource list */
>
> buffer.length = ACPI_ALLOCATE_LOCAL_BUFFER;
> status = acpi_rs_get_method_data(device_handle, name, &buffer);
> diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h index
> b421584..d8dd3bf 100644
> --- a/include/acpi/acnames.h
> +++ b/include/acpi/acnames.h
> @@ -54,6 +54,7 @@
> #define METHOD_NAME__CLS "_CLS"
> #define METHOD_NAME__CRS "_CRS"
> #define METHOD_NAME__DDN "_DDN"
> +#define METHOD_NAME__DMA "_DMA"
> #define METHOD_NAME__HID "_HID"
> #define METHOD_NAME__INI "_INI"
> #define METHOD_NAME__PLD "_PLD"
> --
> 2.10.0
Could you post this as a pull request on our github?
https://github.com/acpica/acpica
> -----Original Message-----
> From: Moore, Robert
> Sent: Thursday, July 20, 2017 8:49 AM
> To: 'Lorenzo Pieralisi' <[email protected]>; linux-
> [email protected]
> Cc: [email protected]; [email protected];
> Zhang, Rui <[email protected]>; Rafael J. Wysocki <[email protected]>;
> Will Deacon <[email protected]>; Robin Murphy <[email protected]>;
> Hanjun Guo <[email protected]>; Feng Kan <[email protected]>; Jon Masters
> <[email protected]>; Nate Watterson <[email protected]>
> Subject: RE: [PATCH 1/4] ACPI: Allow _DMA method in walk resources
>
> I think we can take this as-is into ACPICA.
> Bob
>
>
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:[email protected]]
> > Sent: Thursday, July 20, 2017 7:45 AM
> > To: [email protected]
> > Cc: [email protected];
> > [email protected]; Lorenzo Pieralisi
> > <[email protected]>; Moore, Robert <[email protected]>;
> Zhang, Rui <[email protected]>; Rafael J.
> > Wysocki <[email protected]>; Will Deacon <[email protected]>; Robin
> > Murphy <[email protected]>; Hanjun Guo <[email protected]>;
> > Feng Kan <[email protected]>; Jon Masters <[email protected]>; Nate Watterson
> > <[email protected]>
> > Subject: [PATCH 1/4] ACPI: Allow _DMA method in walk resources
> >
> > The _DMA object contains a resource template, this change adds support
> > for the walk resources function so that ACPI devices containing a _DMA
> > object can actually parse it to detect DMA ranges for the respective
> > bus.
> >
> > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > Cc: Robert Moore <[email protected]>
> > Cc: Zhang Rui <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > ---
> > drivers/acpi/acpica/rsxface.c | 7 ++++---
> > include/acpi/acnames.h | 1 +
> > 2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/rsxface.c
> > b/drivers/acpi/acpica/rsxface.c index 59a4f9e..be65e65 100644
> > --- a/drivers/acpi/acpica/rsxface.c
> > +++ b/drivers/acpi/acpica/rsxface.c
> > @@ -615,7 +615,7 @@ ACPI_EXPORT_SYMBOL(acpi_walk_resource_buffer)
> > * device we are querying
> > * name - Method name of the resources we
> want.
> > * (METHOD_NAME__CRS,
> METHOD_NAME__PRS,
> > or
> > - * METHOD_NAME__AEI)
> > + * METHOD_NAME__AEI or
> METHOD_NAME__DMA)
> > * user_function - Called for each resource
> > * context - Passed to user_function
> > *
> > @@ -641,11 +641,12 @@ acpi_walk_resources(acpi_handle device_handle,
> > if (!device_handle || !user_function || !name ||
> > (!ACPI_COMPARE_NAME(name, METHOD_NAME__CRS) &&
> > !ACPI_COMPARE_NAME(name, METHOD_NAME__PRS) &&
> > - !ACPI_COMPARE_NAME(name, METHOD_NAME__AEI))) {
> > + !ACPI_COMPARE_NAME(name, METHOD_NAME__AEI) &&
> > + !ACPI_COMPARE_NAME(name, METHOD_NAME__DMA))) {
> > return_ACPI_STATUS(AE_BAD_PARAMETER);
> > }
> >
> > - /* Get the _CRS/_PRS/_AEI resource list */
> > + /* Get the _CRS/_PRS/_AEI/_DMA resource list */
> >
> > buffer.length = ACPI_ALLOCATE_LOCAL_BUFFER;
> > status = acpi_rs_get_method_data(device_handle, name, &buffer);
> diff
> > --git a/include/acpi/acnames.h b/include/acpi/acnames.h index
> > b421584..d8dd3bf 100644
> > --- a/include/acpi/acnames.h
> > +++ b/include/acpi/acnames.h
> > @@ -54,6 +54,7 @@
> > #define METHOD_NAME__CLS "_CLS"
> > #define METHOD_NAME__CRS "_CRS"
> > #define METHOD_NAME__DDN "_DDN"
> > +#define METHOD_NAME__DMA "_DMA"
> > #define METHOD_NAME__HID "_HID"
> > #define METHOD_NAME__INI "_INI"
> > #define METHOD_NAME__PLD "_PLD"
> > --
> > 2.10.0
On Thu, Jul 20, 2017 at 03:50:16PM +0000, Moore, Robert wrote:
> Could you post this as a pull request on our github?
>
> https://github.com/acpica/acpica
Sure, will do, thanks !
Lorenzo
> > -----Original Message-----
> > From: Moore, Robert
> > Sent: Thursday, July 20, 2017 8:49 AM
> > To: 'Lorenzo Pieralisi' <[email protected]>; linux-
> > [email protected]
> > Cc: [email protected]; [email protected];
> > Zhang, Rui <[email protected]>; Rafael J. Wysocki <[email protected]>;
> > Will Deacon <[email protected]>; Robin Murphy <[email protected]>;
> > Hanjun Guo <[email protected]>; Feng Kan <[email protected]>; Jon Masters
> > <[email protected]>; Nate Watterson <[email protected]>
> > Subject: RE: [PATCH 1/4] ACPI: Allow _DMA method in walk resources
> >
> > I think we can take this as-is into ACPICA.
> > Bob
> >
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:[email protected]]
> > > Sent: Thursday, July 20, 2017 7:45 AM
> > > To: [email protected]
> > > Cc: [email protected];
> > > [email protected]; Lorenzo Pieralisi
> > > <[email protected]>; Moore, Robert <[email protected]>;
> > Zhang, Rui <[email protected]>; Rafael J.
> > > Wysocki <[email protected]>; Will Deacon <[email protected]>; Robin
> > > Murphy <[email protected]>; Hanjun Guo <[email protected]>;
> > > Feng Kan <[email protected]>; Jon Masters <[email protected]>; Nate Watterson
> > > <[email protected]>
> > > Subject: [PATCH 1/4] ACPI: Allow _DMA method in walk resources
> > >
> > > The _DMA object contains a resource template, this change adds support
> > > for the walk resources function so that ACPI devices containing a _DMA
> > > object can actually parse it to detect DMA ranges for the respective
> > > bus.
> > >
> > > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > > Cc: Robert Moore <[email protected]>
> > > Cc: Zhang Rui <[email protected]>
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > ---
> > > drivers/acpi/acpica/rsxface.c | 7 ++++---
> > > include/acpi/acnames.h | 1 +
> > > 2 files changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpica/rsxface.c
> > > b/drivers/acpi/acpica/rsxface.c index 59a4f9e..be65e65 100644
> > > --- a/drivers/acpi/acpica/rsxface.c
> > > +++ b/drivers/acpi/acpica/rsxface.c
> > > @@ -615,7 +615,7 @@ ACPI_EXPORT_SYMBOL(acpi_walk_resource_buffer)
> > > * device we are querying
> > > * name - Method name of the resources we
> > want.
> > > * (METHOD_NAME__CRS,
> > METHOD_NAME__PRS,
> > > or
> > > - * METHOD_NAME__AEI)
> > > + * METHOD_NAME__AEI or
> > METHOD_NAME__DMA)
> > > * user_function - Called for each resource
> > > * context - Passed to user_function
> > > *
> > > @@ -641,11 +641,12 @@ acpi_walk_resources(acpi_handle device_handle,
> > > if (!device_handle || !user_function || !name ||
> > > (!ACPI_COMPARE_NAME(name, METHOD_NAME__CRS) &&
> > > !ACPI_COMPARE_NAME(name, METHOD_NAME__PRS) &&
> > > - !ACPI_COMPARE_NAME(name, METHOD_NAME__AEI))) {
> > > + !ACPI_COMPARE_NAME(name, METHOD_NAME__AEI) &&
> > > + !ACPI_COMPARE_NAME(name, METHOD_NAME__DMA))) {
> > > return_ACPI_STATUS(AE_BAD_PARAMETER);
> > > }
> > >
> > > - /* Get the _CRS/_PRS/_AEI resource list */
> > > + /* Get the _CRS/_PRS/_AEI/_DMA resource list */
> > >
> > > buffer.length = ACPI_ALLOCATE_LOCAL_BUFFER;
> > > status = acpi_rs_get_method_data(device_handle, name, &buffer);
> > diff
> > > --git a/include/acpi/acnames.h b/include/acpi/acnames.h index
> > > b421584..d8dd3bf 100644
> > > --- a/include/acpi/acnames.h
> > > +++ b/include/acpi/acnames.h
> > > @@ -54,6 +54,7 @@
> > > #define METHOD_NAME__CLS "_CLS"
> > > #define METHOD_NAME__CRS "_CRS"
> > > #define METHOD_NAME__DDN "_DDN"
> > > +#define METHOD_NAME__DMA "_DMA"
> > > #define METHOD_NAME__HID "_HID"
> > > #define METHOD_NAME__INI "_INI"
> > > #define METHOD_NAME__PLD "_PLD"
> > > --
> > > 2.10.0
>
On Thursday, July 20, 2017 03:45:14 PM Lorenzo Pieralisi wrote:
> The function acpi_dev_get_resources() is completely generic and
> can be used to parse resource objects that are not necessarily
> coming from the _CRS method but also from other objects eg _DMA
> that have the same _CRS resource format.
>
> Create an acpi_dev_get_resources() helper, internal to the ACPI
> resources parsing compilation unit, acpi_dev_get_resources_method(),
> that takes a char* parameter to detect which ACPI method should be
> called to retrieve the resources list and make acpi_dev_get_resources()
> call it with a method name _CRS leaving the API behaviour unchanged.
>
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> drivers/acpi/resource.c | 54 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index cd4c427..2b20a09 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -573,6 +573,36 @@ static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
> return AE_OK;
> }
>
> +static
> +int acpi_dev_get_resources_method(struct acpi_device *adev,
Do not break lines like this, please.
It should be
static int acpi_dev_get...
Also I would call it differently, maybe simply __acpi_dev_get_resources()?
> + struct list_head *list,
> + int (*preproc)(struct acpi_resource *, void *),
> + void *preproc_data, char *method)
const char *method ?
Thanks,
Rafael
On Thursday, July 20, 2017 03:45:15 PM Lorenzo Pieralisi wrote:
> Some devices have limited addressing capabilities and cannot
> reference the whole memory address space while carrying out DMA
> operations (eg some devices with bus address bits range smaller than
> system bus - which prevents them from using bus addresses that are
> otherwise valid for the system).
>
> The ACPI _DMA object allows bus devices to define the DMA window that is
> actually addressable by devices that sit upstream the bus, therefore
> providing a means to parse and initialize the devices DMA masks and
> addressable DMA range size.
>
> By relying on the generic ACPI kernel layer to retrieve and parse
> resources, introduce ACPI core code to parse the _DMA object.
>
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> drivers/acpi/resource.c | 35 +++++++++++++++++++++
> drivers/acpi/scan.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/acpi/acpi_bus.h | 2 ++
> include/linux/acpi.h | 8 +++++
> 4 files changed, 128 insertions(+)
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 2b20a09..9602248 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -636,6 +636,41 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
> }
> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>
> +static int is_memory(struct acpi_resource *ares, void *not_used)
> +{
> + struct resource_win win;
> + struct resource *res = &win.res;
> +
> + memset(&win, 0, sizeof(win));
> +
> + return !(acpi_dev_resource_memory(ares, res)
> + || acpi_dev_resource_address_space(ares, &win)
> + || acpi_dev_resource_ext_address_space(ares, &win));
> +}
> +
> +/**
> + * acpi_dev_get_dma_resources - Get current DMA resources of a device.
> + * @adev: ACPI device node to get the resources for.
> + * @list: Head of the resultant list of resources (must be empty).
> + *
> + * Evaluate the _DMA method for the given device node and process its
> + * output.
> + *
> + * The resultant struct resource objects are put on the list pointed to
> + * by @list, that must be empty initially, as members of struct
> + * resource_entry objects. Callers of this routine should use
> + * %acpi_dev_free_resource_list() to free that list.
> + *
> + * The number of resources in the output list is returned on success,
> + * an error code reflecting the error condition is returned otherwise.
> + */
> +int acpi_dev_get_dma_resources(struct acpi_device *adev, struct list_head *list)
> +{
> + return acpi_dev_get_resources_method(adev, list, is_memory, NULL,
> + METHOD_NAME__DMA);
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_get_dma_resources);
> +
> /**
> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
> * types
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 3389729..eb493c2 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1360,6 +1360,89 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
> }
>
> /**
> + * acpi_dma_get_range() - Get device DMA parameters.
> + *
> + * @dev: device to configure
> + * @dma_addr: pointer device DMA address result
> + * @offset: pointer to the DMA offset result
> + * @size: pointer to DMA range size result
> + *
> + * Evaluate DMA regions and return respectively DMA region start, offset
> + * and size in dma_addr, offset and size on parsing success; it does not
> + * update the passed in values on failure.
> + *
> + * Return 0 on success, < 0 on failure.
> + */
> +int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
> + u64 *size)
> +{
> + struct acpi_device *adev;
> + LIST_HEAD(list);
> + struct resource_entry *rentry;
> + int ret;
> + struct device *dma_dev = dev;
> + struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
> +
> + /*
> + * Walk the device tree chasing an ACPI companion with a _DMA
> + * object while we go. Stop if we find a device with an ACPI
> + * companion containing a _DMA method.
> + */
> + do {
> + if (has_acpi_companion(dma_dev)) {
> + adev = ACPI_COMPANION(dma_dev);
> +
> + if (acpi_has_method(adev->handle, METHOD_NAME__DMA))
> + break;
Why don't you do
adev = ACPI_COMPANION(dma_dev);
if (adev && acpi_has_method(adev->handle, METHOD_NAME__DMA))
break;
instead?
> + }
> + } while ((dma_dev = dma_dev->parent));
We had a rule to avoid things like this once and it wasn't a bad one. :-)
Why don't you just do
dma_dev = dma_dev->parent;
} while (dma_dev);
?
> +
> + if (!dma_dev)
> + return -ENODEV;
> +
> + if (!acpi_has_method(adev->handle, METHOD_NAME__CRS)) {
> + acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, &name_buffer);
> + pr_warn(FW_BUG "%s: _DMA object only valid in object with valid _CRS\n",
> + (char *)name_buffer.pointer);
> + kfree(name_buffer.pointer);
We have acpi_handle_warn() and friends for stuff like that ...
> + return -EINVAL;
> + }
> +
> + ret = acpi_dev_get_dma_resources(adev, &list);
> + if (ret > 0) {
> + list_for_each_entry(rentry, &list, node) {
> + if (dma_offset && rentry->offset != dma_offset) {
> + ret = -EINVAL;
> + pr_warn("Can't handle multiple windows with different offsets\n");
> + goto out;
> + }
> + dma_offset = rentry->offset;
> +
> + /* Take lower and upper limits */
> + if (rentry->res->start < dma_start)
> + dma_start = rentry->res->start;
> + if (rentry->res->end > dma_end)
> + dma_end = rentry->res->end;
> + }
> +
> + if (dma_start >= dma_end) {
> + ret = -EINVAL;
> + pr_warn("Invalid DMA regions configuration\n");
dev_warn()?
And why _warn() and not _info()?
> + goto out;
> + }
> +
> + *dma_addr = dma_start - dma_offset;
> + *size = dma_end - dma_start + 1;
> + *offset = dma_offset;
> + }
> + out:
> + acpi_dev_free_resource_list(&list);
> +
> + return ret >= 0 ? 0 : ret;
> +}
Thanks,
Rafael
On Sat, Jul 22, 2017 at 12:05:39AM +0200, Rafael J. Wysocki wrote:
> On Thursday, July 20, 2017 03:45:14 PM Lorenzo Pieralisi wrote:
> > The function acpi_dev_get_resources() is completely generic and
> > can be used to parse resource objects that are not necessarily
> > coming from the _CRS method but also from other objects eg _DMA
> > that have the same _CRS resource format.
> >
> > Create an acpi_dev_get_resources() helper, internal to the ACPI
> > resources parsing compilation unit, acpi_dev_get_resources_method(),
> > that takes a char* parameter to detect which ACPI method should be
> > called to retrieve the resources list and make acpi_dev_get_resources()
> > call it with a method name _CRS leaving the API behaviour unchanged.
> >
> > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > ---
> > drivers/acpi/resource.c | 54 +++++++++++++++++++++++++++++--------------------
> > 1 file changed, 32 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> > index cd4c427..2b20a09 100644
> > --- a/drivers/acpi/resource.c
> > +++ b/drivers/acpi/resource.c
> > @@ -573,6 +573,36 @@ static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
> > return AE_OK;
> > }
> >
> > +static
> > +int acpi_dev_get_resources_method(struct acpi_device *adev,
>
> Do not break lines like this, please.
>
> It should be
>
> static int acpi_dev_get...
>
> Also I would call it differently, maybe simply __acpi_dev_get_resources()?
Ok, it was how I wanted to call it but was not sure you would be happy
with it, I will rename it.
> > + struct list_head *list,
> > + int (*preproc)(struct acpi_resource *, void *),
> > + void *preproc_data, char *method)
>
> const char *method ?
Yes, will update.
Thanks !
Lorenzo
On Sat, Jul 22, 2017 at 12:15:42AM +0200, Rafael J. Wysocki wrote:
[...]
> > + * acpi_dma_get_range() - Get device DMA parameters.
> > + *
> > + * @dev: device to configure
> > + * @dma_addr: pointer device DMA address result
> > + * @offset: pointer to the DMA offset result
> > + * @size: pointer to DMA range size result
> > + *
> > + * Evaluate DMA regions and return respectively DMA region start, offset
> > + * and size in dma_addr, offset and size on parsing success; it does not
> > + * update the passed in values on failure.
> > + *
> > + * Return 0 on success, < 0 on failure.
> > + */
> > +int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
> > + u64 *size)
> > +{
> > + struct acpi_device *adev;
> > + LIST_HEAD(list);
> > + struct resource_entry *rentry;
> > + int ret;
> > + struct device *dma_dev = dev;
> > + struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > + u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
> > +
> > + /*
> > + * Walk the device tree chasing an ACPI companion with a _DMA
> > + * object while we go. Stop if we find a device with an ACPI
> > + * companion containing a _DMA method.
> > + */
> > + do {
> > + if (has_acpi_companion(dma_dev)) {
> > + adev = ACPI_COMPANION(dma_dev);
> > +
> > + if (acpi_has_method(adev->handle, METHOD_NAME__DMA))
> > + break;
>
> Why don't you do
>
> adev = ACPI_COMPANION(dma_dev);
> if (adev && acpi_has_method(adev->handle, METHOD_NAME__DMA))
> break;
>
> instead?
Yes, it is better.
> > + }
> > + } while ((dma_dev = dma_dev->parent));
>
> We had a rule to avoid things like this once and it wasn't a bad one. :-)
>
> Why don't you just do
>
> dma_dev = dma_dev->parent;
> } while (dma_dev);
>
> ?
Yes I should have done that in the first place, will update.
> > +
> > + if (!dma_dev)
> > + return -ENODEV;
> > +
> > + if (!acpi_has_method(adev->handle, METHOD_NAME__CRS)) {
> > + acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, &name_buffer);
> > + pr_warn(FW_BUG "%s: _DMA object only valid in object with valid _CRS\n",
> > + (char *)name_buffer.pointer);
> > + kfree(name_buffer.pointer);
>
> We have acpi_handle_warn() and friends for stuff like that ...
I will update to it.
> > + return -EINVAL;
> > + }
> > +
> > + ret = acpi_dev_get_dma_resources(adev, &list);
> > + if (ret > 0) {
> > + list_for_each_entry(rentry, &list, node) {
> > + if (dma_offset && rentry->offset != dma_offset) {
> > + ret = -EINVAL;
> > + pr_warn("Can't handle multiple windows with different offsets\n");
> > + goto out;
> > + }
> > + dma_offset = rentry->offset;
> > +
> > + /* Take lower and upper limits */
> > + if (rentry->res->start < dma_start)
> > + dma_start = rentry->res->start;
> > + if (rentry->res->end > dma_end)
> > + dma_end = rentry->res->end;
> > + }
> > +
> > + if (dma_start >= dma_end) {
> > + ret = -EINVAL;
> > + pr_warn("Invalid DMA regions configuration\n");
>
> dev_warn()?
>
> And why _warn() and not _info()?
Mmm..ok for the dev_ prefix - basically this would be a FW_BUG (I think
this specific error condition is overkill TBH, the ACPI resource
validation code should catch it before we even get here) not sure
about downgrading it to _info() though, I would leave it at this
loglevel - in particular in the offset check above:
if (dma_offset && rentry->offset != dma_offset) {
ret = -EINVAL;
pr_warn("Can't handle multiple windows with different offsets\n");
goto out;
}
Thanks,
Lorenzo
On Mon, Jul 24, 2017 at 12:40 PM, Lorenzo Pieralisi
<[email protected]> wrote:
> On Sat, Jul 22, 2017 at 12:15:42AM +0200, Rafael J. Wysocki wrote:
>
[cut]
>
>> > + return -EINVAL;
>> > + }
>> > +
>> > + ret = acpi_dev_get_dma_resources(adev, &list);
>> > + if (ret > 0) {
>> > + list_for_each_entry(rentry, &list, node) {
>> > + if (dma_offset && rentry->offset != dma_offset) {
>> > + ret = -EINVAL;
>> > + pr_warn("Can't handle multiple windows with different offsets\n");
>> > + goto out;
>> > + }
>> > + dma_offset = rentry->offset;
>> > +
>> > + /* Take lower and upper limits */
>> > + if (rentry->res->start < dma_start)
>> > + dma_start = rentry->res->start;
>> > + if (rentry->res->end > dma_end)
>> > + dma_end = rentry->res->end;
>> > + }
>> > +
>> > + if (dma_start >= dma_end) {
>> > + ret = -EINVAL;
>> > + pr_warn("Invalid DMA regions configuration\n");
>>
>> dev_warn()?
>>
>> And why _warn() and not _info()?
>
> Mmm..ok for the dev_ prefix - basically this would be a FW_BUG (I think
> this specific error condition is overkill TBH, the ACPI resource
> validation code should catch it before we even get here) not sure
> about downgrading it to _info() though, I would leave it at this
> loglevel - in particular in the offset check above:
>
> if (dma_offset && rentry->offset != dma_offset) {
> ret = -EINVAL;
> pr_warn("Can't handle multiple windows with different offsets\n");
> goto out;
> }
Well, so the "why" question above still has no answer ...
On Mon, Jul 24, 2017 at 08:42:15PM +0200, Rafael J. Wysocki wrote:
> On Mon, Jul 24, 2017 at 12:40 PM, Lorenzo Pieralisi
> <[email protected]> wrote:
> > On Sat, Jul 22, 2017 at 12:15:42AM +0200, Rafael J. Wysocki wrote:
> >
>
> [cut]
>
> >
> >> > + return -EINVAL;
> >> > + }
> >> > +
> >> > + ret = acpi_dev_get_dma_resources(adev, &list);
> >> > + if (ret > 0) {
> >> > + list_for_each_entry(rentry, &list, node) {
> >> > + if (dma_offset && rentry->offset != dma_offset) {
> >> > + ret = -EINVAL;
> >> > + pr_warn("Can't handle multiple windows with different offsets\n");
> >> > + goto out;
> >> > + }
> >> > + dma_offset = rentry->offset;
> >> > +
> >> > + /* Take lower and upper limits */
> >> > + if (rentry->res->start < dma_start)
> >> > + dma_start = rentry->res->start;
> >> > + if (rentry->res->end > dma_end)
> >> > + dma_end = rentry->res->end;
> >> > + }
> >> > +
> >> > + if (dma_start >= dma_end) {
> >> > + ret = -EINVAL;
> >> > + pr_warn("Invalid DMA regions configuration\n");
> >>
> >> dev_warn()?
> >>
> >> And why _warn() and not _info()?
> >
> > Mmm..ok for the dev_ prefix - basically this would be a FW_BUG (I think
> > this specific error condition is overkill TBH, the ACPI resource
> > validation code should catch it before we even get here) not sure
> > about downgrading it to _info() though, I would leave it at this
> > loglevel - in particular in the offset check above:
> >
> > if (dma_offset && rentry->offset != dma_offset) {
> > ret = -EINVAL;
> > pr_warn("Can't handle multiple windows with different offsets\n");
> > goto out;
> > }
>
> Well, so the "why" question above still has no answer ...
It is a firmware misconfiguration, we end up dismissing firmware
information and use the device with default/possibly misconfigured
DMA windows (ie offset == 0) for that platform, that's the reason
why I thought it would deserve a _warn rather than _info loglevel.
Thanks,
Lorenzo
On Sat, Jul 22, 2017 at 12:05:39AM +0200, Rafael J. Wysocki wrote:
> On Thursday, July 20, 2017 03:45:14 PM Lorenzo Pieralisi wrote:
> > The function acpi_dev_get_resources() is completely generic and
> > can be used to parse resource objects that are not necessarily
> > coming from the _CRS method but also from other objects eg _DMA
> > that have the same _CRS resource format.
> >
> > Create an acpi_dev_get_resources() helper, internal to the ACPI
> > resources parsing compilation unit, acpi_dev_get_resources_method(),
> > that takes a char* parameter to detect which ACPI method should be
> > called to retrieve the resources list and make acpi_dev_get_resources()
> > call it with a method name _CRS leaving the API behaviour unchanged.
> >
> > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > ---
> > drivers/acpi/resource.c | 54 +++++++++++++++++++++++++++++--------------------
> > 1 file changed, 32 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> > index cd4c427..2b20a09 100644
> > --- a/drivers/acpi/resource.c
> > +++ b/drivers/acpi/resource.c
> > @@ -573,6 +573,36 @@ static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
> > return AE_OK;
> > }
> >
> > +static
> > +int acpi_dev_get_resources_method(struct acpi_device *adev,
>
> Do not break lines like this, please.
>
> It should be
>
> static int acpi_dev_get...
>
> Also I would call it differently, maybe simply __acpi_dev_get_resources()?
>
> > + struct list_head *list,
> > + int (*preproc)(struct acpi_resource *, void *),
> > + void *preproc_data, char *method)
>
> const char *method ?
drivers/acpi/resource.c: In function '__acpi_dev_get_resources':
drivers/acpi/resource.c:587:37: warning: passing argument 2 of
'acpi_has_method' discards 'const' qualifier from pointer target type
[-Wdiscarded-qualifiers]
if (!acpi_has_method(adev->handle, method))
^~~~~~
In file included from ./include/linux/acpi.h:44:0,
from drivers/acpi/resource.c:21:
./include/acpi/acpi_bus.h:55:6: note: expected 'char *'
but argument is of type 'const char *'
bool acpi_has_method(acpi_handle handle, char *name);
Same problem with acpi_walk_resources().
So either I fiddle (eg cast the const away) with the method string
in __acpi_dev_get_resources() or I can add an int/bool to detect
which method to use ( _CRS or _DMA) instead of passing a const char *.
Thanks,
Lorenzo
On Tuesday, July 25, 2017 10:15:38 AM Lorenzo Pieralisi wrote:
> On Sat, Jul 22, 2017 at 12:05:39AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, July 20, 2017 03:45:14 PM Lorenzo Pieralisi wrote:
> > > The function acpi_dev_get_resources() is completely generic and
> > > can be used to parse resource objects that are not necessarily
> > > coming from the _CRS method but also from other objects eg _DMA
> > > that have the same _CRS resource format.
> > >
> > > Create an acpi_dev_get_resources() helper, internal to the ACPI
> > > resources parsing compilation unit, acpi_dev_get_resources_method(),
> > > that takes a char* parameter to detect which ACPI method should be
> > > called to retrieve the resources list and make acpi_dev_get_resources()
> > > call it with a method name _CRS leaving the API behaviour unchanged.
> > >
> > > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > ---
> > > drivers/acpi/resource.c | 54 +++++++++++++++++++++++++++++--------------------
> > > 1 file changed, 32 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> > > index cd4c427..2b20a09 100644
> > > --- a/drivers/acpi/resource.c
> > > +++ b/drivers/acpi/resource.c
> > > @@ -573,6 +573,36 @@ static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
> > > return AE_OK;
> > > }
> > >
> > > +static
> > > +int acpi_dev_get_resources_method(struct acpi_device *adev,
> >
> > Do not break lines like this, please.
> >
> > It should be
> >
> > static int acpi_dev_get...
> >
> > Also I would call it differently, maybe simply __acpi_dev_get_resources()?
> >
> > > + struct list_head *list,
> > > + int (*preproc)(struct acpi_resource *, void *),
> > > + void *preproc_data, char *method)
> >
> > const char *method ?
>
> drivers/acpi/resource.c: In function '__acpi_dev_get_resources':
> drivers/acpi/resource.c:587:37: warning: passing argument 2 of
> 'acpi_has_method' discards 'const' qualifier from pointer target type
> [-Wdiscarded-qualifiers]
> if (!acpi_has_method(adev->handle, method))
> ^~~~~~
> In file included from ./include/linux/acpi.h:44:0,
> from drivers/acpi/resource.c:21:
> ./include/acpi/acpi_bus.h:55:6: note: expected 'char *'
> but argument is of type 'const char *'
> bool acpi_has_method(acpi_handle handle, char *name);
>
> Same problem with acpi_walk_resources().
>
> So either I fiddle (eg cast the const away) with the method string
> in __acpi_dev_get_resources() or I can add an int/bool to detect
> which method to use ( _CRS or _DMA) instead of passing a const char *.
No, that'd be overkill. Let's keep the (char *) in there.
Thanks,
Rafael
On Tuesday, July 25, 2017 10:06:15 AM Lorenzo Pieralisi wrote:
> On Mon, Jul 24, 2017 at 08:42:15PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Jul 24, 2017 at 12:40 PM, Lorenzo Pieralisi
> > <[email protected]> wrote:
> > > On Sat, Jul 22, 2017 at 12:15:42AM +0200, Rafael J. Wysocki wrote:
> > >
> >
> > [cut]
> >
> > >
> > >> > + return -EINVAL;
> > >> > + }
> > >> > +
> > >> > + ret = acpi_dev_get_dma_resources(adev, &list);
> > >> > + if (ret > 0) {
> > >> > + list_for_each_entry(rentry, &list, node) {
> > >> > + if (dma_offset && rentry->offset != dma_offset) {
> > >> > + ret = -EINVAL;
> > >> > + pr_warn("Can't handle multiple windows with different offsets\n");
> > >> > + goto out;
> > >> > + }
> > >> > + dma_offset = rentry->offset;
> > >> > +
> > >> > + /* Take lower and upper limits */
> > >> > + if (rentry->res->start < dma_start)
> > >> > + dma_start = rentry->res->start;
> > >> > + if (rentry->res->end > dma_end)
> > >> > + dma_end = rentry->res->end;
> > >> > + }
> > >> > +
> > >> > + if (dma_start >= dma_end) {
> > >> > + ret = -EINVAL;
> > >> > + pr_warn("Invalid DMA regions configuration\n");
> > >>
> > >> dev_warn()?
> > >>
> > >> And why _warn() and not _info()?
> > >
> > > Mmm..ok for the dev_ prefix - basically this would be a FW_BUG (I think
> > > this specific error condition is overkill TBH, the ACPI resource
> > > validation code should catch it before we even get here) not sure
> > > about downgrading it to _info() though, I would leave it at this
> > > loglevel - in particular in the offset check above:
> > >
> > > if (dma_offset && rentry->offset != dma_offset) {
> > > ret = -EINVAL;
> > > pr_warn("Can't handle multiple windows with different offsets\n");
> > > goto out;
> > > }
> >
> > Well, so the "why" question above still has no answer ...
>
> It is a firmware misconfiguration, we end up dismissing firmware
> information and use the device with default/possibly misconfigured
> DMA windows (ie offset == 0) for that platform, that's the reason
> why I thought it would deserve a _warn rather than _info loglevel.
Well, _warn only makes it slightly more visible for people with somewhat
unusual logging configurations, but fair enough.
Thanks,
Rafael
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 <[email protected]>
> Cc: Hanjun Guo <[email protected]>
> Cc: Feng Kan <[email protected]>
> Cc: Jon Masters <[email protected]>
> Cc: Robert Moore <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Zhang Rui <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
>
> 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.
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 <[email protected]>
>> Cc: Hanjun Guo <[email protected]>
>> Cc: Feng Kan <[email protected]>
>> Cc: Jon Masters <[email protected]>
>> Cc: Robert Moore <[email protected]>
>> Cc: Robin Murphy <[email protected]>
>> Cc: Zhang Rui <[email protected]>
>> Cc: "Rafael J. Wysocki" <[email protected]>
>>
>> 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(-)
>>
>
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 <[email protected]>
> >> Cc: Hanjun Guo <[email protected]>
> >> Cc: Feng Kan <[email protected]>
> >> Cc: Jon Masters <[email protected]>
> >> Cc: Robert Moore <[email protected]>
> >> Cc: Robin Murphy <[email protected]>
> >> Cc: Zhang Rui <[email protected]>
> >> Cc: "Rafael J. Wysocki" <[email protected]>
> >>
> >> 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(-)
> >>
> >
>
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 <[email protected]>
>
> Thanks,
> Lorenzo
>
>> Robin.
>>
>>> -Nate
>>>>
>>>> Cc: Will Deacon <[email protected]>
>>>> Cc: Hanjun Guo <[email protected]>
>>>> Cc: Feng Kan <[email protected]>
>>>> Cc: Jon Masters <[email protected]>
>>>> Cc: Robert Moore <[email protected]>
>>>> Cc: Robin Murphy <[email protected]>
>>>> Cc: Zhang Rui <[email protected]>
>>>> Cc: "Rafael J. Wysocki" <[email protected]>
>>>>
>>>> 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.
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.
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
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.
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