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

[dmkhn@xxxxxxxxx]

On Tue, Jun 10, 2025 at 10:44:39AM +0100, Julien Grall wrote:
> Hi Denis,
> 
> On 10/06/2025 10:04, dmkhn@xxxxxxxxx wrote:
> > On Tue, Jun 10, 2025 at 10:26:22AM +0200, Jan Beulich wrote:
> >> On 10.06.2025 10:02, dmkhn@xxxxxxxxx wrote:
> >>> 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@xxxxxxxxx wrote:
> >>>>>>> From: Denis Mukhin <dmkhn@xxxxxxxxx>
> >>>>>>>
> >>>>>>> From: Denis Mukhin <dmukhin@xxxxxxxx>
> >>>>>>>
> >>>>>>> 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@xxxxxxxx>
> >>>>>>> ---
> >>>>>>> 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.
> >>
> >> No, no new command line option will address this. Original behavior needs 
> >> to be
> >> restored (either by correcting the earlier change or, as said, be 
> >> reverting).
> >
> > Correction of the earlier change:
> >
> >    
> > https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/b7f194650307a08a9e6da5aa9fdd1f8a9afd67eb
> >
> > re: command line option: I meant something like this:
> >
> >    
> > https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/51a519de6ea73ff3b650fd9bd4f3c5c63f64c069
> 
> I am with Jan here. This used to work before without "control_domid", so
> we ought to keep the same.

OK, thanks for the feedback!
I will post the correction of the earlier change.

> 
> But even if we are ok to break compatibility, I don't see the value of
> "control_domid". The implication of setting "hardware_domid" is you will
> have a separate control domain. At which point, why would it matter to
> specify the domain ID?

I thought there's a plan to use symbols in the ongoing hyperlauch reworks, but
as later Jason explains, this is not the case.

> 
> Cheers,
> 
> --
> Julien Grall
>