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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 7 Apr 2022, at 09:58, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 05/04/2022 09:57, Luca Fancellu wrote:
>> Introduce a way to create different cpupools at boot time, this is
>> particularly useful on ARM big.LITTLE system where there might be the
>> need to have different cpupools for each type of core, but also
>> systems using NUMA can have different cpu pools for each node.
>> The feature on arm relies on a specification of the cpupools from the
>> device tree to build pools and assign cpus to them.
> 
> How will this work for ACPI? Note that I am not suggesting to add suport 
> right now. However, we probably want to clarify that this is not yet 
> supported.

Ok I will add a note saying ACPI is not supported in v6

> 
> [...]
> 
>> diff --git a/docs/misc/arm/device-tree/cpupools.txt 
>> b/docs/misc/arm/device-tree/cpupools.txt
>> new file mode 100644
>> index 000000000000..5dac2b1384e0
>> --- /dev/null
>> +++ b/docs/misc/arm/device-tree/cpupools.txt
>> @@ -0,0 +1,136 @@
>> +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.
> 
> How about ACPI?

Same as above

> 
>> +
>> +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.
> 
> I would clarify what would be the scheduler if cpupool-sched is not specified.
> 
> Also, I would give a pointer to xen-command-line.pandoc so it is easier to 
> know where 'sched' is described.

Good point, I think the info got missed through the series, I’ll add and put 
the link.

> 
> [...]
> 
>> +void __init btcpupools_dtb_parse(void)
>> +{
>> +    const struct dt_device_node *chosen, *node;
>> +
>> +    chosen = dt_find_node_by_path("/chosen");
>> +    if ( !chosen )
>> +        return;
> Aside when using ACPI, the chosen node should always be there. So I think we 
> should throw/print an error if chosen is not present.

When you say error, do you mean like a panic or just a printk XENLOG_ERR and 
return?

> 
> Also, I would check that we haven't booted using ACPI rather than relying on 
> dt_find_node_by_path("/chosen") to return NULL.

Ok I will add a check on acpi_disabled to return if it is false.

Cheers,
Luca

> 
> Cheers,
> 
> -- 
> Julien Grall