| Message ID | 20240308015435.4044339-6-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DOMCTL-based guest magic regions allocation for dom0less | expand |
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
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
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
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
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,
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
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
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
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
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
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
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)
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(-)