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

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

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