| Message ID | 20190621235608.2153-3-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/6] xen/arm: extend device_tree_for_each_node | expand |
On 22.06.19 02:56, Stefano Stabellini wrote: Hi Stefano > As we parse the device tree in Xen, keep track of the reserved-memory > regions as they need special treatment (follow-up patches will make use > of the stored information.) > > Reuse process_memory_node to add reserved-memory regions to the > bootinfo.reserved_mem array. > > Refuse to continue once we reach the max number of reserved memory > regions to avoid accidentally mapping any portions of them into a VM. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > It is cleaner to avoid sharing the whole function process_memory_node > between the normal memory case and the reserved-memory case. I'll do it > in the next version once I understand the best way do to it. > > --- > Changes in v3: > - match only /reserved-memory > - put the warning back in place for reg not present on a normal memory > region > - refuse to continue once we reach the max number of reserved memory > regions > > Changes in v2: > - call process_memory_node from process_reserved_memory_node to avoid > duplication > --- > xen/arch/arm/bootfdt.c | 38 +++++++++++++++++++++++++++++++------ > xen/include/asm-arm/setup.h | 1 + > 2 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 611724433b..b24ab10cb9 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -135,6 +135,8 @@ static int __init process_memory_node(const void *fdt, int node, > const __be32 *cell; > paddr_t start, size; > u32 reg_cells = address_cells + size_cells; > + struct meminfo *mem; > + bool reserved = (bool)data; > > if ( address_cells < 1 || size_cells < 1 ) > { > @@ -143,29 +145,49 @@ static int __init process_memory_node(const void *fdt, int node, > return 0; > } > > + if ( reserved ) > + mem = &bootinfo.reserved_mem; > + else > + mem = &bootinfo.mem; > + > prop = fdt_get_property(fdt, node, "reg", NULL); > if ( !prop ) > { > - printk("fdt: node `%s': missing `reg' property\n", name); > + if ( !reserved ) > + printk("fdt: node `%s': missing `reg' property\n", name); > return 0; > } > > cell = (const __be32 *)prop->data; > banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); > > - for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) > + for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) > { > device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > if ( !size ) > continue; > - bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; > - bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; > - bootinfo.mem.nr_banks++; > + mem->bank[mem->nr_banks].start = start; > + mem->bank[mem->nr_banks].size = size; > + mem->nr_banks++; > } > + /* > + * We reached the max number of supported reserved-memory regions. > + * Stop and refuse to continue. We don't want to risk Xen allocating > + * those regions as normal memory to a VM. > + */ > + BUG_ON(reserved && mem->nr_banks == NR_MEM_BANKS); > > return 0; > } > > +static int __init process_reserved_memory_node(const void *fdt, int node, > + const char *name, int depth, > + u32 address_cells, u32 size_cells) > +{ > + device_tree_for_each_node(fdt, node, depth, process_memory_node, (void*)true); > + return 0; > +} I have tested this series and got the same behavior as with V2 [1]. The "non-reserved-memory" node in my device-tree (sram@47FFF000->scp_shmem@0) is still interpreted as a "reserved-memory". I think, this takes place because current implementation iterates over all device tree nodes starting from real "reserved-memory" node up to the end of the device tree. And if there is at least one "non-reserved-memory" node (with a suitable depth and valid "reg" property) located down the device tree, it will be mistakenly inserted to bootinfo.reserved_mem (as in my case). ---------- Unpacked device tree: { ... memory@48000000 { device_type = "memory"; reg = <0x00000000 0x48000000 0x00000000 0x78000000 0x00000005 0x00000000 0x00000000 0x80000000 0x00000006 0x00000000 0x00000000 0x80000000 0x00000007 0x00000000 0x00000000 0x80000000>; }; reserved-memory { #address-cells = <0x00000002>; #size-cells = <0x00000002>; ranges; linux,lossy_decompress@54000000 { no-map; reg = <0x00000000 0x54000000 0x00000000 0x03000000>; linux,phandle = <0x000000b0>; phandle = <0x000000b0>; }; linux,adsp@57000000 { compatible = "shared-dma-pool"; reusable; reg = <0x00000000 0x57000000 0x00000000 0x01000000>; }; linux,cma@58000000 { compatible = "shared-dma-pool"; reusable; reg = <0x00000000 0x58000000 0x00000000 0x18000000>; linux,cma-default; }; linux,multimedia@70000000 { compatible = "shared-dma-pool"; reusable; reg = <0x00000000 0x70000000 0x00000000 0x10000000>; linux,phandle = <0x000000af>; phandle = <0x000000af>; }; }; ... sram@47FFF000 { compatible = "mmio-sram"; reg = <0x00000000 0x47fff000 0x00000000 0x00001000>; #address-cells = <0x00000001>; #size-cells = <0x00000001>; ranges = <0x00000000 0x00000000 0x47fff000 0x00001000>; scp_shmem@0 { compatible = "mmio-sram"; reg = <0x00000000 0x00000200>; linux,phandle = <0x000000b2>; phandle = <0x000000b2>; }; }; ... }; ---------- Xen log: Starting kernel ... - UART enabled - - CPU 00000000 booting - - Current EL 00000008 - - Xen starting at EL2 - - Zero BSS - - Setting up control registers - - Turning on paging - - Ready - (XEN) Checking for initrd in /chosen (XEN) Initrd 0000000074000040-0000000076252c54 (XEN) node memory@48000000: insert bank 0: 0x48000000->0xc0000000 type: normal (XEN) node memory@48000000: insert bank 1: 0x500000000->0x580000000 type: normal (XEN) node memory@48000000: insert bank 2: 0x600000000->0x680000000 type: normal (XEN) node memory@48000000: insert bank 3: 0x700000000->0x780000000 type: normal (XEN) node linux,lossy_decompress@54000000: insert bank 0: 0x54000000->0x57000000 type: reserved (XEN) node linux,adsp@57000000: insert bank 0: 0x57000000->0x58000000 type: reserved (XEN) node linux,cma@58000000: insert bank 0: 0x58000000->0x70000000 type: reserved (XEN) node linux,multimedia@70000000: insert bank 0: 0x70000000->0x80000000 type: reserved (XEN) node scp_shmem@0: insert bank 0: 0->0x200 type: reserved <--------------- Not a reserved memory (XEN) RAM: 0000000048000000 - 00000000bfffffff (XEN) RAM: 0000000500000000 - 000000057fffffff (XEN) RAM: 0000000600000000 - 000000067fffffff (XEN) RAM: 0000000700000000 - 000000077fffffff (XEN) (XEN) MODULE[0]: 0000000048000000 - 0000000048014080 Device Tree (XEN) MODULE[1]: 0000000074000040 - 0000000076252c54 Ramdisk (XEN) MODULE[2]: 000000007a000000 - 000000007c000000 Kernel (XEN) MODULE[3]: 000000007c000000 - 000000007c010000 XSM (XEN) RESVD[0]: 0000000048000000 - 0000000048014000 (XEN) RESVD[1]: 0000000074000040 - 0000000076252c54 ---------- [1] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg45632.html
Hi, On 6/22/19 12:56 AM, Stefano Stabellini wrote: > As we parse the device tree in Xen, keep track of the reserved-memory > regions as they need special treatment (follow-up patches will make use > of the stored information.) > > Reuse process_memory_node to add reserved-memory regions to the > bootinfo.reserved_mem array. > > Refuse to continue once we reach the max number of reserved memory > regions to avoid accidentally mapping any portions of them into a VM. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > It is cleaner to avoid sharing the whole function process_memory_node > between the normal memory case and the reserved-memory case. I'll do it > in the next version once I understand the best way do to it. parse_reg(....) { if (reg not present) return -ENOPRESENT /* parse regs */ return (full) ? -EFULL : 0; } process_memory_node(....) { return parse_reg(...); } process_reserved_region() { ret = parse_reg(...); if ( ret == -EFULL ) panic(....); else if ( ret != -ENOPRESENT ) return ret; return 0; } > > --- > Changes in v3: > - match only /reserved-memory > - put the warning back in place for reg not present on a normal memory > region > - refuse to continue once we reach the max number of reserved memory > regions > > Changes in v2: > - call process_memory_node from process_reserved_memory_node to avoid > duplication > --- > xen/arch/arm/bootfdt.c | 38 +++++++++++++++++++++++++++++++------ > xen/include/asm-arm/setup.h | 1 + > 2 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 611724433b..b24ab10cb9 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -135,6 +135,8 @@ static int __init process_memory_node(const void *fdt, int node, > const __be32 *cell; > paddr_t start, size; > u32 reg_cells = address_cells + size_cells; > + struct meminfo *mem; > + bool reserved = (bool)data; > > if ( address_cells < 1 || size_cells < 1 ) > { > @@ -143,29 +145,49 @@ static int __init process_memory_node(const void *fdt, int node, > return 0; > } > > + if ( reserved ) > + mem = &bootinfo.reserved_mem; > + else > + mem = &bootinfo.mem; Rather than passing a bool, you could pass bootinfo.{mem, reserved_mem} in parameter. > + > prop = fdt_get_property(fdt, node, "reg", NULL); > if ( !prop ) > { > - printk("fdt: node `%s': missing `reg' property\n", name); > + if ( !reserved ) > + printk("fdt: node `%s': missing `reg' property\n", name); I would just get rid of this print and return an error than allow the caller to decide what to do. > return 0; > } > > cell = (const __be32 *)prop->data; > banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); > > - for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) > + for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) > { > device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > if ( !size ) > continue; > - bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; > - bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; > - bootinfo.mem.nr_banks++; > + mem->bank[mem->nr_banks].start = start; > + mem->bank[mem->nr_banks].size = size; > + mem->nr_banks++; > } > + /* > + * We reached the max number of supported reserved-memory regions. > + * Stop and refuse to continue. We don't want to risk Xen allocating > + * those regions as normal memory to a VM. The last sentence is confusing because reserved-region are normal memory that have been carved out for a specific usage. Also, the problem is not only with VM but any memory allocation. So a better sentence would be: "We don't want to give the pages to the allocator". > + */ > + BUG_ON(reserved && mem->nr_banks == NR_MEM_BANKS); > > return 0; > } > > +static int __init process_reserved_memory_node(const void *fdt, int node, > + const char *name, int depth, > + u32 address_cells, u32 size_cells) > +{ > + device_tree_for_each_node(fdt, node, depth, process_memory_node, (void*)true); > + return 0; > +} > + > static void __init process_multiboot_node(const void *fdt, int node, > const char *name, > u32 address_cells, u32 size_cells) > @@ -299,7 +321,11 @@ static int __init early_scan_node(const void *fdt, > > if ( device_tree_node_matches(fdt, node, "memory") ) > rc = process_memory_node(fdt, node, name, depth, > - address_cells, size_cells, NULL); > + address_cells, size_cells, (void*)false); > + else if ( depth == 1 && !strcmp(name, "reserved-memory") && > + strlen(name) == strlen("reserved-memory") ) > + rc = process_reserved_memory_node(fdt, node, name, depth, > + address_cells, size_cells); > else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) || > device_tree_node_compatible(fdt, node, "multiboot,module" ))) > process_multiboot_node(fdt, node, name, address_cells, size_cells); > diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h > index 8bf3d5910a..efcba545c2 100644 > --- a/xen/include/asm-arm/setup.h > +++ b/xen/include/asm-arm/setup.h > @@ -66,6 +66,7 @@ struct bootcmdlines { > > struct bootinfo { > struct meminfo mem; > + struct meminfo reserved_mem; > struct bootmodules modules; > struct bootcmdlines cmdlines; > #ifdef CONFIG_ACPI > Cheers,
Hi, On 7/8/19 8:02 PM, Oleksandr wrote: > On 22.06.19 02:56, Stefano Stabellini wrote: > I have tested this series and got the same behavior as with V2 [1]. > > The "non-reserved-memory" node in my device-tree > (sram@47FFF000->scp_shmem@0) is still interpreted as a "reserved-memory". > I think, this takes place because current implementation iterates over > all device tree nodes starting > from real "reserved-memory" node up to the end of the device tree. And > if there is at least one "non-reserved-memory" node (with a suitable > depth and valid "reg" property) > located down the device tree, it will be mistakenly inserted to > bootinfo.reserved_mem (as in my case). The problem is because we are passing the depth of the parent. Because of that, it will look for anything at the same depth (see the check depth >= min_depth in device_tree_for_each_node). The correct solution here, would be to use the depth of the child (i.e depth + 1) when calling device_tree_for_each_node in process_reserved_memory_node. Cheers,
On Wed, 10 Jul 2019, Julien Grall wrote: > Hi, > > On 7/8/19 8:02 PM, Oleksandr wrote: > > On 22.06.19 02:56, Stefano Stabellini wrote: > > I have tested this series and got the same behavior as with V2 [1]. > > > > The "non-reserved-memory" node in my device-tree > > (sram@47FFF000->scp_shmem@0) is still interpreted as a "reserved-memory". > > I think, this takes place because current implementation iterates over all > > device tree nodes starting > > from real "reserved-memory" node up to the end of the device tree. And if > > there is at least one "non-reserved-memory" node (with a suitable depth and > > valid "reg" property) > > located down the device tree, it will be mistakenly inserted to > > bootinfo.reserved_mem (as in my case). > > The problem is because we are passing the depth of the parent. Because of > that, it will look for anything at the same depth (see the check depth >= > min_depth in device_tree_for_each_node). > > The correct solution here, would be to use the depth of the child (i.e depth + > 1) when calling device_tree_for_each_node in process_reserved_memory_node. Yes, that is the right thing to do, together with also passing address_cells and size_cells of the parent to device_tree_for_each_node, so that it can be calculate appropriately at all times, regardless of depth.
On Wed, 10 Jul 2019, Julien Grall wrote: > Hi, > > On 6/22/19 12:56 AM, Stefano Stabellini wrote: > > As we parse the device tree in Xen, keep track of the reserved-memory > > regions as they need special treatment (follow-up patches will make use > > of the stored information.) > > > > Reuse process_memory_node to add reserved-memory regions to the > > bootinfo.reserved_mem array. > > > > Refuse to continue once we reach the max number of reserved memory > > regions to avoid accidentally mapping any portions of them into a VM. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > > > --- > > It is cleaner to avoid sharing the whole function process_memory_node > > between the normal memory case and the reserved-memory case. I'll do it > > in the next version once I understand the best way do to it. > > parse_reg(....) > { > > if (reg not present) > return -ENOPRESENT > > /* parse regs */ > > return (full) ? -EFULL : 0; > } > > process_memory_node(....) > { > return parse_reg(...); > } > > process_reserved_region() > { > ret = parse_reg(...); > if ( ret == -EFULL ) > panic(....); > else if ( ret != -ENOPRESENT ) > return ret; > return 0; > } Thank you, that clarified things a lot! > > --- > > Changes in v3: > > - match only /reserved-memory > > - put the warning back in place for reg not present on a normal memory > > region > > - refuse to continue once we reach the max number of reserved memory > > regions > > > > Changes in v2: > > - call process_memory_node from process_reserved_memory_node to avoid > > duplication > > --- > > xen/arch/arm/bootfdt.c | 38 +++++++++++++++++++++++++++++++------ > > xen/include/asm-arm/setup.h | 1 + > > 2 files changed, 33 insertions(+), 6 deletions(-) > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > index 611724433b..b24ab10cb9 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -135,6 +135,8 @@ static int __init process_memory_node(const void *fdt, > > int node, > > const __be32 *cell; > > paddr_t start, size; > > u32 reg_cells = address_cells + size_cells; > > + struct meminfo *mem; > > + bool reserved = (bool)data; > > if ( address_cells < 1 || size_cells < 1 ) > > { > > @@ -143,29 +145,49 @@ static int __init process_memory_node(const void *fdt, > > int node, > > return 0; > > } > > + if ( reserved ) > > + mem = &bootinfo.reserved_mem; > > + else > > + mem = &bootinfo.mem; > > Rather than passing a bool, you could pass bootinfo.{mem, reserved_mem} in > parameter. I'll do that > > + > > prop = fdt_get_property(fdt, node, "reg", NULL); > > if ( !prop ) > > { > > - printk("fdt: node `%s': missing `reg' property\n", name); > > + if ( !reserved ) > > + printk("fdt: node `%s': missing `reg' property\n", name); > > I would just get rid of this print and return an error than allow the caller > to decide what to do. Yep > > return 0; > > } > > cell = (const __be32 *)prop->data; > > banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); > > - for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) > > + for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) > > { > > device_tree_get_reg(&cell, address_cells, size_cells, &start, > > &size); > > if ( !size ) > > continue; > > - bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; > > - bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; > > - bootinfo.mem.nr_banks++; > > + mem->bank[mem->nr_banks].start = start; > > + mem->bank[mem->nr_banks].size = size; > > + mem->nr_banks++; > > } > > + /* > > + * We reached the max number of supported reserved-memory regions. > > + * Stop and refuse to continue. We don't want to risk Xen allocating > > + * those regions as normal memory to a VM. > > The last sentence is confusing because reserved-region are normal memory that > have been carved out for a specific usage. Also, the problem is not only with > VM but any memory allocation. > > So a better sentence would be: "We don't want to give the pages to the > allocator". Thanks, I'll make the change
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 611724433b..b24ab10cb9 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -135,6 +135,8 @@ static int __init process_memory_node(const void *fdt, int node, const __be32 *cell; paddr_t start, size; u32 reg_cells = address_cells + size_cells; + struct meminfo *mem; + bool reserved = (bool)data; if ( address_cells < 1 || size_cells < 1 ) { @@ -143,29 +145,49 @@ static int __init process_memory_node(const void *fdt, int node, return 0; } + if ( reserved ) + mem = &bootinfo.reserved_mem; + else + mem = &bootinfo.mem; + prop = fdt_get_property(fdt, node, "reg", NULL); if ( !prop ) { - printk("fdt: node `%s': missing `reg' property\n", name); + if ( !reserved ) + printk("fdt: node `%s': missing `reg' property\n", name); return 0; } cell = (const __be32 *)prop->data; banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); - for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) + for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) { device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); if ( !size ) continue; - bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; - bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; - bootinfo.mem.nr_banks++; + mem->bank[mem->nr_banks].start = start; + mem->bank[mem->nr_banks].size = size; + mem->nr_banks++; } + /* + * We reached the max number of supported reserved-memory regions. + * Stop and refuse to continue. We don't want to risk Xen allocating + * those regions as normal memory to a VM. + */ + BUG_ON(reserved && mem->nr_banks == NR_MEM_BANKS); return 0; } +static int __init process_reserved_memory_node(const void *fdt, int node, + const char *name, int depth, + u32 address_cells, u32 size_cells) +{ + device_tree_for_each_node(fdt, node, depth, process_memory_node, (void*)true); + return 0; +} + static void __init process_multiboot_node(const void *fdt, int node, const char *name, u32 address_cells, u32 size_cells) @@ -299,7 +321,11 @@ static int __init early_scan_node(const void *fdt, if ( device_tree_node_matches(fdt, node, "memory") ) rc = process_memory_node(fdt, node, name, depth, - address_cells, size_cells, NULL); + address_cells, size_cells, (void*)false); + else if ( depth == 1 && !strcmp(name, "reserved-memory") && + strlen(name) == strlen("reserved-memory") ) + rc = process_reserved_memory_node(fdt, node, name, depth, + address_cells, size_cells); else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) || device_tree_node_compatible(fdt, node, "multiboot,module" ))) process_multiboot_node(fdt, node, name, address_cells, size_cells); diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index 8bf3d5910a..efcba545c2 100644 --- a/xen/include/asm-arm/setup.h +++ b/xen/include/asm-arm/setup.h @@ -66,6 +66,7 @@ struct bootcmdlines { struct bootinfo { struct meminfo mem; + struct meminfo reserved_mem; struct bootmodules modules; struct bootcmdlines cmdlines; #ifdef CONFIG_ACPI
As we parse the device tree in Xen, keep track of the reserved-memory regions as they need special treatment (follow-up patches will make use of the stored information.) Reuse process_memory_node to add reserved-memory regions to the bootinfo.reserved_mem array. Refuse to continue once we reach the max number of reserved memory regions to avoid accidentally mapping any portions of them into a VM. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- It is cleaner to avoid sharing the whole function process_memory_node between the normal memory case and the reserved-memory case. I'll do it in the next version once I understand the best way do to it. --- Changes in v3: - match only /reserved-memory - put the warning back in place for reg not present on a normal memory region - refuse to continue once we reach the max number of reserved memory regions Changes in v2: - call process_memory_node from process_reserved_memory_node to avoid duplication --- xen/arch/arm/bootfdt.c | 38 +++++++++++++++++++++++++++++++------ xen/include/asm-arm/setup.h | 1 + 2 files changed, 33 insertions(+), 6 deletions(-)