On 03.04.2024 10:16, Henry Wang wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -219,7 +219,8 @@ static void populate_physmap(struct memop_args *a)
> }
> else
> {
> - if ( is_domain_direct_mapped(d) )
> + if ( is_domain_direct_mapped(d) &&
> + !(a->memflags & MEMF_force_heap_alloc) )
> {
> mfn = _mfn(gpfn);
>
> @@ -246,7 +247,8 @@ static void populate_physmap(struct memop_args *a)
>
> mfn = _mfn(gpfn);
> }
> - else if ( is_domain_using_staticmem(d) )
> + else if ( is_domain_using_staticmem(d) &&
> + !(a->memflags & MEMF_force_heap_alloc) )
> {
> /*
> * No easy way to guarantee the retrieved pages are
> contiguous,
> @@ -271,6 +273,14 @@ static void populate_physmap(struct memop_args *a)
> }
> else
> {
> + /*
> + * Avoid passing MEMF_force_heap_alloc down to
> + * alloc_domheap_pages() where the meaning would be the
> + * original MEMF_no_refcount.
> + */
> + if ( unlikely(a->memflags & MEMF_force_heap_alloc) )
> + clear_bit(_MEMF_force_heap_alloc, &a->memflags);
Why an atomic operation? &= will to quite fine here. And you can also
drop the if().
> @@ -1408,6 +1418,10 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( copy_from_guest(&reservation, arg, 1) )
> return start_extent;
>
> + if ( op != XENMEM_populate_physmap
> + && (reservation.mem_flags & XENMEMF_force_heap_alloc) )
> + return -EINVAL;
> +
> /* Is size too large for us to encode a continuation? */
> if ( reservation.nr_extents > (UINT_MAX >> MEMOP_EXTENT_SHIFT) )
> return start_extent;
> @@ -1433,6 +1447,10 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> && (reservation.mem_flags & XENMEMF_populate_on_demand) )
> args.memflags |= MEMF_populate_on_demand;
>
> + if ( op == XENMEM_populate_physmap
> + && (reservation.mem_flags & XENMEMF_force_heap_alloc) )
> + args.memflags |= MEMF_force_heap_alloc;
If in the end no new sub-op is used (see below), this and the earlier if()
want combining.
You further may want to assert that the flag isn't already set (as coming
back from construct_memop_from_reservation()).
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -41,6 +41,11 @@
> #define XENMEMF_exact_node(n) (XENMEMF_node(n) | XENMEMF_exact_node_request)
> /* Flag to indicate the node specified is virtual node */
> #define XENMEMF_vnode (1<<18)
> +/*
> + * Flag to force populate physmap to use pages from domheap instead of 1:1
> + * or static allocation.
> + */
> +#define XENMEMF_force_heap_alloc (1<<19)
As before, a separate new sub-op would look to me as being the cleaner
approach, avoiding the need to consume a bit position for something not
even going to be used on all architectures.
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -192,6 +192,13 @@ struct npfec {
> /* memflags: */
> #define _MEMF_no_refcount 0
> #define MEMF_no_refcount (1U<<_MEMF_no_refcount)
> +/*
> + * Alias of _MEMF_no_refcount to avoid introduction of a new, single-use
> flag.
> + * This flag should be used for populate_physmap() only as a re-purposing of
> + * _MEMF_no_refcount to force a non-1:1 allocation from domheap.
> + */
> +#define _MEMF_force_heap_alloc _MEMF_no_refcount
> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
Given its purpose and scope, this alias wants to be local to common/memory.c.
Jan
