Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.09.2026 10:07, Jürgen Groß wrote:
> On 01.09.26 09:22, Jan Beulich wrote:
>> On 31.08.2026 14:59, Jürgen Groß wrote:
>>> On 31.08.26 11:45, Andrew Cooper wrote:
>>>> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
>>>>> Every vcpu_create() call site that builds more than one vcpu loops
>>>>> over ids up to d->max_vcpus and stops on the first failure, but none
>>>>> of them roll max_vcpus back to match. This leaves d->vcpu[i] == NULL
>>>>> for ids below max_vcpus
>>>>
>>>> As I told you before, you must cope with this property in non-error
>>>> scenarios.
>>>>
>>>>> , which anything walking d->vcpu[] can then
>>>>> dereference. This is what caused the crash: sched_move_domain()
>>>>> walks every vcpu slot up to max_vcpus without checking for empty
>>>>> ones, so when a domain built in a non-default cpupool had vcpu
>>>>> creation fail partway through, domain_kill() later moving it back
>>>>> to the default cpupool handed one of its empty slots straight to
>>>>> the new cpupool's scheduler, causing a NULL-pointer dereference
>>>>> inside sched_alloc_udata().
>>>>>
>>>>> Add vcpus_create(d): creates every vcpu of d up to max_vcpus and
>>>>> rolls max_vcpus back to the failed id on error. This keeps
>>>>> d->vcpu[i] is non-NULL for all i < d->max_vcpus
>>>>
>>>> No, it really doesn't.
>>>>
>>>>> , instead of guarding
>>>>> every reader of d->vcpu[] agains holes individually.
>>>>>
>>>>> Convert every site that builds vcpus in a loop to call this function
>>>>> instead.
>>>>>
>>>>> Fixes: 61649709421a ("xen/domain: Allocate d->vcpu[] in domain_create()")
>>>>> Suggested-by: Juergen Gross <jgross@xxxxxxxx>
>>>>> Signed-off-by: Furkan Caliskan <frn1furkan10@xxxxxxxxx>
>>>>
>>>> For the avoidance of a long drawn-out argument, nack.  Under no
>>>> circumstances are you editing d->max_cpus after it's put into the domain
>>>> list.
>>>>
>>>> You've chosen to do so at a point where the domain object is live,
>>>> visible in the system and able to be the target of other hypercalls.
>>>>
>>>> Furthermore you have not fixed what your commit message claims.
>>>> d->vcpu[...] is still NULL for an arbitrary period of time, including
>>>> being able to be the target of hypercalls, before vCPUs are created.
>>>>
>>>> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
>>>> the object lifecycles must work, because creating vCPUs is not atomic
>>>> with respect to creating domains.
>>>
>>> Would you be fine with me creating a patch series moving vcpu creation into
>>> domain_create()?
>>
>> This was discussed before, and however nice it would be for the issue at 
>> hand,
>> it would get in the way of us wanting to have CPU policy for domains put in
>> place before vCPU-s are created, such that on x86 the XSAVE area can be sized
>> once and for all.
> 
> This could be done when unpausing the domain initially.

Imo unpausing shouldn't fail because of memory shortage.

> OTOH I don't see xstate_alloc_save_area() looking at the domain's cpu policy
> at all. Is this a plan for the future?

This is to better accommodate the AMX series (which has been pending for years),
and potentially also for architectural-LBR work (which has been posted once, but
was apparently abandoned).

> And additionally there is no guard for avoiding the vcpus being created before
> the policy is being set.

Addressing that is part of Andrew's plan, aiui.

Jan