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

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

On 12/06/2013 04:58 PM, Ian Campbell wrote:
> On Fri, 2013-12-06 at 15:50 +0000, Julien Grall wrote:
>>
>> 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 ...
>
> For 4.5 maybe, I don't think now is the time to be reordering the domain
> build yet again.
>
>> 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.
>
> You need to know the kernel address in order to figure out a safe
> address for the initrd though.
>
> Maybe we an delay generating the dtb until after all that is done (which
> does make some logical sense anyway), but as I say -- not until 4.5.

It was only a suggestion for later. I'm fine with the current solution 
for now.

>>
>>>
>>> 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?
>
> Oops, actually this (and all the others) were just my debug prints which
> I forgot to remove. I was planning to delete them, but maybe I should
> make them into proper error message.

It's always useful to have these error messages.

> [...]
>>> +/* 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/
>
> Damn, I got my self in a twist when I wrote this but thought I'd gotten
> it right!
>
>> Perhaps you update the comment by saying we are assuming bytes is 4-byte
>> aligned.
>
> Maybe it would be better to round up?

No because if we round up, we would read to much data. dt_read_number is 
not able to check the size.

-- 
Julien Grall