If CONFIG_DMA_RESTRICTED_POOL=n then probing a device with a reference
to a "restricted-dma-pool" will fail with a reasonably cryptic error:
| pci-host-generic: probe of 10000.pci failed with error -22
Print a more helpful message in this case and try to continue probing
the device as we do if the kernel doesn't have the restricted DMA patches
applied or either CONFIG_OF_ADDRESS or CONFIG_HAS_DMA =n.
Cc: Claire Chang <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Rob Herring <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
drivers/of/address.c | 8 ++++----
drivers/of/device.c | 2 +-
drivers/of/of_private.h | 8 +++-----
3 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 973257434398..f6bf4b423c2a 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -997,7 +997,7 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
return ret;
}
-int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
+void of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
{
struct device_node *node, *of_node = dev->of_node;
int count, i;
@@ -1022,11 +1022,11 @@ int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
*/
if (of_device_is_compatible(node, "restricted-dma-pool") &&
of_device_is_available(node))
- return of_reserved_mem_device_init_by_idx(dev, of_node,
- i);
+ break;
}
- return 0;
+ if (i != count && of_reserved_mem_device_init_by_idx(dev, of_node, i))
+ dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
}
#endif /* CONFIG_HAS_DMA */
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 2defdca418ec..258a2b099410 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -166,7 +166,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
if (!iommu)
- return of_dma_set_restricted_buffer(dev, np);
+ of_dma_set_restricted_buffer(dev, np);
return 0;
}
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index f557bd22b0cf..bc883f69496b 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -163,18 +163,16 @@ struct bus_dma_region;
#if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map);
-int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
+void of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
#else
static inline int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map)
{
return -ENODEV;
}
-static inline int of_dma_set_restricted_buffer(struct device *dev,
- struct device_node *np)
+static inline void of_dma_set_restricted_buffer(struct device *dev,
+ struct device_node *np)
{
- /* Do nothing, successfully. */
- return 0;
}
#endif
--
2.32.0.605.g8dce9f2422-goog
On 2021-08-05 10:47, Will Deacon wrote:
> If CONFIG_DMA_RESTRICTED_POOL=n then probing a device with a reference
> to a "restricted-dma-pool" will fail with a reasonably cryptic error:
>
> | pci-host-generic: probe of 10000.pci failed with error -22
>
> Print a more helpful message in this case and try to continue probing
> the device as we do if the kernel doesn't have the restricted DMA patches
> applied or either CONFIG_OF_ADDRESS or CONFIG_HAS_DMA =n.
Makes sense to me;
Reviewed-by: Robin Murphy <[email protected]>
Although if we allow probe to succeed when a pool really was there for a
reason, it may end up being much more fatal if the driver then tries to
do a DMA transfer to any old memory and the device access causes an
SError, or the VM to be killed, or whatever. That's not quite the same
as the stubbed cases where the respective platforms couldn't have a
genuine pool to parse either way, but as you say it is what could happen
already if the user tried to use an older kernel, and I think the chance
of of_reserved_mem_device_init_by_idx() failing without something being
terminally wrong anyway - invalid DT, not enough RAM, etc. - is low
enough that it's probably not a major concern. Plus I'd hope that the
memory protection schemes people do actually implement don't take such
such a zero-tolerance approach anyway - allowing a malicious or
malfunctioning device to take down the system because it tried to make a
rogue access which *was* already contained seems a bit silly.
Robin.
> Cc: Claire Chang <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Rob Herring <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> drivers/of/address.c | 8 ++++----
> drivers/of/device.c | 2 +-
> drivers/of/of_private.h | 8 +++-----
> 3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 973257434398..f6bf4b423c2a 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -997,7 +997,7 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
> return ret;
> }
>
> -int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> +void of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> {
> struct device_node *node, *of_node = dev->of_node;
> int count, i;
> @@ -1022,11 +1022,11 @@ int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> */
> if (of_device_is_compatible(node, "restricted-dma-pool") &&
> of_device_is_available(node))
> - return of_reserved_mem_device_init_by_idx(dev, of_node,
> - i);
> + break;
> }
>
> - return 0;
> + if (i != count && of_reserved_mem_device_init_by_idx(dev, of_node, i))
> + dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
> }
> #endif /* CONFIG_HAS_DMA */
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 2defdca418ec..258a2b099410 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -166,7 +166,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
> arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
>
> if (!iommu)
> - return of_dma_set_restricted_buffer(dev, np);
> + of_dma_set_restricted_buffer(dev, np);
>
> return 0;
> }
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index f557bd22b0cf..bc883f69496b 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -163,18 +163,16 @@ struct bus_dma_region;
> #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
> int of_dma_get_range(struct device_node *np,
> const struct bus_dma_region **map);
> -int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
> +void of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
> #else
> static inline int of_dma_get_range(struct device_node *np,
> const struct bus_dma_region **map)
> {
> return -ENODEV;
> }
> -static inline int of_dma_set_restricted_buffer(struct device *dev,
> - struct device_node *np)
> +static inline void of_dma_set_restricted_buffer(struct device *dev,
> + struct device_node *np)
> {
> - /* Do nothing, successfully. */
> - return 0;
> }
> #endif
>
>
On Thu, Aug 05, 2021 at 11:26:15AM +0100, Robin Murphy wrote:
> On 2021-08-05 10:47, Will Deacon wrote:
> > If CONFIG_DMA_RESTRICTED_POOL=n then probing a device with a reference
> > to a "restricted-dma-pool" will fail with a reasonably cryptic error:
> >
> > | pci-host-generic: probe of 10000.pci failed with error -22
> >
> > Print a more helpful message in this case and try to continue probing
> > the device as we do if the kernel doesn't have the restricted DMA patches
> > applied or either CONFIG_OF_ADDRESS or CONFIG_HAS_DMA =n.
>
> Makes sense to me;
>
> Reviewed-by: Robin Murphy <[email protected]>
Cheers.
> Although if we allow probe to succeed when a pool really was there for a
> reason, it may end up being much more fatal if the driver then tries to do a
> DMA transfer to any old memory and the device access causes an SError, or
> the VM to be killed, or whatever. That's not quite the same as the stubbed
> cases where the respective platforms couldn't have a genuine pool to parse
> either way, but as you say it is what could happen already if the user tried
> to use an older kernel, and I think the chance of
> of_reserved_mem_device_init_by_idx() failing without something being
> terminally wrong anyway - invalid DT, not enough RAM, etc. - is low enough
> that it's probably not a major concern. Plus I'd hope that the memory
> protection schemes people do actually implement don't take such such a
> zero-tolerance approach anyway - allowing a malicious or malfunctioning
> device to take down the system because it tried to make a rogue access which
> *was* already contained seems a bit silly.
There's also a case where swiotlb is forced (swiotlb=force) but restricted
DMA pools have been sized and allocated for individual devices in the DT.
In this case, having the guest fallback to the default shared swiotlb
buffer is better than failing the probe if CONFIG_DMA_RESTRICTED_POOL=n.
Will
On Thu, Aug 5, 2021 at 3:47 AM Will Deacon <[email protected]> wrote:
>
> If CONFIG_DMA_RESTRICTED_POOL=n then probing a device with a reference
> to a "restricted-dma-pool" will fail with a reasonably cryptic error:
I'm left wondering why of_dma_set_restricted_buffer() is even enabled
with CONFIG_DMA_RESTRICTED_POOL=n.
of_dma_set_restricted_buffer() should use IS_ENABLED and it should
also be moved to of/device.c. There's no reason for it to be in
of/address.c. It has nothing to do with address parsing.
> | pci-host-generic: probe of 10000.pci failed with error -22
>
> Print a more helpful message in this case and try to continue probing
> the device as we do if the kernel doesn't have the restricted DMA patches
> applied or either CONFIG_OF_ADDRESS or CONFIG_HAS_DMA =n.
I think you'd have to work pretty hard to hit this code path with
either of the above config options off.
> Cc: Claire Chang <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Rob Herring <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> drivers/of/address.c | 8 ++++----
> drivers/of/device.c | 2 +-
> drivers/of/of_private.h | 8 +++-----
> 3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 973257434398..f6bf4b423c2a 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -997,7 +997,7 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
> return ret;
> }
>
> -int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> +void of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> {
> struct device_node *node, *of_node = dev->of_node;
> int count, i;
> @@ -1022,11 +1022,11 @@ int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> */
> if (of_device_is_compatible(node, "restricted-dma-pool") &&
> of_device_is_available(node))
> - return of_reserved_mem_device_init_by_idx(dev, of_node,
> - i);
> + break;
> }
>
> - return 0;
> + if (i != count && of_reserved_mem_device_init_by_idx(dev, of_node, i))
> + dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
> }
> #endif /* CONFIG_HAS_DMA */
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 2defdca418ec..258a2b099410 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -166,7 +166,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
> arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
>
> if (!iommu)
> - return of_dma_set_restricted_buffer(dev, np);
> + of_dma_set_restricted_buffer(dev, np);
>
> return 0;
> }
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index f557bd22b0cf..bc883f69496b 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -163,18 +163,16 @@ struct bus_dma_region;
> #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
> int of_dma_get_range(struct device_node *np,
> const struct bus_dma_region **map);
> -int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
> +void of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
> #else
> static inline int of_dma_get_range(struct device_node *np,
> const struct bus_dma_region **map)
> {
> return -ENODEV;
> }
> -static inline int of_dma_set_restricted_buffer(struct device *dev,
> - struct device_node *np)
> +static inline void of_dma_set_restricted_buffer(struct device *dev,
> + struct device_node *np)
> {
> - /* Do nothing, successfully. */
> - return 0;
> }
> #endif
>
> --
> 2.32.0.605.g8dce9f2422-goog
>