| Message ID | 20190812222844.9636-3-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/7] xen/arm: pass node to device_tree_for_each_node | expand |
Stefano Stabellini writes: > 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 v5: > - remove unneeded cast > - remove unneeded strlen check > - don't pass address_cells, size_cells, depth to device_tree_for_each_node > > 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 | 41 +++++++++++++++++++++++++++++++------ > xen/include/asm-arm/setup.h | 1 + > 2 files changed, 36 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 590b14304c..0b0e22a3d0 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -136,6 +136,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 = data; > > if ( address_cells < 1 || size_cells < 1 ) > return -ENOENT; > @@ -147,21 +148,46 @@ 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++ ) What is logic behind the second part of the loop condition? You know that if (banks > NR_MEM_BANKS) then you will exit with error. Do you really need to iterate over loop in this case? > { > 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 ) Looks like you have the same off-by-one error, as in previous patch. I can see that it was there earlier. But it is good time to fix it. > 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, > + 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) > @@ -295,7 +321,10 @@ 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") ) I believe you want to use dt_node_cmp() there. > + 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 -- Volodymyr Babchuk at EPAM
Hi, On 8/13/19 3:23 PM, Volodymyr Babchuk wrote: > > Stefano Stabellini writes: > >> 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 v5: >> - remove unneeded cast >> - remove unneeded strlen check >> - don't pass address_cells, size_cells, depth to device_tree_for_each_node >> >> 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 | 41 +++++++++++++++++++++++++++++++------ >> xen/include/asm-arm/setup.h | 1 + >> 2 files changed, 36 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >> index 590b14304c..0b0e22a3d0 100644 >> --- a/xen/arch/arm/bootfdt.c >> +++ b/xen/arch/arm/bootfdt.c >> @@ -136,6 +136,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 = data; >> >> if ( address_cells < 1 || size_cells < 1 ) >> return -ENOENT; >> @@ -147,21 +148,46 @@ 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++ ) > What is logic behind the second part of the loop condition? > > You know that if (banks > NR_MEM_BANKS) then you will exit with error. Do > you really need to iterate over loop in this case? Well, the error is ignored in the case of memory banks. So iterating over the first banks allows you to fill up bootinfo with as much as possible as RAM. The rest of the RAM will not be used by Xen. > >> { >> 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 ) > Looks like you have the same off-by-one error, as in previous patch. > I can see that it was there earlier. But it is good time to fix it. I don't think there was an off-by-one error before this series. So what do you mean? Cheers,
Hi Julien, Julien Grall writes: > Hi, > > On 8/13/19 3:23 PM, Volodymyr Babchuk wrote: >> >> Stefano Stabellini writes: >> >>> 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 v5: >>> - remove unneeded cast >>> - remove unneeded strlen check >>> - don't pass address_cells, size_cells, depth to device_tree_for_each_node >>> >>> 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 | 41 +++++++++++++++++++++++++++++++------ >>> xen/include/asm-arm/setup.h | 1 + >>> 2 files changed, 36 insertions(+), 6 deletions(-) >>> >>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >>> index 590b14304c..0b0e22a3d0 100644 >>> --- a/xen/arch/arm/bootfdt.c >>> +++ b/xen/arch/arm/bootfdt.c >>> @@ -136,6 +136,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 = data; >>> >>> if ( address_cells < 1 || size_cells < 1 ) >>> return -ENOENT; >>> @@ -147,21 +148,46 @@ 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++ ) >> What is logic behind the second part of the loop condition? >> >> You know that if (banks > NR_MEM_BANKS) then you will exit with error. Do >> you really need to iterate over loop in this case? > > Well, the error is ignored in the case of memory banks. So iterating > over the first banks allows you to fill up bootinfo with as much as > possible as RAM. The rest of the RAM will not be used by Xen. Fair enough. >> >>> { >>> 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 ) >> Looks like you have the same off-by-one error, as in previous patch. >> I can see that it was there earlier. But it is good time to fix it. > > I don't think there was an off-by-one error before this series. So > what do you mean? I explained this in patch #2. Imagine that NR_MEM_BANKS = 1 and you have one memory node in the dtb. You'll fill the first element of the array and mem->nr_banks will become 1. This is absolutely normal. But check above will fail, which is not right.
Hi, On 8/13/19 4:14 PM, Volodymyr Babchuk wrote: > Julien Grall writes: >> On 8/13/19 3:23 PM, Volodymyr Babchuk wrote: >>> Stefano Stabellini writes: >>> >>>> { >>>> 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 ) >>> Looks like you have the same off-by-one error, as in previous patch. >>> I can see that it was there earlier. But it is good time to fix it. >> >> I don't think there was an off-by-one error before this series. So >> what do you mean? > I explained this in patch #2. Imagine that NR_MEM_BANKS = 1 and you have > one memory node in the dtb. You'll fill the first element of the array > and mem->nr_banks will become 1. This is absolutely normal. But check > above will fail, which is not right. Ok. So the off-by-one error has been introduced by this series. So this should be fixed in patch #2 not here. Cheers,
Julien Grall writes: > Hi, > > On 8/13/19 4:14 PM, Volodymyr Babchuk wrote: >> Julien Grall writes: >>> On 8/13/19 3:23 PM, Volodymyr Babchuk wrote: >>>> Stefano Stabellini writes: >>>> >>>>> { >>>>> 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 ) >>>> Looks like you have the same off-by-one error, as in previous patch. >>>> I can see that it was there earlier. But it is good time to fix it. >>> >>> I don't think there was an off-by-one error before this series. So >>> what do you mean? >> I explained this in patch #2. Imagine that NR_MEM_BANKS = 1 and you have >> one memory node in the dtb. You'll fill the first element of the array >> and mem->nr_banks will become 1. This is absolutely normal. But check >> above will fail, which is not right. > > Ok. So the off-by-one error has been introduced by this series. So > this should be fixed in patch #2 not here. Yes, sorry. I got lost in the code.
Hi Stefano, On 12/08/2019 23:28, 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 v5: > - remove unneeded cast > - remove unneeded strlen check > - don't pass address_cells, size_cells, depth to device_tree_for_each_node > > 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 | 41 +++++++++++++++++++++++++++++++------ > xen/include/asm-arm/setup.h | 1 + > 2 files changed, 36 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 590b14304c..0b0e22a3d0 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -136,6 +136,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 = data; > > if ( address_cells < 1 || size_cells < 1 ) > return -ENOENT; > @@ -147,21 +148,46 @@ 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 ) Assuming the off-by-one is going to be fixed in patch #2: Acked-by: Julien Grall <julien.grall@arm.com> Cheers,
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 590b14304c..0b0e22a3d0 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -136,6 +136,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 = data; if ( address_cells < 1 || size_cells < 1 ) return -ENOENT; @@ -147,21 +148,46 @@ 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, + 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) @@ -295,7 +321,10 @@ 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") ) + 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 v5: - remove unneeded cast - remove unneeded strlen check - don't pass address_cells, size_cells, depth to device_tree_for_each_node 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 | 41 +++++++++++++++++++++++++++++++------ xen/include/asm-arm/setup.h | 1 + 2 files changed, 36 insertions(+), 6 deletions(-)