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/8/2024 3:03 PM, Jan Beulich wrote:
> On 08.04.2024 08:59, Henry Wang wrote:
> > Hi Jan,
> >
> > On 4/8/2024 2:22 PM, Jan Beulich wrote:
> > > On 08.04.2024 05:19, Henry Wang wrote:
> > > > On 4/4/2024 5:38 PM, Jan Beulich wrote:
> > > > > On 03.04.2024 10:16, Henry Wang wrote:
> > > > > > --- 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.
> > > I'm curious what amount of duplication you still see left. By suitably
> > > adding a new parameter, there should be very little left.
> > The duplication I see so far is basically the exact
> > xc_domain_populate_physmap(), say
> > xc_domain_populate_physmap_heap_alloc(). In init-dom0less.c, We can
> > replace the original call xc_domain_populate_physmap_exact() to call the
> > newly added xc_domain_populate_physmap_heap_alloc() which evokes the new
> > sub-op, then from the hypervisor side we set the alias MEMF flag and
> > share the populate_physmap().
> >
> > Adding a new parameter to xc_domain_populate_physmap() or maybe even
> > xc_domain_populate_physmap_exact() is also a good idea (thanks). I was
> > just worrying there are already too many use cases of these two
> > functions in the existing code: there are 14 for
> > xc_domain_populate_physmap_exact() and 8 for
> > xc_domain_populate_physmap(). Adding a new parameter needs the update of
> > all these and the function declaration. If you really insist this way, I
> > can do this, sure.
> You don't need to change all the callers. You can morph
> xc_domain_populate_physmap() into an internal helper, which a new trivial
> wrapper named xc_domain_populate_physmap() would then call, alongside with
> the new trivial wrapper you want to introduce.
Thanks for the good suggestion. Would below key diff make sense to you 
(naming can be further discussed)? Also by checking the code, if we go 
this way, maybe we can even simplify the 
xc_domain_decrease_reservation() and xc_domain_increase_reservation()? 
(Although there are some hardcoded hypercall name in the error message 
and some small differences between the memflags)
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 8363657dae..5547841e6a 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1124,12 +1124,13 @@ int xc_domain_claim_pages(xc_interface *xch,
     return err;
 }

-int xc_domain_populate_physmap(xc_interface *xch,
-                               uint32_t domid,
-                               unsigned long nr_extents,
-                               unsigned int extent_order,
-                               unsigned int mem_flags,
-                               xen_pfn_t *extent_start)
+static int xc_populate_physmap_cmd(xc_interface *xch,
+                                   unsigned int cmd;
+                                   uint32_t domid,
+                                   unsigned long nr_extents,
+                                   unsigned int extent_order,
+                                   unsigned int mem_flags,
+                                   xen_pfn_t *extent_start)
 {
     int err;
     DECLARE_HYPERCALL_BOUNCE(extent_start, nr_extents * 
sizeof(*extent_start), XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
@@ -1147,12 +1148,50 @@ int xc_domain_populate_physmap(xc_interface *xch,
     }
     set_xen_guest_handle(reservation.extent_start, extent_start);

-    err = xc_memory_op(xch, XENMEM_populate_physmap, &reservation, 
sizeof(reservation));
+    err = xc_memory_op(xch, cmd, &reservation, sizeof(reservation));

     xc_hypercall_bounce_post(xch, extent_start);
     return err;
 }

+int xc_domain_populate_physmap(xc_interface *xch,
+                               uint32_t domid,
+                               unsigned long nr_extents,
+                               unsigned int extent_order,
+                               unsigned int mem_flags,
+                               xen_pfn_t *extent_start)
+{
+    return xc_populate_physmap_cmd(xch, XENMEM_populate_physmap, domid,
+                                   nr_extents, extent_order, mem_flags,
+                                   extent_start);
+}
+
+int xc_domain_populate_physmap_heap_exact(xc_interface *xch,
+                                          uint32_t domid,
+                                          unsigned long nr_extents,
+                                          unsigned int extent_order,
+                                          unsigned int mem_flags,
+                                          xen_pfn_t *extent_start)
+{
+    int err;
+
+    err = xc_populate_physmap_cmd(xch, XENMEM_populate_physmap_heapalloc,
+                                  domid, nr_extents, extent_order, 
mem_flags,
+                                  extent_start);
+    if ( err == nr_extents )
+        return 0;
+
+    if ( err >= 0 )
+    {
+        DPRINTF("Failed allocation for dom %d: %ld extents of order %d\n",
+                domid, nr_extents, extent_order);
+        errno = EBUSY;
+        err = -1;
+    }
+
+    return err;
+}
+


Kind regards,
Henry

> Jan