Re: [Xen-devel] [PATCH v9 08/27] arm/x86/vmap: Add v[z|m]alloc_xen and vm_init_type

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.16 at 17:34, <konrad.wilk@xxxxxxxxxx> wrote:
> For those users who want to use the virtual addresses that
> are in the hypervisor's code/data region address space -
> these three new functions allow that.
> 
> Implementation wise the vmap API keeps track of two virtual
> address regions now:
>  a) VMAP_VIRT_START
>  b) Any provided virtual address space (need start and end).
> 
> The a) one is the default one and the existing behavior
> for users of vmalloc, vmap, etc is the same.
> 
> If however one wishes to use the b) one only has to use
> the vm_init_type to initialize and the v[m|z]alloc_xen to utilize
> it (vfree is capable of searching both address spaces).
> 
> This allows users (such as xSplice) to provide their own
> mechanism to change the the page flags, and also use virtual
> addresses closer to the hypervisor virtual addresses (at least
> on x86) while not having to deal with the allocation of
> pages.
> 
> For example of users, see patch titled "xsplice: Implement payload
> loading", where we parse the payload's ELF relocations - which
> is defined to be signed 32-bit (on x86) (max displacement hence
> is 2GB virtual space, ARM32 is 128MB). The displacement of the
> hypervisor virtual addresses to the vmalloc (on x86)
> is more than 32-bits - which means that ELF relocations would
> truncate the 34 and 33th bit. Hence this alternate API.
> 
> We also add add extra checks in case the b) range has not been
> initialized.
> 
> Part of this patch also removes 'vm_alloc' decleration as
> we do not have any users of it - and if there ever will be - we
> will have to expose and vm_alloc_xen variant.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Acked-by: Julien Grall <julien.grall@xxxxxxx> [ARM]
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

And I again wonder whether this really holds with the changes
done, the more that I had questioned it on v8.1 already.

> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

According to my outbox this likely belongs on another patch.

> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -33,7 +33,6 @@ static struct virtual_region core_init __initdata = {
>   * on deletion.
>   *
>   * All readers of virtual_region_list MUST use list list_for_each_entry_rcu.
> - *
>   */
>  static LIST_HEAD(virtual_region_list);
>  static DEFINE_SPINLOCK(virtual_region_lock);

Wrong patch.

> @@ -52,27 +55,31 @@ void *vm_alloc(unsigned int nr, unsigned int align)
>      else if ( align & (align - 1) )
>          align &= -align;
>  
> +    ASSERT(t != VMAP_REGION_NR);

"t is in range" really means >= 0 and < VMAP_REGION_NR.

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

I'm not really happy about this and ...

> +void vunmap(const void *va)
> +{
> +    vunmap_type(va, VMAP_DEFAULT);
> +}

... this. Just like vfree() they should derive the type from the address.

> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -4,15 +4,26 @@
>  #include <xen/mm.h>
>  #include <asm/page.h>
>  
> -void *vm_alloc(unsigned int nr, unsigned int align);
> +enum vmap_region {
> +    VMAP_DEFAULT,
> +    VMAP_XEN,
> +    VMAP_REGION_NR,
> +};
> +
> +void vm_init_type(enum vmap_region type, void *start, void *end);
> +
>  void vm_free(const void *);

With vm_alloc() getting removed, vm_free() should get removed
here too. And with that, vm_alloc_type() and vm_free_type() can
then just become vm_alloc() and vm_free() respectively (as static
internal functions).

Jan


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