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

[Julien Grall <julien.grall@xxxxxxx>]

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.

>   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.
> +{
> +    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