From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH v3 1/9] libxc: reorganize domain builder guest memory allocator Date: Wed, 28 Oct 2015 16:51:05 +0100 Message-ID: <5630EEE9.2000902@suse.com> References: <1444741878-16610-1-git-send-email-jgross@suse.com> <1444741878-16610-2-git-send-email-jgross@suse.com> <20151028153256.GF18674@zion.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: <20151028153256.GF18674@zion.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: Wei Liu Cc: roger.pau@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, Ian.Campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 10/28/2015 04:32 PM, Wei Liu wrote: > On Tue, Oct 13, 2015 at 03:11:10PM +0200, Juergen Gross wrote: >> Guest memory allocation in the domain builder of libxc is done via >> virtual addresses only. In order to be able to support preallocated >> areas not virtually mapped reorganize the memory allocator to keep >> track of allocated pages globally and in allocated segments. >> >> This requires an interface change of the allocate callback of the >> domain builder which currently is using the last mapped virtual >> address as a parameter. This is no problem as the only user of this >> callback is stubdom/grub/kexec.c using this virtual address to >> calculate the last used pfn. >> >> Signed-off-by: Juergen Gross >> --- >> stubdom/grub/kexec.c | 6 +-- >> tools/libxc/include/xc_dom.h | 11 +++-- >> tools/libxc/xc_dom_core.c | 101 +++++++++++++++++++++++++++++-------------- >> 3 files changed, 75 insertions(+), 43 deletions(-) >> >> diff --git a/stubdom/grub/kexec.c b/stubdom/grub/kexec.c >> index 0b2f4f3..2300318 100644 >> --- a/stubdom/grub/kexec.c >> +++ b/stubdom/grub/kexec.c >> @@ -100,9 +100,9 @@ static void do_exchange(struct xc_dom_image *dom, xen_pfn_t target_pfn, xen_pfn_ >> dom->p2m_host[target_pfn] = source_mfn; >> } >> >> -int kexec_allocate(struct xc_dom_image *dom, xen_vaddr_t up_to) >> +int kexec_allocate(struct xc_dom_image *dom) >> { >> - unsigned long new_allocated = (up_to - dom->parms.virt_base) / PAGE_SIZE; >> + unsigned long new_allocated = dom->pfn_alloc_end - dom->rambase_pfn; >> unsigned long i; >> >> pages = realloc(pages, new_allocated * sizeof(*pages)); >> @@ -319,8 +319,6 @@ void kexec(void *kernel, long kernel_size, void *module, long module_size, char >> >> /* Make sure the bootstrap page table does not RW-map any of our current >> * page table frames */ >> - kexec_allocate(dom, dom->virt_pgtab_end); >> - > > Does this mean pvgrub is now able to use this shiny new feature? That's the plan. I have to admit I didn't test this. And don't mix this up with grub-xen (I'm just working on that beast). > >> if ( (rc = xc_dom_update_guest_p2m(dom))) { >> grub_printf("xc_dom_update_guest_p2m returned %d\n", rc); >> errnum = ERR_BOOT_FAILURE; >> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h >> index e52b023..878dc52 100644 >> --- a/tools/libxc/include/xc_dom.h >> +++ b/tools/libxc/include/xc_dom.h >> @@ -29,6 +29,7 @@ struct xc_dom_seg { >> xen_vaddr_t vstart; >> xen_vaddr_t vend; >> xen_pfn_t pfn; >> + xen_pfn_t pages; >> }; >> >> struct xc_dom_mem { >> @@ -90,6 +91,7 @@ struct xc_dom_image { >> xen_pfn_t xenstore_pfn; >> xen_pfn_t shared_info_pfn; >> xen_pfn_t bootstack_pfn; >> + xen_pfn_t pfn_alloc_end; >> xen_vaddr_t virt_alloc_end; >> xen_vaddr_t bsd_symtab_start; >> >> @@ -177,7 +179,7 @@ struct xc_dom_image { >> /* kernel loader */ >> struct xc_dom_arch *arch_hooks; >> /* allocate up to virt_alloc_end */ > > I think you need to update this comment, too. Hmm, yes. > >> - int (*allocate) (struct xc_dom_image * dom, xen_vaddr_t up_to); >> + int (*allocate) (struct xc_dom_image * dom); >> >> /* Container type (HVM or PV). */ >> enum { >> @@ -361,14 +363,11 @@ static inline void *xc_dom_seg_to_ptr_pages(struct xc_dom_image *dom, >> struct xc_dom_seg *seg, >> xen_pfn_t *pages_out) >> { >> - xen_vaddr_t segsize = seg->vend - seg->vstart; >> - unsigned int page_size = XC_DOM_PAGE_SIZE(dom); >> - xen_pfn_t pages = (segsize + page_size - 1) / page_size; >> void *retval; >> >> - retval = xc_dom_pfn_to_ptr(dom, seg->pfn, pages); >> + retval = xc_dom_pfn_to_ptr(dom, seg->pfn, seg->pages); >> >> - *pages_out = retval ? pages : 0; >> + *pages_out = retval ? seg->pages : 0; >> return retval; >> } >> >> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c >> index fbe4464..b1d7890 100644 >> --- a/tools/libxc/xc_dom_core.c >> +++ b/tools/libxc/xc_dom_core.c >> @@ -535,56 +535,75 @@ void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t pfn, >> return phys->ptr; >> } >> >> -int xc_dom_alloc_segment(struct xc_dom_image *dom, >> - struct xc_dom_seg *seg, char *name, >> - xen_vaddr_t start, xen_vaddr_t size) >> +static int xc_dom_chk_alloc_pages(struct xc_dom_image *dom, char *name, >> + xen_pfn_t pages) >> { >> unsigned int page_size = XC_DOM_PAGE_SIZE(dom); >> - xen_pfn_t pages = (size + page_size - 1) / page_size; >> - xen_pfn_t pfn; >> - void *ptr; >> >> - if ( start == 0 ) >> - start = dom->virt_alloc_end; >> + if ( pages > dom->total_pages || /* multiple test avoids overflow probs */ >> + dom->pfn_alloc_end - dom->rambase_pfn > dom->total_pages || >> + pages > dom->total_pages - dom->pfn_alloc_end + dom->rambase_pfn ) >> + { >> + xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, >> + "%s: segment %s too large (0x%"PRIpfn" > " >> + "0x%"PRIpfn" - 0x%"PRIpfn" pages)", __FUNCTION__, name, >> + pages, dom->total_pages, >> + dom->pfn_alloc_end - dom->rambase_pfn); >> + return -1; >> + } >> + >> + dom->pfn_alloc_end += pages; >> + dom->virt_alloc_end += pages * page_size; >> >> - if ( start & (page_size - 1) ) >> + return 0; >> +} >> + >> +static int xc_dom_alloc_pad(struct xc_dom_image *dom, xen_vaddr_t end) >> +{ >> + unsigned int page_size = XC_DOM_PAGE_SIZE(dom); >> + xen_pfn_t pages; >> + >> + if ( end & (page_size - 1) ) >> { >> xc_dom_panic(dom->xch, XC_INTERNAL_ERROR, >> "%s: segment start isn't page aligned (0x%" PRIx64 ")", > > "segment end"? No. This function is called to add a padding allocation before the start of a new segment, which has to start at page boundary. Maybe a comment should clarify this. :-) > >> - __FUNCTION__, start); >> + __FUNCTION__, end); >> return -1; >> } >> - if ( start < dom->virt_alloc_end ) >> + if ( end < dom->virt_alloc_end ) >> { >> xc_dom_panic(dom->xch, XC_INTERNAL_ERROR, >> "%s: segment start too low (0x%" PRIx64 " < 0x%" PRIx64 > > Ditto. Ditto. ;-) > > A major part of this patch looks like refactoring to me. And to the > best of my knowledge it seems to be doing the right thing. Thanks. Juergen