Re: [Xen-devel] [PATCH v6 07/24] arm/x86/vmap: Add vmalloc_type and vm_init_type

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/04/16 04:49, Konrad Rzeszutek Wilk wrote:
> For those users who want to use the virtual addresses that
> are in the hypervisor's virtual address space - these two new
> functions allow that. Along with providing the underlaying
> MFNs for the user's (such as changing page table permissions).
>
> 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 initalize and the vmalloc_type to utilize it.
>
> 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 (so max displacement is 2GB virtual
> spacE). 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.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
>
> v4: New patch.
> v5: Update per Jan's comments.
> v6: Drop the stray parentheses on typedefs.
>     Ditch the vunmap callback. Stash away the virtual addresses in lists.
>     Ditch the vmap callback. Just provide virtual address.
>     Ditch the vmalloc_range. Require users of alternative virtual address
>     to call vmap_init_type first.

(For anyone else wishing to review this, `git diff --color-words` makes
it a far easier job)

This is definitely an improvement over previous versions, and in
principle looks fine, with moderate issue.

> diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
> index 5671ac8..07fa3b4 100644
> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -4,16 +4,44 @@
>  #include <xen/mm.h>
>  #include <asm/page.h>
>  
> +enum vmap_type {
> +    VMAP_DEFAULT,
> +    VMAP_XEN,
> +    VMAP_nr,
> +};

These really arn't types of vmap.  They are just different regions to
try and use; the underlying infrastructure is still all the same.

I would recommend "enum vmap_region" instead, as well as VMAP_REGION_NR
to avoid having a mixed-case constant.

> +
> +void vm_free_type(const void *, enum vmap_type);
> +void vunmap_type(const void *, enum vmap_type);
> +void *vmalloc_type(size_t size, enum vmap_type type, mfn_t **mfn_array);
> +void vm_init_type(enum vmap_type type, void *start, void *end);
> +void vfree_type(void *va, enum vmap_type type);

Exposing the type (/region) parameter is quite unsafe, when mixed with
the va.  What happens if someone passes in a va for one region, with a
VMAP_$other ?

How likely are we to gain a 3rd region?  My gut feeling is that it would
be safer to hide all of the type/region bits in vmap.c (other than
perhaps the _init() calls), and expose $VMAP_FOO_xen() functions in the API.

~Andrew

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