Hi Jan,
On 4/4/2024 5:38 PM, Jan Beulich wrote:
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().
Ok, I will use &= and drop the if here.
@@ -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()).
Ok I will check and do the combining as suggested. Thanks!
--- 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.
Like discussed in v2, I doubt that if introducing a new sub-op, the
helpers added to duplicate mainly populate_physmap() and the toolstack
helpers would be a good idea. Similarly as the way that we do for the
MEMF_force_heap_alloc, if in the future we run out of the bit positions,
can't we reuse this bit for different architectures as an alias? Or
maybe we can even alias it now?
--- 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.
Sure, I will move it to common/memory.c
Kind regards,
Henry
Jan
