Re: [PATCH v6 4/6] xen/cpupool: Create different cpupools at boot time

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 11 Apr 2022, at 11:58, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 08/04/2022 09:45, Luca Fancellu wrote:
>> diff --git a/docs/misc/arm/device-tree/cpupools.txt 
>> b/docs/misc/arm/device-tree/cpupools.txt
>> new file mode 100644
>> index 000000000000..40cc8135c66f
>> --- /dev/null
>> +++ b/docs/misc/arm/device-tree/cpupools.txt
>> @@ -0,0 +1,140 @@
>> +Boot time cpupools
>> +==================
>> +
>> +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible 
>> to
>> +create cpupools during boot phase by specifying them in the device tree.
>> +ACPI is not supported for this feature.
>> +
>> +Cpupools specification nodes shall be direct childs of /chosen node.
>> +Each cpupool node contains the following properties:
>> +
>> +- compatible (mandatory)
>> +
>> +    Must always include the compatiblity string: "xen,cpupool".
>> +
>> +- cpupool-cpus (mandatory)
>> +
>> +    Must be a list of device tree phandle to nodes describing cpus (e.g. 
>> having
>> +    device_type = "cpu"), it can't be empty.
>> +
>> +- cpupool-sched (optional)
>> +
>> +    Must be a string having the name of a Xen scheduler. Check the 
>> sched=<...>
>> +    boot argument for allowed values [1]. When this property is omitted, 
>> the Xen
>> +    default scheduler will be used.
>> +
>> +
>> +Constraints
>> +===========
>> +
>> +If no cpupools are specified, all cpus will be assigned to one cpupool
>> +implicitly created (Pool-0).
>> +
>> +If cpupools node are specified, but not every cpu brought up by Xen is 
>> assigned,
>> +all the not assigned cpu will be assigned to an additional cpupool.
>> +
>> +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen 
>> will
>> +stop.
>> +
>> +The boot cpu must be assigned to Pool-0, so the cpupool containing that core
>> +will become Pool-0 automatically.
>> +
>> +
>> +Examples
>> +========
>> +
>> +A system having two types of core, the following device tree specification 
>> will
>> +instruct Xen to have two cpupools:
>> +
>> +- The cpupool with id 0 will have 4 cpus assigned.
>> +- The cpupool with id 1 will have 2 cpus assigned.
> 
> AFAIK, there are no guarantee that Xen will parse cpupool_a first. So it 
> would be possible that the ID are inverted here.
> 
> This could happen if you want to keep the boot CPU in pool 0 and it is not 
> cpu@0 (some bootloaders allows you to change the boot CPU).

Hi Julien,

Yes I will specify that the boot cpu is listed in cpupool_a, so that cpupool 
will have id 0 regardless of the parsing order.

> 
> Also, here you write "The cpupool with id X" but ...
> 
>> +A system having the cpupools specification below will instruct Xen to have 
>> three
>> +cpupools:
>> +
>> +- The cpupool Pool-0 will have 2 cpus assigned.
>> +- The cpupool Pool-1 will have 2 cpus assigned.
>> +- The cpupool Pool-2 will have 2 cpus assigned (created by Xen with all the 
>> not
>> +  assigned cpus a53_3 and a53_4).
> 
> here you write "The cpupool Pool-X". Can you be consistent?

Sure, do you have a preference between “The cpupool with id X” and “Pool-X”? 
Otherwise I would go for Pool-X everywhere.

> 
> On a separate topic, I think dom0_max_vcpus() needs to be updated to by 
> default (i.e when opt_dom0_max_vcpus == 0) the number of vCPUs match the 
> number of vCPUs in the cpupool (I think 0) used to created dom0.

Yes right, I didn’t think about that, I think the change could be something 
like that:

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9c67a483d4a4..9787104c3d31 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -73,7 +73,10 @@ custom_param("dom0_mem", parse_dom0_mem);
 unsigned int __init dom0_max_vcpus(void)
 {
     if ( opt_dom0_max_vcpus == 0 )
-        opt_dom0_max_vcpus = num_online_cpus();
+    {
+        ASSERT(cpupool0);
+        opt_dom0_max_vcpus = cpumask_weight(cpupool_valid_cpus(cpupool0));
+    }
     if ( opt_dom0_max_vcpus > MAX_VIRT_CPUS )
         opt_dom0_max_vcpus = MAX_VIRT_CPUS;

And if you agree I will include the changes for the v7.

Cheers,
Luca

> 
> Cheers,
> 
> -- 
> Julien Grall