Re: [Xen-devel] [PATCH v2 08/10] xen/arm: keep track of reserved-memory regions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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

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