Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[dmkhn@proton.me]

On Tue, Jun 10, 2025 at 08:53:12AM +0200, Jan Beulich wrote:
> On 06.06.2025 23:29, Julien Grall wrote:
> > Hi Denis,
> >
> > On 05/06/2025 23:05, Julien Grall wrote:
> >> Hi Denis,
> >>
> >> On 28/05/2025 23:50, dmkhn@proton.me wrote:
> >>> From: Denis Mukhin <dmkhn@proton.me>
> >>>
> >>> From: Denis Mukhin <dmukhin@ford.com>
> >>>
> >>> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
> >>> with a call to get_initial_domain_id() (returns the value of hardware_domid on
> >>> Arm).
> >>
> >> I am not entirely why this is done. Are you intending to pass a different domain ID? If so...
> >>
> >>>
> >>> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
> >>> ID is skipped during domain ID allocation to cover domU case in dom0less
> >>> configuration. That also fixes a potential issue with re-using ID#0 for domUs
> >>> when get_initial_domain_id() returns non-zero.
> >>>
> >>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> >>> ---
> >>> Changes since v8:
> >>> - rebased
> >>> ---
> >>>   xen/arch/arm/domain_build.c             | 4 ++--
> >>>   xen/common/device-tree/dom0less-build.c | 9 +++------
> >>>   xen/common/domain.c                     | 4 ++--
> >>>   3 files changed, 7 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>> index e9d563c269..0ad80b020a 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
> >>
> >> ... naming like create_dom0() probably wants to be renamed.
> >>
> >> That said, I am not convinced a domain other than 0 should have full privilege by default. So I would argue it should stay as ...
> >>
> >>>       if ( !llc_coloring_enabled )
> >>>           flags |= CDF_directmap;
> >>> -    domid = domid_alloc(0);
> >>> +    domid = domid_alloc(get_initial_domain_id());
> >>
> >> ... 0.
> >
> > Looking at the implementation of get_initial_domain_id(), I noticed the behavior was changed for x86 by [1].
> >
> > Before, get_initial_domain_id() was returning 0 except for the PV shim.
> > But now, it would could return the domain ID specified on the command line (via hardware_dom).
> >
> > From my understanding, the goal of the command line was to create the hardware domain *after* boot. So initially we create dom0 and then initialize the hardware domain. With the patch below, this has changed.
> >
> > However, from the commit message, I don't understand why. It seems like we broke late hwdom?
> >
> > For instance, late_hwdom_init() has the following assert:
> >
> >     dom0 = rcu_lock_domain_by_id(0);
> >     ASSERT(dom0 != NULL);
> >
> > Jan, I saw you were involved in the review of the series. Any idea why this was changed?
> 
> I simply overlooked this aspect when looking at the change. You're right, things
> were broken there. Unless a simple and clean fix can be made relatively soon, I
> think this simply needs reverting (which may mean to revert any later commits
> that depend on that). I can't help noting that in this console rework there were
> way too many issues, and I fear more than just this one may have slipped
> through. I therefore wonder whether taken as a whole this was/is worth both the
> submitter's and all the reviewers' time.

Yes, sorry, I overlooked late_hwdom_init() modification.

IMO, the clean fix would be adding another command line parameter
`control_domid` (with default value 0), make get_initial_domain_id() return it
instead of current `hardware_domid` and update late_hwdom_init() to use
`control_domid` insted of open-coded 0.

That should align with the effort of splitting priveleged dom0 into control and
hardware domains: control domain will be the first domain ID allocated,
followed by the hardware domain.

> 
> Jan
> 
> > [1] https://lore.kernel.org/all/20250306075819.154361-1-dmkhn@proton.me/
> >
>