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
