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

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 11/04/2022 12:30, Luca Fancellu wrote:
> > 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).
> 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.
This only covers the case where are two cpupools.

AFAIK, there are no guarantee that Xen will parse the DT or the compiler 
will generate the DT the way you want. So for three cpupools, we still 
don't know which pool will be ID 1/2.
See more below.

> > 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.
Using "cpupool with ID 0" is definitely wrong. Pool-X is marginally 
better because an admin may think that this name will match what we have 
in Xen.
So I think it would be better to use the node name and mention that 
there are no guarantee in which ID will used by Xen.
> > 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.
This should work.

Cheers,

--
Julien Grall