Re: [PATCH v1 14/18] x86: generalize vcpu for domain building

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2022 23:04, Daniel P. Smith wrote:
> Here, the vcpu initialization code for dom0 creation is generalized for use 
> for
> other domains.

Yet with "other domains" still only ones created during boot, aiui.
Imo such details want spelling out.

The title also is too generic / imprecise.

> --- a/xen/arch/x86/domain_builder.c
> +++ b/xen/arch/x86/domain_builder.c
> @@ -28,6 +28,18 @@ static unsigned int __init dom_max_vcpus(struct 
> boot_domain *bd)
>          return bd->ncpus;
>  }
>  
> +struct vcpu *__init alloc_dom_vcpu0(struct boot_domain *bd)

domain_alloc_vcpu0()?

> +{
> +    if ( bd->functions & BUILD_FUNCTION_INITIAL_DOM )
> +        return alloc_dom0_vcpu0(bd->domain);
> +
> +    bd->domain->node_affinity = node_online_map;
> +    bd->domain->auto_node_affinity = true;

I can spot neither consumers of nor code being replaced by this.

> +    return vcpu_create(bd->domain, 0);
> +}
> +
> +
>  void __init arch_create_dom(

No double blank lines please.

> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -14,6 +14,8 @@
>   */
>  
>  #ifndef COMPAT
> +#include <xen/bootdomain.h>
> +#include <xen/domain_builder.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/param.h>
> @@ -3399,13 +3401,13 @@ void wait(void)
>  }
>  
>  #ifdef CONFIG_X86
> -void __init sched_setup_dom0_vcpus(struct domain *d)
> +void __init sched_setup_dom_vcpus(struct boot_domain *bd)

Perhaps simply drop the original _dom0 infix?

>  {
>      unsigned int i;
>      struct sched_unit *unit;
>  
> -    for ( i = 1; i < d->max_vcpus; i++ )
> -        vcpu_create(d, i);
> +    for ( i = 1; i < bd->domain->max_vcpus; i++ )
> +        vcpu_create(bd->domain, i);

Seeing the further uses below, perhaps better introduce a local variable
"d", like you do elsewhere?

> @@ -3413,19 +3415,24 @@ void __init sched_setup_dom0_vcpus(struct domain *d)
>       * onlining them. This avoids pinning a vcpu to a not yet online cpu 
> here.
>       */
>      if ( pv_shim )
> -        sched_set_affinity(d->vcpu[0]->sched_unit,
> +        sched_set_affinity(bd->domain->vcpu[0]->sched_unit,
>                             cpumask_of(0), cpumask_of(0));
>      else
>      {
> -        for_each_sched_unit ( d, unit )
> +        for_each_sched_unit ( bd->domain, unit )
>          {
> -            if ( !opt_dom0_vcpus_pin && !dom0_affinity_relaxed )
> -                sched_set_affinity(unit, &dom0_cpus, NULL);
> -            sched_set_affinity(unit, NULL, &dom0_cpus);
> +            if ( builder_is_initdom(bd) )
> +            {
> +                if ( !opt_dom0_vcpus_pin && !dom0_affinity_relaxed )
> +                    sched_set_affinity(unit, &dom0_cpus, NULL);
> +                sched_set_affinity(unit, NULL, &dom0_cpus);
> +            }
> +            else
> +                sched_set_affinity(unit, NULL, cpupool_valid_cpus(cpupool0));

Hard-coded cpupool0?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -2,6 +2,7 @@
>  #ifndef __SCHED_H__
>  #define __SCHED_H__
>  
> +#include <xen/bootdomain.h>

Please don't - this header has already too many dependencies. All you really
need ...

> @@ -1003,7 +1004,7 @@ static inline bool sched_has_urgent_vcpu(void)
>  }
>  
>  void vcpu_set_periodic_timer(struct vcpu *v, s_time_t value);
> -void sched_setup_dom0_vcpus(struct domain *d);
> +void sched_setup_dom_vcpus(struct boot_domain *d);

... for this is a forward declaration of struct boot_domain.

Jan