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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Jan,

On 4/4/2024 5:38 PM, Jan Beulich wrote:
> 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().
Ok, I will use &= and drop the if here.

> > @@ -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()).
Ok I will check and do the combining as suggested. Thanks!

> > --- 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.
Like discussed in v2, I doubt that if introducing a new sub-op, the 
helpers added to duplicate mainly populate_physmap() and the toolstack 
helpers would be a good idea. Similarly as the way that we do for the 
MEMF_force_heap_alloc, if in the future we run out of the bit positions, 
can't we reuse this bit for different architectures as an alias? Or 
maybe we can even alias it now?
> > --- 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.
Sure, I will move it to common/memory.c

Kind regards,
Henry

> Jan