Re: [Xen-devel] [PATCH RFC 05/15] xen/arm: rename acpi_make_chosen_node to make_chosen_node

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 14 Jun 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 13/06/18 23:15, Stefano Stabellini wrote:
> > acpi_make_chosen_node is actually generic and can be reused. Rename it
> > to make_chosen_node and make it available to non-ACPI builds.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> >   xen/arch/arm/domain_build.c | 84
> > ++++++++++++++++++++++-----------------------
> 
> Probably not the best patch to make the comment but this is the first place
> where you start to have big changes in the file.
> 
> This series is basically adding another 400 lines in a file that is already
> 2245 lines. After this series we are going to handle:
>       - DT generation when booting using ACPI
>       - DT generation when booting using DT
>       - DT generation for guest
> 
> This is probably a bit too much for a single file.

To make it easier to review, I am going to add a separate patch that
will only do code movement at the end of the series.


> >   1 file changed, 42 insertions(+), 42 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index bb88e09..4e4cd19 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -935,6 +935,47 @@ static int make_timer_node(const struct domain *d, void
> > *fdt,
> >       return res;
> >   }
> >   +static int make_chosen_node(const struct kernel_info *kinfo)
> 
> Can you document it who is going to use it? This name gives the impression
> that will be used for creating any DT. But in fact, it will only be the case
> for ACPI based Dom0 and DomU.

I'll add:

 * This function is used as part of the device tree generation for Dom0
 * on ACPI systems, and DomUs started directly from Xen based on device
 * tree information.


> > +{
> > +    int res;
> > +    const char *bootargs = NULL;
> > +    const struct bootmodule *mod = kinfo->kernel_bootmodule;
> > +    void *fdt = kinfo->fdt;
> > +
> > +    dt_dprintk("Create chosen node\n");
> > +    res = fdt_begin_node(fdt, "chosen");
> > +    if ( res )
> > +        return res;
> > +
> > +    if ( mod && mod->cmdline[0] )
> > +    {
> > +        bootargs = &mod->cmdline[0];
> > +        res = fdt_property(fdt, "bootargs", bootargs, strlen(bootargs) +
> > 1);
> > +        if ( res )
> > +           return res;
> > +    }
> > +
> > +    /*
> > +     * If the bootloader provides an initrd, we must create a placeholder
> > +     * for the initrd properties. The values will be replaced later.
> > +     */
> > +    if ( mod && mod->size )
> > +    {
> > +        u64 a = 0;
> > +        res = fdt_property(kinfo->fdt, "linux,initrd-start", &a,
> > sizeof(a));
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property(kinfo->fdt, "linux,initrd-end", &a, sizeof(a));
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    res = fdt_end_node(fdt);
> > +
> > +    return res;
> > +}
> > +
> >   static int map_irq_to_domain(struct domain *d, unsigned int irq,
> >                                bool need_mapping, const char *devname)
> >   @@ -1420,47 +1461,6 @@ static int acpi_route_spis(struct domain *d)
> >       return 0;
> >   }
> >   -static int acpi_make_chosen_node(const struct kernel_info *kinfo)
> > -{
> > -    int res;
> > -    const char *bootargs = NULL;
> > -    const struct bootmodule *mod = kinfo->kernel_bootmodule;
> > -    void *fdt = kinfo->fdt;
> > -
> > -    dt_dprintk("Create chosen node\n");
> > -    res = fdt_begin_node(fdt, "chosen");
> > -    if ( res )
> > -        return res;
> > -
> > -    if ( mod && mod->cmdline[0] )
> > -    {
> > -        bootargs = &mod->cmdline[0];
> > -        res = fdt_property(fdt, "bootargs", bootargs, strlen(bootargs) +
> > 1);
> > -        if ( res )
> > -           return res;
> > -    }
> > -
> > -    /*
> > -     * If the bootloader provides an initrd, we must create a placeholder
> > -     * for the initrd properties. The values will be replaced later.
> > -     */
> > -    if ( mod && mod->size )
> > -    {
> > -        u64 a = 0;
> > -        res = fdt_property(kinfo->fdt, "linux,initrd-start", &a,
> > sizeof(a));
> > -        if ( res )
> > -            return res;
> > -
> > -        res = fdt_property(kinfo->fdt, "linux,initrd-end", &a, sizeof(a));
> > -        if ( res )
> > -            return res;
> > -    }
> > -
> > -    res = fdt_end_node(fdt);
> > -
> > -    return res;
> > -}
> > -
> >   static int acpi_make_hypervisor_node(const struct kernel_info *kinfo,
> >                                        struct membank tbl_add[])
> >   {
> > @@ -1532,7 +1532,7 @@ static int create_acpi_dtb(struct kernel_info *kinfo,
> > struct membank tbl_add[])
> >           return ret;
> >         /* Create a chosen node for DOM0 */
> > -    ret = acpi_make_chosen_node(kinfo);
> > +    ret = make_chosen_node(kinfo);
> >       if ( ret )
> >           goto err;
> >   
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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