Re: [PATCH v3 4/4] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.04.2024 10:16, Henry Wang wrote:
> --- 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,
> @@ -271,6 +273,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) )
> +                    clear_bit(_MEMF_force_heap_alloc, &a->memflags);

Why an atomic operation? &= will to quite fine here. And you can also
drop the if().

> @@ -1408,6 +1418,10 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&reservation, arg, 1) )
>              return start_extent;
>  
> +        if ( op != XENMEM_populate_physmap
> +             && (reservation.mem_flags & XENMEMF_force_heap_alloc) )
> +            return -EINVAL;
> +
>          /* Is size too large for us to encode a continuation? */
>          if ( reservation.nr_extents > (UINT_MAX >> MEMOP_EXTENT_SHIFT) )
>              return start_extent;
> @@ -1433,6 +1447,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 in the end no new sub-op is used (see below), this and the earlier if()
want combining.

You further may want to assert that the flag isn't already set (as coming
back from construct_memop_from_reservation()).

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

As before, a separate new sub-op would look to me as being the cleaner
approach, avoiding the need to consume a bit position for something not
even going to be used on all architectures.

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -192,6 +192,13 @@ struct npfec {
>  /* memflags: */
>  #define _MEMF_no_refcount 0
>  #define  MEMF_no_refcount (1U<<_MEMF_no_refcount)
> +/*
> + * 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)

Given its purpose and scope, this alias wants to be local to common/memory.c.

Jan