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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 2 Oct 2023, Bertrand Marquis wrote:
> > On 2 Oct 2023, at 10:26, Julien Grall <julien@xxxxxxx> wrote:
> > 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.

RISC-V is already using dom0less code, however in a downstream
repository. To make progress faster the code was copied (not shared)
from arch/arm to arch/riscv. More details on the Xen community call this
week. 
https://gitlab.com/xen-project/people/olkur/xen/-/blob/riscv_aia_support/xen/arch/riscv/domain_build.c?ref_type=heads

Hyperlaunch also needs dom0less code to be made common to make progress:
https://marc.info/?l=xen-devel&m=169154172700539

So I think that there is an immediate RISC-V and X86 need.

But the point about "moving the code now will mean moving part of it
back in arm in the future" is valid. How do we move forward?

I don't think we want to block Luca's progress to wait for more
plumbings done on x86 or RISC-V. Also we don't want to scope creep
Luca's series too much.

But I think the goal should be to move dom0less code to xen/common as
soon as possible and make it arch neutral. How do we get there?