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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 22 Mar 2022, at 14:01, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 22/03/2022 09:52, Luca Fancellu wrote:
>>>>> 
>>>>> 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?
>>> 
>> I have tested start/stop of some guest, moving cpus between cpupools, 
>> create/destroy cpupools, shutdown of Dom0
>> [ from your last mail ]
>>>>> 
>>>>> If dom0 must run on core0 and core0 is Little then you cannot build a 
>>>>> system where dom0 is running on big cores.
>>>>> If the limitation is not there, you can build such a configuration 
>>>>> without any dependency to the boot core type.
>>>> This might not be completely clear so let me rephrase:
>>>> In the current system:
>>>> - dom0 must run on cpupool-0
>>> 
>>> I don't think we need this restriction. In fact, with this series it will 
>>> become more a problem because the cpupool ID will based on how we parse the 
>>> Device-Tree.
>>> 
>>> So for dom0, we need to specify explicitely the cpupool to be used.
>>> 
>>>> - cpupool-0 must contain the boot core
>>>> - consequence: dom0 must run on the boot core
>>>> If boot core is little, you cannot build as system where dom0 runs only on 
>>>> the big cores.
>>>> Removing the second limitation (which is not required on arm) is making it 
>>>> possible.
>>> 
>>> IMHO removing the second restriction is a lot more risky than removing the 
>>> first one.
>> I see your point, my concern about moving Dom0 on another cpupool, different 
>> from cpupool0, is that we give the
>> opportunity to destroy the cpupool0 (we can’t let that happen), or remove 
>> every cpu from cpupool0.
> 
> From my understanding a cpupool can only be destroyed when there are no more 
> CPUs in the pool. Given that cpu0 has to be in pool0 then this should prevent 
> the pool to be destroyed.
> 
> Now, it is quite possible that we don't have a check to prevent CPU0 to be 
> removed from cpupool0. If so, then I would argue we should add the check 
> otherwise it is pointless to prevent cpu0 to be initially added in another 
> pool than pool0 but can be moved afterwards.
> 

Hi Julien,

I’ve done a test on fvp, first finding is that cpu0 can be removed from Pool-0, 
there is no check.
Afterwards I’ve created another pool and I’ve assigned a cpu to it, I’ve called 
xl cpupool-destroy and the tool removes every cpu from the pool before 
destroying.

Do you think the check that prevents CPU0 to be removed from Pool-0 should be 
done in the tools or in Xen?

With this change it could be possible to protect cpu0:

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index a6da4970506a..703005839dd6 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -585,6 +585,12 @@ static int cpupool_unassign_cpu(struct cpupool *c, 
unsigned int cpu)
     if ( !cpu_online(cpu) )
         return -EINVAL;
 
+    if ( !c->cpupool_id && !cpu )
+    {
+        debugtrace_printk("Cpu0 must be in pool with id 0.\n");
+        return -EINVAL;
+    }
+
     master_cpu = sched_get_resource_cpu(cpu);
     ret = cpupool_unassign_cpu_start(c, master_cpu);
     if ( ret )

Cheers,
Luca


> Cheers,
> 
> -- 
> Julien Grall