Re: [Xen-devel] [PATCH v5 10/28] xsplice: Implement payload loading

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.04.16 at 17:50, <konrad.wilk@xxxxxxxxxx> wrote:
> That actually ended up pretty simple. It won't compile for ARM yet,
> see below please.

This comes pretty close. A few minor comments below.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -100,6 +100,9 @@ unsigned long __read_mostly xen_phys_start;
>  
>  unsigned long __read_mostly xen_virt_end;
>  
> +unsigned long __read_mostly avail_virt_start;
> +unsigned long __read_mostly avail_virt_end;

These now appear to be unused.

> -void __init vm_init(void)
> +void __init vm_init_type(enum vmap_type type)
>  {
>      unsigned int i, nr;
>      unsigned long va;
> +    void *end;
> +
> +    if ( type == VMAP_VIRT )
> +    {
> +        vm_base[VMAP_VIRT] = (void *)VMAP_VIRT_START;
> +        end = arch_vmap_virt_end();
> +    }
> +    else
> +    {
> +        vm_base[XEN_VIRT] = (void *)xen_virt_end;
> +        end = (void *)(XEN_VIRT_END - NR_CPUS * PAGE_SIZE);
>  
> -    vm_base = (void *)VMAP_VIRT_START;
> -    vm_end = PFN_DOWN(arch_vmap_virt_end() - vm_base);
> -    vm_low = PFN_UP((vm_end + 7) / 8);
> -    nr = PFN_UP((vm_low + 7) / 8);
> -    vm_top = nr * PAGE_SIZE * 8;
> +        BUG_ON(end <= vm_base[XEN_VIRT]);
> +    }
> +    vm_end[type] = PFN_DOWN(end - vm_base[type]);
> +    vm_low[type]= PFN_UP((vm_end[type] + 7) / 8);
> +    nr = PFN_UP((vm_low[type] + 7) / 8);
> +    vm_top[type] = nr * PAGE_SIZE * 8;
>  
> -    for ( i = 0, va = (unsigned long)vm_bitmap; i < nr; ++i, va += PAGE_SIZE 
> )
> +    for ( i = 0, va = (unsigned long)vm_bitmap(type); i < nr; ++i, va += 
> PAGE_SIZE )
>      {
>          struct page_info *pg = alloc_domheap_page(NULL, 0);
>  
>          map_pages_to_xen(va, page_to_mfn(pg), 1, PAGE_HYPERVISOR);
>          clear_page((void *)va);
>      }
> -    bitmap_fill(vm_bitmap, vm_low);
> +    bitmap_fill(vm_bitmap(type), vm_low[type]);
>  
>      /* Populate page tables for the bitmap if necessary. */
> -    populate_pt_range(va, 0, vm_low - nr);
> +    populate_pt_range(va, 0, vm_low[type] - nr);
>  }
>  
> -void *vm_alloc(unsigned int nr, unsigned int align)
> +void __init vm_init(void)
> +{
> +    vm_init_type(VMAP_VIRT);
> +#ifdef CONFIG_XSPLICE
> +    vm_init_type(XEN_VIRT);
> +#endif
> +}

I think we should leave it to the arch to call vm_init_type() for
the non-default type(s) it cares about, namely allowing for this to
be done at a different (later) time. Which means vm_init() could
simply become an inline/macro wrapper of vm_init_type(VMAP_VIRT).

> +void *vm_alloc(unsigned int nr, unsigned int align)
> +{
> +    return vm_alloc_type(nr, align, VMAP_VIRT);
> +}

Inline/macro wrapper?

> +void vm_free(const void *va)
> +{
> +    vm_free_type(va, VMAP_VIRT);
> +}

Again.

>  void vunmap(const void *va)
>  {
> +    enum vmap_type type = VMAP_VIRT;
> +    unsigned int size = vm_size(va, type);
> +#ifndef _PAGE_NONE
> +    unsigned long addr;
> +#endif
> +
> +    if ( !size )
> +    {
> +        type = XEN_VIRT;
> +        size = vm_size(va, type);
> +    }

I don't think such automatic fallback should be tried - the caller ought
to know which region it mapped, so it could call vunmap_type().

> @@ -238,11 +288,15 @@ void *vmalloc(size_t size)
>          mfn[i] = _mfn(page_to_mfn(pg));
>      }
>  
> -    va = vmap(mfn, pages);
> +    va = __vmap(mfn, 1, pages, 1, PAGE_HYPERVISOR, type);
>      if ( va == NULL )
>          goto error;
>  
> -    xfree(mfn);
> +    if ( mfn_array )
> +        *mfn_array = mfn;
> +    else
> +        xfree(mfn);

What's this? I certainly assumed this wouldn't be needed anymore
now.

> @@ -275,7 +334,10 @@ void vfree(void *va)
>      if ( !va )
>          return;
>  
> -    pages = vm_size(va);
> +    pages = vm_size(va, VMAP_VIRT);
> +    if ( !pages )
> +        pages = vm_size(va, XEN_VIRT);

Same comment as as for vunmap().

> +enum vmap_type {
> +    VMAP_VIRT,
> +    XEN_VIRT,
> +    VMAP_TYPE_MAX,
> +};

I think these would benefit from using a common prefix, e.g.

enum vmap_type {
    VMAP_DEFAULT,
    VMAP_XEN,
    VMAP_nr
};

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel