[Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy]

Since modern POWER7/POWER8 chips can have more that 256 CPU threads
(>2000 actually), remove this check from smp_parse.

The CPUs number is still checked against machine->max_cpus and this check
should be enough not to break other archs.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 vl.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/vl.c b/vl.c
index 8d5d874..e6ed260 100644
--- a/vl.c
+++ b/vl.c
@@ -1420,10 +1420,6 @@ static void smp_parse(QemuOpts *opts)
         max_cpus = smp_cpus;
     }
 
-    if (max_cpus > 255) {
-        fprintf(stderr, "Unsupported number of maxcpus\n");
-        exit(1);
-    }
     if (max_cpus < smp_cpus) {
         fprintf(stderr, "maxcpus must be equal to or greater than smp\n");
         exit(1);
-- 
1.8.4.rc4

[Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy]

Since modern POWER7/POWER8 chips can have more that 256 CPU threads
(>2000 actually), remove this check from smp_parse.

The CPUs number is still checked against machine->max_cpus and this check
should be enough not to break other archs.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 vl.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/vl.c b/vl.c
index 8d5d874..e6ed260 100644
--- a/vl.c
+++ b/vl.c
@@ -1420,10 +1420,6 @@ static void smp_parse(QemuOpts *opts)
         max_cpus = smp_cpus;
     }
 
-    if (max_cpus > 255) {
-        fprintf(stderr, "Unsupported number of maxcpus\n");
-        exit(1);
-    }
     if (max_cpus < smp_cpus) {
         fprintf(stderr, "maxcpus must be equal to or greater than smp\n");
         exit(1);
-- 
1.8.4.rc4

Re: [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Michael Tokarev]

25.11.2013 07:39, Alexey Kardashevskiy wrote:
> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> (>2000 actually), remove this check from smp_parse.
> 
> The CPUs number is still checked against machine->max_cpus and this check
> should be enough not to break other archs.
[]
> -    if (max_cpus > 255) {
> -        fprintf(stderr, "Unsupported number of maxcpus\n");
> -        exit(1);
> -    }

I don't know whenever this is actually safe.  Do we have any static arrays
of size 255 somewhere, which will be overflowed without this check? :)

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Michael Tokarev]

25.11.2013 07:39, Alexey Kardashevskiy wrote:
> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> (>2000 actually), remove this check from smp_parse.
> 
> The CPUs number is still checked against machine->max_cpus and this check
> should be enough not to break other archs.
[]
> -    if (max_cpus > 255) {
> -        fprintf(stderr, "Unsupported number of maxcpus\n");
> -        exit(1);
> -    }

I don't know whenever this is actually safe.  Do we have any static arrays
of size 255 somewhere, which will be overflowed without this check? :)

Thanks,

/mjt

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Andreas Färber]

Am 02.12.2013 18:06, schrieb Michael Tokarev:
> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>> (>2000 actually), remove this check from smp_parse.
>>
>> The CPUs number is still checked against machine->max_cpus and this check
>> should be enough not to break other archs.

"should be" is not exactly the highest level of confidence for a
"trivial" patch... :/

> []
>> -    if (max_cpus > 255) {
>> -        fprintf(stderr, "Unsupported number of maxcpus\n");
>> -        exit(1);
>> -    }

I believe Eduardo touched that code last for NUMA, so let's CC him.

> I don't know whenever this is actually safe.  Do we have any static arrays
> of size 255 somewhere, which will be overflowed without this check? :)

s390 has the ipi_states[] array, but not fixed to that size.

x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
Igor?

Alexey, did you actually check that, e.g., x86 machines don't break with
256 or 257 CPUs now?

Adding a qtest would be one way to prove that at least the QEMU code of
all other architectures doesn't break with the ppc change.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Andreas Färber]

Am 02.12.2013 18:06, schrieb Michael Tokarev:
> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>> (>2000 actually), remove this check from smp_parse.
>>
>> The CPUs number is still checked against machine->max_cpus and this check
>> should be enough not to break other archs.

"should be" is not exactly the highest level of confidence for a
"trivial" patch... :/

> []
>> -    if (max_cpus > 255) {
>> -        fprintf(stderr, "Unsupported number of maxcpus\n");
>> -        exit(1);
>> -    }

I believe Eduardo touched that code last for NUMA, so let's CC him.

> I don't know whenever this is actually safe.  Do we have any static arrays
> of size 255 somewhere, which will be overflowed without this check? :)

s390 has the ipi_states[] array, but not fixed to that size.

x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
Igor?

Alexey, did you actually check that, e.g., x86 machines don't break with
256 or 257 CPUs now?

Adding a qtest would be one way to prove that at least the QEMU code of
all other architectures doesn't break with the ppc change.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy]

On 12/03/2013 09:09 AM, Andreas Färber wrote:
> Am 02.12.2013 18:06, schrieb Michael Tokarev:
>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>>> (>2000 actually), remove this check from smp_parse.
>>>
>>> The CPUs number is still checked against machine->max_cpus and this check
>>> should be enough not to break other archs.
> 
> "should be" is not exactly the highest level of confidence for a
> "trivial" patch... :/
> 
>> []
>>> -    if (max_cpus > 255) {
>>> -        fprintf(stderr, "Unsupported number of maxcpus\n");
>>> -        exit(1);
>>> -    }
> 
> I believe Eduardo touched that code last for NUMA, so let's CC him.
> 
>> I don't know whenever this is actually safe.  Do we have any static arrays
>> of size 255 somewhere, which will be overflowed without this check? :)
> 
> s390 has the ipi_states[] array, but not fixed to that size.

s390 machines have QemuMachine::max_cpus == 255.


> x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
> Igor?
> 
> Alexey, did you actually check that, e.g., x86 machines don't break with
> 256 or 257 CPUs now?

PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
which would not define max_cpus, have I missed any?



-- 
Alexey

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy]

On 12/03/2013 09:09 AM, Andreas Färber wrote:
> Am 02.12.2013 18:06, schrieb Michael Tokarev:
>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>>> (>2000 actually), remove this check from smp_parse.
>>>
>>> The CPUs number is still checked against machine->max_cpus and this check
>>> should be enough not to break other archs.
> 
> "should be" is not exactly the highest level of confidence for a
> "trivial" patch... :/
> 
>> []
>>> -    if (max_cpus > 255) {
>>> -        fprintf(stderr, "Unsupported number of maxcpus\n");
>>> -        exit(1);
>>> -    }
> 
> I believe Eduardo touched that code last for NUMA, so let's CC him.
> 
>> I don't know whenever this is actually safe.  Do we have any static arrays
>> of size 255 somewhere, which will be overflowed without this check? :)
> 
> s390 has the ipi_states[] array, but not fixed to that size.

s390 machines have QemuMachine::max_cpus == 255.


> x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
> Igor?
> 
> Alexey, did you actually check that, e.g., x86 machines don't break with
> 256 or 257 CPUs now?

PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
which would not define max_cpus, have I missed any?



-- 
Alexey

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Markus Armbruster]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

[...]
>                                            And I cannot find any machine
> which would not define max_cpus, have I missed any?

If a machine lacks an explicit initializer for max_cpus, it'll remain
zero, which its users will interpret as 1.

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Markus Armbruster]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

[...]
>                                            And I cannot find any machine
> which would not define max_cpus, have I missed any?

If a machine lacks an explicit initializer for max_cpus, it'll remain
zero, which its users will interpret as 1.

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Andreas Färber]

Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
> On 12/03/2013 09:09 AM, Andreas Färber wrote:
>> Am 02.12.2013 18:06, schrieb Michael Tokarev:
>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>>>> (>2000 actually), remove this check from smp_parse.
>>>>
>>>> The CPUs number is still checked against machine->max_cpus and this check
>>>> should be enough not to break other archs.
>>
>> "should be" is not exactly the highest level of confidence for a
>> "trivial" patch... :/
[...]
>> Alexey, did you actually check that, e.g., x86 machines don't break with
>> 256 or 257 CPUs now?
> 
> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
> which would not define max_cpus, have I missed any?

If you've actually *checked* the other machines' code then fine with me,
just say so in the commit message. :)

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Andreas Färber]

Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
> On 12/03/2013 09:09 AM, Andreas Färber wrote:
>> Am 02.12.2013 18:06, schrieb Michael Tokarev:
>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>>>> (>2000 actually), remove this check from smp_parse.
>>>>
>>>> The CPUs number is still checked against machine->max_cpus and this check
>>>> should be enough not to break other archs.
>>
>> "should be" is not exactly the highest level of confidence for a
>> "trivial" patch... :/
[...]
>> Alexey, did you actually check that, e.g., x86 machines don't break with
>> 256 or 257 CPUs now?
> 
> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
> which would not define max_cpus, have I missed any?

If you've actually *checked* the other machines' code then fine with me,
just say so in the commit message. :)

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Eduardo Habkost]

On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote:
> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
> > On 12/03/2013 09:09 AM, Andreas Färber wrote:
> >> Am 02.12.2013 18:06, schrieb Michael Tokarev:
> >>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> >>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> >>>> (>2000 actually), remove this check from smp_parse.
> >>>>
> >>>> The CPUs number is still checked against machine->max_cpus and this check
> >>>> should be enough not to break other archs.
> >>
> >> "should be" is not exactly the highest level of confidence for a
> >> "trivial" patch... :/
> [...]
> >> Alexey, did you actually check that, e.g., x86 machines don't break with
> >> 256 or 257 CPUs now?
> > 
> > PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
> > which would not define max_cpus, have I missed any?
> 
> If you've actually *checked* the other machines' code then fine with me,
> just say so in the commit message. :)

I just grepped for "max_cpus" and checked every match. The largest
values I found were:

hw/ppc/spapr.c: 256
s390: 255
pc: 255

All the rest had values <= 32.

Machines with missing max_cpus value shouldn't be a problem, as
max_cpus==0 is interpreted as 1 by the vl.c code.

But we still need to add a check for max_cpus > machine->max_cpus to
vl.c, before we eliminate the smp_parse() check.

There's also this, at main():

        if (i == nb_numa_nodes) {
            for (i = 0; i < max_cpus; i++) {
                set_bit(i, node_cpumask[i % nb_numa_nodes]);
            }
        }

node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and
MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using
max_cpus instead, if we initialize it after smp_parse().

-- 
Eduardo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Eduardo Habkost]

On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote:
> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
> > On 12/03/2013 09:09 AM, Andreas Färber wrote:
> >> Am 02.12.2013 18:06, schrieb Michael Tokarev:
> >>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> >>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> >>>> (>2000 actually), remove this check from smp_parse.
> >>>>
> >>>> The CPUs number is still checked against machine->max_cpus and this check
> >>>> should be enough not to break other archs.
> >>
> >> "should be" is not exactly the highest level of confidence for a
> >> "trivial" patch... :/
> [...]
> >> Alexey, did you actually check that, e.g., x86 machines don't break with
> >> 256 or 257 CPUs now?
> > 
> > PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
> > which would not define max_cpus, have I missed any?
> 
> If you've actually *checked* the other machines' code then fine with me,
> just say so in the commit message. :)

I just grepped for "max_cpus" and checked every match. The largest
values I found were:

hw/ppc/spapr.c: 256
s390: 255
pc: 255

All the rest had values <= 32.

Machines with missing max_cpus value shouldn't be a problem, as
max_cpus==0 is interpreted as 1 by the vl.c code.

But we still need to add a check for max_cpus > machine->max_cpus to
vl.c, before we eliminate the smp_parse() check.

There's also this, at main():

        if (i == nb_numa_nodes) {
            for (i = 0; i < max_cpus; i++) {
                set_bit(i, node_cpumask[i % nb_numa_nodes]);
            }
        }

node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and
MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using
max_cpus instead, if we initialize it after smp_parse().

-- 
Eduardo

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy]

On 12/04/2013 01:47 AM, Eduardo Habkost wrote:
> On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote:
>> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
>>> On 12/03/2013 09:09 AM, Andreas Färber wrote:
>>>> Am 02.12.2013 18:06, schrieb Michael Tokarev:
>>>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>>>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>>>>>> (>2000 actually), remove this check from smp_parse.
>>>>>>
>>>>>> The CPUs number is still checked against machine->max_cpus and this check
>>>>>> should be enough not to break other archs.
>>>>
>>>> "should be" is not exactly the highest level of confidence for a
>>>> "trivial" patch... :/
>> [...]
>>>> Alexey, did you actually check that, e.g., x86 machines don't break with
>>>> 256 or 257 CPUs now?
>>>
>>> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
>>> which would not define max_cpus, have I missed any?
>>
>> If you've actually *checked* the other machines' code then fine with me,
>> just say so in the commit message. :)
> 
> I just grepped for "max_cpus" and checked every match. The largest
> values I found were:
> 
> hw/ppc/spapr.c: 256
> s390: 255
> pc: 255
> 
> All the rest had values <= 32.
> 
> Machines with missing max_cpus value shouldn't be a problem, as
> max_cpus==0 is interpreted as 1 by the vl.c code.
> 
> But we still need to add a check for max_cpus > machine->max_cpus to
> vl.c, before we eliminate the smp_parse() check.


Since smp_parse() checks if (max_cpus >= smp_cpus), this should just work:

diff --git a/vl.c b/vl.c
index e6ed260..544165a 100644
--- a/vl.c
+++ b/vl.c
@@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp)
     smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));

     machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
-    if (smp_cpus > machine->max_cpus) {
+    if (max_cpus > machine->max_cpus) {
         fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
-                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
+                "supported by machine `%s' (%d)\n", max_cpus,  machine->name,
                 machine->max_cpus);
         exit(1);
     }


> There's also this, at main():
> 
>         if (i == nb_numa_nodes) {
>             for (i = 0; i < max_cpus; i++) {
>                 set_bit(i, node_cpumask[i % nb_numa_nodes]);
>             }
>         }
> 
> node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and
> MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using
> max_cpus instead, if we initialize it after smp_parse().


Nope. At the moment when we parse -numa in vl.c, we may not know yet what
machine is going to be used and machines can have different max_cpus.

For now, I would simply change MAX_CPUMASK_BITS to something crazy, like
16384 (2KB per numa node), I hope QEMU can survive such a memory waste :)

Ok?


-- 
Alexey

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy]

On 12/04/2013 01:47 AM, Eduardo Habkost wrote:
> On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote:
>> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
>>> On 12/03/2013 09:09 AM, Andreas Färber wrote:
>>>> Am 02.12.2013 18:06, schrieb Michael Tokarev:
>>>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>>>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>>>>>> (>2000 actually), remove this check from smp_parse.
>>>>>>
>>>>>> The CPUs number is still checked against machine->max_cpus and this check
>>>>>> should be enough not to break other archs.
>>>>
>>>> "should be" is not exactly the highest level of confidence for a
>>>> "trivial" patch... :/
>> [...]
>>>> Alexey, did you actually check that, e.g., x86 machines don't break with
>>>> 256 or 257 CPUs now?
>>>
>>> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
>>> which would not define max_cpus, have I missed any?
>>
>> If you've actually *checked* the other machines' code then fine with me,
>> just say so in the commit message. :)
> 
> I just grepped for "max_cpus" and checked every match. The largest
> values I found were:
> 
> hw/ppc/spapr.c: 256
> s390: 255
> pc: 255
> 
> All the rest had values <= 32.
> 
> Machines with missing max_cpus value shouldn't be a problem, as
> max_cpus==0 is interpreted as 1 by the vl.c code.
> 
> But we still need to add a check for max_cpus > machine->max_cpus to
> vl.c, before we eliminate the smp_parse() check.


Since smp_parse() checks if (max_cpus >= smp_cpus), this should just work:

diff --git a/vl.c b/vl.c
index e6ed260..544165a 100644
--- a/vl.c
+++ b/vl.c
@@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp)
     smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));

     machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
-    if (smp_cpus > machine->max_cpus) {
+    if (max_cpus > machine->max_cpus) {
         fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
-                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
+                "supported by machine `%s' (%d)\n", max_cpus,  machine->name,
                 machine->max_cpus);
         exit(1);
     }


> There's also this, at main():
> 
>         if (i == nb_numa_nodes) {
>             for (i = 0; i < max_cpus; i++) {
>                 set_bit(i, node_cpumask[i % nb_numa_nodes]);
>             }
>         }
> 
> node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and
> MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using
> max_cpus instead, if we initialize it after smp_parse().


Nope. At the moment when we parse -numa in vl.c, we may not know yet what
machine is going to be used and machines can have different max_cpus.

For now, I would simply change MAX_CPUMASK_BITS to something crazy, like
16384 (2KB per numa node), I hope QEMU can survive such a memory waste :)

Ok?


-- 
Alexey

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Eduardo Habkost]

On Wed, Dec 04, 2013 at 04:50:59PM +1100, Alexey Kardashevskiy wrote:
> On 12/04/2013 01:47 AM, Eduardo Habkost wrote:
> > On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote:
> >> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
> >>> On 12/03/2013 09:09 AM, Andreas Färber wrote:
> >>>> Am 02.12.2013 18:06, schrieb Michael Tokarev:
> >>>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> >>>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> >>>>>> (>2000 actually), remove this check from smp_parse.
> >>>>>>
> >>>>>> The CPUs number is still checked against machine->max_cpus and this check
> >>>>>> should be enough not to break other archs.
> >>>>
> >>>> "should be" is not exactly the highest level of confidence for a
> >>>> "trivial" patch... :/
> >> [...]
> >>>> Alexey, did you actually check that, e.g., x86 machines don't break with
> >>>> 256 or 257 CPUs now?
> >>>
> >>> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
> >>> which would not define max_cpus, have I missed any?
> >>
> >> If you've actually *checked* the other machines' code then fine with me,
> >> just say so in the commit message. :)
> > 
> > I just grepped for "max_cpus" and checked every match. The largest
> > values I found were:
> > 
> > hw/ppc/spapr.c: 256
> > s390: 255
> > pc: 255
> > 
> > All the rest had values <= 32.
> > 
> > Machines with missing max_cpus value shouldn't be a problem, as
> > max_cpus==0 is interpreted as 1 by the vl.c code.
> > 
> > But we still need to add a check for max_cpus > machine->max_cpus to
> > vl.c, before we eliminate the smp_parse() check.
> 
> 
> Since smp_parse() checks if (max_cpus >= smp_cpus), this should just work:
> 
> diff --git a/vl.c b/vl.c
> index e6ed260..544165a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp)
>      smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
> 
>      machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
> -    if (smp_cpus > machine->max_cpus) {
> +    if (max_cpus > machine->max_cpus) {
>          fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
> -                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
> +                "supported by machine `%s' (%d)\n", max_cpus,  machine->name,
>                  machine->max_cpus);
>          exit(1);
>      }
> 
> 
> > There's also this, at main():
> > 
> >         if (i == nb_numa_nodes) {
> >             for (i = 0; i < max_cpus; i++) {
> >                 set_bit(i, node_cpumask[i % nb_numa_nodes]);
> >             }
> >         }
> > 
> > node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and
> > MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using
> > max_cpus instead, if we initialize it after smp_parse().
> 
> 
> Nope. At the moment when we parse -numa in vl.c, we may not know yet what
> machine is going to be used and machines can have different max_cpus.

This will be changed by:

  Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor
  Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com>
  http://article.gmane.org/gmane.comp.emulators.qemu/244826

> 
> For now, I would simply change MAX_CPUMASK_BITS to something crazy, like
> 16384 (2KB per numa node), I hope QEMU can survive such a memory waste :)
> 
> Ok?

I'm OK with that as long the code has proper checks in case max_cpus
gets set to a crazily large value (larger than MAX_CPUMASK_BITS) in the
far future, or if we prevent max_cpus from being larger than
MAX_CPUMASK_BITS.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Eduardo Habkost]

On Wed, Dec 04, 2013 at 04:50:59PM +1100, Alexey Kardashevskiy wrote:
> On 12/04/2013 01:47 AM, Eduardo Habkost wrote:
> > On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote:
> >> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
> >>> On 12/03/2013 09:09 AM, Andreas Färber wrote:
> >>>> Am 02.12.2013 18:06, schrieb Michael Tokarev:
> >>>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> >>>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> >>>>>> (>2000 actually), remove this check from smp_parse.
> >>>>>>
> >>>>>> The CPUs number is still checked against machine->max_cpus and this check
> >>>>>> should be enough not to break other archs.
> >>>>
> >>>> "should be" is not exactly the highest level of confidence for a
> >>>> "trivial" patch... :/
> >> [...]
> >>>> Alexey, did you actually check that, e.g., x86 machines don't break with
> >>>> 256 or 257 CPUs now?
> >>>
> >>> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
> >>> which would not define max_cpus, have I missed any?
> >>
> >> If you've actually *checked* the other machines' code then fine with me,
> >> just say so in the commit message. :)
> > 
> > I just grepped for "max_cpus" and checked every match. The largest
> > values I found were:
> > 
> > hw/ppc/spapr.c: 256
> > s390: 255
> > pc: 255
> > 
> > All the rest had values <= 32.
> > 
> > Machines with missing max_cpus value shouldn't be a problem, as
> > max_cpus==0 is interpreted as 1 by the vl.c code.
> > 
> > But we still need to add a check for max_cpus > machine->max_cpus to
> > vl.c, before we eliminate the smp_parse() check.
> 
> 
> Since smp_parse() checks if (max_cpus >= smp_cpus), this should just work:
> 
> diff --git a/vl.c b/vl.c
> index e6ed260..544165a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp)
>      smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
> 
>      machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
> -    if (smp_cpus > machine->max_cpus) {
> +    if (max_cpus > machine->max_cpus) {
>          fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
> -                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
> +                "supported by machine `%s' (%d)\n", max_cpus,  machine->name,
>                  machine->max_cpus);
>          exit(1);
>      }
> 
> 
> > There's also this, at main():
> > 
> >         if (i == nb_numa_nodes) {
> >             for (i = 0; i < max_cpus; i++) {
> >                 set_bit(i, node_cpumask[i % nb_numa_nodes]);
> >             }
> >         }
> > 
> > node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and
> > MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using
> > max_cpus instead, if we initialize it after smp_parse().
> 
> 
> Nope. At the moment when we parse -numa in vl.c, we may not know yet what
> machine is going to be used and machines can have different max_cpus.

This will be changed by:

  Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor
  Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com>
  http://article.gmane.org/gmane.comp.emulators.qemu/244826

> 
> For now, I would simply change MAX_CPUMASK_BITS to something crazy, like
> 16384 (2KB per numa node), I hope QEMU can survive such a memory waste :)
> 
> Ok?

I'm OK with that as long the code has proper checks in case max_cpus
gets set to a crazily large value (larger than MAX_CPUMASK_BITS) in the
far future, or if we prevent max_cpus from being larger than
MAX_CPUMASK_BITS.

-- 
Eduardo

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy]

On 12/04/2013 11:48 PM, Eduardo Habkost wrote:
> On Wed, Dec 04, 2013 at 04:50:59PM +1100, Alexey Kardashevskiy wrote:
>> On 12/04/2013 01:47 AM, Eduardo Habkost wrote:
>>> On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote:
>>>> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
>>>>> On 12/03/2013 09:09 AM, Andreas Färber wrote:
>>>>>> Am 02.12.2013 18:06, schrieb Michael Tokarev:
>>>>>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>>>>>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>>>>>>>> (>2000 actually), remove this check from smp_parse.
>>>>>>>>
>>>>>>>> The CPUs number is still checked against machine->max_cpus and this check
>>>>>>>> should be enough not to break other archs.
>>>>>>
>>>>>> "should be" is not exactly the highest level of confidence for a
>>>>>> "trivial" patch... :/
>>>> [...]
>>>>>> Alexey, did you actually check that, e.g., x86 machines don't break with
>>>>>> 256 or 257 CPUs now?
>>>>>
>>>>> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
>>>>> which would not define max_cpus, have I missed any?
>>>>
>>>> If you've actually *checked* the other machines' code then fine with me,
>>>> just say so in the commit message. :)
>>>
>>> I just grepped for "max_cpus" and checked every match. The largest
>>> values I found were:
>>>
>>> hw/ppc/spapr.c: 256
>>> s390: 255
>>> pc: 255
>>>
>>> All the rest had values <= 32.
>>>
>>> Machines with missing max_cpus value shouldn't be a problem, as
>>> max_cpus==0 is interpreted as 1 by the vl.c code.
>>>
>>> But we still need to add a check for max_cpus > machine->max_cpus to
>>> vl.c, before we eliminate the smp_parse() check.
>>
>>
>> Since smp_parse() checks if (max_cpus >= smp_cpus), this should just work:
>>
>> diff --git a/vl.c b/vl.c
>> index e6ed260..544165a 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp)
>>      smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>>
>>      machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
>> -    if (smp_cpus > machine->max_cpus) {
>> +    if (max_cpus > machine->max_cpus) {
>>          fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
>> -                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
>> +                "supported by machine `%s' (%d)\n", max_cpus,  machine->name,
>>                  machine->max_cpus);
>>          exit(1);
>>      }
>>
>>
>>> There's also this, at main():
>>>
>>>         if (i == nb_numa_nodes) {
>>>             for (i = 0; i < max_cpus; i++) {
>>>                 set_bit(i, node_cpumask[i % nb_numa_nodes]);
>>>             }
>>>         }
>>>
>>> node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and
>>> MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using
>>> max_cpus instead, if we initialize it after smp_parse().
>>
>>
>> Nope. At the moment when we parse -numa in vl.c, we may not know yet what
>> machine is going to be used and machines can have different max_cpus.
> 
> This will be changed by:
> 
>   Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor
>   Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com>
>   http://article.gmane.org/gmane.comp.emulators.qemu/244826


Any progress with this? Thanks.


>>
>> For now, I would simply change MAX_CPUMASK_BITS to something crazy, like
>> 16384 (2KB per numa node), I hope QEMU can survive such a memory waste :)
>>
>> Ok?
> 
> I'm OK with that as long the code has proper checks in case max_cpus
> gets set to a crazily large value (larger than MAX_CPUMASK_BITS) in the
> far future, or if we prevent max_cpus from being larger than
> MAX_CPUMASK_BITS.
> 


-- 
Alexey

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy]

On 12/04/2013 11:48 PM, Eduardo Habkost wrote:
> On Wed, Dec 04, 2013 at 04:50:59PM +1100, Alexey Kardashevskiy wrote:
>> On 12/04/2013 01:47 AM, Eduardo Habkost wrote:
>>> On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote:
>>>> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy:
>>>>> On 12/03/2013 09:09 AM, Andreas Färber wrote:
>>>>>> Am 02.12.2013 18:06, schrieb Michael Tokarev:
>>>>>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote:
>>>>>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
>>>>>>>> (>2000 actually), remove this check from smp_parse.
>>>>>>>>
>>>>>>>> The CPUs number is still checked against machine->max_cpus and this check
>>>>>>>> should be enough not to break other archs.
>>>>>>
>>>>>> "should be" is not exactly the highest level of confidence for a
>>>>>> "trivial" patch... :/
>>>> [...]
>>>>>> Alexey, did you actually check that, e.g., x86 machines don't break with
>>>>>> 256 or 257 CPUs now?
>>>>>
>>>>> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine
>>>>> which would not define max_cpus, have I missed any?
>>>>
>>>> If you've actually *checked* the other machines' code then fine with me,
>>>> just say so in the commit message. :)
>>>
>>> I just grepped for "max_cpus" and checked every match. The largest
>>> values I found were:
>>>
>>> hw/ppc/spapr.c: 256
>>> s390: 255
>>> pc: 255
>>>
>>> All the rest had values <= 32.
>>>
>>> Machines with missing max_cpus value shouldn't be a problem, as
>>> max_cpus==0 is interpreted as 1 by the vl.c code.
>>>
>>> But we still need to add a check for max_cpus > machine->max_cpus to
>>> vl.c, before we eliminate the smp_parse() check.
>>
>>
>> Since smp_parse() checks if (max_cpus >= smp_cpus), this should just work:
>>
>> diff --git a/vl.c b/vl.c
>> index e6ed260..544165a 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp)
>>      smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>>
>>      machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
>> -    if (smp_cpus > machine->max_cpus) {
>> +    if (max_cpus > machine->max_cpus) {
>>          fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
>> -                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
>> +                "supported by machine `%s' (%d)\n", max_cpus,  machine->name,
>>                  machine->max_cpus);
>>          exit(1);
>>      }
>>
>>
>>> There's also this, at main():
>>>
>>>         if (i == nb_numa_nodes) {
>>>             for (i = 0; i < max_cpus; i++) {
>>>                 set_bit(i, node_cpumask[i % nb_numa_nodes]);
>>>             }
>>>         }
>>>
>>> node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and
>>> MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using
>>> max_cpus instead, if we initialize it after smp_parse().
>>
>>
>> Nope. At the moment when we parse -numa in vl.c, we may not know yet what
>> machine is going to be used and machines can have different max_cpus.
> 
> This will be changed by:
> 
>   Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor
>   Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com>
>   http://article.gmane.org/gmane.comp.emulators.qemu/244826


Any progress with this? Thanks.


>>
>> For now, I would simply change MAX_CPUMASK_BITS to something crazy, like
>> 16384 (2KB per numa node), I hope QEMU can survive such a memory waste :)
>>
>> Ok?
> 
> I'm OK with that as long the code has proper checks in case max_cpus
> gets set to a crazily large value (larger than MAX_CPUMASK_BITS) in the
> far future, or if we prevent max_cpus from being larger than
> MAX_CPUMASK_BITS.
> 


-- 
Alexey

Re: [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Paolo Bonzini]

Il 14/02/2014 07:56, Alexey Kardashevskiy ha scritto:
>> >   Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor
>> >   Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com>
>> >   http://article.gmane.org/gmane.comp.emulators.qemu/244826
>
> Any progress with this? Thanks.

Yes, there was some progress though for some reason it happened onlist.

Igor/Wanlong, can you send the first patches of the "common" NUMA/memory 
series, i.e. excluding the memdev backend?

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Paolo Bonzini]

Il 14/02/2014 07:56, Alexey Kardashevskiy ha scritto:
>> >   Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor
>> >   Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com>
>> >   http://article.gmane.org/gmane.comp.emulators.qemu/244826
>
> Any progress with this? Thanks.

Yes, there was some progress though for some reason it happened onlist.

Igor/Wanlong, can you send the first patches of the "common" NUMA/memory 
series, i.e. excluding the memdev backend?

Paolo

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Igor Mammedov]

On Mon, 02 Dec 2013 23:09:55 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 02.12.2013 18:06, schrieb Michael Tokarev:
> > 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> >> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> >> (>2000 actually), remove this check from smp_parse.
> >>
> >> The CPUs number is still checked against machine->max_cpus and this check
> >> should be enough not to break other archs.
> 
> "should be" is not exactly the highest level of confidence for a
> "trivial" patch... :/
> 
> > []
> >> -    if (max_cpus > 255) {
> >> -        fprintf(stderr, "Unsupported number of maxcpus\n");
> >> -        exit(1);
> >> -    }
> 
> I believe Eduardo touched that code last for NUMA, so let's CC him.
> 
> > I don't know whenever this is actually safe.  Do we have any static arrays
> > of size 255 somewhere, which will be overflowed without this check? :)
> 
> s390 has the ipi_states[] array, but not fixed to that size.
> 
> x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
> Igor?
it's still fixed size array:
hw/intc/apic.c: static APICCommonState *local_apics[MAX_APICS + 1];
with: #define MAX_APICS 255

> 
> Alexey, did you actually check that, e.g., x86 machines don't break with
> 256 or 257 CPUs now?
patch shouldn't hurt x86 machines since
there is check later 
vl.c:
    if (smp_cpus > machine->max_cpus) {
           fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
                machine->max_cpus);
        exit(1);
    }
       
and both piix4/q35 machines have machine->max_cpus initialized to 255 or lower(isa/xen).

> 
> Adding a qtest would be one way to prove that at least the QEMU code of
> all other architectures doesn't break with the ppc change.
> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Igor Mammedov]

On Mon, 02 Dec 2013 23:09:55 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 02.12.2013 18:06, schrieb Michael Tokarev:
> > 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> >> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> >> (>2000 actually), remove this check from smp_parse.
> >>
> >> The CPUs number is still checked against machine->max_cpus and this check
> >> should be enough not to break other archs.
> 
> "should be" is not exactly the highest level of confidence for a
> "trivial" patch... :/
> 
> > []
> >> -    if (max_cpus > 255) {
> >> -        fprintf(stderr, "Unsupported number of maxcpus\n");
> >> -        exit(1);
> >> -    }
> 
> I believe Eduardo touched that code last for NUMA, so let's CC him.
> 
> > I don't know whenever this is actually safe.  Do we have any static arrays
> > of size 255 somewhere, which will be overflowed without this check? :)
> 
> s390 has the ipi_states[] array, but not fixed to that size.
> 
> x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
> Igor?
it's still fixed size array:
hw/intc/apic.c: static APICCommonState *local_apics[MAX_APICS + 1];
with: #define MAX_APICS 255

> 
> Alexey, did you actually check that, e.g., x86 machines don't break with
> 256 or 257 CPUs now?
patch shouldn't hurt x86 machines since
there is check later 
vl.c:
    if (smp_cpus > machine->max_cpus) {
           fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
                machine->max_cpus);
        exit(1);
    }
       
and both piix4/q35 machines have machine->max_cpus initialized to 255 or lower(isa/xen).

> 
> Adding a qtest would be one way to prove that at least the QEMU code of
> all other architectures doesn't break with the ppc change.
> 
> Regards,
> Andreas
> 

Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Eduardo Habkost]

On Tue, Dec 03, 2013 at 11:44:19AM +0100, Igor Mammedov wrote:
> On Mon, 02 Dec 2013 23:09:55 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
> > Am 02.12.2013 18:06, schrieb Michael Tokarev:
> > > 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> > >> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> > >> (>2000 actually), remove this check from smp_parse.
> > >>
> > >> The CPUs number is still checked against machine->max_cpus and this check
> > >> should be enough not to break other archs.
> > 
> > "should be" is not exactly the highest level of confidence for a
> > "trivial" patch... :/
> > 
> > > []
> > >> -    if (max_cpus > 255) {
> > >> -        fprintf(stderr, "Unsupported number of maxcpus\n");
> > >> -        exit(1);
> > >> -    }
> > 
> > I believe Eduardo touched that code last for NUMA, so let's CC him.
> > 
> > > I don't know whenever this is actually safe.  Do we have any static arrays
> > > of size 255 somewhere, which will be overflowed without this check? :)
> > 
> > s390 has the ipi_states[] array, but not fixed to that size.
> > 
> > x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
> > Igor?
> it's still fixed size array:
> hw/intc/apic.c: static APICCommonState *local_apics[MAX_APICS + 1];
> with: #define MAX_APICS 255
> 
> > 
> > Alexey, did you actually check that, e.g., x86 machines don't break with
> > 256 or 257 CPUs now?
> patch shouldn't hurt x86 machines since
> there is check later 
> vl.c:
>     if (smp_cpus > machine->max_cpus) {
>            fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
>                 "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
>                 machine->max_cpus);
>         exit(1);
>     }
>        
> and both piix4/q35 machines have machine->max_cpus initialized to 255 or lower(isa/xen).

This ensures smp_cpus <= machine->max_cpus, but doesn't ensure
max_cpus <= machine->max_cpus. We need to check max_cpus against
machine->max_cpus, too, then we can eliminate the hardcoded
max_cpus > 255 check.

> 
> > 
> > Adding a qtest would be one way to prove that at least the QEMU code of
> > all other architectures doesn't break with the ppc change.
> > 
> > Regards,
> > Andreas
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Eduardo Habkost]

On Tue, Dec 03, 2013 at 11:44:19AM +0100, Igor Mammedov wrote:
> On Mon, 02 Dec 2013 23:09:55 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
> > Am 02.12.2013 18:06, schrieb Michael Tokarev:
> > > 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> > >> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> > >> (>2000 actually), remove this check from smp_parse.
> > >>
> > >> The CPUs number is still checked against machine->max_cpus and this check
> > >> should be enough not to break other archs.
> > 
> > "should be" is not exactly the highest level of confidence for a
> > "trivial" patch... :/
> > 
> > > []
> > >> -    if (max_cpus > 255) {
> > >> -        fprintf(stderr, "Unsupported number of maxcpus\n");
> > >> -        exit(1);
> > >> -    }
> > 
> > I believe Eduardo touched that code last for NUMA, so let's CC him.
> > 
> > > I don't know whenever this is actually safe.  Do we have any static arrays
> > > of size 255 somewhere, which will be overflowed without this check? :)
> > 
> > s390 has the ipi_states[] array, but not fixed to that size.
> > 
> > x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
> > Igor?
> it's still fixed size array:
> hw/intc/apic.c: static APICCommonState *local_apics[MAX_APICS + 1];
> with: #define MAX_APICS 255
> 
> > 
> > Alexey, did you actually check that, e.g., x86 machines don't break with
> > 256 or 257 CPUs now?
> patch shouldn't hurt x86 machines since
> there is check later 
> vl.c:
>     if (smp_cpus > machine->max_cpus) {
>            fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
>                 "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
>                 machine->max_cpus);
>         exit(1);
>     }
>        
> and both piix4/q35 machines have machine->max_cpus initialized to 255 or lower(isa/xen).

This ensures smp_cpus <= machine->max_cpus, but doesn't ensure
max_cpus <= machine->max_cpus. We need to check max_cpus against
machine->max_cpus, too, then we can eliminate the hardcoded
max_cpus > 255 check.

> 
> > 
> > Adding a qtest would be one way to prove that at least the QEMU code of
> > all other architectures doesn't break with the ppc change.
> > 
> > Regards,
> > Andreas
> > 
> 

-- 
Eduardo