Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary

[Julien Grall <julien.grall@linaro.org>]

On 12/05/2013 02:42 PM, Ian Campbell wrote:
> The Xgene platform has no RAM below 4G.
>
> The /chosen/linux,initrd-* properties do not have "reg" semantics and
> therefore #*-size are not used when interpreting. Instead they are are simply
> numbers which are interpreted according to the properties length.
>
> Fix this both when parsing the entry in the host DTB and when creating the
> dom0 DTB. For dom0 we simply hardcode a 64-bit size, this is acceptable
> even for a 32-bit guest.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/domain_build.c   |   19 +++++++++++++------
>   xen/common/device_tree.c      |   26 ++++++++++++++++++++++----
>   xen/include/xen/device_tree.h |    8 +++++++-
>   3 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7b9031b..aac24eb 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -222,11 +222,12 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>            */
>           if ( early_info.modules.module[MOD_INITRD].size )
>           {
> -            res = fdt_property_cell(kinfo->fdt, "linux,initrd-start", 0);
> +            u64 a = 0;
> +            res = fdt_property(kinfo->fdt, "linux,initrd-start", &a, sizeof(a));
>               if ( res )
>                   return res;
>
> -            res = fdt_property_cell(kinfo->fdt, "linux,initrd-end", 0);
> +            res = fdt_property(kinfo->fdt, "linux,initrd-end", &a, sizeof(a));
>               if ( res )
>                   return res;
>           }
> @@ -923,6 +924,8 @@ static void initrd_load(struct kernel_info *kinfo)
>       unsigned long offs;
>       int node;
>       int res;
> +    u32 val[2];
> +    __be32 *cellp;
>
>       if ( !len )
>           return;
> @@ -935,13 +938,17 @@ static void initrd_load(struct kernel_info *kinfo)
>       if ( node < 0 )
>           panic("Cannot find the /chosen node");
>
> -    res = fdt_setprop_inplace_cell(kinfo->fdt, node, "linux,initrd-start",
> -                                   load_addr);
> +    cellp = (__be32 *)val;
> +    dt_set_cell(&cellp, ARRAY_SIZE(val), load_addr);
> +    res = fdt_setprop_inplace(kinfo->fdt, node, "linux,initrd-start",
> +                              val, sizeof(val));
>       if ( res )
>           panic("Cannot fix up \"linux,initrd-start\" property");
>
> -    res = fdt_setprop_inplace_cell(kinfo->fdt, node, "linux,initrd-end",
> -                                   load_addr + len);
> +    cellp = (__be32 *)val;
> +    dt_set_cell(&cellp, ARRAY_SIZE(val), load_addr + len);
> +    res = fdt_setprop_inplace(kinfo->fdt, node, "linux,initrd-end",
> +                              val, sizeof(val));
>       if ( res )
>           panic("Cannot fix up \"linux,initrd-end\" property");


I'm wondering if we can move this code to write_properties ...

It seems that we compute the initrd address in kernel_*_load function, 
but I think we can move place_modules at the end of kernel_prepare.

>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 943b181..2b1b364 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -390,18 +390,36 @@ static void __init process_chosen_node(const void *fdt, int node,
>                                          const char *name,
>                                          u32 address_cells, u32 size_cells)
>   {
> +    const struct fdt_property *prop;
>       struct dt_mb_module *mod = &early_info.modules.module[MOD_INITRD];
> -    u32 start, end;
> +    paddr_t start, end;
> +    int len;
>
>       dt_printk("Checking for initrd in /chosen\n");
>
> -    start = device_tree_get_u32(fdt, node, "linux,initrd-start", 0);
> -    end = device_tree_get_u32(fdt, node, "linux,initrd-end", 0);
> +    prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
> +    if ( !prop || len < sizeof(u32) || len > sizeof(u64) )
> +    {
> +        dt_printk("err start %p len %d %ld %ld\n", prop, len, sizeof(u32), sizeof(u64));

With a user see this message it's not clear where the error message 
comes from ... Can you add 'initrd' or something similar in the message?

> +        return;
> +    }
> +    start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> +
> +    prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
> +    if ( !prop || len < sizeof(u32) || len > sizeof(u64) )
> +    {
> +        dt_printk("err end %p len %d %ld %ld\n", prop, len, sizeof(u32), sizeof(u64));

Same here.

> +        return;
> +    }
> +    end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
>
>       if ( !start || !end || (start >= end) )
> +    {
> +        dt_printk("err limits\n");

Same here.

>           return;
> +    }
>
> -    dt_printk("Initrd 0x%x-0x%x\n", start, end);
> +    dt_printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
>
>       mod->start = start;
>       mod->size = end - start;
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index da78c9f..9f23b07 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -256,12 +256,18 @@ static inline u64 dt_read_number(const __be32 *cell, int size)
>       return r;
>   }
>
> -/* Helper to convert a number of cells in bytes */
> +/* Helper to convert a number of cells to bytes */
>   static inline int dt_cells_to_size(int size)
>   {
>       return (size * sizeof (u32));
>   }
>
> +/* Helper to convert a number of bytes to cells  */
> +static inline int dt_size_to_cells(int cells)
> +{
> +    return (cells / sizeof(u32));
> +}
> +

s/cells/bytes/

Perhaps you update the comment by saying we are assuming bytes is 4-byte 
aligned.

-- 
Julien Grall