[PATCH 0/2] ppc64: do not use MachineClass::max_cpus to limit CPUs

[PATCH 0/2] ppc64: do not use MachineClass::max_cpus to limit CPUs

[Daniel Henrique Barboza]

Hello,

After having to change hardcoded values to launch a 2048 KVM
pSeries guests I decided to post these upstream because, at
least for me, the current max_cpus usage is lackluster for
pSeries. More info in patch 01.

Patch 02 is a trivial follow-up to increase the FDT size.

Daniel Henrique Barboza (2):
  spapr.c: do not use MachineClass::max_cpus to limit CPUs
  spapr.h: increase FDT_MAX_SIZE

 hw/ppc/spapr.c         | 11 ++++++++++-
 include/hw/ppc/spapr.h |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.30.2

[PATCH 1/2] spapr.c: do not use MachineClass::max_cpus to limit CPUs

[Daniel Henrique Barboza]

Up to this patch, 'max_cpus' value is hardcoded to 1024 (commit
6244bb7e5811). In theory this patch would simply bump it to 2048, since
it's the default NR_CPUS kernel setting for ppc64 servers nowadays, but
the whole mechanic of MachineClass:max_cpus is flawed for the pSeries
machine. The two supported accelerators, KVM and TCG, can live without
it.

TCG guests don't have a theoretical limit. The user must be free to
emulate as many CPUs as the hardware is capable of. And even if there
were a limit, max_cpus is not the proper way to report it since it's a
common value checked by SMP code in machine_smp_parse() for KVM as well.

For KVM guests, the proper way to limit KVM CPUs is by host
configuration via NR_CPUS, not a QEMU hardcoded value. There is no
technical reason for a pSeries QEMU guest to forcefully stay below
NR_CPUS.

This hardcoded value also disregard hosts that might have a lower
NR_CPUS limit, say 512. In this case, machine.c:machine_smp_parse() will
allow a 1024 value to pass, but then kvm_init() will complain about it
because it will exceed NR_CPUS:

Number of SMP cpus requested (1024) exceeds the maximum cpus supported
by KVM (512)

A better 'max_cpus' value would consider host settings, but
MachineClass::max_cpus is defined well before machine_init() and
kvm_init(). We can't check for KVM limits because it's too soon, so we
end up making a guess.

This patch makes MachineClass:max_cpus settings innocuous by setting it
to INT32_MAX. machine.c:machine_smp_parse() will not fail the
verification based on max_cpus, letting kvm_init() do the checking with
actual host settings. And TCG guests get to do whatever the hardware is
capable of emulating.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 73a06df3b1..d6a67da21f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4482,7 +4482,16 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->init = spapr_machine_init;
     mc->reset = spapr_machine_reset;
     mc->block_default_type = IF_SCSI;
-    mc->max_cpus = 1024;
+
+    /*
+     * Setting max_cpus to INT32_MAX. Both KVM and TCG max_cpus values
+     * should be limited by the host capability instead of hardcoded.
+     * max_cpus for KVM guests will be checked in kvm_init(), and TCG
+     * guests are welcome to have as many CPUs as the host are capable
+     * of emulate.
+     */
+    mc->max_cpus = INT32_MAX;
+
     mc->no_parallel = 1;
     mc->default_boot_order = "";
     mc->default_ram_size = 512 * MiB;
-- 
2.30.2

Re: [PATCH 1/2] spapr.c: do not use MachineClass::max_cpus to limit CPUs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3406 bytes --]

On Thu, Apr 08, 2021 at 05:40:48PM -0300, Daniel Henrique Barboza wrote:
> Up to this patch, 'max_cpus' value is hardcoded to 1024 (commit
> 6244bb7e5811). In theory this patch would simply bump it to 2048, since
> it's the default NR_CPUS kernel setting for ppc64 servers nowadays, but
> the whole mechanic of MachineClass:max_cpus is flawed for the pSeries
> machine. The two supported accelerators, KVM and TCG, can live without
> it.
> 
> TCG guests don't have a theoretical limit. The user must be free to
> emulate as many CPUs as the hardware is capable of. And even if there
> were a limit, max_cpus is not the proper way to report it since it's a
> common value checked by SMP code in machine_smp_parse() for KVM as well.
> 
> For KVM guests, the proper way to limit KVM CPUs is by host
> configuration via NR_CPUS, not a QEMU hardcoded value. There is no
> technical reason for a pSeries QEMU guest to forcefully stay below
> NR_CPUS.
> 
> This hardcoded value also disregard hosts that might have a lower
> NR_CPUS limit, say 512. In this case, machine.c:machine_smp_parse() will
> allow a 1024 value to pass, but then kvm_init() will complain about it
> because it will exceed NR_CPUS:
> 
> Number of SMP cpus requested (1024) exceeds the maximum cpus supported
> by KVM (512)
> 
> A better 'max_cpus' value would consider host settings, but
> MachineClass::max_cpus is defined well before machine_init() and
> kvm_init(). We can't check for KVM limits because it's too soon, so we
> end up making a guess.

Well.. it's not so much that we're guessing KVM limits.  I think
max_cpus in the generic code is more about hard CPU limits which are
part of the machine architecture itself.  You're right that that
doesn't really make sense for the paravirtual PAPR machine though.

> This patch makes MachineClass:max_cpus settings innocuous by setting it
> to INT32_MAX. machine.c:machine_smp_parse() will not fail the
> verification based on max_cpus, letting kvm_init() do the checking with
> actual host settings. And TCG guests get to do whatever the hardware is
> capable of emulating.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Applied to ppc-for-6.1.

> ---
>  hw/ppc/spapr.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 73a06df3b1..d6a67da21f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4482,7 +4482,16 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->init = spapr_machine_init;
>      mc->reset = spapr_machine_reset;
>      mc->block_default_type = IF_SCSI;
> -    mc->max_cpus = 1024;
> +
> +    /*
> +     * Setting max_cpus to INT32_MAX. Both KVM and TCG max_cpus values
> +     * should be limited by the host capability instead of hardcoded.
> +     * max_cpus for KVM guests will be checked in kvm_init(), and TCG
> +     * guests are welcome to have as many CPUs as the host are capable
> +     * of emulate.
> +     */
> +    mc->max_cpus = INT32_MAX;
> +
>      mc->no_parallel = 1;
>      mc->default_boot_order = "";
>      mc->default_ram_size = 512 * MiB;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] spapr.h: increase FDT_MAX_SIZE

[Daniel Henrique Barboza]

Certain SMP topologies stress, e.g. 1 thread/core, 2048 cores and
1 socket, stress the current maximum size of the pSeries FDT:

Calling ibm,client-architecture-support...qemu-system-ppc64: error
creating device tree: (fdt_setprop(fdt, offset,
"ibm,processor-segment-sizes", segs, sizeof(segs))): FDT_ERR_NOSPACE

2048 is the default NR_CPUS value for the pSeries kernel. It's expected
that users will want QEMU to be able to handle this kind of
configuration.

Bumping FDT_MAX_SIZE to 2MB is enough for these setups to be created.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 include/hw/ppc/spapr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index bf7cab7a2c..3deb382678 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -95,7 +95,7 @@ typedef enum {
 #define SPAPR_CAP_FIXED_CCD             0x03
 #define SPAPR_CAP_FIXED_NA              0x10 /* Lets leave a bit of a gap... */
 
-#define FDT_MAX_SIZE                    0x100000
+#define FDT_MAX_SIZE                    0x200000
 
 /*
  * NUMA related macros. MAX_DISTANCE_REF_POINTS was taken
-- 
2.30.2

Re: [PATCH 2/2] spapr.h: increase FDT_MAX_SIZE

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Thu, Apr 08, 2021 at 05:40:49PM -0300, Daniel Henrique Barboza wrote:
> Certain SMP topologies stress, e.g. 1 thread/core, 2048 cores and
> 1 socket, stress the current maximum size of the pSeries FDT:
> 
> Calling ibm,client-architecture-support...qemu-system-ppc64: error
> creating device tree: (fdt_setprop(fdt, offset,
> "ibm,processor-segment-sizes", segs, sizeof(segs))): FDT_ERR_NOSPACE
> 
> 2048 is the default NR_CPUS value for the pSeries kernel. It's expected
> that users will want QEMU to be able to handle this kind of
> configuration.
> 
> Bumping FDT_MAX_SIZE to 2MB is enough for these setups to be created.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Applied to ppc-for-6.1, thanks.

> ---
>  include/hw/ppc/spapr.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index bf7cab7a2c..3deb382678 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -95,7 +95,7 @@ typedef enum {
>  #define SPAPR_CAP_FIXED_CCD             0x03
>  #define SPAPR_CAP_FIXED_NA              0x10 /* Lets leave a bit of a gap... */
>  
> -#define FDT_MAX_SIZE                    0x100000
> +#define FDT_MAX_SIZE                    0x200000
>  
>  /*
>   * NUMA related macros. MAX_DISTANCE_REF_POINTS was taken

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] spapr.h: increase FDT_MAX_SIZE

[Cédric Le Goater]

On 4/8/21 10:40 PM, Daniel Henrique Barboza wrote:
> Certain SMP topologies stress, e.g. 1 thread/core, 2048 cores and
> 1 socket, stress the current maximum size of the pSeries FDT:
> 
> Calling ibm,client-architecture-support...qemu-system-ppc64: error
> creating device tree: (fdt_setprop(fdt, offset,
> "ibm,processor-segment-sizes", segs, sizeof(segs))): FDT_ERR_NOSPACE
> 
> 2048 is the default NR_CPUS value for the pSeries kernel. It's expected
> that users will want QEMU to be able to handle this kind of
> configuration.
> 
> Bumping FDT_MAX_SIZE to 2MB is enough for these setups to be created.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  include/hw/ppc/spapr.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index bf7cab7a2c..3deb382678 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -95,7 +95,7 @@ typedef enum {
>  #define SPAPR_CAP_FIXED_CCD             0x03
>  #define SPAPR_CAP_FIXED_NA              0x10 /* Lets leave a bit of a gap... */
>  
> -#define FDT_MAX_SIZE                    0x100000
> +#define FDT_MAX_SIZE                    0x200000
>  
>  /*
>   * NUMA related macros. MAX_DISTANCE_REF_POINTS was taken
> 

FYI,

On a very large system, I also had to bump the FDT_MAX_SIZE value in 
softmmu/device-tree.c because QEMU is parsing the host DT. 

Thanks,

C.