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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 4/7/26 02:18, Jan Beulich wrote:
> On 03.04.2026 10:47, Stewart Hildebrand wrote:
>> 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.
> 
> So what does all of this mean for the patch here? Should I switch to Jürgen's
> suggestion? Should I merely add the system_state check, but otherwise keep as
> is? Or should I not change anything?

I think the patch is good as is, but I just want to give it a few more days to
give Nate or others a chance to chime in. I'll follow up next week if there's no
further discussion.