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

[Julien Grall <julien@xxxxxxx>]

On 02/11/2023 11:15, Luca Fancellu wrote:
> Hi Julien,
>
> > > 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.
> Mmm yes, we should be required to put our copyright in everything we create or 
> modify or touch,
> said that, I could modify that to be like this:
This is debatable. IIRC we discussed this in a recent-ish community 
call. From my understanding, we are tracking the authorship/copyright 
using signed-off-by/author in the commit message.
> /*
>   * xen/arch/arm/dom0less-build.c
>   *
>   * Code related to the dom0less functionality
>   *
>   * Original code from xen/arch/arm/domain_build.c
>   * Modifications: Copyright (C) 2023 Arm Ltd.
>   */
>
> What do you think?
TBH, it is not clear to me what sort of modifications you did that would 
require a copyright from Arm. AFAICT, you only moved the code.
Anyway, I will let the others express their views here.

> > > + */
> > > +
> > > +#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.
> Ok I can leave it there, is it ok for you if I protect it using the KConfig in 
> domain_build.c since that code is currently not used apart from dom0less?
> (I will do that in the last patch if you agree)
I am fine with that.

Cheers,

--
Julien Grall