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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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.

> 
> >
> > 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.
[...]
> > +/* 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?

Ian.


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