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

From: Shawn Anastasio <sanastasio@raptorengineering.com>
To: Julien Grall <julien@xen.org>, xen-devel@lists.xenproject.org
Cc: tpearson@raptorengineering.com, Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Michal Orzel <michal.orzel@amd.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	"Daniel P. Smith" <dpsmith@apertussolutions.com>
Subject: Re: [PATCH v3 5/9] xen/device-tree: Move Arm's setup.c bootinfo functions to common
Date: Thu, 11 Apr 2024 21:43:57 -0500	[thread overview]
Message-ID: <fdeabecc-9144-4eb6-9eb8-aac722ed1bb4@raptorengineering.com> (raw)
In-Reply-To: <19c4d0c2-c69c-4310-bf02-28d3894f8006@xen.org>

Hi Julien,

On 3/21/24 12:47 PM, Julien Grall wrote:
> 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@xen.org>
>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>> ---
>>   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.
>

Good suggestion, will do.

> [...]
> 
>> 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@xen.org>
>> + * 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.

Sure, I'll clean up the order and drop any unneeded includes.

> 
> [...]
> 
>> +/*
>> + * 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.

Good catch, will update the conditional as well as the function's
comment accordingly.

> 
> Cheers,

Thanks,
Shawn