* [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
@ 2013-11-25 3:39 Alexey Kardashevskiy
2013-12-02 17:06 ` [Qemu-devel] [Qemu-trivial] " Michael Tokarev
0 siblings, 1 reply; 13+ messages in thread
From: Alexey Kardashevskiy @ 2013-11-25 3:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexey Kardashevskiy, qemu-trivial
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-11-25 3:39 [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse Alexey Kardashevskiy
@ 2013-12-02 17:06 ` Michael Tokarev
2013-12-02 22:09 ` Andreas Färber
0 siblings, 1 reply; 13+ messages in thread
From: Michael Tokarev @ 2013-12-02 17:06 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: qemu-trivial, qemu-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-02 17:06 ` [Qemu-devel] [Qemu-trivial] " Michael Tokarev
@ 2013-12-02 22:09 ` Andreas Färber
2013-12-02 23:03 ` Alexey Kardashevskiy
2013-12-03 10:44 ` Igor Mammedov
0 siblings, 2 replies; 13+ messages in thread
From: Andreas Färber @ 2013-12-02 22:09 UTC (permalink / raw)
To: Michael Tokarev, Alexey Kardashevskiy
Cc: qemu-trivial, Igor Mammedov, qemu-devel, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-02 22:09 ` Andreas Färber
@ 2013-12-02 23:03 ` Alexey Kardashevskiy
2013-12-03 9:00 ` Markus Armbruster
2013-12-03 13:30 ` Andreas Färber
2013-12-03 10:44 ` Igor Mammedov
1 sibling, 2 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2013-12-02 23:03 UTC (permalink / raw)
To: Andreas Färber, Michael Tokarev
Cc: qemu-trivial, Igor Mammedov, qemu-devel, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-02 23:03 ` Alexey Kardashevskiy
@ 2013-12-03 9:00 ` Markus Armbruster
2013-12-03 13:30 ` Andreas Färber
1 sibling, 0 replies; 13+ messages in thread
From: Markus Armbruster @ 2013-12-03 9:00 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Eduardo Habkost, qemu-trivial, Michael Tokarev, qemu-devel,
Igor Mammedov, Andreas Färber
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-02 22:09 ` Andreas Färber
2013-12-02 23:03 ` Alexey Kardashevskiy
@ 2013-12-03 10:44 ` Igor Mammedov
2013-12-03 14:33 ` Eduardo Habkost
1 sibling, 1 reply; 13+ messages in thread
From: Igor Mammedov @ 2013-12-03 10:44 UTC (permalink / raw)
To: Andreas Färber
Cc: Alexey Kardashevskiy, qemu-trivial, Michael Tokarev, qemu-devel,
Eduardo Habkost
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-02 23:03 ` Alexey Kardashevskiy
2013-12-03 9:00 ` Markus Armbruster
@ 2013-12-03 13:30 ` Andreas Färber
2013-12-03 14:47 ` Eduardo Habkost
1 sibling, 1 reply; 13+ messages in thread
From: Andreas Färber @ 2013-12-03 13:30 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: qemu-trivial, Igor Mammedov, Michael Tokarev, qemu-devel,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-03 10:44 ` Igor Mammedov
@ 2013-12-03 14:33 ` Eduardo Habkost
0 siblings, 0 replies; 13+ messages in thread
From: Eduardo Habkost @ 2013-12-03 14:33 UTC (permalink / raw)
To: Igor Mammedov
Cc: Alexey Kardashevskiy, qemu-trivial, Michael Tokarev,
Andreas Färber, qemu-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-03 13:30 ` Andreas Färber
@ 2013-12-03 14:47 ` Eduardo Habkost
2013-12-04 5:50 ` Alexey Kardashevskiy
0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2013-12-03 14:47 UTC (permalink / raw)
To: Andreas Färber
Cc: Alexey Kardashevskiy, qemu-trivial, Michael Tokarev, qemu-devel,
Igor Mammedov
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-03 14:47 ` Eduardo Habkost
@ 2013-12-04 5:50 ` Alexey Kardashevskiy
2013-12-04 12:48 ` Eduardo Habkost
0 siblings, 1 reply; 13+ messages in thread
From: Alexey Kardashevskiy @ 2013-12-04 5:50 UTC (permalink / raw)
To: Eduardo Habkost, Andreas Färber
Cc: qemu-trivial, Igor Mammedov, Michael Tokarev, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-04 5:50 ` Alexey Kardashevskiy
@ 2013-12-04 12:48 ` Eduardo Habkost
2014-02-14 6:56 ` Alexey Kardashevskiy
0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2013-12-04 12:48 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: qemu-trivial, Michael Tokarev, qemu-devel, Igor Mammedov,
Andreas Färber, Wanlong Gao
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2013-12-04 12:48 ` Eduardo Habkost
@ 2014-02-14 6:56 ` Alexey Kardashevskiy
2014-02-14 7:34 ` Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: Alexey Kardashevskiy @ 2014-02-14 6:56 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-trivial, Michael Tokarev, qemu-devel, Igor Mammedov,
Andreas Färber, Wanlong Gao
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse
2014-02-14 6:56 ` Alexey Kardashevskiy
@ 2014-02-14 7:34 ` Paolo Bonzini
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2014-02-14 7:34 UTC (permalink / raw)
To: Alexey Kardashevskiy, Eduardo Habkost
Cc: qemu-trivial, Michael Tokarev, qemu-devel, Igor Mammedov,
Andreas Färber, Wanlong Gao
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-02-14 7:35 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-25 3:39 [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse Alexey Kardashevskiy
2013-12-02 17:06 ` [Qemu-devel] [Qemu-trivial] " Michael Tokarev
2013-12-02 22:09 ` Andreas Färber
2013-12-02 23:03 ` Alexey Kardashevskiy
2013-12-03 9:00 ` Markus Armbruster
2013-12-03 13:30 ` Andreas Färber
2013-12-03 14:47 ` Eduardo Habkost
2013-12-04 5:50 ` Alexey Kardashevskiy
2013-12-04 12:48 ` Eduardo Habkost
2014-02-14 6:56 ` Alexey Kardashevskiy
2014-02-14 7:34 ` Paolo Bonzini
2013-12-03 10:44 ` Igor Mammedov
2013-12-03 14:33 ` Eduardo Habkost
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.