[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 4/5] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less
Hi Jan, On 4/18/2024 8:54 PM, Jan Beulich wrote: On 09.04.2024 06:53, Henry Wang wrote:--- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -155,6 +155,14 @@ static void increase_reservation(struct memop_args *a) a->nr_done = i; }+/*+ * 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)Nit (style): Blanks around << please. Also do you really need both constants? I dont think so. Plus please make sure to #undef the constant once no longer needed, to help spotting / avoiding misuses. Sounds good, I will fix the NIT, drop the first #define and properly add #undef. @@ -219,7 +227,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 +255,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 +281,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) ) + a->memflags &= ~MEMF_force_heap_alloc;As asked before: Why the if()? I think there is no need to clear the flag if it is not set. But you are correct, the if is not needed. I can drop it. @@ -1404,6 +1422,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { case XENMEM_increase_reservation: case XENMEM_decrease_reservation: + case XENMEM_populate_physmap_heap_alloc: case XENMEM_populate_physmap: if ( copy_from_guest(&reservation, arg, 1) ) return start_extent;Nit or not: Please insert the new case label last. Sure. @@ -1433,6 +1452,11 @@ 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;+ /* Assert flag is not set from construct_memop_from_reservation(). */+ ASSERT(!(args.memflags & MEMF_force_heap_alloc)); + if ( op == XENMEM_populate_physmap_heap_alloc ) + args.memflags |= MEMF_force_heap_alloc;Wouldn't this more logically live ...if ( xsm_memory_adjust_reservation(XSM_TARGET, curr_d, d) ) { rcu_unlock_domain(d); @@ -1453,7 +1477,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case XENMEM_decrease_reservation: decrease_reservation(&args); break;here, as case XENMEM_populate_physmap_heap_alloc: ... fallthrough; Ok. - default: /* XENMEM_populate_physmap */ + default: /* XENMEM_populate_{physmap, physmap_heap_alloc} */Otherwise: Just XENMEM_populate_physmap{,_heap_alloc} perhaps? Sounds good, thanks for the suggestion. --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -21,6 +21,7 @@ #define XENMEM_increase_reservation 0 #define XENMEM_decrease_reservation 1 #define XENMEM_populate_physmap 6 +#define XENMEM_populate_physmap_heap_alloc 29Without a comment, how is one supposed to know what the difference is of this new sub-op compared to the "normal" one? I actually wonder whether referring to a Xen internal (allocation requested to come from the heap) is actually a good idea here. I'm inclined to suggest to name this after the purpose it has from the guest or tool stack perspective. Speaking of which: Is this supposed to be guest-accessible, or is it intended for tool-stack use only (I have to admit I don't even know where init-dom0less actually runs)? In the latter case that also wants enforcing. This may require an adjustment to the XSM hook in use here. Cc-ing Daniel for possible advice. This sub-op should be called by the init-dom0less application (toolstack side), which runs in Dom0. Daniel has proposed an alternative solution which is based on the hypfs. If we decide to go that route, I think I will rewrite the series. I will wait for the discussion settled. Thanks for looping in Daniel! Kind regards, Henry Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |