Re: [PATCH v3 1/9] libxc: reorganize domain builder guest memory allocator

[Juergen Gross <jgross@suse.com>]

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 <jgross@suse.com>
>> ---
>>   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