Re: [PATCH] domain: skip more stuff for idle's vCPU-s in vcpu_create()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Nov 26, 2025 at 11:31:46AM +0100, Jan Beulich wrote:
> Nothing hypercall-related needs setting up there. Nor do we need to
> check whether the idle domain is shutting down - it never will.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> For vmtrace_alloc_buffer() adding the conditional may be questionable: The
> function checks d->vmtrace_size first thing, bailing immediately when it's
> zero (which it always will be for the idle domain).

Oh, I notice this now (after having written the comment below).  I
would then add a comment rather than the extra check.  And possibly an
ASSERT(!is_idle_domain(d)); inside of `vmtrace_alloc_buffer()` after
the vmtrace_size check.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -427,12 +427,6 @@ struct vcpu *vcpu_create(struct domain *
>      v->vcpu_id = vcpu_id;
>      v->dirty_cpu = VCPU_CPU_CLEAN;
>  
> -    rwlock_init(&v->virq_lock);
> -
> -    tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
> -
> -    grant_table_init_vcpu(v);
> -
>      if ( is_idle_domain(d) )
>      {
>          v->runstate.state = RUNSTATE_running;
> @@ -440,6 +434,12 @@ struct vcpu *vcpu_create(struct domain *
>      }
>      else
>      {
> +        rwlock_init(&v->virq_lock);
> +
> +        tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
> +
> +        grant_table_init_vcpu(v);
> +
>          v->runstate.state = RUNSTATE_offline;
>          v->runstate.state_entry_time = NOW();
>          set_bit(_VPF_down, &v->pause_flags);
> @@ -450,7 +450,7 @@ struct vcpu *vcpu_create(struct domain *
>      if ( sched_init_vcpu(v) != 0 )
>          goto fail_wq;
>  
> -    if ( vmtrace_alloc_buffer(v) != 0 )
> +    if ( !is_idle_domain(d) && vmtrace_alloc_buffer(v) != 0 )
>          goto fail_wq;

There's an existing issue here, the usage of fail_rq is wrong here.
It should use fail_sched instead.  However I wonder whether we want to
move the `vmtrace_alloc_buffer()` call inside the existing `else {`
branch of the is_idle_domain() condition, as to avoid this extra
is_idle_domain() check here?

Thanks, Roger.