| Message ID | 20190812222844.9636-2-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 |
Hi Stefano, Stefano Stabellini writes: > Change the signature of process_memory_node to match > device_tree_node_func. Thanks to this change, the next patch will be > able to use device_tree_for_each_node to call process_memory_node on all > the children of a provided node. > > Return error if there is no reg property or if nr_banks is reached. Let > the caller deal with the error. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > --- > Changes in v5: > - return -ENOENT if address_cells or size_cells are not properly set > > Changes in v4: > - return error if there is no reg propery, remove printk > - return error if nr_banks is reached > > Changes in v3: > - improve commit message > - check return value of process_memory_node > > Changes in v2: > - new > --- > xen/arch/arm/bootfdt.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index a872ea57d6..590b14304c 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt, int node, > return 0; > } > > -static void __init process_memory_node(const void *fdt, int node, > - const char *name, > - u32 address_cells, u32 size_cells) > +static int __init process_memory_node(const void *fdt, int node, > + const char *name, int depth, > + u32 address_cells, u32 size_cells, > + void *data) > { > const struct fdt_property *prop; > int i; > @@ -137,18 +138,11 @@ static void __init process_memory_node(const void *fdt, int node, > u32 reg_cells = address_cells + size_cells; > > if ( address_cells < 1 || size_cells < 1 ) > - { > - printk("fdt: node `%s': invalid #address-cells or #size-cells", > - name); > - return; > - } > + return -ENOENT; > > prop = fdt_get_property(fdt, node, "reg", NULL); > if ( !prop ) > - { > - printk("fdt: node `%s': missing `reg' property\n", name); > - return; > - } > + return -ENOENT; > > cell = (const __be32 *)prop->data; > banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); > @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node, > bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; > bootinfo.mem.nr_banks++; > } > + > + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) > + return -ENOSPC; Are you sure that this logic is correct? For example, if NR_MEM_BANKS is 1, and we have exactly one memory node in device tree, this function will fail. But it should not. I think you want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS > + return 0; > } > > static void __init process_multiboot_node(const void *fdt, int node, > @@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt, > u32 address_cells, u32 size_cells, > void *data) > { > + int rc = 0; > + > if ( device_tree_node_matches(fdt, node, "memory") ) > - process_memory_node(fdt, node, name, address_cells, size_cells); > + rc = process_memory_node(fdt, node, name, depth, > + address_cells, size_cells, NULL); > 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); > else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") ) > process_chosen_node(fdt, node, name, address_cells, size_cells); > > - return 0; > + return rc; > } > > static void __init early_print_info(void) -- Volodymyr Babchuk at EPAM
Hi, On 8/12/19 11:28 PM, Stefano Stabellini wrote: > Change the signature of process_memory_node to match > device_tree_node_func. Thanks to this change, the next patch will be > able to use device_tree_for_each_node to call process_memory_node on all > the children of a provided node. > > Return error if there is no reg property or if nr_banks is reached. Let > the caller deal with the error. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > --- > Changes in v5: > - return -ENOENT if address_cells or size_cells are not properly set > > Changes in v4: > - return error if there is no reg propery, remove printk > - return error if nr_banks is reached > > Changes in v3: > - improve commit message > - check return value of process_memory_node > > Changes in v2: > - new > --- > xen/arch/arm/bootfdt.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index a872ea57d6..590b14304c 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt, int node, > return 0; > } > > -static void __init process_memory_node(const void *fdt, int node, > - const char *name, > - u32 address_cells, u32 size_cells) > +static int __init process_memory_node(const void *fdt, int node, > + const char *name, int depth, > + u32 address_cells, u32 size_cells, > + void *data) > { > const struct fdt_property *prop; > int i; > @@ -137,18 +138,11 @@ static void __init process_memory_node(const void *fdt, int node, > u32 reg_cells = address_cells + size_cells; > > if ( address_cells < 1 || size_cells < 1 ) > - { > - printk("fdt: node `%s': invalid #address-cells or #size-cells", > - name); > - return; > - } > + return -ENOENT; I saw your answer on the previous version and didn't get the chance. Missing the "regs" property and wrong {address,size}-cells values are two different things. In the former case, it just means the reserved-region will be allocated dynamically. In the latter case, this is an error in the device-tree configuration. If you ignore it, you are at risk to not take into account a reserved-region. I agree this is a bug in the Device-Tree, but doing basic sanity check for the users is always helpful. With that in mind, I would keep the printk and return a different error here. > > prop = fdt_get_property(fdt, node, "reg", NULL); > if ( !prop ) > - { > - printk("fdt: node `%s': missing `reg' property\n", name); > - return; > - } > + return -ENOENT; > > cell = (const __be32 *)prop->data; > banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); > @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node, > bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; > bootinfo.mem.nr_banks++; > } > + > + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) > + return -ENOSPC; > + return 0; > } > > static void __init process_multiboot_node(const void *fdt, int node, > @@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt, > u32 address_cells, u32 size_cells, > void *data) > { > + int rc = 0; > + > if ( device_tree_node_matches(fdt, node, "memory") ) > - process_memory_node(fdt, node, name, address_cells, size_cells); > + rc = process_memory_node(fdt, node, name, depth, > + address_cells, size_cells, NULL); As a small NIT there are no way for the user to know that the parsing failed because of the reg property were missing. Could you print an error message either here or maybe in device_tree_for_each() to say parsing as failed for node N? > 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); > else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") ) > process_chosen_node(fdt, node, name, address_cells, size_cells); > > - return 0; > + return rc; > } > > static void __init early_print_info(void) > Cheers,
On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: > > @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node, > > bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; > > bootinfo.mem.nr_banks++; > > } > > + > > + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) > > + return -ENOSPC; > Are you sure that this logic is correct? > > For example, if NR_MEM_BANKS is 1, and we have exactly one memory node > in device tree, this function will fail. But it should not. I think you > want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device tree the code would return an error while actually it is normal. I think the right check would be: if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) return -ENOSPC; (That's because it is impossible to get to nr_banks > NR_MEM_BANKS.)
On Tue, 13 Aug 2019, Julien Grall wrote: > On 8/12/19 11:28 PM, Stefano Stabellini wrote: > > Change the signature of process_memory_node to match > > device_tree_node_func. Thanks to this change, the next patch will be > > able to use device_tree_for_each_node to call process_memory_node on all > > the children of a provided node. > > > > Return error if there is no reg property or if nr_banks is reached. Let > > the caller deal with the error. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > > Changes in v5: > > - return -ENOENT if address_cells or size_cells are not properly set > > > > Changes in v4: > > - return error if there is no reg propery, remove printk > > - return error if nr_banks is reached > > > > Changes in v3: > > - improve commit message > > - check return value of process_memory_node > > > > Changes in v2: > > - new > > --- > > xen/arch/arm/bootfdt.c | 29 +++++++++++++++-------------- > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > index a872ea57d6..590b14304c 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt, > > int node, > > return 0; > > } > > -static void __init process_memory_node(const void *fdt, int node, > > - const char *name, > > - u32 address_cells, u32 size_cells) > > +static int __init process_memory_node(const void *fdt, int node, > > + const char *name, int depth, > > + u32 address_cells, u32 size_cells, > > + void *data) > > { > > const struct fdt_property *prop; > > int i; > > @@ -137,18 +138,11 @@ static void __init process_memory_node(const void > > *fdt, int node, > > u32 reg_cells = address_cells + size_cells; > > if ( address_cells < 1 || size_cells < 1 ) > > - { > > - printk("fdt: node `%s': invalid #address-cells or #size-cells", > > - name); > > - return; > > - } > > + return -ENOENT; > > I saw your answer on the previous version and didn't get the chance. Missing > the "regs" property and wrong {address,size}-cells values are two different > things. > > In the former case, it just means the reserved-region will be allocated > dynamically. > > In the latter case, this is an error in the device-tree configuration. If you > ignore it, you are at risk to not take into account a reserved-region. > > I agree this is a bug in the Device-Tree, but doing basic sanity check for the > users is always helpful. > > With that in mind, I would keep the printk and return a different error here. OK. I'll use -EINVAL for this case, and keep the printk. > > prop = fdt_get_property(fdt, node, "reg", NULL); > > if ( !prop ) > > - { > > - printk("fdt: node `%s': missing `reg' property\n", name); > > - return; > > - } > > + return -ENOENT; > > cell = (const __be32 *)prop->data; > > banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); > > @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, > > int node, > > bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; > > bootinfo.mem.nr_banks++; > > } > > + > > + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) > > + return -ENOSPC; > > + return 0; > > } > > static void __init process_multiboot_node(const void *fdt, int node, > > @@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt, > > u32 address_cells, u32 size_cells, > > void *data) > > { > > + int rc = 0; > > + > > if ( device_tree_node_matches(fdt, node, "memory") ) > > - process_memory_node(fdt, node, name, address_cells, size_cells); > > + rc = process_memory_node(fdt, node, name, depth, > > + address_cells, size_cells, NULL); > > As a small NIT there are no way for the user to know that the parsing failed > because of the reg property were missing. Could you print an error message > either here or maybe in device_tree_for_each() to say parsing as failed for > node N? I'll add a new printk at the end of early_scan_node > > 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); > > else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") > > ) > > process_chosen_node(fdt, node, name, address_cells, size_cells); > > - return 0; > > + return rc; > > } > > static void __init early_print_info(void)
Hi Stefano, On 14/08/2019 23:35, Stefano Stabellini wrote: > On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: >>> @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node, >>> bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>> bootinfo.mem.nr_banks++; >>> } >>> + >>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>> + return -ENOSPC; >> Are you sure that this logic is correct? >> >> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node >> in device tree, this function will fail. But it should not. I think you >> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS > > You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device > tree the code would return an error while actually it is normal. > > I think the right check would be: > > if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) > return -ENOSPC; FWIW, this check looks good to me. Cheers,
Hi Stefano, Stefano Stabellini writes: > On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: >> > @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node, >> > bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >> > bootinfo.mem.nr_banks++; >> > } >> > + >> > + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) >> > + return -ENOSPC; >> Are you sure that this logic is correct? >> >> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node >> in device tree, this function will fail. But it should not. I think you >> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS > > You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device > tree the code would return an error while actually it is normal. > > I think the right check would be: > > if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) > return -ENOSPC; Actually, this does not cover all corner cases. Here is the resulting code: 150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) 151 { 152 device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); 153 if ( !size ) 154 continue; 155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; 156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; 157 bootinfo.mem.nr_banks++; 158 } 159 160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) 161 return -ENOSPC; Lines 153-154 cause the issue. Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device tree with. Nodes have sizes 0 and 1024. Your code will work as intended. At the end of loop we will have banks = 2, i = 2 and bootinfo.mem.nr_banks = 1. But if we switch order of memory nodes, so first one will be with size 1024 and second one with size 0, your code will return -ENOSPC, because we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1. I think, right solution will be to scan all nodes to count nodes with size > 0. And then - either return an error or do second loop to fill bootinfo.mem.bank[]. > > (That's because it is impossible to get to nr_banks > NR_MEM_BANKS.) Yes, this is my mistake. -- Volodymyr Babchuk at EPAM
Hi Volodymyr, On 15/08/2019 12:20, Volodymyr Babchuk wrote: > > Hi Stefano, > > Stefano Stabellini writes: > >> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: >>>> @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node, >>>> bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>> bootinfo.mem.nr_banks++; >>>> } >>>> + >>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>> + return -ENOSPC; >>> Are you sure that this logic is correct? >>> >>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node >>> in device tree, this function will fail. But it should not. I think you >>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS >> >> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device >> tree the code would return an error while actually it is normal. >> >> I think the right check would be: >> >> if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >> return -ENOSPC; > > Actually, this does not cover all corner cases. Here is the resulting > code: > > 150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) > 151 { > 152 device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > 153 if ( !size ) > 154 continue; > 155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; > 156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; > 157 bootinfo.mem.nr_banks++; > 158 } > 159 > 160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) > 161 return -ENOSPC; > > Lines 153-154 cause the issue. > > Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device > tree with. Nodes have sizes 0 and 1024. Your code will work as > intended. At the end of loop we will have banks = 2, i = 2 and > bootinfo.mem.nr_banks = 1. > > But if we switch order of memory nodes, so first one will be with size > 1024 and second one with size 0, your code will return -ENOSPC, because > we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1. > > I think, right solution will be to scan all nodes to count nodes > with size > 0. And then - either return an error or do second loop to > fill bootinfo.mem.bank[]. To be honest, a memory with size 0 is an error in the DT provided. So I would not care too much if Xen is not working as intended. If we want to fix this, then we should bail out as we do for missing 'regs' and invalid 'address-cells'/'size-cells'. Cheers,
On 15/08/2019 12:24, Julien Grall wrote: > Hi Volodymyr, > > On 15/08/2019 12:20, Volodymyr Babchuk wrote: >> >> Hi Stefano, >> >> Stefano Stabellini writes: >> >>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: >>>>> @@ -162,6 +156,10 @@ static void __init process_memory_node(const void >>>>> *fdt, int node, >>>>> bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>>> bootinfo.mem.nr_banks++; >>>>> } >>>>> + >>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>> + return -ENOSPC; >>>> Are you sure that this logic is correct? >>>> >>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node >>>> in device tree, this function will fail. But it should not. I think you >>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS >>> >>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device >>> tree the code would return an error while actually it is normal. >>> >>> I think the right check would be: >>> >>> if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>> return -ENOSPC; >> >> Actually, this does not cover all corner cases. Here is the resulting >> code: >> >> 150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) >> 151 { >> 152 device_tree_get_reg(&cell, address_cells, size_cells, &start, >> &size); >> 153 if ( !size ) >> 154 continue; >> 155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; >> 156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >> 157 bootinfo.mem.nr_banks++; >> 158 } >> 159 >> 160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >> 161 return -ENOSPC; >> >> Lines 153-154 cause the issue. >> >> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device >> tree with. Nodes have sizes 0 and 1024. Your code will work as >> intended. At the end of loop we will have banks = 2, i = 2 and >> bootinfo.mem.nr_banks = 1. >> >> But if we switch order of memory nodes, so first one will be with size >> 1024 and second one with size 0, your code will return -ENOSPC, because >> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1. >> >> I think, right solution will be to scan all nodes to count nodes >> with size > 0. And then - either return an error or do second loop to >> fill bootinfo.mem.bank[]. > > To be honest, a memory with size 0 is an error in the DT provided. So I would > not care too much if Xen is not working as intended. > > If we want to fix this, then we should bail out as we do for missing 'regs' and > invalid 'address-cells'/'size-cells'. I send this too early. I forgot to mention that I would not be happy with parsing the Device-Tree twice just for benefits of wrong DT. If a DT is wrong then we should treat as such and shout at the user. Repairing any wrong inputs should be done on best efforts. Cheers,
Hi Julien, Julien Grall writes: > On 15/08/2019 12:24, Julien Grall wrote: >> Hi Volodymyr, >> >> On 15/08/2019 12:20, Volodymyr Babchuk wrote: >>> >>> Hi Stefano, >>> >>> Stefano Stabellini writes: >>> >>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: >>>>>> @@ -162,6 +156,10 @@ static void __init >>>>>> process_memory_node(const void *fdt, int node, >>>>>> bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>>>> bootinfo.mem.nr_banks++; >>>>>> } >>>>>> + >>>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>>> + return -ENOSPC; >>>>> Are you sure that this logic is correct? >>>>> >>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node >>>>> in device tree, this function will fail. But it should not. I think you >>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS >>>> >>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device >>>> tree the code would return an error while actually it is normal. >>>> >>>> I think the right check would be: >>>> >>>> if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>> return -ENOSPC; >>> >>> Actually, this does not cover all corner cases. Here is the resulting >>> code: >>> >>> 150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) >>> 151 { >>> 152 device_tree_get_reg(&cell, address_cells, size_cells, >>> &start, &size); >>> 153 if ( !size ) >>> 154 continue; >>> 155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; >>> 156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>> 157 bootinfo.mem.nr_banks++; >>> 158 } >>> 159 >>> 160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>> 161 return -ENOSPC; >>> >>> Lines 153-154 cause the issue. >>> >>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device >>> tree with. Nodes have sizes 0 and 1024. Your code will work as >>> intended. At the end of loop we will have banks = 2, i = 2 and >>> bootinfo.mem.nr_banks = 1. >>> >>> But if we switch order of memory nodes, so first one will be with size >>> 1024 and second one with size 0, your code will return -ENOSPC, because >>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1. >>> >>> I think, right solution will be to scan all nodes to count nodes >>> with size > 0. And then - either return an error or do second loop to >>> fill bootinfo.mem.bank[]. >> >> To be honest, a memory with size 0 is an error in the DT >> provided. So I would not care too much if Xen is not working as >> intended. >> >> If we want to fix this, then we should bail out as we do for missing >> 'regs' and invalid 'address-cells'/'size-cells'. > > I send this too early. I forgot to mention that I would not be happy > with parsing the Device-Tree twice just for benefits of wrong DT. If a > DT is wrong then we should treat as such and shout at the user. Fair enough. But then at line 154 we need to return an error, instead of continuing the iterations. And in this case we can simplify the error check to (banks > NR_MEM_BANKS).
Hi Volodymyr, On 15/08/2019 13:14, Volodymyr Babchuk wrote: > Julien Grall writes: > >> On 15/08/2019 12:24, Julien Grall wrote: >>> Hi Volodymyr, >>> >>> On 15/08/2019 12:20, Volodymyr Babchuk wrote: >>>> >>>> Hi Stefano, >>>> >>>> Stefano Stabellini writes: >>>> >>>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: >>>>>>> @@ -162,6 +156,10 @@ static void __init >>>>>>> process_memory_node(const void *fdt, int node, >>>>>>> bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>>>>> bootinfo.mem.nr_banks++; >>>>>>> } >>>>>>> + >>>>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>>>> + return -ENOSPC; >>>>>> Are you sure that this logic is correct? >>>>>> >>>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node >>>>>> in device tree, this function will fail. But it should not. I think you >>>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS >>>>> >>>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device >>>>> tree the code would return an error while actually it is normal. >>>>> >>>>> I think the right check would be: >>>>> >>>>> if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>> return -ENOSPC; >>>> >>>> Actually, this does not cover all corner cases. Here is the resulting >>>> code: >>>> >>>> 150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) >>>> 151 { >>>> 152 device_tree_get_reg(&cell, address_cells, size_cells, >>>> &start, &size); >>>> 153 if ( !size ) >>>> 154 continue; >>>> 155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; >>>> 156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>> 157 bootinfo.mem.nr_banks++; >>>> 158 } >>>> 159 >>>> 160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>> 161 return -ENOSPC; >>>> >>>> Lines 153-154 cause the issue. >>>> >>>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device >>>> tree with. Nodes have sizes 0 and 1024. Your code will work as >>>> intended. At the end of loop we will have banks = 2, i = 2 and >>>> bootinfo.mem.nr_banks = 1. >>>> >>>> But if we switch order of memory nodes, so first one will be with size >>>> 1024 and second one with size 0, your code will return -ENOSPC, because >>>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1. >>>> >>>> I think, right solution will be to scan all nodes to count nodes >>>> with size > 0. And then - either return an error or do second loop to >>>> fill bootinfo.mem.bank[]. >>> >>> To be honest, a memory with size 0 is an error in the DT >>> provided. So I would not care too much if Xen is not working as >>> intended. >>> >>> If we want to fix this, then we should bail out as we do for missing >>> 'regs' and invalid 'address-cells'/'size-cells'. >> >> I send this too early. I forgot to mention that I would not be happy >> with parsing the Device-Tree twice just for benefits of wrong DT. If a >> DT is wrong then we should treat as such and shout at the user. > Fair enough. But then at line 154 we need to return an error, instead of > continuing the iterations. And in this case we can simplify the error > check to (banks > NR_MEM_BANKS). I am afraid this would not be correct. It is allowed to have multiple memory nodes in the device-tree. This function only deal with one node at the times. In particular banks is the number of regions described in the node. With the check you suggest, you would only catch the case where a node contain more banks than supported. It does not tell you whether there are enough space left in mem.bank[...] to cater the regions described by the node. So we need the check suggested by Stefano. Cheers,
Julien Grall writes: > Hi Volodymyr, > > On 15/08/2019 13:14, Volodymyr Babchuk wrote: >> Julien Grall writes: >> >>> On 15/08/2019 12:24, Julien Grall wrote: >>>> Hi Volodymyr, >>>> >>>> On 15/08/2019 12:20, Volodymyr Babchuk wrote: >>>>> >>>>> Hi Stefano, >>>>> >>>>> Stefano Stabellini writes: >>>>> >>>>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: >>>>>>>> @@ -162,6 +156,10 @@ static void __init >>>>>>>> process_memory_node(const void *fdt, int node, >>>>>>>> bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>>>>>> bootinfo.mem.nr_banks++; >>>>>>>> } >>>>>>>> + >>>>>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>>>>> + return -ENOSPC; >>>>>>> Are you sure that this logic is correct? >>>>>>> >>>>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node >>>>>>> in device tree, this function will fail. But it should not. I think you >>>>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS >>>>>> >>>>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device >>>>>> tree the code would return an error while actually it is normal. >>>>>> >>>>>> I think the right check would be: >>>>>> >>>>>> if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>>> return -ENOSPC; >>>>> >>>>> Actually, this does not cover all corner cases. Here is the resulting >>>>> code: >>>>> >>>>> 150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) >>>>> 151 { >>>>> 152 device_tree_get_reg(&cell, address_cells, size_cells, >>>>> &start, &size); >>>>> 153 if ( !size ) >>>>> 154 continue; >>>>> 155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; >>>>> 156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>>> 157 bootinfo.mem.nr_banks++; >>>>> 158 } >>>>> 159 >>>>> 160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>> 161 return -ENOSPC; >>>>> >>>>> Lines 153-154 cause the issue. >>>>> >>>>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device >>>>> tree with. Nodes have sizes 0 and 1024. Your code will work as >>>>> intended. At the end of loop we will have banks = 2, i = 2 and >>>>> bootinfo.mem.nr_banks = 1. >>>>> >>>>> But if we switch order of memory nodes, so first one will be with size >>>>> 1024 and second one with size 0, your code will return -ENOSPC, because >>>>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1. >>>>> >>>>> I think, right solution will be to scan all nodes to count nodes >>>>> with size > 0. And then - either return an error or do second loop to >>>>> fill bootinfo.mem.bank[]. >>>> >>>> To be honest, a memory with size 0 is an error in the DT >>>> provided. So I would not care too much if Xen is not working as >>>> intended. >>>> >>>> If we want to fix this, then we should bail out as we do for missing >>>> 'regs' and invalid 'address-cells'/'size-cells'. >>> >>> I send this too early. I forgot to mention that I would not be happy >>> with parsing the Device-Tree twice just for benefits of wrong DT. If a >>> DT is wrong then we should treat as such and shout at the user. >> Fair enough. But then at line 154 we need to return an error, instead of >> continuing the iterations. And in this case we can simplify the error >> check to (banks > NR_MEM_BANKS). > > I am afraid this would not be correct. It is allowed to have multiple > memory nodes in the device-tree. This function only deal with one node > at the times. Okay, I see the point there. > In particular banks is the number of regions described in the > node. With the check you suggest, you would only catch the case where > a node contain more banks than supported. It does not tell you whether > there are enough space left in mem.bank[...] to cater the regions > described by the node. Yes, right. But, we can free space: (banks + bootinfo.mem.nr_banks > NR_MEM_BANKS) > So we need the check suggested by Stefano. As I said earlier, it does not cover all corner cases. It will behave differently, depending on ordering of entries in "reg" property (if we allow zero-length regions). Yes, this is the user's problem, but I think it is better to have consistent behavior even in case of user's fault. But were saying, that it is error to have region with zero length. So, instead of device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); if ( !size ) continue; we need device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); if ( !size ) return -ENOENT; In this case, check suggested by Stefano will work fine, but it will be redundant, because we can either do early check for free space in the array, or just write if ( i < banks ) return -ENOSPC; If we want array to be filled no mater what. Anyways, I don't want to press on this anymore. I just wanted to share my concerns. -- Volodymyr Babchuk at EPAM
On 15/08/2019 14:51, Volodymyr Babchuk wrote: > > Julien Grall writes: > >> Hi Volodymyr, >> >> On 15/08/2019 13:14, Volodymyr Babchuk wrote: >>> Julien Grall writes: >>> >>>> On 15/08/2019 12:24, Julien Grall wrote: >>>>> Hi Volodymyr, >>>>> >>>>> On 15/08/2019 12:20, Volodymyr Babchuk wrote: >>>>>> >>>>>> Hi Stefano, >>>>>> >>>>>> Stefano Stabellini writes: >>>>>> >>>>>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: >>>>>>>>> @@ -162,6 +156,10 @@ static void __init >>>>>>>>> process_memory_node(const void *fdt, int node, >>>>>>>>> bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>>>>>>> bootinfo.mem.nr_banks++; >>>>>>>>> } >>>>>>>>> + >>>>>>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>>>>>> + return -ENOSPC; >>>>>>>> Are you sure that this logic is correct? >>>>>>>> >>>>>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node >>>>>>>> in device tree, this function will fail. But it should not. I think you >>>>>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS >>>>>>> >>>>>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device >>>>>>> tree the code would return an error while actually it is normal. >>>>>>> >>>>>>> I think the right check would be: >>>>>>> >>>>>>> if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>>>> return -ENOSPC; >>>>>> >>>>>> Actually, this does not cover all corner cases. Here is the resulting >>>>>> code: >>>>>> >>>>>> 150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ ) >>>>>> 151 { >>>>>> 152 device_tree_get_reg(&cell, address_cells, size_cells, >>>>>> &start, &size); >>>>>> 153 if ( !size ) >>>>>> 154 continue; >>>>>> 155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start; >>>>>> 156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; >>>>>> 157 bootinfo.mem.nr_banks++; >>>>>> 158 } >>>>>> 159 >>>>>> 160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS ) >>>>>> 161 return -ENOSPC; >>>>>> >>>>>> Lines 153-154 cause the issue. >>>>>> >>>>>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device >>>>>> tree with. Nodes have sizes 0 and 1024. Your code will work as >>>>>> intended. At the end of loop we will have banks = 2, i = 2 and >>>>>> bootinfo.mem.nr_banks = 1. >>>>>> >>>>>> But if we switch order of memory nodes, so first one will be with size >>>>>> 1024 and second one with size 0, your code will return -ENOSPC, because >>>>>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1. >>>>>> >>>>>> I think, right solution will be to scan all nodes to count nodes >>>>>> with size > 0. And then - either return an error or do second loop to >>>>>> fill bootinfo.mem.bank[]. >>>>> >>>>> To be honest, a memory with size 0 is an error in the DT >>>>> provided. So I would not care too much if Xen is not working as >>>>> intended. >>>>> >>>>> If we want to fix this, then we should bail out as we do for missing >>>>> 'regs' and invalid 'address-cells'/'size-cells'. >>>> >>>> I send this too early. I forgot to mention that I would not be happy >>>> with parsing the Device-Tree twice just for benefits of wrong DT. If a >>>> DT is wrong then we should treat as such and shout at the user. >>> Fair enough. But then at line 154 we need to return an error, instead of >>> continuing the iterations. And in this case we can simplify the error >>> check to (banks > NR_MEM_BANKS). >> >> I am afraid this would not be correct. It is allowed to have multiple >> memory nodes in the device-tree. This function only deal with one node >> at the times. > Okay, I see the point there. > >> In particular banks is the number of regions described in the >> node. With the check you suggest, you would only catch the case where >> a node contain more banks than supported. It does not tell you whether >> there are enough space left in mem.bank[...] to cater the regions >> described by the node. > Yes, right. But, we can free space: > > (banks + bootinfo.mem.nr_banks > NR_MEM_BANKS) I guess you mean before the loop? If so, this is possible but then you will ignore the full node rather than trying to add as much regions as possible. To give an exagerated example, imagine a the DT has a single node with NR_MEM_BANKS + 1. You will end up to not add any banks, so Xen will see no memory. This is not very ideal. > >> So we need the check suggested by Stefano. > As I said earlier, it does not cover all corner cases. It will behave > differently, depending on ordering of entries in "reg" property (if we > allow zero-length regions). Yes, this is the user's problem, but I think > it is better to have consistent behavior even in case of user's fault. Where did I say it cover all corner cases? As I said "If a DT is wrong then we should treat as such and shout at the user." > > But were saying, that it is error to have region with zero length. So, > instead of > > device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > if ( !size ) > continue; > > we need > > device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > if ( !size ) > return -ENOENT; > > In this case, check suggested by Stefano will work fine, but it will be > redundant, because we can either do early check for free space in the > array, or just write See above for the early check. > if ( i < banks ) > return -ENOSPC; This is another option for Stefano check. I don't particularly care on the check as long as it is correct. > > If we want array to be filled no mater what. > > Anyways, I don't want to press on this anymore. I just wanted to share > my concerns. You are preaching the converted. However, I have already pointed multiple times that we need to fill the array as much as possible. This is not a user fault but a Xen limitation. So I am not sure why you are pushing for an early check. Cheers,
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index a872ea57d6..590b14304c 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt, int node, return 0; } -static void __init process_memory_node(const void *fdt, int node, - const char *name, - u32 address_cells, u32 size_cells) +static int __init process_memory_node(const void *fdt, int node, + const char *name, int depth, + u32 address_cells, u32 size_cells, + void *data) { const struct fdt_property *prop; int i; @@ -137,18 +138,11 @@ static void __init process_memory_node(const void *fdt, int node, u32 reg_cells = address_cells + size_cells; if ( address_cells < 1 || size_cells < 1 ) - { - printk("fdt: node `%s': invalid #address-cells or #size-cells", - name); - return; - } + return -ENOENT; prop = fdt_get_property(fdt, node, "reg", NULL); if ( !prop ) - { - printk("fdt: node `%s': missing `reg' property\n", name); - return; - } + return -ENOENT; cell = (const __be32 *)prop->data; banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node, bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size; bootinfo.mem.nr_banks++; } + + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS ) + return -ENOSPC; + return 0; } static void __init process_multiboot_node(const void *fdt, int node, @@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt, u32 address_cells, u32 size_cells, void *data) { + int rc = 0; + if ( device_tree_node_matches(fdt, node, "memory") ) - process_memory_node(fdt, node, name, address_cells, size_cells); + rc = process_memory_node(fdt, node, name, depth, + address_cells, size_cells, NULL); 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); else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") ) process_chosen_node(fdt, node, name, address_cells, size_cells); - return 0; + return rc; } static void __init early_print_info(void)
Change the signature of process_memory_node to match device_tree_node_func. Thanks to this change, the next patch will be able to use device_tree_for_each_node to call process_memory_node on all the children of a provided node. Return error if there is no reg property or if nr_banks is reached. Let the caller deal with the error. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- Changes in v5: - return -ENOENT if address_cells or size_cells are not properly set Changes in v4: - return error if there is no reg propery, remove printk - return error if nr_banks is reached Changes in v3: - improve commit message - check return value of process_memory_node Changes in v2: - new --- xen/arch/arm/bootfdt.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)