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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 21 Mar 2022, at 17:36, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 21/03/2022 15:58, Luca Fancellu wrote:
>>> On 18 Mar 2022, at 16:12, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> Hi Luca,
>>> 
>>> I only skimmed through the series. I have one question below:
>>> 
>>> On 18/03/2022 15:25, Luca Fancellu wrote:
>>>> +void __init btcpupools_allocate_pools(void)
>>>> +{
>>>> +    unsigned int i;
>>>> +    bool add_extra_cpupool = false;
>>>> +
>>>> +    /*
>>>> +     * If there are no cpupools, the value of next_pool_id is zero, so 
>>>> the code
>>>> +     * below will assign every cpu to cpupool0 as the default behavior.
>>>> +     * When there are cpupools, the code below is assigning all the not
>>>> +     * assigned cpu to a new pool (next_pool_id value is the last id + 1).
>>>> +     * In the same loop we check if there is any assigned cpu that is not
>>>> +     * online.
>>>> +     */
>>>> +    for ( i = 0; i < nr_cpu_ids; i++ )
>>>> +        if ( cpumask_test_cpu(i, &cpu_online_map) )
>>>> +        {
>>>> +            /* Unassigned cpu gets next_pool_id pool id value */
>>>> +            if ( pool_cpu_map[i] < 0 )
>>>> +            {
>>>> +                pool_cpu_map[i] = next_pool_id;
>>>> +                add_extra_cpupool = true;
>>>> +            }
>>>> +            printk(XENLOG_INFO "Logical CPU %u in Pool-%u.\n", i,
>>>> +                   pool_cpu_map[i]);
>>>> +        }
>>>> +        else
>>>> +        {
>>>> +            if ( pool_cpu_map[i] >= 0 )
>>>> +                panic("Pool-%d contains cpu%u that is not online!\n",
>>>> +                      pool_cpu_map[i], i);
>>>> +        }
>>>> +
>>>> +    if ( add_extra_cpupool )
>>>> +        next_pool_id++;
>>>> +
>>>> +    /* Create cpupools with selected schedulers */
>>>> +    for ( i = 0; i < next_pool_id; i++ )
>>>> +        cpupool_create_pool(i, pool_sched_map[i]);
>>>> +
>>>> +#ifdef CONFIG_X86
>>>> +    /* Cpu0 must be in cpupool0 for x86 */
>>>> +    if ( pool_cpu_map[0] != 0 )
>>>> +        panic("Cpu0 must be in Pool-0\n");
>>>> +#endif
>>> 
>>> Can you document why this is necessary on x86 but not on other 
>>> architectures?
>> Hi Julien,
>> I received the warning by Juergen here: 
>> https://patchwork.kernel.org/comment/24740762/ that at least on x86 there 
>> could be
>> some problems if cpu0 is not in cpupool0, I tested it on arm and it was 
>> working fine and I didn’t find any restriction.
> 
> What exactly did you test on Arm?
> 
>> So I don’t know why on x86 we must have cpu0 in cpupool0, maybe x86 
>> maintainer have more knowledge about that and
>> I can put a comment here.
> 
> On Arm, we are not yet supporting all the CPU features that x86 supports 
> (e.g. CPU hotplug, suspend/resume...). So I a am bit concerned that the 
> restriction is just not there yet (or possibly hidden).
> 
> Therefore, before lifting the restriction on Arm (and other arch), I would 
> like us to understand why it is necessary on x86.
> 
> We may not have the answer quickly, so is it going to be a problem to keep 
> the restriction on Arm?

I am ok to keep the limitation to have dom0 always running on cpu0.
Only limitation I can see is that on a big little system, dom0 needs to stay on 
the type of core of the first booted core.
But as the user could modify his firmware to boot on the type of core he wants, 
this limitation can usually be worked around.
So it is not a problem.
Maybe we could make that an unsupported feature that one could activate through 
the configuration ?

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall