Re: [PATCH 1/3] xen/domain: Reorder trivial initialisation in early domain_create()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.12.2020 19:14, Andrew Cooper wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -391,25 +391,7 @@ struct domain *domain_create(domid_t domid,
>  
>      TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>  
> -    /*
> -     * Allocate d->vcpu[] and set ->max_vcpus up early.  Various per-domain
> -     * resources want to be sized based on max_vcpus.
> -     */
> -    if ( !is_system_domain(d) )
> -    {
> -        err = -ENOMEM;
> -        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
> -        if ( !d->vcpu )
> -            goto fail;
> -
> -        d->max_vcpus = config->max_vcpus;
> -    }
> -
> -    lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);

Wouldn't this also count as "trivial initialization", and hence while
moving want to at least be placed ...

> -    if ( (err = xsm_alloc_security_domain(d)) != 0 )
> -        goto fail;
> -
> +    /* Trivial initialisation. */
>      atomic_set(&d->refcnt, 1);
>      RCU_READ_LOCK_INIT(&d->rcu_lock);
>      spin_lock_init_prof(d, domain_lock);
> @@ -434,6 +416,27 @@ struct domain *domain_create(domid_t domid,
>      INIT_LIST_HEAD(&d->pdev_list);
>  #endif
>  
> +    /* All error paths can depend on the above setup. */

... ahead of this comment?

> +    /*
> +     * Allocate d->vcpu[] and set ->max_vcpus up early.  Various per-domain
> +     * resources want to be sized based on max_vcpus.
> +     */
> +    if ( !is_system_domain(d) )
> +    {
> +        err = -ENOMEM;
> +        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
> +        if ( !d->vcpu )
> +            goto fail;
> +
> +        d->max_vcpus = config->max_vcpus;
> +    }
> +
> +    lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
> +
> +    if ( (err = xsm_alloc_security_domain(d)) != 0 )
> +        goto fail;
> +
>      err = -ENOMEM;
>      if ( !zalloc_cpumask_var(&d->dirty_cpumask) )
>          goto fail;
> 

Just as an observation (i.e. unlikely for this patch) I doubt
system domains need ->dirty_cpumask set up, and hence this
allocation may also want moving down a few lines.

Jan