[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
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. > @@ -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()? > @@ -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. > @@ -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; > - default: /* XENMEM_populate_physmap */ > + default: /* XENMEM_populate_{physmap, physmap_heap_alloc} */ Otherwise: Just XENMEM_populate_physmap{,_heap_alloc} perhaps? > --- 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 29 Without 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. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |