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