From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v5 07/19] xen: arm: allocate dom0 memory separately from preparing the dtb Date: Wed, 13 Nov 2013 21:34:53 +0000 Message-ID: <5283F07D.2090404@linaro.org> References: <1384366234.29080.8.camel@kazak.uk.xensource.com> <1384366285-29277-7-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1384366285-29277-7-git-send-email-ian.campbell@citrix.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 , xen-devel@lists.xen.org Cc: tim@xen.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 11/13/2013 06:11 PM, Ian Campbell wrote: > Mixing these two together is a pain, it forces us to prepare the dtb before > processing the kernel which means we don't know whether the guest is 32- or > 64-bit while we construct its DTB. > > Instead split out the memory allocation (including 1:1 workaround handling) > and p2m setup into a separate phase and then create a memory node in the DTB > based on the result. > > This allows us to move kernel parsing before DTB setup. > > As part of this it was also necessary to rework where the decision regarding > the placement of the DTB and initrd in RAM was made. It is now made when > loading the kernel, which allows it to make use of the zImage/ELF specific > information and therefore to make decisions based on complete knowledge and do > it right rather than guessing in prepare_dtb and relying on a later check to > see if things worked. > > Signed-off-by: Ian Campbell > --- > v3: Also rework module placement, v2 broke boot because dtb_paddr wasn't set > soon enough. This ends up cleaner anyway. > v2: Fixed typo in the commit log > Handle multiple memory nodes as well as individual nodes with several > entries in them. > Strip the original memory node and recreate rather than trying to modify. > --- > xen/arch/arm/domain_build.c | 203 ++++++++++++++++++++++--------------------- > xen/arch/arm/kernel.c | 80 +++++++++++------ > xen/arch/arm/kernel.h | 2 - > 3 files changed, 158 insertions(+), 127 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 8645aa1..edfcf14 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -81,11 +81,8 @@ struct vcpu *__init alloc_dom0_vcpu0(void) > return alloc_vcpu(dom0, 0, 0); > } > > -static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo, > - const struct dt_property *pp, > - const struct dt_device_node *np, __be32 *new_cell) > +static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo) > { > - int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np)); > paddr_t start; > paddr_t size; > struct page_info *pg = NULL; > @@ -116,53 +113,61 @@ static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo, > if ( res ) > panic("Unable to add pages in DOM0: %d\n", res); > > - dt_set_range(&new_cell, np, start, size); > - > kinfo->mem.bank[0].start = start; > kinfo->mem.bank[0].size = size; > kinfo->mem.nr_banks = 1; > > - return reg_size; > + kinfo->unassigned_mem -= size; > } > > -static int set_memory_reg(struct domain *d, struct kernel_info *kinfo, > - const struct dt_property *pp, > - const struct dt_device_node *np, __be32 *new_cell) > +static void allocate_memory(struct domain *d, struct kernel_info *kinfo) > { > - int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np)); > - int l = 0; > + > + struct dt_device_node *memory = NULL; > + const void *reg; > + u32 reg_len, reg_size; > unsigned int bank = 0; > - u64 start; > - u64 size; > - int ret; > > if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) ) > - return set_memory_reg_11(d, kinfo, pp, np, new_cell); > + return allocate_memory_11(d, kinfo); > > - while ( kinfo->unassigned_mem > 0 && l + reg_size <= pp->length > - && kinfo->mem.nr_banks < NR_MEM_BANKS ) > + while ( (memory = dt_find_node_by_type(memory, "memory")) ) > { > - ret = dt_device_get_address(np, bank, &start, &size); > - if ( ret ) > - panic("Unable to retrieve the bank %u for %s\n", > - bank, dt_node_full_name(np)); > - > - if ( size > kinfo->unassigned_mem ) > - size = kinfo->unassigned_mem; > - dt_set_range(&new_cell, np, start, size); > - > - printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", start, start + size); > - if ( p2m_populate_ram(d, start, start + size) < 0 ) > - panic("Failed to populate P2M\n"); > - kinfo->mem.bank[kinfo->mem.nr_banks].start = start; > - kinfo->mem.bank[kinfo->mem.nr_banks].size = size; > - kinfo->mem.nr_banks++; > - kinfo->unassigned_mem -= size; > - > - l += reg_size; > - } > + int l; > + > + DPRINT("memory node\n"); > + > + reg_size = dt_cells_to_size(dt_n_addr_cells(memory) + dt_n_size_cells(memory)); > + > + reg = dt_get_property(memory, "reg", ®_len); > + if ( reg == NULL ) > + panic("Memory node has no reg property!\n"); > + > + for ( l = 0; > + kinfo->unassigned_mem > 0 && l + reg_size <= reg_len > + && kinfo->mem.nr_banks < NR_MEM_BANKS; > + l += reg_size ) > + { > + paddr_t start, size; > > - return l; > + if ( dt_device_get_address(memory, bank, &start, &size) ) > + panic("Unable to retrieve the bank %u for %s\n", > + bank, dt_node_full_name(memory)); > + > + if ( size > kinfo->unassigned_mem ) > + size = kinfo->unassigned_mem; > + > + printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", > + start, start + size); > + if ( p2m_populate_ram(d, start, start + size) < 0 ) > + panic("Failed to populate P2M\n"); > + kinfo->mem.bank[kinfo->mem.nr_banks].start = start; > + kinfo->mem.bank[kinfo->mem.nr_banks].size = size; > + kinfo->mem.nr_banks++; > + > + kinfo->unassigned_mem -= size; > + } > + } > } > > static int write_properties(struct domain *d, struct kernel_info *kinfo, > @@ -211,23 +216,6 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo, > continue; > } > } > - /* > - * In a memory node: adjust reg property. > - * TODO: handle properly memory node (ie: device_type = "memory") > - */ > - else if ( dt_node_name_is_equal(np, "memory") ) > - { > - if ( dt_property_name_is_equal(pp, "reg") ) > - { > - new_data = xzalloc_bytes(pp->length); > - if ( new_data == NULL ) > - return -FDT_ERR_XEN(ENOMEM); > - > - prop_len = set_memory_reg(d, kinfo, pp, np, > - (__be32 *)new_data); > - prop_data = new_data; > - } > - } > > res = fdt_property(kinfo->fdt, pp->name, prop_data, prop_len); > > @@ -304,6 +292,46 @@ static int fdt_property_interrupts(void *fdt, gic_interrupt_t *intr, > return res; > } > > +static int make_memory_node(const struct domain *d, > + void *fdt, > + const struct kernel_info *kinfo) > +{ > + int res, i; > + int nr_cells = XEN_FDT_NODE_REG_SIZE*kinfo->mem.nr_banks; What about xzalloc? I don't think it's safe to allocate an uncontrol size (we don't know the size of nr_banks, even if now it's hardcoded). -- Julien Grall