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

[Henry Wang <xin.wang2@xxxxxxx>]

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