diff mbox

[Xen-devel,RFC,21/35] xen/arm: Create memory node for DOM0

Message ID 1423058539-26403-22-git-send-email-parth.dixit@linaro.org
State New
Headers show

Commit Message

Parth Dixit Feb. 4, 2015, 2:02 p.m. UTC
From: Naresh Bhat <naresh.bhat@linaro.org>

Create a memory node for DOM0.

Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
---
 xen/arch/arm/domain_build.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Julien Grall Feb. 5, 2015, 3:51 a.m. UTC | #1
Hi Parth,

On 04/02/2015 14:02, parth.dixit@linaro.org wrote:
> From: Naresh Bhat <naresh.bhat@linaro.org>
>
> Create a memory node for DOM0.

I'm not convince it's necessary to have a separate patch for this. I 
would squash it the #20.

> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> ---
>   xen/arch/arm/domain_build.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bb7f043..30bebe5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1155,6 +1155,50 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>       return res;
>   }
>
> +static int make_memory_node_acpi(const struct domain *d,
> +                            void *fdt,
> +                            int addr_cells,
> +                            int size_cells,
> +                            const struct kernel_info *kinfo)
> +{
> +    int res, i;
> +    int reg_size = addr_cells + size_cells;
> +    int nr_cells = reg_size*kinfo->mem.nr_banks;
> +    __be32 reg[nr_cells];
> +    __be32 *cells;
> +
> +    DPRINT("Create memory node (reg size %d, nr cells %d)\n", reg_size, nr_cells);
> +
> +    /* ePAPR 3.4 */
> +    res = fdt_begin_node(fdt, "memory");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_string(fdt, "device_type", "memory");
> +    if ( res )
> +        return res;
> +
> +    cells = &reg[0];
> +    for ( i = 0 ; i < kinfo->mem.nr_banks; i++ )
> +    {
> +        u64 start = kinfo->mem.bank[i].start;
> +        u64 size = kinfo->mem.bank[i].size;
> +
> +        DPRINT("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> +                i, start, start + size);
> +
> +        dt_set_range(&cells, fdt, start, size);
> +    }
> +
> +    res = fdt_property(fdt, "reg", reg, sizeof(reg));
> +    if ( res )
> +        return res;
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}
> +

Why did you duplicate make_memory_node rather than slightly modify the 
function to support both ACPI and DT version?

>   /*
>    * Prepare a minimal DTB for DOM0 which contains
>    * bootargs, memory information,
> @@ -1196,6 +1240,10 @@ static int prepare_dtb_acpi(struct domain *d, struct kernel_info *kinfo)
>       if ( ret )
>           return ret;
>
> +    ret = make_memory_node_acpi(d, kinfo->fdt, 2, 1, kinfo);

I forget to mention it on the previous patch. Please add 2 defines for 
the value 2/1.

> +    if ( ret )
> +        goto err;
> +
>       ret = fdt_end_node(kinfo->fdt);
>       if ( ret < 0 )
>           goto err;
>

Regards,
Julien Grall Feb. 11, 2015, 6:27 a.m. UTC | #2
Hi Stefano,

On 06/02/2015 00:01, Stefano Stabellini wrote:
> On Wed, 4 Feb 2015, parth.dixit@linaro.org wrote:
>> From: Naresh Bhat <naresh.bhat@linaro.org>
>>
>> Create a memory node for DOM0.
>>
>> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
>> ---
>>   xen/arch/arm/domain_build.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index bb7f043..30bebe5 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1155,6 +1155,50 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>>       return res;
>>   }
>>
>> +static int make_memory_node_acpi(const struct domain *d,
>> +                            void *fdt,
>> +                            int addr_cells,
>> +                            int size_cells,
>> +                            const struct kernel_info *kinfo)
>> +{
>> +    int res, i;
>> +    int reg_size = addr_cells + size_cells;
>> +    int nr_cells = reg_size*kinfo->mem.nr_banks;
>> +    __be32 reg[nr_cells];
>> +    __be32 *cells;
>> +
>> +    DPRINT("Create memory node (reg size %d, nr cells %d)\n", reg_size, nr_cells);
>> +
>> +    /* ePAPR 3.4 */
>> +    res = fdt_begin_node(fdt, "memory");
>> +    if ( res )
>> +        return res;
>> +
>> +    res = fdt_property_string(fdt, "device_type", "memory");
>> +    if ( res )
>> +        return res;
>> +
>> +    cells = &reg[0];
>> +    for ( i = 0 ; i < kinfo->mem.nr_banks; i++ )
>> +    {
>> +        u64 start = kinfo->mem.bank[i].start;
>> +        u64 size = kinfo->mem.bank[i].size;
>> +
>> +        DPRINT("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>> +                i, start, start + size);
>> +
>> +        dt_set_range(&cells, fdt, start, size);
>> +    }
>> +
>> +    res = fdt_property(fdt, "reg", reg, sizeof(reg));
>> +    if ( res )
>> +        return res;
>> +
>> +    res = fdt_end_node(fdt);
>> +
>> +    return res;
>> +}
>
> What's the difference with make_memory_node?  Couldn't you just use that
> instead?

AFAICS, the only difference is the way we get the number of address/size 
cells.

I agree that we should extend make_memory_node to get those number of 
cells in parameter rather than duplicating the function.

Regards,
diff mbox

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bb7f043..30bebe5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1155,6 +1155,50 @@  static int handle_node(struct domain *d, struct kernel_info *kinfo,
     return res;
 }
 
+static int make_memory_node_acpi(const struct domain *d,
+                            void *fdt,
+                            int addr_cells,
+                            int size_cells,
+                            const struct kernel_info *kinfo)
+{
+    int res, i;
+    int reg_size = addr_cells + size_cells;
+    int nr_cells = reg_size*kinfo->mem.nr_banks;
+    __be32 reg[nr_cells];
+    __be32 *cells;
+
+    DPRINT("Create memory node (reg size %d, nr cells %d)\n", reg_size, nr_cells);
+
+    /* ePAPR 3.4 */
+    res = fdt_begin_node(fdt, "memory");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "device_type", "memory");
+    if ( res )
+        return res;
+
+    cells = &reg[0];
+    for ( i = 0 ; i < kinfo->mem.nr_banks; i++ )
+    {
+        u64 start = kinfo->mem.bank[i].start;
+        u64 size = kinfo->mem.bank[i].size;
+
+        DPRINT("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
+                i, start, start + size);
+
+        dt_set_range(&cells, fdt, start, size);
+    }
+
+    res = fdt_property(fdt, "reg", reg, sizeof(reg));
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
 /*
  * Prepare a minimal DTB for DOM0 which contains
  * bootargs, memory information,
@@ -1196,6 +1240,10 @@  static int prepare_dtb_acpi(struct domain *d, struct kernel_info *kinfo)
     if ( ret )
         return ret;
 
+    ret = make_memory_node_acpi(d, kinfo->fdt, 2, 1, kinfo);
+    if ( ret )
+        goto err;
+
     ret = fdt_end_node(kinfo->fdt);
     if ( ret < 0 )
         goto err;