Re: [Xen-devel] [PATCH v5 3/7] xen/arm: keep track of reserved-memory regions

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxx>
> >
> > ---
> > 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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel