Re: [Xen-devel] [PATCH 4/6] xen/arm: keep track of reserved-memory regions

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 2/26/19 11:07 PM, 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.)
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
>   xen/arch/arm/bootfdt.c      | 73 +++++++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/setup.h |  1 +
>   2 files changed, 74 insertions(+)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 44af11c..a86b1b3 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -163,6 +163,76 @@ static void __init process_memory_node(const void *fdt, 
> int node,
>       }
>   }
>   
> +static void __init process_reserved_memory_node(const void *fdt,
> +                                                int node,
> +                                                int depth,
> +                                                const char *name,
> +                                                u32 as,
> +                                                u32 ss)
> +{
> +    const struct fdt_property *prop;
> +    int i;
> +    int banks;
> +    const __be32 *cell;
> +    paddr_t start, size;
> +    u32 reg_cells;
> +    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> +    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> +
> +    address_cells[depth] = as;
> +    size_cells[depth] = ss;
> +    node = fdt_next_node(fdt, node, &depth);
> +
> +    for ( ; node >= 0 && depth > 1;
> +            node = fdt_next_node(fdt, node, &depth) )
> +    {
Please rework device_for_each_node to allow take a depth and node in 
parameter.
> +        name = fdt_get_name(fdt, node, NULL); > +
> +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> +        {
> +            printk("Warning: device tree node `%s' is nested too deep\n",
> +                   name);
> +            continue;
> +        }
> +
> +        address_cells[depth] = device_tree_get_u32(fdt, node,
> +                                                   "#address-cells",
> +                                                   address_cells[depth-1]);
> +        size_cells[depth] = device_tree_get_u32(fdt, node,
> +                                                "#size-cells",
> +                                                size_cells[depth-1]);
> +        if ( address_cells[depth-1] < 1 || size_cells[depth-1] < 1 )
Why this check? 0 is a valid value for "#address-cells" and "#size-cells".

> +        {
> +            printk("fdt: node `%s': invalid #address-cells or #size-cells",
> +                    name);
> +            continue;
> +        }
> +
> +        prop = fdt_get_property(fdt, node, "reg", NULL);
> +        if ( !prop )
> +        {
> +            printk("fdt: node `%s': missing `reg' property\n", name);
> +            continue;
> +        }
The property "reg" is not mandatory for a reserved-memory region. Also 
looking at the code, this looks very similar to how we deal with memory 
nodes. Can you please rework the code to avoid duplication?
> +
> +        reg_cells = address_cells[depth-1] + size_cells[depth-1];
> +        cell = (const __be32 *)prop->data;
> +        banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> +
> +        for ( i = 0; i < banks && bootinfo.reserved_mem.nr_banks < 
> NR_MEM_BANKS; i++ )
There are few problems with this code itself. For a first, you will 
ignore the reserved-memory if there are no more space. This means that 
Xen will be free to allocate the memory for a guest.
I don't think we want that. Note that it is fine to ignore memory banks 
(see process_memory_node) as nothing bad could happen.
Furthermore, while today we would allow up to 128 reserved-memory 
region. I think this is pretty fragile as Xen would break if the DT 
contains more than that. But we can cross that later one.
Lastly, we start already have different way to deal with memory, acpi 
tables, modules. Now you are adding the reserved-memory.
I think it is time for us to create something like e820 internally. This 
would simplify greatly the setup_mm code as we would not need to go 
through all the "reserved" block (modules,...) to check intersection.
Cheers,

--
Julien Grall

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