I'm sending this again to solicit feedback on if this is even the right
approach. After Mimi's patches that change where firmware loading code
is done, I've had to modify fs/exec.c and add a struct to linux/fs.h,
and that feels wrong. If that is OK, then my only other concern is
doing the security checks a page at at time vs. all at once on the
whole buffer. If there isn't any opposition to doing that I'll start
working on the necessary changes.
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.6-rc1.
Changes since v1:
* Rebased onto v4.6-rc1 (large conflicts due to movement of code from Mimi)
* Added some CONFIG_HAS_DMA ifdefs around code that's using DMA ops
TODO:
* Performance metrics for DMA vs. non-DMA based loading
* Test on tiny memory parts with big firmwares
* Integrate/test with IMA/security checks
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 | 192 +++++++++++++++++++++++++++++-----------
fs/exec.c | 95 +++++++++++++++-----
include/linux/dma-mapping.h | 35 ++++++++
include/linux/firmware.h | 13 +++
include/linux/fs.h | 14 ++-
security/integrity/ima/ima_fs.c | 3 +-
7 files changed, 347 insertions(+), 83 deletions(-)
--
2.8.0.rc4
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 <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
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 a6e757cbab77..9686e722a047 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.8.0.rc4
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: Mimi Zohar <[email protected]>
Cc: Vikram Mulukutla <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/base/firmware_class.c | 185 +++++++++++++++++++++++++++++-----------
fs/exec.c | 95 ++++++++++++++++-----
include/linux/firmware.h | 13 +++
include/linux/fs.h | 14 ++-
security/integrity/ima/ima_fs.c | 3 +-
5 files changed, 238 insertions(+), 72 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 5e6ca3e1c77a..eb2c88214c27 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -30,6 +30,9 @@
#include <linux/syscore_ops.h>
#include <linux/reboot.h>
#include <linux/security.h>
+#include <linux/kernel.h>
+#include <linux/dma-mapping.h>
+#include <linux/fs.h>
#include <generated/utsrelease.h>
@@ -302,8 +305,9 @@ static void fw_finish_direct_load(struct device *device,
mutex_unlock(&fw_lock);
}
-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 file_dma *dma)
{
loff_t size;
int i, len;
@@ -328,7 +332,7 @@ static int fw_get_filesystem_firmware(struct device *device,
buf->size = 0;
rc = kernel_read_file_from_path(path, &buf->data, &size,
- INT_MAX, READING_FIRMWARE);
+ INT_MAX, READING_FIRMWARE, dma);
if (rc) {
if (rc == -ENOENT)
dev_dbg(device, "loading %s failed with error %d\n",
@@ -445,6 +449,7 @@ struct firmware_priv {
struct device dev;
struct firmware_buf *buf;
struct firmware *fw;
+ struct file_dma *dma;
};
static struct firmware_priv *to_firmware_priv(struct device *dev)
@@ -692,6 +697,60 @@ out:
static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
+#ifdef CONFIG_HAS_DMA
+static ssize_t firmware_dma_rw(struct file_dma *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;
+}
+#else
+static ssize_t firmware_dma_rw(struct file_dma *dma, char *buffer,
+ loff_t *offset, size_t count, bool read)
+{
+ return -EINVAL;
+}
+#endif
+
+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)
@@ -716,21 +775,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]);
-
- memcpy(buffer, page_data + page_ofs, page_cnt);
+ if (fw_priv->dma)
+ ret_count = firmware_dma_rw(fw_priv->dma, buffer, &offset,
+ count, true);
+ else
+ firmware_rw(buf, buffer, offset, count, true);
- kunmap(buf->pages[page_nr]);
- buffer += page_cnt;
- offset += page_cnt;
- count -= page_cnt;
- }
out:
mutex_unlock(&fw_lock);
return ret_count;
@@ -792,6 +842,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 file_dma *dma = fw_priv->dma;
struct firmware_buf *buf;
ssize_t retval;
@@ -805,26 +856,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);
@@ -862,7 +904,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 file_dma *dma)
{
struct firmware_priv *fw_priv;
struct device *f_dev;
@@ -875,6 +918,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);
@@ -895,7 +939,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);
@@ -930,7 +975,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);
@@ -941,11 +986,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 file_dma *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);
@@ -973,7 +1019,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 file_dma *dma)
{
return -ENOENT;
}
@@ -1094,7 +1140,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 file_dma *dma,
+ unsigned int opt_flags)
{
struct firmware *fw = NULL;
long timeout;
@@ -1131,7 +1178,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,
@@ -1140,7 +1187,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);
}
}
@@ -1187,7 +1235,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;
@@ -1211,7 +1259,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;
@@ -1219,6 +1267,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 file_dma 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
**/
@@ -1250,7 +1339,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/fs/exec.c b/fs/exec.c
index c4010b8207a1..b96ceb5facff 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -57,6 +57,7 @@
#include <linux/oom.h>
#include <linux/compat.h>
#include <linux/vmalloc.h>
+#include <linux/dma-mapping.h>
#include <asm/uaccess.h>
#include <asm/mmu_context.h>
@@ -836,12 +837,15 @@ int kernel_read(struct file *file, loff_t offset,
EXPORT_SYMBOL(kernel_read);
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
+static int _kernel_read_file(struct file *file, void **buf, loff_t *size,
+ loff_t max_size, enum kernel_read_file_id id,
+ struct file_dma *dma)
{
- loff_t i_size, pos;
+ loff_t i_size, r_size = 0, pos, remaining;
ssize_t bytes = 0;
int ret;
+ unsigned long dma_pos = 0;
+ char *dbuf;
if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
return -EINVAL;
@@ -856,33 +860,73 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
if (i_size <= 0)
return -EINVAL;
- *buf = vmalloc(i_size);
- if (!*buf)
- return -ENOMEM;
+ if (dma) {
+#ifdef CONFIG_HAS_DMA
+ if (i_size + dma->offset > dma->size)
+ return -EINVAL;
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, pos, (char *)(*buf) + pos,
- i_size - pos);
- if (bytes < 0) {
- ret = bytes;
- goto out;
+ dma_pos = dma->offset;
+ pos = 0;
+ remaining = i_size;
+
+ while (remaining > 0) {
+ r_size = min_t(int, remaining, PAGE_SIZE);
+
+ dbuf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr,
+ r_size, dma_pos, dma->attrs);
+ if (!dbuf)
+ return -ENOMEM;
+
+ ret = kernel_read(file, pos, dbuf, r_size);
+ if (ret != r_size) {
+ if (ret > 0)
+ ret = -EIO;
+ goto fail_dma;
+ }
+
+ dma_unremap(dma->dev, dbuf, r_size, dma_pos, dma->attrs);
+ dma_pos += r_size;
+ pos += r_size;
+ remaining -= r_size;
}
+#else
+ return -EINVAL;
+#endif
+ } else {
- if (bytes == 0)
- break;
- pos += bytes;
- }
+ *buf = vmalloc(i_size);
+ if (!*buf)
+ return -ENOMEM;
- if (pos != i_size) {
- ret = -EIO;
- goto out;
+ pos = 0;
+ while (pos < i_size) {
+ bytes = kernel_read(file, pos, (char *)(*buf) + pos,
+ i_size - pos);
+ if (bytes < 0) {
+ ret = bytes;
+ goto out;
+ }
+
+ if (bytes == 0)
+ break;
+ pos += bytes;
+ }
+
+ if (pos != i_size) {
+ ret = -EIO;
+ goto out;
+ }
}
ret = security_kernel_post_read_file(file, *buf, i_size, id);
if (!ret)
*size = pos;
+#ifdef CONFIG_HAS_DMA
+fail_dma:
+ dma_unremap(dma->dev, buf, r_size, dma_pos, dma->attrs);
+ return ret;
+#endif
out:
if (ret < 0) {
vfree(*buf);
@@ -890,10 +934,17 @@ out:
}
return ret;
}
+
+int kernel_read_file(struct file *file, void **buf, loff_t *size,
+ loff_t max_size, enum kernel_read_file_id id)
+{
+ return _kernel_read_file(file, buf, size, max_size, id, NULL);
+}
EXPORT_SYMBOL_GPL(kernel_read_file);
int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
+ loff_t max_size, enum kernel_read_file_id id,
+ struct file_dma *dma)
{
struct file *file;
int ret;
@@ -905,7 +956,7 @@ int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = _kernel_read_file(file, buf, size, max_size, id, dma);
fput(file);
return ret;
}
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
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 14a97194b34b..d598944e0700 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -55,6 +55,8 @@ struct workqueue_struct;
struct iov_iter;
struct fscrypt_info;
struct fscrypt_operations;
+struct device;
+struct dma_attrs;
extern void __init inode_init(void);
extern void __init inode_init_early(void);
@@ -2589,11 +2591,21 @@ enum kernel_read_file_id {
READING_MAX_ID
};
+struct file_dma {
+ struct device *dev;
+ void *cpu_addr;
+ dma_addr_t dma_addr;
+ unsigned long offset;
+ size_t size;
+ struct dma_attrs *attrs;
+};
+
extern int kernel_read(struct file *, loff_t, char *, unsigned long);
extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
enum kernel_read_file_id);
extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
+ enum kernel_read_file_id,
+ struct file_dma *dma);
extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
enum kernel_read_file_id);
extern ssize_t kernel_write(struct file *, const char *, size_t, loff_t);
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 60d011aaec38..b922d6ca2fb0 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -272,7 +272,8 @@ static ssize_t ima_read_policy(char *path)
datap = path;
strsep(&datap, "\n");
- rc = kernel_read_file_from_path(path, &data, &size, 0, READING_POLICY);
+ rc = kernel_read_file_from_path(path, &data, &size, 0, READING_POLICY,
+ NULL);
if (rc < 0) {
pr_err("Unable to open file: %s (%d)", path, rc);
return rc;
--
2.8.0.rc4
From: Laura Abbott <[email protected]>
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.
Signed-off-by: Laura Abbott <[email protected]>
[[email protected]: 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 <[email protected]>
---
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 9686e722a047..c5816cfc155f 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 9ea9aba28049..737c38a6151d 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
@@ -467,6 +473,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.8.0.rc4
From: Vikram Mulukutla <[email protected]>
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.
Signed-off-by: Vikram Mulukutla <[email protected]>
[[email protected]: Drop firmware_desc design and use flags]
Signed-off-by: Stephen Boyd <[email protected]>
---
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 773fc3099769..5e6ca3e1c77a 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -112,6 +112,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 */
@@ -1070,14 +1071,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.8.0.rc4
Hi Stefan,
On Tue, 2016-04-19 at 18:04 -0700, Stephen Boyd wrote:
> I'm sending this again to solicit feedback on if this is even the right
> approach. After Mimi's patches that change where firmware loading code
> is done, I've had to modify fs/exec.c and add a struct to linux/fs.h,
> and that feels wrong. If that is OK, then my only other concern is
> doing the security checks a page at at time vs. all at once on the
> whole buffer. If there isn't any opposition to doing that I'll start
> working on the necessary changes.
Reading the file into memory, and then using it to calculate the file
hash, was an optimization to read the file only once. All other hooks,
pre-read the file a buffer at a time, calculating the file hash. If
you're ok with this pre-reading, you could define a new hook named
READING_FIRMWARE_DMA, or something similar. The hash could be
calculated on the pre read hook (security_kernel_read_file), not on the
post read hook (security_kernel_post_read_file). Validating the
firmware signature on the pre-read hook, would eliminate the possibility
of giving the driver unverified firmware.
Mimi
> 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.6-rc1.
>
> Changes since v1:
> * Rebased onto v4.6-rc1 (large conflicts due to movement of code from Mimi)
> * Added some CONFIG_HAS_DMA ifdefs around code that's using DMA ops
>
> TODO:
> * Performance metrics for DMA vs. non-DMA based loading
> * Test on tiny memory parts with big firmwares
> * Integrate/test with IMA/security checks
>
> 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 | 192 +++++++++++++++++++++++++++++-----------
> fs/exec.c | 95 +++++++++++++++-----
> include/linux/dma-mapping.h | 35 ++++++++
> include/linux/firmware.h | 13 +++
> include/linux/fs.h | 14 ++-
> security/integrity/ima/ima_fs.c | 3 +-
> 7 files changed, 347 insertions(+), 83 deletions(-)
>
Hi Stephen,
On Tue, Apr 19, 2016 at 06:04:27PM -0700, Stephen Boyd wrote:
> From: Laura Abbott <[email protected]>
>
> 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.
The first two patches in this series don't make sense to me. I don't
understand what the memory pressure is: physical or virtual? Because
they don't seem to address the former (the DMA buffer is allocated in
full) while the latter doesn't need any addressing at all on arm64, we
have plenty of VA space.
Why do you even need the coherent DMA API? Can you use the streaming API
(map_sg etc.) with a separately allocated buffer?
--
Catalin
On Fri, Apr 22, 2016 at 05:35:16PM -0700, Stephen Boyd wrote:
> Quoting Catalin Marinas (2016-04-21 03:35:12)
> > On Tue, Apr 19, 2016 at 06:04:27PM -0700, Stephen Boyd wrote:
> > > From: Laura Abbott <[email protected]>
> > >
> > > 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.
> >
> > The first two patches in this series don't make sense to me. I don't
> > understand what the memory pressure is: physical or virtual? Because
> > they don't seem to address the former (the DMA buffer is allocated in
> > full) while the latter doesn't need any addressing at all on arm64, we
> > have plenty of VA space.
> >
> > Why do you even need the coherent DMA API? Can you use the streaming API
> > (map_sg etc.) with a separately allocated buffer?
>
> Hmm I guess I need to add in the patches that show how this is used on
> top of "no-map" DT reserved memory regions. There are some more patches
> that allow us to assigned reserved memory regions with the "no-map"
> attribute to devices and then allocate from those regions using the
> coherent DMA APIs. In the downstream kernel it's called a removed dma
> pool[1].
>
> So the plan is to wire that all up so that the device can have a
> reserved chunk of memory for the firmware that doesn't exist in the
> kernel's linear memory mappings. Once we have allocated the region, we
> can map it into the kernel's view of memory for a short time so that we
> can load the firmware into it (dma_remap part). Once that's over, we
> want to destroy the mapping so that we 1) don't use any of the kernel's
> virtual memory space (dma_unremap part) to back the buffer and 2) so
> that the secure world can protect the memory from the non-secure world.
Does the firmware already know about such memory? If yes, I presume the
kernel would have to be told about it and won't try to map it in the
linear mapping.
At this point, wouldn't a combination of:
dma_declare_coherent_memory()
dma_alloc_from_coherent()
dma_release_from_coherent()
dma_release_declared_memory()
work? The removed_alloc() implementation in the link you posted doesn't
seem far from dma_alloc_from_coherent(). The releasing of the declared
memory above would unmap the memory, so there are no VA mappings left.
--
Catalin
On 04/27/2016 08:25 AM, Catalin Marinas wrote:
> On Fri, Apr 22, 2016 at 05:35:16PM -0700, Stephen Boyd wrote:
>> Quoting Catalin Marinas (2016-04-21 03:35:12)
>>> On Tue, Apr 19, 2016 at 06:04:27PM -0700, Stephen Boyd wrote:
>>>> From: Laura Abbott <[email protected]>
>>>>
>>>> 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.
>>>
>>> The first two patches in this series don't make sense to me. I don't
>>> understand what the memory pressure is: physical or virtual? Because
>>> they don't seem to address the former (the DMA buffer is allocated in
>>> full) while the latter doesn't need any addressing at all on arm64, we
>>> have plenty of VA space.
>>>
>>> Why do you even need the coherent DMA API? Can you use the streaming API
>>> (map_sg etc.) with a separately allocated buffer?
>>
>> Hmm I guess I need to add in the patches that show how this is used on
>> top of "no-map" DT reserved memory regions. There are some more patches
>> that allow us to assigned reserved memory regions with the "no-map"
>> attribute to devices and then allocate from those regions using the
>> coherent DMA APIs. In the downstream kernel it's called a removed dma
>> pool[1].
>>
>> So the plan is to wire that all up so that the device can have a
>> reserved chunk of memory for the firmware that doesn't exist in the
>> kernel's linear memory mappings. Once we have allocated the region, we
>> can map it into the kernel's view of memory for a short time so that we
>> can load the firmware into it (dma_remap part). Once that's over, we
>> want to destroy the mapping so that we 1) don't use any of the kernel's
>> virtual memory space (dma_unremap part) to back the buffer and 2) so
>> that the secure world can protect the memory from the non-secure world.
>
> Does the firmware already know about such memory? If yes, I presume the
> kernel would have to be told about it and won't try to map it in the
> linear mapping.
>
> At this point, wouldn't a combination of:
>
> dma_declare_coherent_memory()
> dma_alloc_from_coherent()
> dma_release_from_coherent()
> dma_release_declared_memory()
>
> work? The removed_alloc() implementation in the link you posted doesn't
> seem far from dma_alloc_from_coherent(). The releasing of the declared
> memory above would unmap the memory, so there are no VA mappings left.
>
The removed alloc was specifically written as a fork of the coherent
pool. This was a choice for ease of out of tree maintenance. The better
choice here would be to fold those features back into dma-coherent.c
if needed.
Thanks,
Laura