Re: [PATCH 2/2] x86: generalise vcpu0 creation for a domain

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Tue Jul 22, 2025 at 4:45 PM CEST, Jan Beulich wrote:
> On 17.07.2025 19:51, Alejandro Vallejo wrote:
>> Make alloc_dom0_vcpu0() viable as a general vcpu0 allocator. Keep
>> behaviour on any hwdom/ctldom identical to that dom0 used to have, and
>> make non-dom0 have auto node affinity.
>> 
>> Rename the function to alloc_dom_vcpu0() to reflect this change in
>> scope, and move the prototype to asm/domain.h from xen/domain.h as it's
>> only used in x86.
>
> Which makes we wonder what's really x86-specific about it. Yes, the use of
> ...

Mostly that it's the only arch doing it.

>
>> --- a/xen/arch/x86/dom0_build.c
>> +++ b/xen/arch/x86/dom0_build.c
>> @@ -254,12 +254,16 @@ unsigned int __init dom0_max_vcpus(void)
>>      return max_vcpus;
>>  }
>>  
>> -struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
>> +struct vcpu *__init alloc_dom_vcpu0(struct domain *d)
>>  {
>> -    dom0->node_affinity = dom0_nodes;
>> -    dom0->auto_node_affinity = !dom0_nr_pxms;
>> +    d->auto_node_affinity = true;
>> +    if ( is_hardware_domain(d) || is_control_domain(d) )
>> +    {
>> +        d->node_affinity = dom0_nodes;
>> +        d->auto_node_affinity = !dom0_nr_pxms;
>
> ... dom0_nr_pxms here perhaps is. Yet that could be sorted e.g. by making
> this a function parameter.

ARM doesn't dabble with affinities while allocating the first vcpu. It's a
straight vcpu_create(). We could definitely inline setting that affinity
setting and forego the function altogether. I'm not a big fan of functions
with non-obvious side-effects, and this is one such case.

>
>> --- a/xen/arch/x86/include/asm/dom0_build.h
>> +++ b/xen/arch/x86/include/asm/dom0_build.h
>> @@ -23,6 +23,11 @@ unsigned long dom0_paging_pages(const struct domain *d,
>>  void dom0_update_physmap(bool compat, unsigned long pfn,
>>                           unsigned long mfn, unsigned long vphysmap_s);
>>  
>> +/* general domain construction */
>
> Nit: Comment style.

Ack

>
>> @@ -1054,9 +1055,11 @@ static struct domain *__init create_dom0(struct 
>> boot_info *bi)
>>      if ( IS_ERR(d) )
>>          panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>>  
>> +    bd->d = d;
>> +
>>      init_dom0_cpuid_policy(d);
>>  
>> -    if ( alloc_dom0_vcpu0(d) == NULL )
>> +    if ( alloc_dom_vcpu0(d) == NULL )
>>          panic("Error creating %pdv0\n", d);
>>  
>>      cmdline_size = domain_cmdline_size(bi, bd);
>> @@ -1093,7 +1096,6 @@ static struct domain *__init create_dom0(struct 
>> boot_info *bi)
>>          bd->cmdline = cmdline;
>>      }
>>  
>> -    bd->d = d;
>>      if ( construct_dom0(bd) != 0 )
>>          panic("Could not construct domain 0\n");
>
> Isn't this movement of the bd->d assignment entirely unrelated?
>
> Jan

The prior incarnation of this patch (see Daniel's RFC) took a boot_domain, I
think, for which the change would be required. It indeed doesn't seem to be
required any longer.

Cheers,
Alejandro