Mailing List Archive

[PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map
Previous commits enable the toolstack to get the domain memory map,
therefore 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>
---
v2:
- New patch
---
tools/helpers/init-dom0less.c | 22 ++++++++++++++++++----
xen/common/memory.c | 10 ++++++++--
xen/include/public/memory.h | 5 +++++
xen/include/xen/mm.h | 2 ++
4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index fee93459c4..92c612f6da 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -23,16 +23,30 @@ 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;
+ xen_pfn_t base = ((xen_pfn_t)-1);
+ xen_pfn_t p2m = ((xen_pfn_t)-1);
+ 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);
+
+ for ( i = 0; i < nr_regions; i++ )
+ {
+ if ( mem_regions[i].type == GUEST_MEM_REGION_MAGIC )
+ {
+ base = mem_regions[i].start >> XC_PAGE_SHIFT;
+ 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, &p2m);
if (rc < 0)
return rc;

diff --git a/xen/common/memory.c b/xen/common/memory.c
index b3b05c2ec0..18b6c16aed 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,
@@ -1433,6 +1435,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 bb29b352ec..a4554f730d 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -205,6 +205,8 @@ struct npfec {
#define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
#define _MEMF_no_scrub 8
#define MEMF_no_scrub (1U<<_MEMF_no_scrub)
+#define _MEMF_force_heap_alloc 9
+#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
#define _MEMF_node 16
#define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
#define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
--
2.34.1
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
I'm afraid the title doesn't really say what the patch actually means
to achieve.

On 08.03.2024 02:54, Henry Wang wrote:
> Previous commits enable the toolstack to get the domain memory map,
> therefore 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.

A patch description wants to be (largely) self-contained. "Previous
commits" shouldn't be mentioned; recall that the sequence in which
patches go in is unknown to you up front. (In fact the terms "commit"
or "patch" should be avoided altogether when describing what a patch
does. The only valid use I can think of is when referring to commits
already in the tree, and then typically by quoting their hash and
title.)

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

If this is for populate_physmap only, then other sub-ops need to reject
its use.

I have to admit I'm a little wary of allocating another flag here and ...

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -205,6 +205,8 @@ struct npfec {
> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
> #define _MEMF_no_scrub 8
> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
> +#define _MEMF_force_heap_alloc 9
> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
> #define _MEMF_node 16
> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)

... here - we don't have that many left. Since other sub-ops aren't
intended to support this flag, did you consider adding another (perhaps
even arch-specific) sub-op instead?

Jan
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
Hi Jan,

On 3/12/2024 1:07 AM, Jan Beulich wrote:
> I'm afraid the title doesn't really say what the patch actually means
> to achieve.
>
> On 08.03.2024 02:54, Henry Wang wrote:
>> Previous commits enable the toolstack to get the domain memory map,
>> therefore 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.
> A patch description wants to be (largely) self-contained. "Previous
> commits" shouldn't be mentioned; recall that the sequence in which
> patches go in is unknown to you up front. (In fact the terms "commit"
> or "patch" should be avoided altogether when describing what a patch
> does. The only valid use I can think of is when referring to commits
> already in the tree, and then typically by quoting their hash and
> title.)

Thanks for the detailed explanation. I will rewrite the title and part
of the commit message in v3 to make it clear.

>> --- 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
> If this is for populate_physmap only, then other sub-ops need to reject
> its use.
>
> I have to admit I'm a little wary of allocating another flag here and ...
>
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -205,6 +205,8 @@ struct npfec {
>> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
>> #define _MEMF_no_scrub 8
>> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
>> +#define _MEMF_force_heap_alloc 9
>> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
>> #define _MEMF_node 16
>> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
>> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
> ... here - we don't have that many left. Since other sub-ops aren't
> intended to support this flag, did you consider adding another (perhaps
> even arch-specific) sub-op instead?

Not really, I basically followed the discussion from [1] to implement
this patch. However I understand your concern. Just want to make sure if
I understand your suggestion correctly, by "adding another sub-op" you
mean adding a sub-op similar as "XENMEM_populate_physmap" but only with
executing the "else" part I want, so we can drop the use of these two
added flags? Thanks!

[1]
https://lore.kernel.org/xen-devel/3982ba47-6709-47e3-a9c2-e2d3b4a2d8e3@xen.org/

Kind regards,
Henry

> Jan
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
On 12.03.2024 04:44, Henry Wang wrote:
> On 3/12/2024 1:07 AM, Jan Beulich wrote:
>> On 08.03.2024 02:54, 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)
>>> #endif
>> If this is for populate_physmap only, then other sub-ops need to reject
>> its use.
>>
>> I have to admit I'm a little wary of allocating another flag here and ...
>>
>>> --- a/xen/include/xen/mm.h
>>> +++ b/xen/include/xen/mm.h
>>> @@ -205,6 +205,8 @@ struct npfec {
>>> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
>>> #define _MEMF_no_scrub 8
>>> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
>>> +#define _MEMF_force_heap_alloc 9
>>> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
>>> #define _MEMF_node 16
>>> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
>>> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
>> ... here - we don't have that many left. Since other sub-ops aren't
>> intended to support this flag, did you consider adding another (perhaps
>> even arch-specific) sub-op instead?
>
> Not really, I basically followed the discussion from [1] to implement
> this patch. However I understand your concern. Just want to make sure if
> I understand your suggestion correctly, by "adding another sub-op" you
> mean adding a sub-op similar as "XENMEM_populate_physmap" but only with
> executing the "else" part I want, so we can drop the use of these two
> added flags? Thanks!
>
> [1]
> https://lore.kernel.org/xen-devel/3982ba47-6709-47e3-a9c2-e2d3b4a2d8e3@xen.org/

In which case please check with Julien (and perhaps other Arm maintainers)
before deciding on whether to go this alternative route.

Jan
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
Hi Jan,

On 3/12/2024 3:34 PM, Jan Beulich wrote:
> On 12.03.2024 04:44, Henry Wang wrote:
>> On 3/12/2024 1:07 AM, Jan Beulich wrote:
>>> On 08.03.2024 02:54, 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)
>>>> #endif
>>> If this is for populate_physmap only, then other sub-ops need to reject
>>> its use.
>>>
>>> I have to admit I'm a little wary of allocating another flag here and ...
>>>
>>>> --- a/xen/include/xen/mm.h
>>>> +++ b/xen/include/xen/mm.h
>>>> @@ -205,6 +205,8 @@ struct npfec {
>>>> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
>>>> #define _MEMF_no_scrub 8
>>>> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
>>>> +#define _MEMF_force_heap_alloc 9
>>>> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
>>>> #define _MEMF_node 16
>>>> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
>>>> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
>>> ... here - we don't have that many left. Since other sub-ops aren't
>>> intended to support this flag, did you consider adding another (perhaps
>>> even arch-specific) sub-op instead?
>> Not really, I basically followed the discussion from [1] to implement
>> this patch. However I understand your concern. Just want to make sure if
>> I understand your suggestion correctly, by "adding another sub-op" you
>> mean adding a sub-op similar as "XENMEM_populate_physmap" but only with
>> executing the "else" part I want, so we can drop the use of these two
>> added flags? Thanks!
>>
>> [1]
>> https://lore.kernel.org/xen-devel/3982ba47-6709-47e3-a9c2-e2d3b4a2d8e3@xen.org/
> In which case please check with Julien (and perhaps other Arm maintainers)
> before deciding on whether to go this alternative route.

Yes sure, I will wait a bit longer for the agreement of the discussion
before implementing the code.

Kind regards,
Henry

> Jan
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
On Fri, Mar 08, 2024 at 09:54:35AM +0800, Henry Wang wrote:
> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> index fee93459c4..92c612f6da 100644
> --- a/tools/helpers/init-dom0less.c
> +++ b/tools/helpers/init-dom0less.c
> @@ -23,16 +23,30 @@ 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;
> + xen_pfn_t base = ((xen_pfn_t)-1);
> + xen_pfn_t p2m = ((xen_pfn_t)-1);
> + 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);

Shouldn't you check the value of in `rc`?

> + for ( i = 0; i < nr_regions; i++ )
> + {
> + if ( mem_regions[i].type == GUEST_MEM_REGION_MAGIC )
> + {
> + base = mem_regions[i].start >> XC_PAGE_SHIFT;
> + p2m = (mem_regions[i].start >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
> + }
> + }

Thanks,

--
Anthony PERARD
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
Hi Anthony,

On 3/25/2024 11:35 PM, Anthony PERARD wrote:
> On Fri, Mar 08, 2024 at 09:54:35AM +0800, Henry Wang wrote:
>> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
>> index fee93459c4..92c612f6da 100644
>> --- a/tools/helpers/init-dom0less.c
>> +++ b/tools/helpers/init-dom0less.c
>> @@ -23,16 +23,30 @@ 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;
>> + xen_pfn_t base = ((xen_pfn_t)-1);
>> + xen_pfn_t p2m = ((xen_pfn_t)-1);
>> + 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);
> Shouldn't you check the value of in `rc`?
Yes I should have done so. I will do that in the next version. Thanks!
Kind regards, Henry
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
Hi Jan,

On 3/12/2024 1:07 AM, Jan Beulich wrote:
>> +/*
>> + * 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
> If this is for populate_physmap only, then other sub-ops need to reject
> its use.
>
> I have to admit I'm a little wary of allocating another flag here and ...
>
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -205,6 +205,8 @@ struct npfec {
>> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
>> #define _MEMF_no_scrub 8
>> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
>> +#define _MEMF_force_heap_alloc 9
>> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
>> #define _MEMF_node 16
>> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
>> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
> ... here - we don't have that many left. Since other sub-ops aren't
> intended to support this flag, did you consider adding another (perhaps
> even arch-specific) sub-op instead?

While revisiting this comment when trying to come up with a V3, I
realized adding a sub-op here in the same level as
XENMEM_populate_physmap will basically duplicate the function
populate_physmap() with just the "else" (the non-1:1 allocation) part,
also a similar xc_domain_populate_physmap_exact() & co will be needed
from the toolstack side to call the new sub-op. So I am having the
concern of the duplication of code and not sure if I understand you
correctly. Would you please elaborate a bit more or clarify if I
understand you correctly? Thanks!

Kind regards,
Henry

> Jan
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
On 29.03.2024 06:11, Henry Wang wrote:
> On 3/12/2024 1:07 AM, Jan Beulich wrote:
>>> +/*
>>> + * 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
>> If this is for populate_physmap only, then other sub-ops need to reject
>> its use.
>>
>> I have to admit I'm a little wary of allocating another flag here and ...
>>
>>> --- a/xen/include/xen/mm.h
>>> +++ b/xen/include/xen/mm.h
>>> @@ -205,6 +205,8 @@ struct npfec {
>>> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
>>> #define _MEMF_no_scrub 8
>>> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
>>> +#define _MEMF_force_heap_alloc 9
>>> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
>>> #define _MEMF_node 16
>>> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
>>> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
>> ... here - we don't have that many left. Since other sub-ops aren't
>> intended to support this flag, did you consider adding another (perhaps
>> even arch-specific) sub-op instead?
>
> While revisiting this comment when trying to come up with a V3, I
> realized adding a sub-op here in the same level as
> XENMEM_populate_physmap will basically duplicate the function
> populate_physmap() with just the "else" (the non-1:1 allocation) part,
> also a similar xc_domain_populate_physmap_exact() & co will be needed
> from the toolstack side to call the new sub-op. So I am having the
> concern of the duplication of code and not sure if I understand you
> correctly. Would you please elaborate a bit more or clarify if I
> understand you correctly? Thanks!

Well, the goal is to avoid both code duplication and introduction of a new,
single-use flag. The new sub-op suggestion, I realize now, would mainly have
helped with avoiding the new flag in the public interface. That's still
desirable imo. Internally, have you checked which MEMF_* are actually used
by populate_physmap()? Briefly looking, e.g. MEMF_no_dma and MEMF_no_refcount
aren't. It therefore would be possible to consider re-purposing one that
isn't (likely to be) used there. Of course doing so requires care to avoid
passing that flag down to other code (page_alloc.c functions in particular),
where the meaning would be the original one.

Jan

Jan
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
Hi Jan,

On 4/2/2024 3:05 PM, Jan Beulich wrote:
> On 29.03.2024 06:11, Henry Wang wrote:
>> On 3/12/2024 1:07 AM, Jan Beulich wrote:
>>>> +/*
>>>> + * 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
>>> If this is for populate_physmap only, then other sub-ops need to reject
>>> its use.
>>>
>>> I have to admit I'm a little wary of allocating another flag here and ...
>>>
>>>> --- a/xen/include/xen/mm.h
>>>> +++ b/xen/include/xen/mm.h
>>>> @@ -205,6 +205,8 @@ struct npfec {
>>>> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
>>>> #define _MEMF_no_scrub 8
>>>> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
>>>> +#define _MEMF_force_heap_alloc 9
>>>> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
>>>> #define _MEMF_node 16
>>>> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
>>>> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
>>> ... here - we don't have that many left. Since other sub-ops aren't
>>> intended to support this flag, did you consider adding another (perhaps
>>> even arch-specific) sub-op instead?
>> While revisiting this comment when trying to come up with a V3, I
>> realized adding a sub-op here in the same level as
>> XENMEM_populate_physmap will basically duplicate the function
>> populate_physmap() with just the "else" (the non-1:1 allocation) part,
>> also a similar xc_domain_populate_physmap_exact() & co will be needed
>> from the toolstack side to call the new sub-op. So I am having the
>> concern of the duplication of code and not sure if I understand you
>> correctly. Would you please elaborate a bit more or clarify if I
>> understand you correctly? Thanks!
> Well, the goal is to avoid both code duplication and introduction of a new,
> single-use flag. The new sub-op suggestion, I realize now, would mainly have
> helped with avoiding the new flag in the public interface. That's still
> desirable imo. Internally, have you checked which MEMF_* are actually used
> by populate_physmap()? Briefly looking, e.g. MEMF_no_dma and MEMF_no_refcount
> aren't. It therefore would be possible to consider re-purposing one that
> isn't (likely to be) used there. Of course doing so requires care to avoid
> passing that flag down to other code (page_alloc.c functions in particular),
> where the meaning would be the original one.

I think you made a good point, however, to be honest I am not sure about
the repurposing flags such as MEMF_no_dma and MEMF_no_refcount, because
I think the name and the purpose of the flag should be clear and
less-misleading. Reusing either one for another meaning, namely forcing
a non-heap allocation in populate_physmap() would be confusing in the
future. Also if one day these flags will be needed in
populate_physmap(), current repurposing approach will lead to a even
confusing code base.

I also do agree very much that we need to also avoid the code
duplication, so compared to other two suggested approach, adding a new
MEMF would be the cleanest solution IMHO, as it is just one bit and MEMF
flags are not added very often.

I would also curious what the other common code maintainers will say on
this issue: @Andrew, @Stefano, @Julien, any ideas? Thanks!

Kind regards,
Henry

> Jan
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
On 02.04.2024 10:43, Henry Wang wrote:
> On 4/2/2024 3:05 PM, Jan Beulich wrote:
>> On 29.03.2024 06:11, Henry Wang wrote:
>>> On 3/12/2024 1:07 AM, Jan Beulich wrote:
>>>>> +/*
>>>>> + * 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
>>>> If this is for populate_physmap only, then other sub-ops need to reject
>>>> its use.
>>>>
>>>> I have to admit I'm a little wary of allocating another flag here and ...
>>>>
>>>>> --- a/xen/include/xen/mm.h
>>>>> +++ b/xen/include/xen/mm.h
>>>>> @@ -205,6 +205,8 @@ struct npfec {
>>>>> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
>>>>> #define _MEMF_no_scrub 8
>>>>> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
>>>>> +#define _MEMF_force_heap_alloc 9
>>>>> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
>>>>> #define _MEMF_node 16
>>>>> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
>>>>> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
>>>> ... here - we don't have that many left. Since other sub-ops aren't
>>>> intended to support this flag, did you consider adding another (perhaps
>>>> even arch-specific) sub-op instead?
>>> While revisiting this comment when trying to come up with a V3, I
>>> realized adding a sub-op here in the same level as
>>> XENMEM_populate_physmap will basically duplicate the function
>>> populate_physmap() with just the "else" (the non-1:1 allocation) part,
>>> also a similar xc_domain_populate_physmap_exact() & co will be needed
>>> from the toolstack side to call the new sub-op. So I am having the
>>> concern of the duplication of code and not sure if I understand you
>>> correctly. Would you please elaborate a bit more or clarify if I
>>> understand you correctly? Thanks!
>> Well, the goal is to avoid both code duplication and introduction of a new,
>> single-use flag. The new sub-op suggestion, I realize now, would mainly have
>> helped with avoiding the new flag in the public interface. That's still
>> desirable imo. Internally, have you checked which MEMF_* are actually used
>> by populate_physmap()? Briefly looking, e.g. MEMF_no_dma and MEMF_no_refcount
>> aren't. It therefore would be possible to consider re-purposing one that
>> isn't (likely to be) used there. Of course doing so requires care to avoid
>> passing that flag down to other code (page_alloc.c functions in particular),
>> where the meaning would be the original one.
>
> I think you made a good point, however, to be honest I am not sure about
> the repurposing flags such as MEMF_no_dma and MEMF_no_refcount, because
> I think the name and the purpose of the flag should be clear and
> less-misleading. Reusing either one for another meaning, namely forcing
> a non-heap allocation in populate_physmap() would be confusing in the
> future. Also if one day these flags will be needed in
> populate_physmap(), current repurposing approach will lead to a even
> confusing code base.

For the latter - hence "(likely to be)" in my earlier reply.

For the naming - of course an aliasing #define ought to be used then, to
make the purpose clear at the use sites.

Jan

> I also do agree very much that we need to also avoid the code
> duplication, so compared to other two suggested approach, adding a new
> MEMF would be the cleanest solution IMHO, as it is just one bit and MEMF
> flags are not added very often.
>
> I would also curious what the other common code maintainers will say on
> this issue: @Andrew, @Stefano, @Julien, any ideas? Thanks!
>
> Kind regards,
> Henry
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map [ In reply to ]
Hi Jan,

On 4/2/2024 4:51 PM, Jan Beulich wrote:
> On 02.04.2024 10:43, Henry Wang wrote:
>> On 4/2/2024 3:05 PM, Jan Beulich wrote:
>>> On 29.03.2024 06:11, Henry Wang wrote:
>>>> On 3/12/2024 1:07 AM, Jan Beulich wrote:
>>>>>> +/*
>>>>>> + * 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
>>>>> If this is for populate_physmap only, then other sub-ops need to reject
>>>>> its use.
>>>>>
>>>>> I have to admit I'm a little wary of allocating another flag here and ...
>>>>>
>>>>>> --- a/xen/include/xen/mm.h
>>>>>> +++ b/xen/include/xen/mm.h
>>>>>> @@ -205,6 +205,8 @@ struct npfec {
>>>>>> #define MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
>>>>>> #define _MEMF_no_scrub 8
>>>>>> #define MEMF_no_scrub (1U<<_MEMF_no_scrub)
>>>>>> +#define _MEMF_force_heap_alloc 9
>>>>>> +#define MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
>>>>>> #define _MEMF_node 16
>>>>>> #define MEMF_node_mask ((1U << (8 * sizeof(nodeid_t))) - 1)
>>>>>> #define MEMF_node(n) ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
>>>>> ... here - we don't have that many left. Since other sub-ops aren't
>>>>> intended to support this flag, did you consider adding another (perhaps
>>>>> even arch-specific) sub-op instead?
>>>> While revisiting this comment when trying to come up with a V3, I
>>>> realized adding a sub-op here in the same level as
>>>> XENMEM_populate_physmap will basically duplicate the function
>>>> populate_physmap() with just the "else" (the non-1:1 allocation) part,
>>>> also a similar xc_domain_populate_physmap_exact() & co will be needed
>>>> from the toolstack side to call the new sub-op. So I am having the
>>>> concern of the duplication of code and not sure if I understand you
>>>> correctly. Would you please elaborate a bit more or clarify if I
>>>> understand you correctly? Thanks!
>>> Well, the goal is to avoid both code duplication and introduction of a new,
>>> single-use flag. The new sub-op suggestion, I realize now, would mainly have
>>> helped with avoiding the new flag in the public interface. That's still
>>> desirable imo. Internally, have you checked which MEMF_* are actually used
>>> by populate_physmap()? Briefly looking, e.g. MEMF_no_dma and MEMF_no_refcount
>>> aren't. It therefore would be possible to consider re-purposing one that
>>> isn't (likely to be) used there. Of course doing so requires care to avoid
>>> passing that flag down to other code (page_alloc.c functions in particular),
>>> where the meaning would be the original one.
>> I think you made a good point, however, to be honest I am not sure about
>> the repurposing flags such as MEMF_no_dma and MEMF_no_refcount, because
>> I think the name and the purpose of the flag should be clear and
>> less-misleading. Reusing either one for another meaning, namely forcing
>> a non-heap allocation in populate_physmap() would be confusing in the
>> future. Also if one day these flags will be needed in
>> populate_physmap(), current repurposing approach will lead to a even
>> confusing code base.
> For the latter - hence "(likely to be)" in my earlier reply.

Agreed.

> For the naming - of course an aliasing #define ought to be used then, to
> make the purpose clear at the use sites.

Well I have to admit the alias #define approach is clever (thanks) and I
am getting persuaded gradually, as there will be also another benefit
for me to do less modification from my side :) I will firstly try this
approach in v3 if ...

>> I also do agree very much that we need to also avoid the code
>> duplication, so compared to other two suggested approach, adding a new
>> MEMF would be the cleanest solution IMHO, as it is just one bit and MEMF
>> flags are not added very often.
>>
>> I would also curious what the other common code maintainers will say on
>> this issue: @Andrew, @Stefano, @Julien, any ideas? Thanks!

...not receiving any other input, and we can continue the discussion in
v3. Thanks!

Kind regards,
Henry

> Jan