Re: [PATCH v2 3/5] arm/dom0less: put dom0less feature code in a separate module

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 2 Oct 2023, at 10:26, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Stefano,
> 
> On 29/09/2023 20:48, Stefano Stabellini wrote:
>> On Fri, 29 Sep 2023, Luca Fancellu wrote:
>>>> On 29 Sep 2023, at 01:33, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
>>>> wrote:
>>>> 
>>>> On Wed, 27 Sep 2023, Luca Fancellu wrote:
>>>>> Currently the dom0less feature code is mostly inside domain_build.c
>>>>> and setup.c, it is a feature that may not be useful to everyone so
>>>>> put the code in a different compilation module in order to make it
>>>>> easier to disable the feature in the future.
>>>>> 
>>>>> Move gic_interrupt_t in domain_build.h to use it with the function
>>>>> declaration, move its comment above the declaration.
>>>>> 
>>>>> The following functions are now visible externally from domain_build
>>>>> because they are used also from the dom0less-build module:
>>>>> - get_allocation_size
>>>>> - set_interrupt
>>>>> - domain_fdt_begin_node
>>>>> - make_memory_node
>>>>> - make_resv_memory_node
>>>>> - make_hypervisor_node
>>>>> - make_psci_node
>>>>> - make_cpus_node
>>>>> - make_timer_node
>>>>> - handle_device_interrupts
>>>>> - construct_domain
>>>>> - process_shm
>>>>> 
>>>>> The functions allocate_static_memory and assign_static_memory_11
>>>>> are now externally visible, so put their declarations into
>>>>> domain_build.h and move the #else and stub definition in the header
>>>>> as well.
>>>>> 
>>>>> Move is_dom0less_mode from setup.c to dom0less-build.c and make it
>>>>> externally visible.
>>>>> 
>>>>> Where spotted, fix code style issues.
>>>>> 
>>>>> No functional change is intended.
>>>>> 
>>>>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>>> 
>>>> This is great! A couple of questions.
>>>> 
>>>> Why was allocate_static_memory not moved to dom0less-build.c ?
>>> 
>>> My aim is to decouple the features, so in patch 4 we move (just once as 
>>> Julien suggested)
>>> the static memory code on a module on its own, because we can have a guest 
>>> booted with
>>> dom0less feature but having it with static memory is optional.
>> OK
>>>> Would it make sense to also move construct_dom0 to dom0less-build.c
>>>> given the similarities with construct_domU? I am not sure about this.
>>>> 
>>> 
>>> We can’t do that because the final goal of this serie is to have a Kconfig 
>>> disabling dom0less,
>>> so in that case we will end up removing from the compilation also 
>>> construct_dom0.
>> OK. Probably we can't do much better than this.
>> One more question on the code movement, and I would also like Julien and
>> Bertrand to express their opinions on this.
>> Given that code movement is painful from a git history perspective, and
>> given that we have to move dom0less code to xen/common anyway to make
>> it available to RISC-V and also x86, could we do it in one shot here?
> 
> Looking at the name of the functions, I would expect that we would need 
> another code movement in the future to move back Arm specific function under 
> arch/arm/. So we would end up with two code movement as well.
> 
> I would prefer if we wait until RISC-V/x86 needs it so we don't unnecessarily 
> move Arm specific code in common/.

I agree with Julien here.
Moving the code now will mean moving part of it back in arm in the future once 
we have a second user of this.
I would rather wait for the need to come so that we do this cleanly.

Also using hyperlaunch name now would be weird as there was no agreement on the 
naming (as far as I know) so far.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall