| Message ID | 20190806214925.7534-3-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/7] xen/arm: extend device_tree_for_each_node | expand |
Hi Stefano, On 06/08/2019 22:49, 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> > > --- > Changes in v4: > - depth + 1 in process_reserved_memory_node Ah, you fixed it in this patch. But then, this does not match the documentation in patch #1. > - pass address_cells and size_cells to device_tree_for_each_node > - pass struct meminfo * instead of a boolean to process_memory_node > - improve in-code comment I can't see any comment, is that an improvement? :) > - use a separate process_reserved_memory_node (separate from > process_memory_node) function wrapper to have different error handling > > 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 | 43 +++++++++++++++++++++++++++++++------ > xen/include/asm-arm/setup.h | 1 + > 2 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index c22d57cd72..3e6fd63b16 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -144,6 +144,7 @@ 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 = (struct meminfo *)data; The cast is unnecessary. The rest of the code looks good. Pending the discussion about device_tree_for_each_node: Acked-by: Julien Grall <julien.grall@arm.com> > > if ( address_cells < 1 || size_cells < 1 ) > { > @@ -159,21 +160,47 @@ static int __init process_memory_node(const void *fdt, int node, > 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++; > } > > - if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) > + if ( mem->nr_banks == NR_MEM_BANKS ) > return -ENOSPC; > 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, > + void *data) > +{ > + int rc = process_memory_node(fdt, node, name, depth, address_cells, > + size_cells, data); > + > + if ( rc == -ENOSPC ) > + panic("Max number of supported reserved-memory regions reached."); > + else if ( rc != -ENOENT ) > + return rc; > + return 0; > +} > + > +static int __init process_reserved_memory(const void *fdt, int node, > + const char *name, int depth, > + u32 address_cells, u32 size_cells) > +{ > + return device_tree_for_each_node(fdt, node, depth + 1, > + address_cells, size_cells, > + process_reserved_memory_node, > + &bootinfo.reserved_mem); > +} > + > static void __init process_multiboot_node(const void *fdt, int node, > const char *name, > u32 address_cells, u32 size_cells) > @@ -307,7 +334,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, &bootinfo.mem); > + else if ( depth == 1 && !strcmp(name, "reserved-memory") && > + strlen(name) == strlen("reserved-memory") ) > + rc = process_reserved_memory(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,
On 06/08/2019 22:49, Stefano Stabellini wrote: > static void __init process_multiboot_node(const void *fdt, int node, > const char *name, > u32 address_cells, u32 size_cells) > @@ -307,7 +334,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, &bootinfo.mem); > + else if ( depth == 1 && !strcmp(name, "reserved-memory") && > + strlen(name) == strlen("reserved-memory") ) Unless my stdlib knowledge is rusty, strcmp() will only return 0 if the two string exactly matches. This implies the two strings are exactly the same length. Cheers,
On Wed, 7 Aug 2019, Julien Grall wrote: > On 06/08/2019 22:49, Stefano Stabellini wrote: > > static void __init process_multiboot_node(const void *fdt, int node, > > const char *name, > > u32 address_cells, u32 > > size_cells) > > @@ -307,7 +334,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, &bootinfo.mem); > > + else if ( depth == 1 && !strcmp(name, "reserved-memory") && > > + strlen(name) == strlen("reserved-memory") ) > > Unless my stdlib knowledge is rusty, strcmp() will only return 0 if the two > string exactly matches. This implies the two strings are exactly the same > length. Good memory :-) You are right, I double-checked to confirm it. I'll remove the strlen check.
On Wed, 7 Aug 2019, Julien Grall wrote: > Hi Stefano, > > On 06/08/2019 22:49, 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> > > > > --- > > Changes in v4: > > - depth + 1 in process_reserved_memory_node > > Ah, you fixed it in this patch. But then, this does not match the > documentation in patch #1. Yes good point, see below > > - pass address_cells and size_cells to device_tree_for_each_node > > - pass struct meminfo * instead of a boolean to process_memory_node > > - improve in-code comment > > I can't see any comment, is that an improvement? :) It got lost with the refactoring of the code, but I don't think we need it anymore > > - use a separate process_reserved_memory_node (separate from > > process_memory_node) function wrapper to have different error handling > > > > 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 | 43 +++++++++++++++++++++++++++++++------ > > xen/include/asm-arm/setup.h | 1 + > > 2 files changed, 38 insertions(+), 6 deletions(-) > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > index c22d57cd72..3e6fd63b16 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -144,6 +144,7 @@ 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 = (struct meminfo *)data; > > The cast is unnecessary. > > The rest of the code looks good. Pending the discussion about > device_tree_for_each_node: > > Acked-by: Julien Grall <julien.grall@arm.com> Thank you. I removed the cast. Also, I think that it makes more sense to do the depth increase (depth + 1) inside the implementation of device_tree_for_each_node instead of at the caller site, like it is done in this patch. This would match the documentation better and is cleaner from an interface point of view. So I'll remove the depth increase from this patch and move it to the first patch (min_depth = depth + 1). Given the change, I won't add the acked-by to give you the chance to give it another look.
On Fri, 9 Aug 2019, 23:21 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Wed, 7 Aug 2019, Julien Grall wrote: > > Hi Stefano, > > > > On 06/08/2019 22:49, 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> > > > > > > --- > > > Changes in v4: > > > - depth + 1 in process_reserved_memory_node > > > > Ah, you fixed it in this patch. But then, this does not match the > > documentation in patch #1. > > Yes good point, see below > > > > > - pass address_cells and size_cells to device_tree_for_each_node > > > - pass struct meminfo * instead of a boolean to process_memory_node > > > - improve in-code comment > > > > I can't see any comment, is that an improvement? :) > > It got lost with the refactoring of the code, but I don't think we need > it anymore > > > > > - use a separate process_reserved_memory_node (separate from > > > process_memory_node) function wrapper to have different error > handling > > > > > > 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 | 43 > +++++++++++++++++++++++++++++++------ > > > xen/include/asm-arm/setup.h | 1 + > > > 2 files changed, 38 insertions(+), 6 deletions(-) > > > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > > index c22d57cd72..3e6fd63b16 100644 > > > --- a/xen/arch/arm/bootfdt.c > > > +++ b/xen/arch/arm/bootfdt.c > > > @@ -144,6 +144,7 @@ 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 = (struct meminfo *)data; > > > > The cast is unnecessary. > > > > The rest of the code looks good. Pending the discussion about > > device_tree_for_each_node: > > > > Acked-by: Julien Grall <julien.grall@arm.com> > > Thank you. I removed the cast. Also, I think that it makes more sense to > do the depth increase (depth + 1) inside the implementation of > device_tree_for_each_node instead of at the caller site, like it is done > in this patch. This would match the documentation better and is cleaner > from an interface point of view. So I'll remove the depth increase from > this patch and move it to the first patch (min_depth = depth + 1). > Well, you don't need to pass the depth at all. It is just an artificial number for libfdt to know were to stop. We also don't need the absolute depth in any of the early FDT. The relative one is sufficient. > Given the change, I won't add the acked-by to give you the chance to > give it another look. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On Sat, 10 Aug 2019, Julien Grall wrote: > On Fri, 9 Aug 2019, 23:21 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Wed, 7 Aug 2019, Julien Grall wrote: > > Hi Stefano, > > > > On 06/08/2019 22:49, 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> > > > > > > --- > > > Changes in v4: > > > - depth + 1 in process_reserved_memory_node > > > > Ah, you fixed it in this patch. But then, this does not match the > > documentation in patch #1. > > Yes good point, see below > > > > > - pass address_cells and size_cells to device_tree_for_each_node > > > - pass struct meminfo * instead of a boolean to process_memory_node > > > - improve in-code comment > > > > I can't see any comment, is that an improvement? :) > > It got lost with the refactoring of the code, but I don't think we need > it anymore > > > > > - use a separate process_reserved_memory_node (separate from > > > process_memory_node) function wrapper to have different error handling > > > > > > 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 | 43 +++++++++++++++++++++++++++++++------ > > > xen/include/asm-arm/setup.h | 1 + > > > 2 files changed, 38 insertions(+), 6 deletions(-) > > > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > > index c22d57cd72..3e6fd63b16 100644 > > > --- a/xen/arch/arm/bootfdt.c > > > +++ b/xen/arch/arm/bootfdt.c > > > @@ -144,6 +144,7 @@ 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 = (struct meminfo *)data; > > > > The cast is unnecessary. > > > > The rest of the code looks good. Pending the discussion about > > device_tree_for_each_node: > > > > Acked-by: Julien Grall <julien.grall@arm.com> > > Thank you. I removed the cast. Also, I think that it makes more sense to > do the depth increase (depth + 1) inside the implementation of > device_tree_for_each_node instead of at the caller site, like it is done > in this patch. This would match the documentation better and is cleaner > from an interface point of view. So I'll remove the depth increase from > this patch and move it to the first patch (min_depth = depth + 1). > > > Well, you don't need to pass the depth at all. It is just an artificial number for libfdt to know were to stop. > > We also don't need the absolute depth in any of the early FDT. The relative one is sufficient. Yes, you are right, good suggestion
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index c22d57cd72..3e6fd63b16 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -144,6 +144,7 @@ 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 = (struct meminfo *)data; if ( address_cells < 1 || size_cells < 1 ) { @@ -159,21 +160,47 @@ static int __init process_memory_node(const void *fdt, int node, 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++; } - if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) + if ( mem->nr_banks == NR_MEM_BANKS ) return -ENOSPC; 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, + void *data) +{ + int rc = process_memory_node(fdt, node, name, depth, address_cells, + size_cells, data); + + if ( rc == -ENOSPC ) + panic("Max number of supported reserved-memory regions reached."); + else if ( rc != -ENOENT ) + return rc; + return 0; +} + +static int __init process_reserved_memory(const void *fdt, int node, + const char *name, int depth, + u32 address_cells, u32 size_cells) +{ + return device_tree_for_each_node(fdt, node, depth + 1, + address_cells, size_cells, + process_reserved_memory_node, + &bootinfo.reserved_mem); +} + static void __init process_multiboot_node(const void *fdt, int node, const char *name, u32 address_cells, u32 size_cells) @@ -307,7 +334,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, &bootinfo.mem); + else if ( depth == 1 && !strcmp(name, "reserved-memory") && + strlen(name) == strlen("reserved-memory") ) + rc = process_reserved_memory(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> --- Changes in v4: - depth + 1 in process_reserved_memory_node - pass address_cells and size_cells to device_tree_for_each_node - pass struct meminfo * instead of a boolean to process_memory_node - improve in-code comment - use a separate process_reserved_memory_node (separate from process_memory_node) function wrapper to have different error handling 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 | 43 +++++++++++++++++++++++++++++++------ xen/include/asm-arm/setup.h | 1 + 2 files changed, 38 insertions(+), 6 deletions(-)