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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 3 Oct 2023, Michal Orzel wrote:
> On 03/10/2023 09:44, Luca Fancellu wrote:
> >>>>> 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?
> > 
> > So here is why I felt painful doing now a move to the common code, but 
> > maybe you (maintainers) can give me some
> > feedbacks.
> > 
> > I see that the functions that might be put in common are these, some of 
> > them however have arm specific code in them:
> > 
> > is_dom0less_mode
> > allocate_bank_memory
> > allocate_memory
> > handle_passthrough_prop
> > handle_prop_pfdt
> > scan_pfdt_node
> > check_partial_fdt
> > domain_p2m_pages
> > alloc_xenstore_evtchn
> > domain_handle_dtb_bootmodule (contains reference to the gic)
> > prepare_dtb_domU (have reference to psci, gic, vpl011)
> > construct_domU (have reference to vpl011, static shared memory)
> > create_domUs(have reference to vpl011, sve)
> > 
> > Here the functions that can stay in arm code:
> > 
> > make_gicv2_domU_node
> > make_gicv3_domU_node
> > make_gic_domU_node
> > make_vpl011_uart_node
> > 
> > 
> > Given that the status after the move to common of the above functions is 
> > not very clean, I’ve decided to don’t do that,
> > however if you are fine with it, I can do the modification and who is going 
> > to work further on the subject can consolidate
> > and make them build for other architecture.
> > 
> Another option would be to hold off for a while until work on 
> hyperlaunch/RISCV dom0less starts to better understand the needs,
> concepts and to avoid multiple code movement which results in a horrible 
> history. I know this is not nice but I can tell you that
> I had to stop working on some features like FLASK with dom0less, static 
> domids for dom0less domUs, because according to the hyperlaunch design,
> this will need to be common. With hyperlaunch, everything starts with the 
> domain configuration that is supposed to be arch neutral, so
> until this is done, it's difficult to do anything in this area.

This is not good. In an ideal world, Hyperlaunch shouldn't block
progress for dom0less. We shouldn't have to wait many months to make
progress on FLASK with dom0less, static domids for dom0less domUs, etc.
because of potential Hyperlaunch implications.

In my option a delay of few weeks might be OK; we should be reasonable.
But a delay of few months is not. Cosidering review times, release
schedules etc. it could become a very significant delay.

Also, hyperlaunch contributors are not familiar with dom0less and are
not familiar with arm. (This is so true that they have their own
reimplementation of the parser.) I think the dom0less separation / code
movement is better done by us in the arm community because we know the
code far better.

So I think Luca's suggestion above is in the right direction. I would
follow Luca's suggestion with only one difference: I would keep
prepare_dtb_domU in the arm code, together with make_gicv*_domU_node and
make_vpl011_uart_node. I would move the rest to common.