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

[Jan Beulich <jbeulich@xxxxxxxx>]

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