Mailing List Archive: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC

[PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less

Apr 3, 2024, 1:16 AM

Post #1 of 9 (19 views)

Currently the GUEST_MAGIC_BASE in the init-dom0less application is
hardcoded, which will lead to failures for 1:1 direct-mapped Dom0less
DomUs.

Instead of hardcoding the guest magic pages region, use the
XEN_DOMCTL_get_mem_map domctl to get the start address of the guest
magic pages region. Add the (XEN)MEMF_force_heap_alloc memory
flags to force populate_physmap() to allocate page from domheap
instead of using 1:1 or static allocated pages to map the magic pages.

Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v3:
- Don't ignore the error from xc_get_domain_mem_map().
- Re-purposing the _MEMF_no_refcount as _MEMF_force_heap_alloc to
avoid introduction of a new, single-use flag.
- Reject other reservation sub-ops to use the newly added flag.
- Replace all the GUEST_MAGIC_BASE usages.
v2:
- New patch
---
tools/helpers/init-dom0less.c | 36 +++++++++++++++++++++++++----------
xen/common/memory.c | 22 +++++++++++++++++++--
xen/include/public/memory.h | 5 +++++
xen/include/xen/mm.h | 7 +++++++
4 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index fee93459c4..8eec4d1f49 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -19,24 +19,43 @@
#define XENSTORE_PFN_OFFSET 1
#define STR_MAX_LENGTH 128

+static xen_pfn_t xs_page_base;
+static xen_pfn_t xs_page_p2m;
+
static int alloc_xs_page(struct xc_interface_core *xch,
libxl_dominfo *info,
uint64_t *xenstore_pfn)
{
- int rc;
- const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
- xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
+ int rc, i;
+ uint32_t nr_regions = XEN_MAX_MEM_REGIONS;
+ struct xen_mem_region mem_regions[XEN_MAX_MEM_REGIONS] = {0};
+
+ rc = xc_get_domain_mem_map(xch, info->domid, mem_regions, &nr_regions);
+ if (rc < 0)
+ return rc;
+
+ for ( i = 0; i < nr_regions; i++ )
+ {
+ if ( mem_regions[i].type == GUEST_MEM_REGION_MAGIC )
+ {
+ xs_page_base = mem_regions[i].start >> XC_PAGE_SHIFT;
+ xs_page_p2m = (mem_regions[i].start >> XC_PAGE_SHIFT) +
+ XENSTORE_PFN_OFFSET;
+ }
+ }

rc = xc_domain_setmaxmem(xch, info->domid,
info->max_memkb + (XC_PAGE_SIZE/1024));
if (rc < 0)
return rc;

- rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
+ rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0,
+ XENMEMF_force_heap_alloc,
+ &xs_page_p2m);
if (rc < 0)
return rc;

- *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+ *xenstore_pfn = xs_page_base + XENSTORE_PFN_OFFSET;
rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
if (rc < 0)
return rc;
@@ -145,8 +164,7 @@ static int create_xenstore(struct xs_handle *xsh,
rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%"PRIu64, info->current_memkb);
if (rc < 0 || rc >= STR_MAX_LENGTH)
return rc;
- rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
- (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
+ rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%"PRIu_xen_pfn, xs_page_p2m);
if (rc < 0 || rc >= STR_MAX_LENGTH)
return rc;
rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
@@ -282,9 +300,7 @@ static int init_domain(struct xs_handle *xsh,
if (rc)
err(1, "writing to xenstore");

- rc = xs_introduce_domain(xsh, info->domid,
- (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
- xenstore_evtchn);
+ rc = xs_introduce_domain(xsh, info->domid, xs_page_p2m, xenstore_evtchn);
if (!rc)
err(1, "xs_introduce_domain");
return 0;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index b3b05c2ec0..4f8c665870 100644
--- 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);
+
page = alloc_domheap_pages(d, a->extent_order, a->memflags);

if ( unlikely(!page) )
@@ -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 ( xsm_memory_adjust_reservation(XSM_TARGET, curr_d, d) )
{
rcu_unlock_domain(d);
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 5e545ae9a4..2a1bfa5bfa 100644
--- 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)
#endif

struct xen_memory_reservation {
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3e84960a36..0f65d84532 100644
--- 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)
#define _MEMF_populate_on_demand 1
#define MEMF_populate_on_demand (1U<<_MEMF_populate_on_demand)
#define _MEMF_no_dma 3
--
2.34.1

Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less [ In reply to ]

jbeulich at suse

Apr 4, 2024, 2:38 AM

Post #2 of 9 (16 views)

Permalink

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

Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less [ In reply to ]

xin.wang2 at amd

Apr 7, 2024, 8:19 PM

Post #3 of 9 (9 views)

Permalink

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

Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less [ In reply to ]

jbeulich at suse

Apr 7, 2024, 11:22 PM

Post #4 of 9 (9 views)

Permalink

On 08.04.2024 05:19, Henry Wang wrote:
> On 4/4/2024 5:38 PM, Jan Beulich wrote:
>> On 03.04.2024 10:16, Henry Wang wrote:
>>> --- 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.

I'm curious what amount of duplication you still see left. By suitably
adding a new parameter, there should be very little left.

> 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?

I view this as far less desirable in the public interface.

Jan

Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less [ In reply to ]

xin.wang2 at amd

Apr 7, 2024, 11:59 PM

Post #5 of 9 (9 views)

Permalink

Hi Jan,

On 4/8/2024 2:22 PM, Jan Beulich wrote:
> On 08.04.2024 05:19, Henry Wang wrote:
>> On 4/4/2024 5:38 PM, Jan Beulich wrote:
>>> On 03.04.2024 10:16, Henry Wang wrote:
>>>> --- 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.
> I'm curious what amount of duplication you still see left. By suitably
> adding a new parameter, there should be very little left.

The duplication I see so far is basically the exact
xc_domain_populate_physmap(), say
xc_domain_populate_physmap_heap_alloc(). In init-dom0less.c, We can
replace the original call xc_domain_populate_physmap_exact() to call the
newly added xc_domain_populate_physmap_heap_alloc() which evokes the new
sub-op, then from the hypervisor side we set the alias MEMF flag and
share the populate_physmap().

Adding a new parameter to xc_domain_populate_physmap() or maybe even
xc_domain_populate_physmap_exact() is also a good idea (thanks). I was
just worrying there are already too many use cases of these two
functions in the existing code: there are 14 for
xc_domain_populate_physmap_exact() and 8 for
xc_domain_populate_physmap(). Adding a new parameter needs the update of
all these and the function declaration. If you really insist this way, I
can do this, sure.

>> 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?
> I view this as far less desirable in the public interface.

I agree.

Kind regards,
Henry

> Jan

Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less [ In reply to ]

jbeulich at suse

Apr 8, 2024, 12:03 AM

Post #6 of 9 (9 views)

Permalink

On 08.04.2024 08:59, Henry Wang wrote:
> Hi Jan,
>
> On 4/8/2024 2:22 PM, Jan Beulich wrote:
>> On 08.04.2024 05:19, Henry Wang wrote:
>>> On 4/4/2024 5:38 PM, Jan Beulich wrote:
>>>> On 03.04.2024 10:16, Henry Wang wrote:
>>>>> --- 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.
>> I'm curious what amount of duplication you still see left. By suitably
>> adding a new parameter, there should be very little left.
>
> The duplication I see so far is basically the exact
> xc_domain_populate_physmap(), say
> xc_domain_populate_physmap_heap_alloc(). In init-dom0less.c, We can
> replace the original call xc_domain_populate_physmap_exact() to call the
> newly added xc_domain_populate_physmap_heap_alloc() which evokes the new
> sub-op, then from the hypervisor side we set the alias MEMF flag and
> share the populate_physmap().
>
> Adding a new parameter to xc_domain_populate_physmap() or maybe even
> xc_domain_populate_physmap_exact() is also a good idea (thanks). I was
> just worrying there are already too many use cases of these two
> functions in the existing code: there are 14 for
> xc_domain_populate_physmap_exact() and 8 for
> xc_domain_populate_physmap(). Adding a new parameter needs the update of
> all these and the function declaration. If you really insist this way, I
> can do this, sure.

You don't need to change all the callers. You can morph
xc_domain_populate_physmap() into an internal helper, which a new trivial
wrapper named xc_domain_populate_physmap() would then call, alongside with
the new trivial wrapper you want to introduce.

Jan

Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less [ In reply to ]

xin.wang2 at amd

Apr 8, 2024, 1:12 AM

Post #7 of 9 (9 views)

Permalink

Hi Jan,

On 4/8/2024 3:03 PM, Jan Beulich wrote:
> On 08.04.2024 08:59, Henry Wang wrote:
>> Hi Jan,
>>
>> On 4/8/2024 2:22 PM, Jan Beulich wrote:
>>> On 08.04.2024 05:19, Henry Wang wrote:
>>>> On 4/4/2024 5:38 PM, Jan Beulich wrote:
>>>>> On 03.04.2024 10:16, Henry Wang wrote:
>>>>>> --- 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.
>>> I'm curious what amount of duplication you still see left. By suitably
>>> adding a new parameter, there should be very little left.
>> The duplication I see so far is basically the exact
>> xc_domain_populate_physmap(), say
>> xc_domain_populate_physmap_heap_alloc(). In init-dom0less.c, We can
>> replace the original call xc_domain_populate_physmap_exact() to call the
>> newly added xc_domain_populate_physmap_heap_alloc() which evokes the new
>> sub-op, then from the hypervisor side we set the alias MEMF flag and
>> share the populate_physmap().
>>
>> Adding a new parameter to xc_domain_populate_physmap() or maybe even
>> xc_domain_populate_physmap_exact() is also a good idea (thanks). I was
>> just worrying there are already too many use cases of these two
>> functions in the existing code: there are 14 for
>> xc_domain_populate_physmap_exact() and 8 for
>> xc_domain_populate_physmap(). Adding a new parameter needs the update of
>> all these and the function declaration. If you really insist this way, I
>> can do this, sure.
> You don't need to change all the callers. You can morph
> xc_domain_populate_physmap() into an internal helper, which a new trivial
> wrapper named xc_domain_populate_physmap() would then call, alongside with
> the new trivial wrapper you want to introduce.

Thanks for the good suggestion. Would below key diff make sense to you
(naming can be further discussed)? Also by checking the code, if we go
this way, maybe we can even simplify the
xc_domain_decrease_reservation() and xc_domain_increase_reservation()?
(Although there are some hardcoded hypercall name in the error message
and some small differences between the memflags)

diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 8363657dae..5547841e6a 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1124,12 +1124,13 @@ int xc_domain_claim_pages(xc_interface *xch,
     return err;
}

-int xc_domain_populate_physmap(xc_interface *xch,
-                               uint32_t domid,
-                               unsigned long nr_extents,
-                               unsigned int extent_order,
-                               unsigned int mem_flags,
-                               xen_pfn_t *extent_start)
+static int xc_populate_physmap_cmd(xc_interface *xch,
+                                   unsigned int cmd;
+                                   uint32_t domid,
+                                   unsigned long nr_extents,
+                                   unsigned int extent_order,
+                                   unsigned int mem_flags,
+                                   xen_pfn_t *extent_start)
{
     int err;
     DECLARE_HYPERCALL_BOUNCE(extent_start, nr_extents *
sizeof(*extent_start), XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
@@ -1147,12 +1148,50 @@ int xc_domain_populate_physmap(xc_interface *xch,
     }
     set_xen_guest_handle(reservation.extent_start, extent_start);

-    err = xc_memory_op(xch, XENMEM_populate_physmap, &reservation,
sizeof(reservation));
+    err = xc_memory_op(xch, cmd, &reservation, sizeof(reservation));

     xc_hypercall_bounce_post(xch, extent_start);
     return err;
}

+int xc_domain_populate_physmap(xc_interface *xch,
+                               uint32_t domid,
+                               unsigned long nr_extents,
+                               unsigned int extent_order,
+                               unsigned int mem_flags,
+                               xen_pfn_t *extent_start)
+{
+    return xc_populate_physmap_cmd(xch, XENMEM_populate_physmap, domid,
+                                   nr_extents, extent_order, mem_flags,
+                                   extent_start);
+}
+
+int xc_domain_populate_physmap_heap_exact(xc_interface *xch,
+                                          uint32_t domid,
+                                          unsigned long nr_extents,
+                                          unsigned int extent_order,
+                                          unsigned int mem_flags,
+                                          xen_pfn_t *extent_start)
+{
+    int err;
+
+    err = xc_populate_physmap_cmd(xch, XENMEM_populate_physmap_heapalloc,
+                                  domid, nr_extents, extent_order,
mem_flags,
+                                  extent_start);
+    if ( err == nr_extents )
+        return 0;
+
+    if ( err >= 0 )
+    {
+        DPRINTF("Failed allocation for dom %d: %ld extents of order %d\n",
+                domid, nr_extents, extent_order);
+        errno = EBUSY;
+        err = -1;
+    }
+
+    return err;
+}
+

Kind regards,
Henry

> Jan

Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less [ In reply to ]

jbeulich at suse

Apr 8, 2024, 1:26 AM

Post #8 of 9 (9 views)

Permalink

On 08.04.2024 10:12, Henry Wang wrote:
> Hi Jan,
>
> On 4/8/2024 3:03 PM, Jan Beulich wrote:
>> On 08.04.2024 08:59, Henry Wang wrote:
>>> Hi Jan,
>>>
>>> On 4/8/2024 2:22 PM, Jan Beulich wrote:
>>>> On 08.04.2024 05:19, Henry Wang wrote:
>>>>> On 4/4/2024 5:38 PM, Jan Beulich wrote:
>>>>>> On 03.04.2024 10:16, Henry Wang wrote:
>>>>>>> --- 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.
>>>> I'm curious what amount of duplication you still see left. By suitably
>>>> adding a new parameter, there should be very little left.
>>> The duplication I see so far is basically the exact
>>> xc_domain_populate_physmap(), say
>>> xc_domain_populate_physmap_heap_alloc(). In init-dom0less.c, We can
>>> replace the original call xc_domain_populate_physmap_exact() to call the
>>> newly added xc_domain_populate_physmap_heap_alloc() which evokes the new
>>> sub-op, then from the hypervisor side we set the alias MEMF flag and
>>> share the populate_physmap().
>>>
>>> Adding a new parameter to xc_domain_populate_physmap() or maybe even
>>> xc_domain_populate_physmap_exact() is also a good idea (thanks). I was
>>> just worrying there are already too many use cases of these two
>>> functions in the existing code: there are 14 for
>>> xc_domain_populate_physmap_exact() and 8 for
>>> xc_domain_populate_physmap(). Adding a new parameter needs the update of
>>> all these and the function declaration. If you really insist this way, I
>>> can do this, sure.
>> You don't need to change all the callers. You can morph
>> xc_domain_populate_physmap() into an internal helper, which a new trivial
>> wrapper named xc_domain_populate_physmap() would then call, alongside with
>> the new trivial wrapper you want to introduce.
>
> Thanks for the good suggestion. Would below key diff make sense to you

Yes.

> (naming can be further discussed)?

Personally I wouldn't use xc_ on internal helpers. But for guidance on
naming in the libraries the maintainer(s) would need consulting.

> Also by checking the code, if we go
> this way, maybe we can even simplify the
> xc_domain_decrease_reservation() and xc_domain_increase_reservation()?
> (Although there are some hardcoded hypercall name in the error message
> and some small differences between the memflags)

There may be room for improvement there, but as you say, some care would
need applying.

Jan

Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less [ In reply to ]

xin.wang2 at amd

Apr 8, 2024, 1:34 AM

Post #9 of 9 (9 views)

Permalink

Hi Jan,

On 4/8/2024 4:26 PM, Jan Beulich wrote:
> On 08.04.2024 10:12, Henry Wang wrote:
>> Hi Jan,
>>
>> On 4/8/2024 3:03 PM, Jan Beulich wrote:
>>> On 08.04.2024 08:59, Henry Wang wrote:
>>>> Hi Jan,
>>>>
>>>> On 4/8/2024 2:22 PM, Jan Beulich wrote:
>>>>> On 08.04.2024 05:19, Henry Wang wrote:
>>>>>> On 4/4/2024 5:38 PM, Jan Beulich wrote:
>>>>>>> On 03.04.2024 10:16, Henry Wang wrote:
>>>>>>>> --- 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.
>>>>> I'm curious what amount of duplication you still see left. By suitably
>>>>> adding a new parameter, there should be very little left.
>>>> The duplication I see so far is basically the exact
>>>> xc_domain_populate_physmap(), say
>>>> xc_domain_populate_physmap_heap_alloc(). In init-dom0less.c, We can
>>>> replace the original call xc_domain_populate_physmap_exact() to call the
>>>> newly added xc_domain_populate_physmap_heap_alloc() which evokes the new
>>>> sub-op, then from the hypervisor side we set the alias MEMF flag and
>>>> share the populate_physmap().
>>>>
>>>> Adding a new parameter to xc_domain_populate_physmap() or maybe even
>>>> xc_domain_populate_physmap_exact() is also a good idea (thanks). I was
>>>> just worrying there are already too many use cases of these two
>>>> functions in the existing code: there are 14 for
>>>> xc_domain_populate_physmap_exact() and 8 for
>>>> xc_domain_populate_physmap(). Adding a new parameter needs the update of
>>>> all these and the function declaration. If you really insist this way, I
>>>> can do this, sure.
>>> You don't need to change all the callers. You can morph
>>> xc_domain_populate_physmap() into an internal helper, which a new trivial
>>> wrapper named xc_domain_populate_physmap() would then call, alongside with
>>> the new trivial wrapper you want to introduce.
>> Thanks for the good suggestion. Would below key diff make sense to you
> Yes.

Thanks for confirming!

>> (naming can be further discussed)?
> Personally I wouldn't use xc_ on internal helpers. But for guidance on
> naming in the libraries the maintainer(s) would need consulting.

Sure, thanks for sharing your idea. Well I added xc_ prefix because
seeing static int xc_domain_pod_target() & int xc_domain_{set,
get}_pod_target() etc. pairs in the existing code of the file. I agree
that the opinion of libxc/toolstack maintainers matter too, and hence we
can continue this discussion in the formal v4.

>> Also by checking the code, if we go
>> this way, maybe we can even simplify the
>> xc_domain_decrease_reservation() and xc_domain_increase_reservation()?
>> (Although there are some hardcoded hypercall name in the error message
>> and some small differences between the memflags)
> There may be room for improvement there, but as you say, some care would
> need applying.

Yeah...not really sure if it is ok (TBH I think not) to do it all
together in this patch, I will probably add another patch to clean-up
all these duplications when sending v4 (depending on how far I can go).

Kind regards,
Henry

> Jan