Re: [PATCH v3] arinc653: don't assume Dom0 is the control domain

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 4/1/26 09:03, Jan Beulich wrote:
> On 01.04.2026 14:57, Jürgen Groß wrote:
>> On 01.04.26 14:29, Jan Beulich wrote:
>>> Leaving aside highly disaggregated environments, the control domain is
>>> what will invoke XEN_SYSCTL_SCHEDOP_putinfo. Its vCPU-s therefore need to
>>> be able to run unconditionally, not those of the domain with ID 0 (which
>>> may not exist at all).
>>>
>>> Fixes: 9f0c658baedc ("arinc: add cpu-pool support to scheduler")
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> There being no "else" to the if(), what about other control domain vCPU-s?
>>
>> I guess this is a stale leftover. Doesn't matter for committing anyway.
>>
>>> ---
>>> v3: Don't mistakenly include the idle domain.
>>> v2: New.
>>>
>>> --- a/xen/common/sched/arinc653.c
>>> +++ b/xen/common/sched/arinc653.c
>>> @@ -411,10 +411,10 @@ a653sched_alloc_udata(const struct sched
>>>       spin_lock_irqsave(&sched_priv->lock, flags);
>>>   
>>>       /*
>>> -     * Add every one of dom0's units to the schedule, as long as there are
>>> -     * slots available.
>>> +     * Add every one of the control domain's units to the schedule, as 
>>> long as
>>> +     * there are slots available.
>>>        */
>>> -    if ( unit->domain->domain_id == 0 )
>>> +    if ( is_control_domain(unit->domain) && !is_idle_domain(unit->domain) )
>>>       {
>>>           entry = sched_priv->num_schedule_entries;
>>>   
>>
>> Hmm, is it really the control domain only which wants to be scheduled 
>> initially?
>> I would think that at least the hardware domain and probably a Xenstore 
>> domain
>> would want to be included, too.
>>
>> In the end it might even be that other domains created via dom0less would 
>> want
>> to be able to run initially. They could be part of a mandatory 
>> infrastructure.
>> Why would they need to be created at boot if they are NOT important?
> 
> This part is easy to answer: Because in a dom0less setup you simply may have
> no toolstack at all. (At which point there may also be nothing to set a
> schedule, yes.)

This is a known limitation. In a dom0less/hyperlaunch scenario, as future work,
I would like to see the ability to configure the ARINC653 schedule in device
tree, which would likely extend the existing boot time cpu pool work.

>> The question is whether the arinc653 scheduler is really meant for such 
>> setups.
>> OTOH just modifying the test to:
>>
>>      if ( system_state < SYS_STATE_active &&
>>           unit->domain->domain_id < DOMID_FIRST_RESERVED )
>>
>> seems to be fine for catching all those cases.
>>
>> With or without this modification:
>>
>> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
> 
> Thanks, yet I'll have to leave to the maintainers to decide which form it
> should ultimately take. One remark: A restartable control domain wouldn't
> pass that conditional. Granted that's looking far into the future.

It may not be desirable to schedule domUs until the control domain has had a say
in the matter, considering that the default schedule is unlikely to contain the
desired minor frame runtimes. It's less clear whether to include hardware and
xenstore domains in the default schedule, though I'm leaning toward only
including the domain with ability to invoke XEN_SYSCTL_SCHEDOP_putinfo for now
(i.e. the control domain).

Hm, the suggested 'system_state < SYS_STATE_active' check is possibly a good
addition. This reinforces that the default schedule's purpose is merely to get a
system booting until a user-provided schedule can be installed. Without this
check, restarting the control domain could result in new entries being added
while old entries remain, possibly ending up with duplicates and/or exhausting
the schedule. However, the restarted domain would need to retain its uuid if it
expects to be scheduled after restart.

Lastly, we may consider restricting the default schedule to Pool-0, and
eventually we may want a mechanism to disable the default schedule altogether
(e.g. when boot time cpupools are in use), but I don't think it's necessary to
conflate those with the current patch.