Re: [PATCH v2 3/5] arm/dom0less: put dom0less feature code in a separate module

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 27/09/2023 15:01, Luca Fancellu wrote:
> Currently the dom0less feature code is mostly inside domain_build.c
> and setup.c, it is a feature that may not be useful to everyone so
> put the code in a different compilation module in order to make it
> easier to disable the feature in the future.
>
> Move gic_interrupt_t in domain_build.h to use it with the function
> declaration, move its comment above the declaration.
>
> The following functions are now visible externally from domain_build
> because they are used also from the dom0less-build module:
>   - get_allocation_size
>   - set_interrupt
>   - domain_fdt_begin_node
>   - make_memory_node
>   - make_resv_memory_node
>   - make_hypervisor_node
>   - make_psci_node
>   - make_cpus_node
>   - make_timer_node
>   - handle_device_interrupts
>   - construct_domain
>   - process_shm
>
> The functions allocate_static_memory and assign_static_memory_11
> are now externally visible, so put their declarations into
> domain_build.h and move the #else and stub definition in the header
> as well.
>
> Move is_dom0less_mode from setup.c to dom0less-build.c and make it
> externally visible.
>
> Where spotted, fix code style issues.
>
> No functional change is intended.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
>   xen/arch/arm/Makefile                     |    1 +
>   xen/arch/arm/dom0less-build.c             | 1087 ++++++++++++++++++
>   xen/arch/arm/domain_build.c               | 1276 ++-------------------
>   xen/arch/arm/include/asm/dom0less-build.h |   25 +
>   xen/arch/arm/include/asm/domain_build.h   |   58 +
>   xen/arch/arm/include/asm/setup.h          |    1 -
>   xen/arch/arm/setup.c                      |   33 +-
>   7 files changed, 1288 insertions(+), 1193 deletions(-)
>   create mode 100644 xen/arch/arm/dom0less-build.c
>   create mode 100644 xen/arch/arm/include/asm/dom0less-build.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 81c31c36fc3d..70dd7201ef30 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -15,6 +15,7 @@ obj-y += cpufeature.o
>   obj-y += decode.o
>   obj-y += device.o
>   obj-$(CONFIG_IOREQ_SERVER) += dm.o
> +obj-y += dom0less-build.init.o
>   obj-y += domain.o
>   obj-y += domain_build.init.o
>   obj-$(CONFIG_ARCH_MAP_DOMAIN_PAGE) += domain_page.o
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> new file mode 100644
> index 000000000000..dc9c90cf00a7
> --- /dev/null
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -0,0 +1,1087 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * xen/arch/arm/dom0less-build.c
> + *
> + * Code related to the dom0less functionality
> + *
> + * Copyright (C) 2023 Arm Ltd.
This feels a bit odd to add your copyright here. It sounds like Arm 
wrote all the code, but you only moved. That said, I am not a lawyer.
> + */
> +
> +#include <xen/device_tree.h>
> +#include <xen/err.h>
> +#include <xen/event.h>
> +#include <xen/grant_table.h>
> +#include <xen/iocap.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/sched.h>
> +#include <xen/serial.h>
> +#include <xen/sizes.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/arm64/sve.h>
> +#include <asm/dom0less-build.h>
> +#include <asm/domain_build.h>
> +
> +bool __init is_dom0less_mode(void)
> +{
> +    struct bootmodules *mods = &bootinfo.modules;
> +    struct bootmodule *mod;
> +    unsigned int i;
> +    bool dom0found = false;
> +    bool domUfound = false;
> +
> +    /* Look into the bootmodules */
> +    for ( i = 0 ; i < mods->nr_mods ; i++ )
> +    {
> +        mod = &mods->module[i];
> +        /* Find if dom0 and domU kernels are present */
> +        if ( mod->kind == BOOTMOD_KERNEL )
> +        {
> +            if ( mod->domU == false )
> +            {
> +                dom0found = true;
> +                break;
> +            }
> +            else
> +                domUfound = true;
> +        }
> +    }
> +
> +    /*
> +     * If there is no dom0 kernel but at least one domU, then we are in
> +     * dom0less mode
> +     */
> +    return ( !dom0found && domUfound );
> +}
> +
> +static bool __init allocate_bank_memory(struct domain *d,
> +                                        struct kernel_info *kinfo,
> +                                        gfn_t sgfn,
> +                                        paddr_t tot_size)
I understand that this is today only used by domUs. However, we could 
technically use it for allocating dom0 memory if it is not 1:1.
So I think this function should stay in domain_build.c.

The rest of the code movement looks alright to me.

Cheers,

--
Julien Grall