Re: [PATCH v3 5/9] xen/device-tree: Move Arm's setup.c bootinfo functions to common

[Julien Grall <julien@xxxxxxx>]

Hi Shawn,

On 14/03/2024 22:15, Shawn Anastasio wrote:
> Arm's setup.c contains a collection of functions for parsing memory map
> and other boot information from a device tree. Since these routines are
> generally useful on any architecture that supports device tree booting,
> move them into xen/common/device-tree.
>
> Suggested-by: Julien Grall <julien@xxxxxxx>
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> ---
>   MAINTAINERS                       |   1 +
>   xen/arch/arm/setup.c              | 419 --------------------------
>   xen/common/Makefile               |   1 +
>   xen/common/device-tree/Makefile   |   1 +
>   xen/common/device-tree/bootinfo.c | 469 ++++++++++++++++++++++++++++++
The new bootinfo.c is exported quite a few functions. Please introduce
an generic header with the associated functions/structures.

[...]

> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index e5eee19a85..3a39dd35f2 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_UBSAN) += ubsan/
>   
>   obj-$(CONFIG_NEEDS_LIBELF) += libelf/
>   obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/
> +obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>   
>   CONF_FILE := $(if $(patsubst /%,,$(KCONFIG_CONFIG)),$(objtree)/)$(KCONFIG_CONFIG)
>   $(obj)/config.gz: $(CONF_FILE)
> diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
> new file mode 100644
> index 0000000000..c97b2bd88c
> --- /dev/null
> +++ b/xen/common/device-tree/Makefile
> @@ -0,0 +1 @@
> +obj-y += bootinfo.o
> diff --git a/xen/common/device-tree/bootinfo.c 
> b/xen/common/device-tree/bootinfo.c
> new file mode 100644
> index 0000000000..a6c0fe7917
> --- /dev/null
> +++ b/xen/common/device-tree/bootinfo.c
> @@ -0,0 +1,469 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Derived from $xen/arch/arm/setup.c.
> + *
> + * Early device tree parsing and bookkeeping routines.
> + *
> + * Tim Deegan <tim@xxxxxxx>
> + * Copyright (c) 2011 Citrix Systems.
> + * Copyright (c) 2024 Raptor Engineering LLC
> + */
> +
> +#include <xen/compile.h>
> +#include <xen/errno.h>
> +#include <xen/device_tree.h>
> +#include <xen/domain_page.h>
> +#include <xen/grant_table.h>
> +#include <xen/types.h>
> +#include <xen/string.h>
> +#include <xen/serial.h>
> +#include <xen/sched.h>
> +#include <xen/console.h>
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/irq.h>
> +#include <xen/mm.h>
> +#include <xen/param.h>
> +#include <xen/softirq.h>
> +#include <xen/keyhandler.h>
> +#include <xen/cpu.h>
> +#include <xen/pfn.h>
> +#include <xen/virtual_region.h>
> +#include <xen/vmap.h>
> +#include <xen/trace.h>
> +#include <xen/libfdt/libfdt-xen.h>
> +#include <xen/acpi.h>
> +#include <xen/warning.h>
> +#include <xen/hypercall.h>
> +#include <asm/page.h>
> +#include <asm/current.h>
> +#include <asm/setup.h>
> +#include <asm/setup.h>
setup.h seems duplicated. But this list of headers look suspiciously 
very long for the code you are moving. Can you look at reduce the number 
of includes?
Also, please take the opportunity to sort them out.

[...]

> +/*
> + * Populate the boot allocator.
> + * If a static heap was not provided by the admin, all the RAM but the
> + * following regions will be added:
> + *  - Modules (e.g., Xen, Kernel)
> + *  - Reserved regions
> + *  - Xenheap (arm32 only)
> + * If a static heap was provided by the admin, populate the boot
> + * allocator with the corresponding regions only, but with Xenheap excluded
> + * on arm32.
> + */
> +void __init populate_boot_allocator(void)
> +{
> +    unsigned int i;
> +    const struct meminfo *banks = &bootinfo.mem;
> +    paddr_t s, e;
> +
> +    if ( bootinfo.static_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
> +                continue;
> +
> +            s = bootinfo.reserved_mem.bank[i].start;
> +            e = s + bootinfo.reserved_mem.bank[i].size;
> +#ifdef CONFIG_ARM_32
I think this wants to be replaced with #ifdef CONFIG_SEPARATE_XENHEAP 
same ...
> +            /* Avoid the xenheap, note that the xenheap cannot across a bank */
> +            if ( s <= mfn_to_maddr(directmap_mfn_start) &&
> +                 e >= mfn_to_maddr(directmap_mfn_end) )
> +            {
> +                init_boot_pages(s, mfn_to_maddr(directmap_mfn_start));
> +                init_boot_pages(mfn_to_maddr(directmap_mfn_end), e);
> +            }
> +            else
> +#endif
> +                init_boot_pages(s, e);
> +        }
> +
> +        return;
> +    }
> +
> +    for ( i = 0; i < banks->nr_banks; i++ )
> +    {
> +        const struct membank *bank = &banks->bank[i];
> +        paddr_t bank_end = bank->start + bank->size;
> +
> +        s = bank->start;
> +        while ( s < bank_end )
> +        {
> +            paddr_t n = bank_end;
> +
> +            e = next_module(s, &n);
> +
> +            if ( e == ~(paddr_t)0 )
> +                e = n = bank_end;
> +
> +            /*
> +             * Module in a RAM bank other than the one which we are
> +             * not dealing with here.
> +             */
> +            if ( e > bank_end )
> +                e = bank_end;
> +
> +#ifdef CONFIG_ARM_32
... here. This comment on top of the function would also need to be updated.

> +            /* Avoid the xenheap */
> +            if ( s < mfn_to_maddr(directmap_mfn_end) &&
> +                 mfn_to_maddr(directmap_mfn_start) < e )
> +            {
> +                e = mfn_to_maddr(directmap_mfn_start);
> +                n = mfn_to_maddr(directmap_mfn_end);
> +            }
> +#endif
> +
> +            fw_unreserved_regions(s, e, init_boot_pages, 0);
> +            s = n;
> +        }
> +    }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
Cheers,

--
Julien Grall