Ian Campbell
2013-Dec-05 14:42 UTC
[PATCH] xen: arm: handle initrd addresses above the 4G boundary
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");
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));
+ 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));
+ return;
+ }
+ end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
if ( !start || !end || (start >= end) )
+ {
+ dt_printk("err limits\n");
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));
+}
+
static inline u64 dt_next_cell(int s, const __be32 **cellp)
{
const __be32 *p = *cellp;
--
1.7.10.4
Julien Grall
2013-Dec-06 15:50 UTC
Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary
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
Ian Campbell
2013-Dec-06 16:58 UTC
Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary
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.> > > > > 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.
Julien Grall
2013-Dec-06 17:39 UTC
Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary
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
Ian Campbell
2013-Dec-09 11:15 UTC
Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary
On Fri, 2013-12-06 at 17:39 +0000, Julien Grall wrote:> >> 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.Right. I think what I shall do is change the caller to accept exactly sizeof(u32) or sizeof(u64) bytes and not any value between. Ian.
Julien Grall
2013-Dec-09 13:52 UTC
Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary
On 12/09/2013 11:15 AM, Ian Campbell wrote:> On Fri, 2013-12-06 at 17:39 +0000, Julien Grall wrote: >>>> 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. > > Right. > > I think what I shall do is change the caller to accept exactly > sizeof(u32) or sizeof(u64) bytes and not any value between.It''s fine to have size between sizeof(u32) or sizeof(u64), the address will just be truncated. We have several location in Xen, where we rely on this behavior. -- Julien Grall
Ian Campbell
2013-Dec-09 14:00 UTC
Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary
On Mon, 2013-12-09 at 13:52 +0000, Julien Grall wrote:> > On 12/09/2013 11:15 AM, Ian Campbell wrote: > > On Fri, 2013-12-06 at 17:39 +0000, Julien Grall wrote: > >>>> 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. > > > > Right. > > > > I think what I shall do is change the caller to accept exactly > > sizeof(u32) or sizeof(u64) bytes and not any value between. > > It''s fine to have size between sizeof(u32) or sizeof(u64), the address > will just be truncated. > > We have several location in Xen, where we rely on this behavior.Do you have examples? Since dt_read_number can''t cope with anything which isn''t a multiple of 32-bits I don''t see a problem with rejecting them early instead of waiting until later and failing with an obscure error because an address has been truncated. Ian.
Julien Grall
2013-Dec-09 14:11 UTC
Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary
On 12/09/2013 02:00 PM, Ian Campbell wrote:> On Mon, 2013-12-09 at 13:52 +0000, Julien Grall wrote: >> >> On 12/09/2013 11:15 AM, Ian Campbell wrote: >>> On Fri, 2013-12-06 at 17:39 +0000, Julien Grall wrote: >>>>>> 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. >>> >>> Right. >>> >>> I think what I shall do is change the caller to accept exactly >>> sizeof(u32) or sizeof(u64) bytes and not any value between. >> >> It''s fine to have size between sizeof(u32) or sizeof(u64), the address >> will just be truncated. >> >> We have several location in Xen, where we rely on this behavior. > > Do you have examples? > > Since dt_read_number can''t cope with anything which isn''t a multiple of > 32-bits I don''t see a problem with rejecting them early instead of > waiting until later and failing with an obscure error because an address > has been truncated.In smp_init_cpus (arch/arm/smpboot.c), Xen only checks that reg_len is greater than the required number of cells. -- Julien Grall
Ian Campbell
2013-Dec-09 14:15 UTC
Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary
On Mon, 2013-12-09 at 14:11 +0000, Julien Grall wrote:> > On 12/09/2013 02:00 PM, Ian Campbell wrote: > > On Mon, 2013-12-09 at 13:52 +0000, Julien Grall wrote: > >> > >> On 12/09/2013 11:15 AM, Ian Campbell wrote: > >>> On Fri, 2013-12-06 at 17:39 +0000, Julien Grall wrote: > >>>>>> 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. > >>> > >>> Right. > >>> > >>> I think what I shall do is change the caller to accept exactly > >>> sizeof(u32) or sizeof(u64) bytes and not any value between. > >> > >> It''s fine to have size between sizeof(u32) or sizeof(u64), the address > >> will just be truncated. > >> > >> We have several location in Xen, where we rely on this behavior. > > > > Do you have examples? > > > > Since dt_read_number can''t cope with anything which isn''t a multiple of > > 32-bits I don''t see a problem with rejecting them early instead of > > waiting until later and failing with an obscure error because an address > > has been truncated. > > In smp_init_cpus (arch/arm/smpboot.c), Xen only checks that reg_len is > greater than the required number of cells.Hrm, I''m not sure that is setting a precedent which we should be following elsewhere. I''m pretty sure ePAPR says that numeric fields should be either 1 or 2 cells. I don''t think the fact that Xen is more forgiving in some places means it has to be everywhere. Ian.