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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan