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 4:26 PM, Jan Beulich wrote:
> On 08.04.2024 10:12, Henry Wang wrote:
> > 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
> Yes.
Thanks for confirming!

> > (naming can be further discussed)?
> Personally I wouldn't use xc_ on internal helpers. But for guidance on
> naming in the libraries the maintainer(s) would need consulting.
Sure, thanks for sharing your idea. Well I added xc_ prefix because 
seeing static int xc_domain_pod_target() & int xc_domain_{set, 
get}_pod_target() etc. pairs in the existing code of the file. I agree 
that the opinion of libxc/toolstack maintainers matter too, and hence we 
can continue this discussion in the formal v4.
> > 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)
> There may be room for improvement there, but as you say, some care would
> need applying.
Yeah...not really sure if it is ok (TBH I think not) to do it all 
together in this patch, I will probably add another patch to clean-up 
all these duplications when sending v4 (depending on how far I can go).
Kind regards,
Henry

> Jan