From: Henry Wang <xin.wang2@amd.com>
To: Jan Beulich <jbeulich@suse.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Julien Grall <julien@xen.org>,
Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>, Anthony PERARD <anthony.perard@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Alec Kwapis <alec.kwapis@medtronic.com>,
<xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map
Date: Tue, 2 Apr 2024 16:43:51 +0800 [thread overview]
Message-ID: <4a0e4c60-9401-4031-bf80-66033ad068e3@amd.com> (raw)
In-Reply-To: <d9e3b7c4-d8be-4642-9212-b48ae885b46a@suse.com>
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
next prev parent reply other threads:[~2024-04-02 8:44 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-08 1:54 [PATCH v2 0/5] DOMCTL-based guest magic regions allocation for dom0less Henry Wang
2024-03-08 1:54 ` [PATCH v2 1/5] xen/arm: Rename assign_static_memory_11() for consistency Henry Wang
2024-03-08 8:18 ` Michal Orzel
2024-03-08 8:22 ` Henry Wang
2024-03-08 1:54 ` [PATCH v2 2/5] xen/domain.h: Centrialize is_domain_direct_mapped() Henry Wang
2024-03-08 8:59 ` Michal Orzel
2024-03-08 9:06 ` Henry Wang
2024-03-08 9:41 ` Jan Beulich
2024-03-11 18:02 ` Shawn Anastasio
2024-03-08 1:54 ` [PATCH v2 3/5] xen/domctl, tools: Introduce a new domctl to get guest memory map Henry Wang
2024-03-11 9:10 ` Michal Orzel
2024-03-11 9:46 ` Henry Wang
2024-03-11 16:58 ` Jan Beulich
2024-03-12 3:06 ` Henry Wang
2024-03-08 1:54 ` [PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU Henry Wang
2024-03-11 13:46 ` Michal Orzel
2024-03-11 13:50 ` Michal Orzel
2024-03-12 3:25 ` Henry Wang
2024-03-13 11:09 ` Carlo Nonato
2024-03-08 1:54 ` [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map Henry Wang
2024-03-11 17:07 ` Jan Beulich
2024-03-12 3:44 ` Henry Wang
2024-03-12 7:34 ` Jan Beulich
2024-03-12 7:36 ` Henry Wang
2024-03-29 5:11 ` Henry Wang
2024-04-02 7:05 ` Jan Beulich
2024-04-02 8:43 ` Henry Wang [this message]
2024-04-02 8:51 ` Jan Beulich
2024-04-02 9:03 ` Henry Wang
2024-03-25 15:35 ` Anthony PERARD
2024-03-26 1:21 ` Henry Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4a0e4c60-9401-4031-bf80-66033ad068e3@amd.com \
--to=xin.wang2@amd.com \
--cc=alec.kwapis@medtronic.com \
--cc=andrew.cooper3@citrix.com \
--cc=anthony.perard@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.