[RFC/PATCH 0/4] request_firmware() on memory constrained devices

[RFC/PATCH 0/4] request_firmware() on memory constrained devices

[Stephen Boyd]

Some systems are memory constrained but they need to load very
large firmwares. The firmware subsystem allows drivers to request
this firmware be loaded from the filesystem, but this requires
that the entire firmware be loaded into kernel memory first
before it's provided to the driver. This can lead to a situation
where we map the firmware twice, once to load the firmware into
kernel memory and once to copy the firmware into the final
resting place.

This design creates needless memory pressure and delays loading
because we have to copy from kernel memory to somewhere else.
This patch sets adds support to the request firmware and DMA APIs
to map DMA buffers a page at a time and load the firmware directly
into those pages, skipping the intermediate copying step and
alleviating memory pressure during firmware loading. The drawback
is that we can't use the firmware caching feature because the
memory for the firmware cache is never allocated.

Patches based on v4.5-rc1.

Laura Abbott (1):
  dma-mapping: Add dma_remap() APIs

Stephen Boyd (2):
  ARM64: dma: Add support for NO_KERNEL_MAPPING attribute
  firmware: Support requesting firmware directly into DMA memory

Vikram Mulukutla (1):
  firmware_class: Provide infrastructure to make fw caching optional

 arch/arm64/mm/dma-mapping.c   |  78 +++++++++++--
 drivers/base/firmware_class.c | 263 ++++++++++++++++++++++++++++++++----------
 include/linux/dma-mapping.h   |  35 ++++++
 include/linux/firmware.h      |  13 +++
 4 files changed, 316 insertions(+), 73 deletions(-)

-- 
2.7.0.25.gfc10eb5

[PATCH 1/4] ARM64: dma: Add support for NO_KERNEL_MAPPING attribute

[Stephen Boyd]

Both the IOMMU and non-IOMMU allocations don't respect the
NO_KERNEL_MAPPING attribute, therefore drivers can't save virtual
address space and time spent mapping large buffers that are
intended only for userspace. Plumb this attribute into the code
for both types of DMA ops.

Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
 arch/arm64/mm/dma-mapping.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 331c4ca6205c..06a593653f23 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -169,6 +169,9 @@ static void *__dma_alloc(struct device *dev, size_t size,
 
 	/* create a coherent mapping */
 	page = virt_to_page(ptr);
+	if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
+		return page;
+
 	coherent_ptr = dma_common_contiguous_remap(page, size, VM_USERMAP,
 						   prot, NULL);
 	if (!coherent_ptr)
@@ -194,7 +197,8 @@ static void __dma_free(struct device *dev, size_t size,
 	if (!is_device_dma_coherent(dev)) {
 		if (__free_from_pool(vaddr, size))
 			return;
-		vunmap(vaddr);
+		if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
+			vunmap(vaddr);
 	}
 	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
 }
@@ -567,6 +571,9 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 		if (!pages)
 			return NULL;
 
+		if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
+			return pages;
+
 		addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
 					      __builtin_return_address(0));
 		if (!addr)
@@ -624,18 +631,32 @@ static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 		if (WARN_ON(!area || !area->pages))
 			return;
 		iommu_dma_free(dev, area->pages, iosize, &handle);
-		dma_common_free_remap(cpu_addr, size, VM_USERMAP);
+		if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
+			dma_common_free_remap(cpu_addr, size, VM_USERMAP);
 	} else {
 		iommu_dma_unmap_page(dev, handle, iosize, 0, NULL);
 		__free_pages(virt_to_page(cpu_addr), get_order(size));
 	}
 }
 
+static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
+{
+	struct vm_struct *area;
+
+	if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
+		return cpu_addr;
+
+	area = find_vm_area(cpu_addr);
+	if (area)
+		return area->pages;
+	return NULL;
+}
+
 static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
 			      void *cpu_addr, dma_addr_t dma_addr, size_t size,
 			      struct dma_attrs *attrs)
 {
-	struct vm_struct *area;
+	struct page **pages;
 	int ret;
 
 	vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot,
@@ -644,11 +665,11 @@ static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
 	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
 		return ret;
 
-	area = find_vm_area(cpu_addr);
-	if (WARN_ON(!area || !area->pages))
+	pages = __iommu_get_pages(cpu_addr, attrs);
+	if (WARN_ON(!pages))
 		return -ENXIO;
 
-	return iommu_dma_mmap(area->pages, size, vma);
+	return iommu_dma_mmap(pages, size, vma);
 }
 
 static int __iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
@@ -656,12 +677,12 @@ static int __iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
 			       size_t size, struct dma_attrs *attrs)
 {
 	unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
-	struct vm_struct *area = find_vm_area(cpu_addr);
+	struct page **pages = __iommu_get_pages(cpu_addr, attrs);
 
-	if (WARN_ON(!area || !area->pages))
+	if (WARN_ON(!pages))
 		return -ENXIO;
 
-	return sg_alloc_table_from_pages(sgt, area->pages, count, 0, size,
+	return sg_alloc_table_from_pages(sgt, pages, count, 0, size,
 					 GFP_KERNEL);
 }
 
-- 
2.7.0.25.gfc10eb5

Re: [PATCH 1/4] ARM64: dma: Add support for NO_KERNEL_MAPPING attribute

[Robin Murphy]

On 08/03/16 09:22, Stephen Boyd wrote:
> Both the IOMMU and non-IOMMU allocations don't respect the
> NO_KERNEL_MAPPING attribute, therefore drivers can't save virtual
> address space and time spent mapping large buffers that are
> intended only for userspace. Plumb this attribute into the code
> for both types of DMA ops.

I have to say I'm a little dubious about how much time we save by not 
creating one mapping once, and instead going on to repeatedly create and 
tear down lots of mappings - my initial hunch would place that number 
somewhere below 0. Similarly for the degree of pressure on our hundreds 
of gigabytes of vmalloc space, especially on "memory constrained" systems.

Some real data would be a lot more convincing ;)

Robin.

> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>   arch/arm64/mm/dma-mapping.c | 39 ++++++++++++++++++++++++++++++---------
>   1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 331c4ca6205c..06a593653f23 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -169,6 +169,9 @@ static void *__dma_alloc(struct device *dev, size_t size,
>
>   	/* create a coherent mapping */
>   	page = virt_to_page(ptr);
> +	if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
> +		return page;
> +
>   	coherent_ptr = dma_common_contiguous_remap(page, size, VM_USERMAP,
>   						   prot, NULL);
>   	if (!coherent_ptr)
> @@ -194,7 +197,8 @@ static void __dma_free(struct device *dev, size_t size,
>   	if (!is_device_dma_coherent(dev)) {
>   		if (__free_from_pool(vaddr, size))
>   			return;
> -		vunmap(vaddr);
> +		if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
> +			vunmap(vaddr);
>   	}
>   	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
>   }
> @@ -567,6 +571,9 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
>   		if (!pages)
>   			return NULL;
>
> +		if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
> +			return pages;
> +
>   		addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
>   					      __builtin_return_address(0));
>   		if (!addr)
> @@ -624,18 +631,32 @@ static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
>   		if (WARN_ON(!area || !area->pages))
>   			return;
>   		iommu_dma_free(dev, area->pages, iosize, &handle);
> -		dma_common_free_remap(cpu_addr, size, VM_USERMAP);
> +		if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
> +			dma_common_free_remap(cpu_addr, size, VM_USERMAP);
>   	} else {
>   		iommu_dma_unmap_page(dev, handle, iosize, 0, NULL);
>   		__free_pages(virt_to_page(cpu_addr), get_order(size));
>   	}
>   }
>
> +static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
> +{
> +	struct vm_struct *area;
> +
> +	if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
> +		return cpu_addr;
> +
> +	area = find_vm_area(cpu_addr);
> +	if (area)
> +		return area->pages;
> +	return NULL;
> +}
> +
>   static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
>   			      void *cpu_addr, dma_addr_t dma_addr, size_t size,
>   			      struct dma_attrs *attrs)
>   {
> -	struct vm_struct *area;
> +	struct page **pages;
>   	int ret;
>
>   	vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot,
> @@ -644,11 +665,11 @@ static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
>   	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
>   		return ret;
>
> -	area = find_vm_area(cpu_addr);
> -	if (WARN_ON(!area || !area->pages))
> +	pages = __iommu_get_pages(cpu_addr, attrs);
> +	if (WARN_ON(!pages))
>   		return -ENXIO;
>
> -	return iommu_dma_mmap(area->pages, size, vma);
> +	return iommu_dma_mmap(pages, size, vma);
>   }
>
>   static int __iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
> @@ -656,12 +677,12 @@ static int __iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
>   			       size_t size, struct dma_attrs *attrs)
>   {
>   	unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> -	struct vm_struct *area = find_vm_area(cpu_addr);
> +	struct page **pages = __iommu_get_pages(cpu_addr, attrs);
>
> -	if (WARN_ON(!area || !area->pages))
> +	if (WARN_ON(!pages))
>   		return -ENXIO;
>
> -	return sg_alloc_table_from_pages(sgt, area->pages, count, 0, size,
> +	return sg_alloc_table_from_pages(sgt, pages, count, 0, size,
>   					 GFP_KERNEL);
>   }
>
>

[RFC/PATCH 2/4] dma-mapping: Add dma_remap() APIs

[Stephen Boyd]

From: Laura Abbott <lauraa@codeaurora.org>

Some systems are memory constrained but they need to load very
large firmwares. The firmware subsystem allows drivers to request
this firmware be loaded from the filesystem, but this requires
that the entire firmware be loaded into kernel memory first
before it's provided to the driver. This can lead to a situation
where we map the firmware twice, once to load the firmware into
kernel memory and once to copy the firmware into the final
resting place.

This design creates needless memory pressure and delays loading
because we have to copy from kernel memory to somewhere else.
Let's add a couple DMA APIs that allow us to map DMA buffers into
the CPU's address space in arbitrary sizes. With this API, we can
allocate a DMA buffer with DMA_ATTR_NO_KERNEL_MAPPING and move a
small mapping window across our large DMA buffer to load the
firmware directly into buffer.

Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
[stephen.boyd@linaro.org: Add dma_attrs and offset to API, use
dma_common_contiguous_remap() instead of ioremap_page_range(),
support dma_remap() even when DMA_ATTR_NO_KERNEL_MAPPING isn't
specified, rewrite commit text]
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---

TODO: Split off arm64 part into own patch?

 arch/arm64/mm/dma-mapping.c | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/dma-mapping.h | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 06a593653f23..92a313a7309b 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -345,6 +345,43 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
 	return ret;
 }
 
+static void *arm64_dma_remap(struct device *dev, void *cpu_addr,
+			     dma_addr_t handle, size_t size,
+			     unsigned long offset, struct dma_attrs *attrs)
+{
+	struct page *page = phys_to_page(dma_to_phys(dev, handle) + offset);
+	bool coherent = is_device_dma_coherent(dev);
+	pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
+	void *ptr;
+
+	if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs)) {
+		offset &= ~PAGE_MASK;
+		size = PAGE_ALIGN(size + offset);
+
+		ptr = dma_common_contiguous_remap(page, size, VM_USERMAP, prot,
+						  NULL);
+	} else {
+		ptr = cpu_addr;
+	}
+	if (!ptr)
+		return NULL;
+
+	return ptr + offset;
+}
+
+static void arm64_dma_unremap(struct device *dev, void *cpu_addr,
+			      size_t size, unsigned long offset,
+			      struct dma_attrs *attrs)
+{
+	if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
+		return;
+
+	offset &= ~PAGE_MASK;
+	cpu_addr -= offset;
+
+	vunmap(cpu_addr);
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -360,6 +397,8 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = swiotlb_dma_supported,
 	.mapping_error = swiotlb_dma_mapping_error,
+	.remap = arm64_dma_remap,
+	.unremap = arm64_dma_unremap,
 };
 
 static int __init atomic_pool_init(void)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 75857cda38e9..d4ae45746d61 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -64,6 +64,12 @@ struct dma_map_ops {
 	int (*mapping_error)(struct device *dev, dma_addr_t dma_addr);
 	int (*dma_supported)(struct device *dev, u64 mask);
 	int (*set_dma_mask)(struct device *dev, u64 mask);
+	void *(*remap)(struct device *dev, void *cpu_addr, dma_addr_t handle,
+			size_t size, unsigned long offset,
+			struct dma_attrs *attrs);
+	void (*unremap)(struct device *dev, void *cpu_addr,
+			size_t size, unsigned long offset,
+			struct dma_attrs *attrs);
 #ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
 	u64 (*get_required_mask)(struct device *dev);
 #endif
@@ -465,6 +471,35 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
 }
 #endif
 
+static inline void *dma_remap(struct device *dev, void *cpu_addr,
+		dma_addr_t dma_handle, size_t size, unsigned long offset,
+		struct dma_attrs *attrs)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (!ops)
+		return NULL;
+	if (!ops->remap)
+		return NULL;
+
+	return ops->remap(dev, cpu_addr, dma_handle, size, offset, attrs);
+}
+
+
+static inline void dma_unremap(struct device *dev, void *remapped_addr,
+				size_t size, unsigned long offset,
+				struct dma_attrs *attrs)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (!ops)
+		return;
+	if (!ops->unremap)
+		return;
+
+	return ops->unremap(dev, remapped_addr, size, offset, attrs);
+}
+
 static inline u64 dma_get_mask(struct device *dev)
 {
 	if (dev && dev->dma_mask && *dev->dma_mask)
-- 
2.7.0.25.gfc10eb5

[RFC/PATCH 3/4] firmware_class: Provide infrastructure to make fw caching optional

[Stephen Boyd]

From: Vikram Mulukutla <markivx@codeaurora.org>

Some low memory systems with complex peripherals cannot afford to
have the relatively large firmware images taking up valuable
memory during suspend and resume. Change the internal
implementation of firmware_class to disallow caching based on a
configurable option. In the near future, variants of
request_firmware will take advantage of this feature.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Vikram Mulukutla <markivx@codeaurora.org>
[stephen.boyd@linaro.org: Drop firmware_desc design and use flags]
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
 drivers/base/firmware_class.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b9250e564ebf..9a616cf7ac9f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -111,6 +111,7 @@ static inline long firmware_loading_timeout(void)
 #define FW_OPT_FALLBACK		0
 #endif
 #define FW_OPT_NO_WARN	(1U << 3)
+#define FW_OPT_NOCACHE	(1U << 4)
 
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
@@ -1095,14 +1096,16 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	 * should be fixed in devres or driver core.
 	 */
 	/* don't cache firmware handled without uevent */
-	if (device && (opt_flags & FW_OPT_UEVENT))
+	if (device && (opt_flags & FW_OPT_UEVENT) &&
+	    !(opt_flags & FW_OPT_NOCACHE))
 		fw_add_devm_name(device, buf->fw_id);
 
 	/*
 	 * After caching firmware image is started, let it piggyback
 	 * on request firmware.
 	 */
-	if (buf->fwc->state == FW_LOADER_START_CACHE) {
+	if (!(opt_flags & FW_OPT_NOCACHE) &&
+	    buf->fwc->state == FW_LOADER_START_CACHE) {
 		if (fw_cache_piggyback_on_request(buf->fw_id))
 			kref_get(&buf->ref);
 	}
-- 
2.7.0.25.gfc10eb5

[RFC/PATCH 4/4] firmware: Support requesting firmware directly into DMA memory

[Stephen Boyd]

Some systems are memory constrained but they need to load very
large firmwares. The firmware subsystem allows drivers to request
this firmware be loaded from the filesystem, but this requires
that the entire firmware be loaded into kernel memory first
before it's provided to the driver. This can lead to a situation
where we map the firmware twice, once to load the firmware into
kernel memory and once to copy the firmware into the final
resting place.

This design creates needless memory pressure and delays loading
because we have to copy from kernel memory to somewhere else.
Let's add a request_firmware_dma() API that allows drivers to
request firmware be loaded directly into a DMA buffer that's been
pre-allocated. This skips the intermediate step of allocating a
buffer in kernel memory to hold the firmware image while it's
read from the filesystem and copying it to another location. It
also requires that drivers know how much memory they'll require
before requesting the firmware and negates any benefits of
firmware caching.

This is based on a patch from Vikram Mulukutla on
codeaurora.org[1].

[1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=rel/msm-3.18&id=0a328c5f6cd999f5c591f172216835636f39bcb5
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Vikram Mulukutla <markivx@codeaurora.org>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---

TODO:
 * Handle security_kernel_fw_from_file() in the userhelper fallback case
 * Rework for pending patches from Mimi Zohar that change the way files
   are read in kernel space

 drivers/base/firmware_class.c | 256 ++++++++++++++++++++++++++++++++----------
 include/linux/firmware.h      |  13 +++
 2 files changed, 207 insertions(+), 62 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 9a616cf7ac9f..8ab7a418f1d7 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,8 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/kernel.h>
+#include <linux/dma-mapping.h>
 
 #include <generated/utsrelease.h>
 
@@ -154,6 +156,15 @@ struct firmware_buf {
 	const char *fw_id;
 };
 
+struct firmware_dma_desc {
+	struct device *dev;
+	void *cpu_addr;
+	dma_addr_t dma_addr;
+	unsigned long offset;
+	size_t size;
+	struct dma_attrs *attrs;
+};
+
 struct fw_cache_entry {
 	struct list_head list;
 	const char *name;
@@ -292,39 +303,83 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
-static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
+static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf,
+				 struct firmware_dma_desc *dma)
 {
-	int size;
+	int size, rsize, remaining;
 	char *buf;
 	int rc;
+	loff_t offset;
+	unsigned long dma_offset;
 
 	if (!S_ISREG(file_inode(file)->i_mode))
 		return -EINVAL;
 	size = i_size_read(file_inode(file));
 	if (size <= 0)
 		return -EINVAL;
-	buf = vmalloc(size);
-	if (!buf)
-		return -ENOMEM;
-	rc = kernel_read(file, 0, buf, size);
-	if (rc != size) {
-		if (rc > 0)
-			rc = -EIO;
-		goto fail;
+
+	if (dma) {
+		if (size + dma->offset > dma->size)
+			return -EINVAL;
+
+		dma_offset = dma->offset;
+		offset = 0;
+		remaining = size;
+
+		while (remaining > 0) {
+			rsize = min_t(int, remaining, PAGE_SIZE);
+
+			buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr,
+					rsize, dma_offset, dma->attrs);
+			if (!buf)
+				return -ENOMEM;
+
+			rc = kernel_read(file, offset, buf, rsize);
+			if (rc != rsize) {
+				if (rc > 0)
+					rc = -EIO;
+				goto fail_dma;
+			}
+			rc = security_kernel_fw_from_file(file, buf, rsize);
+			if (rc)
+				goto fail_dma;
+
+			dma_unremap(dma->dev, buf, rsize, dma_offset,
+				    dma->attrs);
+			dma_offset += rsize;
+			offset += rsize;
+			remaining -= rsize;
+		}
+	} else {
+		buf = vmalloc(size);
+		if (!buf)
+			return -ENOMEM;
+		rc = kernel_read(file, 0, buf, size);
+		if (rc != size) {
+			if (rc > 0)
+				rc = -EIO;
+			goto fail;
+		}
+		rc = security_kernel_fw_from_file(file, buf, size);
+		if (rc)
+			goto fail;
+
+		fw_buf->data = buf;
 	}
-	rc = security_kernel_fw_from_file(file, buf, size);
-	if (rc)
-		goto fail;
-	fw_buf->data = buf;
+
 	fw_buf->size = size;
 	return 0;
 fail:
 	vfree(buf);
 	return rc;
+fail_dma:
+	dma_unremap(dma->dev, buf, rsize, dma_offset, dma->attrs);
+	return rc;
 }
 
-static int fw_get_filesystem_firmware(struct device *device,
-				       struct firmware_buf *buf)
+static int
+fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf,
+			   struct firmware_dma_desc *dma)
 {
 	int i, len;
 	int rc = -ENOENT;
@@ -351,7 +406,7 @@ static int fw_get_filesystem_firmware(struct device *device,
 		file = filp_open(path, O_RDONLY, 0);
 		if (IS_ERR(file))
 			continue;
-		rc = fw_read_file_contents(file, buf);
+		rc = fw_read_file_contents(file, buf, dma);
 		fput(file);
 		if (rc)
 			dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
@@ -470,6 +525,7 @@ struct firmware_priv {
 	struct device dev;
 	struct firmware_buf *buf;
 	struct firmware *fw;
+	struct firmware_dma_desc *dma;
 };
 
 static struct firmware_priv *to_firmware_priv(struct device *dev)
@@ -716,6 +772,52 @@ out:
 
 static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
 
+static ssize_t firmware_dma_rw(struct firmware_dma_desc *dma, char *buffer,
+			       loff_t *offset, size_t count, bool read)
+{
+	char *fw_buf;
+	int retval = count;
+
+	if ((dma->offset + *offset + count) > dma->size)
+		return -EINVAL;
+
+	fw_buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr, count,
+			   dma->offset + *offset, dma->attrs);
+	if (!fw_buf)
+		return -ENOMEM;
+
+	if (read)
+		memcpy(buffer, fw_buf, count);
+	else
+		memcpy(fw_buf, buffer, count);
+
+	dma_unremap(dma->dev, fw_buf, count, dma->offset + *offset, dma->attrs);
+	*offset += count;
+
+	return retval;
+}
+
+static void firmware_rw(struct firmware_buf *buf, char *buffer,
+			loff_t offset, size_t count, bool read)
+{
+	while (count) {
+		void *page_data;
+		int page_nr = offset >> PAGE_SHIFT;
+		int page_ofs = offset & (PAGE_SIZE-1);
+		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
+
+		page_data = kmap(buf->pages[page_nr]);
+
+		memcpy(buffer, page_data + page_ofs, page_cnt);
+
+		kunmap(buf->pages[page_nr]);
+		buffer += page_cnt;
+		offset += page_cnt;
+		count -= page_cnt;
+	}
+
+}
+
 static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 				  struct bin_attribute *bin_attr,
 				  char *buffer, loff_t offset, size_t count)
@@ -740,21 +842,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	ret_count = count;
 
-	while (count) {
-		void *page_data;
-		int page_nr = offset >> PAGE_SHIFT;
-		int page_ofs = offset & (PAGE_SIZE-1);
-		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
-
-		page_data = kmap(buf->pages[page_nr]);
+	if (fw_priv->dma)
+		ret_count = firmware_dma_rw(fw_priv->dma, buffer, &offset,
+					    count, true);
+	else
+		firmware_rw(buf, buffer, offset, count, true);
 
-		memcpy(buffer, page_data + page_ofs, page_cnt);
-
-		kunmap(buf->pages[page_nr]);
-		buffer += page_cnt;
-		offset += page_cnt;
-		count -= page_cnt;
-	}
 out:
 	mutex_unlock(&fw_lock);
 	return ret_count;
@@ -817,6 +910,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct firmware_dma_desc *dma = fw_priv->dma;
 	struct firmware_buf *buf;
 	ssize_t retval;
 
@@ -830,26 +924,17 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 		goto out;
 	}
 
-	retval = fw_realloc_buffer(fw_priv, offset + count);
-	if (retval)
-		goto out;
-
-	retval = count;
-
-	while (count) {
-		void *page_data;
-		int page_nr = offset >> PAGE_SHIFT;
-		int page_ofs = offset & (PAGE_SIZE - 1);
-		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
-
-		page_data = kmap(buf->pages[page_nr]);
-
-		memcpy(page_data + page_ofs, buffer, page_cnt);
+	if (dma) {
+		retval = firmware_dma_rw(dma, buffer, &offset, count, false);
+		if (retval < 0)
+			goto out;
+	} else {
+		retval = fw_realloc_buffer(fw_priv, offset + count);
+		if (retval)
+			goto out;
 
-		kunmap(buf->pages[page_nr]);
-		buffer += page_cnt;
-		offset += page_cnt;
-		count -= page_cnt;
+		retval = count;
+		firmware_rw(buf, buffer, offset, count, false);
 	}
 
 	buf->size = max_t(size_t, offset, buf->size);
@@ -887,7 +972,8 @@ static const struct attribute_group *fw_dev_attr_groups[] = {
 
 static struct firmware_priv *
 fw_create_instance(struct firmware *firmware, const char *fw_name,
-		   struct device *device, unsigned int opt_flags)
+		   struct device *device, unsigned int opt_flags,
+		   struct firmware_dma_desc *dma)
 {
 	struct firmware_priv *fw_priv;
 	struct device *f_dev;
@@ -900,6 +986,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 
 	fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
 	fw_priv->fw = firmware;
+	fw_priv->dma = dma;
 	f_dev = &fw_priv->dev;
 
 	device_initialize(f_dev);
@@ -920,7 +1007,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 	struct firmware_buf *buf = fw_priv->buf;
 
 	/* fall back on userspace loading */
-	buf->is_paged_buf = true;
+	if (!fw_priv->dma)
+		buf->is_paged_buf = true;
 
 	dev_set_uevent_suppress(f_dev, true);
 
@@ -955,7 +1043,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 
 	if (is_fw_load_aborted(buf))
 		retval = -EAGAIN;
-	else if (!buf->data)
+	else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
 
 	device_del(f_dev);
@@ -966,11 +1054,12 @@ err_put_dev:
 
 static int fw_load_from_user_helper(struct firmware *firmware,
 				    const char *name, struct device *device,
-				    unsigned int opt_flags, long timeout)
+				    unsigned int opt_flags, long timeout,
+				    struct firmware_dma_desc *dma)
 {
 	struct firmware_priv *fw_priv;
 
-	fw_priv = fw_create_instance(firmware, name, device, opt_flags);
+	fw_priv = fw_create_instance(firmware, name, device, opt_flags, dma);
 	if (IS_ERR(fw_priv))
 		return PTR_ERR(fw_priv);
 
@@ -998,7 +1087,7 @@ static void kill_requests_without_uevent(void)
 static inline int
 fw_load_from_user_helper(struct firmware *firmware, const char *name,
 			 struct device *device, unsigned int opt_flags,
-			 long timeout)
+			 long timeout, struct firmware_dma_desc *dma)
 {
 	return -ENOENT;
 }
@@ -1119,7 +1208,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-		  struct device *device, unsigned int opt_flags)
+		  struct device *device, struct firmware_dma_desc *dma,
+		  unsigned int opt_flags)
 {
 	struct firmware *fw = NULL;
 	long timeout;
@@ -1156,7 +1246,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		}
 	}
 
-	ret = fw_get_filesystem_firmware(device, fw->priv);
+	ret = fw_get_filesystem_firmware(device, fw->priv, dma);
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
@@ -1165,7 +1255,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		if (opt_flags & FW_OPT_USERHELPER) {
 			dev_warn(device, "Falling back to user helper\n");
 			ret = fw_load_from_user_helper(fw, name, device,
-						       opt_flags, timeout);
+						       opt_flags, timeout,
+						       dma);
 		}
 	}
 
@@ -1212,7 +1303,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device,
+	ret = _request_firmware(firmware_p, name, device, NULL,
 				FW_OPT_UEVENT | FW_OPT_FALLBACK);
 	module_put(THIS_MODULE);
 	return ret;
@@ -1236,7 +1327,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
 	int ret;
 
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device,
+	ret = _request_firmware(firmware_p, name, device, NULL,
 				FW_OPT_UEVENT | FW_OPT_NO_WARN);
 	module_put(THIS_MODULE);
 	return ret;
@@ -1244,6 +1335,47 @@ int request_firmware_direct(const struct firmware **firmware_p,
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
 /**
+ * request_firmware_dma - load firmware into a DMA allocated buffer
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded and DMA region allocated
+ * @cpu_addr: address returned from dma_alloc_*()
+ * @dma_addr: address returned from dma_alloc_*()
+ * @offset: offset into DMA buffer to copy firmware into
+ * @size: size of DMA buffer
+ * @attrs: attributes used during DMA allocation time
+ *
+ * This function works pretty much like request_firmware(), but it doesn't
+ * load the firmware into @firmware_p's data member. Instead, the firmware
+ * is loaded directly into the buffer pointed to by @cpu_addr and @dma_addr.
+ * This function doesn't cache firmware either.
+ */
+int
+request_firmware_dma(const struct firmware **firmware_p, const char *name,
+		     struct device *device, void *cpu_addr, dma_addr_t dma_addr,
+		     unsigned long offset, size_t size, struct dma_attrs *attrs)
+{
+	int ret;
+	struct firmware_dma_desc dma = {
+		.dev = device,
+		.cpu_addr = cpu_addr,
+		.dma_addr = dma_addr,
+		.offset = offset,
+		.size = size,
+		.attrs = attrs,
+	};
+
+	/* Need to pin this module until return */
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, &dma,
+				FW_OPT_UEVENT | FW_OPT_FALLBACK |
+				FW_OPT_NOCACHE);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL(request_firmware_dma);
+
+/**
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
@@ -1275,7 +1407,7 @@ static void request_firmware_work_func(struct work_struct *work)
 
 	fw_work = container_of(work, struct firmware_work, work);
 
-	_request_firmware(&fw, fw_work->name, fw_work->device,
+	_request_firmware(&fw, fw_work->name, fw_work->device, NULL,
 			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5c41c5e75b5c..06bc8d5965ca 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -19,6 +19,7 @@ struct firmware {
 
 struct module;
 struct device;
+struct dma_attrs;
 
 struct builtin_fw {
 	char *name;
@@ -47,6 +48,10 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
+int request_firmware_dma(const struct firmware **firmware_p, const char *name,
+		     struct device *device, void *cpu_addr, dma_addr_t dma_addr,
+		     unsigned long offset, size_t size,
+		     struct dma_attrs *attrs);
 
 void release_firmware(const struct firmware *fw);
 #else
@@ -75,5 +80,13 @@ static inline int request_firmware_direct(const struct firmware **fw,
 	return -EINVAL;
 }
 
+static inline int request_firmware_dma(const struct firmware **firmware_p,
+	const char *name, struct device *device, void *cpu_addr,
+	dma_addr_t dma_addr, unsigned long offset, size_t size,
+	struct dma_attrs *attrs)
+{
+	return -EINVAL;
+}
+
 #endif
 #endif
-- 
2.7.0.25.gfc10eb5

Re: [RFC/PATCH 4/4] firmware: Support requesting firmware directly into DMA memory

[Mimi Zohar]

Hi Stephen,

[CC''ing linux-security-module]

On Tue, 2016-03-08 at 16:22 +0700, Stephen Boyd wrote:
> Some systems are memory constrained but they need to load very
> large firmwares. The firmware subsystem allows drivers to request
> this firmware be loaded from the filesystem, but this requires
> that the entire firmware be loaded into kernel memory first
> before it's provided to the driver. This can lead to a situation
> where we map the firmware twice, once to load the firmware into
> kernel memory and once to copy the firmware into the final
> resting place.
> 
> This design creates needless memory pressure and delays loading
> because we have to copy from kernel memory to somewhere else.
> Let's add a request_firmware_dma() API that allows drivers to
> request firmware be loaded directly into a DMA buffer that's been
> pre-allocated. This skips the intermediate step of allocating a
> buffer in kernel memory to hold the firmware image while it's
> read from the filesystem and copying it to another location. It
> also requires that drivers know how much memory they'll require
> before requesting the firmware and negates any benefits of
> firmware caching.

Prior to the kernel_read_file() family of functions, IMA/IMA-appraisal,
when enabled, pre-read the firmware a page at a time calculating the
file hash and then verified the file signature, before the firmware was
re-read into memory.   With the kernel_read_file() family of functions
the firmware is read into memory once, the signature verified, before
the driver is given access.

This patch set wants to read the file piecemeal, giving the driver
direct access to the pieces read, without first verifying the firmware
signature.  This breaks the concepts of measuring (trusted boot) or
appraising (secure boot) a file before usage.
   
> This is based on a patch from Vikram Mulukutla on
> codeaurora.org[1].
> 
> [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=rel/msm-3.18&id=0a328c5f6cd999f5c591f172216835636f39bcb5
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Vikram Mulukutla <markivx@codeaurora.org>
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
> 
> TODO:
>  * Handle security_kernel_fw_from_file() in the userhelper fallback case
>  * Rework for pending patches from Mimi Zohar that change the way files
>    are read in kernel space
> 
>  drivers/base/firmware_class.c | 256 ++++++++++++++++++++++++++++++++----------
>  include/linux/firmware.h      |  13 +++
>  2 files changed, 207 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 9a616cf7ac9f..8ab7a418f1d7 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -29,6 +29,8 @@
>  #include <linux/syscore_ops.h>
>  #include <linux/reboot.h>
>  #include <linux/security.h>
> +#include <linux/kernel.h>
> +#include <linux/dma-mapping.h>
> 
>  #include <generated/utsrelease.h>
> 
> @@ -154,6 +156,15 @@ struct firmware_buf {
>  	const char *fw_id;
>  };
> 
> +struct firmware_dma_desc {
> +	struct device *dev;
> +	void *cpu_addr;
> +	dma_addr_t dma_addr;
> +	unsigned long offset;
> +	size_t size;
> +	struct dma_attrs *attrs;
> +};
> +
>  struct fw_cache_entry {
>  	struct list_head list;
>  	const char *name;
> @@ -292,39 +303,83 @@ static const char * const fw_path[] = {
>  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
> 
> -static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf,
> +				 struct firmware_dma_desc *dma)
>  {
> -	int size;
> +	int size, rsize, remaining;
>  	char *buf;
>  	int rc;
> +	loff_t offset;
> +	unsigned long dma_offset;
> 
>  	if (!S_ISREG(file_inode(file)->i_mode))
>  		return -EINVAL;
>  	size = i_size_read(file_inode(file));
>  	if (size <= 0)
>  		return -EINVAL;
> -	buf = vmalloc(size);
> -	if (!buf)
> -		return -ENOMEM;
> -	rc = kernel_read(file, 0, buf, size);
> -	if (rc != size) {
> -		if (rc > 0)
> -			rc = -EIO;
> -		goto fail;
> +
> +	if (dma) {
> +		if (size + dma->offset > dma->size)
> +			return -EINVAL;
> +
> +		dma_offset = dma->offset;
> +		offset = 0;
> +		remaining = size;
> +
> +		while (remaining > 0) {
> +			rsize = min_t(int, remaining, PAGE_SIZE);
> +
> +			buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr,
> +					rsize, dma_offset, dma->attrs);
> +			if (!buf)
> +				return -ENOMEM;
> +
> +			rc = kernel_read(file, offset, buf, rsize);
> +			if (rc != rsize) {
> +				if (rc > 0)
> +					rc = -EIO;
> +				goto fail_dma;
> +			}
> +			rc = security_kernel_fw_from_file(file, buf, rsize); 

The security_kernel_fw_from_file hook was removed and replaced with the
pre and post hooks named security_kernel_read_file() hook and
security_kernel_post_read_file() respectively.  The pre security hook
gives the LSMs and IMA-appraisal opportunity to deny the read.   In
addition to giving the LSMs the opportunity to fail the firmware
request, the post security hook allows IMA/IMA-appraisal to measure the
file and appraise the file signature.

Mimi

> +			if (rc)
> +				goto fail_dma;
> +
> +			dma_unremap(dma->dev, buf, rsize, dma_offset,
> +				    dma->attrs);
> +			dma_offset += rsize;
> +			offset += rsize;
> +			remaining -= rsize;
> +		}
> +	} else {
> +		buf = vmalloc(size);
> +		if (!buf)
> +			return -ENOMEM;
> +		rc = kernel_read(file, 0, buf, size);
> +		if (rc != size) {
> +			if (rc > 0)
> +				rc = -EIO;
> +			goto fail;
> +		}
> +		rc = security_kernel_fw_from_file(file, buf, size);
> +		if (rc)
> +			goto fail;
> +
> +		fw_buf->data = buf;
>  	}
> -	rc = security_kernel_fw_from_file(file, buf, size);
> -	if (rc)
> -		goto fail;
> -	fw_buf->data = buf;
> +
>  	fw_buf->size = size;
>  	return 0;
>  fail:
>  	vfree(buf);
>  	return rc;
> +fail_dma:
> +	dma_unremap(dma->dev, buf, rsize, dma_offset, dma->attrs);
> +	return rc;
>  }
> 
> -static int fw_get_filesystem_firmware(struct device *device,
> -				       struct firmware_buf *buf)
> +static int
> +fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf,
> +			   struct firmware_dma_desc *dma)
>  {
>  	int i, len;
>  	int rc = -ENOENT;
> @@ -351,7 +406,7 @@ static int fw_get_filesystem_firmware(struct device *device,
>  		file = filp_open(path, O_RDONLY, 0);
>  		if (IS_ERR(file))
>  			continue;
> -		rc = fw_read_file_contents(file, buf);
> +		rc = fw_read_file_contents(file, buf, dma);
>  		fput(file);
>  		if (rc)
>  			dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
> @@ -470,6 +525,7 @@ struct firmware_priv {
>  	struct device dev;
>  	struct firmware_buf *buf;
>  	struct firmware *fw;
> +	struct firmware_dma_desc *dma;
>  };
> 
>  static struct firmware_priv *to_firmware_priv(struct device *dev)
> @@ -716,6 +772,52 @@ out:
> 
>  static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
> 
> +static ssize_t firmware_dma_rw(struct firmware_dma_desc *dma, char *buffer,
> +			       loff_t *offset, size_t count, bool read)
> +{
> +	char *fw_buf;
> +	int retval = count;
> +
> +	if ((dma->offset + *offset + count) > dma->size)
> +		return -EINVAL;
> +
> +	fw_buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr, count,
> +			   dma->offset + *offset, dma->attrs);
> +	if (!fw_buf)
> +		return -ENOMEM;
> +
> +	if (read)
> +		memcpy(buffer, fw_buf, count);
> +	else
> +		memcpy(fw_buf, buffer, count);
> +
> +	dma_unremap(dma->dev, fw_buf, count, dma->offset + *offset, dma->attrs);
> +	*offset += count;
> +
> +	return retval;
> +}
> +
> +static void firmware_rw(struct firmware_buf *buf, char *buffer,
> +			loff_t offset, size_t count, bool read)
> +{
> +	while (count) {
> +		void *page_data;
> +		int page_nr = offset >> PAGE_SHIFT;
> +		int page_ofs = offset & (PAGE_SIZE-1);
> +		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> +
> +		page_data = kmap(buf->pages[page_nr]);
> +
> +		memcpy(buffer, page_data + page_ofs, page_cnt);
> +
> +		kunmap(buf->pages[page_nr]);
> +		buffer += page_cnt;
> +		offset += page_cnt;
> +		count -= page_cnt;
> +	}
> +
> +}
> +
>  static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>  				  struct bin_attribute *bin_attr,
>  				  char *buffer, loff_t offset, size_t count)
> @@ -740,21 +842,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
> 
>  	ret_count = count;
> 
> -	while (count) {
> -		void *page_data;
> -		int page_nr = offset >> PAGE_SHIFT;
> -		int page_ofs = offset & (PAGE_SIZE-1);
> -		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> -
> -		page_data = kmap(buf->pages[page_nr]);
> +	if (fw_priv->dma)
> +		ret_count = firmware_dma_rw(fw_priv->dma, buffer, &offset,
> +					    count, true);
> +	else
> +		firmware_rw(buf, buffer, offset, count, true);
> 
> -		memcpy(buffer, page_data + page_ofs, page_cnt);
> -
> -		kunmap(buf->pages[page_nr]);
> -		buffer += page_cnt;
> -		offset += page_cnt;
> -		count -= page_cnt;
> -	}
>  out:
>  	mutex_unlock(&fw_lock);
>  	return ret_count;
> @@ -817,6 +910,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  {
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
> +	struct firmware_dma_desc *dma = fw_priv->dma;
>  	struct firmware_buf *buf;
>  	ssize_t retval;
> 
> @@ -830,26 +924,17 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  		goto out;
>  	}
> 
> -	retval = fw_realloc_buffer(fw_priv, offset + count);
> -	if (retval)
> -		goto out;
> -
> -	retval = count;
> -
> -	while (count) {
> -		void *page_data;
> -		int page_nr = offset >> PAGE_SHIFT;
> -		int page_ofs = offset & (PAGE_SIZE - 1);
> -		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> -
> -		page_data = kmap(buf->pages[page_nr]);
> -
> -		memcpy(page_data + page_ofs, buffer, page_cnt);
> +	if (dma) {
> +		retval = firmware_dma_rw(dma, buffer, &offset, count, false);
> +		if (retval < 0)
> +			goto out;
> +	} else {
> +		retval = fw_realloc_buffer(fw_priv, offset + count);
> +		if (retval)
> +			goto out;
> 
> -		kunmap(buf->pages[page_nr]);
> -		buffer += page_cnt;
> -		offset += page_cnt;
> -		count -= page_cnt;
> +		retval = count;
> +		firmware_rw(buf, buffer, offset, count, false);
>  	}
> 
>  	buf->size = max_t(size_t, offset, buf->size);
> @@ -887,7 +972,8 @@ static const struct attribute_group *fw_dev_attr_groups[] = {
> 
>  static struct firmware_priv *
>  fw_create_instance(struct firmware *firmware, const char *fw_name,
> -		   struct device *device, unsigned int opt_flags)
> +		   struct device *device, unsigned int opt_flags,
> +		   struct firmware_dma_desc *dma)
>  {
>  	struct firmware_priv *fw_priv;
>  	struct device *f_dev;
> @@ -900,6 +986,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
> 
>  	fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
>  	fw_priv->fw = firmware;
> +	fw_priv->dma = dma;
>  	f_dev = &fw_priv->dev;
> 
>  	device_initialize(f_dev);
> @@ -920,7 +1007,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>  	struct firmware_buf *buf = fw_priv->buf;
> 
>  	/* fall back on userspace loading */
> -	buf->is_paged_buf = true;
> +	if (!fw_priv->dma)
> +		buf->is_paged_buf = true;
> 
>  	dev_set_uevent_suppress(f_dev, true);
> 
> @@ -955,7 +1043,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
> 
>  	if (is_fw_load_aborted(buf))
>  		retval = -EAGAIN;
> -	else if (!buf->data)
> +	else if (buf->is_paged_buf && !buf->data)
>  		retval = -ENOMEM;
> 
>  	device_del(f_dev);
> @@ -966,11 +1054,12 @@ err_put_dev:
> 
>  static int fw_load_from_user_helper(struct firmware *firmware,
>  				    const char *name, struct device *device,
> -				    unsigned int opt_flags, long timeout)
> +				    unsigned int opt_flags, long timeout,
> +				    struct firmware_dma_desc *dma)
>  {
>  	struct firmware_priv *fw_priv;
> 
> -	fw_priv = fw_create_instance(firmware, name, device, opt_flags);
> +	fw_priv = fw_create_instance(firmware, name, device, opt_flags, dma);
>  	if (IS_ERR(fw_priv))
>  		return PTR_ERR(fw_priv);
> 
> @@ -998,7 +1087,7 @@ static void kill_requests_without_uevent(void)
>  static inline int
>  fw_load_from_user_helper(struct firmware *firmware, const char *name,
>  			 struct device *device, unsigned int opt_flags,
> -			 long timeout)
> +			 long timeout, struct firmware_dma_desc *dma)
>  {
>  	return -ENOENT;
>  }
> @@ -1119,7 +1208,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
>  /* called from request_firmware() and request_firmware_work_func() */
>  static int
>  _request_firmware(const struct firmware **firmware_p, const char *name,
> -		  struct device *device, unsigned int opt_flags)
> +		  struct device *device, struct firmware_dma_desc *dma,
> +		  unsigned int opt_flags)
>  {
>  	struct firmware *fw = NULL;
>  	long timeout;
> @@ -1156,7 +1246,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  		}
>  	}
> 
> -	ret = fw_get_filesystem_firmware(device, fw->priv);
> +	ret = fw_get_filesystem_firmware(device, fw->priv, dma);
>  	if (ret) {
>  		if (!(opt_flags & FW_OPT_NO_WARN))
>  			dev_warn(device,
> @@ -1165,7 +1255,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  		if (opt_flags & FW_OPT_USERHELPER) {
>  			dev_warn(device, "Falling back to user helper\n");
>  			ret = fw_load_from_user_helper(fw, name, device,
> -						       opt_flags, timeout);
> +						       opt_flags, timeout,
> +						       dma);
>  		}
>  	}
> 
> @@ -1212,7 +1303,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
> 
>  	/* Need to pin this module until return */
>  	__module_get(THIS_MODULE);
> -	ret = _request_firmware(firmware_p, name, device,
> +	ret = _request_firmware(firmware_p, name, device, NULL,
>  				FW_OPT_UEVENT | FW_OPT_FALLBACK);
>  	module_put(THIS_MODULE);
>  	return ret;
> @@ -1236,7 +1327,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
>  	int ret;
> 
>  	__module_get(THIS_MODULE);
> -	ret = _request_firmware(firmware_p, name, device,
> +	ret = _request_firmware(firmware_p, name, device, NULL,
>  				FW_OPT_UEVENT | FW_OPT_NO_WARN);
>  	module_put(THIS_MODULE);
>  	return ret;
> @@ -1244,6 +1335,47 @@ int request_firmware_direct(const struct firmware **firmware_p,
>  EXPORT_SYMBOL_GPL(request_firmware_direct);
> 
>  /**
> + * request_firmware_dma - load firmware into a DMA allocated buffer
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded and DMA region allocated
> + * @cpu_addr: address returned from dma_alloc_*()
> + * @dma_addr: address returned from dma_alloc_*()
> + * @offset: offset into DMA buffer to copy firmware into
> + * @size: size of DMA buffer
> + * @attrs: attributes used during DMA allocation time
> + *
> + * This function works pretty much like request_firmware(), but it doesn't
> + * load the firmware into @firmware_p's data member. Instead, the firmware
> + * is loaded directly into the buffer pointed to by @cpu_addr and @dma_addr.
> + * This function doesn't cache firmware either.
> + */
> +int
> +request_firmware_dma(const struct firmware **firmware_p, const char *name,
> +		     struct device *device, void *cpu_addr, dma_addr_t dma_addr,
> +		     unsigned long offset, size_t size, struct dma_attrs *attrs)
> +{
> +	int ret;
> +	struct firmware_dma_desc dma = {
> +		.dev = device,
> +		.cpu_addr = cpu_addr,
> +		.dma_addr = dma_addr,
> +		.offset = offset,
> +		.size = size,
> +		.attrs = attrs,
> +	};
> +
> +	/* Need to pin this module until return */
> +	__module_get(THIS_MODULE);
> +	ret = _request_firmware(firmware_p, name, device, &dma,
> +				FW_OPT_UEVENT | FW_OPT_FALLBACK |
> +				FW_OPT_NOCACHE);
> +	module_put(THIS_MODULE);
> +	return ret;
> +}
> +EXPORT_SYMBOL(request_firmware_dma);
> +
> +/**
>   * release_firmware: - release the resource associated with a firmware image
>   * @fw: firmware resource to release
>   **/
> @@ -1275,7 +1407,7 @@ static void request_firmware_work_func(struct work_struct *work)
> 
>  	fw_work = container_of(work, struct firmware_work, work);
> 
> -	_request_firmware(&fw, fw_work->name, fw_work->device,
> +	_request_firmware(&fw, fw_work->name, fw_work->device, NULL,
>  			  fw_work->opt_flags);
>  	fw_work->cont(fw, fw_work->context);
>  	put_device(fw_work->device); /* taken in request_firmware_nowait() */
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5c41c5e75b5c..06bc8d5965ca 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -19,6 +19,7 @@ struct firmware {
> 
>  struct module;
>  struct device;
> +struct dma_attrs;
> 
>  struct builtin_fw {
>  	char *name;
> @@ -47,6 +48,10 @@ int request_firmware_nowait(
>  	void (*cont)(const struct firmware *fw, void *context));
>  int request_firmware_direct(const struct firmware **fw, const char *name,
>  			    struct device *device);
> +int request_firmware_dma(const struct firmware **firmware_p, const char *name,
> +		     struct device *device, void *cpu_addr, dma_addr_t dma_addr,
> +		     unsigned long offset, size_t size,
> +		     struct dma_attrs *attrs);
> 
>  void release_firmware(const struct firmware *fw);
>  #else
> @@ -75,5 +80,13 @@ static inline int request_firmware_direct(const struct firmware **fw,
>  	return -EINVAL;
>  }
> 
> +static inline int request_firmware_dma(const struct firmware **firmware_p,
> +	const char *name, struct device *device, void *cpu_addr,
> +	dma_addr_t dma_addr, unsigned long offset, size_t size,
> +	struct dma_attrs *attrs)
> +{
> +	return -EINVAL;
> +}
> +
>  #endif
>  #endif

Re: [RFC/PATCH 4/4] firmware: Support requesting firmware directly into DMA memory

[Mimi Zohar]

On Tue, 2016-04-12 at 10:27 -0700, Stephen Boyd wrote:
> Quoting Mimi Zohar (2016-03-08 03:58:41)
> > Hi Stephen,
> > 
> > [CC''ing linux-security-module]
> > 
> > On Tue, 2016-03-08 at 16:22 +0700, Stephen Boyd wrote:
> > > Some systems are memory constrained but they need to load very
> > > large firmwares. The firmware subsystem allows drivers to request
> > > this firmware be loaded from the filesystem, but this requires
> > > that the entire firmware be loaded into kernel memory first
> > > before it's provided to the driver. This can lead to a situation
> > > where we map the firmware twice, once to load the firmware into
> > > kernel memory and once to copy the firmware into the final
> > > resting place.
> > > 
> > > This design creates needless memory pressure and delays loading
> > > because we have to copy from kernel memory to somewhere else.
> > > Let's add a request_firmware_dma() API that allows drivers to
> > > request firmware be loaded directly into a DMA buffer that's been
> > > pre-allocated. This skips the intermediate step of allocating a
> > > buffer in kernel memory to hold the firmware image while it's
> > > read from the filesystem and copying it to another location. It
> > > also requires that drivers know how much memory they'll require
> > > before requesting the firmware and negates any benefits of
> > > firmware caching.
> > 
> > Prior to the kernel_read_file() family of functions, IMA/IMA-appraisal,
> > when enabled, pre-read the firmware a page at a time calculating the
> > file hash and then verified the file signature, before the firmware was
> > re-read into memory.   With the kernel_read_file() family of functions
> > the firmware is read into memory once, the signature verified, before
> > the driver is given access.
> > 
> > This patch set wants to read the file piecemeal, giving the driver
> > direct access to the pieces read, without first verifying the firmware
> > signature.  This breaks the concepts of measuring (trusted boot) or
> > appraising (secure boot) a file before usage.
> 
> I'm not sure I follow. This patch is allowing drivers to choose where
> the firmware is loaded. The assumption is that the device isn't actively
> running or using the firmware when it's being loaded into the DMA
> buffer. I hope that we can give the IMA/IMA-appraisal code a way to
> calculate the file hash a page at a time on the region of memory that
> the caller has chosen to use. Once the entire file is read into the DMA
> buffer, the signature can be verified and then the caller will know if
> the firmware is trusted or not.

In the original version of the patches (Dec 2015), IMA read the file a
buffer at a time to calculate the file hash.  Based on David Young's
comment, instead of calling IMA to read the file, the file was read
inline and then called IMA to calculate the hash based on the buffer
data.

> I suppose the driver could use the firmware data even if the
> request_firmware() call fails due to a bad IMA/IMA-appraisal, but is
> that important? Wouldn't it be a bug in the driver because it isn't
> checking the return value of request_firmware_dma()?

True, it would be a bug, but the main point of the patch set was to
close this class of measurement/appraisal gaps.

> >    
> > > This is based on a patch from Vikram Mulukutla on
> > > codeaurora.org[1].
> > > 
> > > [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=rel/msm-3.18&id=0a328c5f6cd999f5c591f172216835636f39bcb5
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Vikram Mulukutla <markivx@codeaurora.org>
> > > Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> > > ---
> > > 
> > > TODO:
> > >  * Handle security_kernel_fw_from_file() in the userhelper fallback case
> > >  * Rework for pending patches from Mimi Zohar that change the way files
> > >    are read in kernel space
> > > 
> > >  drivers/base/firmware_class.c | 256 ++++++++++++++++++++++++++++++++----------
> > >  include/linux/firmware.h      |  13 +++
> > >  2 files changed, 207 insertions(+), 62 deletions(-)
> > > 
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 9a616cf7ac9f..8ab7a418f1d7 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -29,6 +29,8 @@
> > >  #include <linux/syscore_ops.h>
> > >  #include <linux/reboot.h>
> > >  #include <linux/security.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/dma-mapping.h>
> > > 
> > >  #include <generated/utsrelease.h>
> > > 
> > > @@ -154,6 +156,15 @@ struct firmware_buf {
> > >       const char *fw_id;
> > >  };
> > > 
> > > +struct firmware_dma_desc {
> > > +     struct device *dev;
> > > +     void *cpu_addr;
> > > +     dma_addr_t dma_addr;
> > > +     unsigned long offset;
> > > +     size_t size;
> > > +     struct dma_attrs *attrs;
> > > +};
> > > +
> > >  struct fw_cache_entry {
> > >       struct list_head list;
> > >       const char *name;
> > > @@ -292,39 +303,83 @@ static const char * const fw_path[] = {
> > >  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
> > >  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
> > > 
> > > -static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> > > +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf,
> > > +                              struct firmware_dma_desc *dma)
> > >  {
> > > -     int size;
> > > +     int size, rsize, remaining;
> > >       char *buf;
> > >       int rc;
> > > +     loff_t offset;
> > > +     unsigned long dma_offset;
> > > 
> > >       if (!S_ISREG(file_inode(file)->i_mode))
> > >               return -EINVAL;
> > >       size = i_size_read(file_inode(file));
> > >       if (size <= 0)
> > >               return -EINVAL;
> > > -     buf = vmalloc(size);
> > > -     if (!buf)
> > > -             return -ENOMEM;
> > > -     rc = kernel_read(file, 0, buf, size);
> > > -     if (rc != size) {
> > > -             if (rc > 0)
> > > -                     rc = -EIO;
> > > -             goto fail;
> > > +
> > > +     if (dma) {
> > > +             if (size + dma->offset > dma->size)
> > > +                     return -EINVAL;
> > > +
> > > +             dma_offset = dma->offset;
> > > +             offset = 0;
> > > +             remaining = size;
> > > +
> > > +             while (remaining > 0) {
> > > +                     rsize = min_t(int, remaining, PAGE_SIZE);
> > > +
> > > +                     buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr,
> > > +                                     rsize, dma_offset, dma->attrs);
> > > +                     if (!buf)
> > > +                             return -ENOMEM;
> > > +
> > > +                     rc = kernel_read(file, offset, buf, rsize);
> > > +                     if (rc != rsize) {
> > > +                             if (rc > 0)
> > > +                                     rc = -EIO;
> > > +                             goto fail_dma;
> > > +                     }
> > > +                     rc = security_kernel_fw_from_file(file, buf, rsize); 
> > 
> > The security_kernel_fw_from_file hook was removed and replaced with the
> > pre and post hooks named security_kernel_read_file() hook and
> > security_kernel_post_read_file() respectively.  The pre security hook
> > gives the LSMs and IMA-appraisal opportunity to deny the read.   In
> > addition to giving the LSMs the opportunity to fail the firmware
> > request, the post security hook allows IMA/IMA-appraisal to measure the
> > file and appraise the file signature.
> > 
> 
> Thanks for the information. Would it be possible to move to appraising
> the file signature a page at a time in the case of DMA firmware loading?
> I see that this is all sort of moved away from the firmware code, so I'm
> trying to figure out how to make this work with the security checks now.

The signature is based on the hash of the entire file.

Mimi 

Re: [RFC/PATCH 4/4] firmware: Support requesting firmware directly into DMA memory

[Ming Lei]

On Tue, Mar 8, 2016 at 5:22 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote:
> Some systems are memory constrained but they need to load very
> large firmwares. The firmware subsystem allows drivers to request
> this firmware be loaded from the filesystem, but this requires
> that the entire firmware be loaded into kernel memory first
> before it's provided to the driver. This can lead to a situation
> where we map the firmware twice, once to load the firmware into
> kernel memory and once to copy the firmware into the final
> resting place.

The 2nd copy can be avoided and it should be OK to feed fw->pages
to DMA since you can get the pages via 'struct firmware *'.
We still can replace the vmalloc() in fw_read_file_contents() with
N*alloc_page() plus vmap() in case of direct loading.

>
> This design creates needless memory pressure and delays loading
> because we have to copy from kernel memory to somewhere else.

Given firmware request can't be a frequent operation, I don't think it is
a big deal about the so called memory pressure and delay.

> Let's add a request_firmware_dma() API that allows drivers to
> request firmware be loaded directly into a DMA buffer that's been
> pre-allocated. This skips the intermediate step of allocating a
> buffer in kernel memory to hold the firmware image while it's
> read from the filesystem and copying it to another location. It
> also requires that drivers know how much memory they'll require
> before requesting the firmware and negates any benefits of
> firmware caching.
>
> This is based on a patch from Vikram Mulukutla on
> codeaurora.org[1].
>
> [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=rel/msm-3.18&id=0a328c5f6cd999f5c591f172216835636f39bcb5
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Vikram Mulukutla <markivx@codeaurora.org>
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>
> TODO:
>  * Handle security_kernel_fw_from_file() in the userhelper fallback case
>  * Rework for pending patches from Mimi Zohar that change the way files
>    are read in kernel space
>
>  drivers/base/firmware_class.c | 256 ++++++++++++++++++++++++++++++++----------
>  include/linux/firmware.h      |  13 +++
>  2 files changed, 207 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 9a616cf7ac9f..8ab7a418f1d7 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -29,6 +29,8 @@
>  #include <linux/syscore_ops.h>
>  #include <linux/reboot.h>
>  #include <linux/security.h>
> +#include <linux/kernel.h>
> +#include <linux/dma-mapping.h>
>
>  #include <generated/utsrelease.h>
>
> @@ -154,6 +156,15 @@ struct firmware_buf {
>         const char *fw_id;
>  };
>
> +struct firmware_dma_desc {
> +       struct device *dev;
> +       void *cpu_addr;
> +       dma_addr_t dma_addr;
> +       unsigned long offset;
> +       size_t size;
> +       struct dma_attrs *attrs;
> +};
> +
>  struct fw_cache_entry {
>         struct list_head list;
>         const char *name;
> @@ -292,39 +303,83 @@ static const char * const fw_path[] = {
>  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
>
> -static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf,
> +                                struct firmware_dma_desc *dma)
>  {
> -       int size;
> +       int size, rsize, remaining;
>         char *buf;
>         int rc;
> +       loff_t offset;
> +       unsigned long dma_offset;
>
>         if (!S_ISREG(file_inode(file)->i_mode))
>                 return -EINVAL;
>         size = i_size_read(file_inode(file));
>         if (size <= 0)
>                 return -EINVAL;
> -       buf = vmalloc(size);
> -       if (!buf)
> -               return -ENOMEM;
> -       rc = kernel_read(file, 0, buf, size);
> -       if (rc != size) {
> -               if (rc > 0)
> -                       rc = -EIO;
> -               goto fail;
> +
> +       if (dma) {
> +               if (size + dma->offset > dma->size)
> +                       return -EINVAL;
> +
> +               dma_offset = dma->offset;
> +               offset = 0;
> +               remaining = size;
> +
> +               while (remaining > 0) {
> +                       rsize = min_t(int, remaining, PAGE_SIZE);
> +
> +                       buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr,
> +                                       rsize, dma_offset, dma->attrs);
> +                       if (!buf)
> +                               return -ENOMEM;
> +
> +                       rc = kernel_read(file, offset, buf, rsize);
> +                       if (rc != rsize) {
> +                               if (rc > 0)
> +                                       rc = -EIO;
> +                               goto fail_dma;
> +                       }
> +                       rc = security_kernel_fw_from_file(file, buf, rsize);
> +                       if (rc)
> +                               goto fail_dma;
> +
> +                       dma_unremap(dma->dev, buf, rsize, dma_offset,
> +                                   dma->attrs);
> +                       dma_offset += rsize;
> +                       offset += rsize;
> +                       remaining -= rsize;
> +               }
> +       } else {
> +               buf = vmalloc(size);
> +               if (!buf)
> +                       return -ENOMEM;
> +               rc = kernel_read(file, 0, buf, size);
> +               if (rc != size) {
> +                       if (rc > 0)
> +                               rc = -EIO;
> +                       goto fail;
> +               }
> +               rc = security_kernel_fw_from_file(file, buf, size);
> +               if (rc)
> +                       goto fail;
> +
> +               fw_buf->data = buf;
>         }
> -       rc = security_kernel_fw_from_file(file, buf, size);
> -       if (rc)
> -               goto fail;
> -       fw_buf->data = buf;
> +
>         fw_buf->size = size;
>         return 0;
>  fail:
>         vfree(buf);
>         return rc;
> +fail_dma:
> +       dma_unremap(dma->dev, buf, rsize, dma_offset, dma->attrs);
> +       return rc;
>  }
>
> -static int fw_get_filesystem_firmware(struct device *device,
> -                                      struct firmware_buf *buf)
> +static int
> +fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf,
> +                          struct firmware_dma_desc *dma)
>  {
>         int i, len;
>         int rc = -ENOENT;
> @@ -351,7 +406,7 @@ static int fw_get_filesystem_firmware(struct device *device,
>                 file = filp_open(path, O_RDONLY, 0);
>                 if (IS_ERR(file))
>                         continue;
> -               rc = fw_read_file_contents(file, buf);
> +               rc = fw_read_file_contents(file, buf, dma);
>                 fput(file);
>                 if (rc)
>                         dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
> @@ -470,6 +525,7 @@ struct firmware_priv {
>         struct device dev;
>         struct firmware_buf *buf;
>         struct firmware *fw;
> +       struct firmware_dma_desc *dma;
>  };
>
>  static struct firmware_priv *to_firmware_priv(struct device *dev)
> @@ -716,6 +772,52 @@ out:
>
>  static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
>
> +static ssize_t firmware_dma_rw(struct firmware_dma_desc *dma, char *buffer,
> +                              loff_t *offset, size_t count, bool read)
> +{
> +       char *fw_buf;
> +       int retval = count;
> +
> +       if ((dma->offset + *offset + count) > dma->size)
> +               return -EINVAL;
> +
> +       fw_buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr, count,
> +                          dma->offset + *offset, dma->attrs);
> +       if (!fw_buf)
> +               return -ENOMEM;
> +
> +       if (read)
> +               memcpy(buffer, fw_buf, count);
> +       else
> +               memcpy(fw_buf, buffer, count);
> +
> +       dma_unremap(dma->dev, fw_buf, count, dma->offset + *offset, dma->attrs);
> +       *offset += count;
> +
> +       return retval;
> +}
> +
> +static void firmware_rw(struct firmware_buf *buf, char *buffer,
> +                       loff_t offset, size_t count, bool read)
> +{
> +       while (count) {
> +               void *page_data;
> +               int page_nr = offset >> PAGE_SHIFT;
> +               int page_ofs = offset & (PAGE_SIZE-1);
> +               int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> +
> +               page_data = kmap(buf->pages[page_nr]);
> +
> +               memcpy(buffer, page_data + page_ofs, page_cnt);
> +
> +               kunmap(buf->pages[page_nr]);
> +               buffer += page_cnt;
> +               offset += page_cnt;
> +               count -= page_cnt;
> +       }
> +
> +}
> +
>  static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>                                   struct bin_attribute *bin_attr,
>                                   char *buffer, loff_t offset, size_t count)
> @@ -740,21 +842,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>
>         ret_count = count;
>
> -       while (count) {
> -               void *page_data;
> -               int page_nr = offset >> PAGE_SHIFT;
> -               int page_ofs = offset & (PAGE_SIZE-1);
> -               int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> -
> -               page_data = kmap(buf->pages[page_nr]);
> +       if (fw_priv->dma)
> +               ret_count = firmware_dma_rw(fw_priv->dma, buffer, &offset,
> +                                           count, true);
> +       else
> +               firmware_rw(buf, buffer, offset, count, true);
>
> -               memcpy(buffer, page_data + page_ofs, page_cnt);
> -
> -               kunmap(buf->pages[page_nr]);
> -               buffer += page_cnt;
> -               offset += page_cnt;
> -               count -= page_cnt;
> -       }
>  out:
>         mutex_unlock(&fw_lock);
>         return ret_count;
> @@ -817,6 +910,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  {
>         struct device *dev = kobj_to_dev(kobj);
>         struct firmware_priv *fw_priv = to_firmware_priv(dev);
> +       struct firmware_dma_desc *dma = fw_priv->dma;
>         struct firmware_buf *buf;
>         ssize_t retval;
>
> @@ -830,26 +924,17 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>                 goto out;
>         }
>
> -       retval = fw_realloc_buffer(fw_priv, offset + count);
> -       if (retval)
> -               goto out;
> -
> -       retval = count;
> -
> -       while (count) {
> -               void *page_data;
> -               int page_nr = offset >> PAGE_SHIFT;
> -               int page_ofs = offset & (PAGE_SIZE - 1);
> -               int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> -
> -               page_data = kmap(buf->pages[page_nr]);
> -
> -               memcpy(page_data + page_ofs, buffer, page_cnt);
> +       if (dma) {
> +               retval = firmware_dma_rw(dma, buffer, &offset, count, false);
> +               if (retval < 0)
> +                       goto out;
> +       } else {
> +               retval = fw_realloc_buffer(fw_priv, offset + count);
> +               if (retval)
> +                       goto out;
>
> -               kunmap(buf->pages[page_nr]);
> -               buffer += page_cnt;
> -               offset += page_cnt;
> -               count -= page_cnt;
> +               retval = count;
> +               firmware_rw(buf, buffer, offset, count, false);
>         }
>
>         buf->size = max_t(size_t, offset, buf->size);
> @@ -887,7 +972,8 @@ static const struct attribute_group *fw_dev_attr_groups[] = {
>
>  static struct firmware_priv *
>  fw_create_instance(struct firmware *firmware, const char *fw_name,
> -                  struct device *device, unsigned int opt_flags)
> +                  struct device *device, unsigned int opt_flags,
> +                  struct firmware_dma_desc *dma)
>  {
>         struct firmware_priv *fw_priv;
>         struct device *f_dev;
> @@ -900,6 +986,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
>
>         fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
>         fw_priv->fw = firmware;
> +       fw_priv->dma = dma;
>         f_dev = &fw_priv->dev;
>
>         device_initialize(f_dev);
> @@ -920,7 +1007,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>         struct firmware_buf *buf = fw_priv->buf;
>
>         /* fall back on userspace loading */
> -       buf->is_paged_buf = true;
> +       if (!fw_priv->dma)
> +               buf->is_paged_buf = true;
>
>         dev_set_uevent_suppress(f_dev, true);
>
> @@ -955,7 +1043,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>
>         if (is_fw_load_aborted(buf))
>                 retval = -EAGAIN;
> -       else if (!buf->data)
> +       else if (buf->is_paged_buf && !buf->data)
>                 retval = -ENOMEM;
>
>         device_del(f_dev);
> @@ -966,11 +1054,12 @@ err_put_dev:
>
>  static int fw_load_from_user_helper(struct firmware *firmware,
>                                     const char *name, struct device *device,
> -                                   unsigned int opt_flags, long timeout)
> +                                   unsigned int opt_flags, long timeout,
> +                                   struct firmware_dma_desc *dma)
>  {
>         struct firmware_priv *fw_priv;
>
> -       fw_priv = fw_create_instance(firmware, name, device, opt_flags);
> +       fw_priv = fw_create_instance(firmware, name, device, opt_flags, dma);
>         if (IS_ERR(fw_priv))
>                 return PTR_ERR(fw_priv);
>
> @@ -998,7 +1087,7 @@ static void kill_requests_without_uevent(void)
>  static inline int
>  fw_load_from_user_helper(struct firmware *firmware, const char *name,
>                          struct device *device, unsigned int opt_flags,
> -                        long timeout)
> +                        long timeout, struct firmware_dma_desc *dma)
>  {
>         return -ENOENT;
>  }
> @@ -1119,7 +1208,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
>  /* called from request_firmware() and request_firmware_work_func() */
>  static int
>  _request_firmware(const struct firmware **firmware_p, const char *name,
> -                 struct device *device, unsigned int opt_flags)
> +                 struct device *device, struct firmware_dma_desc *dma,
> +                 unsigned int opt_flags)
>  {
>         struct firmware *fw = NULL;
>         long timeout;
> @@ -1156,7 +1246,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>                 }
>         }
>
> -       ret = fw_get_filesystem_firmware(device, fw->priv);
> +       ret = fw_get_filesystem_firmware(device, fw->priv, dma);
>         if (ret) {
>                 if (!(opt_flags & FW_OPT_NO_WARN))
>                         dev_warn(device,
> @@ -1165,7 +1255,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>                 if (opt_flags & FW_OPT_USERHELPER) {
>                         dev_warn(device, "Falling back to user helper\n");
>                         ret = fw_load_from_user_helper(fw, name, device,
> -                                                      opt_flags, timeout);
> +                                                      opt_flags, timeout,
> +                                                      dma);
>                 }
>         }
>
> @@ -1212,7 +1303,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
>
>         /* Need to pin this module until return */
>         __module_get(THIS_MODULE);
> -       ret = _request_firmware(firmware_p, name, device,
> +       ret = _request_firmware(firmware_p, name, device, NULL,
>                                 FW_OPT_UEVENT | FW_OPT_FALLBACK);
>         module_put(THIS_MODULE);
>         return ret;
> @@ -1236,7 +1327,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
>         int ret;
>
>         __module_get(THIS_MODULE);
> -       ret = _request_firmware(firmware_p, name, device,
> +       ret = _request_firmware(firmware_p, name, device, NULL,
>                                 FW_OPT_UEVENT | FW_OPT_NO_WARN);
>         module_put(THIS_MODULE);
>         return ret;
> @@ -1244,6 +1335,47 @@ int request_firmware_direct(const struct firmware **firmware_p,
>  EXPORT_SYMBOL_GPL(request_firmware_direct);
>
>  /**
> + * request_firmware_dma - load firmware into a DMA allocated buffer
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded and DMA region allocated
> + * @cpu_addr: address returned from dma_alloc_*()
> + * @dma_addr: address returned from dma_alloc_*()
> + * @offset: offset into DMA buffer to copy firmware into
> + * @size: size of DMA buffer
> + * @attrs: attributes used during DMA allocation time
> + *
> + * This function works pretty much like request_firmware(), but it doesn't
> + * load the firmware into @firmware_p's data member. Instead, the firmware
> + * is loaded directly into the buffer pointed to by @cpu_addr and @dma_addr.
> + * This function doesn't cache firmware either.
> + */
> +int
> +request_firmware_dma(const struct firmware **firmware_p, const char *name,
> +                    struct device *device, void *cpu_addr, dma_addr_t dma_addr,
> +                    unsigned long offset, size_t size, struct dma_attrs *attrs)
> +{
> +       int ret;
> +       struct firmware_dma_desc dma = {
> +               .dev = device,
> +               .cpu_addr = cpu_addr,
> +               .dma_addr = dma_addr,
> +               .offset = offset,
> +               .size = size,
> +               .attrs = attrs,
> +       };
> +
> +       /* Need to pin this module until return */
> +       __module_get(THIS_MODULE);
> +       ret = _request_firmware(firmware_p, name, device, &dma,
> +                               FW_OPT_UEVENT | FW_OPT_FALLBACK |
> +                               FW_OPT_NOCACHE);
> +       module_put(THIS_MODULE);
> +       return ret;
> +}
> +EXPORT_SYMBOL(request_firmware_dma);
> +
> +/**
>   * release_firmware: - release the resource associated with a firmware image
>   * @fw: firmware resource to release
>   **/
> @@ -1275,7 +1407,7 @@ static void request_firmware_work_func(struct work_struct *work)
>
>         fw_work = container_of(work, struct firmware_work, work);
>
> -       _request_firmware(&fw, fw_work->name, fw_work->device,
> +       _request_firmware(&fw, fw_work->name, fw_work->device, NULL,
>                           fw_work->opt_flags);
>         fw_work->cont(fw, fw_work->context);
>         put_device(fw_work->device); /* taken in request_firmware_nowait() */
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5c41c5e75b5c..06bc8d5965ca 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -19,6 +19,7 @@ struct firmware {
>
>  struct module;
>  struct device;
> +struct dma_attrs;
>
>  struct builtin_fw {
>         char *name;
> @@ -47,6 +48,10 @@ int request_firmware_nowait(
>         void (*cont)(const struct firmware *fw, void *context));
>  int request_firmware_direct(const struct firmware **fw, const char *name,
>                             struct device *device);
> +int request_firmware_dma(const struct firmware **firmware_p, const char *name,
> +                    struct device *device, void *cpu_addr, dma_addr_t dma_addr,
> +                    unsigned long offset, size_t size,
> +                    struct dma_attrs *attrs);
>
>  void release_firmware(const struct firmware *fw);
>  #else
> @@ -75,5 +80,13 @@ static inline int request_firmware_direct(const struct firmware **fw,
>         return -EINVAL;
>  }
>
> +static inline int request_firmware_dma(const struct firmware **firmware_p,
> +       const char *name, struct device *device, void *cpu_addr,
> +       dma_addr_t dma_addr, unsigned long offset, size_t size,
> +       struct dma_attrs *attrs)
> +{
> +       return -EINVAL;
> +}
> +
>  #endif
>  #endif
> --
> 2.7.0.25.gfc10eb5
>



-- 
Ming Lei

Re: [RFC/PATCH 4/4] firmware: Support requesting firmware directly into DMA memory

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

On Tue, Mar 08, 2016 at 09:42:17PM +0800, Ming Lei wrote:
> On Tue, Mar 8, 2016 at 5:22 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote:

> > This design creates needless memory pressure and delays loading
> > because we have to copy from kernel memory to somewhere else.

> Given firmware request can't be a frequent operation, I don't think it is
> a big deal about the so called memory pressure and delay.

Boot time is a very important metric for some embedded product classes
and is obviously one of the most common times when we need to load lots
of firmware.  Some mobile platforms have some individual firmwares that
are getting on for 100M in size, holding a copy of one of them in RAM is
a substantial proportion of the available memory in a system with 1-2G
total especially when the system is also busy with other things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC/PATCH 0/4] request_firmware() on memory constrained devices

[Alexander Stein]

On Tuesday 08 March 2016 16:22:15, Stephen Boyd wrote:
> Some systems are memory constrained but they need to load very
> large firmwares. 

Out of curiousity, about which sizes of memory and firmware are you talking about?

Regards,
Alexander

Re: [RFC/PATCH 0/4] request_firmware() on memory constrained devices

[Stephen Boyd]

Quoting Alexander Stein (2016-03-08 16:32:21)
> On Tuesday 08 March 2016 16:22:15, Stephen Boyd wrote:
> > Some systems are memory constrained but they need to load very
> > large firmwares. 
> 
> Out of curiousity, about which sizes of memory and firmware are you talking about?
> 

Hm.. I've seen an extreme case where there's around 100MB of a 128MB DDR
part dedicated to non-Linux firmware. So attempting to load that
firmware into DDR just doesn't work at all unless you have this patch
series. This is the most extreme case I've seen though.

Please also note that this is also about skipping the memcpy() step,
which I should probably quantify how much that actually matters if at
all. I'll make sure to do some loading time measurements in the next
round.