From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen: arm: handle initrd addresses above the 4G boundary Date: Fri, 06 Dec 2013 17:39:49 +0000 Message-ID: <52A20BE5.6020702@linaro.org> References: <1386254540-14231-1-git-send-email-ian.campbell@citrix.com> <52A1F238.90900@linaro.org> <1386349114.6672.61.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1386349114.6672.61.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.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 >>> --- >>> 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