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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Jan