| Message ID | 1556658172-8824-8-git-send-email-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/10] xen: add a p2mt parameter to map_mmio_regions | expand |
Hi Stefano, On 30/04/2019 22:02, 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. Remove the warning if there is no reg in > process_memory_node because it is a normal condition for > reserved-memory. And it is not a normal condition for /memory... So your argument here is not sufficient for me to not keep the warning here for /memory. Rather than trying to re-purpose process_memory_node, I would prefer if you move out the parsing of "reg" and then provide 2 functions (one for /memory and one for /reserved-memory). The parsing function will return an error if "reg" is not present, but it can be ignored by /reserved-memory and a warning is added for /memory. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > > Not done: create an e820-like structure on ARM. > > Changes in v2: > - call process_memory_node from process_reserved_memory_node to avoid > duplication > --- > xen/arch/arm/bootfdt.c | 30 ++++++++++++++++++++++-------- > xen/include/asm-arm/setup.h | 1 + > 2 files changed, 23 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index b6600ab..9355a6e 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,39 @@ 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); > 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++ ) As I pointed out on v1, this is pretty fragile. While ignoring /memory bank is fine if we have no more space, for /reserved-region this may mean using them in Xen allocator with the consequences we all know. If you split the function properly, then you will be able to treat reserved-regions and memory differently. Cheers,
Hi, Some more review on this patch. On 4/30/19 10:02 PM, 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. Remove the warning if there is no reg in > process_memory_node because it is a normal condition for > reserved-memory. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > > Not done: create an e820-like structure on ARM. > > Changes in v2: > - call process_memory_node from process_reserved_memory_node to avoid > duplication > --- > xen/arch/arm/bootfdt.c | 30 ++++++++++++++++++++++-------- > xen/include/asm-arm/setup.h | 1 + > 2 files changed, 23 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index b6600ab..9355a6e 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,39 @@ 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); > 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++; > } > > 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) > @@ -297,7 +309,9 @@ static int __init early_scan_node(const void *fdt, > { > if ( device_tree_node_matches(fdt, node, "memory") ) > process_memory_node(fdt, node, name, depth, address_cells, size_cells, > - NULL); > + (void*)false); > + else if ( device_tree_node_matches(fdt, node, "reserved-memory") ) > + process_reserved_memory_node(fdt, node, name, depth, address_cells, size_cells); This will match reserved-memory@... or even /foo/reserved-memory. Here, we only want to match /reserved-memory. > 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 48187e1..5c3fc2d 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,
On Wed, 1 May 2019, Julien Grall wrote: > Hi Stefano, > > On 30/04/2019 22:02, 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. Remove the warning if there is no reg in > > process_memory_node because it is a normal condition for > > reserved-memory. > > And it is not a normal condition for /memory... So your argument here is not > sufficient for me to not keep the warning here for /memory. You are right, I'll put the warning back in place. > Rather than trying to re-purpose process_memory_node, I would prefer if you > move out the parsing of "reg" and then provide 2 functions (one for /memory > and one for /reserved-memory). > > The parsing function will return an error if "reg" is not present, but it can > be ignored by /reserved-memory and a warning is added for /memory. I am OK with making this change, but I gave a look at the code for some time and I cannot exactly figure out the interface you have in mind. I understand completely separating the functions as I did in v1, but not the partial split you are suggesting here. I managed to address your other comments keeping a single function. I suggest you take a look at the new version, then maybe write some psedo-code to help me figure out what you would like me to do? > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > > > --- > > > > Not done: create an e820-like structure on ARM. > > > > Changes in v2: > > - call process_memory_node from process_reserved_memory_node to avoid > > duplication > > --- > > xen/arch/arm/bootfdt.c | 30 ++++++++++++++++++++++-------- > > xen/include/asm-arm/setup.h | 1 + > > 2 files changed, 23 insertions(+), 8 deletions(-) > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > index b6600ab..9355a6e 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,39 @@ 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); > > 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++ ) > > As I pointed out on v1, this is pretty fragile. While ignoring /memory bank is > fine if we have no more space, for /reserved-region this may mean using them > in Xen allocator with the consequences we all know. Yeah, we don't want that. > If you split the function properly, then you will be able to treat > reserved-regions and memory differently. I did so, but without splitting the functions.
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index b6600ab..9355a6e 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,39 @@ 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); 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++; } 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) @@ -297,7 +309,9 @@ static int __init early_scan_node(const void *fdt, { if ( device_tree_node_matches(fdt, node, "memory") ) process_memory_node(fdt, node, name, depth, address_cells, size_cells, - NULL); + (void*)false); + else if ( device_tree_node_matches(fdt, node, "reserved-memory") ) + 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 48187e1..5c3fc2d 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. Remove the warning if there is no reg in process_memory_node because it is a normal condition for reserved-memory. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- Not done: create an e820-like structure on ARM. Changes in v2: - call process_memory_node from process_reserved_memory_node to avoid duplication --- xen/arch/arm/bootfdt.c | 30 ++++++++++++++++++++++-------- xen/include/asm-arm/setup.h | 1 + 2 files changed, 23 insertions(+), 8 deletions(-)