| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <229857ad19690de7d6ec3590c3a37cb6@mail.gmail.com>
Date: Sat, 28 Jun 2025 14:04:53 -0700
From: William Zhang <william.zhang@...adcom.com>
To: Oreoluwa Babatunde <quic_obabatun@...cinc.com>, robh@...nel.org
Cc: aisheng.dong@....com, andy@...ck.fi.intel.com, catalin.marinas@....com,
devicetree@...r.kernel.org, hch@....de, iommu@...ts.linux.dev,
kernel@...cinc.com, klarasmodin@...il.com, linux-kernel@...r.kernel.org,
m.szyprowski@...sung.com, quic_ninanaik@...cinc.com, robin.murphy@....com,
saravanak@...gle.com, will@...nel.org, oreoluwa.babatunde@....qualcomm.com
Subject: RE: [PATCH v10 1/2] of: reserved_mem: Restruture how the reserved
memory regions are processed
> -----Original Message-----
> From: Oreoluwa Babatunde <quic_obabatun@...cinc.com>
> Sent: Friday, June 27, 2025 11:02 AM
> To: William Zhang <william.zhang@...adcom.com>; robh@...nel.org
> Cc: aisheng.dong@....com; andy@...ck.fi.intel.com;
> catalin.marinas@....com; devicetree@...r.kernel.org; hch@....de;
> iommu@...ts.linux.dev; kernel@...cinc.com; klarasmodin@...il.com; linux-
> kernel@...r.kernel.org; m.szyprowski@...sung.com;
> quic_ninanaik@...cinc.com; robin.murphy@....com; saravanak@...gle.com;
> will@...nel.org; oreoluwa.babatunde@....qualcomm.com
> Subject: Re: [PATCH v10 1/2] of: reserved_mem: Restruture how the reserved
> memory regions are processed
>
>
>
> On 6/22/2025 6:24 PM, William Zhang wrote:
> > On Tue, Jun 17, 2025 at 10:15 AM William Zhang
> > <william.zhang@...adcom.com> wrote:
> >>
> >> Hi Oreoluwa,
> >>
> >> On 10/8/2024 3:06 PM, Oreoluwa Babatunde wrote:
> >>> Reserved memory regions defined in the devicetree can be broken up
> >>> into
> >>> two groups:
> >>> i) Statically-placed reserved memory regions
> >>> i.e. regions defined with a static start address and size using the
> >>> "reg" property.
> >>> ii) Dynamically-placed reserved memory regions.
> >>> i.e. regions defined by specifying an address range where they can be
> >>> placed in memory using the "alloc_ranges" and "size" properties.
> >>>
> >>> These regions are processed and set aside at boot time.
> >>> This is done in two stages as seen below:
> >>>
> >>> Stage 1:
> >>> At this stage, fdt_scan_reserved_mem() scans through the child nodes
> >>> of
> >>> the reserved_memory node using the flattened devicetree and does the
> >>> following:
> >>>
> >>> 1) If the node represents a statically-placed reserved memory region,
> >>> i.e. if it is defined using the "reg" property:
> >>> - Call memblock_reserve() or memblock_mark_nomap() as needed.
> >>> - Add the information for that region into the reserved_mem array
> >>> using fdt_reserved_mem_save_node().
> >>> i.e. fdt_reserved_mem_save_node(node, name, base, size).
> >>>
> >>> 2) If the node represents a dynamically-placed reserved memory region,
> >>> i.e. if it is defined using "alloc-ranges" and "size" properties:
> >>> - Add the information for that region to the reserved_mem array
> >>> with
> >>> the starting address and size set to 0.
> >>> i.e. fdt_reserved_mem_save_node(node, name, 0, 0).
> >>> Note: This region is saved to the array with a starting address of
> >>> 0
> >>> because a starting address is not yet allocated for it.
> >>>
> >>> Stage 2:
> >>> After iterating through all the reserved memory nodes and storing
> >>> their
> >>> relevant information in the reserved_mem array,fdt_init_reserved_mem()
> >>> is
> >>> called and does the following:
> >>>
> >>> 1) For statically-placed reserved memory regions:
> >>> - Call the region specific init function using
> >>> __reserved_mem_init_node().
> >>> 2) For dynamically-placed reserved memory regions:
> >>> - Call __reserved_mem_alloc_size() which is used to allocate
> >>> memory
> >>> for each of these regions, and mark them as nomap if they have
> >>> the
> >>> nomap property specified in the DT.
> >>> - Call the region specific init function.
> >>>
> >>> The current size of the resvered_mem array is 64 as is defined by
> >>> MAX_RESERVED_REGIONS. This means that there is a limitation of 64 for
> >>> how many reserved memory regions can be specified on a system.
> >>> As systems continue to grow more and more complex, the number of
> >>> reserved memory regions needed are also growing and are starting to
> >>> hit
> >>> this 64 count limit, hence the need to make the reserved_mem array
> >>> dynamically sized (i.e. dynamically allocating memory for the
> >>> reserved_mem array using membock_alloc_*).
> >>>
> >>> On architectures such as arm64, memory allocated using memblock is
> >>> writable only after the page tables have been setup. This means that
> >>> if
> >>> the reserved_mem array is going to be dynamically allocated, it needs
> >>> to
> >>> happen after the page tables have been setup, not before.
> >>>
> >>> Since the reserved memory regions are currently being processed and
> >>> added to the array before the page tables are setup, there is a need
> >>> to
> >>> change the order in which some of the processing is done to allow for
> >>> the reserved_mem array to be dynamically sized.
> >>>
> >>> It is possible to process the statically-placed reserved memory
> >>> regions
> >>> without needing to store them in the reserved_mem array until after
> >>> the
> >>> page tables have been setup because all the information stored in the
> >>> array is readily available in the devicetree and can be referenced at
> >>> any time.
> >>> Dynamically-placed reserved memory regions on the other hand get
> >>> assigned a start address only at runtime, and hence need a place to be
> >>> stored once they are allocated since there is no other referrence to
> >>> the
> >>> start address for these regions.
> >>>
> >>> Hence this patch changes the processing order of the reserved memory
> >>> regions in the following ways:
> >>>
> >>> Step 1:
> >>> fdt_scan_reserved_mem() scans through the child nodes of
> >>> the reserved_memory node using the flattened devicetree and does the
> >>> following:
> >>>
> >>> 1) If the node represents a statically-placed reserved memory region,
> >>> i.e. if it is defined using the "reg" property:
> >>> - Call memblock_reserve() or memblock_mark_nomap() as needed.
> >>>
> >>> 2) If the node represents a dynamically-placed reserved memory region,
> >>> i.e. if it is defined using "alloc-ranges" and "size" properties:
> >>> - Call __reserved_mem_alloc_size() which will:
> >>> i) Allocate memory for the reserved region and call
> >>> memblock_mark_nomap() as needed.
> >>> ii) Call the region specific initialization function using
> >>> fdt_init_reserved_mem_node().
> >>> iii) Save the region information in the reserved_mem array using
> >>> fdt_reserved_mem_save_node().
> >>>
> >>> Step 2:
> >>> 1) This stage of the reserved memory processing is now only used to
> >>> add
> >>> the statically-placed reserved memory regions into the
> >>> reserved_mem
> >>> array using fdt_scan_reserved_mem_reg_nodes(), as well as call
> >>> their
> >>> region specific initialization functions.
> >>>
> >>> 2) This step has also been moved to be after the page tables are
> >>> setup. Moving this will allow us to replace the reserved_mem
> >>> array with a dynamically sized array before storing the rest of
> >>> these regions.
> >> I am running into a call trace with this order change on armv7 chip
> >> when
> >> I tried to allocate dma coherent memory from the device reserved
> >> memory.
> >> The issue does not happen on armv8 chips.
> >>
> >> [ 0.000000] Reserved memory: created CMA memory pool at 0x1e000000,
> >> size 32 MiB
> >> [ 0.000000] OF: reserved mem: initialized node dt_reserved_cma,
> >> compatible id shared-dma-pool
> >> [ 0.000000] OF: reserved mem: 0x1e000000..0x1fffffff (32768 KiB) map
> >> reusable dt_reserved_cma
> >> ....
> >>
> >> [ 0.445322] ------------[ cut here ]------------
> >> [ 0.445353] WARNING: CPU: 0 PID: 1 at mm/memory.c:3069
> >> __apply_to_page_range+0x380/0x388
> >> [ 0.488911] Modules linked in:
> >> [ 0.492027] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted
> >> 6.16.0-rc1-g27605c8c0f69-dirty #3 NONE
> >> [ 0.501174] Hardware name: Generic DT based system
> >> [ 0.505965] Call trace:
> >> [ 0.505985] unwind_backtrace from show_stack+0x10/0x14
> >> [ 0.513764] show_stack from dump_stack_lvl+0x54/0x68
> >> [ 0.518834] dump_stack_lvl from __warn+0x7c/0x128
> >> [ 0.523639] __warn from warn_slowpath_fmt+0x184/0x18c
> >> [ 0.527676] Freeing initrd memory: 65536K
> >> [ 0.532788] warn_slowpath_fmt from
> __apply_to_page_range+0x380/0x388
> >> [ 0.539242] __apply_to_page_range from
> apply_to_page_range+0x1c/0x24
> >> [ 0.545689] apply_to_page_range from
> __alloc_from_contiguous+0xc0/0x14c
> >> [ 0.552398] __alloc_from_contiguous from
> cma_allocator_alloc+0x34/0x3c
> >> [ 0.559016] cma_allocator_alloc from arch_dma_alloc+0x11c/0x2ac
> >> [ 0.565025] arch_dma_alloc from dma_alloc_attrs+0x90/0x2e8
> >> [ 0.570603] dma_alloc_attrs from dmydev_probe+0x8c/0xe8
> >> [ 0.575919] dmydev_probe from platform_probe+0x5c/0xb0
> >> [ 0.581152] platform_probe from really_probe+0xc8/0x2c8
> >> [ 0.586467] really_probe from __driver_probe_device+0x88/0x19c
> >> [ 0.592387] __driver_probe_device from
> >> driver_probe_device+0x30/0x104
> >> [ 0.598915] driver_probe_device from __driver_attach+0x90/0x178
> >> [ 0.604921] __driver_attach from bus_for_each_dev+0x7c/0xcc
> >> [ 0.610582] bus_for_each_dev from bus_add_driver+0xcc/0x1ec
> >> [ 0.616241] bus_add_driver from driver_register+0x7c/0x114
> >> [ 0.621814] driver_register from dmydev_init+0x20/0x28
> >> [ 0.627045] dmydev_init from do_one_initcall+0x58/0x200
> >> [ 0.632363] do_one_initcall from kernel_init_freeable+0x1cc/0x228
> >> [ 0.638550] kernel_init_freeable from kernel_init+0x1c/0x12c
> >> [ 0.644299] kernel_init from ret_from_fork+0x14/0x28
> >> [ 0.649351] Exception stack(0xe0819fb0 to 0xe0819ff8)
> >> [ 0.654401] 9fa0: 00000000
> >> 00000000 00000000 00000000
> >> [ 0.662575] 9fc0: 00000000 00000000 00000000 00000000 00000000
> >> 00000000 00000000 00000000
> >> [ 0.670747] 9fe0: 00000000 00000000 00000000 00000000 00000013
> 00000000
> >> [ 0.677403] ---[ end trace 0000000000000000 ]---
> >> [ 0.682083] dmydev dmy_device: Allocate dma memory at 0xde000000 dma
> >> addr 0x1e000000
> >>
> >> The reason is that now reserved memory's fixup function
> >> dma_contiguous_early_fixup is called after the page table is
> >> initialized. This fixup function increases the dma_mmu_remap count for
> >> each reserved memory. And the dma_contiguous_remap function depends
> on
> >> it to properly set up the reserved memory mmu table entry. Before this
> >> change, the paging_init function calls dma_contiguous_remap and it
> >> founds the reserved memory and set it up properly. After the change,
> >> this function found there is no reserved memory so skip any
> >> initialization hence causes the crash later on when my driver tries to
> >> allocate dma memory from the reserved memory.
> >>
> >> My workaround below is to move the dma_contiguous_remap out from the
> >> paging_init function to the place right after unflatten_device_tree
> >> where the dma_mmu_remap count is correctly set. But this is not ideal
> >> solution and would like to see if you have any better way to solve the
> >> issue.
> >>
> >> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> >> index a41c93988d2c..535d1bf44529 100644
> >> --- a/arch/arm/kernel/setup.c
> >> +++ b/arch/arm/kernel/setup.c
> >> @@ -1079,6 +1079,7 @@ void __init hyp_mode_check(void)
> >> #endif
> >> }
> >>
> >> +void __init dma_contiguous_remap(void);
> >> static void (*__arm_pm_restart)(enum reboot_mode reboot_mode, const
> >> char *cmd);
> >>
> >> static int arm_restart(struct notifier_block *nb, unsigned long
> >> action,
> >> @@ -1164,6 +1165,7 @@ void __init setup_arch(char **cmdline_p)
> >> }
> >>
> >> unflatten_device_tree();
> >> + dma_contiguous_remap();
> >>
> >> arm_dt_init_cpu_maps();
> >> psci_dt_init();
> >> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> >> index edb7f56b7c91..1828c8737d70 100644
> >> --- a/arch/arm/mm/mmu.c
> >> +++ b/arch/arm/mm/mmu.c
> >> @@ -1773,7 +1773,6 @@ void __init paging_init(const struct machine_desc
> >> *mdesc)
> >> * be used
> >> */
> >> map_kernel();
> >> - dma_contiguous_remap();
> >> early_fixmap_shutdown();
> >> devicemaps_init(mdesc);
> >> kmap_init();
> >>
> >> You can reproduce the issue on any v7 devices by adding these nodes to
> >> the device tree
> >> + reserved-memory {
> >> + #address-cells = <1>;
> >> + #size-cells = <1>;
> >> + ranges;
> >> +
> >> + dt_reserved_cma: dt_reserved_cma {
> >> + compatible = "shared-dma-pool";
> >> + reusable;
> >> +
> >> + reg = <0x1e000000 0x2000000>;
> >> + };
> >> + };
> >> +
> >> + dmy_device {
> >> + compatible = "xyz,dmydev";
> >> + memory-region = <&dt_reserved_cma>;
> >> + };
> >>
> >> And use this test driver to trigger the call stack:
> >> diff --git a/drivers/char/dmydev.c b/drivers/char/dmydev.c
> >> new file mode 100644
> >> index 000000000000..1dd52ec492eb
> >> --- /dev/null
> >> +++ b/drivers/char/dmydev.c
> >> @@ -0,0 +1,67 @@
> >> +#include<linux/module.h>
> >> +#include<linux/kernel.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_reserved_mem.h>
> >> +#include <linux/dma-mapping.h>
> >> +
> >> +static int dmydev_probe(struct platform_device *pdev)
> >> +{
> >> + void* virt_addr;
> >> + dma_addr_t dma_addr;
> >> + int ret;
> >> +
> >> + printk(KERN_ALERT "dmydev_probe called\n");
> >> +
> >> + ret = of_reserved_mem_device_init(&pdev->dev);
> >> + if (ret && ret != -ENODEV) {
> >> + dev_err(&pdev->dev, "Couldn't assign reserve memory to
> >> device ret = %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + virt_addr = dma_alloc_coherent(&pdev->dev, 0x800000, &dma_addr,
> >> GFP_KERNEL);
> >> + if (virt_addr == NULL) {
> >> + dev_err(&pdev->dev,"Failed to allocated cma memory\n");
> >> + ret = -ENOMEM;
> >> + }
> >> + else
> >> + dev_err(&pdev->dev,"Allocate dma memory at 0x%px dma
> >> addr %pad\n", virt_addr, &dma_addr);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void dmydev_remove(struct platform_device *pdev)
> >> +{
> >> +}
> >> +
> >> +static const struct of_device_id dmydev_of_match[] = {
> >> + {.compatible = "xyz,dmydev"},
> >> + {}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, dmydev_of_match);
> >>
> >> Let me know if you need more info.
> >>
> >>>
> >>> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@...cinc.com>
> >>> ---
> >>> drivers/of/fdt.c | 5 +-
> >>> drivers/of/of_private.h | 3 +-
> >>> drivers/of/of_reserved_mem.c | 168
> >>> ++++++++++++++++++++++++----------
> -
> >>> 3 files changed, 122 insertions(+), 54 deletions(-)
> >>>
> >>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>> index 4d528c10df3a..d0dbc8183ac4 100644
> >>> --- a/drivers/of/fdt.c
> >>> +++ b/drivers/of/fdt.c
> >>> @@ -511,8 +511,6 @@ void __init early_init_fdt_scan_reserved_mem(void)
> >>> break;
> >>> memblock_reserve(base, size);
> >>> }
> >>> -
> >>> - fdt_init_reserved_mem();
> >>> }
> >>>
> >>> /**
> >>> @@ -1212,6 +1210,9 @@ void __init unflatten_device_tree(void)
> >>> {
> >>> void *fdt = initial_boot_params;
> >>>
> >>> + /* Save the statically-placed regions in the reserved_mem array
> >>> */
> >>> + fdt_scan_reserved_mem_reg_nodes();
> >>> +
> >>> /* Don't use the bootloader provided DTB if ACPI is enabled */
> >>> if (!acpi_disabled)
> >>> fdt = NULL;
> >>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> >>> index 04aa2a91f851..29525c0b9939 100644
> >>> --- a/drivers/of/of_private.h
> >>> +++ b/drivers/of/of_private.h
> >>> @@ -9,6 +9,7 @@
> >>> */
> >>>
> >>> #define FDT_ALIGN_SIZE 8
> >>> +#define MAX_RESERVED_REGIONS 64
> >>>
> >>> /**
> >>> * struct alias_prop - Alias property in 'aliases' node
> >>> @@ -180,7 +181,7 @@ static inline struct device_node
> *__of_get_dma_parent(const struct device_node *
> >>> #endif
> >>>
> >>> int fdt_scan_reserved_mem(void);
> >>> -void fdt_init_reserved_mem(void);
> >>> +void __init fdt_scan_reserved_mem_reg_nodes(void);
> >>>
> >>> bool of_fdt_device_is_available(const void *blob, unsigned long
> >>> node);
> >>>
> >>> diff --git a/drivers/of/of_reserved_mem.c
> >>> b/drivers/of/of_reserved_mem.c
> >>> index 46e1c3fbc769..2011174211f9 100644
> >>> --- a/drivers/of/of_reserved_mem.c
> >>> +++ b/drivers/of/of_reserved_mem.c
> >>> @@ -27,7 +27,6 @@
> >>>
> >>> #include "of_private.h"
> >>>
> >>> -#define MAX_RESERVED_REGIONS 64
> >>> static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
> >>> static int reserved_mem_count;
> >>>
> >>> @@ -56,6 +55,7 @@ static int __init
> early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
> >>> return err;
> >>> }
> >>>
> >>> +static void __init fdt_init_reserved_mem_node(struct reserved_mem
> *rmem);
> >>> /*
> >>> * fdt_reserved_mem_save_node() - save fdt node for second pass
> initialization
> >>> */
> >>> @@ -74,6 +74,9 @@ static void __init
> fdt_reserved_mem_save_node(unsigned long node, const char *un
> >>> rmem->base = base;
> >>> rmem->size = size;
> >>>
> >>> + /* Call the region specific initialization function */
> >>> + fdt_init_reserved_mem_node(rmem);
> >>> +
> >>> reserved_mem_count++;
> >>> return;
> >>> }
> >>> @@ -106,7 +109,6 @@ static int __init
> __reserved_mem_reserve_reg(unsigned long node,
> >>> phys_addr_t base, size;
> >>> int len;
> >>> const __be32 *prop;
> >>> - int first = 1;
> >>> bool nomap;
> >>>
> >>> prop = of_get_flat_dt_prop(node, "reg", &len);
> >>> @@ -134,10 +136,6 @@ static int __init
> __reserved_mem_reserve_reg(unsigned long node,
> >>> uname, &base, (unsigned long)(size /
> >>> SZ_1M));
> >>>
> >>> len -= t_len;
> >>> - if (first) {
> >>> - fdt_reserved_mem_save_node(node, uname, base,
> >>> size);
> >>> - first = 0;
> >>> - }
> >>> }
> >>> return 0;
> >>> }
> >>> @@ -165,12 +163,77 @@ static int __init
> __reserved_mem_check_root(unsigned long node)
> >>> return 0;
> >>> }
> >>>
> >>> +static void __init __rmem_check_for_overlap(void);
> >>> +
> >>> +/**
> >>> + * fdt_scan_reserved_mem_reg_nodes() - Store info for the "reg"
> >>> defined
> >>> + * reserved memory regions.
> >>> + *
> >>> + * This function is used to scan through the DT and store the
> >>> + * information for the reserved memory regions that are defined using
> >>> + * the "reg" property. The region node number, name, base address,
> >>> and
> >>> + * size are all stored in the reserved_mem array by calling the
> >>> + * fdt_reserved_mem_save_node() function.
> >>> + */
> >>> +void __init fdt_scan_reserved_mem_reg_nodes(void)
> >>> +{
> >>> + int t_len = (dt_root_addr_cells + dt_root_size_cells) *
> >>> sizeof(__be32);
> >>> + const void *fdt = initial_boot_params;
> >>> + phys_addr_t base, size;
> >>> + const __be32 *prop;
> >>> + int node, child;
> >>> + int len;
> >>> +
> >>> + if (!fdt)
> >>> + return;
> >>> +
> >>> + node = fdt_path_offset(fdt, "/reserved-memory");
> >>> + if (node < 0) {
> >>> + pr_info("Reserved memory: No reserved-memory node in the
> DT\n");
> >>> + return;
> >>> + }
> >>> +
> >>> + if (__reserved_mem_check_root(node)) {
> >>> + pr_err("Reserved memory: unsupported node format,
> >>> ignoring\n");
> >>> + return;
> >>> + }
> >>> +
> >>> + fdt_for_each_subnode(child, fdt, node) {
> >>> + const char *uname;
> >>> +
> >>> + prop = of_get_flat_dt_prop(child, "reg", &len);
> >>> + if (!prop)
> >>> + continue;
> >>> + if (!of_fdt_device_is_available(fdt, child))
> >>> + continue;
> >>> +
> >>> + uname = fdt_get_name(fdt, child, NULL);
> >>> + if (len && len % t_len != 0) {
> >>> + pr_err("Reserved memory: invalid reg property in
> >>> '%s', skipping
> node.\n",
> >>> + uname);
> >>> + continue;
> >>> + }
> >>> + base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> >>> + size = dt_mem_next_cell(dt_root_size_cells, &prop);
> >>> +
> >>> + if (size)
> >>> + fdt_reserved_mem_save_node(child, uname, base,
> >>> size);
> >>> + }
> >>> +
> >>> + /* check for overlapping reserved regions */
> >>> + __rmem_check_for_overlap();
> >>> +}
> >>> +
> >>> +static int __init __reserved_mem_alloc_size(unsigned long node, const
> char *uname);
> >>> +
> >>> /*
> >>> * fdt_scan_reserved_mem() - scan a single FDT node for reserved
> >>> memory
> >>> */
> >>> int __init fdt_scan_reserved_mem(void)
> >>> {
> >>> int node, child;
> >>> + int dynamic_nodes_cnt = 0;
> >>> + int dynamic_nodes[MAX_RESERVED_REGIONS];
> >>> const void *fdt = initial_boot_params;
> >>>
> >>> node = fdt_path_offset(fdt, "/reserved-memory");
> >>> @@ -192,8 +255,24 @@ int __init fdt_scan_reserved_mem(void)
> >>> uname = fdt_get_name(fdt, child, NULL);
> >>>
> >>> err = __reserved_mem_reserve_reg(child, uname);
> >>> - if (err == -ENOENT && of_get_flat_dt_prop(child, "size",
> >>> NULL))
> >>> - fdt_reserved_mem_save_node(child, uname, 0, 0);
> >>> + /*
> >>> + * Save the nodes for the dynamically-placed regions
> >>> + * into an array which will be used for allocation right
> >>> + * after all the statically-placed regions are reserved
> >>> + * or marked as no-map. This is done to avoid
> >>> dynamically
> >>> + * allocating from one of the statically-placed regions.
> >>> + */
> >>> + if (err == -ENOENT && of_get_flat_dt_prop(child, "size",
> >>> NULL)) {
> >>> + dynamic_nodes[dynamic_nodes_cnt] = child;
> >>> + dynamic_nodes_cnt++;
> >>> + }
> >>> + }
> >>> + for (int i = 0; i < dynamic_nodes_cnt; i++) {
> >>> + const char *uname;
> >>> +
> >>> + child = dynamic_nodes[i];
> >>> + uname = fdt_get_name(fdt, child, NULL);
> >>> + __reserved_mem_alloc_size(child, uname);
> >>> }
> >>> return 0;
> >>> }
> >>> @@ -253,8 +332,7 @@ static int __init
> __reserved_mem_alloc_in_range(phys_addr_t size,
> >>> * __reserved_mem_alloc_size() - allocate reserved memory described
> >>> by
> >>> * 'size', 'alignment' and 'alloc-ranges' properties.
> >>> */
> >>> -static int __init __reserved_mem_alloc_size(unsigned long node,
> >>> - const char *uname, phys_addr_t *res_base, phys_addr_t *res_size)
> >>> +static int __init __reserved_mem_alloc_size(unsigned long node, const
> char *uname)
> >>> {
> >>> int t_len = (dt_root_addr_cells + dt_root_size_cells) *
> >>> sizeof(__be32);
> >>> phys_addr_t start = 0, end = 0;
> >>> @@ -334,9 +412,8 @@ static int __init
> __reserved_mem_alloc_size(unsigned long node,
> >>> return -ENOMEM;
> >>> }
> >>>
> >>> - *res_base = base;
> >>> - *res_size = size;
> >>> -
> >>> + /* Save region in the reserved_mem array */
> >>> + fdt_reserved_mem_save_node(node, uname, base, size);
> >>> return 0;
> >>> }
> >>>
> >>> @@ -425,48 +502,37 @@ static void __init
> __rmem_check_for_overlap(void)
> >>> }
> >>>
> >>> /**
> >>> - * fdt_init_reserved_mem() - allocate and init all saved reserved
> >>> memory
> regions
> >>> + * fdt_init_reserved_mem_node() - Initialize a reserved memory region
> >>> + * @rmem: reserved_mem struct of the memory region to be initialized.
> >>> + *
> >>> + * This function is used to call the region specific initialization
> >>> + * function for a reserved memory region.
> >>> */
> >>> -void __init fdt_init_reserved_mem(void)
> >>> +static void __init fdt_init_reserved_mem_node(struct reserved_mem
> *rmem)
> >>> {
> >>> - int i;
> >>> -
> >>> - /* check for overlapping reserved regions */
> >>> - __rmem_check_for_overlap();
> >>> -
> >>> - for (i = 0; i < reserved_mem_count; i++) {
> >>> - struct reserved_mem *rmem = &reserved_mem[i];
> >>> - unsigned long node = rmem->fdt_node;
> >>> - int err = 0;
> >>> - bool nomap;
> >>> + unsigned long node = rmem->fdt_node;
> >>> + int err = 0;
> >>> + bool nomap;
> >>>
> >>> - nomap = of_get_flat_dt_prop(node, "no-map", NULL) !=
> >>> NULL;
> >>> + nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> >>>
> >>> - if (rmem->size == 0)
> >>> - err = __reserved_mem_alloc_size(node,
> >>> rmem->name,
> >>> - &rmem->base,
> >>> &rmem->size);
> >>> - if (err == 0) {
> >>> - err = __reserved_mem_init_node(rmem);
> >>> - if (err != 0 && err != -ENOENT) {
> >>> - pr_info("node %s compatible matching
> >>> fail\n",
> >>> - rmem->name);
> >>> - if (nomap)
> >>> - memblock_clear_nomap(rmem->base,
> >>> rmem->size);
> >>> - else
> >>> - memblock_phys_free(rmem->base,
> >>> - rmem->size);
> >>> - } else {
> >>> - phys_addr_t end = rmem->base +
> >>> rmem->size - 1;
> >>> - bool reusable =
> >>> - (of_get_flat_dt_prop(node,
> >>> "reusable", NULL)) !=
> NULL;
> >>> -
> >>> - pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
> >>> - &rmem->base, &end, (unsigned
> >>> long)(rmem->size /
> SZ_1K),
> >>> - nomap ? "nomap" : "map",
> >>> - reusable ? "reusable" :
> >>> "non-reusable",
> >>> - rmem->name ? rmem->name :
> >>> "unknown");
> >>> - }
> >>> - }
> >>> + err = __reserved_mem_init_node(rmem);
> >>> + if (err != 0 && err != -ENOENT) {
> >>> + pr_info("node %s compatible matching fail\n",
> >>> rmem->name);
> >>> + if (nomap)
> >>> + memblock_clear_nomap(rmem->base, rmem->size);
> >>> + else
> >>> + memblock_phys_free(rmem->base, rmem->size);
> >>> + } else {
> >>> + phys_addr_t end = rmem->base + rmem->size - 1;
> >>> + bool reusable =
> >>> + (of_get_flat_dt_prop(node, "reusable", NULL)) !=
> >>> NULL;
> >>> +
> >>> + pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
> >>> + &rmem->base, &end, (unsigned long)(rmem->size /
> >>> SZ_1K),
> >>> + nomap ? "nomap" : "map",
> >>> + reusable ? "reusable" : "non-reusable",
> >>> + rmem->name ? rmem->name : "unknown");
> >>> }
> >>> }
> >>>
> >>
> > Just want to follow up on this issue. Do you need any further detail
> > or clarification?
> > Any ARM memory manage guru on this thread can comment?
> > Or is my workaround acceptable as a patch?
>
>
> Hi William,
>
> Sorry about the delay in getting back to you.
>
> Instead of moving dma_contiguous_remap(), I suggest moving
> dma_contiguous_early_fixup()
> to the function that parses the reserved regions so that it is done before
> paging_init.
>
> Here is what that could look like. Can you please give this a try?
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 77016c0cc296..132d2c66cafc 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -25,6 +25,7 @@
> #include <linux/memblock.h>
> #include <linux/kmemleak.h>
> #include <linux/cma.h>
> +#include <linux/dma-map-ops.h>
>
> #include "of_private.h"
>
> @@ -175,13 +176,17 @@ static int __init
> __reserved_mem_reserve_reg(unsigned long node,
> base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> size = dt_mem_next_cell(dt_root_size_cells, &prop);
>
> - if (size &&
> - early_init_dt_reserve_memory(base, size, nomap) == 0)
> + if (size && early_init_dt_reserve_memory(base, size,
> nomap) == 0) {
> + /* Architecture specific contiguous memory fixup.
> */
> + if (of_flat_dt_is_compatible(node,
> "shared-dma-pool"))
> + dma_contiguous_early_fixup(base, size);
> +
> pr_debug("Reserved memory: reserved region for
> node '%s':
> base %pa, size %lu MiB\n",
> uname, &base, (unsigned long)(size /
> SZ_1M));
> - else
> + } else {
> pr_err("Reserved memory: failed to reserve memory
> for node '%s':
> base %pa, size %lu MiB\n",
> uname, &base, (unsigned long)(size /
> SZ_1M));
> + }
>
> len -= t_len;
> }
> @@ -472,6 +477,9 @@ static int __init __reserved_mem_alloc_size(unsigned
> long node, const char *unam
> uname, (unsigned long)(size / SZ_1M));
> return -ENOMEM;
> }
> + /* Architecture specific contiguous memory fixup. */
> + if (of_flat_dt_is_compatible(node, "shared-dma-pool"))
> + dma_contiguous_early_fixup(base, size);
> /* Save region in the reserved_mem array */
> fdt_reserved_mem_save_node(node, uname, base, size);
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index 8df0dfaaca18..9e5d63efe7c5 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -480,8 +480,6 @@ static int __init rmem_cma_setup(struct reserved_mem
> *rmem)
> pr_err("Reserved memory: unable to setup CMA region\n");
> return err;
> }
> - /* Architecture specific contiguous memory fixup. */
> - dma_contiguous_early_fixup(rmem->base, rmem->size);
>
> if (default_cma)
> dma_contiguous_default_area = cma;
>
> Regards,
> Oreoluwa
Thank you Oreoluwa! Your patch fixed the issue too and it looks a more
localized and better fix!
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4199 bytes)
Powered by blists - more mailing lists