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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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