Re: [Xen-devel] [PATCH v2 23/41] arm : acpi create chosen node for DOM0

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

+shannon

On 2 June 2015 at 23:10, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Parth,
>
> On 17/05/15 21:03, Parth Dixit wrote:
>> Create a chosen node for DOM0 with
>>  - bootargs
>>  - initrd
>
> I would have merge this patch with #22. It doesn't contain
> controversial/difficult code.
>
>> Signed-off-by: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
>> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
>> ---
>>  xen/arch/arm/domain_build.c | 46 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index c830702..e688a78 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1219,6 +1219,47 @@ static int handle_node(struct domain *d, struct 
>> kernel_info *kinfo,
>>      return res;
>>  }
>>  #ifdef CONFIG_ACPI
>> +static int make_chosen_node(struct domain *d, const struct kernel_info 
>> *kinfo)
>> +{
>> +    int res = 0;
>
> Not necessary to initialize.
>
>> +    const char *bootargs = NULL;
>> +    const struct bootmodule *mod = kinfo->kernel_bootmodule;
>> +    void *fdt = kinfo->fdt;
>> +
>> +    DPRINT("Create bootargs chosen node\n");
>
> The name of the node is "chosen" not "bootargs chosen".
>
>> +
>> +    if ( mod && mod->cmdline[0] )
>> +         bootargs = &mod->cmdline[0];
>> +    res = fdt_begin_node(fdt, "chosen");
>> +    if ( res )
>> +        return res;
>> +
>> +    res = fdt_property(fdt, "bootargs", bootargs, (strlen(bootargs)+1));
>
> strlen(bootargs) + 1. And the ( ) around it is not necessary.
>
> Furthermore, you are assuming that bootargs is always present here.
> Although if the module is not present, the variable will be NULL (see
> your check above) and Xen will crash.
>
>> +    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;
>> +}
>> +
>>  /*
>>   * Prepare a minimal DTB for DOM0 which contains
>>   * bootargs, initrd, memory information,
>> @@ -1259,6 +1300,11 @@ static int create_acpi_dtb(struct domain *d, struct 
>> kernel_info *kinfo, struct m
>>      if ( ret )
>>          return ret;
>>
>> +    /* Create a chosen node for DOM0 */
>> +    ret = make_chosen_node(d, kinfo);
>> +    if ( ret )
>> +        goto err;
>> +
>>      ret = fdt_end_node(kinfo->fdt);
>>      if ( ret < 0 )
>>          goto err;
>>
>
> Regards,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel