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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 4 Oct 2023, Julien Grall wrote:
> On 03/10/2023 21:40, Stefano Stabellini wrote:
> > On Tue, 3 Oct 2023, Julien Grall wrote:
> > > On 03/10/2023 20:52, Stefano Stabellini wrote:
> > > > On Tue, 3 Oct 2023, Michal Orzel wrote:
> > > > > On 03/10/2023 09:44, Luca Fancellu wrote:
> > > > > > 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.
> > > 
> > > It depends what are the implications. If it means that the bindings will
> > > change a release after. Then I think we should instead work on
> > > standardizing
> > > Hyperlaunch (or whichever name we decide to use) earlier so our users can
> > > rely
> > > on the interface for multiple revisions.
> > > 
> > > We could of course decide to maintain both interfaces. But this means more
> > > maintenance work which could have been avoided by fast tracking
> > > Hyperlaunch
> > > (it is not like we don't know it is coming...).
> > > 
> > > > 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.
> > > 
> > > I think we need both the arm and hyperlaunch community to work together
> > > (see
> > > more below).
> > > 
> > > > 
> > > > 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.
> > > 
> > > Luca's pointed out that some function (such as construct_domU) would
> > > contain
> > > reference to Arm specific code. So with your proposal, I am under the
> > > impression that we would move code that would then end up to be moved
> > > again in
> > > a few months time. So it is defeating your goal (even though the movement
> > > will
> > > hopefully be smaller).
> > 
> > I was assuming that e.g. construct_domU would reference ARM code, but
> > the ARM code would live under arch/arm. So yes construct_domU would need
> > some refactoring in the future to make the call generic instead of
> > arm-specific, but wouldn't require significant additional code movement.
> 
> This is not really great. We could end up to have an extension of Arm in
> common for multiple release (the timeline for hyperlaunch is known). If you
> want to move the code now, then it would be preferable that we abstract the
> calls (e.g. arch_construct_domU()) at least have a sensible interface.
> 
> >  >> As I wrote above, I don't think the Arm community alone is in the 
> position to
> > > decide what should be in common and what should be in arch specific. We
> > > need
> > > the hyperlaunch community to agree on their approach so we can know which
> > > split makes sense. This is similar to the on-going MMU split to cater the
> > > MPU.
> > > We looked the MPU code to decide of the best split.
> > > 
> > > A potential approach would be to look at the RISC-V implementation of
> > > dom0less
> > > and see the common parts. But then we are going in the the scope creep you
> > > mention earlier.
> > 
> > I didn't do a proper investigation and I didn't look at the RISC-V or
> > hyperlaunch code in details. From my discussions with both groups, here
> > is my understanding of arm-specific things we currently have:
> > 
> > - 1:1 memory mapping / static memory / static heap because they are
> >    currently unimplemented on other arches
> 
> But they might. So this is the sort of feature that would want to be
> implemented in common.
> 
> > - most domU device tree building because most of virtual devices are
> >    different (timer, interrupt controller, uart, etc)
> 
> Make sense for this one
> 
> > - the equivalent for dom0: the gic/timer device tree generated nodes
> 
> and this one.
> 
> > - the blacklist at the beginning of handle_node
> 
> Hmmm... I would expect this to be useful at least for RISC-V as surely they
> want to hide some device. Maybe this could be abstract.
> 
> > - for hyperlaunch, we need to support "module-index" as a way to get the
> >    physical address of a module
> > - reserved_memory doesn't exist on x86
> 
> Are you referring to the variable? If so, the concept of reserved region would
> likely exists for other architecture. So this is a feature that would need to
> be abstracted so it can be re-used.

Yes, you are right. It doesn't seem that there is an easy way to
abstract these things.


> > - a couple of dom0less device tree properties are arm-specific, such as
> >    "sve" and "vpl011"
> > 
> > This is me hand-waiving, so it might not be useful.
> > 
> > 
> > > So overall, I feel that Lucas' approach is better until Hyperlaunch gain
> > > momentum.
> > 
> > I am OK with Luca's original approach. I just wanted to open the
> > discussion in case there is a better way.
> 
> I don't particular mind moving the code to common. But it needs to be done
> correctly (i.e. not common/<foo>.c be a simple extension of Arm). We also need
> to agree on a name for the file/directory (and most likely of the feature) to
> avoid another renaming.

I think the best way forward is to keep Luca's series as is because
anything else would require a significant amount of work. There is no
easy way to properly abstract the things listed above (1:1 memory
mappings, handle_node blacklist etc.) Still extracting dom0less out of
domain_build.c is a very good step in the right direction and I think we
should take it.