Re: [for-4.22][PATCH] xen/arm: Fail domain construction if a secondary vCPU cannot be created

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 08-Jul-26 13:27, Halder, Ayan Kumar wrote:
> 
> On 08/07/2026 12:20, Orzel, Michal wrote:
>>
>> On 08-Jul-26 11:06, Halder, Ayan Kumar wrote:
>>> Hi MIchal,
>>>
>>> Nice catch. Few questions.
>>>
>>> On 08/07/2026 08:49, Michal Orzel wrote:
>>>> construct_domain() creates the secondary vCPUs in a loop, but on a
>>>> vcpu_create() failure it only prints a message and breaks out of the
>>>> loop returning success. As a result the domain can be constructed
>>>> with fewer vCPUs than d->max_vcpus, leaving NULL holes in d->vcpu[]
>>>> below max_vcpus.
>>>>
>>>> When the guest probes the redistributor of a vCPU that was never created,
>>> Shouldn't the guest check how many vCPUs were created and probe the ones
>>> that were created ?
>> See below about DTB.
>>
>>>> get_vcpu_from_rdist() only checks vcpu_id against d->max_vcpus and then
>>>> dereferences the NULL d->vcpu[vcpu_id], resulting in a data abort.
>>>>
>>>> Return an error instead of breaking out of the loop. Both callers
>>>> (construct_domU() and construct_hwdom()) already propagate a negative
>>>> return value and fail domain construction, which is the correct
>>>> behaviour: a domain that cannot provide the requested number of vCPUs
>>>> should not be brought up.
>>> I see your reasoning.
>>>
>>> Alternatively it can be a design choice. Xen does not commit to create
>>> the max_vcpus that was requested.
>> Everything can be a design choice but this one wouldn't be wise, would it?
>> All in all, we (Arm maintainers) aim at following the contract to fail as 
>> soon
>> as possible if the user request cannot be satisfied.
> Can we remove the redundant DTB node so that the contract is maintained 
> ? IMHO , stopping a domain creation seems severe if the other vCPUs were 
> created.
No, we can't. First of all, the contract is to fail on unsatisifed user
requests. User requested e.g. 5 vCPUS, we created only 2 - we should bail out.
Please see all the dom0/dom0less code on Arm. Next, DTB generation and vCPU
creation happens *before* starting domains, so this is definitely not something
severe.

~Michal

>>
>>> If Xen is unable to create any vCPU, it should abort domain creation.
>>>
>>> If Xen creates lesser number of vCPUs than requested by max_vcpus, it
>>> can just print a warning and carry on.
>> No. Xen creates domain DTB before creating vCPUs, so Xen advertises something
>> that is not true. Here, Xen would create a DTB with e.g. 2 vCPUs while only 1
>> was created.
> 
> I agree that this is a problem.
> 
> - Ayan
> 
>>
>> ~Michal
>>
>>> In that case it should be the guest's responsibility to check the number
>>> of CPUs that it has.
>>>
>>> - Ayan
>>>
>>>> Fixes: 6b0e8e43348a ("xen/arm: allocate secondaries dom0 vcpus")
>>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>>> ---
>>>>    xen/arch/arm/domain_build.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index 550617f152bb..b46574fd32aa 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -1847,7 +1847,7 @@ int __init construct_domain(struct domain *d, struct 
>>>> kernel_info *kinfo)
>>>>            if ( vcpu_create(d, i) == NULL )
>>>>            {
>>>>                printk("Failed to allocate d%dv%d\n", d->domain_id, i);
>>>> -            break;
>>>> +            return -EINVAL;
>>>>            }
>>>>    
>>>>            if ( is_64bit_domain(d) )