cpus: verify that number of created cpus do not exceed smp params

[PATCH] cpus: verify that number of created cpus do not exceed smp params

Posted by Pavel Dovgalyuk 3 years, 6 months ago

Machine definitions may miss some vCPU-related parameters.
E.g., xlnx-versal-virt missed min_cpus and it was set to 1 by default.
This allowed using -smp 1 command line argument. But the machine
still created 2 vCPUs and passed all checks.
This patch adds one more check that does not allow creating
extra cpus that exceed the values specified in machine/smp.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
---
 0 files changed

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 47cceddd80..da74794e09 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -603,6 +603,11 @@ void qemu_init_vcpu(CPUState *cpu)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
 
+    if (cpu->cpu_index >= ms->smp.cpus) {
+        fprintf(stderr, "Machine definition error: trying to create too many CPUs\n");
+        exit(1);
+    }
+
     cpu->nr_cores = ms->smp.cores;
     cpu->nr_threads =  ms->smp.threads;
     cpu->stopped = true;

Re: [PATCH] cpus: verify that number of created cpus do not exceed smp params

Posted by Philippe Mathieu-Daudé 3 years, 6 months ago

On 10/23/20 9:34 AM, Pavel Dovgalyuk wrote:
> Machine definitions may miss some vCPU-related parameters.
> E.g., xlnx-versal-virt missed min_cpus and it was set to 1 by default.
> This allowed using -smp 1 command line argument. But the machine
> still created 2 vCPUs and passed all checks.
> This patch adds one more check that does not allow creating
> extra cpus that exceed the values specified in machine/smp.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> ---
>   0 files changed
> 
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 47cceddd80..da74794e09 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -603,6 +603,11 @@ void qemu_init_vcpu(CPUState *cpu)
>   {
>       MachineState *ms = MACHINE(qdev_get_machine());
>   
> +    if (cpu->cpu_index >= ms->smp.cpus) {
> +        fprintf(stderr, "Machine definition error: trying to create too many CPUs\n");
> +        exit(1);

Shouldn't this be an assert()?

> +    }
> +
>       cpu->nr_cores = ms->smp.cores;
>       cpu->nr_threads =  ms->smp.threads;
>       cpu->stopped = true;
> 
>

Re: [PATCH] cpus: verify that number of created cpus do not exceed smp params

Posted by Pavel Dovgalyuk 3 years, 6 months ago

On 23.10.2020 11:10, Philippe Mathieu-Daudé wrote:
> On 10/23/20 9:34 AM, Pavel Dovgalyuk wrote:
>> Machine definitions may miss some vCPU-related parameters.
>> E.g., xlnx-versal-virt missed min_cpus and it was set to 1 by default.
>> This allowed using -smp 1 command line argument. But the machine
>> still created 2 vCPUs and passed all checks.
>> This patch adds one more check that does not allow creating
>> extra cpus that exceed the values specified in machine/smp.
>>
>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>> ---
>>   0 files changed
>>
>> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
>> index 47cceddd80..da74794e09 100644
>> --- a/softmmu/cpus.c
>> +++ b/softmmu/cpus.c
>> @@ -603,6 +603,11 @@ void qemu_init_vcpu(CPUState *cpu)
>>   {
>>       MachineState *ms = MACHINE(qdev_get_machine());
>> +    if (cpu->cpu_index >= ms->smp.cpus) {
>> +        fprintf(stderr, "Machine definition error: trying to create 
>> too many CPUs\n");
>> +        exit(1);
> 
> Shouldn't this be an assert()?

I thought about this.
Bugs could be unnoticed and reveal in release. Therefore user should 
know the details and shouldn't see an assert.

> 
>> +    }
>> +
>>       cpu->nr_cores = ms->smp.cores;
>>       cpu->nr_threads =  ms->smp.threads;
>>       cpu->stopped = true;
>>
>>
>

Re: [PATCH] cpus: verify that number of created cpus do not exceed smp params

Posted by Igor Mammedov 3 years, 6 months ago

On Fri, 23 Oct 2020 10:34:41 +0300
Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote:

> Machine definitions may miss some vCPU-related parameters.
> E.g., xlnx-versal-virt missed min_cpus and it was set to 1 by default.
> This allowed using -smp 1 command line argument. But the machine
> still created 2 vCPUs and passed all checks.
> This patch adds one more check that does not allow creating
> extra cpus that exceed the values specified in machine/smp.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> ---
>  0 files changed
> 
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 47cceddd80..da74794e09 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -603,6 +603,11 @@ void qemu_init_vcpu(CPUState *cpu)
>  {
>      MachineState *ms = MACHINE(qdev_get_machine());
>  
> +    if (cpu->cpu_index >= ms->smp.cpus) {
looks like for such machines we need MachineClass:min_cpus + setting it in affected machines
and corresponding check in smp_parse(), instead of terminating from qemu_init_vcpu();


> +        fprintf(stderr, "Machine definition error: trying to create too many CPUs\n");
> +        exit(1);
> +    }
> +
>      cpu->nr_cores = ms->smp.cores;
>      cpu->nr_threads =  ms->smp.threads;
>      cpu->stopped = true;
> 
>